forgejo-mcp/openspec/changes/forgejo-action-code-review/battle-test.md

Battle Test — forgejo-action-code-review

Date: 2026-05-12 Targets: proposal.md, design.md, specs/action-code-review/spec.md Team: adversary (devils-advocate, opus), defender (proponent, opus)

Verdict

Patches applied; C4 spike resolved; C5 spike deferred. Proceed to tasks.md.

Of 9 critiques: 7 patched, 1 resolved by spike (C4), 1 deferred via spec split (C5 — Path A copy-paste is supported; Path B cross-repo uses: is recommended-when-supported and tracked in bd for follow-up).

The change was written against the dep change's design, not its shipped artifact. The /code-review skill drifted from its archived design before this action layered on top, and this change inherited the drift. The single most damaging finding (C1): the action invoked the skill with --comment to enable posting; the shipped skill posts by default and accepts --dry-run to suppress. Every action artifact referenced the wrong flag. C2 was similarly load-bearing: the D6 dedup design assumed the skill embeds head-SHA in review bodies — it does not. Both fixed by patches and a follow-up tasks.md entry to amend skill Step 12 body templates.

C4 spike (2026-05-13) validated D2 (BASE workflow file executes under pull_request_target on fork PRs; Forgejo's display label pull_request is cosmetic, not semantic) and D7 (ephemeral GITEA_TOKEN + permissions: pull-requests: write permits create_pull_review on fork PRs on Codeberg v11.x). Spike artifacts retained under openspec/changes/forgejo-action-code-review/spikes/c4-ephemeral-token-fork-pr/ for reproducibility.

Surviving Critiques

C1. --comment flag does not exist; posting is the skill's default

Severity: blocker Failure mode: workflow passes --comment to a skill that does not recognize it; or omits it and posts anyway. Either way the action ships against a non-existent flag contract. The proposal's "supports --comment flag to post findings" is fiction relative to the shipped .claude/commands/code-review.md. Hits: proposal.md "What Changes" bullet 2; design.md D6 "Tagging mechanism" paragraph; spec.md "Findings posted as a Forgejo PR review" requirement. Canonical conflict: .claude/commands/code-review.md:3 declares argument-hint: [PR#] [--dry-run]; line 12 reads "By default, findings are posted as a PR review with inline comments. Use --dry-run for terminal-only output." Falsifier: grep -n "dry-run\|--comment" .claude/commands/code-review.md — done; confirms inversion. Disposition: patch (concede-patch)

C2. D6 dedup tagging is fabricated

Severity: blocker Failure mode: D6 promises action-side dedup via "current head SHA tag in review body footer." Skill's body templates (Step 12 of .claude/commands/code-review.md) use fixed strings with no SHA and no placeholder. Without a contract-level mechanism, dedup either no-ops (every run posts a new review) or relies on soft prompt-injection via additional_instructions — unreliable for a mechanism the spec promises to honor. Hits: design.md D6 paragraph beginning "Tagging mechanism"; the change's "duplicate-review suppression" goal in design.md "Goals" list. Canonical conflict: skill design.md (archived dep change) marks dedup as future work; shipped skill provides no tagging surface. Falsifier: grep -n "sha\|SHA\|<!--" .claude/commands/code-review.md — done; no trailer mechanism present. Disposition: patch (concede-patch — choose Option A drop L2 or Option B lift dedup to spec contract + skill amendment)

C3. D2 protects filesystem but not LLM prompt context

Severity: major Failure mode: D2's "no PR-head checkout" claim collapses the runner-filesystem threat model but leaves the LLM prompt-injection vector wide open. The skill (Steps 7.2 and 8) loads PR-controlled diff and file content into Claude's context, and the in-session forgejo-mcp server has write-side tools (create_pull_review). Industry-weak "do not follow embedded instructions" defenses are mitigation, not prevention. D2 reads as if it solved the whole problem; it solved one half. Hits: design.md D2 "This collapses the standard pull_request_target threat model"; design.md Risks "additional_instructions injection vector" bullet (too narrow); spec.md "Required secrets" requirement. Canonical conflict: design honestly says "untrusted text input fed to Claude" remains; that line undersells what an injected agent can do with a tool-equipped Claude. Falsifier: code-read of .claude/commands/code-review.md Step 7-8 and the in-session MCP tool list — done; PR-controlled bytes do reach Claude's context. Disposition: patch (concede-patch — bound residual threat via FORGEJO_TOKEN scope; rename Risks bullet to "Prompt-injection vectors")

C4. Fork-PR ephemeral-token create_pull_review — RESOLVED

Severity: major (resolved) Failure mode (originally): D7 marked "verification owed" for whether Codeberg Forgejo v11.x honors pull-requests: write on ${{ secrets.GITEA_TOKEN }} for fork PRs. Spike result (2026-05-13): ran C4 spike against goern/c4-spike-target ← fork tinytalesshop/c4-spike-target. Ephemeral GITEA_TOKEN + permissions: pull-requests: write PERMITTED create_pull_review on the fork PR. Bot identity: forgejo-actions (review id 1350819 on PR #2, HTTP 200). Follow-up test: pushed a modified workflow to the fork's spike-pr branch with a distinctive body string; the BASE workflow body was posted, not the fork's modified body. Conclusion: Forgejo Actions honors pull_request_target semantics (BASE workflow file executes with secrets), even though the displayed event label is pull_request (cosmetic UI artifact). Both D2 and D7 validated. Disposition: defend / verified by spike. No design changes needed. Spike artifacts retained under openspec/changes/forgejo-action-code-review/spikes/c4-ephemeral-token-fork-pr/ for reproducibility.

C5. Cross-repo workflow_call semantics on Forgejo — DEFERRED

Severity: major Failure mode: D9 mandates uses: goern/forgejo-mcp/.forgejo/workflows/claude-code-review.yml@<tag> as distribution. Forgejo Actions' workflow_call cross-repo semantics, secrets: inherit behavior, and tag resolution are not GitHub-equivalent. Hits: design.md D9; spec.md "Reusable Forgejo Actions workflow definition" Scenarios. Canonical conflict: none directly; Forgejo Actions docs not loaded into repo. Falsifier (scaffolded but not executed): spike scaffolding lives at openspec/changes/forgejo-action-code-review/spikes/c5-cross-repo-workflow-call/. Library + consumer repos created on Codeberg (goern/c5-spike-lib, goern/c5-spike-consumer). Tag, secrets, and trigger commit not yet applied. Disposition: deferred to follow-up bd issue. Spec patch already applied (copy-paste vs. uses: paths split into two scenarios). D9 documents Path A (copy-paste) as the certain path and Path B (cross-repo uses:) as recommended-when-supported. Implementation can proceed on Path A; C5 spike result will determine whether Path B is documented as supported or marked experimental.

C6. max_turns trip kills posting; "submit partial review" unenforceable

Severity: major Failure mode: spec promises "the workflow SHALL still attempt to submit the partial review (if any findings exist) before exiting" when max_turns is reached. Step 12 (the post step) is the last skill pipeline step. --max-turns typically trips mid-pipeline (Steps 8-11). Claude Code has no "post on the way out" hook. Spec contract is unenforceable. Hits: spec.md "Cost cap via max-turns" Scenario "max_turns reached"; design.md D10 "max_turns exhausted" row. Canonical conflict: .claude/commands/code-review.md step ordering puts posting last; Claude Code CLI has no exit hook. Falsifier: test run with --max-turns 5 on a non-trivial PR — done by code-read of skill pipeline; trip occurs before Step 12. Disposition: patch (concede-patch — rewrite scenario to record max_turns_exhausted status in step summary, accept zero-finding outcome, recommend max_turns: 25 floor)

C7. --max-turns is soft cost cap; proposal's "$0.02-0.15" understates

Severity: major Failure mode: proposal claims --max-turns is "a hard cap" on cost; spec says "hard upper bound on Claude Code execution." --max-turns bounds main-loop turn count, not subagent token spend. Three parallel Opus subagents per main-loop turn can each consume large contexts. Worst-case cost on a large PR is closer to $0.50-$5.00 than $0.02-$0.15. Marketing copy must match implementation. Hits: proposal.md "Cost implications"; spec.md "Cost cap via max-turns"; design.md Risks "Cost runaway despite max_turns" (which already admits soft cap — contradicting spec). Canonical conflict: Claude Code's documented --max-turns semantics + dep change design's tiered subagent model. Falsifier: single test run on a 500-line PR with cost_usd reported via --output-format json — confirms or refutes range. Disposition: patch (concede-patch — rewrite to "soft cap on main-loop turns"; surface cost_usd in step summary; recalibrate proposal cost range)

C8. Action must pass PR# explicitly; base-only checkout breaks auto-detect

Severity: major Failure mode: skill Step 3 auto-detects PR via git branch --show-current when PR# not passed. With D2's base-only checkout, current branch is the base branch — auto-detect either fails or matches the wrong PR (e.g., a PR landing into base). Spec scenario says "against the PR identified by the workflow event context" without specifying mechanism. Hits: spec.md "Non-interactive Claude Code invocation" Scenario "/code-review skill is the entry point". Canonical conflict: .claude/commands/code-review.md Step 3 (auto-detect logic) + D2's base-only checkout decision. Falsifier: code-read of skill Step 3 + workflow checkout strategy — done; confirms mismatch. Disposition: patch (concede-patch — invocation prompt MUST be literal /code-review ${{ github.event.pull_request.number }}; add D11 "Skill invocation prompt shape")

C9. --allowedTools whitelist unspecified; Bash fallback inheritance has security implications

Severity: major Failure mode: skill's allowed-tools frontmatter includes Bash(forgejo-mcp --cli:*) as MCP fallback. Action's --allowedTools must either inherit (an injected agent can craft arbitrary --cli calls within FORGEJO_TOKEN scope) or drop (skill's MCP fallback silently dies). Either choice is load-bearing for security and reliability; design leaves it unspecified. Hits: proposal.md "Capabilities" (mentions --allowedTools as cost control with no detail); design.md (no decision present). Canonical conflict: .claude/commands/code-review.md frontmatter allowed-tools declaration. Falsifier: code-read of skill frontmatter — done; Bash MCP fallback is present. Disposition: patch (concede-patch — add D12 "--allowedTools whitelist: MCP-only, no Bash fallback"; add corresponding spec requirement)

Conceded Patches

Apply these before tasks.md is written and implementation begins. Numbers map to critiques above.

proposal.md

• "What Changes" bullet 2 (C1): replace invokes the /code-review skill in non-interactive (-p) mode with invokes /code-review <PR#> in non-interactive (-p) mode (skill posts by default; --dry-run is not passed).
• "Impact" → "Cost implications" (C7): replace Each PR review run costs ~$0.02-0.15 depending on diff size and model tier. --max-turns provides a hard cap. with Each PR review run costs ~$0.10-$5.00 depending on diff size, model tier, and subagent dispatch fan-out. --max-turns caps main-loop turns (soft cost cap); per-run cost_usd is surfaced in the step summary for monitoring; large PRs scale cost super-linearly with subagent count.
• "Impact" → "Capabilities" (C9): expand cost controls (--max-turns, --allowedTools) to cost controls (--max-turns, --allowedTools restricted to MCP review tools per design D12).

design.md

• D2 (C3): append sub-section "What D2 does not protect against" listing (1) prompt-injection via PR-controlled bytes reaching Claude's context, (2) write-side MCP tool invocation by an injected agent, (3) "do not follow embedded instructions" prompt-string defense as mitigation not prevention. Bound residual threat: "FORGEJO_TOKEN scope (PR read + PR-review write only on the target repo). An injected attacker can post a malicious review body or comment but cannot read other repos, modify code, or escalate beyond review write."
• D6 (C1 + C2): rewrite "Tagging mechanism" paragraph. Pick Option A (drop L2 dedup, document duplicate-review limitation matching dep design, remove "duplicate-review suppression" from Goals) or Option B (lift dedup to a spec.md "Dedup contract" requirement that binds the workflow to append <!-- forgejo-mcp-review: <head_sha> --> trailer, requires skill amendment via separate tasks.md entry). User must choose.
• D7 (C4): retain "Verification owed" — but mark explicitly stalemate pending spike. After spike, either remove note (ephemeral works) or flip default to PAT-required (ephemeral does not work for fork PRs).
• D9 (C5): lead with copy-paste as the supported distribution path; describe cross-repo uses: as recommended when supported by the target Forgejo version. List specific cross-repo concerns: secrets: inherit behavior, tag resolution, token grants. Mark uses: path as stalemate pending spike.
• D10 (C6): clarify "max_turns exhausted" row rationale — "trips most often mid-pipeline before Step 12, so partial-review-not-posted is the expected outcome. Surface max_turns_exhausted in step summary so consumers raise the limit. Recommended floor: 25." Add new failure row: | inconclusive run (max_turns trip before posting) | review | exit 0 | step summary records max_turns_exhausted with zero findings |.
• Risks → "additional_instructions injection vector" bullet (C3): rename to "Prompt-injection vectors". Expand to note every PR-controlled byte (diff, file contents, PR title/body) reaches Claude's context. Mitigation: scope FORGEJO_TOKEN to PR read + review-write on the single target repo, no org-wide scope.
• Risks → "Cost runaway despite max_turns" bullet (C7): strengthen — recommend max_turns: 20 ceiling and document worst-case cost range $0.50-$5.00 per large-PR review.
• New decision D11 (C8): "Skill invocation prompt shape" — invocation is the literal string /code-review ${{ github.event.pull_request.number }}. PR# from event context. No --dry-run. Skill's branch-based auto-detection bypassed because base-only checkout makes it unreliable.
• New decision D12 (C9): "--allowedTools whitelist: MCP-only, no Bash fallback" — enumerate exact allowed tools (mcp__codeberg__get_pull_request_by_index, mcp__codeberg__list_pull_request_files, mcp__codeberg__get_pull_request_diff, mcp__codeberg__get_file_content, mcp__codeberg__list_pull_reviews, mcp__codeberg__create_pull_review, Task). No Bash(...), no Read, no AskUserQuestion. Trade-off: brittleness (no fallback) for tighter blast-radius (no shell escape from prompt injection). Bounded further by FORGEJO_TOKEN scope.

spec.md

• "Findings posted as a Forgejo PR review" (C1): add sentence The workflow MUST invoke the skill without the --dry-run flag.
• "Required secrets" (C3): add scenario Scenario: FORGEJO_TOKEN broader than PR read + PR-review write → WHEN the workflow runs with a FORGEJO_TOKEN whose scope exceeds PR read + PR-review write on the target repository THEN preflight SHALL fail with a clear error.
• "Reusable Forgejo Actions workflow definition" (C5): split the existing scenario into two — Scenario: PR opened on a repository that copies the workflow file (the supported path) and Scenario: PR opened on a repository that references via uses: (gated on workflow_call support, marked uncertain until spike).
• "Cost cap via max-turns" (C6 + C7): rewrite first sentence to The workflow SHALL impose a soft upper bound on Claude Code's main-loop turn count via the --max-turns flag. The cap bounds the number of subagent dispatches, not the token spend within each subagent. Add requirement The workflow SHALL surface per-run cost_usd from --output-format json in the step summary. Rewrite Scenario "max_turns reached" to WHEN Claude Code consumes its configured max_turns budget without finishing organically THEN Claude Code SHALL terminate AND the workflow step SHALL exit zero AND the step summary SHALL record a max_turns_exhausted status with whatever findings were posted (which may be none). Drop the unenforceable "SHALL still attempt to submit."
• "Non-interactive Claude Code invocation" → Scenario "/code-review skill is the entry point" (C8): replace against the PR identified by the workflow event context with the prompt SHALL be the literal string "/code-review ${{ github.event.pull_request.number }}". The workflow MUST NOT rely on the skill's branch-based auto-detection.
• New requirement (C2 Option B only): "Dedup contract" — The workflow SHALL append a machine-parseable trailer of the form <!-- forgejo-mcp-review: <head_sha> --> to every posted review body. The workflow's pre-flight step SHALL parse trailers from list_pull_reviews results and skip if the current head SHA matches an existing bot review's trailer. Plus a tasks.md entry to amend skill Step 12 templates.
• New requirement (C9): "Allowed tools" — The workflow SHALL invoke Claude Code with --allowedTools restricted to the MCP tools enumerated in design D12. The list MUST NOT include any Bash(...) entries.

Future Work

• C5 spike is deferred to a separate bd issue. Path A (copy-paste) is the certain distribution; Path B (cross-repo uses:) is recommended-when-supported and gated on a future spike. Implementation may proceed on Path A without blocking.

Lead Recommendation

Status (2026-05-13): C4 resolved by spike. C5 deferred but documentation already split into Path A + Path B. C2 decision made (Option B: lift dedup to spec + skill amendment). C1, C3, C6, C7, C8, C9 patches applied.

Resolved before tasks.md:

1. ✅ C4 spike complete. Ephemeral GITEA_TOKEN works for create_pull_review on fork PRs on Codeberg Forgejo v11.x. D2's BASE-workflow-file guarantee verified (pull_request_target semantics honored despite displayed pull_request label).
2. C5 spike deferred. Repos created (goern/c5-spike-lib, goern/c5-spike-consumer); tag + secrets + trigger commit not yet applied. Tracked in bd. Implementation proceeds on Path A; Path B documented as conditional.
3. ✅ C2 decision: Option B chosen. Spec gained "Dedup contract" requirement; skill amendment for SHA trailer is a tasks.md entry (forward dependency on forgejo-code-review-skill follow-up).
4. ✅ Mechanical patches applied for C1, C3, C6, C7, C8, C9 to proposal.md, design.md, spec.md.

Change is now hardened enough for tasks.md with the following caveats: the C5 spike result may demote D9 Path B from "recommended" to "experimental" if cross-repo workflow_call semantics turn out unreliable on Codeberg Forgejo. That demotion is doc-only and does not block implementation.

Most embarrassing findings (recorded for future battle-tests): C1 (--comment vs --dry-run flag inversion) and C2 (fabricated dedup tagging mechanism). Both indicate the change set was written against the dep change's design rather than the shipped skill artifact. Future battle-tests should explicitly include "code-read the shipped artifact" as a pre-flight, not just "read the dep change's design."