Fix exit-code verification across completion paths - #44
Conversation
Session-Id: 01a037bb-4e8c-7c20-8962-621515f5e335
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/verification.ts">
<violation number="1" location="packages/core/src/verification.ts:232">
P2: When an `exit_code` value is empty or whitespace, `Number` converts it to `0`, so malformed configuration passes for clean exits. Reject blank values before numeric conversion.</violation>
</file>
<file name="packages/core/src/step-executor.ts">
<violation number="1" location="packages/core/src/step-executor.ts:551">
P2: When exit-code verification fails in the process-spawner path, `monitorStep` falls back to a generic failure and drops the spawn output and recorded `exitCode`. Preserve the last spawn result and `failed_verification` reason in the failure result so failure callbacks and persisted evidence retain the process result.</violation>
<violation number="2" location="packages/core/src/step-executor.ts:551">
P2: When a verifier returns `{ passed: false }` without throwing, the process-spawner path still accepts terminal success. Derive the status and error from `verificationResult`, as the injected-execution branch already does.</violation>
</file>
<file name="packages/core/src/runner.ts">
<violation number="1" location="packages/core/src/runner.ts:5793">
P2: API-backed agent steps with `verification: { type: 'exit_code', value: '0' }` always fail because the branch records code 0 only on `spawnResult`, not completion evidence. Pass `lastExitCode` here, which is assigned from that result before this verification runs.</violation>
<violation number="2" location="packages/core/src/runner.ts:6660">
P1: For supervised steps, `exit_code` verification can validate the owner’s exit instead of the specialist whose output it checks. Keep worker and owner exit codes separate, then pass the specialist/worker code to this verification.</violation>
</file>
<file name="packages/core/src/__tests__/completion-pipeline.test.ts">
<violation number="1" location="packages/core/src/__tests__/completion-pipeline.test.ts:110">
P3: mockSpawnExitCodes only feeds the pty handle (makeMockHandle), but the node:child_process `spawn` mock still emits `close` with a hardcoded 0 and never consumes from mockSpawnExitCodes. Any test in this file that drives exit_code verification through the child-spawn path would silently report 0 regardless of the configured code, and because WorkflowAgentHandle.exitCode reads `inner.exitCode`, the affected path depends on which mock the spawn goes through. Either wire the spawn mock to mockSpawnExitCodes or leave a comment stating that exit codes are only controllable for the pty path so future tests don't rely on a mechanism that doesn't apply.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ? this.runVerification(step.verification, specialistOutput, step.name, verificationTaskText, { | ||
| allowFailure: true, | ||
| completionMarkerFound: hasMarker, | ||
| exitCode: this.getStepCompletionEvidence(step.name)?.process.exitCode, |
There was a problem hiding this comment.
P1: For supervised steps, exit_code verification can validate the owner’s exit instead of the specialist whose output it checks. Keep worker and owner exit codes separate, then pass the specialist/worker code to this verification.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/runner.ts, line 6660:
<comment>For supervised steps, `exit_code` verification can validate the owner’s exit instead of the specialist whose output it checks. Keep worker and owner exit codes separate, then pass the specialist/worker code to this verification.</comment>
<file context>
@@ -6652,6 +6657,7 @@ export class WorkflowRunner {
? this.runVerification(step.verification, specialistOutput, step.name, verificationTaskText, {
allowFailure: true,
completionMarkerFound: hasMarker,
+ exitCode: this.getStepCompletionEvidence(step.name)?.process.exitCode,
})
: { passed: false };
</file context>
| // verification hook runs, so this check is currently an unconditional pass. | ||
| return true; | ||
| export function checkExitCode(expectedExitCode: string, actualExitCode?: number): boolean { | ||
| const expected = Number(expectedExitCode); |
There was a problem hiding this comment.
P2: When an exit_code value is empty or whitespace, Number converts it to 0, so malformed configuration passes for clean exits. Reject blank values before numeric conversion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/verification.ts, line 232:
<comment>When an `exit_code` value is empty or whitespace, `Number` converts it to `0`, so malformed configuration passes for clean exits. Reject blank values before numeric conversion.</comment>
<file context>
@@ -223,10 +228,9 @@ export function stripInjectedTaskEcho(output: string, injectedTaskText?: string)
- // verification hook runs, so this check is currently an unconditional pass.
- return true;
+export function checkExitCode(expectedExitCode: string, actualExitCode?: number): boolean {
+ const expected = Number(expectedExitCode);
+ return Number.isInteger(expected) && actualExitCode !== undefined && actualExitCode === expected;
}
</file context>
| const expected = Number(expectedExitCode); | |
| const normalizedExpected = String(expectedExitCode).trim(); | |
| const expected = normalizedExpected === '' ? Number.NaN : Number(normalizedExpected); |
| }; | ||
| } | ||
|
|
||
| const verificationResult = step.verification |
There was a problem hiding this comment.
P2: When exit-code verification fails in the process-spawner path, monitorStep falls back to a generic failure and drops the spawn output and recorded exitCode. Preserve the last spawn result and failed_verification reason in the failure result so failure callbacks and persisted evidence retain the process result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/step-executor.ts, line 551:
<comment>When exit-code verification fails in the process-spawner path, `monitorStep` falls back to a generic failure and drops the spawn output and recorded `exitCode`. Preserve the last spawn result and `failed_verification` reason in the failure result so failure callbacks and persisted evidence retain the process result.</comment>
<file context>
@@ -539,12 +548,19 @@ export class StepExecutor<TState extends StateLike = StateLike> {
};
}
+ const verificationResult = step.verification
+ ? this.runVerification(step.verification, output, step.name, undefined, {
+ exitCode: spawnResult.exitCode,
</file context>
| }; | ||
| } | ||
|
|
||
| const verificationResult = step.verification |
There was a problem hiding this comment.
P2: When a verifier returns { passed: false } without throwing, the process-spawner path still accepts terminal success. Derive the status and error from verificationResult, as the injected-execution branch already does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/step-executor.ts, line 551:
<comment>When a verifier returns `{ passed: false }` without throwing, the process-spawner path still accepts terminal success. Derive the status and error from `verificationResult`, as the injected-execution branch already does.</comment>
<file context>
@@ -539,12 +548,19 @@ export class StepExecutor<TState extends StateLike = StateLike> {
};
}
+ const verificationResult = step.verification
+ ? this.runVerification(step.verification, output, step.name, undefined, {
+ exitCode: spawnResult.exitCode,
</file context>
| step.name, | ||
| promptTaskText | ||
| promptTaskText, | ||
| { exitCode: this.getStepCompletionEvidence(step.name)?.process.exitCode } |
There was a problem hiding this comment.
P2: API-backed agent steps with verification: { type: 'exit_code', value: '0' } always fail because the branch records code 0 only on spawnResult, not completion evidence. Pass lastExitCode here, which is assigned from that result before this verification runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/runner.ts, line 5793:
<comment>API-backed agent steps with `verification: { type: 'exit_code', value: '0' }` always fail because the branch records code 0 only on `spawnResult`, not completion evidence. Pass `lastExitCode` here, which is assigned from that result before this verification runs.</comment>
<file context>
@@ -5785,7 +5789,8 @@ export class WorkflowRunner {
step.name,
- promptTaskText
+ promptTaskText,
+ { exitCode: this.getStepCompletionEvidence(step.name)?.process.exitCode }
);
completionReason = verificationResult.completionReason;
</file context>
| { exitCode: this.getStepCompletionEvidence(step.name)?.process.exitCode } | |
| { exitCode: lastExitCode } |
| name, | ||
| runtime: 'pty' as const, | ||
| exitCode: undefined as number | undefined, | ||
| exitCode: mockSpawnExitCodes.shift(), |
There was a problem hiding this comment.
P3: mockSpawnExitCodes only feeds the pty handle (makeMockHandle), but the node:child_process spawn mock still emits close with a hardcoded 0 and never consumes from mockSpawnExitCodes. Any test in this file that drives exit_code verification through the child-spawn path would silently report 0 regardless of the configured code, and because WorkflowAgentHandle.exitCode reads inner.exitCode, the affected path depends on which mock the spawn goes through. Either wire the spawn mock to mockSpawnExitCodes or leave a comment stating that exit codes are only controllable for the pty path so future tests don't rely on a mechanism that doesn't apply.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/__tests__/completion-pipeline.test.ts, line 110:
<comment>mockSpawnExitCodes only feeds the pty handle (makeMockHandle), but the node:child_process `spawn` mock still emits `close` with a hardcoded 0 and never consumes from mockSpawnExitCodes. Any test in this file that drives exit_code verification through the child-spawn path would silently report 0 regardless of the configured code, and because WorkflowAgentHandle.exitCode reads `inner.exitCode`, the affected path depends on which mock the spawn goes through. Either wire the spawn mock to mockSpawnExitCodes or leave a comment stating that exit codes are only controllable for the pty path so future tests don't rely on a mechanism that doesn't apply.</comment>
<file context>
@@ -106,7 +107,7 @@ function makeMockHandle(name: string) {
name,
runtime: 'pty' as const,
- exitCode: undefined as number | undefined,
+ exitCode: mockSpawnExitCodes.shift(),
exitSignal: undefined as string | undefined,
waitForExit: (ms?: number) => waitForExitFn(ms).then((reason) => ({ reason })),
</file context>
Better factoring than mine — one behaviour question before mergeI was fixing this same defect inside #39 when this appeared. This PR wins and I have backed mine out: #39 no longer touches One question, and it is the reason I am not just approving
export function checkExitCode(expectedExitCode: string, actualExitCode?: number): boolean {
const expected = Number(expectedExitCode);
return Number.isInteger(expected) && actualExitCode !== undefined && actualExitCode === expected;
}The old stub returned Concretely: a user with I traced the call sites on this branch — 6 of 7 in Three things would close it:
Failing closed is probably right. I just want it to be a decision with the blast radius named, not a default. Merge order#42 → #44 → #39. #39 is rebased onto this branch, suite green at 964, typecheck exit 0. |
3 participants
Summary
Execution path coverage
A green suite proves the paths that have tests, and nothing else.
Compatibility with #39
This branch is based on the CI gate branch and does not modify the in-flight PR #39 branch. When #39 adds terminalSuccessExitCodes, an exit such as 78 may be terminal for process control, but it cannot satisfy verification expecting 0. The three named regressions include that 78-versus-0 composition shape and are intended to remain valid when the branches meet.
Verification
Base is ci/pr-test-gate-0825 so pull-request CI covers this stack. Retarget to main after PR-A merges.