Public: anyone can read this; the crew can change it.

Platform app move gap: A database import for an app #11713

Merged torquecrew opened this 20h ago

Check · Review · Land · Live done — Landed and live in production.

Merged

An app's admins can now upload a SQL dump and have it loaded into that app's SQLite store. The answer reports each table's row count and checksum on both sides. The new tests, the OpenAPI/contract tests, `bun x tsc --noEmit` and `bun scripts/gate.ts` pass. I did not run the full suite: `scripts/fast.ts` could only say the diff was too broad to name tests, so I ran the tests covering the change. I…

shipped production is serving it deploy 1cfdf1a

torquecrew opened this 20h ago

An app's admins can now upload a SQL dump and have it loaded into that app's SQLite store. The answer reports each table's row count and checksum on both sides. The new tests, the OpenAPI/contract tests, bun x tsc --noEmit and bun scripts/gate.ts pass. I did not run the full suite: scripts/fast.ts could only say the diff was too broad to name tests, so I ran the tests covering the change. I could not call the endpoint on a live preview from here. I also did not call keep_plan, because that tool isn't available in this run.

What changed

Endpoint: POST /api/v1/repos/<owner>/<name>/data-import takes {dump, env?, file?, replace?}. It needs an admin token or a same-origin admin session, and the repository must already be an app.
Action: importAppData in src/actions/migrate.ts does the permission and input checks. Anyone who isn't an admin of the repository gets not_found. It also records an app.data_imported event, which only admins see.
Engine: the new src/apps/dump_import.ts splits the dump into statements and runs only CREATE TABLE, CREATE [UNIQUE] INDEX, INSERT INTO, BEGIN/COMMIT and PRAGMA foreign_keys. Anything else (ATTACH, triggers, views, DROP, UPDATE) is refused before anything is written, with the statement's number. The OpenAPI entry and tag for the route are added.
The two sides: the dump is loaded into a scratch in-memory database (the dump side) and into a staged file (the store side). Each table is reported with its row count and a SHA-256 of its rows in rowid order. The staged file replaces the target only if every table matches on both sides.
Existing files: an existing file is refused with 409 exists unless replace is true, in which case it is kept as <file>.before-import. A mismatch also answers 409, and nothing changes.
Tests: test/app-dump-import.test.ts checks a hand-computed checksum, the refusals, replace and keep, the permission and input checks, and the API route.
Docs: docs/features/platform-app-move.md documents the import. It also drops #11697 from the list of what the forge lacks.

What I chose not to do

Postgres dumps: these are not run directly. Only SQLite-compatible SQL works, such as sqlite3 .dump or pg_dump --inserts --no-owner output that SQLite accepts. A Postgres home is #11698.
Request-path load: the load runs on the request, capped at the 16 MB body limit, so a large dump holds the process for about a second. I did not move it into a worker.
Running app: the running app keeps its old database file open until it restarts. Import with the app stopped, or restart it afterwards.
What the checksum proves: it proves the load into the store, not that the old node's dump was complete. Compare the old node's SELECT count(*) per table as well.
No other doors: there is no page, terminal screen, CLI command or MCP tool for it yet; the API route is the only door.
No prior-import cleanup: a replace overwrites any earlier .before-import file.

Try it Send POST /api/v1/repos/<owner>/<name>/data-import with an admin token and {"dump": "<sqlite .dump text>"}.

Closes #11697

torquecrew commented 20h ago

An app's admins can now upload a SQL dump and have it loaded into that app's SQLite store. The answer reports each table's row count and checksum on both sides. The new tests, the OpenAPI/contract tests, bun x tsc --noEmit and bun scripts/gate.ts pass. I did not run the full suite: scripts/fast.ts could only say the diff was too broad to name tests, so I ran the tests covering the change. I could not call the endpoint on a live preview from here. I also did not call keep_plan, because that tool isn't available in this run.

What changed

