fix(ce-code-review): collect matchable native terminal answers - #1660
Closed
tmchow wants to merge 2 commits into
Closed
fix(ce-code-review): collect matchable native terminal answers#1660tmchow wants to merge 2 commits into
tmchow wants to merge 2 commits into
Conversation
Pin the collection contract that #1654 needs: a host-native terminal answer that names the launched work is a terminal result, and a progress notification is not. Current prose still requires an ID-addressed blocking collector, so this test fails until the condition is restated. Co-authored-by: Trevin Chow <tmchow@users.noreply.github.com>
Restate reviewer and validator collection as matching a launch to a terminal result payload. A host-native terminal answer that names the launched work counts. A progress notification does not. An ID-addressed blocking collector remains sufficient, not required. Fixes #1654 Co-authored-by: Trevin Chow <tmchow@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
ce-code-reviewfailed closed on a Codex host that delivers reviewer finals as native terminal answers. The host returns a task name from spawn, then a later message that names that task and carries the result. It has no ID-addressed collector tool.The old rule required a collector that accepts the launch identifier, blocks until terminal, and returns the outcome. It also forbade waiting for notifications. Literal runs treated those terminal answers as notifications and interrupted the reviewers.
This restates the collection condition. A launch is collected when a terminal result payload can be matched to it. A host-native terminal answer that names the launched work is that payload. A progress notification is not. An ID-addressed blocking collector remains one sufficient path. The complete-roster barrier, output validation, peer cleanup, and fail-closed path stay.
Fixes #1654
Scope
skills/ce-code-review/SKILL.mdStage 4 collection gateskills/ce-code-review/references/dispatch-reviewers.mdreviewer collectionskills/ce-code-review/references/finish-review.mdvalidator collectiondocs/solutions/skill-design/anti-poll-scope-and-async-subagent-dispatch.mdrecorded contracttests/review-skill-contract.test.tsgreppable pinstests/skill-eval-cell/catalog.tsdecision cells for the matched-answer path and the progress-notification regressionOut of scope: live Codex session reproduction with those exact collaboration tools. Agent-slot cleanup remains in #1649.
Tradeoffs
The ID-addressed collector stays as a sufficient path rather than being deleted. #1523 still needs fail-closed when the host only emits progress. Naming Codex or FINAL_ANSWER in the skill would encode a case. The condition plus one subordinated success shape covers that host without a catalog.
Blast Radius
Orchestrators that already had an ID-addressed blocking collector keep the same path. Orchestrators whose host delivers matchable terminal answers can now collect instead of failing closed. The #1523 print-mode failure still fails closed.
ce-code-reviewSKILL.md stays under the 8KB Codex prompt bound with 707 bytes of headroom.Verification
progress or status notification is not a terminal resultmissing) and pass after the restatement.bun test tests/review-skill-contract.test.ts tests/skill-eval-cell/catalog.test.ts: 106 pass, 0 fail.bun run release:validatepassed (0 agents, 35 skills, 0 MCP servers).bun run testunder parallel load: 3899 pass, 1 skip, 4 fail. The four failures were 20s timeouts ince-code-review-mechanics,codex-dev, andce-work-cross-model-integration. None touch collection prose. Isolated reruns of those tests passed (52ms, 53ms, 1858ms).9f935794):pr-title,test,windows-native, and Cursor Security Agent all succeeded.ce-code-review/collect-matched-terminal-answer→COLLECTION: collectce-code-review/progress-notification-is-not-terminal→COLLECTION: fail-closedclaude,codex, andgrokare not on PATH here, sobun run test:skill-eval-packis skipped on this runner. Run those two rows on Claude and Codex before merge for the required cross-host eval.Security Disclosure
No security-relevant changes.
Agent Disclosure