Skip to content

Add shrink-only context budget gate - #622

Merged
thomasluizon merged 5 commits into
mainfrom
feature/orb-117-ship-the-always-loaded-context-budget
Jul 28, 2026
Merged

thomasluizon merged 5 commits into
mainfrom
feature/orb-117-ship-the-always-loaded-context-budget

Conversation

@thomasluizon

@thomasluizon thomasluizon commented Jul 28, 2026

Copy link
Copy Markdown
Owner

ORB-117

Adds a deterministic, dependency-free gate for the always-loaded context budget. The checker enforces a shrink-only byte total for repository-visible files, rejects additions to the root import and unconditional-rule allowlists, and reports resolvable sibling context without enforcing it.

The gate compares pull request measurements with the target branch baseline. CLAUDE.md and .claude/rules/*.md are pinned to LF so Windows baseline writes measure the same bytes Git and CI store.

Developer impact

Changes to root CLAUDE.md or unconditional .claude/rules/*.md files can no longer increase the committed byte budget silently. New root imports and unconditional rules also fail structurally even when their target content is unavailable in CI.

Seeded baseline

Final output from commit ccc1849f:

{
  "files": {
    "CLAUDE.md": 18864,
    ".claude/rules/core.md": 3380
  },
  "enforcedBytes": 22244,
  "estimatedTokens": 5561,
  "imports": [
    "@../orbit-api/CLAUDE.md",
    "@../orbit-landing-page/CLAUDE.md"
  ],
  "unconditionalRules": [
    ".claude/rules/core.md"
  ],
  "unexpectedImports": [],
  "unexpectedRules": [],
  "siblingFiles": [],
  "fullSessionBytes": 22244,
  "fullSessionEstimatedTokens": 5561,
  "baselineBytes": 22244,
  "baselineSource": "refs/remotes/origin/main (working tree bootstrap)",
  "deltaBytes": 0,
  "fileGrowth": [],
  "structuralFindings": [],
  "status": "ok"
}

Raw blob verification

$ git rev-parse HEAD:CLAUDE.md
3feb01627e13dfe6e6e42e69611e543638a58cf2
$ git cat-file -s $(git rev-parse HEAD:CLAUDE.md)
18864
$ git rev-parse HEAD:.claude/rules/core.md
009fac02b053fa0457b24ef214558f8a297746df
$ git cat-file -s $(git rev-parse HEAD:.claude/rules/core.md)
3380

The recorded file counts equal the raw Git blob sizes, and 18864 + 3380 = 22244.

Validation

$ npm run lint
Tasks: 3 successful, 3 total
Cached: 3 cached, 3 total
Exit code: 0

$ npm run type-check
Tasks: 3 successful, 3 total
Cached: 3 cached, 3 total
Exit code: 0

$ npm run test
@orbit/eslint-rules: 2 test files passed, 190 tests passed
@orbit/shared: 127 test files passed, 1599 tests passed
@orbit/web: 309 test files passed, 2550 tests passed
@orbit/mobile: 247 test files passed, 1611 tests passed
Tasks: 4 successful, 4 total
Cached: 4 cached, 4 total
Exit code: 0

$ node tools/test-tools.mjs
PASS check-context-budget.mjs: total over baseline exits 1 and names the offending file
PASS check-context-budget.mjs: total under baseline exits 0
PASS check-context-budget.mjs: an under-budget check does not rewrite context-budget.json
PASS check-context-budget.mjs: a regenerated working baseline cannot hide growth from the target branch
PASS check-context-budget.mjs: a first-run baseline bootstraps only when absent from the target branch
PASS check-context-budget.mjs: an unfetched target branch fails closed
PASS check-context-budget.mjs: all existing decision paths
ORBIT TOOLS GATE OK
Exit code: 0

$ node .claude/hooks/test-hooks.mjs
ORBIT HOOK PARITY OK
Exit code: 0

$ node tools/check-copy.mjs --check
Exit code: 0

$ node tools/check-frontmatter.mjs
frontmatter ok: 36 skill and agent files parse
Exit code: 0

$ node tools/check-dashes.mjs --files .gitattributes .github/workflows/guards.yml CLAUDE.md tools/README.md tools/test-tools.mjs tools/check-context-budget.mjs tools/context-budget.json
Exit code: 0

$ node tools/check-context-budget.mjs --check --json
{
  "files": {
    "CLAUDE.md": 18864,
    ".claude/rules/core.md": 3380
  },
  "enforcedBytes": 22244,
  "estimatedTokens": 5561,
  "imports": [
    "@../orbit-api/CLAUDE.md",
    "@../orbit-landing-page/CLAUDE.md"
  ],
  "unconditionalRules": [
    ".claude/rules/core.md"
  ],
  "unexpectedImports": [],
  "unexpectedRules": [],
  "siblingFiles": [],
  "fullSessionBytes": 22244,
  "fullSessionEstimatedTokens": 5561,
  "baselineBytes": 22244,
  "baselineSource": "refs/remotes/origin/main (working tree bootstrap)",
  "deltaBytes": 0,
  "fileGrowth": [],
  "structuralFindings": [],
  "status": "ok"
}
Exit code: 0

Decisions taken unattended

  • Stored the total byte baseline together with historical per-file counts. The verdict remains total-only and shrink-only, while failures identify which file grew.
  • Kept the import and unconditional-rule allowlists in the checker so a baseline write cannot silently approve structural growth.
  • Used the GitHub pull request base commit SHA in CI so the comparison is pinned to the actual target revision.
  • Local checks prefer origin/main, then main; if neither ref resolves, the tool exits 2 and requests target history.
  • Distinguished a missing target baseline from missing target history with git ls-tree. Only the former may use the working tree baseline for first-run bootstrap.
  • Added explicit LF attributes for root CLAUDE.md and .claude/rules/*.md, then freshly restored the measured files before running --write-baseline.
  • Regenerated tools/context-budget.json through the tool rather than editing its numbers by hand.
  • Left all review threads unresolved and posted no replies, as required by the review-round work order.

@vercel

vercel Bot commented Jul 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
orbit-ui-mobile-web Ignored Ignored Jul 28, 2026 3:16am

Request Review

@thomasluizon
thomasluizon marked this pull request as ready for review July 28, 2026 01:05

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d55a789acd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tools/check-context-budget.mjs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #622 — Add shrink-only context budget gate

Recommendation: REQUEST CHANGES

Summary

This PR adds a deterministic CI gate (tools/check-context-budget.mjs) enforcing a shrink-only byte budget over the always-loaded CLAUDE.md + .claude/rules/core.md, guards the root-import and unconditional-rules allowlists, wires a standalone Context Budget job into guards.yml, and adds nine hermetic decision-path tests. The tool's --check logic itself is sound and well-covered. The defect is in the baseline the PR commits alongside it: tools/context-budget.json does not match the actual byte size of the files it certifies — including .claude/rules/core.md, a file this PR never touches.

Findings

High

Seeded tools/context-budget.json does not match the actual byte counts of the files it certifies

  • Location: tools/context-budget.json:1 (baseline), CLAUDE.md, .claude/rules/core.md
  • The committed baseline records CLAUDE.md=18950, .claude/rules/core.md=3443 (total 22393). Verified directly via git cat-file -s against the PR's merged blobs: CLAUDE.md is actually 18864 bytes and core.md is actually 3380 bytes (total 22244) — 149 bytes lower than the seeded baseline.
  • core.md is untouched by this PR (git diff between the pre-PR base and the PR commit shows zero changes to that file), yet its baseline entry is inflated by exactly 63 bytes — which exactly matches the file's own line count (63 lines via wc -l), the signature of one stray \r per line. Since the file's content is unchanged, the only explanation is that the baseline was seeded from a CRLF-materialized local checkout.
  • .gitattributes has no text eol=lf rule for CLAUDE.md or .claude/rules/*.md (only .claude/workorders/*.md and a few others are covered), so a --write-baseline run on a CRLF checkout silently inflates every measured byte count — the exact class of bug this repo has hit before (per the .gitattributes comment about CRLF materializing on a fresh Windows clone).
  • Risk: CI checks out on Linux with no CRLF conversion, so the real enforced measurement (22244) is below the inflated baseline (22393) — the check passes today, but only by accident, with ~149 bytes of unearned, undocumented slack baked into the exact commit whose entire purpose is byte-for-byte shrink-only precision. Any future growth up to that slack goes undetected, so the shrink-only guarantee is false from the seed forward.
  • Fix: Add CLAUDE.md text eol=lf and .claude/rules/*.md text eol=lf to .gitattributes (matching the existing .claude/workorders/*.md precedent), then re-run node tools/check-context-budget.mjs --write-baseline from an LF-normalized checkout and recommit tools/context-budget.json with the corrected numbers (CLAUDE.md 18864, core.md 3380, total 22244) before merge.

What's good

  • check-context-budget.mjs's --check logic (allowlist checks, delta computation, JSON/human output, malformed-baseline handling) is clean and thoroughly exercised by nine well-targeted hermetic decision-path tests.
  • The context:reseed escape hatch mirrors the repo's existing ratchet:reseed / parity:exempt pattern.
  • Docs kept in lockstep: tools/README.md and the CLAUDE.md docs registry both updated in the same commit.

Deferred — not applicable to this diff

No apps/web/**, apps/mobile/**, packages/shared/src/types/*, endpoints.ts, or orbit-api files changed, so design/parity/i18n/contract/security/backend-hard-rules dimensions don't apply. Lint/type/test are covered by this PR's own required CI checks (Build / Unit Tests / SonarCloud), per this workflow's Phase-6 skip.

Severity Count
Critical 0
High 1
Medium 0
Low / Info 0

Comment thread tools/context-budget.json

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #622 — Add shrink-only context budget gate

Scope: PR #622 in thomasluizon/orbit-ui-mobile (ORB-117)
Recommendation: NEEDS WORK

Summary

This PR ships tools/check-context-budget.mjs, a new CI gate that enforces a shrink-only
byte budget on the always-loaded context surface (root CLAUDE.md + unconditional
.claude/rules/*.md), plus structural allowlists for sibling @ imports and unconditional
rule files, wiring into guards.yml, tools/test-tools.mjs, tools/README.md, and root
CLAUDE.md's own docs. The tool's logic, git-based target-branch comparison (fixed in the
second commit to compare against the actual PR base SHA instead of a mutable branch/local
file), and the eleven hermetic decision-path tests are sound and well covered. However, the
seeded baseline this PR commits, the entire deliverable of "seed the baseline", does not
match the actual byte count of the files it describes, verified directly against the
committed git blobs. This is a Correctness/Harness-evidence defect that blocks merge; see
High below. No parity, i18n, contract, design, or backend-hard-rule surfaces are touched
(tooling/CI-only diff), so those dimensions are N/A.

Findings

Critical

None.

High

[HIGH] Committed context-budget baseline does not match the files it claims to measure
· dimension: Correctness (#1), reinforced by Harness changes need EXECUTED evidence (#15)
· location: orbit-ui-mobile/tools/context-budget.json:1-7 (and the "Seeded baseline" block in the PR description)
· issue: The committed baseline records CLAUDE.md: 18950 bytes and .claude/rules/core.md: 3443 bytes
(total 22393), but the actual blobs committed in this same PR are 18864 and 3380 bytes
(total 22244), a 149-byte overstatement. Verified independently two ways against the raw git
objects (bypassing any working-tree/checkout filter):
git rev-parse 71d8bc3:CLAUDE.md -> 3feb01627e13dfe6e6e42e69611e543638a58cf2
git cat-file -s 3feb01627e13dfe6e6e42e69611e543638a58cf2 -> 18864
git rev-parse 71d8bc3:.claude/rules/core.md -> 009fac02b053fa0457b24ef214558f8a297746df
git cat-file -s 009fac02b053fa0457b24ef214558f8a297746df -> 3380
git diff d55a789a 71d8bc3a -- CLAUDE.md .claude/rules/core.md tools/context-budget.json is empty,
so the mismatch isn't later-commit drift within the PR, the baseline was stale from the very
commit (d55a789a) that both edited CLAUDE.md and seeded the baseline in the same commit.
An independent skeptic pass (fresh subagent, told to try to refute this) confirmed the same
numbers via the same blob-hash method and could not find an alternate-path or
BOM/CRLF/tool-semantics explanation, check-context-budget.mjs's byteCount() is a plain
readFileSync(path).byteLength on CLAUDE.md and .claude/rules/core.md only, with no
normalization that could account for 149 bytes.
· risk: The gate is shrink-only (fails only when measured bytes exceed the baseline). Because the
seeded baseline is 149 bytes higher than the true byte count, every future PR gets up to 149
bytes of real, unflagged growth in root CLAUDE.md + core.md before the Context Budget job
ever fires, precisely the "changes... can no longer increase the committed byte budget
silently" guarantee the PR's own "Developer impact" section claims to deliver. The pasted
"Seeded baseline" JSON and the "Review round 1 validation" --check --json transcript in the
PR body are themselves not accurate evidence of the final shipped state (execution happened,
but not against the content that actually merged), which is exactly what rubric dimension 15
exists to catch: a claim that a tool works, without a command that ran against the real
state, is a review finding.
· fix: Regenerate the baseline against the final committed content and commit the corrected
file: node tools/check-context-budget.mjs --write-baseline, which should produce
{"bytes": 22244, "files": {"CLAUDE.md": 18864, ".claude/rules/core.md": 3380}}. Update the
"Seeded baseline" block in the PR description to match, and re-paste a --check --json
transcript run against the final commit (not an intermediate draft).
· reference: CLAUDE.md rule 1 (root cause); rubric dimension 1 (Correctness) and dimension 15
(Harness changes need EXECUTED evidence).

Medium

None concretely actionable beyond the High above.

Low / Info

None posted (signal gate: Low/Info are not posted on a PR review).

Subagents

Agent Verdict
parity-checker N/A, no apps/web/** or apps/mobile/** file changed
i18n-syncer N/A, no user-facing strings or i18n JSON changed
contract-aligner N/A, no packages/shared/src/types/* / endpoints.ts / orbit-api DTO changed, and orbit-api is untouched
security-reviewer N/A, no orbit-api code changed
design-reviewer N/A, no apps/* or landing-page UI file changed

Validation

Check Result
Lint PASS (CI: Lint job, PR Tests, SUCCESS)
Type check PASS (CI: Type Check job, PR Tests, SUCCESS)
Tests PASS (CI: Unit Tests job, PR Tests, SUCCESS)
Build (api) N/A, orbit-api untouched
tools/test-tools.mjs + .claude/hooks/test-hooks.mjs PASS (CI: Harness Execution job, Guards, SUCCESS)
check-context-budget.mjs --check (this PR's own gate, against itself) PASS (CI: Context Budget job, Guards, SUCCESS) — expected given the baseline is higher than the true value (see High finding); a passing gate here does not contradict the finding

node execution was unavailable directly in this review sandbox, so validation above is
sourced from the PR's own live CI check-run results (gh pr view 622 --json statusCheckRollup)
rather than a local re-run; the High finding itself was verified independently via raw
git cat-file / git rev-parse (no node required), cross-checked by an adversarial skeptic
subagent.

Deferred — N/A dimensions & files not verdicted

  • Dimension 8 (DESIGN.md / AI-slop): N/A, no apps/* UI files touched.
  • Dimension 9 (Parity): N/A, no apps/web/** or apps/mobile/** files touched.
  • Dimension 10 (i18n): N/A, no locale JSON or user-facing strings touched.
  • Dimension 11 (Contract drift + backward-compat): N/A, no packages/shared/src/types/*,
    endpoints.ts, or orbit-api DTO touched; no backward-compat candidates in this diff.
  • Dimension 12 (Security): reviewed inline (no injection surface, spawnSync uses arg
    arrays, not shell strings; CONTEXT_BUDGET_BASE_REF cannot reach a shell); no findings.
  • Dimension 13 (Backend hard rules): N/A, orbit-api not touched.
  • Dimension 14 (FEATURES.md parity): N/A, no user-facing feature surface changed (internal
    CI tooling only).
  • Every changed file (guards.yml, CLAUDE.md, tools/README.md,
    tools/check-context-budget.mjs, tools/context-budget.json, tools/test-tools.mjs) was
    read in full and given a verdict; nothing left unexamined.

What's good

  • The git-based target-branch comparison (comparing against the PR's actual base SHA via
    git show <ref>:<path> / git ls-tree, not a local file or a mutable branch name) correctly
    defeats a local --write-baseline attempt to hide real growth from CI, confirmed by the
    dedicated test "a regenerated working baseline cannot hide growth from the target branch".
  • Fail-closed behavior throughout: an unresolvable target ref, a malformed baseline, and an
    unallowlisted import/rule file all exit non-zero rather than silently passing.
  • Strong, concrete hermetic test coverage: eleven distinct decision paths in
    tools/test-tools.mjs, matching the Harness Execution contract exactly (dimension 15
    satisfied on the coverage axis, independent of the baseline-data defect above).
  • Zero comments in the new script; all self-documenting via naming, clean under the strict
    comment policy.
  • spawnSync is used with argument arrays throughout (resolveTargetRef, readComparisonBaseline),
    not shell string interpolation, so CONTEXT_BUDGET_BASE_REF cannot become a command-injection
    vector even though it flows from a GitHub Actions expression into an env var.

Recommendation

Regenerate and commit an accurate tools/context-budget.json against the PR's final content
(node tools/check-context-budget.mjs --write-baseline), correct the PR description's "Seeded
baseline" and validation transcript to match, then this is ready to merge, the tool's logic,
CI wiring, and test coverage are otherwise solid.

@thomasluizon

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 9a10e8d6e3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #622 — Add shrink-only context budget gate

Scope: PR #622 in thomasluizon/orbit-ui-mobile (ORB-117)
Recommendation: APPROVE

Summary

This is a pure tooling/CI-infra PR: a new tools/check-context-budget.mjs gate that enforces a shrink-only byte budget for the always-loaded CLAUDE.md + unconditional .claude/rules/*.md files, plus a seeded baseline, a new Context Budget guards.yml job, an LF .gitattributes pin for the two enforced files, harness coverage for all decision paths, and doc updates (CLAUDE.md, tools/README.md). No apps/* or orbit-api files are touched, so the platform-parity, i18n, contract-drift, backend-security, DESIGN.md, and FEATURES.md dimensions are all N/A.

Two prior claude reviews on this PR (CHANGES_REQUESTED) both flagged the same High finding: the committed tools/context-budget.json baseline (18950/3443/22393) did not match the true byte count of the CRLF-free committed blobs (18864/3380/22244), a 149-byte overstatement traced to a CRLF-materialized local checkout used to seed the baseline. This review independently re-verified the current state: git show HEAD:tools/context-budget.json now records {"bytes": 22244, "CLAUDE.md": 18864, ".claude/rules/core.md": 3380}, and git show HEAD:CLAUDE.md | wc -c → 18864, git show HEAD:.claude/rules/core.md | wc -c → 3380 — an exact match. The .gitattributes LF pin for CLAUDE.md and .claude/rules/*.md is present in the diff, and the git log shows the two follow-up commits ("Fix context budget LF baseline", "Fix context budget base comparison") that resolved this. The High finding from both prior rounds is resolved; no new issues found.

Findings

Critical

None.

High

None. (Prior High — stale/CRLF-inflated baseline — is resolved; verified independently against the current committed blobs, see Summary.)

Medium

None.

Low / Info

  • tools/check-context-budget.mjs — the pass/fail verdict is deliberately total-only: a file that individually grows can still pass if another enforced file shrinks by an equal or greater amount. This is an explicit, PR-documented decision, not an oversight — flagging for visibility only.
  • The context:reseed label (guards.yml context-budget job) skips the CI check but doesn't itself force a --write-baseline commit; a reseeded PR that forgets to commit the new baseline leaves main's baseline stale. This mirrors the existing ratchet:reseed pattern in the same file, so it's consistent with established convention rather than a new gap.

Subagents

Agent Verdict
parity-checker N/A — no apps/web/** or apps/mobile/** file changed
i18n-syncer N/A — no user-facing strings or shared i18n JSON changed
contract-aligner N/A — no packages/shared/src/types/* / endpoints.ts, no both-repo change
security-reviewer N/A — no orbit-api code changed
design-reviewer N/A — no apps/* or landing-page UI file changed

Validation

Check Result
Lint / Type check / Tests PASS (this PR's own required CI checks — Build / Unit Tests / SonarCloud — per this workflow's Phase-6 skip)
Baseline accuracy (tools/context-budget.json vs actual blobs) PASS — independently re-verified in this session, exact match (18864 / 3380 / 22244)
Harness (tools/test-tools.mjs) PASS per PR body / prior review verification; test wiring for check-context-budget.mjs confirmed present and registered
Hooks (.claude/hooks/test-hooks.mjs) PASS per PR body

Deferred — N/A dimensions

  • Parity, i18n, contract drift/backward-compat, backend security/hard rules, DESIGN.md/AI-slop, FEATURES.md parity — all N/A, no apps/* or orbit-api files in the diff.

What's good

  • The bootstrap-vs-enforce split is correctly asymmetric: first-run reads the working-tree baseline, every subsequent check reads the baseline from the git target ref, so a contributor cannot defeat shrink-only enforcement via a local regeneration.
  • The .gitattributes LF pin for exactly the two enforced files is the right fix for the CRLF byte-count instability the prior review rounds caught, and it's independently confirmed correct against the committed blobs.
  • Structural findings (new root imports, new unconditional rules) are evaluated independently of shrink/grow and block even a --write-baseline run.
  • Harness coverage is thorough across the tool's decision paths.

Recommendation

Nothing blocks merge. Both prior High findings are resolved and independently reverified. The two Info-level notes above don't need action in this PR.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #622 — Add shrink-only context budget gate

Recommendation: APPROVE

Summary

Adds tools/check-context-budget.mjs, a deterministic, dependency-free CI gate enforcing a shrink-only byte budget for the always-loaded context (CLAUDE.md + unconditional .claude/rules/*.md), plus a structural allowlist for root @ imports and unconditional rule files. Wires it into guards.yml behind a context:reseed label escape hatch, pins CLAUDE.md and .claude/rules/*.md to LF via .gitattributes so a Windows checkout measures the same bytes CI does, seeds tools/context-budget.json, and adds thorough test-tools.mjs coverage. Traced line-by-line against tools/CONVENTIONS.md and the actual current .claude/rules/ state (which file is unconditional vs. paths:-scoped); no correctness defect found.

Findings

Critical / High / Medium

None.

Low / Info

--check doesn't itself flag a diff that removes tools/context-budget.json while the target branch still has one (tools/check-context-budget.mjs, readComparisonBaseline() / the --check path in main()). --check compares current bytes against the target branch's committed baseline; it never asserts the current branch still carries the file when the target has one. A PR that deletes tools/context-budget.json (bad merge/rebase, stray cleanup) would still pass as long as the byte delta is ≤ 0, silently merging the deletion. This is self-limiting though: the next PR against that main hits the bootstrap path, finds the file missing from both target and its own working tree, and readWorkingBaseline() correctly fails closed with exit 2. So the real blast radius is one merged PR, not a permanently disabled gate. Worth having --check name the deletion directly rather than relying on the next unrelated PR's failure to surface it — not blocking.

Subagents

All five (parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer) are N/A: the diff touches no apps/* UI, no packages/shared/src/types/*/endpoints.ts, no orbit-api code, and no i18n strings — this is a pure CI-tooling / internal-DX change to orbit-ui-mobile only.

Validation

This review's sandbox could not obtain approval to re-execute npm run lint/type-check/test or node tools/test-tools.mjs directly. In place of trusting the PR body's transcript outright, the byte totals underpinning the whole PR were independently re-derived from the merge commit:

$ git show HEAD:CLAUDE.md | wc -c
18864
$ wc -c .claude/rules/core.md
3380

Both match the PR body's raw-blob-verification section and tools/context-budget.json's 22244 total exactly. The PR body's own transcript is a full command+output log (not a bare claim), which is what D9/rubric dimension 15 requires of a harness change.

What's good

  • The structural allowlists mean --write-baseline itself refuses to run if the import/rule allowlist has silently grown (structuralFindings is checked before either the write or delta path) — matches tools/CONVENTIONS.md's "verdict computed from artifacts on disk" gate-tools rule.
  • resolveTargetRef correctly distinguishes "target branch unreachable" (fail closed, exit 2) from "target branch reachable but lacks the baseline file" (first-run bootstrap) via git ls-tree.
  • The .gitattributes LF pin is a genuine root-cause fix (Code Standard 1) for a real cross-OS byte-counting hazard, not a workaround inside the checker.
  • Test coverage is unusually complete: over/under budget, no-rewrite-on-pass, baseline-regeneration-can't-hide-growth, first-run bootstrap, unfetched-target fail-closed, unallowlisted-import-even-when-target-absent, new-unconditional-rule, scoped-rule-stays-out, missing/present sibling imports (both asserted), malformed baseline, and a --help completeness assertion.

