diff --git a/.claude/skills/pr-review/rubric.md b/.claude/skills/pr-review/rubric.md index 46c6bc4f..154bdda3 100644 --- a/.claude/skills/pr-review/rubric.md +++ b/.claude/skills/pr-review/rubric.md @@ -1,12 +1,22 @@ # Orbit Review Rubric -The single source of truth for what a review checks, shared verbatim by two skills: -`/pr-review` walks it dimension-by-dimension over a **diff**, and `/audit-code-quality` -(#228) walks it over the **whole repo**. Both read this one file -(`.claude/skills/pr-review/rubric.md`) — there is no second copy, so the two can never -drift. 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. +The single source of truth for what a review checks **in this repo**. Exactly two things +read it: `/pr-review` (`.claude/skills/pr-review/SKILL.md`), which walks it +dimension-by-dimension over a **diff**, and `.github/workflows/claude-review.yml`, which +invokes that same skill in CI and maintains no criteria of its own. Unlike +orbit-ui-mobile, this repo ships no `/audit-code-quality` skill, so nothing here walks the +rubric over the whole repo. + +**A second copy exists**, at `orbit-ui-mobile/.claude/skills/pr-review/rubric.md`, where +`/audit-code-quality` also walks it. Two repos and two CIs mean the file cannot be +deduped, so the copies are lockstep twins kept aligned by hand, exactly like the two +`pr-review/SKILL.md` copies. Sanctioned divergences are backend-only material, such as +dimension 13's transaction-teardown bullet (`ORBIT0002`). 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` line, an orbit-api hard rule, or a security category) so the author can @@ -64,7 +74,7 @@ Every finding, every dimension, the same shape: · issue: <1-2 sentences — what is wrong> · risk: <1-2 sentences — what goes wrong if it ships> · fix: -· reference: +· reference: ``` --- @@ -164,30 +174,71 @@ well-named function** so the code reads without prose. ### 8. DESIGN.md / AI-slop -> Reference: DESIGN.md:181 (AI-slop tells), DESIGN.md:185 (scene-sentence test), -> DESIGN.md:159-169 (bans). **Gated: only when the diff touches `apps/*` UI files.** - -Scan for the AI-slop tells from `DESIGN.md:181`: - -- Gradients used as decoration outside the sanctioned gradient-header. -- Cards-in-cards (opaque card-on-card on dark). -- Gray text on colored backgrounds. -- Rounded-square icon tiles above headings. -- Semantic-red destructive fills where the artboard shows a text pill. -- Oversized centered H1 outside hero contexts; decorative gradient borders. - -Token / ban checks (`DESIGN.md:159-169`): - -- No raw `--slate-*` references or hardcoded violet rgba — semantic tokens only - (`--primary` / `--primary-rgb` / `tintFromPrimary`). -- No `transition-all` (animate `transform` / `opacity`, named); no `h-screen` (use +> 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 in CI the file is not checked out at all, so it +> is "not verifiable in CI". + +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 branches — schemes resolve through tokens. -- No em dashes in user-facing copy. - -Then the **scene-sentence test** (`:185`): describe the rendered screen in one sentence. -If it reads like every other SaaS app, it is generic — flag it to rework until the -sentence names Orbit's navy-cosmic, violet-glow character. +- 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) diff --git a/.github/workflows/arch-map.yml b/.github/workflows/arch-map.yml index 5d474ee0..4ddede73 100644 --- a/.github/workflows/arch-map.yml +++ b/.github/workflows/arch-map.yml @@ -5,6 +5,9 @@ on: push: branches: [main] +permissions: + contents: read + jobs: drift: runs-on: ubuntu-latest diff --git a/.github/workflows/guards.yml b/.github/workflows/guards.yml index 31df83aa..6ec0bd7b 100644 --- a/.github/workflows/guards.yml +++ b/.github/workflows/guards.yml @@ -1,9 +1,15 @@ name: Guards -# Vendored from orbit-ui-mobile (REBUILD.md 6.1.1): the cross-repo dash ban. -# The old session-only hook matched /Email/ and ResendEmailService.cs only, so -# 71 tracked .cs files carried dashes ungated (a confirmed audit gap). This job -# holds for every tool because CI is the one layer nothing bypasses. +# Cheap repo-hygiene gates vendored from orbit-ui-mobile, one job each. They +# hold for every tool because CI is the one layer nothing bypasses. +# +# Dash Ban (REBUILD.md 6.1.1): the old session-only hook matched /Email/ and +# ResendEmailService.cs only, so 71 tracked .cs files carried dashes ungated (a +# confirmed audit gap). +# +# Skill and Agent Frontmatter (REBUILD.md section 14, item 1): an unquoted YAML +# value containing ": " drops the skill or agent silently, which is how this +# repo's security-reviewer stopped loading. on: pull_request: @@ -34,8 +40,24 @@ jobs: - name: Baseline only shrinks run: node tools/check-dashes.mjs --check-baseline - name: PR title and body carry no dashes - if: github.event_name == 'pull_request' + # Human-authored prose only. A bot PR body is machine-copied upstream text + # (Dependabot quotes the dependency's own changelog verbatim), which Orbit + # neither wrote nor can edit, and which Dependabot regenerates on every + # rebase. The file-level dash check above still runs on bot PRs, because + # the files they change ARE Orbit's. See orbit-api#425, where a Hangfire + # release note's en dashes blocked a security-relevant NuGet bump. + if: github.event_name == 'pull_request' && github.event.pull_request.user.type != 'Bot' env: PR_TITLE: ${{ github.event.pull_request.title }} PR_BODY: ${{ github.event.pull_request.body }} run: node tools/check-dashes.mjs --text "$PR_TITLE $PR_BODY" + + frontmatter: + name: Skill and Agent Frontmatter + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-node@v7 + with: + node-version: 22 + - run: node tools/check-frontmatter.mjs diff --git a/src/Orbit.Api/Controllers/OAuthController.cs b/src/Orbit.Api/Controllers/OAuthController.cs index fb859ef9..d2759435 100644 --- a/src/Orbit.Api/Controllers/OAuthController.cs +++ b/src/Orbit.Api/Controllers/OAuthController.cs @@ -280,6 +280,18 @@ await ConcurrencyRetry.SaveWithRetryAsync( return Result.Success(user); } + /// + /// Exchanges an authorization code for an Orbit API key. + /// + /// + /// A client_id form field is accepted by the protocol and ignored here. This server is a + /// public-client authorization server (token_endpoint_auth_method: none) whose + /// /oauth/register hands out an unpersisted random identifier, so a client_id presented + /// at this endpoint is an unverified, client-asserted string and matching it against the equally + /// client-asserted value captured at authorize time proves nothing about the caller. Client binding + /// comes from PKCE S256, the exact redirect_uri match, single-use codes, and the 5-minute expiry, all + /// enforced in . + /// [HttpPost("/oauth/token")] [DistributedRateLimit("auth")] [Consumes("application/x-www-form-urlencoded")] @@ -287,7 +299,6 @@ public async Task Token( [FromForm] string grant_type, [FromForm] string code, [FromForm] string code_verifier, - [FromForm] string? client_id, [FromForm] string redirect_uri, CancellationToken ct) { diff --git a/src/Orbit.Api/openapi.json b/src/Orbit.Api/openapi.json index c33cdfd0..baaa6ed1 100644 --- a/src/Orbit.Api/openapi.json +++ b/src/Orbit.Api/openapi.json @@ -8814,14 +8814,6 @@ } } }, - { - "type": "object", - "properties": { - "client_id": { - "type": "string" - } - } - }, { "type": "object", "properties": { diff --git a/tests/Orbit.Infrastructure.Tests/Controllers/OAuthControllerTests.cs b/tests/Orbit.Infrastructure.Tests/Controllers/OAuthControllerTests.cs index c3b5878c..61181a36 100644 --- a/tests/Orbit.Infrastructure.Tests/Controllers/OAuthControllerTests.cs +++ b/tests/Orbit.Infrastructure.Tests/Controllers/OAuthControllerTests.cs @@ -537,7 +537,7 @@ public async Task Token_UnsupportedGrantType_ReturnsBadRequest() { var result = await _controller.Token( "client_credentials", "code-abc", "verifier-xyz", - "client-123", "https://claude.ai/callback", CancellationToken.None); + "https://claude.ai/callback", CancellationToken.None); var bad = result.Should().BeOfType().Subject; var json = JsonSerializer.Serialize(bad.Value); @@ -549,7 +549,7 @@ public async Task Token_InvalidCode_ReturnsBadRequest() { var result = await _controller.Token( "authorization_code", "nonexistent-code", "verifier-xyz", - "client-123", "https://claude.ai/callback", CancellationToken.None); + "https://claude.ai/callback", CancellationToken.None); var bad = result.Should().BeOfType().Subject; var json = JsonSerializer.Serialize(bad.Value); @@ -561,7 +561,7 @@ public async Task Token_InvalidRedirectUri_ReturnsBadRequest() { var result = await _controller.Token( "authorization_code", "nonexistent-code", "verifier-xyz", - "client-123", "https://evil.com/callback", CancellationToken.None); + "https://evil.com/callback", CancellationToken.None); var bad = result.Should().BeOfType().Subject; JsonSerializer.Serialize(bad.Value).Should().Contain("invalid_redirect_uri"); @@ -580,7 +580,7 @@ public async Task Token_ValidCodeExchange_ReturnsAccessToken() var result = await _controller.Token( "authorization_code", authCode, codeVerifier, - "client-123", "https://claude.ai/callback", CancellationToken.None); + "https://claude.ai/callback", CancellationToken.None); var ok = result.Should().BeOfType().Subject; var json = JsonSerializer.Serialize(ok.Value); @@ -589,6 +589,39 @@ public async Task Token_ValidCodeExchange_ReturnsAccessToken() await _apiKeyRepo.Received(1).AddAsync(Arg.Any(), Arg.Any()); } + [Fact] + public async Task Token_TakesNoClientId_AndBindsOnPkceAndRedirectUriInstead() + { + var (verifier, challenge) = GeneratePkce(); + var code = _authStore.CreateCode(UserId, challenge, "https://claude.ai/callback", "client-registered-at-authorize"); + + var result = await _controller.Token( + "authorization_code", code, verifier, + "https://claude.ai/callback", CancellationToken.None); + + result.Should().BeOfType(); + + typeof(OAuthController).GetMethod(nameof(OAuthController.Token))! + .GetParameters().Select(p => p.Name) + .Should().NotContain("client_id"); + } + + [Fact] + public async Task Token_WrongCodeVerifier_ReturnsInvalidGrant() + { + var (_, challenge) = GeneratePkce(); + var (otherVerifier, _) = GeneratePkce(); + var code = _authStore.CreateCode(UserId, challenge, "https://claude.ai/callback", "client-123"); + + var result = await _controller.Token( + "authorization_code", code, otherVerifier, + "https://claude.ai/callback", CancellationToken.None); + + var bad = result.Should().BeOfType().Subject; + JsonSerializer.Serialize(bad.Value).Should().Contain("invalid_grant"); + await _apiKeyRepo.DidNotReceive().AddAsync(Arg.Any(), Arg.Any()); + } + [Fact] public async Task Token_ValidExchange_CreatesReadWriteClaudeKeyWithDefaultScopes() { @@ -601,7 +634,7 @@ public async Task Token_ValidExchange_CreatesReadWriteClaudeKeyWithDefaultScopes var result = await _controller.Token( "authorization_code", code, verifier, - "client-123", "https://claude.ai/callback", CancellationToken.None); + "https://claude.ai/callback", CancellationToken.None); result.Should().BeOfType(); created.Should().NotBeNull(); @@ -632,7 +665,7 @@ public async Task Token_ValidExchange_RevokesPriorClaudeKeysBeforeIssuingNew() var result = await _controller.Token( "authorization_code", code, verifier, - "client-123", "https://claude.ai/callback", CancellationToken.None); + "https://claude.ai/callback", CancellationToken.None); result.Should().BeOfType(); priorKey.IsRevoked.Should().BeTrue(); @@ -712,7 +745,7 @@ public async Task Token_WithAttackerDomainRedirectUri_ReturnsBadRequest() { var result = await _controller.Token( "authorization_code", "any-code", "verifier-xyz", - "client-123", "https://attacker.com/callback", CancellationToken.None); + "https://attacker.com/callback", CancellationToken.None); var bad = result.Should().BeOfType().Subject; var json = JsonSerializer.Serialize(bad.Value); @@ -865,7 +898,7 @@ public async Task Token_WithNonce_EchoesNonceInResponse() var result = await _controller.Token( "authorization_code", code, verifier, - "client-123", "https://claude.ai/callback", CancellationToken.None); + "https://claude.ai/callback", CancellationToken.None); var ok = result.Should().BeOfType().Subject; var json = JsonSerializer.Serialize(ok.Value); @@ -882,7 +915,7 @@ public async Task Token_WithoutNonce_OmitsNonceFromResponse() var result = await _controller.Token( "authorization_code", code, verifier, - "client-123", "https://claude.ai/callback", CancellationToken.None); + "https://claude.ai/callback", CancellationToken.None); var ok = result.Should().BeOfType().Subject; JsonSerializer.Serialize(ok.Value).Should().NotContain("nonce"); @@ -904,7 +937,7 @@ public async Task VerifyCode_WithNonce_BindsNonceRetrievableAtTokenExchange() var tokenResult = await _controller.Token( "authorization_code", code, verifier, - "client-123", "https://claude.ai/callback", CancellationToken.None); + "https://claude.ai/callback", CancellationToken.None); var ok = tokenResult.Should().BeOfType().Subject; JsonSerializer.Serialize(ok.Value).Should().Contain("verify-nonce-7"); @@ -931,7 +964,7 @@ public async Task GoogleAuth_WithNonce_BindsNonceRetrievableAtTokenExchange() var tokenResult = await _controller.Token( "authorization_code", code, verifier, - "client-123", "https://claude.ai/callback", CancellationToken.None); + "https://claude.ai/callback", CancellationToken.None); var ok = tokenResult.Should().BeOfType().Subject; JsonSerializer.Serialize(ok.Value).Should().Contain("google-nonce-42"); diff --git a/tools/arch-map.mjs b/tools/arch-map.mjs index 696239f4..b9c9aeed 100644 --- a/tools/arch-map.mjs +++ b/tools/arch-map.mjs @@ -10,11 +10,14 @@ import { fileURLToPath } from "node:url"; const repoRoot = join(dirname(fileURLToPath(import.meta.url)), ".."); +const BUILD_OUTPUT_DIRS = new Set(["bin", "obj"]); + function walk(dir, out = []) { for (const name of readdirSync(dir).sort()) { const full = join(dir, name); - if (statSync(full).isDirectory()) walk(full, out); - else if (name.endsWith(".cs")) out.push(full); + if (statSync(full).isDirectory()) { + if (!BUILD_OUTPUT_DIRS.has(name)) walk(full, out); + } else if (name.endsWith(".cs")) out.push(full); } return out; } diff --git a/tools/check-frontmatter.mjs b/tools/check-frontmatter.mjs new file mode 100644 index 00000000..ce7f80d5 --- /dev/null +++ b/tools/check-frontmatter.mjs @@ -0,0 +1,102 @@ +#!/usr/bin/env node +/** + * Gate: every skill and agent definition must have parseable YAML frontmatter. + * + * An unquoted YAML scalar containing ": " (colon followed by whitespace) breaks + * the parse. The file then loads with NO description, or does not load at all, + * and nothing reports an error. Measured 2026-07-24 in orbit-ui-mobile: this + * silently disabled five skills and the `security-reviewer` agent, which is one + * of the subagents `/pr-review` orchestrates, so its security pass had been a + * no-op. This repo's own `security-reviewer` carried the same defect and was + * hand-fixed; this gate is what stops the ninth recurrence. + * + * The fix is always the same: make the value a folded block scalar. + * + * description: >- + * Text that: contains a colon. + * + * Vendored from orbit-ui-mobile/tools/check-frontmatter.mjs. This repo has no + * Node toolchain and no package.json, so the script stays dependency-free and + * CI installs Node itself. One deliberate divergence from the upstream copy: a + * declared root that does not exist is an error here rather than a quiet skip. + * All three roots are tracked directories, so a missing one means the gate has + * become a no-op, and a verifier that cannot look must never report clean. + * + * Usage: node tools/check-frontmatter.mjs [--fix] + * Exit 0 clean, 1 on any offender. + */ + +import fs from 'node:fs'; +import path from 'node:path'; + +const ROOTS = ['.claude/agents', '.claude/skills', '.opencode/agents']; +const BLOCK_SCALAR = /^[>|][-+]?\d*$/; + +const missingRoots = ROOTS.filter((root) => !fs.existsSync(root)); +if (missingRoots.length > 0) { + console.error(`Frontmatter roots missing, so the gate cannot verify anything:`); + for (const root of missingRoots) console.error(` ${root}`); + console.error('\nRun this from the orbit-api repo root. If a root was deliberately'); + console.error('removed, delete it from ROOTS in this script in the same change.'); + process.exit(1); +} + +const targets = []; +for (const root of ROOTS) { + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) walk(full); + else if (entry.name.endsWith('.md')) targets.push(full); + } + }; + walk(root); +} + +const offenders = []; +for (const file of targets) { + const text = fs.readFileSync(file, 'utf8'); + const match = text.match(/^---\r?\n([\s\S]*?)\r?\n---/); + if (!match) continue; + for (const raw of match[1].split(/\r?\n/)) { + const line = raw.endsWith('\r') ? raw.slice(0, -1) : raw; + const kv = line.match(/^([A-Za-z][\w-]*):[ \t](.*)$/); + if (!kv) continue; + const value = kv[2].trim(); + if (!value) continue; + if (BLOCK_SCALAR.test(value)) continue; + if (/^"[^"]*"$/.test(value) || /^'[^']*'$/.test(value)) continue; + if (/^\[.*\]$/.test(value) || /^\{.*\}$/.test(value)) continue; + if (/:[ \t]/.test(value)) offenders.push({ file, key: kv[1], value }); + } +} + +if (offenders.length === 0) { + console.log(`frontmatter ok: ${targets.length} skill and agent files parse`); + process.exit(0); +} + +if (process.argv.includes('--fix')) { + const byFile = new Map(); + for (const o of offenders) byFile.set(o.file, [...(byFile.get(o.file) ?? []), o]); + for (const [file, items] of byFile) { + const text = fs.readFileSync(file, 'utf8'); + const eol = text.includes('\r\n') ? '\r\n' : '\n'; + const lines = text.split(/\r?\n/); + for (const { key, value } of items) { + const index = lines.findIndex((line) => line.startsWith(`${key}: `)); + if (index !== -1) lines.splice(index, 1, `${key}: >-`, ` ${value}`); + } + fs.writeFileSync(file, lines.join(eol)); + console.log(`fixed ${file}`); + } + process.exit(0); +} + +console.error(`Unparseable frontmatter in ${offenders.length} file(s).`); +console.error('A ": " inside an unquoted YAML value breaks the parse, so the skill or agent'); +console.error('loads with no description or does not load at all, silently.\n'); +for (const { file, key } of offenders) console.error(` ${file} [${key}]`); +console.error('\nFix: make the value a folded block scalar, or run:'); +console.error(' node tools/check-frontmatter.mjs --fix'); +process.exit(1);