ci: harden review gate and CI enforcement - #2
Merged
Conversation
Make risk classification, coverage, Swift validation, and adversarial review requirements binding so protected PRs cannot pass on skipped or incomplete evidence. Co-authored-by: Cursor <cursoragent@cursor.com>
…mantic tier advisory
…ne npm audit locally
…make semgrep findings advisory
The two GILLY_ACK_TRUST audit rows sat below the footer line in the tracked loop-log template — leaked local acks, not real review incidents. Restore the clean template state. Risk-Tier: trust-critical
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Risk tier
Path-derived minimum (from review-policy.yaml):
trust-critical— this PR modifiesINVARIANTS.md(trust-critical) andevals/budgets.json(semantic).routinesemantictrust-criticalWhat Changed
scripts/check-tier.mjs+review-policy.yaml): trust-critical paths (including the redaction module) require a conscious ack and CODEOWNERS/human-judge review, semantic tier is advisory locally but fails in CI, and the check fails loudly on an unresolvableGILLY_TIER_RANGEinstead of passing on an empty diff.scripts/check-all.mjsandscripts/check-swift.sh: npm-audit network failures FAIL in CI but only WARN locally (offline detection scoped to the error channel so real CVEs still fail), semgrep findings are advisory-only, and the CLAUDE.md-alignment check skipsowner/reporeference tokens..github/workflows/check-all.ymljobs, CODEOWNERS, PR template, branch-protection doc,.captain/reviewadversary/fix-verify prompts, and.cursorreview/model-routing rules.Risk Assessment
✅ Low: The latest commit is a well-bounded gate-hardening change that correctly and completely resolves the prior round's findings (offline-audit false-negative scoped to npm's error channel, semgrep findings made truly non-gating), with no new bugs or behavior regressions found on a full pass.
Testing
Exercised the branch's core deliverable — the machine-checked risk-tier gate (scripts/check-tier.mjs) — through its actual CLI, the same way CI and the pre-commit hook call it, across nine scenarios covering both local and --ci modes in an isolated temp git repo (worktree left clean). It correctly enforces the path-derived floor (trust-critical cannot self-declare routine or under-declare semantic), requires a conscious ack for trust-critical locally, treats semantic as advisory locally but blocking in CI, fails loudly on an unresolvable GILLY_TIER_RANGE instead of passing on an empty diff, and maintains a de-duped durable audit log. Full command transcript captured as reviewer-visible evidence; all behaviors passed. This is a CLI/gate change with no rendered UI surface, so the appropriate product artifact is the CLI transcript rather than a screenshot.
Evidence: check-tier gate CLI transcript (9 scenarios: tier enforcement, ack audit log, unresolvable-range fail-loud)
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed (4) ✅
scripts/check-tier.mjs:86- changedFiles(): therangebranch doesreturn sh(["diff","--name-only", range])with no exit-status check. sh() discards git's non-zero status and returns [] on any failure. Because CI always sets GILLY_TIER_RANGE, an unresolvable base (origin/mainnot fetched as a remote-tracking ref, orgithub.event.before= all-zeros on a new-branch/force push) makes the diff error → zero changed files → path minimum 'routine' → the risk-tier gate passes vacuously. This reintroduces exactly the 'soft-lie green' the branch is closing everywhere else. The non-range CI branch (line 91) already guards onr.status === 0and falls back to HEAD~1; the range branch should do the same — check the exit status and fail loudly (or fall through) instead of returning empty.scripts/check-tier.mjs:110- minTier(): hits.semantic.push is guarded bymin !== "trust-critical", so any semantic-tier file encountered after the first trust-critical file is dropped from the displayed hits list. Cosmetic only — the pass/fail decision usesmin, not the hit lists — but the console diagnostic can under-report which files pushed the tier up.scripts/check-all.mjs:302- The new owner/repo heuristic (^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$&& !existsSync) skips any two-segment token absent from disk to allow references likefarzaa/clicky. Side effect: a genuinely-missing two-segment repo-relative path (e.g.scripts/foo) is now silently ignored by the CLAUDE.md-alignment check rather than flagged. Deliberate loosening per the comment, but it does narrow that check's coverage.🔧 Fix: fail check-tier loudly on unresolvable GILLY_TIER_RANGE
3 issues (2 warnings, 1 info) still open:
review-policy.yaml:39- trust_critical_paths uses**/redaction/**, which compiles to^.*/redaction/.*$and only matches aredaction/directory. The real redaction module is a single file,core/src/surface/redaction.ts(confirmed: arch-checks allow-lists it as a file, and the glob returns false against it). No other trust-critical glob matches it, so a change touching only the redaction module is path-derived tier 'routine' and skips human_judge + codeowners — defeating INVARIANTS row 13, the invariant this same branch adds an arch-check for. The mirrored**/redaction/**lines in .github/CODEOWNERS have the identical gap. Fix: addcore/src/surface/redaction.ts(or**/redaction*.ts) to trust_critical_paths and CODEOWNERS.scripts/check-all.mjs:421- checkRiskTier(false) is now part of the main check-all suite, and scripts/hooks/pre-commit:12 also runs check-tier.mjs standalone. In local mode check-tier FAILs when the path minimum is 'semantic' (or higher) and no tier is declared. Trust-critical has a lightweight ack (GILLY_ACK_TRUST=1) but semantic has none, and check-all passes no --declare through — so every commit touching client/** (the whole Swift client) or the semantic specs files fails pre-commit unless the developer exports GILLY_DECLARE_TIER=semantic each time. That friction pushes developers to permanently export the var, which hollows out the conscious-ack intent. Consider a GILLY_ACK_SEMANTIC-style escape, or scoping the local tier check to trust-critical only and leaving the full semantic/routine enforcement to CI PRs..github/workflows/check-all.yml:38- The push-event tier step sets GILLY_TIER_RANGE to${{ github.event.before }}...${{ github.sha }}. On the first push that creates a branch, github.event.before is all-zeros, the range is unresolvable, and check-tier now exits 1 (the round-1 fail-loud behavior). New branches' initial push will red until a PR is opened (pull_request uses base_ref, which resolves). This is the deliberate 'refuse to pass on an empty diff' tradeoff — noting it so the operational cost is understood.🔧 Fix: classify redaction trust-critical; make local semantic tier advisory
2 warnings still open:
scripts/hooks/pre-commit:12- The tier check runs twice per pre-commit: check-all.mjs:421 (checkRiskTier(false)) spawns check-tier.mjs, then pre-commit:12 runs it standalone. When GILLY_ACK_TRUST=1 on a trust-critical commit, check-tier.mjs:210 appendFileSync writes an audit row on EACH run — confirmed live: .captain/review/loop-log.md already contains two identical '2026-07-23 | local ack | ... ack for: INVARIANTS.md' rows from one commit. Two further problems: (a) the log is a tracked file, so every ack'd commit leaves the working tree dirty with rows not included in the commit; (b) a row is appended even if a later check aborts the commit, recording acks for commits that never landed. Fix: drop the redundant pre-commit:12 invocation (the comment already notes it is 'also embedded in check-all'), so the tier check — and its logging — runs once.scripts/check-all.mjs:351- checkDeps was changed SKIP→FAIL when npm audit hits a network error (enotfound/etimedout/offline/...). check-all runs at pre-commit (hook line 8), so any commit made without network connectivity now hard-fails the gate and blocks the commit. Unlike the missing-tool FAILs (biome/tsc/vitest — a one-time install), registry reachability is transient/environmental. This creates exactly the friction that pushes developers toward --no-verify, which this branch is otherwise closing off. Consider distinguishing local vs CI here (as was done for the semantic tier): treat a genuine registry-unreachable result as SKIP/WARN locally while keeping FAIL in CI, rather than blocking all offline commits.🔧 Fix: de-dupe tier audit log; warn (not fail) on offline npm audit locally
2 issues (1 warning, 1 info) still open:
scripts/check-all.mjs:352- checkDeps now downgrades a matched 'offline' result from FAIL to a non-blocking WARN locally. But the offline heuristic matches the token regex againstblob = (r.stdout + r.stderr).toLowerCase()— i.e. the fullnpm audit --jsonREPORT, not just an error stream. When real high/critical vulns exist, npm exits non-zero (r.status truthy) AND emits the advisory JSON on stdout, whosevia[].title/name/dependency-path fields commonly contain the wordnetwork(and possiblyoffline). Such a collision makesr.status && /network|offline|.../true, so the code treats a genuine-vulnerability run as 'registry unreachable' and returns WARN — silently unblocking a commit that has real CVEs. Before this change the same match returned FAIL, so the collision was harmless; the SKIP→WARN change turns it into a local security-gate false-negative (CI still FAILs, but with a misleading 'registry unreachable' message). Fix: only treat as offline when the audit produced no parseable report (e.g.JSON.parse(r.stdout).metadata?.vulnerabilitiesis absent) or scope the token regex to stderr / an{"error":...}payload, so an advisory title containing 'network' can't suppress a real finding.scripts/check-swift.sh:153- The green path is relabeledsemgrep (advisory)and the not-installed branch now says 'advisory only, not part of GATE PASS/FAIL', but the findings branch is stillred "semgrep findings", which increments FAIL and produces GATE FAIL. So when semgrep IS installed and reports findings, it is not advisory — it hard-fails the Swift gate, contradicting both the new label and CLAUDE.md's 'semgrep advisory-only'. Either make findings non-gating (echo/warn) to match the stated policy, or drop the 'advisory' framing.🔧 Fix: scope offline audit detection to error channel; make semgrep findings advisory
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
node scripts/check-tier.mjson a routine (README) change → OK, exit 0node scripts/check-tier.mjs --declare=routineon a trust-critical redaction.ts change → FAIL (below path minimum), exit 1node scripts/check-tier.mjson trust-critical change, no ack → FAIL, exit 1GILLY_ACK_TRUST=1 node scripts/check-tier.mjson trust-critical change → OK + audit-log row writtennode scripts/check-tier.mjson a semantic (client/App.swift) change locally → WARN, non-blocking (exit 0)GILLY_TIER_RANGE=HEAD node scripts/check-tier.mjs --cisemantic change, no PR declaration → FAIL, exit 1GILLY_TIER_RANGE=deadbeef...HEAD node scripts/check-tier.mjs --ci→ FAIL loudly, exit 1 (verified directly)GILLY_TIER_RANGE=HEAD~1...HEAD node scripts/check-tier.mjs --ci --declare=trust-criticalon specs/grants.md → OK;--declare=semantic→ FAILTwo consecutive GILLY_ACK_TRUST=1 runs → single header + two rows in .captain/review/loop-log.md (de-dup verified)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.