feat(review): capture reviewer-stage model usage as ADR-012 records

[mashkovd]

Part of #50. The issue stays open: every other DevLoop stage lives in mctl-agents, and the end-to-end pilot needs the usage ledger released and deployed.

What

The reviewer is the one DevLoop stage whose model call runs in this repo (claude-review.yml). This PR measures its model usage as ADR-012 usage records.

• Capture. After each reviewer attempt (primary, then fallback), the run's result entry becomes one usage record per model, shaped as the body of mctl-api's POST /api/v1/usage/records. Each record carries repo, PR, agent=claude-review and devloop_stage=reviewer, plus the token counters: input, output, cache read/write and web search. Each attempt is captured immediately, because the fallback overwrites the primary's output file.
• Dedup. Records are merged on the ledger's own dedupe key, keeping the first. A fallback that only re-read the primary's file is counted once; a fallback that really re-ran the model counts as a second charge.
• No cost is set. A subscription token has no provider-reported cost, and the CLI's own price has no mctl pricing version. The ledger prices records at ingest, which keeps provider-reported and calculated cost separate.
• No prompt or completion text. Only counters and status fields are read; a test plants text and asserts it never reaches a record.
• Never blocks a review. Every usage step is if: always() and continue-on-error: true, and sits before the existing fail-closed classification. An attempt with nothing readable is reported as not measured, never as zero.
• Output. The job summary and a model-usage-records artifact. Nothing is sent to the ledger yet: its write endpoint is admin-only, and an admin credential does not belong in all 18 caller repos.
• workflow-tests.yml runs the new tests in CI, because the reviewer cannot review its own workflow.

Tests

• 21 new tests in .github/tests/test_claude_review_usage.py. They extract the scripts from the workflow file and run them. All pass.
• The existing suites still pass: roadmap tests (267), events (3), and validate.py roadmap/epics.
• actionlint 1.7.7 with shellcheck is clean on the changed files.
• Mutation check: each of 12 guards was reverted and its test failed:
  • absent-vs-zero counters;
  • no cost;
  • the retry number kept out of the dedupe key;
  • keep-first on duplicates;
  • never failing the job;
  • unreadable output exits 0;
  • cancelled attempts captured;
  • no text copied;
  • only the last result entry;
  • no session means no record;
  • malformed counters;
  • publish before the fail-closed check.

Merge note

This PR edits claude-review.yml itself, so the reviewer cannot approve it and it needs a human review. After merge, the 18 caller repos need their pinned SHA bumped to pick it up.

Follow-ups (other repos)

• mctl-agents: usage capture in the investigator, implementer and shepherd paths, with Temporal/Argo, issue/PR and work-item correlation, plus replay tests and the pilot.
• mctl-api: release the ledger (4.51.0), a narrow write-only producer credential, and ingestion of reviewer artifacts.
• mctl-gitops: USAGE_DB_URL and USAGE_PRICING_CATALOG.
• The agy reviewer is not instrumented.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TpRNAyt63CpPb9xpQPBy2G

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @mashkovd's task in 5m 17s —— View job

---

Review: reviewer-stage model usage capture

• Read the pinned diff (8f0631e...648be54)
• Review claude-review.yml usage-capture steps
• Review workflow-tests.yml (permissions, triggers)
• Review .github/tests/test_claude_review_usage.py
• Post findings

---

Verdict: no P1/P2 findings — 4 P3 nits. Good to merge.

The design holds up under scrutiny. I traced each of the stated invariants through the actual jq/bash and they are real, not aspirational:

