-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: fail loud when a delegated Codex task run performs no work #617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import { | |
| runAppServerTurn | ||
| } from "./lib/codex.mjs"; | ||
| import { resolveClaudeSessionPath } from "./lib/claude-session-transfer.mjs"; | ||
| import { assessWorkEvidence, renderWorkEvidenceBanner } from "./lib/work-evidence.mjs"; | ||
| import { readStdinIfPiped } from "./lib/fs.mjs"; | ||
| import { collectReviewContext, ensureGitRepository, resolveReviewTarget } from "./lib/git.mjs"; | ||
| import { binaryAvailable, terminateProcessTree } from "./lib/process.mjs"; | ||
|
|
@@ -496,24 +497,43 @@ async function executeTaskRun(request) { | |
|
|
||
| const rawOutput = typeof result.finalMessage === "string" ? result.finalMessage : ""; | ||
| const failureMessage = result.error?.message ?? result.stderr ?? ""; | ||
| const rendered = renderTaskResult( | ||
| { | ||
| rawOutput, | ||
| failureMessage, | ||
| reasoningSummary: result.reasoningSummary | ||
| }, | ||
| { | ||
| title: taskMetadata.title, | ||
| jobId: request.jobId ?? null, | ||
| write: Boolean(request.write) | ||
| } | ||
| ); | ||
|
|
||
| // fleet#264: a completed turn (exitStatus 0) must not be reported as work done when the turn | ||
| // touched no files and ran no commands. Mirrors the grok bridge's fleet#254 fix; the codex | ||
| // app-server already collects fileChanges/commandExecutions per turn, so no extra telemetry | ||
| // extraction is needed here. | ||
| const workVerdict = assessWorkEvidence({ | ||
| write: Boolean(request.write), | ||
| touchedFiles: result.touchedFiles, | ||
| commandExecutions: result.commandExecutions, | ||
| text: rawOutput, | ||
| requireWork: request.requireWork !== false | ||
| }); | ||
|
|
||
| const rendered = | ||
| renderTaskResult( | ||
| { | ||
| rawOutput, | ||
| failureMessage, | ||
| reasoningSummary: result.reasoningSummary | ||
| }, | ||
| { | ||
| title: taskMetadata.title, | ||
| jobId: request.jobId ?? null, | ||
| write: Boolean(request.write) | ||
| } | ||
| ) + renderWorkEvidenceBanner(workVerdict); | ||
| const payload = { | ||
| status: result.status, | ||
| threadId: result.threadId, | ||
| rawOutput, | ||
|
Comment on lines
526
to
529
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For a background empty run whose final message claims success, the new banner exists only in Useful? React with 👍 / 👎. |
||
| touchedFiles: result.touchedFiles, | ||
| reasoningSummary: result.reasoningSummary | ||
| reasoningSummary: result.reasoningSummary, | ||
| workEvidence: workVerdict.evidence, | ||
| workVerdict: { | ||
| noWork: workVerdict.noWork, | ||
| reasons: workVerdict.reasons | ||
| } | ||
| }; | ||
|
|
||
| return { | ||
|
|
@@ -522,7 +542,10 @@ async function executeTaskRun(request) { | |
| turnId: result.turnId, | ||
| payload, | ||
| rendered, | ||
| summary: firstMeaningfulLine(rawOutput, firstMeaningfulLine(failureMessage, `${taskMetadata.title} finished.`)), | ||
| workVerdict, | ||
| summary: workVerdict.noWork | ||
| ? `EMPTY RUN (no work performed): ${firstMeaningfulLine(rawOutput, taskMetadata.title)}` | ||
| : firstMeaningfulLine(rawOutput, firstMeaningfulLine(failureMessage, `${taskMetadata.title} finished.`)), | ||
| jobTitle: taskMetadata.title, | ||
| jobClass: "task", | ||
| write: Boolean(request.write) | ||
|
|
@@ -601,15 +624,27 @@ function buildTaskJob(workspaceRoot, taskMetadata, write) { | |
| }); | ||
| } | ||
|
|
||
| function buildTaskRequest({ cwd, model, effort, prompt, write, resumeLast, jobId }) { | ||
| function buildTaskRequest({ | ||
| cwd, | ||
| model, | ||
| effort, | ||
| prompt, | ||
| write, | ||
| resumeLast, | ||
| jobId, | ||
| requireWork = true | ||
| }) { | ||
| return { | ||
| cwd, | ||
| model, | ||
| effort, | ||
| prompt, | ||
| write, | ||
| resumeLast, | ||
| jobId | ||
| jobId, | ||
| // fleet#264: persisted into the queued request so background task-worker runs enforce the | ||
| // same work-evidence gate as foreground runs. | ||
| requireWork | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -664,6 +699,10 @@ async function runForegroundCommand(job, runner, options = {}) { | |
| outputResult(options.json ? execution.payload : execution.rendered, options.json); | ||
| if (execution.exitStatus !== 0) { | ||
| process.exitCode = execution.exitStatus; | ||
| } else if (execution.workVerdict?.noWork) { | ||
| // fleet#264: an empty run must be visible to the shell that invoked codex-companion, not | ||
| // just in the job record. | ||
| process.exitCode = 3; | ||
|
Comment on lines
+702
to
+705
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For a foreground empty run, setting exit code 3 makes the companion's Bash invocation fail even though the diagnostic banner was written to stdout. The primary delegated path in Useful? React with 👍 / 👎. |
||
| } | ||
| return execution; | ||
| } | ||
|
|
@@ -762,7 +801,16 @@ async function handleReview(argv) { | |
| async function handleTask(argv) { | ||
| const { options, positionals } = parseCommandInput(argv, { | ||
| valueOptions: ["model", "effort", "cwd", "prompt-file"], | ||
| booleanOptions: ["json", "write", "resume-last", "resume", "fresh", "background"], | ||
| booleanOptions: [ | ||
| "json", | ||
| "write", | ||
| "resume-last", | ||
| "resume", | ||
| "fresh", | ||
| "background", | ||
| // fleet#264 work-evidence gate control. | ||
| "allow-no-work" | ||
| ], | ||
| aliasMap: { | ||
| m: "model" | ||
| } | ||
|
|
@@ -780,6 +828,7 @@ async function handleTask(argv) { | |
| throw new Error("Choose either --resume/--resume-last or --fresh."); | ||
| } | ||
| const write = Boolean(options.write); | ||
| const requireWork = !options["allow-no-work"]; | ||
| const taskMetadata = buildTaskRunMetadata({ | ||
| prompt, | ||
| resumeLast | ||
|
|
@@ -797,7 +846,8 @@ async function handleTask(argv) { | |
| prompt, | ||
| write, | ||
| resumeLast, | ||
| jobId: job.id | ||
| jobId: job.id, | ||
| requireWork | ||
| }); | ||
| const { payload } = enqueueBackgroundTask(cwd, job, request); | ||
| outputCommandResult(payload, renderQueuedTaskLaunch(payload), options.json); | ||
|
|
@@ -816,6 +866,7 @@ async function handleTask(argv) { | |
| write, | ||
| resumeLast, | ||
| jobId: job.id, | ||
| requireWork, | ||
| onProgress: progress | ||
| }), | ||
| { json: options.json } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| /** | ||
| * Work-evidence assessment for Codex task runs (fleet#264). | ||
| * | ||
| * The codex bridge previously treated `exitStatus === 0` (turn status "completed") as | ||
| * "the delegate did the work". A turn's exit status reports app-server/process health, never | ||
| * work completion: a turn in which the model described a plan and ended with no tool call | ||
| * exits/completes exactly like a turn that made the change. Tonight's live specimen (fleet#292, | ||
| * closed) is this shape end to end -- the REPORT claimed work and run_state said delivered, but | ||
| * the actual PR diff was empty. | ||
| * | ||
| * Unlike the Grok CLI bridge (fleet#254 / xai-org/grok-build-plugin-cc#16), the Codex app-server | ||
| * protocol already returns structured per-turn telemetry for every run -- `fileChanges` and | ||
| * `commandExecutions` thread items collected in `captureTurn` (scripts/lib/codex.mjs) -- so there | ||
| * is no JSON-envelope-parsing step to port. The signal is always available; this module only has | ||
| * to decide, from that already-collected telemetry, whether the turn shows real work. | ||
| */ | ||
|
|
||
| /** | ||
| * Decide whether a finished Codex task turn carries positive evidence that work happened. | ||
| * | ||
| * Two outcomes, deliberately distinct from "the process/turn exited cleanly": | ||
| * - `noWork: true` proof of no work for a `--write` run (no files touched, no commands run), | ||
| * or a run of any kind that returned no output text at all. Terminal status | ||
| * must be `failed`. | ||
| * - neither positive evidence of at least one tool round-trip (or, for a read-only | ||
| * run, at least some output text). | ||
| * | ||
| * A read-only (`write: false`) run is not expected to touch files or run shell commands -- it | ||
| * may legitimately just answer a question -- so the touched-file/command-count gate only applies | ||
| * when `write` is true. The empty-output check applies either way: a turn with zero output text | ||
| * and zero tool activity is not evidence of anything. | ||
| * | ||
| * @param {object} args | ||
| * @param {boolean} [args.write] whether this task run was invoked with `--write` | ||
| * @param {Array} [args.touchedFiles] touched-file list from the turn (codex.mjs `collectTouchedFiles`) | ||
| * @param {Array} [args.commandExecutions] command-execution thread items from the turn | ||
| * @param {string} [args.text] the turn's rendered/final output text | ||
| * @param {boolean} [args.requireWork] enforce the gate at all (default true) | ||
| */ | ||
| export function assessWorkEvidence({ | ||
| write = false, | ||
| touchedFiles = [], | ||
| commandExecutions = [], | ||
| text = "", | ||
| requireWork = true | ||
| } = {}) { | ||
| const evidence = { | ||
| write: Boolean(write), | ||
| touchedFileCount: Array.isArray(touchedFiles) ? touchedFiles.length : 0, | ||
| commandCount: Array.isArray(commandExecutions) ? commandExecutions.length : 0, | ||
| outputChars: typeof text === "string" ? text.trim().length : 0 | ||
| }; | ||
|
|
||
| if (!requireWork) { | ||
| return { noWork: false, reasons: [], evidence, enforced: false }; | ||
| } | ||
|
|
||
| const reasons = []; | ||
|
|
||
| if (evidence.write && evidence.touchedFileCount === 0 && evidence.commandCount === 0) { | ||
| reasons.push( | ||
| "This task was run with --write but the turn touched no files and executed no commands: it described the change instead of making it (fleet#292 shape)." | ||
| ); | ||
|
Comment on lines
+60
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a write-capable turn completes work through an Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| if (evidence.outputChars === 0 && evidence.commandCount === 0 && evidence.touchedFileCount === 0) { | ||
| reasons.push("The delegate returned no output text and made no tool calls."); | ||
| } | ||
|
|
||
| return { noWork: reasons.length > 0, reasons, evidence, enforced: true }; | ||
| } | ||
|
|
||
| /** | ||
| * Human-readable banner appended to the rendered result so a caller reading only the | ||
| * rendered text cannot miss an empty run. | ||
| */ | ||
| export function renderWorkEvidenceBanner(verdict) { | ||
| if (!verdict || !verdict.enforced || !verdict.noWork) { | ||
| return ""; | ||
| } | ||
| return [ | ||
| "", | ||
| "!! EMPTY RUN -- NO WORK PERFORMED (fleet#264 guard) !!", | ||
| ...verdict.reasons.map((reason) => `- ${reason}`), | ||
| "This run is marked FAILED. Do not treat its output, or any REPORT/run_state built from it, as evidence of delivered work.", | ||
| "" | ||
| ].join("\n"); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repository's normal fake app-server task returns a final message without emitting command or file-change items, so enabling this gate by default changes every existing
task --writeintegration run to exit 3. Runningnode --test --test-name-pattern='write task output focuses' tests/runtime.test.mjson this commit fails at the expected-zero exit assertion (3 !== 0), which means the checked-in test suite no longer passes. Update the fixture to emit positive work evidence for successful write scenarios, or explicitly disable the gate in tests that are not exercising it.Useful? React with 👍 / 👎.