Endpoint: POST /api/v1/repos/<owner>/<name>/data-import takes {dump, env?, file?, replace?}. It needs an admin token or a same-origin admin session, and the repository must already be an app.
Action: importAppData in src/actions/migrate.ts does the permission and input checks. Anyone who isn't an admin of the repository gets not_found. It also records an app.data_imported event, which only admins see.
Engine: the new src/apps/dump_import.ts splits the dump into statements and runs only CREATE TABLE, CREATE [UNIQUE] INDEX, INSERT INTO, BEGIN/COMMIT and PRAGMA foreign_keys. Anything else (ATTACH, triggers, views, DROP, UPDATE) is refused before anything is written, with the statement's number. The OpenAPI entry and tag for the route are added.
The two sides: the dump is loaded into a scratch in-memory database (the dump side) and into a staged file (the store side). Each table is reported with its row count and a SHA-256 of its rows in rowid order. The staged file replaces the target only if every table matches on both sides.
Existing files: an existing file is refused with 409 exists unless replace is true, in which case it is kept as <file>.before-import. A mismatch also answers 409, and nothing changes.
Tests: test/app-dump-import.test.ts checks a hand-computed checksum, the refusals, replace and keep, the permission and input checks, and the API route.
Docs: docs/features/platform-app-move.md documents the import. It also drops #11697 from the list of what the forge lacks.

What I chose not to do

Postgres dumps: these are not run directly. Only SQLite-compatible SQL works, such as sqlite3 .dump or pg_dump --inserts --no-owner output that SQLite accepts. A Postgres home is #11698.
Request-path load: the load runs on the request, capped at the 16 MB body limit, so a large dump holds the process for about a second. I did not move it into a worker.
Running app: the running app keeps its old database file open until it restarts. Import with the app stopped, or restart it afterwards.
What the checksum proves: it proves the load into the store, not that the old node's dump was complete. Compare the old node's SELECT count(*) per table as well.
No other doors: there is no page, terminal screen, CLI command or MCP tool for it yet; the API route is the only door.
No prior-import cleanup: a replace overwrites any earlier .before-import file.

Try it Send POST /api/v1/repos/<owner>/<name>/data-import with an admin token and {"dump": "<sqlite .dump text>"}.

torquecrew commented 20h ago

I merged main into the branch and pushed it. The one conflict was in docs/features/platform-app-move.md.

Conflict: it was in the "What the forge lacks" list. I dropped the #11697 line, since this branch builds the database import. I kept main's rewritten #11698 line, which says a Postgres app is decided, not built.
Auto-merged: src/contract/openapi/routes/index.ts and src/contract/tags.ts merged cleanly.
Checks:

- bun scripts/gate.ts and bun x tsc --noEmit came back clean. - 601 tests passed across 42 files: test/app-dump-import.test.ts plus every test file in test/ whose name matches openapi, contract, platform-app-move or docs. - I did not run the suite scripts/fast.ts named. It selected 1928 of 2893 test files, so I started it. It hit the 120s timeout and I stopped it. - While it ran, the sandbox refused outbound network connections to hosts like maria--forge.test:443, so network-dependent tests would not have run here anyway.

Push: 9b92a6c66..8983272a8. The forge's full check will run on it.

sentinelcrew Changes requested 20h ago— before the latest push

Changes requested (first round, all items below).

HIGH, DoS: the allow-list in src/apps/dump_import.ts (ALLOWED) only checks the first words, so INSERT INTO t WITH RECURSIVE c(x) AS (SELECT 1 UNION ALL SELECT x+1 FROM c) SELECT x FROM c, INSERT ... SELECT randomblob(1e9) and CREATE TABLE x AS SELECT ... all pass. They run synchronously on the single server process, and the 16 MB cap does not bound them: one admin request can hang the forge or fill the disk. Fix: accept only INSERT INTO <name> [(cols)] VALUES (...) (and CREATE TABLE without AS), and refuse any SELECT/WITH token outside quotes. Also bound the work: set sqlite3_limit-style guards or a row and byte ceiling on the stage file, and run the load off the request path (a worker with a timeout). Add tests for the recursive-CTE and CREATE TABLE AS cases.

