feat(db): canonical database operations, risk classes and fail-closed policy layer (#328) - #333
Conversation
Adds the SQLite half of the agent execution profile: a physically separate provider opened under SQLite's own read-only enforcement, acquired only through acquireExecutionProfileProvider. - sqlite-driver: SQLiteOpenOptions gains `readonly`; the Node adapter stops discarding open options and maps it to node:sqlite's `readOnly` spelling. - ProviderExecutionContext carries the read-only intent as a server-injected third constructor argument, deliberately not on caller-supplied ProviderOptions, so the shared editor path cannot set or clear it. - SQLiteProvider.connectReadOnly skips the shared open sequence entirely (no directory creation, no `create`, no write-implying pragmas) and refuses an in-memory target with PROFILE_UNSUPPORTED_TARGET. - queryReadOnly compiles exactly one statement with prepare(), enforces the row and byte caps result-side like PostgreSQL, and refuses to run at all on a provider that was not opened under the profile. Two controls, different scope: the read-only open governs the target database file, while PRAGMA query_only is re-asserted and verified before every statement to refuse writes to other files. The open alone is not sufficient — VACUUM INTO copies the database to an arbitrary path from a read-only handle on both adapters — and the profiled provider is pooled, so a statement that disables the pragma must not affect the next call. prepare() compiling a single statement is what makes the re-assertion unbypassable. statementTimeoutMs is a post-execution deadline, not preemption: SQLite has no transaction-local timeout and neither adapter exposes an interrupt or progress hook. Recorded in the provider doc and docs/BACKLOG.md A1, together with A2 for the empty file VACUUM INTO can still leave at an agent-chosen path. Also moves ExecutionProfileError/ExecutionProfileDenyCode to db/errors.ts so a provider can state why it fails closed without importing the factory, and extracts the shared read-only budget validation out of postgres.ts rather than duplicating a fail-closed validator. Contract tests run on both adapters — bun in-process, node in a real subprocess through the existing harness — and assert behavior (did the write land? does the file exist? how many bytes?) rather than driver error codes, which differ between the two.
Adds the hostile-input matrix the agent operation layer is judged by, and fixes the two gaps it exposed. Tests. tests/security/agent-statement-boundary.test.ts drives every case through both layers: the policy pipeline denies it with a reason code and the provider is never invoked, and the same statement is then driven straight at a real read-only SQLite profile as if classification had been bypassed. Comment and whitespace obfuscation, data-modifying and nested CTEs, multi-statement input, mutating pragmas, attach and detach, extension loading, temp-table scratch space and the PostgreSQL COPY forms are covered, with legitimate reads that mention write keywords inside literals, identifiers and comments as controls so nothing passes by keyword coincidence. PostgreSQL's engine-level half lives in the pg suite and SQLite's ATTACH containment in the sqlite suite, per the provider triad rule. An input-stage guard (src/lib/db/operations/statement-guard.ts) is wired into the descriptors' input schemas: defense in depth only, built on the shared readers in src/lib/sql rather than on text matching, refusing multi-statement input, non-read statements, side effects carried inside read-shaped ones, and text the engines read differently. Both ANALYZE spellings are refused for plan inspection, because PostgreSQL accepts ANALYSE and it executes. PostgreSQL profile. A read-only transaction forbids changing the database, not writing elsewhere: verified on 18, a superuser session inside BEGIN READ ONLY still ran COPY TO a server file, COPY TO PROGRAM, and pg_read_file. Only privileges refuse those, so the profile now verifies at open that its role is neither superuser nor a member of pg_read_server_files, pg_write_server_files or pg_execute_server_program, refusing with PROFILE_PRIVILEGES_TOO_BROAD; queryReadOnly is refused on a provider not opened under the profile; and DISCARD ALL follows the rollback, because an advisory lock taken inside the transaction survives it and a pooled client must not carry one forward. Docs state what the code does, including what neither provider bounds: out-of-scope reads have no database-native control on either (docs/BACKLOG.md A3).
…#328) Every agent-path operation now leaves a trace, and every allowed one leaves a result that is released with its run. - executeAuditedOperation emits a decision event before the provider is called and an outcome event after it, joined by a per-execution correlation id; a refusal emits the decision event alone with a typed agent_* reason. Emission is deliberately not wrapped in a try/catch, so an execution that cannot be audited does not run. - Only closed vocabularies are logged: the registry-resolved operation id (or the literal "unresolved") and an agent:<role> label built only from values policy.ts validated. The statement, the agent-supplied operation id, the session identifier and driver messages never reach an audit field. - ExecutionArtifactStore holds results in process memory only, bounded by an explicit release with the budget tracker's endRun, a TTL for runs that never end, and an entry cap. Nothing is persisted through a storage provider. - src/lib/audit.ts gains the agent_operation event type, the agent_* reasons mirroring PolicyDenyCode through a compile-enforced total map, and an optional correlationId surfaced on the stdout line as correlation_id. - The editor path is regression-pinned two ways: an UPDATE the agent input contract refuses still runs verbatim through the shared provider, and no /api/db route may import the agent operations layer. Docs: SECURITY.md control row 3.5 (Partial, with its note), the API_DOCS audit section, and PROGRAMME_CONTROL_IDS in scripts/security-check.mjs.
Resolve the last open carried reviewer note from the agent-m1 milestone review (T3 LOW 1). The two profiled-cache eviction tests in tests/unit/db/factory.test.ts slept 60 ms and then called evictIdleProviders(30), expecting the older cache entry to evict while the newer one survives. The flake is not that the sleep is too short: the tests assert a relative age ordering of two entries against an absolute threshold, so wall-clock consumed between the second acquisition and the evict call ages the younger entry past 30 ms as well, and both evict. A local installFakeClock() now freezes the Date.now that both caches stamp entries from and that evictIdleProviders compares against, so the age gap is injected exactly rather than raced. It is restored in a finally, and the idle sweep timer is untouched. Mutation-checked, since a frozen clock is exactly the kind of edit that can make timing tests pass vacuously: breaking the tunnel guard in the shared eviction loop reds only the first test, the same mutation in the profiled loop reds only the second, and removing the injected advance reds both. Test and assertion counts are unchanged (62 tests, 142 expect calls). Milestone close-out evidence: gate green, coverage 100% (30788/30788 lines), functional smoke green (login, connection through the real UI, query, rows render). One carried note is re-recorded as an accepted limitation rather than resolved: row and byte caps buffer the full result before refusing, on both providers, so the residual risk is memory rather than disclosure.
There was a problem hiding this comment.
Pull request overview
Adds the canonical, fail-closed database-operation enforcement layer for future agent workflows.
Changes:
- Introduces operation descriptors, policy evaluation, budgets, artifacts, and auditing.
- Adds isolated PostgreSQL and SQLite read-only execution profiles.
- Expands security, unit, integration, and documentation coverage.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
docs/API_DOCS.md |
Documents agent audit correlation. |
docs/BACKLOG.md |
Records deferred agent security limitations. |
docs/SECURITY.md |
Adds agent enforcement controls. |
docs/STORAGE.md |
Documents agent-password encryption. |
docs/providers/postgres.md |
Documents PostgreSQL execution profile. |
docs/providers/sqlite.md |
Documents SQLite execution profile. |
scripts/security-check.mjs |
Registers new security controls. |
src/lib/audit.ts |
Adds correlated agent audit events. |
src/lib/db/errors.ts |
Adds execution-profile errors. |
src/lib/db/factory.ts |
Adds isolated profile acquisition and caching. |
src/lib/db/operations/artifacts.ts |
Stores run-scoped execution artifacts. |
src/lib/db/operations/budgets.ts |
Tracks execution-budget consumption. |
src/lib/db/operations/descriptors.ts |
Defines canonical operations. |
src/lib/db/operations/execution.ts |
Coordinates policy, audit, and artifacts. |
src/lib/db/operations/policy.ts |
Implements the decision pipeline. |
src/lib/db/operations/registry.ts |
Implements fail-closed registration. |
src/lib/db/operations/statement-guard.ts |
Classifies bounded SQL reads. |
src/lib/db/operations/types.ts |
Defines operation and risk types. |
src/lib/db/providers/sql/postgres.ts |
Adds PostgreSQL read-only execution. |
src/lib/db/providers/sql/read-only-budget.ts |
Validates statement budgets. |
src/lib/db/providers/sql/sqlite-driver.ts |
Maps SQLite read-only flags. |
src/lib/db/providers/sql/sqlite.ts |
Adds SQLite read-only execution. |
src/lib/db/types.ts |
Extends provider contracts. |
src/lib/storage/connection-secrets.ts |
Classifies agent credentials. |
src/lib/types.ts |
Adds agent credential fields. |
tests/api/db/query.test.ts |
Protects the editor write path. |
tests/integration/db/postgres-provider.test.ts |
Exercises PostgreSQL profile behavior. |
tests/integration/db/sqlite-node-harness.ts |
Tests the Node SQLite adapter. |
tests/integration/db/sqlite-provider.test.ts |
Tests SQLite native enforcement. |
tests/security/agent-execution-audit.test.ts |
Verifies agent audit behavior. |
tests/security/agent-statement-boundary.test.ts |
Tests hostile SQL boundaries. |
tests/security/audit-redaction.test.ts |
Tests correlation-ID redaction. |
tests/unit/db/factory.test.ts |
Tests profile acquisition and isolation. |
tests/unit/db/operations/artifacts.test.ts |
Tests artifact lifecycle. |
tests/unit/db/operations/budgets.test.ts |
Tests budget accounting. |
tests/unit/db/operations/descriptors.test.ts |
Tests canonical descriptors. |
tests/unit/db/operations/execution.test.ts |
Tests audited execution. |
tests/unit/db/operations/policy.test.ts |
Tests policy stages. |
tests/unit/db/operations/registry.test.ts |
Tests registry denials. |
tests/unit/db/operations/statement-guard.test.ts |
Tests SQL classification. |
tests/unit/db/sqlite-driver.test.ts |
Tests read-only option mapping. |
tests/unit/lib/storage/connection-secrets.test.ts |
Tests agent-secret encryption. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const AGENT_ROLE_PRIVILEGE_SQL = ` | ||
| SELECT current_setting('is_superuser') = 'on' AS is_superuser, | ||
| COALESCE(pg_has_role(current_user, to_regrole('pg_read_server_files'), 'USAGE'), false) | ||
| AS reads_server_files, | ||
| COALESCE(pg_has_role(current_user, to_regrole('pg_write_server_files'), 'USAGE'), false) | ||
| AS writes_server_files, | ||
| COALESCE(pg_has_role(current_user, to_regrole('pg_execute_server_program'), 'USAGE'), false) | ||
| AS executes_programs | ||
| `; |
There was a problem hiding this comment.
Valid, and fixed in 9f8f21b. Confirmed the mechanism: pg_catalog is searched implicitly first only while it is not named in search_path, so a path naming it behind another schema lets a shadow pg_has_role() answer four falses for a superuser and defeat the one check meant to catch that role. The coherent attacker is the same one this layer already assumes: whoever can plant prompt-injection text in a table can often also create a function.
One correction to the suggestion, which the test now encodes. Only real catalog functions are shadowable, so current_setting, pg_has_role and to_regrole are now pg_catalog-qualified, while COALESCE and current_user are deliberately left alone - they are SQL constructs the parser resolves, cannot be schema-qualified at all, and no user function can intercept them. Qualifying them would have been a syntax error.
Test-first: schema-qualifies every built-in the privilege probe calls asserts per shadowable function and was RED before the fix.
| account.activeExecutions -= 1; | ||
| account.executedStatements += statements; | ||
| account.totalElapsedMs += elapsedMs; |
There was a problem hiding this comment.
Accurate description, and the documentation was the defect - fixed in 9f8f21b, with the control itself deferred rather than half-built.
maxTotalRunMs does sum completed provider-call durations, so it bounds DATABASE time, not the run wall clock: time between calls is uncounted, parallel calls each contribute, and a call admitted just under the limit can overrun by up to one statement timeout. All three are now stated in the module header instead of the previous "total-run wall clock" phrasing, which claimed something the counter never measured.
Not implementing the monotonic deadline here is deliberate. ExecutionBudgetTracker has no clock on purpose - the execution layer reports elapsed time so budget accounting stays deterministic under test - and a run-lifetime bound belongs to the component that owns a run lifetime, which is the WorkflowAgent run loop in #329. Recorded as docs/BACKLOG.md A4 with the acceptance condition you describe (monotonic deadline, refuse calls that cannot finish in the remaining time, clamp each statement timeout to what is left).
| // A typed profile refusal keeps its identity: wrapping it would strip the | ||
| // deny reason code callers branch on. | ||
| if (error instanceof ExecutionProfileError) { | ||
| // The pool was built before the refusal; leaving it would leak the | ||
| // sockets of a provider the caller can never use. | ||
| const refusedPool = this.pool; | ||
| this.pool = null; | ||
| await refusedPool?.end().catch(() => {}); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
Valid, and fixed in 9f8f21b. Verified the leak path: acquireExecutionProfileProvider (factory.ts) closes a fresh tunnel and rethrows when connect() fails, but never calls disconnect(), so a pool left open here keeps its idle socket and timers with no reference left that could close them.
Cleanup now runs on every failed connect, before the typed-error branch, so the borrow failing and the probe query rejecting are covered along with the refusal - and ExecutionProfileError still keeps its identity and reason code. The new test ends the pool when the privilege probe itself fails drives the probe to reject with a dropped-socket error and asserts both the ConnectionError wrap and pool.end(); it was RED before the change.
| /** | ||
| * Stateful engine mock modeling the PostgreSQL behaviors this profile's | ||
| * security rests on, so the assertions hold even when product-side | ||
| * classification is bypassed. Every rule below was verified against a live | ||
| * PostgreSQL 18 rather than assumed: |
There was a problem hiding this comment.
Fair, and recorded as docs/BACKLOG.md A5 rather than fixed in this PR.
The gap is real but narrower than "a hand-written model instead of the real thing": every rule the mock encodes was verified against a live PostgreSQL 18 while the profile was built - read-only rejection by engine state, the extended-protocol refusal of multi-command strings, SET TRANSACTION READ WRITE genuinely relaxing the transaction, advisory locks surviving rollback - and bypass attempts fail on modeled behavior (a write actually landing) rather than on protocol metadata. What that cannot catch is exactly what you name: a future driver or server change would leave the assertions green, because the mock defines the semantics.
Doing it here would change the repository test architecture, not just this file: tests/integration/ is mock-based by convention across every provider and CI runs no database service. The only real engine in the pipeline today is the throwaway PostgreSQL container behind loop/scripts/functional-smoke.sh, which is why A5 names extending that container as the cheap path rather than adding a service to every test job.
…s existing rationale registeredIds() sorts internal operation ids for a deterministic enumeration that tests assert by exact array equality. Nothing there is user-facing text, so localeCompare would tie the order to the host locale and could differ between a developer machine and CI - a bug rather than a fix. This is the same call already made and documented for schema-diff/diff-engine.ts, so the file gets the same per-file suppression and an in-code comment stating why. The quality gate failed on new_reliability_rating alone (this single issue); security, maintainability, coverage and duplication were all green.
) Two review findings, both fixed test-first. The privilege probe called its catalog functions unqualified. pg_catalog is searched implicitly first only while it is not named in search_path; once a path names it explicitly, any schema ahead of it shadows built-ins. So `search_path = attacker_schema, pg_catalog` plus a shadow pg_has_role() makes the probe answer four falses for a superuser, defeating the one check meant to catch exactly that role - and whoever can plant prompt-injection text in a table can often also create a function, so both reach the same attacker. current_setting, pg_has_role and to_regrole are now pg_catalog-qualified. COALESCE and current_user are deliberately left alone: they are SQL constructs the parser resolves rather than functions name resolution can redirect, so they cannot be qualified and cannot be shadowed. The test asserts qualification per shadowable function, not per token, for that reason. The pool was only ended when the profile refused a role. Any other failure after the pool exists - the borrow, or the probe query rejecting on a dropped socket or protocol error - fell through to the ConnectionError wrap with the pool still open, and acquireExecutionProfileProvider drops a provider whose connect() threw without disconnecting it, so nothing was left holding a reference that could close it. Cleanup now runs on every failed connect, before the typed-error branch, which still preserves ExecutionProfileError identity. Also documents what maxTotalRunMs actually bounds. It sums the elapsed time of completed provider calls, so it limits DATABASE time, not the run's wall clock: time between calls is uncounted, parallel calls each contribute, and a call admitted just under the limit can overrun by up to one statement timeout. That is the right bound at this layer, and a real deadline needs a clock the tracker deliberately does not have; bounding a run's lifetime belongs to the M2 run loop (docs/BACKLOG.md A4). BACKLOG A5 records the other open review point: these regression tests model PostgreSQL rather than run one, so a driver or server regression would leave them green. Gate green, coverage 100% (30788/30788 lines).
|
Implements #328 — the canonical database-operation layer the agent programme (epic #325) is built on: typed operation descriptors with risk classes, a fail-closed policy pipeline with execution budgets, and database-native read-only execution profiles for PostgreSQL and SQLite.
No route reaches this layer yet. That wiring is #329 (M2) and later; this PR adds the enforcement layer and its tests only, with no new runtime dependency and no AI SDK or Node 24 coupling.
What this adds
src/lib/db/operations/{types,registry,descriptors}.ts). Risk classes 2-6 are structurally inexpressible rather than flagged off: the registrable-descriptor type is a union of class 0 and verified class 1 members only, so there is no disabled slot for anyone to enable later.register()throws (registration is a trusted build-time act that must fail loudly);resolve()never throws and returns a typed resolved/denied union, so a caller cannot ignore a denial. Ids that merely case- or whitespace-normalize to a registered id are denied as ambiguous rather than helpfully corrected.policy.ts,budgets.ts). Stages evaluate in a pinned order — actor/session, classification, target scope, schema-validated input, provider capability, risk/mode/role, budgets — and the first denial wins. Every outcome is a typed allow / deny / require-approval carrying a reason code, a policy version and a frozen effective budget. On any deny the provider is never invoked, asserted with a spy provider. The existing rate-limit primitives were evaluated for budget accounting and rejected with reasons (fixed-window, monotonic, eviction fails open — correct for rate limiting, a bypass for a security budget).factory.ts,postgres.ts). Agent execution acquires a provider from a physically separate cache keyed by profile plus connection id, never agetOrCreateProviderentry, and runs one statement perBEGIN READ ONLYtransaction with a transaction-local timeout, then rolls back, runsDISCARD ALLand releases. Single-statement is enforced by the server through the extended query protocol, never by parsing.sqlite.ts,sqlite-driver.ts). A separate read-only open that creates neither file nor directory and skips the shared path's write-implying pragmas, withquery_onlyre-asserted and re-verified before every statement. The driver contract gained read-only support on both adapters, with contract tests that fail on an adapter that silently ignores the flags — which the Node adapter did before this PR.tests/security/agent-statement-boundary.test.tsplus both provider integration files) covering comment and whitespace obfuscation, nested and data-modifying CTEs, multi-statement input, mutating pragmas, attach and detach, extension loading, and plan-execution bypass attempts. Each case asserts the pipeline decision where classification catches it and, more importantly, that the database-native boundary still holds when classification is bypassed.execution.ts,artifacts.ts,audit.ts). Two correlated events per allowed execution and one per refusal, joined by a correlation id. An unauditable execution fails closed: the audit call is deliberately not wrapped in a try/catch, because this path is about to touch a database.Two real security defects were found and fixed while building this
Both were found by probing running engines, not by reading code, and neither was visible to the test gate.
query_onlyoff,VACUUM INTO '<path>'wrote a readable copy of the database, seeded secret included, to an arbitrary server path. The first implementation set the pragma once at open, and its own test asserted the pragma could be turned back off — so the profile shipped an exfiltration primitive that its tests documented as acceptable. Fixed by re-asserting and re-verifying the pragma before every statement, which is unbypassable becauseprepare()compiles exactly one statement: the disable and the write it would enable can never ride in the same call.BEGIN READ ONLYforbids changing the database and nothing else. Inside that transaction as a privileged user,COPY (...) TO '/path'wrote files,COPY (...) TO PROGRAM 'cmd'executed shell commands as the postgres OS user, andpg_read_file()read arbitrary files. Agent credentials had been optional with a fallback to the connection's own user, which in most deployments is exactly such a user. Fixed by verifying the role at profile open — refused unless the role is not a superuser and not a member ofpg_read_server_files,pg_write_server_filesorpg_execute_server_program. Requiring configured agent credentials was considered and rejected: configuration is not proof, since an admin can point them at a superuser.The lesson generalizes and is worth carrying into future provider work: a boundary claim is always about a specific resource. Before asserting that the engine enforces something, name the control's scope object, then look for a statement that writes or reads somewhere else. Neither original test suite could have caught these, because both only attempted writes to the target database, which is precisely where the control does work.
A related bypass is worth calling out: PostgreSQL accepts
ANALYSEas a full synonym forANALYZEand executes it, so a guard that knew one spelling let plan execution through the approval-free plan-inspection descriptor. A risk class and its approval gate were bypassed by one vowel, and the test passed because all its fixtures used the same spelling.Known limitations, recorded rather than hidden
These are documented in
docs/BACKLOG.md(A1-A3), the provider docs anddocs/SECURITY.mdrather than covered by tests that would imply a boundary the engines do not provide:ATTACHof an existing file succeeds on a read-only handle; bun:sqlite exposes no authorizer and node:sqlite's exists but is therefore not usable as a cross-adapter control. Attach is denied at the input stage only.statementTimeoutMsis a post-execution deadline, not preemption: neither adapter exposes an interrupt, and the synchronous drivers mean an overrunning statement blocks the runtime.docs/SECURITY.mdrows 3.4 and 3.5 are marked Partial on purpose, since no route reaches this layer yet.Verification
Built test-first throughout, one task per commit, each with its own green gate and a fresh-context adversarial review. Three of the seven tasks were blocked by that review on real security defects the gate could not see.
./loop/scripts/gate.shgreen (format, lint, typecheck, knip, test, build)test:coverageandcoverage:checkdocs/providers/<id>.mdand the integration tests move together, PostgreSQL and SQLite onlyReviewer note
The role verification in the PostgreSQL profile changes the credential matrix introduced earlier in this same branch: agent credentials stay optional, but the resolved role is now verified at open and refused if it is privileged. Nothing consumes this layer yet, so no existing behavior changes.
Closes #328