Skip to content

fix(rescue): await the delegated Codex result instead of returning a placeholder - #608

Open
petersimmons1972 wants to merge 1 commit into
openai:mainfrom
petersimmons1972:fix/rescue-await-delegated-result
Open

fix(rescue): await the delegated Codex result instead of returning a placeholder#608
petersimmons1972 wants to merge 1 commit into
openai:mainfrom
petersimmons1972:fix/rescue-await-delegated-result

Conversation

@petersimmons1972

Copy link
Copy Markdown

The bug

codex-rescue can exit with a placeholder instead of the delegated result.

The agent definition currently tells the subagent to choose foreground vs background for its single codex-companion.mjs task ... call, and to "prefer background execution" when the task looks complicated, open-ended, or long-running. When it takes that branch, the subagent returns as soon as the job is queued — so the caller receives something like "I'll wait for the background task…" or a bare run id, and never the actual Codex output. The work may complete fine; the result is simply lost.

The fix

Keep the subagent's sole Bash invocation in the foreground and wait for it, so it returns completed stdout.

This does not remove background execution. Backgrounding is already owned one level up: /rescue --background backgrounds the entire subagent. It is only the inner task call that must not be backgrounded — doing so is what lets the subagent exit early with a placeholder while the outer command believes it finished.

Three files:

  • plugins/codex/agents/codex-rescue.md — replace the foreground/background selection guidance with an unconditional foreground rule, and state why (the outer command owns backgrounding).
  • plugins/codex/skills/codex-cli-runtime/SKILL.md — same rule in the runtime skill, so the two documents cannot drift.
  • tests/commands.test.mjs — assert the new rule in both files, plus assert.doesNotMatch(agent, /prefer background execution/i) so the old guidance cannot silently return.

+8 / -4 across 3 files. Documentation and one test; no runtime code changes.

Verification

Merges cleanly onto main @ db52e28.

npm test was run on this branch and on a pristine clone of upstream main. The failures in tests/state.test.mjs are pre-existing and identical in both — that suite needs live Codex job state on the host ("No finished Codex jobs found for this repository yet", "Active jobs:"), which a clean environment does not have. This change neither causes nor fixes them. tests/commands.test.mjs, the suite this change actually touches, passes.

Why it matters beyond convenience

A subagent that returns a placeholder while reporting success is worse than one that fails: the caller records a completed delegation that never delivered. We hit exactly this downstream — a review task came back marked complete, exit 0, in ~7 seconds with zero tool calls, and was very nearly accepted as a genuine second opinion. Making the wait unconditional removes the branch where that can happen.

Happy to adjust the wording or split the test change if you'd prefer it separately.

Keep the rescue subagent's sole task invocation in the foreground so it returns completed stdout. Explicit background mode remains owned by the outer rescue command, which backgrounds the entire subagent.
@petersimmons1972
petersimmons1972 requested a review from a team August 6, 2026 11:10
@petersimmons1972

Copy link
Copy Markdown
Author

Downstream confirmation, in case it's useful.

I applied this patch to a local install of the plugin (v1.0.6) and exercised the rescue path directly against the codex-companion.mjs runtime.

The defect: the rescue subagent was allowed to choose background execution for its single Bash call to codex-companion.mjs task. When it chose background, the subagent returned before the task finished, so the caller received a placeholder rather than the delegated result. Backgrounding is already owned by the outer /rescue --background command, which backgrounds the entire subagent — so the inner call has no reason to background itself.

After the patch: a foreground rescue task returns completed stdout. I gave it a read-only task whose answer I computed independently beforehand (count the bats @test blocks in a specific file — 7), and the run returned the correct count and the test names instead of a placeholder.

Test suite on the patched install: 87/91 passing. The 4 failures (3 in tests/runtime.test.mjs, 1 in tests/state.test.mjs) reproduce identically on an unpatched checkout of the same commit, so they are pre-existing in my environment and unrelated to this change.

One separate observation while testing, noted only in case it is independently interesting and explicitly not something this PR claims to fix: in tracked-jobs.mjs, completion is derived as exitStatus === 0 ? "completed" : "failed". A run that does no useful work still exits 0, so it is recorded as completed with an empty touchedFiles. I can open a separate issue if that would be useful.

Happy to adjust this patch in whatever direction you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant