chore(harness): make /pr-review a true verification twin (adversarial Phase 6 + /second-opinion)

[thomasluizon]

Summary

Brings the orbit-api /pr-review copy to full behavioral parity with orbit-ui-mobile's.

The gap: the api copy had no adversarial verification — Critical/High findings went straight to the report with no independent refute pass, unlike the ui-mobile copy (which spawns a default-refuted skeptic per serious finding). This closes it.

What's in it

• Mirror .claude/skills/_shared/verification-protocol.md into orbit-api (body verbatim; ui-mobile is the source of truth, mirror note on top).
• Mirror the /second-opinion skill + helper — .mjs byte-identical to ui-mobile; SKILL.md verbatim + mirror note.
• Add the adversarial Phase 6 (Verify findings) to the api pr-review: skeptic refute pass → cross-model /second-opinion on Critical survivors → completeness pass → deferred ledger. Renumber Validate → Phase 7, Report → Phase 8. Load the shared protocol in Phase 2, add the coverage contract to Phase 3, add the Deferred + CONTESTED sections to the report.
• Lockstep note → "aligned twin" documenting the sanctioned differences: default repo (api#/ui# selector), subagent set (security-reviewer + contract-aligner here; parity/i18n/design are ui-only), and dotnet validate vs the ui /validate skill.
• .gitattributes eol=lf gate for skill .mjs.

CI behavior

opencode is absent in orbit-api CI (this copy's only runtime), so the /second-opinion step degrades to UNAVAILABLE and findings stand — identical to how the ui copy behaves in ui CI. The new adversarial skeptic pass itself does run in CI (Task subagents), so orbit-api PRs now get the same false-positive filtering ui-mobile PRs already had.

Paired with orbit-ui-mobile#440 (the /second-opinion skill).

🤖 Generated with Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Code Review: PR #315 (thomasluizon/orbit-api)

Scope: PR #315 — chore(harness): make /pr-review a true verification twin (adversarial Phase 6 + /second-opinion)
Recommendation: APPROVE

Prior reviews / threads

Checked via gh pr view --json reviews,comments: zero prior reviews, zero comments. Nothing to avoid re-flagging.

Summary

Pure harness/tooling change — no src/Orbit.* C# touched. Five files: a new shared verification-protocol.md, an updated pr-review/SKILL.md (adds adversarial Phase 6, renumbers Validate→7 and Report→8), a new /second-opinion skill (SKILL.md + second-opinion.mjs), and a .gitattributes LF rule for skill .mjs files. The one real piece of executable code (second-opinion.mjs) is defensive and well-built: spawnSync uses shell: true but the only variable argv content (--model) is regex-locked to /^[\w./:-]+$/ (no shell metacharacters possible), and the finding text goes through input: (piped to stdin, never part of the command line), so no injection path exists despite the shell flag. The script always exits 0 with parseable JSON and degrades to UNAVAILABLE on any failure (spawn error, timeout, unparseable output) — no throw, no hang.

Findings

Critical: None.

High: None.

Medium

[MEDIUM] CI workflow's hardcoded phase-number references go stale after this PR's renumbering
· dimension: Correctness (#1) — cross-file contract between SKILL.md and its CI caller
· location: .github/workflows/claude-review.yml:38, :40 (NOT touched by this PR)
· issue: PR #315 inserts a new "Phase 6 — Verify findings (adversarial)" into
  .claude/skills/pr-review/SKILL.md, pushing the old "Phase 6 — Validate" (dotnet
  build/test) to Phase 7, and old "Phase 7 — Report" to Phase 8. claude-review.yml still
  says "Skip the skill's Phase 6 (dotnet build / test)" (line 38) and "Skip the skill's
  own Phase 7 posting" (line 40) — both now point at the wrong phase by number.
· risk: Read literally, the workflow now asks the CI reviewer to skip the new adversarial
  pass (now Phase 6) and skip Validate (now Phase 7, mislabeled "posting"). An independent
  skeptic pass argued the practical risk is muted — the CI agent reads SKILL.md itself and
  the parentheticals ("dotnet build/test", "posting") disambiguate intent regardless of the
  stale number, and worst case is redundant build/test or a slightly less rigorous review,
  not data loss or an actual API contract break — hence Medium rather than Critical.
· fix: update claude-review.yml lines 38/40 to say "Phase 7" and "Phase 8" in the same PR,
  or better, stop referencing phases by ordinal in the workflow prompt and reference them
  by name ("skip the skill's Validate phase" / "skip the skill's Report-posting phase") so
  future SKILL.md renumbering can't silently desync the workflow again.
· reference: rubric.md dimension 1 (Correctness — data flow across every boundary the
  change crosses)

Low / Info: None posted (signal gate — no other concretely-actionable items survived).

Subagents

Agent	Verdict
security-reviewer	N/A — no src/ (orbit-api C#) code changed
contract-aligner	N/A — no DTO/Controller/packages/shared surface touched
skeptic (ad hoc, on the one Medium/would-be-Critical finding)	DOWNGRADE Critical→Medium, reasoning above

Dimension coverage (rubric walk)

Touched/verdicted: #1 Correctness (Medium finding above), #2 Dead/stale code (none — purely additive/edited prose), #4 Comment policy (clean — .mjs uses only JSDoc-style block comments, markdown files carry the sanctioned mirror/WHY notes), #12 Security (clean — see second-opinion.mjs analysis above).

N/A (surface not touched by this diff): #3 SOLID/clean-arch (no app-layer code), #5 No-workaround, #6 Type safety (plain JS, no TS/C# under review), #7 console.log (none present), #8 DESIGN.md/AI-slop (no apps/*), #9 Parity, #10 i18n (frontend-only, owned by ui-mobile side), #11 Contract drift/backward-compat (no DTO/Zod field changed), #13 Backend hard rules (no orbit-api C# src touched), #14 FEATURES.md parity (not a user-facing feature).

Deferred / not verifiable here

• The PR's claim that verification-protocol.md and second-opinion.mjs/SKILL.md are byte-identical/verbatim mirrors of the orbit-ui-mobile originals — not verifiable, sibling repo not checked out in this job.
• The second-opinion.mjs assumption about the shape of opencode run --format json's JSONL event stream (event.type === 'text', event.part.text) is untested against a real opencode binary (unavailable in this sandbox and absent from orbit-api CI by design). Worst case if wrong: every invocation returns UNAVAILABLE (safe, silent no-op) rather than a wrong verdict — so not reported as a finding, just flagged as unverified.
• Build/Unit Tests/SonarCloud — skipped per this job's CI adaptation (separate required checks); N/A anyway since no app code changed.

What's good

• Clean, minimal diff scoped exactly to what the PR body describes.
• second-opinion.mjs is defensively written: argv sanitized against the one real injection vector, graceful degradation on every failure mode (spawn error, timeout, malformed model output), always exits 0 with parseable JSON so it can never hang or crash a calling skill.
• Phase numbering inside SKILL.md itself is internally consistent (checked every Phase-N cross-reference within the file, including the CI-wrapper bullet and the Output section — all correctly updated to the new 0–8 sequence).
• .gitattributes LF rule is correctly globbed (.claude/skills/**/*.mjs) and matches the new file.

Recommendation

Approve as-is. The one Medium finding (workflow phase-number drift in claude-review.yml) is real and worth a fast-follow fix, but per the rubric's deterministic gate it does not block merge — Medium never forces NEEDS WORK.