Skip to content

fix(#1169): write a provisional review result before synthesis - #1918

Merged
ggallen merged 1 commit into
mainfrom
agent/1169-review-provisional-result
Nov 9, 2026
Merged

ggallen merged 1 commit into
mainfrom
agent/1169-review-provisional-result

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

This changes the review agent's instructions so a review run always leaves a schema-valid agent-result.json, even when the session ends early.

  • agents/review.md: as soon as the PR is identified, the orchestrator now writes a provisional failure result (reason: tool-failure). The final result overwrites it. The contextual-labels step now runs before the final write, and the text says outright that it never ends the run, with or without label_actions.
  • skills/pr-review/SKILL.md: step 1 points to the provisional write. Step 7 writes the result after the label step, even when there are no labels. The skill stays under its skillsaw context-budget ceiling (17,101 tokens; the ceiling is 17,111).
  • docs/review.md: explains the provisional result and adds a troubleshooting row for the Reason: tool-failure notice it produces.
  • scripts/pr-review-provisional-result-test.sh (added to make test): runs the documented jq template, checks the output against schemas/review-result.schema.json with validate-output-schema.sh, checks that a final result overwrites it, and pins the guidance wording.

Why

Before this change, agent-result.json was only written in the last pr-review step. Every early exit reported in the issue left no file behind: a crash before synthesis, a kill, or a clean stop right after issue-labels returned "No label_actions to emit". Validation then failed and the post-script was skipped, so nothing was posted on the PR. With this change, those runs post a "PR was NOT reviewed" failure notice instead of failing silently.

Design notes

Testing

  • bash scripts/pr-review-provisional-result-test.sh: passes. Also passing: pr-review-challenger-contract-test.sh, pr-review-remediation-test.sh, pr-review-dependency-scope-test.sh, post-review-test.sh, review-body-fallback-test.sh.
  • skillsaw 0.18.0 --strict: 0 errors.
  • pre-commit couldn't fetch its hook repos in the sandbox (TLS error), so I ran the hooks directly: the pre-commit-hooks 6.0.0 checks, shellcheck 0.10.0.1, hack/lint-agent-docs, eval/lint-release-cases.py, and gitleaks via scan-secrets.
  • make couldn't spawn /bin/sh in the sandbox, so I ran the test scripts above directly rather than through make test.

These paths are protected (agents/, skills/, scripts/), so a human needs to approve.

🤖 Generated with Claude Code


Closes #1169

Post-script verification

  • Branch is not main/master (agent/1169-review-provisional-result)
  • Secret scan passed (gitleaks — e41be7b41a17e17686425892a2f4968031f7a92b..HEAD)
  • PR body secret scan passed (gitleaks — no-git)
  • Final PR description secret scan passed (gitleaks — no-git)

The review orchestrator only wrote agent-result.json in the final
pr-review step, so any session that ended early (crash, kill, or a
clean stop after the issue-labels step with no label_actions) left no
file. Validation then failed and the post-script posted nothing, which
hid completed sub-agent findings from the PR author.

- agents/review.md: add a provisional `failure` result (reason
  tool-failure) written as soon as the PR is identified, overwritten by
  the final result; run the label step before the final write and state
  that it never ends the run.
- skills/pr-review/SKILL.md: point step 1 at the provisional write and
  make step 7 write the result after the label step even with no labels.
  Kept within the skillsaw context-budget ceiling.
- docs/review.md: describe the provisional result and add a
  troubleshooting row for the resulting tool-failure notice.
- scripts/pr-review-provisional-result-test.sh (wired into make test):
  runs the documented jq template, validates it against the review
  schema, checks a final result overwrites it, and pins the guidance.

The provisional result is a failure notice, not a partial review, per
the agent's "report failure rather than a partial review" constraint.
Runner-side diagnostics for sessions that end without output are out of
scope here (tracked in fullsend-ai/fullsend#7042).

Note: pre-commit could not fetch hook repos (TLS error in sandbox);
hooks were run directly instead: pre-commit-hooks 6.0.0 checks,
shellcheck 0.10.0.1, lint-agent-docs, lint-release-cases, scan-secrets
(gitleaks). make could not spawn /bin/sh in the sandbox, so the
pr-review, post-review and review-body-fallback test scripts and
skillsaw 0.18.0 --strict were run directly; all passed.

Closes #1169

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner November 9, 2026 04:13
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Nov 9, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 4:15 AM UTC · Completed 4:23 AM UTC

Commit: dd50e9e · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.81

@fullsend-ai-review fullsend-ai-review Bot added the risk/elevated PR risk: elevated label Nov 9, 2026
@fullsend-ai-review

Copy link
Copy Markdown

Risk Assessment: elevated (3/5)

Details

Touches multiple protected paths and CI configuration in core review orchestrator files with high git churn and regression history, modifying existing behavior without a feature flag despite moderate change size and well-scoped issue alignment.

@fullsend-ai-review

Copy link
Copy Markdown

Review

Findings

Medium

  • [protected-path] agents/review.md — This PR modifies protected paths: agents/review.md, skills/pr-review/SKILL.md, and scripts/pr-review-provisional-result-test.sh (new). The PR links issue Review orchestrator fails to synthesize sub-agent results into output file #1169 and its description explains the rationale (write a provisional failure result so early-terminated review sessions still leave a schema-valid agent-result.json), so the context is sufficient. Human approval is always required for protected-path changes, regardless of context.

Labels: The PR fixes a reliability bug in the review agent (issue #1169).

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment bug Something isn't working labels Nov 9, 2026
@ggallen
ggallen added this pull request to the merge queue Nov 9, 2026
Merged via the queue into main with commit 82226d2 Nov 9, 2026
103 of 112 checks passed
@ggallen
ggallen deleted the agent/1169-review-provisional-result branch November 9, 2026 16:32

This branch was previously deployed

1 inactive deployment
dev — dd50e9e0 Deployed Nov 9, 2026 by ggallen via behaviour #1835
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ok-to-test ready-for-review Triggers review agent dispatch requires-manual-review Review requires human judgment risk/elevated PR risk: elevated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Review orchestrator fails to synthesize sub-agent results into output file

1 participant