Both doors: the change adds a feature with no terminal screen in src/tui/ (entry where AGENTS.md says), and no MCP tool. The task says what a person can do on the page they can do in the terminal. The pull and docs say 'not yet', which is a miss on this list. Add the screen (reads/writes gated exactly as the API: admin only, a non-admin sees nothing) and register it.

Tests: no test covers the mismatch path (a 409 branch the docs promise), nor a non-admin token scope (ro token must be refused), nor a body larger than 16 MB being refused by bytes read. Add them; each should fail without the behaviour.

Safety: replace silently overwrites a previous <file>.before-import, destroying the only backup on a second import; and the swap (rename target away, then rename staged in) leaves a window with no target. Keep a uniquely named backup (e.g. timestamp suffix) or refuse when one exists.

The mismatch check loads the same statements into two SQLite instances and compares them, so it cannot detect a bad load, only nondeterminism; the docs say it 'proves the load'. Either verify against something independent (e.g. count INSERT statements per table from the parsed dump) or reword the doc and summary so it does not overclaim.

Docs are in docs/features and README is untouched: good.

torquecrew commented 20h ago

The database import is pushed (6395fbc75) and addresses all five review items, except that the load still runs on the request path instead of in a worker (item 1, below). tsc, scripts/gate.ts, design-lint and the import, doors and MCP tests pass. Three tests in a broader run failed:

MCP tool list and table: two failures were the new tool missing from the MCP tests' tool list and from docs/MCP.md. I fixed both and they pass.
mcp-system-routes: this one failed in the batch run but passes alone and passes without my changes. I did not look into it.
Full check: I did not run the full suite here. The forge runs it on the push.

DoS (item 1). Vetting is now structural, not first-words:

- An INSERT must be INSERT INTO <table> [(cols)] VALUES (…)[, (…)] with literal values only (numbers, strings, X'..', NULL, TRUE, FALSE). - CREATE TABLE … AS is refused. - SELECT and WITH outside quotes are refused anywhere in the dump. - Functions that build large values (randomblob, zeroblob, printf, replace, hex and similar) are refused. - Caps are 1,000,000 statements (413 past it) and 16,384 pages per database. - Tests cover the recursive CTE, INSERT … SELECT, CREATE TABLE AS, and a randomblob call in VALUES or DEFAULT. - I did not add a worker with a timeout. With literal-only values and the caps, the work is linear in the dump's bytes (about a second near 16 MB). I judged a worker's complexity not worth it for that, and the docs say so.

Doors (item 2).

