Skip to content

Add centralized codex-code-review reusable workflow#17

Merged
ratley merged 5 commits into
masterfrom
use-reusable-codex-review
Apr 30, 2026
Merged

Add centralized codex-code-review reusable workflow#17
ratley merged 5 commits into
masterfrom
use-reusable-codex-review

Conversation

@ratley

@ratley ratley commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a thin caller that delegates the codex-code-review workflow to happycatlabs/codex-review-workflow. One central place owns the prompt, auth model, incremental-review state, and comment lifecycle.

Notes

  • CODEX_AUTH_JSON repo secret needs to be set before the workflow can run (passed through via secrets: inherit).
  • Optional: drop a REVIEW.md at the repo root for project-specific conventions; the workflow reads it at runtime.
  • See README at https://github.com/happycatlabs/codex-review-workflow for inputs and rationale.

ratley added 3 commits March 12, 2026 01:09
Adds a thin caller that delegates to happycatlabs/codex-review-workflow.
The actual prompt, auth model, and sticky-comment lifecycle live in
that repo. Update there to update every consumer at once.
@github-actions

Copy link
Copy Markdown

Review of PR #17 — Add centralized codex-code-review reusable workflow

This PR adds a reusable code-review workflow, a Codex clarification question flow (requestUserInputorca answer → resume), multi-agent prompt guidance, per-step review thinking level, Codex binary auto-resolution, skill body inlining, MCP diagnostics, and supporting docs/tests.

Findings

BUG: Cancelled run status overwritten to waiting_for_answer

src/agents/codex/session.ts:827

if (currentRun.overallStatus === "cancelled") {
  rejectUserInputRequest(request.requestId, `Run ${interactionContext.runId} was cancelled while waiting for input.`);
  await clearPendingQuestion(request.requestId, "waiting_for_answer");
  return;
}

When a run is cancelled while waiting for input, clearPendingQuestion is called with "waiting_for_answer" as the new overallStatus. This function (line 758–772) unconditionally writes the passed status to the store, overwriting the "cancelled" status back to "waiting_for_answer":

await interactionContext.store.updateRun(interactionContext.runId, {
  overallStatus,          // ← "waiting_for_answer" overwrites "cancelled"
  pendingQuestion: undefined,
});

Scenario: User runs orca cancel --run <id> while a run is in waiting_for_answer. The polling loop detects the cancellation, correctly rejects the Codex request, but then reverts the status to waiting_for_answer with no pendingQuestion. The run is now stuck — orca status shows it as waiting_for_answer with no question, orca answer can't submit anything useful, and the user must cancel again.

Fix: Either skip the overallStatus update when the run is already cancelled (just clear pendingQuestion), or pass "cancelled" as the status parameter in the cancellation branch.

@ratley ratley closed this Apr 30, 2026
@ratley ratley reopened this Apr 30, 2026
@github-actions

Copy link
Copy Markdown

Review

BUG: Cancelled run status overwritten with "waiting_for_answer" after cancellation is detected

src/agents/codex/session.ts:825-828

if (currentRun.overallStatus === "cancelled") {
  rejectUserInputRequest(request.requestId, `Run ${interactionContext.runId} was cancelled while waiting for input.`);
  await clearPendingQuestion(request.requestId, "waiting_for_answer");
  return;
}

When a run is cancelled while waiting for user input, the code correctly detects the cancellation and rejects the Codex request. However, it then calls clearPendingQuestion(request.requestId, "waiting_for_answer"), which unconditionally writes overallStatus: "waiting_for_answer" back to the store (line 768-771), overwriting the "cancelled" status.

Failure scenario: User cancels a run while it is in waiting_for_answer state. The cancellation is persisted to the store. The polling loop detects it and rejects the Codex request — correct. But clearPendingQuestion then overwrites overallStatus from "cancelled" back to "waiting_for_answer", making the run appear stuck instead of cancelled. Any downstream logic or UI checking run status will see "waiting_for_answer" instead of "cancelled".

Fix: Either skip the clearPendingQuestion call on cancellation (just clear the pendingQuestion field directly without touching overallStatus), or add a guard inside clearPendingQuestion to not overwrite a "cancelled" status.

@github-actions

Copy link
Copy Markdown

No issues found. The PR centralizes the Codex review workflow and adds Codex path resolution, multi-agent-aware prompting, question/answer handling, review effort config, and related docs/tests.

@github-actions

Copy link
Copy Markdown

Review

BUG: Cancelled run status overwritten to "waiting_for_answer" after cancellation

src/agents/codex/session.ts:825-828

When a run is cancelled while waiting for user input, the cancellation handler correctly detects the "cancelled" status and rejects the Codex request, but then calls clearPendingQuestion with "waiting_for_answer" as the target status:

if (currentRun.overallStatus === "cancelled") {
  rejectUserInputRequest(request.requestId, `Run ${interactionContext.runId} was cancelled while waiting for input.`);
  await clearPendingQuestion(request.requestId, "waiting_for_answer");  // <-- overwrites "cancelled"
  return;
}

clearPendingQuestion (line 758-771) re-reads the run, confirms the pending question matches, then unconditionally writes { overallStatus: "waiting_for_answer", pendingQuestion: undefined }. This overwrites the "cancelled" status back to "waiting_for_answer" with no pending question — effectively un-cancelling the run and leaving it in a state where it is marked as waiting for input that will never arrive.

Failure scenario: User runs orca cancel --run <id> while the run is blocked on a Codex requestUserInput prompt. The run briefly shows "cancelled" but is then silently reset to "waiting_for_answer" (with no question to answer), leaving a zombie run.

Similarly, the serverRequest:resolved handler at line 871 would overwrite a cancelled status to "running" via the same clearPendingQuestion mechanism.

Suggested fix: clearPendingQuestion should check whether the current status is "cancelled" and preserve it, or the cancellation path should only clear pendingQuestion without touching overallStatus.

@ratley ratley merged commit a25b58c into master Apr 30, 2026
1 of 3 checks passed
@ratley ratley deleted the use-reusable-codex-review branch April 30, 2026 21:02
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