Skip to content

refactor: split graph_review_9000 into a local review core and a GitHub wrapper - #109

Merged
tylerdavis merged 9 commits into
mainfrom
worktree-review-core-split
Aug 6, 2026
Merged

refactor: split graph_review_9000 into a local review core and a GitHub wrapper#109
tylerdavis merged 9 commits into
mainfrom
worktree-review-core-split

Conversation

@tylerdavis

Copy link
Copy Markdown
Owner

What

Splits the PR reviewer into three composable plans plus a worktree-capable evidence-tool family, so the review engine can run locally (including over uncommitted work) while CI keeps the exact same entry point.

  • graph_review_core (new) — the eight-pass review engine, purely local. Inputs base/head/title/body/known_findings; head empty (the default) reviews the working tree against merge-base(base, HEAD). Finishes with an output: map (structured JSON findings + severity-ordered pass table); on an empty diff it exits success with the same contract.
  • graph_review_thread_audit (new) — the review-thread lifecycle (verify fixes at head, resolve/decline via gh_pr_thread_sync). No exit steps, so its result shape is always the output map.
  • graph_review_9000 (rewritten) — thin GitHub wrapper: meta → gate-only diff → thread readback → decide (audit only when prior findings exist; else an empty contract via reshape) → empty-diff exit → plan__graph_review_core → post new findings → summary comment. Same identifier, same CI command, same markers — existing threads stay keyed.
  • user__repo_changed_files/repo_diff/repo_file/repo_grep (new, .graph/tools/) — copies of the github pack's git tools with one addition: empty head/ref = the working tree. Distinct repo_* names avoid pack shadowing. Untracked files need git add -N first (documented).

Why

  • The core is meant to be called hook-like by an agent finalizing its worktree (graph plan run graph_review_core --input base=main), with findings actioned before the PR ever opens — CLAUDE.md's Worktrees section now says so.
  • First-run PRs skip the audit's review-sized prompt entirely (the decide gate), so a first review pays for the review alone.
  • Scout concurrency raised 4 → 6 (grouping caps at 6 groups).

