From 7208afb8828787d0ca22ae561ffa9d7a4a9ab02a Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 14:34:59 -0300 Subject: [PATCH 01/10] ORB-261: canonicalize the API pr-review contract --- .claude/skills/pr-review/SKILL.md | 420 ++++++------------- .claude/skills/pr-review/rubric.md | 620 +++++++++++------------------ 2 files changed, 355 insertions(+), 685 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index ba53df5d..8602707e 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -1,363 +1,181 @@ --- name: pr-review -description: Deep code review of a diff across both Orbit repos against one shared rubric, orchestrating the review subagents and a backward-compat guard. Use when the user asks to review a PR, file, folder, or staged changes in orbit-api or orbit-ui-mobile. Replaces /review and /security-review. -argument-hint: -context: fork +description: Capped two-round review of ONE PR diff against rubric.md, by a session that did not write the code. Use when asked to review a PR in orbit-ui-mobile or orbit-api. +argument-hint: --- # PR Review **Input**: $ARGUMENTS -Review a diff end-to-end against `rubric.md`, fold in the review subagents, guard -against changes that break already-shipped mobile clients, and produce one -severity-ranked report — posted to the PR when the scope is a PR. +Review one diff against `rubric.md` and emit `findings.json`. This review is built to +**terminate**, not to be exhaustive. Read the whole contract below before reading any code. -This skill subsumes the old `/review` and `/security-review` commands: it does -everything both did and adds the backward-compat guard and a single shared rubric. +## Why this shape - +PR #672 ran **9 local `/pr-review` rounds over 38 hours**. Verdict every time `NEEDS_WORK`. +**19 findings, 19 unique, zero repeats.** The fixer fixed everything; the reviewer found +brand-new issues every round on a 7,078-line diff. Termination required +"the reviewer finds nothing", which on that diff has probability about zero. The skill was +correctly implemented and mathematically unable to stop. -**Golden rule**: every finding is constructive and actionable — a clear fix, a file:line, -and the rule it traces to. Severity is about blast radius, not which dimension raised it. +A severity floor alone would not have saved it: every one of the 19 was High or Critical, and +there were no nitpicks. What was missing was a **round cap** and a **frozen finding list**. ---- - -## Phase 0 — Provenance & self-containment - -The review dimensions in `rubric.md` were adapted at authoring time from the -**code-review base on claudeskills.info** (https://claudeskills.info — the "code-review" -/ reviewing-AI-code base), then specialized to Orbit's own standards (the ten Code -Standards in root `CLAUDE.md`, the orbit-api hard rules, `eslint-rules/no-comments.cjs`, -`DESIGN.md`, and the folded-in `/security-review` categories), which are richer than any -generic base. The adapted result is committed in-repo at `rubric.md`. - -This skill is **self-contained**: it makes **no network call at run time** and has no -runtime marketplace dependency. It reads only local repo files and runs `gh` / `git` -against the project's own remotes. The provenance above is the single WHY-with-URL note -the standard allows; nothing here is fetched live. +Research backing: every mainstream reviewer converges on severity floors (Codex's GitHub surface +flags only P0/P1) and on verification gating before a finding is shown. Practitioner consensus on +round caps is 1-2 for CI automation, escalating to a human at the cap. Anthropic's own docs concede +that "a rule like after the first review, suppress new nits and post Important findings only stops +a one-line fix from reaching round seven on style alone." --- -## Phase 1 — Resolve scope - -Parse `$ARGUMENTS` into a review target and detect which repos it touches. - -| Input | Repo | Example | Action | -|---|---|---|---| -| Number `123` | api (default) | `#123` | `gh pr view 123 --repo thomasluizon/orbit-api` | -| `ui#123` or `orbit-ui-mobile#123` | ui-mobile | `ui#42` | `gh pr view 42 --repo thomasluizon/orbit-ui-mobile` | -| Full PR URL | parsed from URL | `https://github.com/thomasluizon/orbit-ui-mobile/pull/9` | use the URL's repo | -| File path | local repo | `src/Orbit.Application/Habits/Commands/CreateHabitCommand.cs` | review that single file | -| Folder path | local repo | `src/Orbit.Api/Controllers/` | review every source file under it | -| Blank | local | (none) | review staged changes; if none staged, review unstaged | - -**For a PR:** +## The termination contract -```bash -gh pr view {N} --repo {OWNER/REPO} --json number,title,body,author,baseRefName,headRefName,files,labels -gh pr diff {N} --repo {OWNER/REPO} -``` - -**For a file / folder:** use Glob with `**/*.cs` scoped to the target path. - -**For blank:** - -```bash -git diff --cached --name-only -git diff --cached -``` +Six rules. All six bind. None is advisory, and none is negotiable mid-review. -(If nothing is staged, fall back to `git diff`.) +**1. Freeze the ruleset before round 1.** The dimensions and severity definitions in `rubric.md` +are fixed at PR-open time and cannot move during the review. A bar you would like to raise +mid-review is a follow-up ticket against the rubric, never a new bar this PR has to clear. -Then classify the diff: **frontend** (`apps/`, `packages/`), **backend** -(`orbit-api/src/`), or **both**. The classification drives which dimensions are gated in -and which subagents fire in Phase 4. +**2. Cross-vendor reviewer, fresh session.** Normal: Claude Opus 5 at `high`. `--codex-only`: +Sol at `xhigh` in a **separate** session, and the run prints `DEGRADED: same-vendor review` in its +opening line and in the PR comment. **The invariant in both modes: the session that writes the code +is never the session that reviews it.** Same model is acceptable; same session is not. LLM judges +measurably favour their own family's output, and the bias extends across the whole vendor family +(arXiv 2603.04582, arXiv 2508.06709). The direction is corroborated; the magnitude in a real PR +loop is **unmeasured**. State it that honestly, and never claim the degraded mode is unbiased. ---- +**3. Classify every finding at report time as Blocking or Non-blocking.** Blocking means it +**breaks behaviour, security, or data integrity**. Everything else is **auto-filed as a follow-up +Linear ticket** and never fixed in this PR. This is the severity floor. It is applied once, at +report time, against the definition above, not renegotiated per round. -## Phase 2 — Load context +**4. Diff-only scope.** Read `gh pr diff`. You are reviewing a diff, not a repository. Open a +repository file only to resolve a symbol the diff itself cites. A defect reachable only by browsing +code the diff never touched is out of scope; if it matters, it is a ticket. -In parallel: +**5. Monotonic round 2.** Re-check **only** the frozen Blocking list, answering `CLOSED` or `OPEN` +per finding. New findings are forbidden, with exactly one mechanical carve-out: **any defect on a +line the fixer's own round-2 diff touched**. That line set is computed as `git diff .. +--unified=0` and handed to the reviewer as data it cannot widen. -- `C:\Users\thoma\Documents\Programming\Projects\orbit-api\CLAUDE.md` (root + the scoped - project `CLAUDE.md` for any touched `src/Orbit.*` project or `tests/`). -- `C:\Users\thoma\Documents\Programming\Projects\orbit-ui-mobile\CLAUDE.md` (root + - `packages/shared`) — only if the diff changes a DTO, endpoint, or contract surface the - web/mobile clients consume. -- **`.claude/skills/pr-review/rubric.md`** — the dimensions, severities, and finding - template this review walks. -- **`.claude/skills/_shared/verification-protocol.md`** — the shared reliability contract; - its Verify phase and Deferred ledger run below. +> The carve-out was widened during cross-model review. The original admitted a new finding only on +> the cited line of an existing finding, which let a fixer break something elsewhere in the same +> file unreported. GPT-5.6 Sol found the hole. -Understand intent: for a PR read the title, body, and linked issue; for a file -understand its role; for staged changes, what is in flight. +**6. Hard cap of 2 rounds, enforced in code. No round-3 path exists.** At the cap, hand to Thomas +with the OPEN findings listed. Do not re-review, do not request one more round, do not merge. --- -## Phase 3 — Walk the rubric - -Go dimension-by-dimension through `rubric.md` against the diff. For each, emit findings -in the rubric's finding template, tagged with a severity from the ladder. Honor the -gates: skip a dimension whose surface the diff never touches (mark N/A — do not invent -findings), and only run the UI dimension (DESIGN.md / AI-slop, #8) when `apps/*` UI -files changed, the backend hard rules (#13) only when `orbit-api` changed, and -FEATURES.md parity (#14) only when the diff changes the user-facing feature surface. - -The dimensions, in order: Correctness · Dead/stale code · SOLID/clean-arch · Comment -policy · No-workaround · Type safety · No `console.log` · DESIGN.md/AI-slop · -Parity · i18n · Contract drift + backward-compat · Security · Backend hard rules · -FEATURES.md parity. +## Reviewer environment -Focus on changed code, not pre-existing issues — unless a pre-existing issue is Critical. +The reviewer runs **from the MAIN CHECKOUT, never the worktree**, so it cannot load the PR's own +`AGENTS.md`. A reviewer that loads it is reading instructions written by the change under review. -**Coverage contract (verification protocol §1):** the diff's changed files are the binding -inventory — rank them worst-first (highest-blast-radius / most-churned files and the -trust-boundary + contract surfaces before stable leaves) so the riskiest code is reviewed -even under pressure, and every changed file ends with a verdict or in the Deferred ledger. -Nothing changed is silently skipped. +Concretely, before round 1: -Apply the rubric's **Signal gate**: post Critical/High and concretely-actionable Medium only — drop Low/Info nits and style preferences (manufacturing nits to avoid approving is a defect). The outcome is deterministic: **NEEDS WORK** iff any Critical/High finding survives, otherwise **APPROVE**. +- Confirm this session did not write any of the code in the diff. If it did, **stop**: the review + is invalid under rule 2. Fork-inherited context counts as the same session. +- `cwd` is the main checkout of the repo the PR targets. Never a worktree, never the fixer's tree. +- The only inputs are the diff, `rubric.md`, and the PR title/body/linked ticket for intent. --- -## Phase 4 — Orchestrate subagents +## Procedure -Delegate the specialist subagents, gated by what the diff touches. Pass each the list of -changed files. Fold every result back into the Phase 3 findings under the matching rubric -dimension. +### Resolve scope -**Block on them within this same turn.** Spawn the gated `security-reviewer` and -`contract-aligner` subagents, wait for every one to return, and fold its result in before -moving to Phase 5. This skill runs as a single subagent turn with **no** -background-completion wake-up, so a subagent still running when the turn ends strands the -review half-done and posts nothing. If you cannot block on one, run its check inline -yourself. - -| Subagent | Gate (fire when…) | Folds into rubric dimension | +| Input | Repo | Command | |---|---|---| -| `security-reviewer` | any `src/` code changed (i.e. every backend PR) | Security (#12, API side) | -| `contract-aligner` | a DTO, Controller route, or sibling `packages/shared` type / `endpoints.ts` changed | Contract drift (#11) | - -Launch both together when both gates fire. - -Parity (#9) and i18n (#10) are **frontend-only** dimensions owned by the `orbit-ui-mobile` -side of the review: `parity-checker` and `i18n-syncer` do not live in this repo and never -fire on an orbit-api diff. On a cross-repo PR reviewed from here, mark those dimensions -"not verifiable here" and let the orbit-ui-mobile review cover them. - ---- - -## Phase 5 — Backward-compat guard - -Answer one question: **does this diff rename or remove a field that an already-shipped -(old) mobile client still sends or reads?** Old Android builds run a frozen -`@orbit/shared` snapshot, so a server/shared rename is invisible to them — they keep the -old name and silently break. This leans on `contract-aligner`'s field comparison from -Phase 4 and adds the direction + add/remove judgment. - -1. From the diff, isolate hunks in `packages/shared/src/types/*.ts` (Zod - `z.object({...})` schemas) and in `orbit-api/**/DTOs/*.cs` (records / classes). -2. A **removed line** declaring a field (`fieldName: z.…` removed with no matching add), - OR a **renamed field** (one field removed + one added in the same schema, types - compatible), is a candidate. -3. Classify each candidate and tag per `rubric.md` dimension 11: - - Removed/renamed in a **response** shape → old readers get `undefined` → - **`⚠️ breaks old mobile clients` (Critical)**, unless already optional AND unused - (cite the grep). - - Removed/renamed in a **request** shape, or a field made **newly-required** → old - senders are rejected by validation → **`⚠️ breaks old mobile clients` (Critical)**. - - **Added optional** field → forward-compatible → **Info**. - - **Enum value removed** → old clients may still send it → flag. -4. In the fix, recommend the compatible alternative: keep-and-deprecate the old field, - accept both names server-side for a release, or gate behind the min-version gate. - When old-client reach is uncertain, downgrade to **High** with a "verify old-client - usage" note rather than over-claiming Critical. - -Scope is **field add/remove/rename in the reviewed diff**. Semantic/behavioral breaks -under an unchanged field name are caught by Correctness (#1) and the human reviewer — do -not over-claim completeness here. - ---- - -## Phase 6 — Verify findings (adversarial) - -Run `.claude/skills/_shared/verification-protocol.md` before validating — every finding -that will decide the outcome has to survive a challenge first. - -1. **Adversarial pass (§2).** For every **Critical / High** finding (including any - `⚠️ breaks old mobile clients`), spawn an independent skeptic subagent (3 concurrent) - whose only job is to *refute* it — read the cited `file:line` in full diff context and - argue it is a false positive (the path is unreachable, the value already validated, the - field actually still present or optional-and-unused with the grep to prove it, a - duplicate, the severity inflated). Default to refuted when uncertain. Drop or downgrade - anything the skeptic disproves — a false Critical that blocks a clean PR is as costly as - a missed one. The survivors decide the recommendation. -2. **Cross-model second opinion (§2, Critical and High survivors).** For each **Critical** - or **High** finding that survives step 1 (including any `⚠️ breaks old mobile clients`), - fire **`/second-opinion`**: pipe the finding dossier (title · severity · - `repo/path:line` · the claimed defect · the cited code hunk) to - `node .claude/skills/second-opinion/second-opinion.mjs` and apply the verdict table that - skill carries. Scope is **Critical and High**: the two decisive findings of the - 2026-07-28/29 run were both High, so a Critical-only scope would have skipped both. There - is no unattended mode to distinguish, because every review is this one local subagent and - no automated review path remains in either repository. - Never name a model here; `/second-opinion` owns which model answers. Two bindings are - this skill's own: a **DISAGREE** finding is tagged **`CONTESTED`**, records the other - model's `reasoning` beside Claude's, and keeps its severity for the human to resolve; - **UNAVAILABLE** (the second-opinion engine is absent, unauthenticated, capped, or - offline) leaves the finding exactly as step 1 left it, stated in one line. CONTESTED - never changes the deterministic recommendation, so a surviving Critical or High still - means NEEDS WORK. -3. **Completeness pass (§3).** One pass only — a diff is its own boundary, so no loop: ask - *"what changed file or hunk did I not give a verdict, what dimension did I mark N/A - without checking its surface?"* and close the gap before reporting. -4. **Deferred ledger (§4).** Every dimension marked N/A and every changed file not - verdicted goes into the report's **Deferred** line with a one-line reason — so "clean" - never hides "not looked at." +| `ui#123` | orbit-ui-mobile | `gh pr view 123 --repo thomasluizon/orbit-ui-mobile` | +| `api#123` | orbit-api | `gh pr view 123 --repo thomasluizon/orbit-api` | +| Full PR URL | parsed from the URL | use the URL's repo | ---- - -## Phase 7 — Validate - -Run the backend checks from the orbit-api root: +A bare number or blank scope is ambiguous across two repositories and must be refused. The caller +provides a repository-qualified selector or full PR URL; caller cwd never chooses the repository. ```bash -dotnet build -dotnet test +gh pr view {N} --repo {OWNER/REPO} --json number,title,body,baseRefName,headRefName,files,labels +gh pr diff {N} --repo {OWNER/REPO} > /pr-{N}.diff ``` -Record each result as PASS / FAIL with the error summary for the report's validation -table. For a file/folder scope with no working-tree changes, validation is N/A. - ---- - -## Phase 8 — Report - -Write the report, then post it to the PR when the scope is a PR. - -```bash -mkdir -p .claude/reviews -``` - -**Output path**: `.claude/reviews/{scope-name}-review.md` - -```markdown -# Code Review: {SCOPE} - -**Scope**: {PR #N in repo / file / folder / staged} -**Recommendation**: APPROVE / NEEDS WORK +Read `rubric.md` once, then classify the diff as **frontend** (`apps/`, `packages/`), **backend** +(`orbit-api/src/`), or **both**. That classification gates which rubric dimensions apply. -## Summary +### Round 1 -{2-3 sentences: what was reviewed and the overall assessment.} +1. Walk `rubric.md` dimension by dimension over the diff. Skip a dimension whose surface the diff + never touches and record it as N/A with the reason. Do not invent findings to fill a dimension. +2. Verify each candidate finding against the diff text before writing it down: quote the line you + are claiming about. A finding you cannot anchor to a diff line does not get reported. +3. Classify each survivor Blocking or Non-blocking by rule 3. +4. Write `findings.json`. **The list is now frozen.** +5. File every Non-blocking finding as a follow-up Linear ticket (one per finding, title = the + claim, body = file, line, and the rubric dimension). They are not fixed in this PR. +6. Zero Blocking findings means the review is over. Hand to Thomas. -## Findings +### Round 2 -### Critical -{findings in the rubric template, or "None". `⚠️ breaks old mobile clients` findings sort here first. -A finding a cross-model second opinion disputed carries a **`CONTESTED`** tag with both -verdicts inline, for example "Claude: Critical · second opinion: DISAGREE, {its reasoning}", -so the human sees the disagreement. It keeps its severity; the tag never downgrades it.} +1. Compute the fixer's line set: `git diff .. --unified=0`. Pass it as data. +2. For each frozen Blocking finding, answer `CLOSED` or `OPEN` with the line that settles it. +3. A new finding is admissible **only** if its line is in the round-2 line set from step 1, and it + is Blocking. Anything else is a follow-up ticket. +4. All `CLOSED` means hand to Thomas. Any `OPEN` means stop and hand to Thomas with the open list. + There is no round 3. -### High -{… or "None"} - -### Medium -{… or "None"} - -### Low / Info -{… or "None"} - -## Subagents - -| Agent | Verdict | -|---|---| -| security-reviewer | PASS / FAIL / N/A | -| contract-aligner | MATCH / DRIFT / NOT VERIFIABLE / N/A | - -## Validation - -| Check | Result | -|---|---| -| Build (dotnet) | PASS / FAIL / N/A | -| Tests (dotnet) | PASS / FAIL / N/A | - -## Deferred — N/A dimensions & files not verdicted +--- -{Per the verification protocol §4: each dimension marked N/A (with why its surface wasn't -touched) and any changed file not given a verdict — one line each. "Nothing deferred" if -every dimension and file got a verdict.} +## Output contract -## What's good +`findings.json` is one receipt object. Its `findings` array contains one object per finding: -{positive observations} +```json +{"reviewerKind":"independent","verdict":"BLOCKING","rounds":1, + "reviewedHeadOid":"","baseSha":"", + "artifactPath":"", + "findings":[{"id":"F1","severity":"High","file":"apps/web/hooks/use-streak.ts","line":42, + "claim":"one sentence: what is wrong and what goes wrong if it ships","blocking":true}]} +``` -## Recommendation +`severity` is descriptive and comes from the rubric's ladder. `blocking` is the decision, and it is +the answer to rule 3's question alone: a High that does not break behaviour, security, or data +integrity is `"blocking": false` and becomes a ticket. -{what needs to happen next} -``` +Round 2 rewrites the same receipt, sets `rounds` to 2, and adds +`"status": "CLOSED" | "OPEN"` to every round-1 Blocking finding. Round-1 entries are never removed. +Set `verdict` to `CLEAN` only when no Blocking finding remains OPEN. Capture `reviewedHeadOid` and +`baseSha` from the PR state reviewed; a review of any other head/base is stale by construction. -### Post to GitHub (PR scope only) +### Posting -The review is **decisive** — it ends as APPROVE or REQUEST_CHANGES, never a bare comment. -Map the deterministic recommendation (NEEDS WORK iff any Critical/High finding): +Post the report as a **PR comment**, then hand to Thomas: ```bash -# NEEDS WORK — any Critical/High (incl. ⚠️ old-client break) -gh pr review {N} --repo {OWNER/REPO} --request-changes --body-file .claude/reviews/{scope-name}-review.md -# APPROVE — no Critical/High -gh pr review {N} --repo {OWNER/REPO} --approve --body-file .claude/reviews/{scope-name}-review.md +gh pr comment {N} --repo {OWNER/REPO} --body-file /review-{N}.md ``` -Inline comments (Critical/High, tied to a specific line) via the PR review-comments -endpoint / `mcp__github_inline_comment__create_inline_comment`. - -**Caller context decides who posts:** - -- **Orchestrator-side subagent**: `/orchestrate` runs this skill in a FRESH worktree at the - pull request head, as a subagent and never in the main session. Post the decisive review - yourself per the recommendation, and mark any dimension needing a repository that is not - checked out beside this one as "not verifiable here" rather than guessing. -- **Local, a PR you do NOT own**: post the decisive review yourself per the recommendation. -- **Local, your OWN PR** (GitHub blocks self-approval): write the report and post it with - `--comment` instead, and never fail trying to `--approve`. -- **Local file / folder / staged** scope: only write the report file, never post. - -**There is no review status check in either repository** and -`required_approving_review_count` is **0** on both `main` branches. `claude` was the only -account that ever posted an approving review, GitHub forbids a pull request author -approving their own pull request, and leaving the count at 1 with no producer would have -made every merge an admin merge, which the conventions forbid an agent from performing. -The blocking path is the deterministic `guards.yml` required contexts plus this local -`/pr-review`, run by a session that did not write the code. So read this review's verdict from the -report body, never from the GitHub review state: a `--comment` post reads neutral even -when the verdict is NEEDS WORK. +Never `--approve` and never `--request-changes`. `required_approving_review_count` is **0** on both +`main` branches, GitHub forbids a PR author approving their own PR, and no review status check +exists in either repository, so a GitHub review state gates nothing and reads misleadingly. The +verdict lives in `findings.json` and the comment body. ---- +The comment body is: the verdict line (plus `DEGRADED: same-vendor review` when applicable), the +Blocking findings with file and line, the follow-up tickets filed, and the dimensions marked N/A +with why. **A machine never merges.** -## Output +--- -```markdown -## Review Complete +## The risk this design accepts -**Scope**: {what was reviewed} -**Recommendation**: APPROVE / NEEDS WORK +A frozen finding list can bury a defect the fixer introduced in round 2. Three nets catch it, and +**none of them is the reviewer**: -| Severity | Count | -|---|---| -| Critical (incl. ⚠️ old-client breaks) | {N} | -| High | {N} | -| Medium | {N} | -| Low / Info | {N} | +1. The mechanical carve-out on the round-2 diff line set (rule 5). +2. The 18 required CI checks, which run on the fixer's commit independently of any reviewer verdict. +3. Thomas reads the PR with exact advisory file/line counts and all required generated artifacts + attached to their source change. -**Report**: `.claude/reviews/{scope-name}-review.md` -{Posted to PR #N — only if scope was a PR} -``` +This is a deliberate trade: a review that stops and hands a human a bounded review beats one that +is still finding true defects on hour 38. diff --git a/.claude/skills/pr-review/rubric.md b/.claude/skills/pr-review/rubric.md index c8935985..406e48f6 100644 --- a/.claude/skills/pr-review/rubric.md +++ b/.claude/skills/pr-review/rubric.md @@ -1,405 +1,257 @@ # Orbit Review Rubric -The single source of truth for what a review checks **in this repo**. Exactly one thing -reads it: `/pr-review` (`.claude/skills/pr-review/SKILL.md`), which walks it -dimension-by-dimension over a **diff**, orchestrator-side in a fresh worktree at the pull -request head. There is no CI reviewer in either repository. Unlike -orbit-ui-mobile, this repo ships no `/audit-code-quality` skill, so nothing here walks the -rubric over the whole repo. - -**A TWIN does exist**, at `orbit-ui-mobile/.claude/skills/pr-review/rubric.md`, -where `/audit-code-quality` also walks it. Two repos mean the file cannot be deduped, so the -two are mirrored **by hand**. Only a careful author keeps the pair honest now. -Intentional divergences run in both directions: backend-only -material here, such as dimension 13's transaction-teardown bullet (`ORBIT0002`), and -orbit-ui-mobile-only material there, such as dimension 15's harness-execution evidence, -which has no counterpart in this repository because the harness runner -`tools/test-tools.mjs` and the per-tool case modules it loads from `tools/__tests__/` exist -only in orbit-ui-mobile. Change a dimension here and mirror it there in the same task; a -drift between the two is a defect, not a variant. - -It is command-agnostic on purpose: it contains **dimensions, severities, and templates**, -no orchestration, no scope resolution, no GitHub mechanics. Those live in the consuming -skill. - -Every finding cites the rule it came from (a `CLAUDE.md` rule number, `no-comments.cjs`, -a `DESIGN.md` section, an orbit-api hard rule, or a security category) so the author can -trace it back. Tag every finding with a severity from the ladder at the bottom. - ---- - -## Severity ladder - -One vocabulary for every dimension. A finding's severity is about blast radius, not -which dimension raised it. - -| Severity | Meaning | Action | -|---|---|---| -| **Critical** | Exploitable, data loss, crash, broken contract, or **breaks an already-shipped client**. | Block merge. Fix now. | -| **High** | Type-safety hole, missing error handling, missing parity, missing validation, dead code that ships. | Fix before merge. | -| **Medium** | Pattern inconsistency, missing edge case, missing test, defense-in-depth gap. | Fix soon; OK to merge with a tracked follow-up. | -| **Low** | Style deviation, minor naming, micro-cleanup. | Address when convenient. | -| **Info** | Observation, forward-compatible note, praise. | No action required. | - -### The `⚠️ breaks old mobile clients` marker - -A **Critical-class** marker, separate from the severity word, applied to any finding -where a `packages/shared` Zod schema or an orbit-api DTO change makes an -already-installed Android client misbehave. Old Android builds ship a **frozen -`@orbit/shared` snapshot** — a server-side or shared rename is invisible to them; they -keep using the old field name and silently break. Detection and classification are -defined in the **Contract drift + backward-compat guard** dimension below. Any finding -carrying this marker is Critical regardless of how small the diff looks. - ---- - -## Signal gate — post high-signal only - -The review CONVERGES; it is not a nit machine. What gets posted is gated by severity: - -- **Critical / High** — always post; these decide the outcome. -- **Medium** — post only when concretely actionable (a specific missing test, a real unhandled edge case, a definite pattern break). Never speculative. -- **Low / Info** — do **not** post as PR-review findings. A local deep audit may list them; on a PR they are noise. - -**Never post — not findings, in any dimension:** style preferences (verbose vs concise, arrow vs named function, optional-chaining vs guard); naming bikeshed; reformatting; "consider extracting / hoisting / future-proofing" on code that already works; Zod modifier ordering (`.nullable().optional()`) when behavior is correct; magic-number→const when the value is obvious from context; anything the author chose defensibly that you would merely prefer otherwise; anything already addressed in an earlier commit or a resolved review thread. - -**Outcome is deterministic:** `NEEDS WORK` iff ≥1 surviving **Critical or High** finding (including any `⚠️ breaks old mobile clients`); otherwise `APPROVE`. Medium / Low / Info never force NEEDS WORK. Never manufacture a Critical/High finding to avoid approving — a clean diff earns a plain approval. - ---- +**At a glance:** the frozen list of what a review checks here. `/pr-review` walks it over a diff and +`/audit-code-quality` walks the same file over the whole repo, so it stays orchestration-free: dimensions, +severities, and the finding template only. A twin lives at `orbit-api/.claude/skills/pr-review/rubric.md` +because `/pr-review` runs from either repo root and two git repos cannot share a file; mirror any +substantive edit there in the same task. Every finding quotes the diff line it is about and cites the rule +it came from by section, never by line number: line numbers rot silently and then point at the wrong rule. + +## Severity and the blocking decision + +| Severity | Meaning | +|---|---| +| **Critical** | Exploitable, data loss, crash, broken contract, or breaks an already-shipped client. | +| **High** | Type-safety hole, missing error handling, missing parity, missing validation, a test that cannot fail. | +| **Medium** | Pattern inconsistency, missing edge case, defense-in-depth gap. | +| **Low / Info** | Style deviation, minor naming, observation. | + +Severity is descriptive. **`blocking` is the decision**, and it has one test: does the finding break +behaviour, security, or data integrity? A High that does not is `"blocking": false` and becomes a follow-up +ticket. Never manufacture a Blocking finding to avoid handing over a clean diff. + +`BREAKS-OLD-CLIENTS` is a Critical-class marker, always Blocking, for a change that makes an +already-installed Android client misbehave, because old builds ship a frozen `@orbit/shared` snapshot. +Detection is dimension 7. + +## Gate-owned: never a hand-written finding + +The most-broken rule in `.claude/rules/core.md`. If a required check already fails on it, a hand-written +finding is noise, because the PR cannot merge either way. ESLint `local/*` and Roslyn `ORBIT0001..0005` +(`Lint`) own `any` / `as any` / `as unknown as X`, `console.log`, the whole comment policy, full-bleed web +buttons, overshoot easing, and `will-change` discipline. `Type Check`, `Unit Tests`, `Build`, `Dash Ban` +(em and en dashes, PR title and body included), `Copy Register` (shouted strings and the cliche register in +i18n values), `Design Token Guard` (raw `--slate-*`, `transition-all`, `h-screen` in `apps/*`), +`Suppressions Ratchet`, `Expo SDK Pin`, `Dependency Review`, `Dependency Audit`, `GitGuardian Security +Checks`, `Analyze` (CodeQL), `SonarCloud Code Analysis`, and `React Doctor` own the rest. Three gates are +partial and the uncovered half is yours: `local/no-decorative-glow` and `local/no-raw-gradient` ship at +`warn` in `apps/*`, so pre-existing violations are known debt but a **newly introduced** one is a finding; +`Cross-Platform Parity` and `Contract Drift` are scoped in dimensions 8 and 7. ## Finding template -Every finding, every dimension, the same shape: - ``` -[SEVERITY] ⚠️ breaks old mobile clients (only if applicable) -· dimension: -· location: /: -· issue: <1-2 sentences — what is wrong> -· risk: <1-2 sentences — what goes wrong if it ships> -· fix: -· reference: +[SEVERITY] blocking: true|false [BREAKS-OLD-CLIENTS if applicable] +- dimension: +- location: /: (a line in the diff; quote it) +- claim: +- fix: +- reference: ``` ---- - ## Dimensions -Each dimension is a checklist. A diff that doesn't touch a dimension's surface skips it -(noted as N/A) — do not invent findings for files the diff never changes. UI dimensions -are **gated to `apps/*` changes**; backend hard rules are gated to `orbit-api` changes. +A diff that does not touch a dimension's surface skips it, recorded as N/A with the reason. Do not invent +findings for files the diff never changes. ### 1. Correctness -> Reference: the change's own intent (PR body / linked issue / plan). +> Reference: the change's own intent (PR body, linked ticket). -- Does it do what the PR/issue says, across every boundary it crosses? -- Data flow: request shape in → handler → response shape out → consumer reads it. Any - mismatch in that chain? -- Boundary conditions: empty list, zero, null, first/last item, timezone edges (dates - must route through `IUserDateService` on the backend — see dimension 13). -- State: are loading / error / empty states all handled, not just the happy path? -- Concurrency / ordering assumptions that the diff silently relies on. +Does it do what the ticket says, across every boundary it crosses? Follow the data flow (request shape in, +handler, response shape out, consumer reads it) and name any mismatch. Check the boundaries (empty list, +zero, null, first and last item, timezone edges), that loading, error, and empty states are all handled, +and any concurrency or ordering assumption the diff silently relies on. ### 2. Dead / stale code -> Reference: CLAUDE.md rule 2; orbit-api "No dead code". +> Reference: CLAUDE.md rule 2. -- Orphaned exports, functions, or types with **zero references** after this change - (cite the zero-reference grep). -- Dead branches that can no longer be reached. -- Commented-out code blocks. -- Stub functions and speculative "just in case" parameters. -- Imports / variables the diff itself left unused. +Orphaned exports, functions, or types with zero references after this change (cite the grep); dead branches; +commented-out code; stubs and speculative parameters; imports the diff itself orphaned. ### 3. SOLID / clean architecture -> Reference: CLAUDE.md rules 6, 7, 10. - -- Function size soft cap ~50 lines, nesting ~3 levels; hard cap ~100. Over → the - function is doing too much, split it (rule 7). A file around 1,000 lines or one - carrying several unrelated responsibilities is a cohesion finding when the evidence - supports it. Split by responsibility or extract well-named pure helpers; when a split - merely moves the same tangle, report relocation, not simplification. -- New endpoints follow CQRS (Command/Query + Handler + Validator) on the backend. -- Frontend respects the adapter split: Server Action (web) vs `apiClient` (mobile); - shared logic in `packages/shared`, not duplicated per app. -- For branch-heavy code, look for the **code-judo move**: a state-model or data-shape - reframe that deletes whole branches. Prefer that reframe to adding more conditionals. -- Flag special-case `if/else` ladders, deeply coupled branching, and flag soup that grows - per case. Prefer the smallest fitting remedy: early returns, a lookup table, or - polymorphism that makes the variants explicit. -- No premature abstraction — extract on the third real use, not the second (rule 6). - Three similar lines beat a helper invented for two. Apply the **deletion test** to thin - wrappers: if removing the module makes its complexity vanish instead of exposing useful - behavior, it is pass-through indirection. Flag magical abstractions that hide control - flow; delete needless wrappers or deepen the abstraction until its boundary is clear. -- Repeated casts or optionality juggling indicate a structural type mismatch when one - better type or one parse at the trust boundary would remove the churn. Recommend that - structural fix, but exclude gate-owned mechanical forms such as `as any`, - `as unknown as X`, and unjustified `null!`; dimension 6 owns those direct violations. -- DRY at the right level (rule 10): cross-app → `packages/shared`; cross-component → - `apps//components/`; repeated handler or cross-function logic → one - well-named helper at the narrowest shared layer. Don't lift to `shared` for one caller. -- Business logic belongs in its canonical domain, CQRS, or shared-logic layer, not in a - controller, component, DTO, or platform adapter. Move the rule to the owning layer - instead of duplicating or coordinating it at the edges. - -### 4. Comment policy - -> Reference: `eslint-rules/no-comments.cjs:17-24` (local/no-comments); orbit-api `ORBIT0001`. - -The reviewer flags a comment exactly when the linter would. **Allowed**, nothing else: - -- `/** … */` JSDoc block (a `Block` comment whose value starts with `*`) on an exported - function, hook, or type — one short paragraph on intent and contract. -- A `///` line (a `Line` comment whose trimmed value starts with `/`) — TS triple-slash - reference / C# XML doc. -- A tooling directive matching `no-comments.cjs`'s `DIRECTIVE` set: `eslint-disable*`, - `@ts-*`, `ts-*`, `prettier-ignore`, `@jsx`, coverage/bundler pragmas - (`c8`/`v8`/`istanbul`/`webpack`/`@vite`/`@vitest`/`@__PURE__`). -- A WHY note that contains an `http(s)://` URL to an upstream issue/PR/doc — a real - external constraint the author cannot fix here. - -Everything else is a finding: `//` narration, restating code, task/PR/fix references, -TODOs. The fix is never "reword the comment" — it is **rename the symbol or extract a -well-named function** so the code reads without prose. - -### 5. No-workaround / root-cause - -> Reference: CLAUDE.md rule 1; orbit-api "No workarounds". - -- The signature smell: **ugly frontend written to dodge a missing or awkward API** — - client-side reshaping, refetch-and-merge, optimistic patches that paper over a shape - the backend should return directly. Flag it and point at the upstream fix. -- Fallbacks, defensive branches, or local patches for a problem that belongs to a - config, a type, or a shared util. -- An unavoidable workaround is allowed **only** with a one-line WHY-with-URL note - (dimension 4). No link → it is not a sanctioned workaround. - -### 6. Type safety - -> Reference: CLAUDE.md rule 3. - -- TypeScript: any `any`, `as any`, or `as unknown as X` escape hatch. Use `unknown` - with narrowing instead. -- C#: implicit conversions and unjustified `null!` (the C# analog of an `as any`). -- Inferred-`any` callbacks and untyped external payloads crossing a trust boundary - without a Zod parse. - -### 7. No `console.log` - -> Reference: CLAUDE.md rule 4. - -- Any `console.log` (or stray `print`/`Debug.WriteLine`) in production code. Use the - project logger or remove it. Test files are exempt. - -### 8. DESIGN.md / AI-slop - -> Reference: `DESIGN.md` at the orbit-ui-mobile repo root, sections **Identity & anchor -> (locked)** (:18), **Bans** (:469), **AI-slop test** (:505), **Scene-sentence test** -> (:525). Cite the section name, not the line alone: the line numbers move, the section -> names do not. **Gated: only when the diff touches `apps/*` UI files.** An -> orbit-api-only diff marks this N/A, and where the orbit-ui-mobile checkout is absent the -> reference cannot be read at all, so it is "not verifiable here". - -The anchor is the **de-decorated navy-violet orbital** (the #539 freeze, 2026-07-17). -Identity comes from three carriers and nothing else: the **orbital logo mark**, the -**Astra orbital glyph**, and **ring-shaped status and progress indicators**. It never -comes from a background gradient, a glow, decorative background orbit arcs, or texture. -**Quiet decoration is still decoration**: a softened glow, a 0.03-opacity texture, a -"subtle" mesh is the same finding as the loud version. The freeze removed the layer, it -did not dim it. - -Scan for the AI-slop tells: - -- Decoration used as hierarchy: any glow, gradient wash, gradient border, gradient text - (`bg-clip-text` over a gradient), mesh, bloom, texture, or "quiet" background effect. - There is no sanctioned gradient and no sanctioned glow left: `--gradient-header`, - `GradientTop`, and the primary-glow shadow token are **deleted**. -- Cards in cards (opaque card-on-card on dark), and cards used where spacing would have - grouped. -- A coloured side-stripe border on a row, card, callout, or alert. -- Connector or tree lines in a hierarchy. -- Grey text on coloured backgrounds; rounded-square icon tiles above headings. -- Semantic-red destructive fills where the spec shows a text pill. -- An oversized centered H1 outside a hero context. -- The hero-metric template used as decoration, or any invented precise-looking number. -- A whole-section fade-and-rise scroll reveal, or any page-load choreography. -- An animation whose purpose cannot be named from the closed list. -- A heading and the intro beneath it saying the same thing; an eyebrow that enumerates - rather than labels. - -Token checks that need judgment: - -- **`--primary` is fill and graphic only; `--primary-soft` is the accent text token.** - Accent-coloured small text on the canvas is a finding, not a preference. On light both - resolve to the same value, so the split only bites on dark. -- **Accent rationing**: the accent appears on the active tab, progress and ring - indicators, done dots, the primary CTA, the FAB, and active nav. That is the whole - list. Accent on a card, a row, a border, a heading, or an icon not communicating state - is decoration. -- No raw `--slate-*` reference and no hardcoded violet rgba. Semantic tokens only; tints - come from `--primary-rgb` (web) or `tintFromPrimary` (mobile). -- No hand-rolled `box-shadow` heavier than `--shadow-1/2/3`. Shadows model occlusion - under a lifted surface; they never carry the accent hue. -- No `transition-all` (animate `transform` and `opacity`, named); no `h-screen` (use - `min-h-dvh`); no new font families, radii, or colors outside the spec. -- No per-component scheme branch. Schemes resolve through tokens. - -**Do not hand-flag what an ESLint `local/*` rule already fails on** in orbit-ui-mobile: -decorative glow, raw gradient and gradient text, side-stripe borders, off-scale spacing, -`space-x-*` / `space-y-*`, overshoot easing, arbitrary z-index, full-bleed pill CTAs, and -em dashes in copy each have a gate. Report the gate's verdict; re-flagging it by hand is -noise. `DESIGN.md`'s **Enforcement** section is the authoritative gate-versus-reviewer -split. - -Then the **scene-sentence test**: describe the rendered screen in one sentence, as if -narrating a film scene. If it reads like every other SaaS app ("a clean modern dashboard -with cards"), it is generic. The sentence must name Orbit's character: a near-black -neutral canvas, quiet tonal panels separated by hairlines, one violet reserved for what is -done and what is next, and the orbital ring language carrying the identity. **If the only -way to make the sentence specific is to describe decoration, the design has failed and the -decoration is not the fix.** - -### 9. Parity (web ↔ mobile) - -> Reference: root CLAUDE.md "Cross-platform parity (MANDATORY)". Engine: `parity-checker`. - -- Every changed `apps/web/**` file has its `apps/mobile/**` mirror changed in the same - PR (and vice-versa), per the mirror map in the `parity-checker` contract. -- The mirror is **behaviorally identical** — same logic, data flow, error handling. - Only platform adapters may differ (BFF vs direct API, cookie vs SecureStore, shadcn vs - NativeWind, next-intl vs i18next). -- `MISSING` (no mirror file) is High; `PARTIAL` (mirror exists, not updated) is High - until proven intentional. - -### 10. i18n - -> Reference: root CLAUDE.md (add keys to both locales in the same edit). Engine: `i18n-syncer`. - -- Every new user-facing string has a key in **both** `packages/shared/src/i18n/en.json` - AND `pt-BR.json` (`MISSING_PT` / `MISSING_EN` are findings). -- No `ORPHANED` callsite referencing a key that exists in neither locale. -- Brand words (`Orbit`, `Astra`) stay untranslated. -- Keys stay dot-notation hierarchical and alphabetized within their hierarchy. - -### 11. Contract drift + backward-compat guard - -> Reference: CLAUDE.md "API contract" / orbit-api "Cross-repo parity contract". -> Engine: `contract-aligner` for the field-by-field shape comparison. - -First, drift (from `contract-aligner`): `MISSING_DTO`, `MISSING_ZOD`, `FIELD_DRIFT`, -`PATH_DRIFT` between `packages/shared/src/types/*` + `endpoints.ts` and the orbit-api -DTOs + Controller routes. - -Then the **backward-compat judgment** drift detection alone does not make — the -direction and the add/remove of each field, because old Android clients run a frozen -`@orbit/shared`: - -- **Field removed from / renamed in a *response* DTO or schema** → old clients that read - it now get `undefined` → **`⚠️ breaks old mobile clients` (Critical)**, unless the - field was already optional AND unused (cite the grep proving it). -- **Field removed from / renamed in a *request* DTO or schema, or a field made - newly-required** → old clients still send the old shape → server validation rejects - it → **`⚠️ breaks old mobile clients` (Critical)**. -- **Field added as optional** → forward-compatible → **Info**, not a break. -- **Enum value removed** → old clients may still send it → flag. - -Recommend the compatible alternative in the fix: keep-and-deprecate the old field, -accept both names server-side for a release, or gate behind the min-version gate. When -old-client reach is uncertain, downgrade to **High** with a "verify old-client usage" -note rather than over-claiming Critical. - -### 12. Security - -> Reference: OWASP + orbit-api hard rules. Engine for API code: `security-reviewer` -> (the frontend categories below are what that agent explicitly does NOT cover). - -Review the categories relevant to the change. - -**Injection** — raw or string-interpolated SQL / EF queries; XSS via unescaped user -input in JSX or `dangerouslySetInnerHTML`; command injection (`exec()` / -`Process.Start()` with user input); path traversal from unsanitized input in file paths. - -**Authentication & authorization** — missing `[Authorize]` on a new API endpoint (the -default is `[Authorize]`; missing both it and `[AllowAnonymous]` is a bug); missing auth -checks on Server Actions / BFF routes; hardcoded credentials, JWT secrets, or API keys; -session config must stay httpOnly + sameSite strict + secure always; CORS must stay -restrictive (no `AllowAnyHeader()` / `AllowAnyMethod()`, never `AllowAnyOrigin()` with -`AllowCredentials()`); the Stripe API key set globally in `Program.cs`, never -per-request. - -**Data exposure** — sensitive data (passwords, tokens, PII) in `console.log` or -`ILogger`; responses leaking stack traces or DB schema; secrets in source / config; -missing input validation at the API boundary; webhook handlers must verify signatures -(Stripe `WebhookSecret`). - -**Dependency & configuration** — known-vulnerable dependency versions; debug mode -enabled in production config; `SecurityHeadersMiddleware` (nosniff, DENY, -referrer-policy, XSS) must not be disabled; request size limits (Kestrel 10MB global, -chat endpoint 20MB) intact. - -**Cryptography** — weak hashing (MD5 / SHA1 for passwords — BCrypt is the standard); -hardcoded encryption keys; insecure RNG for security-sensitive values; HTTPS enforcement -intact. - -**Error handling** — verbose error messages exposing internals; unhandled promise -rejections / unobserved tasks; catch blocks that swallow errors silently; `Result` -propagated correctly (`PropagateError()` / `ToPayGateAwareResult()` per -`orbit-api/CLAUDE.md`). - -**Validation (Orbit-specific)** — the backend is the source of truth; frontend Zod is -convenience only. Every new endpoint needs FluentValidation **and** a domain-entity -guard in the factory/update method. Numeric bounds, date ranges, and mutually exclusive -options are enforced server-side. - -### 13. Backend hard rules - -> Reference: orbit-api/CLAUDE.md "Cross-cutting hard rules". **Gated: only when the diff -> touches `orbit-api`.** - -- **Timezone**: user-facing dates use `IUserDateService.GetUserTodayAsync(userId)`, - never `DateOnly.FromDateTime(DateTime.UtcNow)`. `DateTime.UtcNow` is only for - `CreatedAtUtc` timestamps and cache keys. -- **Authorization**: every controller endpoint requires JWT Bearer unless it is - `/health` or `/api/auth/*`; new endpoints default to `[Authorize]`. -- **Validation**: validators in `Orbit.Application//Validators/` **and** - domain-entity guards. -- **Logging**: structured, PascalCase properties, English only — - `logger.LogInformation("Action {Property}", value)`, never interpolated. -- **Transaction teardown**: no explicit `RollbackAsync()`/`Rollback()` inside a - `using`/`await using`-scoped EF transaction — scope disposal already rolls back an - uncommitted transaction. The `ORBIT0002` analyzer (`src/Orbit.Analyzers`) fails the CI - build on violations; a genuinely manually-owned transaction (no `using`) is exempt. -- **Tests**: every new command/query handler, validator, and service has a unit test - (unit only — no integration or E2E suite exists). - -### 14. FEATURES.md parity (feature inventory) - -> Reference: `FEATURES.md` at the orbit-ui-mobile repo root — the code-derived feature -> inventory (#378). **Gated: only when the diff changes the user-facing feature surface.** - -- Triggers: a feature added, materially changed, or removed — new screen/route/tab, new - or removed Astra (`IAiTool`) or MCP (`[McpServerTool]`) tool, plan-gating change - (`PayGateService` / `AppConstants`), platform-availability change, or locale-specific - behavior change. Pure refactors, bugfixes, and visual polish with no behavior change - are N/A. -- The same PR updates `FEATURES.md` — row added, edited, or removed, with the Gating / - Platform / Locale columns still accurate, and the stated tool counts corrected when - tools are added or removed. A missing update is **High** (same bar as a missing - web↔mobile mirror); a gating or platform claim the diff makes stale is **High** too. -- Headline-set features (Astra, MCP, social, core tracker) also surface in the in-app - feature guide (`onboarding.featureGuide.*`) — if the change makes the guide wrong or - incomplete, flag it (**Medium**). -- In the orbit-api repo the file is not in the checkout: do not verify — emit the - finding as "FEATURES.md update required in thomasluizon/orbit-ui-mobile" (**High**) - so it lands in the paired frontend PR. - ---- - -## Self-review note - -This rubric and the skill that walks it are themselves held to the standard they -enforce: every code snippet here is exemplary (no narration comments, no `any`, no -`console.log`). Dogfood the rubric against the review output before posting. +> Reference: CLAUDE.md rules 3, 4, 7. + +- Function soft cap ~50 lines and ~3 nesting levels, hard cap ~100. A file near 1,000 lines or carrying + unrelated responsibilities is a cohesion finding when the evidence supports it, but when a split would + merely relocate the same tangle, say that instead. +- For branch-heavy code look for the **code-judo move**: a state-model or data-shape reframe that deletes + whole branches. Prefer it to another conditional; flag flag-soup and special-case ladders. +- No premature abstraction: extract on the third real use, not the second. Apply the **deletion test** to + thin wrappers, since if removing the module makes its complexity vanish rather than exposing useful + behaviour it is pass-through indirection. Repeated casts or optionality juggling means a structural type + mismatch that one better type, or one parse at the trust boundary, would remove. +- DRY at the right level: cross-app to `packages/shared`, cross-component to `apps//components/`, + never lifted to shared for one caller. Business logic belongs in its domain, CQRS, or shared layer, never + in a controller, component, DTO, or adapter; new backend endpoints follow CQRS. + +### 4. No-workaround / root cause + +> Reference: CLAUDE.md rule 1. + +The signature smell is **frontend written to dodge a missing or awkward API**: client-side reshaping, +refetch-and-merge, optimistic patches papering over a shape the backend should return. Flag it and point at +the upstream fix, and the same for fallbacks or defensive branches covering a problem that belongs to a +config, a type, or a shared util. An unavoidable workaround is allowed only with a one-line WHY comment +carrying an `http(s)://` link to the upstream issue; no link means it is not sanctioned. + +### 5. Test quality + +> Reference: the `tdd` skill (`~/.claude/skills/tdd/`). `Unit Tests` proves the suite is green; it cannot +> tell a real test from one that passes by construction. + +- **Tautological test (High).** The assertion recomputes the expected value the way the code does + (`expect(add(a, b)).toBe(a + b)`, a snapshot derived by hand the same way, a constant asserted equal to + itself), so it passes by construction and can never disagree with the code. Expected values must come + from an independent source of truth: a known-good literal, a worked example, the spec. This is exactly + what a worker under an evidence gate produces when it needs a green result, so read the new tests before + believing the green check. +- **Implementation-coupled test (Medium).** Mocks internal collaborators, tests private methods, or + verifies through a side channel (querying the database instead of reading back through the interface). + The tell: it breaks on a refactor that changed no behaviour. Mock only at system boundaries (external + APIs, time, randomness), never your own modules. +- **New behaviour with no test that would fail without it is High.** Assert the behaviour a caller cares + about, not the shape of the call. + +### 6. Security + +> Reference: OWASP and the orbit-api hard rules. CodeQL, SonarCloud, and GitGuardian own the mechanical half +> (injection patterns, committed secrets); the judgement below is yours. + +- **Authorization.** A new controller endpoint carries `[Authorize]` or `[AllowAnonymous]`; missing both is + a bug and the default is `[Authorize]`. `[AllowAnonymous]` on anything touching user data is + public-by-mistake, a `userId` read from the request body instead of `User.GetUserId()` is a tenancy hole, + and Server Actions and BFF routes need their own auth check. +- **Webhooks and keys.** Every Stripe webhook calls `EventUtility.ConstructEvent(json, signature, + WebhookSecret)` before any processing and rejects a null or empty `WebhookSecret`; the Stripe API key is + set once at startup, never per request. The JWT secret comes from configuration with no dev fallback in + production, HS256 pinned with `none` and asymmetric rejected, short-lived access tokens, DB-backed + revocable refresh tokens, never logged. +- **CORS and session.** No `AllowAnyHeader()`, no `AllowAnyMethod()`, never `AllowAnyOrigin()` with + `AllowCredentials()`. Web session stays httpOnly, sameSite strict, secure; mobile tokens live in + SecureStore, never AsyncStorage. +- **Validation and rate limits.** The backend is the source of truth and frontend Zod is convenience only: + every new endpoint has a FluentValidation validator **and** a domain-entity guard, with request size + limits intact (Kestrel 10MB global, chat 20MB). Abuse-prone endpoints (auth `send-code` / `verify-code`, + chat, AI summary) carry `[DistributedRateLimit]`. +- **Leakage.** No stack traces, DB schema, passwords, tokens, or PII in responses or logs, and response DTOs + carry no password hashes or refresh tokens. Logging is structured, never interpolated, because + interpolation leaks PII into log analytics. Weak hashing (MD5, SHA1) or an insecure RNG for a + security-sensitive value is Critical. Frontend: XSS via unescaped input in JSX or + `dangerouslySetInnerHTML`, and auth state reaching logs or analytics. + +### 7. Contract alignment and backward compatibility + +> Reference: CLAUDE.md "Security & contracts"; issue #206. **`Contract Drift` regenerates the Zod snapshot +> from orbit-api `main`**, so it cannot see a paired in-flight API PR and it never makes the append-only +> judgement. Both are yours. + +Compare `packages/shared/src/types/*` and `packages/shared/src/api/endpoints.ts` against the orbit-api DTOs +(feature-local records under `src/Orbit.Application/`) and the Controller routes; casing is expected to +differ (PascalCase C#, camelCase over JSON). Report `MISSING_DTO`, `MISSING_ZOD`, `FIELD_DRIFT` (name, type, +or required-vs-optional), `PATH_DRIFT` (route or method). Then make the append-only judgement drift +detection does not: shared and DTO changes **add optional fields** and never rename, remove, or retype a +field an old mobile client still reads, because mobile lags via the Play store. + +- Removed or renamed in a **response** DTO or schema: old clients read `undefined`. **Critical, + `BREAKS-OLD-CLIENTS`**, unless it was already optional AND unused (cite the grep). +- Removed or renamed in a **request** DTO or schema, or made newly required: old clients still send the old + shape and validation rejects it. **Critical, `BREAKS-OLD-CLIENTS`**. +- Added as optional: forward-compatible, **Info**. Enum value removed: old clients may still send it. + +The fix is always the compatible alternative: keep-and-deprecate, accept both names server-side for a +release, or expand-contract behind the `AppConfig.MinSupportedVersion` gate, raised only after the carrying +build is live in the Play fleet. When old-client reach is genuinely uncertain, downgrade to High with a +"verify old-client usage" note rather than over-claiming Critical. Semantic breaks under an unchanged field +name belong to dimension 1; do not over-claim completeness here. + +### 8. Cross-platform parity + +> Reference: root CLAUDE.md "Cross-platform parity (MANDATORY)". **The `Cross-Platform Parity` gate only +> counts changed files per platform**, so it catches a wholly one-sided PR and nothing else. Per-file +> mirrors and behavioural equivalence are yours. + +Every changed `apps/web/**` file has its `apps/mobile/**` mirror changed in the same PR and vice versa: +`hooks/use-.ts`, `stores/-store.ts`, and `components//.tsx` map one to one, +`app/(app)//page.tsx` maps to `app/.tsx`, and `app/actions/.ts` maps to a mobile hook calling +`apiClient` directly. The mirror is **behaviourally identical**: same logic, data flow, and error handling, +reverts included. Only platform adapters may differ (BFF vs direct API, cookie vs SecureStore, shadcn vs +NativeWind, next-intl vs i18next). A missing mirror file, or one that exists but was not updated, is +**High** until proven intentional (the `parity:exempt` label plus a justification in the PR body). Never +flag `packages/shared/**`, `apps/web/middleware.ts`, `apps/web/app/api/[...path]/route.ts` (its mobile +equivalent is built into `apps/mobile/lib/api-client.ts`), or a platform-specific test. + +### 9. i18n + +> Reference: root CLAUDE.md. No gate checks locale parity. + +Every new user-facing string has a key in **both** `packages/shared/src/i18n/en.json` and `pt-BR.json`, in +the same edit (`MISSING_PT` / `MISSING_EN`), and no callsite references a key that exists in neither +(`ORPHANED`). `Orbit` and `Astra` are never translated. Keys stay dot-notation hierarchical and alphabetized +within their hierarchy, with ICU plurals where a count is interpolated. Copy that is wordy, redundant with +its own heading, or inconsistent in terminology across the two locales is a finding even though the Copy +Register gate passed it. + +### 10. Design + +> Reference: `DESIGN.md`. **Gated: only when the diff touches `apps/*` UI files.** Cite the DESIGN.md section +> for every finding; if a rule is in neither `DESIGN.md` nor `.claude/rules/`, it is your taste and you must +> label it as such. + +The anchor is the **de-decorated navy-violet orbital** (#539 freeze): a near-black canvas, one rationed +violet, opaque surface steps, hairline rings and dividers, with hierarchy bought by surface steps, size, +weight, and whitespace. **Quiet decoration is still decoration**: a softened glow or a 0.03-opacity texture +is the same violation as the loud version. + +- **AI-slop test.** Any decorative glow or gradient (the tokens are deleted, so anything reaching this diff + is hand-rolled), including gradient borders and `bg-clip-text` gradient text; decorative background orbit + arcs; a coloured side-stripe border; cards-in-cards; connector or tree lines; gray text on coloured + backgrounds; rounded-square icon tiles above headings; an oversized centered H1 outside a hero; the + hero-metric template used as decoration, or any invented precise-looking number. +- **Token adherence and accent rationing.** `--primary` is fill and graphic only: CTA background, FAB, + progress ring, done dots, level bar, active tab, active nav. That is the whole list. It is never small + text on the canvas (that is `--primary-soft`), never decorative on a card, row, border, or heading. No new + font, radius, shadow, or colour outside the spec. +- **Scene-sentence test.** Describe the rendered screen in one sentence. If it reads like generic SaaS ("a + clean modern dashboard with cards"), name what would make it read as Orbit; if the only way to make it + specific is to describe decoration, the design has failed and decoration is not the fix. **Restraint has a + floor** on the other side: a quieting pass that flattens everything to one size and weight, goes + grayscale, or trades an affordance for calm has also failed. +- **Responsive and layout.** Desktop composes horizontally rather than stretching one mobile column, with + intrinsic-width pills; the 65ch measure; concentric radii (outer = inner + padding); one focal element per + view; a card is not a layout primitive; every sub-screen has a visible back affordance. A pill hugs its + content unless it is a mobile bottom-sheet or dialog's single primary action, a mobile auth or onboarding + submit, a full-screen empty-state CTA, or a paired confirm row, and the lint rule cannot see mobile + StyleSheet width, so flag `alignSelf: 'stretch'` and `width: '100%'` pills by eye. +- **A11y.** Colour as the only signal; the 3:1 non-text contrast floor for icons, borders, and state + indicators; where focus lands on open, that it is trapped, and where it returns on close; a localized + label in both locales for icon-only controls; hit targets at least 44, reached by padding rather than by + growing the glyph. Arbitrary `z-*` values have no lint rule yet, so flag them by eye. 200% zoom and + screen-reader semantics need the live DOM: say a pass is owed, do not guess. + +### 11. Backend hard rules + +> Reference: orbit-api/CLAUDE.md "Cross-cutting hard rules". **Gated: `orbit-api` changes only.** + +What dimensions 5 and 6 do not already carry. **Timezone**: user-facing dates use +`IUserDateService.GetUserTodayAsync(userId)`, never `DateOnly.FromDateTime(DateTime.UtcNow)`, which is for +`CreatedAtUtc` and cache keys only. **Validator placement**: `Orbit.Application//Validators/`. +**Result flow**: `Result` propagated correctly (`PropagateError()`, `ToPayGateAwareResult()`), with no +catch block that swallows an error silently. **Test scope**: every new command or query handler, validator, +and service has a unit test, and unit is all there is, so never ask for an integration or E2E suite. + +### 12. FEATURES.md gating + +> Reference: `FEATURES.md` at the orbit-ui-mobile root. **Gated: only when the diff changes the user-facing +> feature surface.** Hand-maintained, so nothing generates it and no gate checks it. + +Triggers: a new screen, route, or tab; a new or removed Astra (`IAiTool`) or MCP (`[McpServerTool]`) tool; a +plan-gating change (`PayGateService`, `AppConstants`); a platform-availability or locale-specific behaviour +change. Pure refactors, bugfixes, and visual polish are N/A. The same PR updates the row, keeping the +Gating, Platform, and Locale columns accurate and the stated tool counts correct; a missing update is +**High**, as is a gating or platform claim the diff makes stale, and a change that makes the in-app guide +(`onboarding.featureGuide.*`) wrong is **Medium**. In the orbit-api repo the file is not checked out, so do +not guess: emit "FEATURES.md update required in thomasluizon/orbit-ui-mobile" (**High**) so it lands in the +paired frontend PR. From cd09d70ea5fa93f41dfdfc8ec547f09198ffd99d Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 15:24:21 -0300 Subject: [PATCH 02/10] ORB-261: close canonical review blind spots --- .claude/skills/pr-review/SKILL.md | 21 +++++++++++++++------ .claude/skills/pr-review/rubric.md | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 8602707e..fd6ea846 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -53,7 +53,12 @@ report time, against the definition above, not renegotiated per round. **4. Diff-only scope.** Read `gh pr diff`. You are reviewing a diff, not a repository. Open a repository file only to resolve a symbol the diff itself cites. A defect reachable only by browsing -code the diff never touched is out of scope; if it matters, it is a ticket. +code the diff never touched is out of scope; if it matters, it is a ticket. Dimension 7 has one +bounded exception: when the target diff changes a request, response, endpoint, or schema contract, +perform a targeted read/search in the sibling repository's primary `main` checkout for the shipped +consumer or provider symbols cited by that contract. If the PR body links a paired contract PR, +inspect that paired diff too. This is contract evidence, not permission to browse for unrelated +defects. **5. Monotonic round 2.** Re-check **only** the frozen Blocking list, answering `CLOSED` or `OPEN` per finding. New findings are forbidden, with exactly one mechanical carve-out: **any defect on a @@ -79,7 +84,8 @@ Concretely, before round 1: - Confirm this session did not write any of the code in the diff. If it did, **stop**: the review is invalid under rule 2. Fork-inherited context counts as the same session. - `cwd` is the main checkout of the repo the PR targets. Never a worktree, never the fixer's tree. -- The only inputs are the diff, `rubric.md`, and the PR title/body/linked ticket for intent. +- The inputs are the diff, `rubric.md`, and the PR title/body/linked ticket for intent, plus the + targeted sibling-primary/paired-PR contract evidence permitted by rule 4 and nothing broader. --- @@ -121,9 +127,11 @@ Read `rubric.md` once, then classify the diff as **frontend** (`apps/`, `package 1. Compute the fixer's line set: `git diff .. --unified=0`. Pass it as data. 2. For each frozen Blocking finding, answer `CLOSED` or `OPEN` with the line that settles it. 3. A new finding is admissible **only** if its line is in the round-2 line set from step 1, and it - is Blocking. Anything else is a follow-up ticket. -4. All `CLOSED` means hand to Thomas. Any `OPEN` means stop and hand to Thomas with the open list. - There is no round 3. + is Blocking. Append every admitted new blocker to `findings.json` with `status: "OPEN"`; it is + part of the open list and verdict calculation. Anything else is a follow-up ticket. +4. Hand to Thomas as `CLEAN` only when every frozen blocker is `CLOSED` **and no admitted round-2 + blocker is OPEN**. Otherwise stop with every open frozen or admitted blocker listed. There is no + round 3 and no opportunity to hide the new blocker behind the frozen list. --- @@ -145,7 +153,8 @@ integrity is `"blocking": false` and becomes a ticket. Round 2 rewrites the same receipt, sets `rounds` to 2, and adds `"status": "CLOSED" | "OPEN"` to every round-1 Blocking finding. Round-1 entries are never removed. -Set `verdict` to `CLEAN` only when no Blocking finding remains OPEN. Capture `reviewedHeadOid` and +Every newly admitted round-2 blocker is appended with `status: "OPEN"`. Set `verdict` to `CLEAN` +only when no frozen or admitted Blocking finding remains OPEN. Capture `reviewedHeadOid` and `baseSha` from the PR state reviewed; a review of any other head/base is stale by construction. ### Posting diff --git a/.claude/skills/pr-review/rubric.md b/.claude/skills/pr-review/rubric.md index 406e48f6..a360e94a 100644 --- a/.claude/skills/pr-review/rubric.md +++ b/.claude/skills/pr-review/rubric.md @@ -255,3 +255,21 @@ Gating, Platform, and Locale columns accurate and the stated tool counts correct (`onboarding.featureGuide.*`) wrong is **Medium**. In the orbit-api repo the file is not checked out, so do not guess: emit "FEATURES.md update required in thomasluizon/orbit-ui-mobile" (**High**) so it lands in the paired frontend PR. + +### 13. External-interface evidence + +> Reference: root AGENTS.md "Never assume an external interface. Check it, then use it." Gated whenever +> the diff adds or changes a read of a CLI, GitHub/Linear/provider API, Git response, SDK, or library field, +> flag, subcommand, exit code, enum, event argument, or response shape. + +Inspect the PR body's evidence for every such read. It must show the **complete selected key/type shape** +or complete compared enum/value set from a real invocation or installed source, redact credentials and +personal/account values, preserve literal formats the code parses, and include an exact reproduction +command or installed `file:line`. An existing callsite, documentation, memory, and a fixture authored with +the implementation are not evidence. Compare the code and its fixture to that evidence; a fixture that +invents the same field proves nothing. + +Missing or guessed evidence for a field on the correctness path is **High and Blocking** because the +implementation is unproven against the interface it will execute. If the diff redesigns so the unknown is +not read and success depends only on a confirmed exit code, record why this dimension passes. Do not +manufacture a failure from an absent unconfirmed field, and do not expose a credential while proving it. From 41b38e06821b40c2dc17edd5ea032ba26a38430d Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 16:05:39 -0300 Subject: [PATCH 03/10] ORB-261: close canonical review gaps --- .claude/skills/pr-review/SKILL.md | 44 ++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index fd6ea846..6b759f9d 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -34,9 +34,12 @@ a one-line fix from reaching round seven on style alone." Six rules. All six bind. None is advisory, and none is negotiable mid-review. -**1. Freeze the ruleset before round 1.** The dimensions and severity definitions in `rubric.md` -are fixed at PR-open time and cannot move during the review. A bar you would like to raise -mid-review is a follow-up ticket against the rubric, never a new bar this PR has to clear. +**1. Freeze the ruleset before round 1.** Capture the PR's live `baseRefOid`, materialize +`rubric.md` from that exact Git blob, and store both the OID and artifact path in the receipt. +That snapshot supplies the dimensions and severity definitions for both rounds; never reload the +mutable main-checkout copy in round 2. A head or base change invalidates the review and starts a +fresh round 1. A bar raised after the captured base is a follow-up ticket against the rubric, never +a new bar this review may apply. **2. Cross-vendor reviewer, fresh session.** Normal: Claude Opus 5 at `high`. `--codex-only`: Sol at `xhigh` in a **separate** session, and the run prints `DEGRADED: same-vendor review` in its @@ -46,10 +49,12 @@ measurably favour their own family's output, and the bias extends across the who (arXiv 2603.04582, arXiv 2508.06709). The direction is corroborated; the magnitude in a real PR loop is **unmeasured**. State it that honestly, and never claim the degraded mode is unbiased. -**3. Classify every finding at report time as Blocking or Non-blocking.** Blocking means it -**breaks behaviour, security, or data integrity**. Everything else is **auto-filed as a follow-up -Linear ticket** and never fixed in this PR. This is the severity floor. It is applied once, at -report time, against the definition above, not renegotiated per round. +**3. Apply the target repository's review floor, then classify each survivor as Blocking or +Non-blocking.** `orbit-api/AGENTS.md` permits P0/P1 only: Critical maps to P0, High maps to P1, +and Medium/Low/Info candidates are discarded before the receipt and create no ticket. For a +surviving candidate, Blocking means it **breaks behaviour, security, or data integrity**. +Everything else is **auto-filed as a follow-up Linear ticket** and never fixed in this PR. Apply +the floor and classification once at report time; neither is renegotiated per round. **4. Diff-only scope.** Read `gh pr diff`. You are reviewing a diff, not a repository. Open a repository file only to resolve a symbol the diff itself cites. A defect reachable only by browsing @@ -84,7 +89,7 @@ Concretely, before round 1: - Confirm this session did not write any of the code in the diff. If it did, **stop**: the review is invalid under rule 2. Fork-inherited context counts as the same session. - `cwd` is the main checkout of the repo the PR targets. Never a worktree, never the fixer's tree. -- The inputs are the diff, `rubric.md`, and the PR title/body/linked ticket for intent, plus the +- The inputs are the diff, the captured rubric artifact, and the PR title/body/linked ticket for intent, plus the targeted sibling-primary/paired-PR contract evidence permitted by rule 4 and nothing broader. --- @@ -103,12 +108,18 @@ A bare number or blank scope is ambiguous across two repositories and must be re provides a repository-qualified selector or full PR URL; caller cwd never chooses the repository. ```bash -gh pr view {N} --repo {OWNER/REPO} --json number,title,body,baseRefName,headRefName,files,labels +gh pr view {N} --repo {OWNER/REPO} --json number,title,body,baseRefName,baseRefOid,headRefName,headRefOid,files,labels gh pr diff {N} --repo {OWNER/REPO} > /pr-{N}.diff +git show {baseRefOid}:.claude/skills/pr-review/rubric.md > /pr-{N}-rubric.md ``` -Read `rubric.md` once, then classify the diff as **frontend** (`apps/`, `packages/`), **backend** -(`orbit-api/src/`), or **both**. That classification gates which rubric dimensions apply. +If the base object is not present locally, fetch that exact OID from `origin` before `git show`; +never substitute the current working-tree rubric. Record `baseRefOid`, `headRefOid`, the rubric +artifact path, and the complete live selected key/type evidence required by the target AGENTS.md. + +Read the captured rubric once, then classify repository-relative paths using the target repository: +**frontend** is `apps/` or `packages/` in orbit-ui-mobile; **backend** is `src/` or `tests/` in +orbit-api. A paired diff can be **both**. That classification gates which rubric dimensions apply. ### Round 1 @@ -116,7 +127,8 @@ Read `rubric.md` once, then classify the diff as **frontend** (`apps/`, `package never touches and record it as N/A with the reason. Do not invent findings to fill a dimension. 2. Verify each candidate finding against the diff text before writing it down: quote the line you are claiming about. A finding you cannot anchor to a diff line does not get reported. -3. Classify each survivor Blocking or Non-blocking by rule 3. +3. Drop candidates below the target repository floor, then classify each survivor Blocking or + Non-blocking by rule 3. 4. Write `findings.json`. **The list is now frozen.** 5. File every Non-blocking finding as a follow-up Linear ticket (one per finding, title = the claim, body = file, line, and the rubric dimension). They are not fixed in this PR. @@ -142,14 +154,16 @@ Read `rubric.md` once, then classify the diff as **frontend** (`apps/`, `package ```json {"reviewerKind":"independent","verdict":"BLOCKING","rounds":1, "reviewedHeadOid":"","baseSha":"", + "rubricBaseOid":"","rubricArtifactPath":"", "artifactPath":"", "findings":[{"id":"F1","severity":"High","file":"apps/web/hooks/use-streak.ts","line":42, "claim":"one sentence: what is wrong and what goes wrong if it ships","blocking":true}]} ``` -`severity` is descriptive and comes from the rubric's ladder. `blocking` is the decision, and it is -the answer to rule 3's question alone: a High that does not break behaviour, security, or data -integrity is `"blocking": false` and becomes a ticket. +`severity` is descriptive and comes from the rubric's ladder. A candidate below the target +repository floor never enters this array. `blocking` is the decision for a surviving candidate: +a High that does not break behaviour, security, or data integrity is `"blocking": false` and +becomes a ticket where the repository floor permits it. Round 2 rewrites the same receipt, sets `rounds` to 2, and adds `"status": "CLOSED" | "OPEN"` to every round-1 Blocking finding. Round-1 entries are never removed. From c9c7b4b043a2c9865e3c4e616be842abb4b8d79c Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 16:25:55 -0300 Subject: [PATCH 04/10] ORB-261: sync updated canonical rubric --- .claude/skills/pr-review/rubric.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/.claude/skills/pr-review/rubric.md b/.claude/skills/pr-review/rubric.md index a360e94a..4f27f873 100644 --- a/.claude/skills/pr-review/rubric.md +++ b/.claude/skills/pr-review/rubric.md @@ -177,11 +177,18 @@ Every changed `apps/web/**` file has its `apps/mobile/**` mirror changed in the `hooks/use-.ts`, `stores/-store.ts`, and `components//.tsx` map one to one, `app/(app)//page.tsx` maps to `app/.tsx`, and `app/actions/.ts` maps to a mobile hook calling `apiClient` directly. The mirror is **behaviourally identical**: same logic, data flow, and error handling, -reverts included. Only platform adapters may differ (BFF vs direct API, cookie vs SecureStore, shadcn vs -NativeWind, next-intl vs i18next). A missing mirror file, or one that exists but was not updated, is -**High** until proven intentional (the `parity:exempt` label plus a justification in the PR body). Never -flag `packages/shared/**`, `apps/web/middleware.ts`, `apps/web/app/api/[...path]/route.ts` (its mobile -equivalent is built into `apps/mobile/lib/api-client.ts`), or a platform-specific test. +reverts included. Allowed differences are platform adapters (BFF vs direct API, cookie vs SecureStore, +shadcn vs NativeWind, next-intl vs i18next) and a layout shell divergence enumerated in `DESIGN.md`, limited +to navigation chrome (sidebar vs tab bar), the desktop stats rail, the command palette and keyboard +shortcuts, and hover affordances on that shell chrome. Everything below the shell, including a screen, +component, data flow, error path, or behavior, remains parity-bound. The per-file mirror requirement does +not apply when a file's diff is confined to one of the enumerated layout shell surfaces above. It still +applies to every non-shell surface and to any shared product behavior in that file. Otherwise, a missing +mirror file, or one that exists but was not updated, is **High** until proven intentional (the +`parity:exempt` label plus a justification in the PR body). Never flag `packages/shared/**`, +`apps/web/proxy.ts`, +`apps/web/app/api/[...path]/route.ts` (its mobile equivalent is built into +`apps/mobile/lib/api-client.ts`), or a platform-specific test. ### 9. i18n From 1457c0dde5ede519c6eed1d095ec9a8e08c16b0a Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 16:57:58 -0300 Subject: [PATCH 05/10] fix: close canonical review gaps --- .claude/skills/pr-review/SKILL.md | 14 +++++++++----- .claude/skills/pr-review/rubric.md | 4 +++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 6b759f9d..2376a5b8 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -1,7 +1,7 @@ --- name: pr-review description: Capped two-round review of ONE PR diff against rubric.md, by a session that did not write the code. Use when asked to review a PR in orbit-ui-mobile or orbit-api. -argument-hint: +argument-hint: --- # PR Review @@ -37,9 +37,11 @@ Six rules. All six bind. None is advisory, and none is negotiable mid-review. **1. Freeze the ruleset before round 1.** Capture the PR's live `baseRefOid`, materialize `rubric.md` from that exact Git blob, and store both the OID and artifact path in the receipt. That snapshot supplies the dimensions and severity definitions for both rounds; never reload the -mutable main-checkout copy in round 2. A head or base change invalidates the review and starts a -fresh round 1. A bar raised after the captured base is a follow-up ticket against the rubric, never -a new bar this review may apply. +mutable main-checkout copy in round 2. A base change or an unexpected head change invalidates the +review and starts a fresh round 1. The single prescribed round-1-to-round-2 fixer head change keeps +the frozen receipt only when the reviewer is handed that receipt plus both exact head OIDs; any +other head transition restarts round 1. A bar raised after the captured base is a follow-up ticket +against the rubric, never a new bar this review may apply. **2. Cross-vendor reviewer, fresh session.** Normal: Claude Opus 5 at `high`. `--codex-only`: Sol at `xhigh` in a **separate** session, and the run prints `DEGRADED: same-vendor review` in its @@ -136,7 +138,9 @@ orbit-api. A paired diff can be **both**. That classification gates which rubric ### Round 2 -1. Compute the fixer's line set: `git diff .. --unified=0`. Pass it as data. +1. Fetch both exact reviewed head OIDs from `origin`, verify both objects exist, then compute the + fixer's line set: `git diff .. --unified=0`. Pass it as data. Never assume either + head object is already present in the mandated main checkout. 2. For each frozen Blocking finding, answer `CLOSED` or `OPEN` with the line that settles it. 3. A new finding is admissible **only** if its line is in the round-2 line set from step 1, and it is Blocking. Append every admitted new blocker to `findings.json` with `status: "OPEN"`; it is diff --git a/.claude/skills/pr-review/rubric.md b/.claude/skills/pr-review/rubric.md index 4f27f873..fe958891 100644 --- a/.claude/skills/pr-review/rubric.md +++ b/.claude/skills/pr-review/rubric.md @@ -244,7 +244,9 @@ is the same violation as the loud version. What dimensions 5 and 6 do not already carry. **Timezone**: user-facing dates use `IUserDateService.GetUserTodayAsync(userId)`, never `DateOnly.FromDateTime(DateTime.UtcNow)`, which is for -`CreatedAtUtc` and cache keys only. **Validator placement**: `Orbit.Application//Validators/`. +`CreatedAtUtc` and cache keys only. A background schedule window, notification cutoff, or streak +calculation must derive `today` per user timezone and carry a boundary-hour unit test; server-local or +one-global-timezone behavior is a P1 finding. **Validator placement**: `Orbit.Application//Validators/`. **Result flow**: `Result` propagated correctly (`PropagateError()`, `ToPayGateAwareResult()`), with no catch block that swallows an error silently. **Test scope**: every new command or query handler, validator, and service has a unit test, and unit is all there is, so never ask for an integration or E2E suite. From 76cbc8930bc0789218e0d709bcdbd940fa770adc Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 22:11:59 -0300 Subject: [PATCH 06/10] Preserve frozen findings in review receipts --- .claude/skills/pr-review/SKILL.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 2376a5b8..e2ffb378 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -160,6 +160,7 @@ orbit-api. A paired diff can be **both**. That classification gates which rubric "reviewedHeadOid":"","baseSha":"", "rubricBaseOid":"","rubricArtifactPath":"", "artifactPath":"", + "frozenFindingIds":["F1"], "findings":[{"id":"F1","severity":"High","file":"apps/web/hooks/use-streak.ts","line":42, "claim":"one sentence: what is wrong and what goes wrong if it ships","blocking":true}]} ``` @@ -171,6 +172,9 @@ becomes a ticket where the repository floor permits it. Round 2 rewrites the same receipt, sets `rounds` to 2, and adds `"status": "CLOSED" | "OPEN"` to every round-1 Blocking finding. Round-1 entries are never removed. +`frozenFindingIds` is the exact ordered list of round-1 Blocking IDs, is written in round 1 (an +empty array for a clean round 1), and is never changed in round 2. Readiness rejects a round-2 +receipt when that list is absent, empty, duplicated, or no longer represented by Blocking entries. Every newly admitted round-2 blocker is appended with `status: "OPEN"`. Set `verdict` to `CLEAN` only when no frozen or admitted Blocking finding remains OPEN. Capture `reviewedHeadOid` and `baseSha` from the PR state reviewed; a review of any other head/base is stale by construction. From 6a01b6f70ca836a35c4fcc3036f632401f32bea2 Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 22:42:07 -0300 Subject: [PATCH 07/10] Keep canonical review receipts immutable --- .claude/skills/pr-review/SKILL.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index e2ffb378..718862ac 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -175,6 +175,11 @@ Round 2 rewrites the same receipt, sets `rounds` to 2, and adds `frozenFindingIds` is the exact ordered list of round-1 Blocking IDs, is written in round 1 (an empty array for a clean round 1), and is never changed in round 2. Readiness rejects a round-2 receipt when that list is absent, empty, duplicated, or no longer represented by Blocking entries. +Round 1's receipt file is immutable. Round 2 writes a new receipt instead of rewriting it and adds +`roundOneArtifactPath` plus `roundOneArtifactSha256`; readiness rereads that original file, verifies +its SHA-256, derives the complete ordered Blocking ID list, and requires exact equality with +`frozenFindingIds`. A round-one CLEAN receipt must contain no Blocking finding, regardless of any +caller-supplied status. Every newly admitted round-2 blocker is appended with `status: "OPEN"`. Set `verdict` to `CLEAN` only when no frozen or admitted Blocking finding remains OPEN. Capture `reviewedHeadOid` and `baseSha` from the PR state reviewed; a review of any other head/base is stale by construction. From 7f0cfcb2859404e0b7ac3c995f7cdcea2688cf45 Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 22:59:56 -0300 Subject: [PATCH 08/10] Complete canonical interface evidence --- .claude/skills/pr-review/SKILL.md | 5 +++-- .claude/skills/pr-review/rubric.md | 25 ++++++++++++++----------- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index 718862ac..c4a06b10 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -170,8 +170,9 @@ repository floor never enters this array. `blocking` is the decision for a survi a High that does not break behaviour, security, or data integrity is `"blocking": false` and becomes a ticket where the repository floor permits it. -Round 2 rewrites the same receipt, sets `rounds` to 2, and adds -`"status": "CLOSED" | "OPEN"` to every round-1 Blocking finding. Round-1 entries are never removed. +Round 2 writes a separate receipt, sets `rounds` to 2, and adds +`"status": "CLOSED" | "OPEN"` to every round-1 Blocking finding. Round-1 entries are never removed +from the round-2 copy, while the round-one receipt itself remains byte-for-byte immutable. `frozenFindingIds` is the exact ordered list of round-1 Blocking IDs, is written in round 1 (an empty array for a clean round 1), and is never changed in round 2. Readiness rejects a round-2 receipt when that list is absent, empty, duplicated, or no longer represented by Blocking entries. diff --git a/.claude/skills/pr-review/rubric.md b/.claude/skills/pr-review/rubric.md index fe958891..c7450ac3 100644 --- a/.claude/skills/pr-review/rubric.md +++ b/.claude/skills/pr-review/rubric.md @@ -271,14 +271,17 @@ paired frontend PR. > the diff adds or changes a read of a CLI, GitHub/Linear/provider API, Git response, SDK, or library field, > flag, subcommand, exit code, enum, event argument, or response shape. -Inspect the PR body's evidence for every such read. It must show the **complete selected key/type shape** -or complete compared enum/value set from a real invocation or installed source, redact credentials and -personal/account values, preserve literal formats the code parses, and include an exact reproduction -command or installed `file:line`. An existing callsite, documentation, memory, and a fixture authored with -the implementation are not evidence. Compare the code and its fixture to that evidence; a fixture that -invents the same field proves nothing. - -Missing or guessed evidence for a field on the correctness path is **High and Blocking** because the -implementation is unproven against the interface it will execute. If the diff redesigns so the unknown is -not read and success depends only on a confirmed exit code, record why this dimension passes. Do not -manufacture a failure from an absent unconfirmed field, and do not expose a credential while proving it. +Inspect the PR body's evidence for every such read. For fields and response shapes it must show the +**complete selected key/type shape**; for enums or compared values, the complete accepted set; and for a +flag, subcommand, exit code, or event argument, a real redacted invocation and its complete consumed +result or the installed source/usage `file:line` that constructs it. Preserve literal formats the code +parses and include an exact reproduction command. An existing callsite, documentation, memory, and a +fixture authored with the implementation are not evidence. Compare the code and its fixture to that +evidence; a fixture that invents the same interface contract proves nothing. + +Missing or guessed evidence for any field, flag, subcommand, exit code, enum, event argument, or response +shape on the correctness path is **High and Blocking** because the implementation is unproven against the +interface it will execute. If the diff redesigns so the unknown is not read and success depends only on a +confirmed exit code, cite the real invocation or installed source that confirms that exit signal and record +why this dimension passes. Do not manufacture a failure from an absent unconfirmed field, and do not expose +a credential while proving it. From 773bea2d815bbc5a0c4c90d8ad3b9e91050b8830 Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 23:31:53 -0300 Subject: [PATCH 09/10] Tighten canonical review proof --- .claude/skills/pr-review/SKILL.md | 6 ++++++ .claude/skills/pr-review/rubric.md | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index c4a06b10..c2f52e71 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -181,6 +181,12 @@ Round 1's receipt file is immutable. Round 2 writes a new receipt instead of rew its SHA-256, derives the complete ordered Blocking ID list, and requires exact equality with `frozenFindingIds`. A round-one CLEAN receipt must contain no Blocking finding, regardless of any caller-supplied status. +After posting a BLOCKING round-one receipt, hand its exact path to the orchestrator and do not begin +round two until it returns the independent `ROUND_ONE_REGISTERED` ledger path. The orchestrator runs +`record-readiness.mjs --repo --pr --review --register-round-one` before the +fixer transition and stores that returned ledger path in run-state. Final readiness requires the +round-two path, SHA-256, base/head, and frozen IDs to match this pre-fixer ledger; values supplied only +by the round-two artifact are not authority. Every newly admitted round-2 blocker is appended with `status: "OPEN"`. Set `verdict` to `CLEAN` only when no frozen or admitted Blocking finding remains OPEN. Capture `reviewedHeadOid` and `baseSha` from the PR state reviewed; a review of any other head/base is stale by construction. diff --git a/.claude/skills/pr-review/rubric.md b/.claude/skills/pr-review/rubric.md index c7450ac3..308d1c76 100644 --- a/.claude/skills/pr-review/rubric.md +++ b/.claude/skills/pr-review/rubric.md @@ -156,7 +156,9 @@ detection does not: shared and DTO changes **add optional fields** and never ren field an old mobile client still reads, because mobile lags via the Play store. - Removed or renamed in a **response** DTO or schema: old clients read `undefined`. **Critical, - `BREAKS-OLD-CLIENTS`**, unless it was already optional AND unused (cite the grep). + `BREAKS-OLD-CLIENTS`**, unless it was already optional AND proven unused across every + still-supported shipped client build. Cite version-indexed source/artifacts or keep the field + append-only; a grep of the current UI checkout alone is never fleet-safe evidence. - Removed or renamed in a **request** DTO or schema, or made newly required: old clients still send the old shape and validation rejects it. **Critical, `BREAKS-OLD-CLIENTS`**. - Added as optional: forward-compatible, **Info**. Enum value removed: old clients may still send it. From 45a1c1e58602ccfba1801b01216088c588388014 Mon Sep 17 00:00:00 2001 From: thomasluizon Date: Fri, 7 Aug 2026 23:51:00 -0300 Subject: [PATCH 10/10] Bind canonical reviews to exact diffs --- .claude/skills/pr-review/SKILL.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.claude/skills/pr-review/SKILL.md b/.claude/skills/pr-review/SKILL.md index c2f52e71..b374cfd6 100644 --- a/.claude/skills/pr-review/SKILL.md +++ b/.claude/skills/pr-review/SKILL.md @@ -112,9 +112,14 @@ provides a repository-qualified selector or full PR URL; caller cwd never choose ```bash gh pr view {N} --repo {OWNER/REPO} --json number,title,body,baseRefName,baseRefOid,headRefName,headRefOid,files,labels gh pr diff {N} --repo {OWNER/REPO} > /pr-{N}.diff +gh pr view {N} --repo {OWNER/REPO} --json baseRefOid,headRefOid git show {baseRefOid}:.claude/skills/pr-review/rubric.md > /pr-{N}-rubric.md ``` +The second OID read is mandatory and occurs after the diff download. Compare both values byte-for-byte +with the first read; any change discards the diff and restarts round 1. A diff is never paired with OIDs +captured only before that separate GitHub call. + If the base object is not present locally, fetch that exact OID from `origin` before `git show`; never substitute the current working-tree rubric. Record `baseRefOid`, `headRefOid`, the rubric artifact path, and the complete live selected key/type evidence required by the target AGENTS.md. @@ -183,7 +188,8 @@ its SHA-256, derives the complete ordered Blocking ID list, and requires exact e caller-supplied status. After posting a BLOCKING round-one receipt, hand its exact path to the orchestrator and do not begin round two until it returns the independent `ROUND_ONE_REGISTERED` ledger path. The orchestrator runs -`record-readiness.mjs --repo --pr --review --register-round-one` before the +`node /tools/record-readiness.mjs --repo --pr --review +--register-round-one` before the fixer transition and stores that returned ledger path in run-state. Final readiness requires the round-two path, SHA-256, base/head, and frozen IDs to match this pre-fixer ledger; values supplied only by the round-two artifact are not authority.