ai-code-review v2: subagent split, embedded Python CLI, deterministic post-steps

[Andrey9kin]

Summary

Rewrite of the reusable ai-code-review workflow (OpenCode + Amazon Bedrock), plus hardening fixes from a follow-up code review of the rewrite.

Architecture (v2)

• Subagent split. The orchestrator (main model) reads one pre-assembled context file, picks 2–3 review dimensions from the diff (canonical: business_logic, security, performance; repo config can force extras), and spawns review subagents in parallel. One mechanical subagent runs on a cheap model (subagent_model, default nvidia.nemotron-super-3-120b): comment-categorizer, which classifies existing bot comments.
• Judgment up, execution down. Comment management is code, not an agent: the orchestrator writes one comment-plan.json (what to post/reply/resolve, honoring human "false positive"/"won't fix" feedback), and the CLI's apply-plan command executes it deterministically — validates every id against the categorizer output, posts with a reply → new-inline → summary-observations fallback so findings are never dropped, resolve-replies stale/fixed threads (skipping already-marked ones), deletes no_issues leftovers, and renders the summary comment from a fixed template (including a note pointing AI coding agents at the inline threads). This replaced a comment-lifecycle agent that was the slowest and least reliable phase.
• Timing report. Every CLI invocation logs UTC time + duration to .ai-review-work/timing.log; a final workflow step prints it with work-file mtimes so per-phase time inside the OpenCode step is visible.
• One embedded Python CLI (stdlib-only, shells out to gh/yq) replaces all bash pre-compute logic: config sanitization, per-file diff splitting, comment/thread pre-fetch, rules-file discovery (AGENTS.md/CLAUDE.md incl. per-dir), generated-only skip check, and the agent-facing comment operations.
• Deterministic post-steps. Thread resolution and summary dedup are plain Python steps, not agent decisions. The agent only posts a resolve-reply; the CLI tags it with a hidden <!-- ai-review-resolve --> marker and the post-step resolves marker-tagged threads via GraphQL — skipping any thread where a human commented after the marker, so disputes are never buried.
• Findings pipeline. Structured findings with dedup across dimensions, a strict severity filter, consolidation of repeated patterns, and a max_comments cap (default 10; overflow goes to the summary).
• Optional BORIS infra context. Setting boris_mcp_url (org-controlled caller input, never repo config) installs the bmcp CLI from a pinned, checksum-verified release (v0.3.0, which adds OpenCode harness support); review subagents get read-only live AWS context for IaC changes. Auth reuses the existing Bedrock OIDC role. bmcp install opencode --scope project writes ./BORIS.md and a managed block in ./AGENTS.md into the runner's worktree, so the orchestrator and every subagent pick up the live tool catalog through OpenCode project rules instead of a hand-written prompt blurb. It runs after precompute, so the managed block never leaks into the assembled review rules.

Behavior changes

• The auto-approve step ("approve PR when no unresolved bot threads") is removed — humans approve PRs.
• Dropped vestigial inputs: review_plugin, plugin_marketplace_repo, max_turns (was dead plumbing).
• Human feedback like "false positive" / "won't fix" on a thread is honored; the issue is not re-raised.
• Only bot comments that are genuine "no issues"/LGTM-style leftovers are cleaned up; other automation on the same GitHub App identity is never touched.

Hardening (from review of the rewrite)

• Diffs computed from the merge base to match pulls/files API patch semantics when the base branch has advanced.
• Null comment user fields (deleted/ghost accounts) tolerated everywhere.
• Config edge cases: scalar additional_dimensions, max_comments: 0, injection-shaped values.
• Comment-mutation failures exit nonzero; apply-plan falls back (reply → new inline comment → summary observations) instead of silently dropping a finding.
• review-context.md caps inlined diffs (20k/file, 250k total) with pointers to on-disk per-file diffs.
• Pinned, SHA256-verified release installs for both OpenCode and bmcp.

Validation

• Workflow YAML parses (reusable workflow, repo caller, adoption template).
• Embedded Python extracted and byte-compiled; 29 functional checks pass against the extracted CLI with mocked gh/yq/git (config sanitization, resolve-thread marker/dispute/null-author matrix, null-user pre-compute, context truncation, exit codes).
• apply-plan functionally tested against the extracted CLI with a mocked gh: id validation, fallback demotion, marker skipping, no_issues cleanup, template summary, exit codes.
• Exercised on this PR's own review runs; live findings so far: GitHub requires the App to hold Contents R/W for resolveReviewThread (documented in README), and the old lifecycle agent posted a literal-\n summary once (now impossible — the summary is code-rendered).

🤖 Generated with Claude Code

[fivexl-reviewer]

AI Code Review Summary

• Findings posted: 2 inline, 0 thread replies
• By dimension: business_logic: 2
• Threads marked resolved: 0

Additional observations

• Additional observations (lower severity): (1) Line 548: malformed post items are silently skipped without incrementing 'failed' count, causing silent data loss if agent generates invalid plan. (2) Line 370: git merge-base failure exits without graceful error handling for edge cases like unrelated histories or shallow checkouts. (3) Security: path validation in read_body() and line number validation could be strengthened for defense-in-depth, though the author correctly noted the CLI is not a security boundary.

Note for AI coding agents: detailed findings are posted as inline review comments on the diff — read the unresolved review threads (e.g. gh api repos/<owner>/<repo>/pulls/<pr>/comments) before making changes; this summary is only an overview.

This summary was automatically generated by the AI code review workflow.

[fivexl-reviewer]

cmd_upsert_summary has a logic bug: if summary_id exists and the PATCH request fails, the code falls through to CREATE at line 479, creating a duplicate summary comment instead of returning False. The CREATE should only execute when summary_id was empty (first-time summary), not on PATCH failure. Recommend: move the CREATE block inside an 'else' branch after the 'if summary_id:' block, or add 'return False' after logging a PATCH failure that had a non-empty summary_id.

[fivexl-reviewer]

THREADS_QUERY fetches only first 100 threads and 50 comments per thread. PRs with >100 threads or threads with >50 comments will have unresolved threads silently left behind. The warning at line 664 only triggers when exactly 100 threads are returned, but pagination could return 100, 200, etc. Consider implementing proper pagination or documenting this limitation.

[fivexl-reviewer]

cmd_upsert_summary has a logic bug: when summary_id exists and the PATCH request fails with a non-404 error (e.g., rate limit, network issue), the code falls through to line 488 creating a new comment, resulting in duplicate summaries. The 404 check at line 485 only prevents fallthrough for deleted comments. Should return False on any non-404 failure.