Verification

  • All three plans load via graph plan show (static template validation).
  • Empty-range run exits 0 with the full empty contract on stdout, zero LLM calls.
  • Worktree-mode run over this very change set completed end to end (82 steps, scouts included) — and its one blocker finding (missing reason key parity between the core's two output paths) was fixed before this commit.
  • The wrapper's live verification is this PR's own CI run: no prior findings exist on a fresh PR, so this exercises the decide else-path + core + posting; the next push exercises the audit path.

🤖 Generated with Claude Code

…ub wrapper

graph_review_9000 is now a thin GitHub wrapper composing two new plans:
graph_review_core, the eight-pass review engine — purely local, runnable
over a worktree (empty head = working tree, via new user__repo_* exec
tools in .graph/tools/) and returning structured JSON — and
graph_review_thread_audit, the review-thread lifecycle, called through a
decide gate only when prior findings exist so first reviews skip the
audit's review-sized prompt entirely.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@mintlify

mintlify Bot commented Aug 5, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
Graph 🟢 Ready View Preview Aug 5, 2026, 10:20 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

Comment thread .graph/plans/graph_review_core.yaml
Comment thread .graph/plans/graph_review_9000.yaml
@graph-graph

graph-graph Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

graph_review_9000 · Splits the monolithic PR reviewer into a locally-runnable review-engine plan, a GitHub thread-lifecycle plan, and a thin GitHub wrapper, plus worktree-capable git tool copies, while keeping the same CI entry point and marker contract.

At a glance

Pass Area Status
2 Pipeline/Control-Step Invariants 🟡 Unverified / Intentional
5 Plan Composition/Model Routing 🟡 Unverified / Intentional
1 Stated Intent ✅ Pass
3 Template/Shape Engine ⚪ N/A
4 Rust/Async/Storage Footguns ⚪ N/A
6 Test Coverage ✅ Pass
7 Repo Conventions/Docs Parity ✅ Pass
8 CLI Surface/Copy ✅ Pass

🧵 0 new · 5 open · 1 resolved this run · 0 declined

🧵 Thread updates (1)
  • exit output shape mismatch (.graph/plans/graph_review_core.yaml:36) → resolved — graph_review_core's exit output now nests reviewed/reason alongside the full normal output contract (overall_alert_type, overall_framing, passes, has_blockers, blockers_count, has_unverified, unverified_count, has_na_or_pass, findings) with empty/false defaults, matching the shape of a normal output finish.
🟡 Needs verification / intentional (2)

Pipeline/Control-Step Invariants — The wrapper assumes plan__graph_review_core's own empty-diff exit never fires when called by the wrapper (E2's tiny gate and the core's own full gate share base/head/excludes); this is a documented, mitigated tradeoff but the mitigation is fragile if the plan is invoked with divergent inputs — already tracked as an open known finding.

Plan Composition/Model Routing — Plan-calls-plan depth (wrapper -> audit/core, cap 8) is respected on paper, but cross-plan output-contract lockstep (decide else-branch literals vs. audit plan's real output keys) relies on manual maintenance with only a code comment as a safeguard, not a test.

N/A · ✅ Pass

Stated Intent — Diff matches the stated split — three plans, worktree-capable tool family, same CI entry point and markers, concurrency raised 4→6 as described.

Template/Shape Engine — No changes to graph-core template/shape engine code.

Rust/Async/Storage Footguns — No Rust/async/storage/MCP code touched — only plan YAML and bash exec tools.

Test Coverage — Author states worktree-mode end-to-end verification and static plan-load checks; new exec tools are shell scripts with no repo test harness for plan YAML, consistent with existing conventions.

Repo Conventions/Docs Parity — CLAUDE.md, cookbook, and skill doc all updated in lockstep with the new plan/tool architecture.

CLI Surface/Copy — New CLAUDE.md worktree instructions and tool descriptions read clearly; no stdout/exit-code contract changes found.

tylerdavis and others added 3 commits August 5, 2026 17:10
…rules

Live calibration from PR #109's first run: a finding asserted two output
keys were missing from a 42KB plan file fetched under a 30KB budget — the
keys sat past the visible truncation marker. The finding bar gains two
corollaries (truncated evidence never supports an absence claim; a
documented, mitigated tradeoff is a pass-summary note, not a finding),
the audit is told a truncated file is insufficient evidence in either
direction, and the per-file fetch budget rises to 60KB so this repo's
largest reviewed files arrive whole.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…split

# Conflicts:
#	docs/cookbook/ci-checks.mdx
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread .graph/plans/graph_review_core.yaml
Its own reviewer flagged the omission — the other three repo_* tools
carry the git add -N caveat and repo_grep is bound by the same limit.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread .graph/tools/repo_changed_files.yaml
Live calibration round two: a finding shipped with 'probably acceptable'
and 'wasn't traceable from the diff alone' in its own explanation — the
bar already forbids that, but implicitly. The final gate now names the
hedge words and the rule: a finding that needs a hedge to be honest is a
pass-summary note, deleted from the findings array.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread .graph/plans/graph_review_core.yaml
Round three of live calibration: a finding shipped whose explanation
conceded 'it's correct behavior, not a defect' — the lexical hedge list
didn't cover a concession, so it slipped through as a likely bug asking
for a YAML comment. The gate now deletes findings that refute themselves
and floors comment-only suggestions at cleanup. Also takes that
finding's (reasonable) suggestion: the empty-ref-means-working-tree
contract E4/E4g lean on is now stated where they lean on it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread .graph/tools/repo_diff.yaml
tylerdavis and others added 2 commits August 5, 2026 21:41
…ation fails

Repo-local user__gh_pr_thread_sync override (pack version untouched;
upstreaming tracked by #111): resolve the thread FIRST and post the note
only on success — the pack tool's reply-then-resolve order re-posted an
identical reply every run while resolveReviewThread kept failing (three
copies on one PR #109 thread). A failed resolve now carries its error in
the entry's output instead of being discarded, and the wrapper's summary
comment renders any transition errors. Both review plans call the
override; shim-tested both mutation paths.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rounds 4-6 of live calibration: self-hedged and self-conceding findings
kept shipping under new phrasings no lexical gate anticipated ('probably
acceptable', 'not a defect', 'kept as unverified/cleanup-level'). Prose
rules lose that fight, so the bar is now structural — every finding
carries a schema-required verified_defect boolean, and a filter step
drops false entries before the output. Logic in steps, not instructions.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@tylerdavis
tylerdavis merged commit 01fee57 into main Aug 6, 2026
7 checks passed
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