• No expression injection. Every new run: block is ${{ }}-free; the programs live in job-level env and are reached as shell variables (bash -c "$USAGE_CAPTURE_SH"). The one ${{ }} in the new steps is runner.temp in a with: input, which is not attacker-reachable. No new permissions: scope is requested, and upload-artifact uses the runtime token rather than GITHUB_TOKEN, so the 18 callers need no permission change.
• Absent ≠ zero really works: def count rejects non-numbers, negatives and non-integers, and with_entries(select(.value != null)) drops the key rather than defaulting it (claude-review.yml:407, :439).
• Short-circuit is load-bearing and correct — jq's and short-circuits, so "9" >= 0 is never evaluated in count.
• Precedence is right where it easily wouldn't be: ($pr | tonumber? // null | count) parses as $pr | ((tonumber?) // null) | count, and empty // null yields null, so an empty PR_NUMBER drops the field instead of erroring (:425).
• Step placement is the actual guarantee and it's asserted: capture-primary (:1046) sits between Claude review and the fallback, so the file is read before the fallback can overwrite it; publish (:1125) and upload (:1155) precede the fail-closed Classify reviewer outcome (:1172). The wiring tests assert these indices, which is the right way to test a workflow.
• git checkout . && git clean -fd in Reset git state before fallback cannot clobber the attempt files — they live under RUNNER_TEMP, outside the workspace.
• The model can't forge the input. --allowedTools grants no write tool, so claude-execution-output.json stays action-written; injected PR content cannot reach the artifact or the step summary.
• workflow-tests.yml matches the established house pattern exactly (permissions: {}, same pinned checkout SHA, same pip install) as events-validate.yml / roadmap-validate.yml.

The test file is unusually good: it extracts the programs from the shipped YAML rather than copying them, and RunnerSandbox.publish() invokes bash -e -c to match how Actions runs a run: block.

---

P3 — nice to have, non-blocking

1. The one unmeasured attempt that isn't annotated — .github/workflows/claude-review.yml:450

The stated invariant is "an attempt with nothing readable is reported as not measured, never as zero." One path escapes it. If the fallback genuinely runs (and is genuinely charged) but crashes before rewriting claude-execution-output.json, capture(1) re-reads the primary's result and writes a non-empty attempt-1.json — so no ::warning:: fires — and USAGE_MERGE_JQ then silently collapses it into the primary record. The merged output shows only retry_attempt: 0, with nothing saying the fallback's spend is unknown.

Capturing nothing is the right call (the counters really are unknowable), but this is precisely the scenario the fallback exists for, so it deserves the same annotation as the other two. A cheap fix in the publish step: warn when the merged record count is lower than the summed per-attempt count.

Fix this →

2. Run-level fields are duplicated across per-model records — .github/workflows/claude-review.yml:435

num_turns and duration_api_ms are properties of the run, but they are written into every per-model record. A run that used two models (say a sub-agent on a cheaper tier) yields two records each carrying the full duration_api_ms, so any ledger aggregate that sums those columns double-counts. The token counters are correctly per-model, so only these two are affected. Worth either documenting as a known consequence of the one-record-per-(run, model) shape, or emitting them on the first model only.

3. workflow-tests.yml path filter omits its own dependency file — .github/workflows/workflow-tests.yml:9

