Skip to content

Keep required review audits in the foreground - #132

Merged
gontzess merged 3 commits into
mainfrom
steve.gontzes/review-completion-fix
Sep 25, 2026
Merged

gontzess merged 3 commits into
mainfrom
steve.gontzes/review-completion-fix

Conversation

@gontzess

@gontzess gontzess commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A one-shot review can post its provisional summary, start background audit agents, then finish its main turn saying it is waiting for them. The SDK reports a successful turn, but the action has no later conversation turn to synthesize those results. The publication guard correctly rejects the still-provisional output.

An isolated diagnostic reproduced that sequence: two agents launched asynchronously with the background argument omitted, neither completed before the parent returned, and no final summary was written. Tool denials occurred but the agents recovered and continued reading; permission widening is not the demonstrated correction.

Change

Set CLAUDE_CODE_DISABLE_BACKGROUND_TASKS=1 for the Claude review step. Required subagent work returns in the foreground before the parent can finish. Subagents remain available; model, effort, prompts, tool allow-list, permissions and review limits are unchanged.

The provisional, freshness, head-binding and publication guards remain intact. No blind retry, prior-finding cap, raw public logging or approval fallback is added.

Reference: Claude Code background-task environment control.

Verification

  • Existing focused suite: 134 tests passed.
  • Independent source review confirmed the flag reaches the pinned SDK process.
  • Tiny pinned-runtime lifecycle probe passed: two foreground agents completed and returned their results before the parent terminal result; no backgrounded agents.
  • Full frozen-input replay passed with the same generated prompt, prior-finding input and source tree. Before: two background agents, zero completed before the parent ended. After: the same two general-purpose agents (background argument omitted) ran in the foreground, both returned before the parent terminal result, and the final non-provisional report plus commit-bound verdict were published.
  • Both before and after used the same model, effort configuration, tool allow-list and safety gates. Permission friction remained, but the corrected run recovered and completed. This is lifecycle/publication proof, not an assertion that every review finding is correct.
  • The latest replay tested exact PR head de1f60868b429bc43fbffff737afe55110433428, including the official release pin below. The 134-test count is local execution evidence.

The diagnostic proves the reproduced mechanism. Historical runs without retained raw SDK traces do not expose their exact internal tool calls.

Deployment note

The reusable workflow checks out job.workflow_sha. After merging, trigger a fresh review event so it uses the updated workflow revision; rerunning an older execution may retain the older revision. This PR does not modify consumer branches or rerun production reviews.

Official release pin

The action is now pinned to v1.0.234 at immutable commit 9171db3e57d6a3140a37ddc2ba92788584e0ead6, rather than the earlier untagged main commit. The annotated release tag resolves to that commit, which includes the previous pin and is three commits ahead.

This moves Claude Code / Agent SDK from 2.1.280 / 0.3.280 to 2.1.282 / 0.3.282. Background tasks remain disabled; the configured model, effort, prompts, permissions and review limits are unchanged. The upstream commits are the integration-test model fix and two CLI/SDK patch-version bumps.

The local 134-test suite passed again. Both live validation jobs installed Claude Code 2.1.282 and Agent SDK 0.3.282:

  • Tiny lifecycle probe: two agents omitted the background argument; both started in the foreground and completed before the parent terminal result.
  • Full frozen 89-finding replay: two general-purpose agents, zero background starts, two completions before parent termination; 26 parent turns. A final non-provisional report and exact-head-bound neutral COMMENT verdict were published, and publication reached completed. Baseline mode did not approve.
  • Remaining limitations: 11 permission denials occurred. The model saved a condensed final report but could not append its requested “Prompt for AI agents” section. Finding counts/severity varied from the previous runtime trial; this validates lifecycle and publication, not substantive equivalence of every finding. No permissions or publication guards were weakened.

The release validation is complete; the earlier 2.1.280 proof is retained as historical evidence, not substituted for the new-runtime check.

gontzess and others added 2 commits September 24, 2026 23:52
Disable background tasks for the one-shot SDK review so the parent receives audit results before ending its turn. Preserve tools, permissions and fail-closed publication.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@gontzess
gontzess marked this pull request as ready for review September 25, 2026 00:18
@gontzess
gontzess marked this pull request as draft September 25, 2026 12:16
Use the release commit for Claude Code 2.1.282 and Agent SDK 0.3.282 while retaining foreground-only review audits.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@gontzess
gontzess marked this pull request as ready for review September 25, 2026 12:41
@gontzess
gontzess merged commit 89d8440 into main Sep 25, 2026
3 checks passed
@gontzess
gontzess deleted the steve.gontzes/review-completion-fix branch September 25, 2026 13:01
gontzess added a commit that referenced this pull request Sep 28, 2026
## Why

Sending complete review summaries inside shell heredocs can exceed
Claude Code's command-analysis limit or trigger shell-safety approval,
leaving a shortened report without its AI-agent appendix. Agent mode
also stopped automatically supplying the PR description, making author
intent depend on a separate model-initiated query.

## Changes

- Reuse the existing upstream
`mcp__github_comment__update_claude_comment` tool; no custom MCP server
or new dependency.
- Before the model runs, validate repository/PR/checkout/live-head
identity, create a fresh provisional working comment and bind its ID
through `CLAUDE_COMMENT_ID`.
- Send provisional and final Markdown as tool data, retaining upstream
sanitization and secret redaction. GitHub's comment-size limit still
applies; the shell's command-length limit does not.
- Preserve the complete author-written PR description as `pr_body` from
the existing PR metadata request. Read title/description as untrusted
claims to verify against the diff, never as review instructions or
verdict policy. No extra API call or permission is needed.
- Preserve CI-owned completed reports, exact-head verdicts, finality
checks and completion-before-supersession. Ambiguous working-comment
creation fails closed rather than reusing another run's slot or blindly
retrying.

No model, effort, runtime pin, background-task policy or shell
permission expansion relative to #132. Existing broad `gh api`
capability remains; this is not credential isolation.

## Latest validation

- **138 reviewer tests passed locally.** Four preparation tests caught
13 targeted compiling mutations.
- Before/after context smoke reproduced the old description omission,
then verified complete 23,400-character Markdown, empty/null
descriptions, preservation through preparation and unchanged API-request
counts.
- **A real full 89-finding workflow replay passed on this exact PR
head.** The entire 1,354-character PR description matched GitHub and
appeared in the successful context-read output, independent of an
additional description/CI-status query.
- Native MCP delivered both summaries. The **14,416-character final
body**, including the AI-agent appendix, was retained intact in the
archived working comment. Foreground subagents completed, then CI
published the completed report and exact-head `CHANGES_REQUESTED`
verdict before superseding prior output. No shell summary writes or
weakened publication guards.
- Scoped source/security review found no blocker. These checks validate
input delivery, transport and publication—not the correctness of every
model finding or immunity to prompt injection.

---------

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
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.

2 participants