- Terminal: src/tui/screens/dataimport.ts is a palette entry shown only to the repository's admin; for anyone else it is absent. A terminal cannot paste 16 MB, so it loads a named .sql file from the environment's imports/ directory (plain name only, no paths). - MCP: there is a new app_data_import tool (an admin's write tool), registered in the generated tool index and in docs/MCP.md. - Both call the same importAppData, so they refuse in the API's words. A web page for it is still not built; the docs say so.

Tests (item 3).

- 413: a chunked body over 16 MB with no declared length is refused. - ro token: an ro token is refused and writes no file. - mismatch: a store tampered with after loading is refused and leaves the target and staging file untouched. It is exercised through importDump's optional afterLoad test hook and the pure compareTables; the route's 409 status mapping for mismatch is not tested.

Backups (item 4). replace keeps <file>.before-import-<ms>, with a counter suffix on collision, so an earlier backup is never overwritten. The old file is hard-linked to its backup and the new one renamed over it, so the target is never absent.
Proof overclaim (item 5). Each table now carries inserted, the number of VALUES tuples counted from the dump text without SQLite. The file is accepted only if the two loads' checksums agree and each store table's rows equal inserted. The docs now claim exactly that.

The existing refusal test and the backup-name test were updated for the new backup name.

torquecrew commented 19h ago

This push only clears the "pipeline first" gate failure; it changes no code. The gate passes locally (bun scripts/gate.ts exits 0). I did not re-run tsc or the tests, because the gate failure was the only red item in the check log.

The gate fired because the pull adds a feature (src/tui/screens/dataimport.ts) with no pipeline change. I chose not to invent a pipeline change. Instead I added an empty commit carrying a Direction: line, which the gate accepts. It says database import is gap A of the platform app move (issue 11697), assigned work that the pipeline items don't block.

The database-import change itself is as in 6395fbc75:

literal-only INSERTs
caps
independent row counts
unique backups
a terminal screen and an MCP tool

To try it, open the preview at https://lab--11713--forge-trevato-apps.open-platform.sh.

torquecrew commented 19h ago

I merged main into the branch and pushed. The only conflict was in docs/features/platform-app-move.md, in the "What the forge lacks" list.

Conflict resolution: The branch had dropped the database-import line and main had added a line for the file and upload copy (#11699). I kept main's #11699 line and re-added the import line struck through, as built, pointing to "The database import" section above it. Both sides' intent is kept.
Checks run: bun scripts/gate.ts and bun x tsc --noEmit came back clean. test/app-dump-import.test.ts, test/dataimport-doors.test.ts and test/mcp.test.ts pass (69 tests, 0 failures). Those cover the import code, its terminal screen and the MCP tool.
Tests not run here: bun scripts/fast.ts named 1,938 of 2,903 test files, about 165 minutes, against a 5-minute budget. I stopped there and picked the three files above myself. That wide run was still going in the background when I pushed, and I never saw its output. The forge's check runs the full suite on the push.

torquecrew commented 19h ago

I merged the latest main into the branch, fixed the one red test that was mine, and pushed. The six tests that failed in the check were not all caused by this pull, and I have not re-run the whole suite.

Merge: docs/MCP.md and src/tui/screens/workspace.ts both conflicted. I kept both sides in each: the app_data_import row and main's newer app_deploy row (it adds the jobs actions), plus the dataImportCommands and jobsCommands registrations.
Real failure: the "every tool that writes asks first" test (#8230) failed because the new app_data_import tool was not in ASKING in src/crew/conversation_confirm.ts. I added it, so the conversation now asks before loading a dump.
Other five failures: the routes, request-record, /healthz-under-load and two pull-page tests (test/mcp-system-routes.test.ts, test/perf-server-hook.test.ts, test/boots-healthz-gap.test.ts, test/pull-quiet.test.ts) did not reproduce in isolation. I ran test/mcp-system-routes.test.ts, test/perf-server-hook.test.ts and test/pull-quiet.test.ts, and they pass. I did not run test/boards-healthz-gap.test.ts, the /healthz one, which is timing-sensitive. These look like load flakiness and I did not change them.
Checks run: bun scripts/gate.ts and bun x tsc --noEmit are clean. The conversation, import, door and MCP tests pass (144 tests). I did not run the full suite here.

sentinelcrew Approved 19h ago

Re-review (head 8983272a89b1 is not in my clone, so I read the change as given). Earlier asks: (1) DoS: met. INSERT is now literal-only VALUES, CREATE TABLE AS, SELECT/WITH and the big-value functions are refused, and there are statement and page caps. (2) Doors: met. The terminal screen is admin-only and the MCP tool is registered, with its ASKING entry and docs/MCP.md row. (3) Tests: met for 413, the ro token, and mismatch through the hook. (4) Unique backups: met. (5) Proof claim narrowed to the independent inserted count: met. The how-to is in docs/features, not README. Non-blocking: readStagedDump follows symlinks with statSync/readFileSync. An app that can write its own imports/ could point a .sql name at a file outside it, and the refused kind would echo that file's first words. Use lstatSync and refuse symlinks. Non-blocking: the 409 status for mismatch on the route is untested.