Repository navigation
ci: invariant suite from base and a security reviewer agent on protected paths - #98
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.
Once extra usage is available, someone with write access to this repository can comment @claude review on this pull request to trigger a review.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 29 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the Decision 7 review flow in CI by introducing a protected-path detection script, a dedicated invariant test suite, and its runner configuration, while refactoring existing integration tests to separate product flows from security invariants. The feedback recommends improving the robustness of a PostgreSQL privilege check by using has_function_privilege instead of aclexplode to correctly handle default privileges, and refactoring several test cases to use the existing withAdmin helper to simplify the code and prevent potential connection leaks.
| const { rows } = await admin.query( | ||
| `SELECT p.proname FROM pg_proc p, aclexplode(p.proacl) a WHERE a.grantee = 0 AND p.pronamespace = 'crove_kernel'::regnamespace`, | ||
| ); |
There was a problem hiding this comment.
The query uses aclexplode(p.proacl) to check if any function is executable by PUBLIC. However, in PostgreSQL, if a function has default privileges (i.e., proacl is null), aclexplode will return no rows, meaning functions with default privileges (which are executable by PUBLIC by default) will be missed by this test. Using has_function_privilege with 'public' is more robust as it correctly handles default privileges.
const { rows } = await admin.query(
"SELECT p.proname FROM pg_proc p WHERE p.pronamespace = 'crove_kernel'::regnamespace AND has_function_privilege('public', p.oid, 'EXECUTE')"
);| const admin = new pg.Client({ connectionString: adminUrl }); | ||
| await admin.connect(); | ||
| try { | ||
| await expect(admin.query('UPDATE crove_kernel.audit_event SET outcome = $1', ['denied'])).rejects.toThrow(/append-only/); | ||
| await expect(admin.query('DELETE FROM crove_kernel.outbox_event')).rejects.toThrow(/append-only/); | ||
| const { rows } = await admin.query<{ n: string }>('SELECT count(*) AS n FROM crove_kernel.audit_event'); | ||
| expect(Number(rows[0]?.n)).toBeGreaterThan(0); | ||
| } finally { | ||
| await admin.end(); | ||
| } |
There was a problem hiding this comment.
The test manually instantiates, connects, and ends a pg.Client. This can be simplified and made safer against connection leaks by using the existing withAdmin helper function.
await withAdmin(async (admin) => {
await expect(admin.query("UPDATE crove_kernel.audit_event SET outcome = $1", ["denied"])).rejects.toThrow(/append-only/);
await expect(admin.query("DELETE FROM crove_kernel.outbox_event")).rejects.toThrow(/append-only/);
const { rows } = await admin.query<{ n: string }>("SELECT count(*) AS n FROM crove_kernel.audit_event");
expect(Number(rows[0]?.n)).toBeGreaterThan(0);
});| const admin = new pg.Client({ connectionString: adminUrl }); | ||
| await admin.connect(); | ||
| try { | ||
| await admin.query('BEGIN'); | ||
| const { rows } = await admin.query<{ id: string }>( | ||
| `INSERT INTO crove_finance.journal_entry (org_id, entry_date, description, source_type, source_id) | ||
| VALUES ($1, '2026-10-01', 'test', 'test', crove_kernel.uuid_v7()) RETURNING id`, | ||
| [orgA], | ||
| ); | ||
| await admin.query( | ||
| `INSERT INTO crove_finance.journal_line (org_id, entry_id, line_no, account_code, debit, credit) VALUES | ||
| ($1, $2, 1, '6422', 100, 0), ($1, $2, 2, '331', 0, 90)`, | ||
| [orgA, rows[0]?.id], | ||
| ); | ||
| await expect(admin.query('COMMIT')).rejects.toThrow(/does not balance/); | ||
| } finally { | ||
| await admin.query('ROLLBACK').catch(() => {}); | ||
| await admin.end(); |
There was a problem hiding this comment.
The test manually instantiates, connects, and ends a pg.Client. This can be simplified and made safer against connection leaks by using the existing withAdmin helper function.
await withAdmin(async (admin) => {
await admin.query("BEGIN");
try {
const { rows } = await admin.query<{ id: string }>(
"INSERT INTO crove_finance.journal_entry (org_id, entry_date, description, source_type, source_id) VALUES ($1, '2026-10-01', 'test', 'test', crove_kernel.uuid_v7()) RETURNING id",
[orgA],
);
await admin.query(
"INSERT INTO crove_finance.journal_line (org_id, entry_id, line_no, account_code, debit, credit) VALUES ($1, $2, 1, '6422', 100, 0), ($1, $2, 2, '331', 0, 90)",
[orgA, rows[0]?.id],
);
await expect(admin.query("COMMIT")).rejects.toThrow(/does not balance/);
} finally {
await admin.query("ROLLBACK").catch(() => {});
}
});| const admin = new pg.Client({ connectionString: adminUrl }); | ||
| await admin.connect(); | ||
| try { | ||
| const { rows } = await admin.query<{ docs: number; lines: number }>( | ||
| 'SELECT (SELECT count(*)::int FROM crove_finance.purchase_document) AS docs, (SELECT count(*)::int FROM crove_finance.journal_line) AS lines', | ||
| ); | ||
| expect(rows[0]?.docs).toBeGreaterThan(0); | ||
| expect(rows[0]?.lines).toBeGreaterThan(0); | ||
| await expect(admin.query('UPDATE crove_finance.journal_line SET debit = debit')).rejects.toThrow(/append-only/); | ||
| await expect(admin.query('DELETE FROM crove_finance.purchase_document')).rejects.toThrow(/append-only/); | ||
| } finally { | ||
| await admin.end(); |
There was a problem hiding this comment.
The test manually instantiates, connects, and ends a pg.Client. This can be simplified and made safer against connection leaks by using the existing withAdmin helper function.
await withAdmin(async (admin) => {
const { rows } = await admin.query<{ docs: number; lines: number }>(
"SELECT (SELECT count(*)::int FROM crove_finance.purchase_document) AS docs, (SELECT count(*)::int FROM crove_finance.journal_line) AS lines",
);
expect(rows[0]?.docs).toBeGreaterThan(0);
expect(rows[0]?.lines).toBeGreaterThan(0);
await expect(admin.query("UPDATE crove_finance.journal_line SET debit = debit")).rejects.toThrow(/append-only/);
await expect(admin.query("DELETE FROM crove_finance.purchase_document")).rejects.toThrow(/append-only/);
});
…n protected paths Decision 7 (External-tenant gate T15). Adds the `protected` workflow: - `Invariant suite (from base)`: detects protected paths (G6) with the base branch's copy of tools/ci/protected-paths.mjs; when any changed, replaces the PR's test/invariant/ files and vitest.invariant.config.ts with the base branch's copy and runs them against the PR's code on PostgreSQL 17. Fails when any invariant test fails or is skipped. - `Security review (agent)`: runs anthropics/claude-code-action (pinned) with a security, tenancy, authorization and money prompt when the ANTHROPIC_API_KEY secret exists; otherwise passes with a "pending owner setup" notice (#95). The invariant suite is apps/kernel/test/invariant/: the RLS, EXECUTE matrix, system context, approval policy (P10, P11, P12), ledger immutability, search_path hijack, role grant and token assurance tests, moved out of kernel.db.test.ts with self-contained setup, plus the ADR-0028 session tests (sessions.db.test.ts, moved). Documented in AGENTS.md; T15 status updated in the plan. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review fixes for the Decision 7 gate: - Detect protected paths from paths only: every change under supabase/migrations/ is protected, so neither .gitattributes tricks (binary or -diff hide content from git diff) nor keyword evasion can skip the gate. .gitattributes and .gitmodules are protected too. - Protect dependencies and test runners (root package.json, lockfile, workspace, .npmrc, vitest configs), CODEOWNERS, .mcp.json, CLAUDE.md, .claude/agents, the migration runner and the HTTP edge. - NUL-separated git listings everywhere (detection script, rm and checkout of the suite), literal pathspecs, and a check that the files on disk match the base list and content. - Fork PRs touching protected paths fail the review job with a maintainer-review message instead of passing. - The reviewer reads AGENTS.md from the base commit. - Invariant suite: add #96's org creation tests and #97's TEMP and search_path tests, move packages/db/test/migrate.db.test.ts into packages/db/test/invariant/, and give the kernel invariants their own copy of the e-invoice fixture. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ff0e034 to
ea1b73e
Compare
Workspace manifests can repoint a package the invariant suite imports (exports/main/imports) or run code (scripts, pnpm fields) without touching the lockfile. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Closes #30
Decision 7 review flow in CI (External-tenant gate T15).
What
.github/workflows/protected.yml, new workflow (kept out ofci.ymlto avoid conflicts with test: Playwright end-to-end test of the Inbox flow on docker compose #91). Both jobs run on every PR and pass in seconds when no protected path changed, so docs-only PRs are never blocked.Invariant suite (from base): detects protected paths with the base branch's copy oftools/ci/protected-paths.mjs. When any changed, it deletes the PR'stest/invariant/files andvitest.invariant.config.ts, checks out the base branch's copy over the PR's tree and runs it against the PR's code on PostgreSQL 17. It fails if any invariant test fails or is skipped. The job summary lists the protected files, whether the base or PR copy ran, and the invariant files the PR changed (ignored until merge).Security review (agent):anthropics/claude-code-actionpinned tofd1c128(v1.0.237),github_tokenonly, read-only contents, max 40 turns, tools limited to read andgh pr comment/diff/view. The prompt covers security, tenancy, authorization and money, treats PR content as untrusted, and posts one## Security review (agent)comment plus inline comments. Without theANTHROPIC_API_KEYsecret it passes with the notice "Security review pending owner setup" (owner action Approve spend and add ANTHROPIC_API_KEY for the security reviewer agent #95).test/invariant/directory. Todayapps/kernel/test/invariant/:kernel-invariants.db.test.ts: 21 tests moved out ofkernel.db.test.ts(RLS isolation, non-member refusal, append-only audit/outbox/ledger/documents, balance and posting rules, definersearch_pathhijack, user account visibility, owner-only admin grants, storage key scope, R3 roles, token assurance, fresh MFA P11, EXECUTE matrix and system context, four-eyes P10, eligibility P12, live role re-check, revisions). Self-contained setup; one test now records its own document so it never passes on an empty table.sessions.db.test.ts: moved unchanged from feat: enforce session status and fresh MFA per ADR-0028 (self-host) #90 (ADR-0028 session status).kernel.db.test.tskeeps the 4 product-flow tests; one now adds the admin it used to inherit from a moved test. All 25 kernel tests from main are kept.tools/ci/protected-paths.mjs, unit tested intools/test/): G6 mapped to this repo, plus the gate itself, plus migrations with GRANT/REVOKE/POLICY/DEFINER/ROLE/RLS and any edited or deleted migration.Checks (local)
pnpm lint,pnpm typecheck,pnpm test(91),pnpm test:db(41),vitest --config vitest.invariant.config.ts(34),pnpm docs:check, actionlint 1.7.7 clean. Detection run on history: #87 -> 3 protected files, #86 ->AGENTS.md, #83 (VI page only) -> none. A suite run withoutCROVE_TEST_ADMIN_URL(all skipped) makes the skip check fail.Proof that the base copy wins
Draft #99 (closed, branch deleted) targeted this branch with a throwaway commit that disabled four-eyes in
packages/kernel/src/approval.tsandit.skip-ed the two invariant tests that catch it:Lint, typecheck, test(PR copy): passed, 39 passed, 2 skipped.Invariant suite (from base)(base copy): failed onfour-eyes (P10)andturns an edit into a new revision, with the warning "This PR changes the invariant suite; CI ran the base copy instead" (run 38714775507).On this PR the base has no suite yet, so the job ran the PR copy (bootstrap warning): 34 passed, 0 skipped.
Security review (agent)reported "Security review pending owner setup".Update after the security review (rebased on main, including #96, #97 and #103)
supabase/migrations/is now protected, and file contents are no longer scanned. A-difforbinaryentry in.gitattributes, or a keyword trick, can no longer skip the gate..gitattributes,.gitmodules,CODEOWNERS, rootpackage.json,pnpm-lock.yaml,pnpm-workspace.yaml,.npmrc,vitest*.config.ts,.mcp.json,CLAUDE.md,.claude/agents/**,packages/db/src/migrate.tsandapps/kernel/src/http.ts. The plan's G6 row now matches this list.-z) in both the script and the workflow, with literal pathspecs. A check confirms that the suite files on disk match the base list and the base content.Security review (agent)with "Maintainer security review required".git show <base>:AGENTS.md, not from the PR's tree.packages/db/test/invariant/migrate.db.test.ts, moved whole);context.db.test.ts(moved);cipassed.Required check (left for the parent session)
Not made required here. Making
Invariant suite (from base)required before this merges would block every open PR that does not have the workflow yet. After merge, add it (and optionallySecurity review (agent)) tomain's required checks next toSecret scanandDocs check. It is safe for docs-only PRs because the job always reports success when nothing protected changed.Limits
pull_request, a PR runs its own copy ofprotected.yml; the base copy wins for the detection script and the suite, but a PR that edits the workflow could bypass both. That PR is itself protected (reviewer flags it), and once the check is required, deleting or renaming the job leaves the check pending.pull_request_targetwas rejected: it would run untrusted PR code with base-branch privileges and cache scope.package.json, lockfile) still come from the PR.🤖 Generated with Claude Code
📌 TL;DR
This PR implements the "Decision 7" security gate by introducing a protected-path CI workflow that enforces invariant tests from the base branch and triggers an automated security review agent. It establishes a new
test/invariant/suite to verify critical security properties (RLS, ledger immutability, authorization) against a live PostgreSQL instance, ensuring PRs cannot weaken security checks by modifying the tests themselves.🎯 Type of Change
🔍 Changes Walkthrough
.github/workflows/protected.ymlAGENTS.mdapps/kernel/test/invariant/kernel-invariants.db.test.tsapps/kernel/test/invariant/sessions.db.test.tsapps/kernel/test/sessions.db.test.tsinto the invariant suite. Updated import paths to reflect the new directory structure.apps/kernel/test/kernel.db.test.tstest/invariant/.📊 Architectural Flow
flowchart TD PR[Pull Request Created] --> Detect{Detect Protected Paths<br/>(using base branch script)} Detect -->|No Protected Paths| Pass[Jobs Pass Quickly] Detect -->|Protected Paths Changed| InvariantJob[Invariant Suite Job] Detect -->|Protected Paths Changed| SecurityJob[Security Review Job] subgraph Invariant Suite Execution InvariantJob --> CheckoutBase[Checkout Base Branch Tests] CheckoutBase --> RunTests[Run Invariant Tests<br/>against PR Code + Postgres 17] RunTests --> CheckSkip{Any Tests Skipped?} CheckSkip -->|Yes| Fail[Fail Build] CheckSkip -->|No| PassInv[Pass] end subgraph Security Review Execution SecurityJob --> CheckKey{ANTHROPIC_API_KEY<br/>Available?} CheckKey -->|No| Notice[Post Notice:<br/>Pending Owner Setup] CheckKey -->|Yes| Agent[Run Claude Code Agent] Agent --> Review[Review Diff for:<br/>Tenancy, Auth, Money, RLS] Review --> Post[Post Inline Comments<br/>+ Top-level Verdict] end