Repository navigation
refactor(sql-fs): centralize sandbox fencing policy - #274
Conversation
…octor `suite_names | grep -qx` let grep exit on the first match, so suite_names died with SIGPIPE and pipefail rejected `suite integration` every time on bash 3.2. Doctor's `find | head -3` had the same latent failure on large trees and aborted silently when dist/api/server.js was missing. Capture output before reading it, report a missing build explicitly, and use explicit empty SERVER_ENV resets.
Five composite statements in namespace.ts each carried a copy of the epoch acceptance, version advance, and epoch publication CTEs. fenceAndAdvance and the new compositeFenceAndAdvance now share one private fenceCtes in sandbox.ts and differ only in the ctx projection. Executed SQL, parameters, lock ordering, and statement counts are unchanged. A real-Postgres suite pins statement counts, caught-ESTALE-then-commit for every composite and for ingest, the epoch fallback, mismatched RLS context, hardlinks, and observed advisory-lock waits under READ COMMITTED and REPEATABLE READ. check:architecture rejects fence policy or raw SQL access in namespace.ts and ingest.ts.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files
You’re at about 94% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/check-architecture-boundaries.mjs">
<violation number="1" location="scripts/check-architecture-boundaries.mjs:51">
P3: This generic pattern rejects unrelated version counters, not only `sandboxes.version` advancement. Scope the match to a `sandboxes` update or use SQL-aware matching so namespace and ingest queries do not need workarounds.</violation>
<violation number="2" location="scripts/check-architecture-boundaries.mjs:62">
P2: Assignment destructuring bypasses this raw-SQL check. Handle assignment-pattern destructuring and validate its source type before allowing the extracted `unsafe` or `file` capability.</violation>
</file>
<file name="scripts/check-architecture-boundaries.test.mjs">
<violation number="1" location="scripts/check-architecture-boundaries.test.mjs:211">
P2: This assertion relies on an undocumented filesystem traversal order. Sort the diagnostics before comparing them, or make checker output ordering deterministic, to prevent platform-dependent test failures.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| } else if (ast.isElementAccessExpression(node) && ast.isStringLiteral(node.argumentExpression)) { | ||
| receiver = node.expression; | ||
| name = node.argumentExpression.text; | ||
| } else if (ast.isBindingElement(node) && ast.isObjectBindingPattern(node.parent)) { |
There was a problem hiding this comment.
P2: Assignment destructuring bypasses this raw-SQL check. Handle assignment-pattern destructuring and validate its source type before allowing the extracted unsafe or file capability.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/check-architecture-boundaries.mjs, line 62:
<comment>Assignment destructuring bypasses this raw-SQL check. Handle assignment-pattern destructuring and validate its source type before allowing the extracted `unsafe` or `file` capability.</comment>
<file context>
@@ -44,6 +44,40 @@ for (const importer of [
+ } else if (ast.isElementAccessExpression(node) && ast.isStringLiteral(node.argumentExpression)) {
+ receiver = node.expression;
+ name = node.argumentExpression.text;
+ } else if (ast.isBindingElement(node) && ast.isObjectBindingPattern(node.parent)) {
+ receiver = node.parent;
+ const property = node.propertyName ?? node.name;
</file context>
| }); | ||
| const diagnostics = checkArchitectureBoundaries(root).filter(({ code }) => code === "FENCE_POLICY_OWNER"); | ||
| assert.deepEqual( | ||
| diagnostics.map(({ file, line }) => [file, line]), |
There was a problem hiding this comment.
P2: This assertion relies on an undocumented filesystem traversal order. Sort the diagnostics before comparing them, or make checker output ordering deterministic, to prevent platform-dependent test failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/check-architecture-boundaries.test.mjs, line 211:
<comment>This assertion relies on an undocumented filesystem traversal order. Sort the diagnostics before comparing them, or make checker output ordering deterministic, to prevent platform-dependent test failures.</comment>
<file context>
@@ -188,3 +188,112 @@ it("permits literal non-worker imports and rejects indirect scope method calls",
+ });
+ const diagnostics = checkArchitectureBoundaries(root).filter(({ code }) => code === "FENCE_POLICY_OWNER");
+ assert.deepEqual(
+ diagnostics.map(({ file, line }) => [file, line]),
+ [
+ ["src/sql-fs/dialects/postgres/ingest.ts", 3],
</file context>
| diagnostics.map(({ file, line }) => [file, line]), | |
| diagnostics.toSorted((a, b) => a.file.localeCompare(b.file) || a.line - b.line).map(({ file, line }) => [file, line]), |
| "src/sql-fs/dialects/postgres/namespace.ts", | ||
| "src/sql-fs/dialects/postgres/ingest.ts", | ||
| ]); | ||
| const fencePolicyStatements = [/\bapp\.sandbox_epoch\b/i, /\bversion\s*=\s*(?:\w+\.)?version\s*\+/i]; |
There was a problem hiding this comment.
P3: This generic pattern rejects unrelated version counters, not only sandboxes.version advancement. Scope the match to a sandboxes update or use SQL-aware matching so namespace and ingest queries do not need workarounds.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/check-architecture-boundaries.mjs, line 51:
<comment>This generic pattern rejects unrelated version counters, not only `sandboxes.version` advancement. Scope the match to a `sandboxes` update or use SQL-aware matching so namespace and ingest queries do not need workarounds.</comment>
<file context>
@@ -44,6 +44,40 @@ for (const importer of [
+ "src/sql-fs/dialects/postgres/namespace.ts",
+ "src/sql-fs/dialects/postgres/ingest.ts",
+]);
+const fencePolicyStatements = [/\bapp\.sandbox_epoch\b/i, /\bversion\s*=\s*(?:\w+\.)?version\s*\+/i];
+
+function forbiddenSqlApiAccess(node, checker) {
</file context>
|
@codex review |
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
Five composite filesystem operations duplicated the sandbox fence policy. Share that policy while preserving the executed SQL, lock order, transaction context, and statement counts.
What changed
sandbox.tsand use lazy SQL fragments in namespace operations.Scope
Tradeoffs
Keep context setup inline. A separate context query adds statements and changes how a mismatched existing sandbox context behaves.
Blast Radius
Filesystem mutations share one fence builder, so a policy regression could affect several operations. SQL equivalence checks and real Postgres tests cover the preserved behavior.
Verification
pnpm checkpassed with 2,334 unit tests, both SDK checks, and builds.pnpm verify:architecturepassed with 279/279 integration tests and zero skips. Contracts, live SDKs, worker failures, stream finalization, input budgets, and tenant isolation passed.