Recommendation

Approve as-is. The one Low/Info note is a future polish, not a blocker.

@thomasluizon

Copy link
Copy Markdown
Owner Author

@codex review

@sonarqubecloud

Copy link
Copy Markdown

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 480946a222

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: PR #622 — Add shrink-only context budget gate

Scope: PR #622 in thomasluizon/orbit-ui-mobile (ORB-117)
Recommendation: APPROVE

Summary

Adds tools/check-context-budget.mjs: a dependency-free CLI enforcing a shrink-only total byte budget for always-loaded context (CLAUDE.md + unconditional .claude/rules/*.md), rejecting unlisted growth of the root @ import allowlist and the unconditional-rules allowlist, and reporting (not enforcing) resolvable sibling CLAUDE.md files. Wires it into a new Context Budget job in .github/workflows/guards.yml, seeds tools/context-budget.json, pins CLAUDE.md/.claude/rules/*.md to LF in .gitattributes (closing a real Windows-vs-CI byte-count hazard the repo already hit once), and documents the tool in CLAUDE.md's docs registry and tools/README.md.

Changed files: .gitattributes, .github/workflows/guards.yml, CLAUDE.md, tools/README.md, tools/check-context-budget.mjs, tools/context-budget.json, tools/test-tools.mjs.

I traced the tool's full control flow (arg parsing, measurement, baseline parse/compare, target-ref resolution, bootstrap vs steady-state, write-baseline) against every fixture in the new contextBudgetCases in tools/test-tools.mjs and found it internally consistent — no correctness defect.

Prior review history on this PR

This PR went through two rounds of CHANGES_REQUESTED (addressed by the "Fix context budget base comparison" and "Fix context budget LF baseline" commits), followed by two prior APPROVED reviews after those fixes landed. No PR-content commits have landed since the last approval — the subsequent merge commits (9a10e8d6, 480946a2) only carried forward unrelated changes already on main (#624); git diff between the last-reviewed commit and the current PR head shows zero changes to files this PR actually touches. This review confirms that assessment independently rather than re-litigating resolved findings.

Findings

Critical: None. High: None. Medium: None.

Low / Info:

[INFO] Context Budget CI job self-compares on push to main

  • dimension: SOLID/clean-arch (informational)
  • location: .github/workflows/guards.yml (context-budget job)
  • issue: The job's if excludes pull_request from the context:reseed-labeled case but still runs on push to main. There, CONTEXT_BUDGET_BASE_REF resolves to origin/main, which a push-triggered actions/checkout --fetch-depth 0 updates to the very commit just checked out — so the job compares that commit's baseline against itself, a guaranteed pass. Since main is protected (no direct push), every commit reaching main already passed this same check at PR time, so this is a wasted (never-failing) CI job on push, not a masked regression.
  • fix: Optional — gate to pull_request only, mirroring the ratchet job's if: github.event_name == 'pull_request' && ... pattern.

Subagents

All five parity/contract/security/i18n/design subagents gated N/A: no apps/web/**/apps/mobile/** file, no user-facing string, no packages/shared/src/types/*/DTO, no orbit-api code, no UI file changed. Cross-repo dimensions (contract-aligner, orbit-api side of backward-compat guard) not verifiable in this CI job — orbit-api is not checked out here, and this PR's diff doesn't touch any cross-repo surface regardless.

Validation

Per this workflow's CI adaptations, Phase 6 (/validate) was skipped — Build, Unit Tests, and SonarCloud already run as separate required checks on this PR and are green. All CI checks are green, including the new Context Budget job itself (self-validating).

What's good

  • Structural allowlists (imports, unconditional rules) are closed sets that --write-baseline cannot launder growth into — main() fails closed on structural findings before it ever reaches the write path.
  • readComparisonBaseline correctly distinguishes "target branch has no baseline yet" (bootstrap) from "target branch is simply unfetched" (fail closed, exit 2, actionable message).
  • Test coverage is decision-path-shaped (bootstrap, regenerated-baseline-can't-hide-growth, unfetched-target fail-closed, malformed-baseline, scoped-rule exemption), not happy-path-only.
  • The LF-pinning fix in .gitattributes closes a real cross-platform byte-count hazard this repo already hit once (ccc1849f).

Recommendation

No Critical/High findings, all CI green, and no PR-content changes since the last approved review. Safe to merge as-is; the one Low/Info CI-redundancy note is optional cleanup, not a blocker.

@thomasluizon
thomasluizon merged commit 83593ca into main Jul 28, 2026
29 checks passed
@thomasluizon
thomasluizon deleted the feature/orb-117-ship-the-always-loaded-context-budget branch July 28, 2026 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant