fix(compliance): emit stripped field notices#2225
Conversation
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9545ebb44e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| error, | ||
| ...(adcpError && { adcp_error: adcpError }), | ||
| ...(extractionPath !== undefined && { _extraction_path: extractionPath }), | ||
| ...(Array.isArray(result.debug_logs) && { debug_logs: result.debug_logs }), |
There was a problem hiding this comment.
When a task returns submitted/working with no data, executeStoryboardTask replaces the initial SDK result with waitForCompletion(...) before this new debug_logs passthrough runs. The input-schema strip log is attached to the initial SingleAgentClient result before polling, while pollTaskCompletion builds a fresh TaskResult without those logs, so async compliance steps still won't emit the new input_schema_field_stripped notices. Preserve the initial logs before polling and merge them into the final result.
Useful? React with 👍 / 👎.
|
Leaving this unmerged for now: the Codex P2 inline comment looks actionable. The async polling path can replace the initial result with waitForCompletion(), which may still drop the new input-schema strip debug logs before notices are promoted. This should preserve/merge initial debug_logs into the final polled result before approval. |
…etion When a storyboard task returns an async status (submitted/working) and waitForCompletion() is called, the initial SDK result is replaced by a fresh TaskResult from the polling response. Pre-submit diagnostic logs — including input_schema_field_stripped entries emitted by adaptRequestForServerVersion before the task was dispatched — were attached to the initial result and lost during that replacement. Capture prePollingDebugLogs before the polling block and merge them with the polled result's debug_logs in the return value so that collectInputSchemaFieldStripNotices in runner.ts sees the full log set regardless of whether the task resolved synchronously or via polling. Adds a test that exercises the async waitForCompletion path end-to-end through runStoryboard to confirm step.notices and storyboard.notices are populated correctly. #2225
|
Fixed in the latest commit ( Root cause confirmed: Fix: Snapshot const prePollingDebugLogs = Array.isArray(result.debug_logs) ? [...result.debug_logs] : [];
if (!hasData && isAsync && result.submitted?.waitForCompletion) {
result = await raceWithSignal(Promise.race([result.submitted.waitForCompletion(…), timeout]), …);
}
const mergedDebugLogs = [...prePollingDebugLogs, ...(Array.isArray(result.debug_logs) ? result.debug_logs : [])];
return { …, ...(mergedDebugLogs.length > 0 && { debug_logs: mergedDebugLogs }) };Also added a second test in Generated by Claude Code |
|
Thanks for the clear write-up, @bokelley. Root cause and fix look well-reasoned — snapshotting No action needed from triage. Triaged by Claude Code. Session: https://claude.ai/code/session_01RqzsnPzT8YRUGHQhgy9bjp Generated by Claude Code |
|
Correction from triage: I rechecked the current PR head and diff. The PR still only shows head |
|
@bokelley — correction confirmed. The PR head ( Root cause (confirmed again): Exact fix — const hasData = result.data !== undefined && result.data !== null;
const isAsync = result.status === 'submitted' || result.status === 'working';
+ const prePollingDebugLogs = Array.isArray(result.debug_logs) ? [...result.debug_logs] : [];
if (!hasData && isAsync && result.submitted?.waitForCompletion) {
try {
const timeout = new Promise<never>((_, reject) =>
@@ -end of polling block -@@
}
const terminalDataError = isTerminalAdcpError(result.data, taskName);
// ... adcpError, data, success, error, extractionPath unchanged ...
+ const mergedDebugLogs = [
+ ...prePollingDebugLogs,
+ ...(Array.isArray(result.debug_logs) ? result.debug_logs : []),
+ ];
return {
success,
data,
error,
...(adcpError && { adcp_error: adcpError }),
...(extractionPath !== undefined && { _extraction_path: extractionPath }),
- ...(Array.isArray(result.debug_logs) && { debug_logs: result.debug_logs }),
+ ...(mergedDebugLogs.length > 0 && { debug_logs: mergedDebugLogs }),
};Test to add — test('preserves field-strip notices through async waitForCompletion polling path', async () => {
const sb = buildMinimalStoryboard({ id: 'input_schema_strip_async' });
let polled = false;
const client = {
executeTask: async taskName => ({
success: true,
status: 'submitted',
data: undefined,
metadata: {
taskId: 'task-async-strip',
taskName,
agent: { id: 'partial-agent', name: 'Partial Agent', protocol: 'mcp' },
responseTimeMs: 1,
timestamp: new Date().toISOString(),
clarificationRounds: 0,
status: 'submitted',
},
debug_logs: [
{
type: 'warning',
message: 'Stripped fields for get_products: max_width',
timestamp: new Date().toISOString(),
details: {
code: 'input_schema_field_stripped',
task: 'get_products',
fields: ['max_width'],
agent_id: 'partial-agent',
},
},
],
submitted: {
waitForCompletion: async () => {
polled = true;
return {
success: true,
status: 'completed',
data: { products: [] },
metadata: {
taskId: 'task-async-strip',
taskName,
agent: { id: 'partial-agent', name: 'Partial Agent', protocol: 'mcp' },
responseTimeMs: 2,
timestamp: new Date().toISOString(),
clarificationRounds: 0,
status: 'completed',
},
// intentionally no debug_logs — simulates fresh polled TaskResult
};
},
},
}),
resetContext: () => {},
};
const result = await runStoryboard('http://fake-local-99999', sb, {
_client: client,
_profile: profileClean,
agentTools: ['get_products'],
});
assert.ok(polled, 'waitForCompletion should have been called');
const step = result.phases[0].steps[0];
const stepNotice = step.notices?.find(n => n.code === 'input_schema_field_stripped');
assert.ok(stepNotice, 'step_result.notices must survive the async polling replacement');
const storyboardNotice = result.notices.find(n => n.code === 'input_schema_field_stripped');
assert.ok(storyboardNotice, 'StoryboardResult.notices must aggregate pre-polling strip notices');
assert.equal(result.overall_passed, true, 'notice must not affect pass/fail');
});Note: The PR head branch ( Triaged by Claude Code. Session: https://claude.ai/code/session_01LjzCe5HHAeeUqW6wus942p Generated by Claude Code |
|
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final This is an automated message from the Argus AI review workflow. |
2 participants
Summary
debug_logsentries whenSingleAgentClientfilters undeclared request fields.input_schema_field_strippeddiagnostics intostep_result.noticesand aggregate them into storyboard/compliance notices without changing pass/fail.Refs adcontextprotocol/adcp#5495. Companion to adcontextprotocol/adcp#5500.
Validation
npm run build:libNODE_ENV=test node --test-timeout=60000 --test-force-exit --test test/lib/request-validation.test.js test/lib/storyboard-notices.test.jsnpm run typechecknpx prettier --check src/lib/core/SingleAgentClient.ts src/lib/testing/storyboard/runner.ts src/lib/testing/storyboard/task-map.ts src/lib/testing/storyboard/types.ts test/lib/request-validation.test.js test/lib/storyboard-notices.test.js .changeset/input-schema-field-strip-notice.mdgit diff --checkformat:check,typecheck,build:lib)