fix(ci): frontmatter gate, correct review phase numbers, refresh rubric dimension 8

[thomasluizon]

Closes items 1 and 3 of "Open follow-ups carried out of Phase 6" (orbit-ui-mobile/REBUILD.md section 14), plus three follow-ups found while working.

Item 2 moved to #436. It edits .github/workflows/claude-review.yml, and claude-code-action refuses to run when that file differs from the default branch: it logs Skipping action due to workflow validation, reports the review check as passing, and posts nothing. This PR's first run hit exactly that (reviewDecision: REVIEW_REQUIRED, all 21 checks green, zero reviews). Keeping that one file out means the five fixes below get a real automated verdict; #436 carries it alone and needs a human approval.

1. Vendor the frontmatter gate (section 14, item 1)

An unquoted YAML frontmatter scalar whose value contains ": " makes the parser drop the entire skill or agent, silently. In orbit-ui-mobile that had disabled five skills and the security-reviewer agent, so /pr-review was running with no security pass. This repo's own security-reviewer copy was hand fixed on the #434 branch, but nothing stopped a recurrence.

• tools/check-frontmatter.mjs, vendored from orbit-ui-mobile main (landed there as PR #594). Dependency free, as the upstream copy is.
• New Skill and Agent Frontmatter job in guards.yml, alongside the Dash Ban. It runs on pull requests targeting main and on push to main. Since this repo has no Node toolchain and no package.json, the job sets Node up itself (actions/setup-node@v7, node 22) and calls the script directly. No package.json, no npm script, no dependency was added.
• Roots are the three directories that actually exist here: .claude/agents, .claude/skills, .opencode/agents.

Diff against the upstream copy, so the divergence is auditable: the parser, the offender detection, the --fix rewrite and the output are byte-identical. Three intentional differences only, all in this file: the roots above, the repo-specific header comment, and a declared root that does not exist being an error rather than a quiet skip. All three roots are tracked, so a missing one means the gate has become a no op, and a verifier that cannot look must never report clean. Nothing else was inherited or dropped.

Proof, run before committing:

case	result
real orbit-api tree	frontmatter ok: 8 skill and agent files parse, exit 0
scratch copy, security-reviewer reverted to the broken unquoted form	exit 1, names .claude/agents/security-reviewer.md [description]
same scratch copy, --fix	rewrites to a block scalar, re-run exits 0
scratch copy with .opencode/agents deleted	exit 1, "roots missing, so the gate cannot verify anything"

The job then ran green on this PR in real CI.

2. Refresh rubric.md dimension 8 against the post #539 DESIGN.md (section 14, item 3)

Dimension 8 still sanctioned "the sanctioned gradient-header" and asked reviewers to flag until the scene sentence named Orbit's "navy-cosmic, violet-glow character". Both were deleted by the design freeze of 2026-07-17. It now states the de decorated navy violet anchor: no decorative glow and no gradient wash anywhere, identity carried by the orbital logo, the Astra glyph and ring indicators, --primary as fill and graphic only with --primary-soft as accent text, plus accent rationing and the current AI slop tell list. References cite DESIGN.md section names as well as line numbers, since the line numbers are exactly what rotted.

It also names the gate versus reviewer split, so the dimension stops asking a human to re-flag what an ESLint local/* rule already fails on.

Preamble claims, verified rather than assumed. Both were false in this repo:

• "shared verbatim by two skills, /pr-review and /audit-code-quality" is wrong here. .claude/skills/ in orbit-api holds only pr-review, second-opinion and _shared. The only readers of the rubric in this repo are .claude/skills/pr-review/SKILL.md (lines 94 and 106) and .github/workflows/claude-review.yml (line 32), and the workflow only invokes the skill.
• "there is no second copy" is wrong. orbit-ui-mobile/.claude/skills/pr-review/rubric.md exists, and it is not even byte identical: the api copy carries four extra lines under dimension 13 for the ORBIT0002 transaction teardown rule.

The preamble now describes the two copies as hand aligned lockstep twins with backend only divergences sanctioned, and says a drift is a defect.

3. Scope GITHUB_TOKEN in arch-map.yml (CodeQL alert 10)

arch-map.yml declared no permissions block, so GITHUB_TOKEN ran with the repository default (actions/missing-workflow-permissions, medium). Its steps are checkout, setup-node, node tools/arch-map.mjs, git diff, so contents: read is exactly the scope it needs.

The other eight workflows were audited for the same gap rather than assuming the one CodeQL surfaced was the only one. All eight already declare an explicit top-level block, and each grants only what its jobs use: claude-review needs pull-requests: write plus id-token: write for the review post and OIDC, dependabot-auto-merge needs contents: write plus pull-requests: write to merge, the remaining six are contents: read. No job needed a per-job widening, so the class closes with one edit.

4. Drop the unread client_id on the OAuth token endpoint

OAuthController.Token bound [FromForm] string? client_id and never read it: not passed to ExchangeCode, never compared to entry.ClientId. Dead code under standard 2, and an incomplete client-identity check under RFC 6749 section 4.1.3.

Dropped rather than wired, for two reasons:

1. Nothing in the repo can establish that wiring it is safe for the live Claude.ai client. The parameter has been nullable and unread since the flow's first commit (13f8a29), no test or log records what the real client sends at the token endpoint, and entry.ClientId is only used in a Debug log that prod filters out. Wiring it would be an unverifiable behaviour change on the one live consumer.
2. On the merits it would not add a control. /oauth/register returns Guid.NewGuid() and persists nothing, and SupportedTokenEndpointAuthMethods advertises none, so the value captured at authorize time and the value presented at token time are both unverified client assertions. Comparing two strings the same party chose proves nothing about the caller.

The real client binding is untouched: PKCE S256, the exact ordinal redirect_uri match, the atomic single-use TryRemove and the 5 minute expiry, all in OAuthAuthorizationStore.ExchangeCode. Callers that still send client_id are unaffected, since an unbound form field is ignored. The reasoning is recorded as XML doc on the action so the parameter cannot be re-added as dead weight.

Two tests added: the endpoint takes no client_id and still issues a token, and a wrong code_verifier is rejected as invalid_grant with no key created. openapi.json regenerated. The CI's own tool was run locally against origin/main's spec, tufin/oasdiff breaking --fail-on ERR: 0 errors, 1 warning (request-body-all-of-removed), so the OpenAPI Breaking-Change Gate passes. It then passed in CI.

Also fixed while here: tools/arch-map.mjs walked bin/ and obj/, so any local dotnet build made the generated map pick up build output and report false drift. CI never builds before regenerating, so CI never saw it. The walker now skips both.

5. The Dash Ban gate failed on Dependabot PRs

The PR title and body carry no dashes step fed the raw PR body to the checker with no author scoping. Dependabot copies the dependency's own changelog verbatim into the body, so an upstream release note carrying an en or em dash failed a gate on text Orbit neither wrote nor can edit, and which Dependabot regenerates on every rebase. #425, a 21 package NuGet security bump, was red on exactly that: a Hangfire release note's en dashes.

Only the prose step is scoped to github.event.pull_request.user.type != 'Bot', since its whole premise is that the text is Orbit-authored. The file-level check and the baseline ratchet still run on bot PRs, because the files a Dependabot PR changes ARE Orbit's. Byte-identical to the fix already landed in orbit-ui-mobile (896ff118) and orbit-landing-page, so the three copies stay in lockstep.

Validation

• dotnet build Orbit.slnx: 0 errors.
• dotnet test Orbit.slnx: 5573 passed, 0 failed.
• node tools/check-frontmatter.mjs, check-dashes --check-baseline, --files, and the node tools/arch-map.mjs drift check: all clean.
• dotnet format --verify-no-changes on both changed C# files: clean.

Scope

orbit-api only. The orbit-ui-mobile copy of rubric.md is owned by a separate PR there.

Two notes for the human

• Required checks. Neither Guards job is a required context on main today, so a red Dash Ban or Skill and Agent Frontmatter does not block a merge, and a gate that does not block is not a gate. Both should become required after this PR lands: Skill and Agent Frontmatter does not exist on main until then, so adding it now would block every open PR. Not changed here; branch protection is yours to set.
• Upstream. The missing-root-is-an-error divergence is arguably right for orbit-ui-mobile too, where the same script quietly skips a missing root. Not touched from here, since other agents are active in that repo.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[claude]

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

Recommendation: APPROVE

Summary

Five focused fixes: vendors a YAML-frontmatter parse gate, refreshes rubric.md dimension 8, scopes arch-map.yml's GITHUB_TOKEN, drops an unread client_id form parameter from the OAuth token endpoint (with two new tests + regenerated OpenAPI spec), and scopes the Dash Ban PR-body check away from bot-authored PRs. No prior reviews exist on this PR, so this covers the full diff.

Findings

Critical / High / Medium

None.

Low / Info

[Info] check-frontmatter.mjs's clean-run count includes files with no frontmatter to check

• tools/check-frontmatter.mjs:53 — targets includes files like rubric.md / _shared/verification-protocol.md that have no --- frontmatter block at all, so the success message overcounts how many files actually "parse". Purely cosmetic (never a false negative); optional to fix.

Security pass — OAuth client_id removal

Verified directly (and independently confirmed by a security-reviewer subagent):

• OAuthController.Token drops [FromForm] string? client_id, which was bound but never used for any security decision (not passed to ExchangeCode, never compared to entry.ClientId; only use of entry.ClientId is a Debug-level log, filtered in prod).
• OAuthAuthorizationStore.ExchangeCode (unmodified) still enforces: PKCE S256 verification, exact redirect_uri match, atomic single-use code removal, and a 5-minute expiry.
• ASP.NET Core silently ignores unbound form fields, so removing this parameter is not a backward-compat break for any existing caller.
• [AllowAnonymous] and [DistributedRateLimit("auth")] are both untouched.
• openapi.json's removed client_id property matches only the /oauth/token request schema; the /oauth/authorize query-param client_id is correctly left alone.

Not verifiable in CI

• Cross-repo dimensions (contract-aligner, packages/shared) — orbit-ui-mobile sibling repo not checked out. Substantively N/A anyway: /oauth/token is Anthropic's Claude.ai OAuth client, not a shared-DTO mobile/web endpoint.
• Dimension 8 (DESIGN.md) — diff only edits rubric prose describing DESIGN.md, not actual UI code; can't verify cited anchors against the sibling file.

What's good

• The client_id removal is root-caused and documented via XML doc <remarks>, not silently dropped.
• check-frontmatter.mjs correctly treats a missing frontmatter root as an error rather than silently skipping.
• PR self-audits its own preamble claims (verified skill directory counts, verified sibling rubric.md diff) matching the review culture the rubric asks for.

Recommendation

Approve as-is. Nothing here blocks merge; the one Info-level note is optional polish, not a defect.

[claude]

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

Scope: PR #435, fix/phase6-followups → main
Recommendation: APPROVE

Summary

Eight files changed: a new tools/check-frontmatter.mjs gate + guards.yml job, an arch-map.yml permissions fix, an arch-map.mjs bin/obj-skip fix, a rubric.md refresh, and one real behavior change — dropping the dead, never-read client_id form parameter from OAuthController.Token. security-reviewer and contract-aligner independently verified the OAuth change against OAuthAuthorizationStore.ExchangeCode: ClientId was never compared server-side before or after, real client binding (PKCE S256, exact redirect_uri match, single-use code, 5-minute expiry) is untouched, and ASP.NET Core silently ignores extra unbound [FromForm] fields, so old callers (Claude.ai's MCP connector) that still send client_id are unaffected — no ⚠️ breaks old mobile clients. No Critical/High findings survived.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

[Info] PR title/body still claim item 2 ("correct stale phase numbers in claude-review.yml") but the diff doesn't touch that file.

• dimension: Correctness
• location: PR description, section "2. Correct the stale phase numbers in claude-review.yml"
• issue: Commit 6e11da5 ("chore(ci): move the claude-review.yml phase-number fix to its own PR"), already on this branch, deliberately reverted claude-review.yml and tools/dash-baseline.json back to main's version and split that fix into fix/claude-review-phase-numbers — because claude-code-action skips validation (silently "passes" with zero reviews) when a PR's own claude-review.yml differs from the default branch's. That's a sound call, but the PR body/title text wasn't updated afterward, so a reader following the description would believe item 2 shipped here.
• risk: Low — purely descriptive; no code impact.
• fix: Trim the "## 2." section from the PR body (or note it was split out) before merge/squash, since the squash message becomes the permanent commit record.

[Info] tools/check-frontmatter.mjs --fix matches key: by line prefix repo-wide, not scoped to the frontmatter block.

• dimension: Dead/stale code, defense-in-depth
• location: tools/check-frontmatter.mjs:83
• issue: --fix rewrites the first line in the whole file starting with "<key>: ", not the first line inside the detected ---…--- block.
• risk: Very low — --fix is a manual, local-only convenience not invoked by CI, and requires an exact-prefix collision.
• fix: Scope the findIndex search to the slice of lines between the two --- markers.

Both Low/Info items are non-blocking and not posted as inline PR comments per the rubric's signal gate.

Subagents

Agent	Verdict
security-reviewer	PASS — no findings; client_id removal, guards.yml bot-scoping, arch-map.yml permissions, and the new frontmatter job all reviewed clean
contract-aligner	MATCH (local: openapi.json ⇄ OAuthController.cs 1:1) / NOT VERIFIABLE (cross-repo: no orbit-ui-mobile checkout to compare @orbit/shared Zod schemas — low risk since this endpoint's consumer is Claude.ai's MCP connector, not the mobile app)

Validation

Build/test skipped per this PR's CI adaptation (Build / Unit Tests / SonarCloud run as separate required checks).

What's good

• The client_id removal is a textbook root-cause fix: verified dead end-to-end, documented with an XML-doc <remarks> explaining why removal is safe, backed by two new unit tests, and a regenerated oasdiff-verified openapi.json.
• guards.yml's Dependabot-body-text carve-out is correctly scoped, not a blanket bot exemption.
• check-frontmatter.mjs's "missing root = hard error" design is right for a verifier.
• arch-map.yml permissions fix closes a real CodeQL-flagged gap, with a proactive audit of the other eight workflows.

Recommendation

Approve as-is. The stale "item 2" language in the PR title/body is worth a quick edit before squash-merge (squash message becomes permanent history), but it's cosmetic and not blocking.