The filter covers claude-review.yml, the workflow itself and .github/tests/**, but not roadmap/requirements.txt, which the install step reads. A PyYAML bump that breaks yaml.safe_load on the workflow would land without these tests ever running. (The sibling validate workflows have the same gap, so this is a pre-existing convention rather than something this PR introduced.)

Fix this →

4. Fixed artifact name across 18 callers — .github/workflows/claude-review.yml:1159

name: model-usage-records is a constant. upload-artifact v4+ rejects a second upload under a name already used in the same workflow run, so a caller that invokes this reusable workflow from two jobs in one run would lose the second one. continue-on-error: true means that failure is quiet. Suffixing with ${{ github.job }} or the run attempt would make it collision-proof.

---

What I could not verify

• The test suite did not run here. python3 -m unittest and even jq --version were refused by this job's sandbox, so I reviewed the jq and bash by reading rather than by execution. My reasoning about jq semantics (and short-circuiting, // binding tighter than |, empty // null → null, to_entries preserving insertion order) is from the language definition, not from a run. The author reports all 21 passing plus a 12-guard mutation check, which is the right level of rigour for this.
• actions/upload-artifact@043fb46 = v7.0.1 is unconfirmed — network access was unavailable. It follows the repo's SHA-pinning convention; worth a glance before merge, since a bad pin would fail silently behind continue-on-error.
• I cannot submit a formal GitHub review (approve / request changes) — that is outside my permissions, and the gh api call was refused by the sandbox besides. This comment is the review. As the PR notes, claude-review.yml edits itself here, so a human approval is required regardless.
  · branch feat/50-devloop-instrumentation

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit 648be54.

Finding 1 (P2)

• File: .github/workflows/claude-review.yml (approx. line 1078)
• Anchor: Anchor: + if: always() && contains(fromJSON('["success","failure","cancelled"]'), steps.review2.outcome)
• Concrete Failure Scenario: Step Capture fallback reviewer model usage guards its execution with contains(fromJSON('["success","failure","cancelled"]'), steps.review2.outcome). However, the preceding step Claude review (fallback token) (lines 955–964 in the workflow) does not define id: review2. In GitHub Actions, referencing a step that lacks an id evaluates to null. Therefore, steps.review2.outcome is always null, causing contains(...) to evaluate to false. As a result, Capture fallback reviewer model usage will never run when a fallback review executes, and model usage/tokens spent on the fallback token will silently fail to be captured.
• Proposed Fix: Add id: review2 to the Claude review (fallback token) step in .github/workflows/claude-review.yml:

        - name: Claude review (fallback token)
          id: review2
          if: steps.review-check.outputs.failed == 'true' && steps.token-check.outputs.has_fallback == 'true'

---

Finding 2 (P2)

• File: .github/workflows/claude-review.yml (approx. line 460)
• Anchor: Anchor: + if ! jq --arg repo "$REPO" --arg pr "$PR_NUMBER" \
• Concrete Failure Scenario: USAGE_CAPTURE_SH invokes jq using environment variables $REPO and $PR_NUMBER, and Publish reviewer model usage interpolates $REPO and $PR_NUMBER into the step summary. GitHub Actions runners do not populate environment variables named REPO or PR_NUMBER by default (standard variables are GITHUB_REPOSITORY, etc.), and neither variable is defined in env: for the job or the individual capture/publish steps. When executed in GitHub Actions, $REPO and $PR_NUMBER evaluate to empty strings. In USAGE_RECORDS_JQ, target_repo: ($repo | text) and pr_number: ($pr | ... | count) evaluate to null and are stripped, resulting in records that lack repository and PR attribution in the usage ledger, while the job summary prints Stage reviewer, #..
• Proposed Fix: Define REPO and PR_NUMBER in jobs.review.env (or in the env: block of each usage step) using GitHub context expressions:

        REPO: ${{ github.repository }}
        PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }}

---

Finding 3 (P3)

• File: .github/tests/test_claude_review_usage.py (approx. line 385)
• Anchor: Anchor: + self.assertIn("steps.review2.outcome", fallback["if"])
• Concrete Failure Scenario: WorkflowWiringTest.test_captures_read_their_own_attempt checks that the string "steps.review2.outcome" is present in the if expression of Capture fallback reviewer model usage, but no test visible in this diff verifies that a step with id: review2 actually exists in the workflow. Because of this gap, the test suite passes even though the workflow references an undeclared step ID.
• Proposed Fix: In test_claude_review_usage.py, assert that the step named "Claude review (fallback token)" explicitly declares id: review2:

  self.assertEqual("review2", step("Claude review (fallback token)").get("id"))

[mashkovd]

Review round on 648be54:

agy, both P2s are false positives. Checked against the file at this head:

1. "The fallback step has no id: review2": it does, at claude-review.yml:1065 (- name: Claude review (fallback token) / id: review2).
2. "REPO and PR_NUMBER are undefined": both are in the review job's env at lines 308-309 (REPO: ${{ github.repository }}, PR_NUMBER: ${{ github.event.pull_request.number || github.event.issue.number }}), and the usage steps run in that job.

agy's P3 (assert review2 exists) is a reasonable cheap guard and is listed below.

Claude: no P1/P2, 4 P3s. All non-blocking and left as follow-ups:

1. Warn when the merged record count is lower than the per-attempt sum. This covers a fallback that ran but left the primary's file unchanged.
2. num_turns/duration_api_ms repeat on every per-model record. Emit them on the first model only, or document it.
3. workflow-tests.yml path filter: add roadmap/requirements.txt. The sibling validate workflows have the same gap.
4. Make the artifact name collision-proof for a caller that invokes the reviewer twice in one run.
5. agy P3: assert the step Claude review (fallback token) declares id: review2.

Merge gate: this PR edits claude-review.yml itself, so it needs a human review and merge. After merge, the 18 caller repos need their pinned SHA bumped.