Add dev pairing and seed utilities#4557
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
github-actions
Bot
added
vouch:trusted
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review This PR introduces substantial new dev tooling capabilities (~2000+ lines) including a new CLI command for pairing URLs and a database seeding script with destructive operations. While well-tested and dev-focused, the scope of new functionality warrants human review. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
|
Picked this up, reviewed it, and pushed fixes in 331fc71. Review found five ways a seeded database could describe something the copy cannot back up. All are in the
Turns copied mid-flight stayed Attachment metadata was copied without the files. Orphaned provider bindings survived.
Also corrected a comment: it claimed Verification: 33 tests pass (9 new covering the guard, turn settling, streaming, attachment ids, and binding cleanup), scripts typecheck and lint clean. |
github-actions
Bot
added
size:XXL
|
Second round pushed in 73669b3, covering the Macroscope comment and findings from a follow-up review of the pairing path.
Turns copied in the The source was read across a dozen implicit transactions while the installed app may be writing to it, so a project could vanish between the query that selected it and the copy that read its row — leaving an orphaned thread, with no foreign keys to catch it. Now one deferred read transaction pinning every query to a single snapshot.
Verification: 38 tests pass, full-repo typecheck reports 0 errors, lint clean on changed files. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
edc8e4f to
dbad3bc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
scripts/mobile-showcase-environment.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFinish the dedup:
PROJECTION_TABLES_IN_DEPENDENCY_ORDERis also exported now.
seedDatabasestill inlines the same nine-table delete list that the new module exports, so the two can drift the moment a projection table is added.♻️ Import the shared list too
-import { PROJECTOR_NAMES } from "./lib/projection-tables.ts"; +import { + PROJECTION_TABLES_IN_DEPENDENCY_ORDER, + PROJECTOR_NAMES, +} from "./lib/projection-tables.ts";Then in
seedDatabase:for (const table of PROJECTION_TABLES_IN_DEPENDENCY_ORDER) { database.exec(`DELETE FROM ${table}`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/mobile-showcase-environment.ts` at line 2, Update the imports in scripts/mobile-showcase-environment.ts to include PROJECTION_TABLES_IN_DEPENDENCY_ORDER, then replace seedDatabase’s inline projection-table delete list with iteration over that shared exported list while preserving the existing DELETE execution.scripts/dev-seed.ts (1)
266-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
catch: (cause) => cause as DevSeedErroris an unchecked cast.
seedDevDatabasewraps its own failures today, so this holds in practice, but any future throw that isn't aDevSeedError(or isn't anErrorat all) reaches thetapErrorat Line 312-315 and dereferences.messageon it. Normalizing here keeps the error channel type honest.♻️ Normalize unexpected causes
- catch: (cause) => cause as DevSeedError, + catch: (cause) => + cause instanceof DevSeedError + ? cause + : new DevSeedError(`could not seed the dev database: ${String(cause)}`),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/dev-seed.ts` around lines 266 - 276, Replace the unchecked cast in the Effect.try catch handler around seedDevDatabase with normalization that preserves existing DevSeedError instances and converts unexpected thrown values into a DevSeedError-compatible error. Ensure non-Error causes are also safely represented so the downstream tapError handling can access message without throwing.scripts/lib/dev-seed.test.ts (1)
545-572: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider marking the 32k-thread fixture as a slow test.
Building 32,766 thread and turn rows on every run of this file is the one materially slow case here. A
it.skipIf/tag or a longer explicit timeout keeps the focused-test workflow snappy without losing the ceiling guarantee in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/dev-seed.test.ts` around lines 545 - 572, Mark the maximum-thread-limit test identified by its “seeds at the maximum thread limit without exhausting SQL variables” description as slow, using the project’s existing skip/tag mechanism or an explicit extended timeout. Preserve its CI execution and the existing ceiling assertions while keeping focused local test runs fast.packages/tailscale/src/tailscale.ts (1)
219-228: 📐 Maintainability & Code Quality | 🔵 TrivialMinor:
stderrPreviewOf(stderr)computed twice per call site.Both
readTailscaleStatusandrunTailscaleCommandcallstderrPreviewOf(stderr)once in the ternary condition and again in the object body. Hoisting into a local avoids the redundant (if cheap) call and reads slightly cleaner.♻️ Proposed dedup
- ...(stderrPreviewOf(stderr) !== undefined - ? { stderrPreview: stderrPreviewOf(stderr) } - : {}), + ...(preview !== undefined ? { stderrPreview: preview } : {}),(with
const preview = stderrPreviewOf(stderr);added above each call site)Also applies to: 286-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tailscale/src/tailscale.ts` around lines 219 - 228, Deduplicate stderrPreviewOf(stderr) in both readTailscaleStatus and runTailscaleCommand by assigning its result to a local preview variable before constructing TailscaleCommandExitError, then reuse that variable for the conditional property and stderrPreview value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/test-t3-app/SKILL.md:
- Around line 51-58: Update the later troubleshooting guidance that recommends
manually minting replacement tokens to direct agents to run the existing `bun
run dev:pair` workflow instead, preserving the documented `--base-dir` usage
when applicable and removing the obsolete manual-token recovery instructions.
In `@docs/reference/scripts.md`:
- Around line 54-55: Update the runtime-state documentation to describe probing
both dev and userdata for the directory containing the active database,
resolving dev/userdata ties by modification time. In docs/reference/scripts.md
at lines 54-55, replace the userdata-first wording; in
.agents/skills/test-t3-app/SKILL.md at line 58, explain that pairing checks both
directories and applies the same active-state selection rule.
- Around line 4-6: Standardize the documented development workflow on the
repository’s bun wrappers: in docs/reference/scripts.md lines 4-6, replace the
vp run dev, vp run dev:share, vp run dev:pair, and vp run dev:seed examples with
their bun run equivalents while preserving the existing options and
descriptions; in .agents/skills/test-t3-app/SKILL.md line 16, replace vp run dev
with bun run dev and keep the sharing variant as bun run dev:share.
---
Nitpick comments:
In `@packages/tailscale/src/tailscale.ts`:
- Around line 219-228: Deduplicate stderrPreviewOf(stderr) in both
readTailscaleStatus and runTailscaleCommand by assigning its result to a local
preview variable before constructing TailscaleCommandExitError, then reuse that
variable for the conditional property and stderrPreview value.
In `@scripts/dev-seed.ts`:
- Around line 266-276: Replace the unchecked cast in the Effect.try catch
handler around seedDevDatabase with normalization that preserves existing
DevSeedError instances and converts unexpected thrown values into a
DevSeedError-compatible error. Ensure non-Error causes are also safely
represented so the downstream tapError handling can access message without
throwing.
In `@scripts/lib/dev-seed.test.ts`:
- Around line 545-572: Mark the maximum-thread-limit test identified by its
“seeds at the maximum thread limit without exhausting SQL variables” description
as slow, using the project’s existing skip/tag mechanism or an explicit extended
timeout. Preserve its CI execution and the existing ceiling assertions while
keeping focused local test runs fast.
In `@scripts/mobile-showcase-environment.ts`:
- Line 2: Update the imports in scripts/mobile-showcase-environment.ts to
include PROJECTION_TABLES_IN_DEPENDENCY_ORDER, then replace seedDatabase’s
inline projection-table delete list with iteration over that shared exported
list while preserving the existing DELETE execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1139449d-5adc-49f6-a961-eb622b3e4674
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.agents/skills/test-t3-app/SKILL.md.agents/skills/test-t3-app/references/sqlite-fixtures.mdAGENTS.mdapps/server/src/auth/EnvironmentAuth.test.tsapps/server/src/auth/EnvironmentAuth.tsapps/server/src/auth/EnvironmentAuthPolicy.test.tsapps/server/src/auth/EnvironmentAuthPolicy.tsapps/server/src/auth/PairingGrantStore.tsapps/server/src/auth/SessionStore.tsapps/server/src/auth/utils.tsapps/server/src/cli/auth.test.tsapps/server/src/cli/auth.tsapps/server/src/config.tsapps/server/src/http.tsapps/server/src/serverRuntimeState.test.tsapps/server/src/serverRuntimeState.tsapps/web/vite.config.tsdocs/reference/scripts.mdpackage.jsonpackages/shared/package.jsonpackages/shared/src/devProxy.tspackages/tailscale/src/tailscale.tsscripts/dev-runner.test.tsscripts/dev-runner.tsscripts/dev-seed.tsscripts/lib/dev-seed.test.tsscripts/lib/dev-seed.tsscripts/lib/dev-share.test.tsscripts/lib/dev-share.tsscripts/lib/projection-tables.tsscripts/lib/server-runtime-probe.test.tsscripts/lib/server-runtime-probe.tsscripts/mobile-showcase-environment.tsscripts/package.json
| Ask the running server for a fresh pairing URL: | ||
|
|
||
| ```bash | ||
| T3CODE_PORT=<server-port> node apps/server/src/bin.ts auth pairing create \ | ||
| --base-dir <base-dir> \ | ||
| --dev-url <web-url> \ | ||
| --base-url <web-url> \ | ||
| --ttl 15m \ | ||
| --label agent-ui-test | ||
| bun run dev:pair | ||
| bun run dev:pair -- --base-dir <base-dir> | ||
| ``` | ||
|
|
||
| Use the `Pair URL` from this command once. Derive `<server-port>` and `<web-url>` from the current dev-runner output, including any automatically selected port offset. Setting `T3CODE_PORT` keeps the administrative CLI from probing for an unrelated free port. | ||
|
|
||
| Always pass `--dev-url` for a dev-runner environment so the generated pairing URL uses the current web origin. An explicit base directory stores runtime state in `<base-dir>/userdata`; the `<base-dir>/dev` fallback is only used by an implicit dev home. Use `auth pairing list` to inspect active token metadata; it intentionally cannot reveal token secrets. | ||
| It finds the live server state under either the `dev` or `userdata` state directory and reuses the recorded web origin, including a tailnet URL. Pass `--base-dir` only when the server was started with `--home-dir`. Use the printed URL once. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove the obsolete manual-token recovery path.
The new dev:pair workflow conflicts with the later troubleshooting guidance at Lines 86-88, which still tells agents to mint replacement tokens manually. Update that later guidance to call bun run dev:pair instead.
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 36: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/test-t3-app/SKILL.md around lines 51 - 58, Update the later
troubleshooting guidance that recommends manually minting replacement tokens to
direct agents to run the existing `bun run dev:pair` workflow instead,
preserving the documented `--base-dir` usage when applicable and removing the
obsolete manual-token recovery instructions.
| - `vp run dev --share` (or `vp run dev:share`) — Also publishes the web port over HTTPS on this machine's tailnet. The startup pairing URL is built against the shared origin, and the mapping is removed on exit. | ||
| - `vp run dev:pair` — Prints a fresh pairing URL for the running dev server, using its recorded state directory, port, and web origin. Add `--base-dir <path>` only when the server was started with `--home-dir`. | ||
| - `vp run dev:seed` — Copies recent projects and threads from the shared home into the isolated dev database. Stop the server before running it, then restart. Tune the copy with `--threads` and `--activities`; it refuses to write to the shared home. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one supported command wrapper in operational documentation.
docs/reference/scripts.md#L4-L6: standardize the new workflow examples on the repository'sbun runwrappers..agents/skills/test-t3-app/SKILL.md#L16-L16: replacevp run devwithbun run devand keep the sharing variant aligned.
As per coding guidelines, start the web stack with bun run dev; use bun run dev:share when access from another tailnet device is required.
📍 Affects 2 files
docs/reference/scripts.md#L4-L6(this comment).agents/skills/test-t3-app/SKILL.md#L16-L16
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/scripts.md` around lines 4 - 6, Standardize the documented
development workflow on the repository’s bun wrappers: in
docs/reference/scripts.md lines 4-6, replace the vp run dev, vp run dev:share,
vp run dev:pair, and vp run dev:seed examples with their bun run equivalents
while preserving the existing options and descriptions; in
.agents/skills/test-t3-app/SKILL.md line 16, replace vp run dev with bun run dev
and keep the sharing variant as bun run dev:share.
Source: Coding guidelines
| Both source and target follow whichever state directory actually holds a database — `userdata` when present, otherwise `dev` — so a main-checkout `bun run dev`, whose state lives in `~/.t3/dev`, is seedable without naming the directory. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document active state-directory selection consistently.
docs/reference/scripts.md#L54-L55: replace theuserdata-first wording with the active-database probe anddev/userdatatie-resolution rule..agents/skills/test-t3-app/SKILL.md#L58-L58: explain that pairing checks both directories and follows the same active-state rule.
The PR objectives state that runtime-state selection follows the active database directory and uses modification time to resolve dev/userdata ties.
📍 Affects 2 files
docs/reference/scripts.md#L54-L55(this comment).agents/skills/test-t3-app/SKILL.md#L58-L58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/reference/scripts.md` around lines 54 - 55, Update the runtime-state
documentation to describe probing both dev and userdata for the directory
containing the active database, resolving dev/userdata ties by modification
time. In docs/reference/scripts.md at lines 54-55, replace the userdata-first
wording; in .agents/skills/test-t3-app/SKILL.md at line 58, explain that pairing
checks both directories and applies the same active-state selection rule.
Review of the seeding path found five ways a seeded database could describe something the copy cannot back up: - Nothing stopped `dev:seed` from running against a live server. SQLite locking lets the seed take the write lock between the server's own writes and commit cleanly; what it cannot do is tell that server its projections and event log were replaced underneath it. Refuse instead, via a pid probe over both state directories. - A thread copied mid-turn kept `state = 'running'`. The session override did not cover it — the turn is read independently, and an unsettled one leaves the thread spinning and its timeline unfoldable forever. Settle those to "interrupted", matching what the server does when a session leaves "running". Streaming messages are finished for the same reason. - Attachment metadata was copied without the files behind it, so a seeded thread rendered an image whose request 404s. Report the referenced ids and copy the matching files alongside the rows. - Orphaned `provider_session_runtime` rows survived the seed naming threads it had just deleted, which the session reaper then tried to stop against a provider instance the worktree may not define. - `--threads` had no upper bound, so a large enough value died with "too many SQL variables" after the target was already emptied. Also corrects the projector-cursor comment: `orchestration_events.sequence` is AUTOINCREMENT, so emptying the log does not reset its high-water mark and sequence does not restart at 1. Zero remains the right cursor, for the reason now stated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up review of the pairing path and a second pass over seeding: - `isPersistedServerRuntimeStateLive` reported non-positive pids as live. `process.kill` reads 0 and -1 as signal-group targets rather than process lookups, so neither throws and both were caught by the "it didn't throw, so it's running" branch. A truncated or hand-edited file recording either would be believed forever — the dev candidate wins the devUrl preference, so it also outranks a genuinely live server elsewhere and mints tokens into a database nothing reads. Rejected in the schema and re-checked at the call site; the errno cast is now a guard, since an out-of-range pid throws a TypeError with no `code`. - `pairing url --json` emitted warnings on stdout ahead of the payload. The probe reads both state directories, so a corrupt file in the one *not* in use broke JSON for any consumer piping it; the quiet-logs layer was only applied around the issuing step that runs after. Verified with a crafted fixture: stdout is now valid JSON. - Turns copied in the `pending` state were left unsettled. Same defect as the `running` case fixed previously — a user message accepted before its provider turn started, which only that thread's next message would ever clear. - The source was read across a dozen implicit transactions while the installed app may be writing to it, so a project could vanish between the query that selected it and the copy that read its row. One deferred read transaction pins every query to a single snapshot. - `--from` and `--to` resolving to the same directory is now refused. Not for the data loss it appears to be (the source reads a pre-transaction snapshot, so the rows come back — verified in both journal modes), but because the event log, the cursors, and everything outside the caps do not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adding "pending" to the settled states bound them as parameters, costing two bindings on top of one per thread. `--threads` is capped at 32766 — SQLite's ceiling — so the documented maximum landed exactly two over and threw "too many SQL variables", after the target had already been emptied. Inline the literals; they are internal constants, never input. The regression test seeds at the cap for real rather than asserting the statement text, since the property is that no code path adds a binding. Fixture rows are batched into one transaction and the scale case builds thread rows only, keeping it near a second. Caught by Macroscope review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cap was declared in the CLI while the constraint belongs to the seeder's statements, and the number appeared in three places. That split is the mechanism behind the last regression: a change to a statement in the library silently invalidated a bound stated in the CLI, and the mismatch only surfaced at the maximum — after the target was emptied. Move it to `MAX_SEED_THREAD_LIMIT` beside the statements that have to satisfy it, have the flag derive its bound from there, and check it in `seedDevDatabase` before anything is opened so a caller bypassing the flag fails while the target is still intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three findings from Macroscope review: - Settling `pending` rows to `interrupted` was the wrong verb. The server deletes them (`clearPendingProjectionTurnsByThread`) once the turn starts or the thread stops, and never settles one in place. They also cannot be settled coherently: `turn_id` is NULL, so an "interrupted" pending row is a terminal turn with no id, a shape no real turn has. Now deleted, matching the server's predicate exactly so a checkpoint row is never caught by it. - `COALESCE(completed_at, requested_at)` preserved a stale completion on a running turn. One can carry a completion from an earlier settle that a later checkpoint reverted to "running" — the `...existingTurn` spread in ProjectionPipeline keeps the old value — and dating the interruption to that checkpoint gives a wrong duration. Overwritten unconditionally. - The CLI hard-coded `<base>/userdata/state.sqlite` while a dev server under an implicit dev home stores state in `<base>/dev`. A main-checkout `bun run dev` was therefore unseedable, and said so by claiming no database existed and suggesting the user start the server that was already running. Source and target now follow whichever directory holds a database, `userdata` breaking the tie; attachments follow the same directory. Verified both a dev-only target and the precedence order. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review of the settle path, which has been the source of every regression in this branch. The server filters `turnId !== null && state === "running"` before settling; this filtered only on state. No row is both null-id and running today — the sole null-id insert writes 'pending' — so this changes no behavior. It is the invariant the UPDATE already depends on, written down where it is relied upon, and it keeps the two predicates directly comparable if a null-id running row ever becomes possible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more from Macroscope review: - `ORDER BY COALESCE(...)` throws on a source down to one recency column. SQLite requires at least two arguments, so the pre-017 schema the column filter exists to support was exactly the case that aborted. Emit the column directly when only one survives the filter. - `seedDevDatabase` accepted non-positive limits. Both are bound into `LIMIT ?`, where SQLite reads a negative as "no limit" — a caller bypassing the CLI flags silently got a full-table copy instead of the bounded one the options contract promises. Rejected before the target is touched, alongside the existing upper-bound check. Both regression tests confirmed failing without their fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Preferring `userdata` whenever both directories exist was wrong for the case that motivated resolving at all: an implicit `bun run dev` stores state in `dev`, so a base directory that had also been used with an explicit `--home-dir` would seed the database the running server does not open — looking like the seed did nothing. Neither directory is categorically right, so the tie now goes to whichever was written most recently: the one the last server actually used. `userdata` still wins an equal or unavailable mtime. The chosen path is already printed, so a wrong guess is visible rather than silent. Verified both directions against a target holding both databases. Caught by Cursor Bugbot. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rebasing onto the updated #4556 turned up two doc drifts: the base moved the script list from `vp run` to `bun run`, and the state-directory paragraph still described the fixed `userdata` preference that the mtime tiebreak replaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3fa6578 to
97401b1
Compare
CodeRabbit flagged docstring coverage. Added docs where a caller would actually need them — that `seedDevDatabase` is destructive and not incremental, that it validates before opening anything so a rejected run leaves the target intact, that the caller still owns the attachment files, and that `DevSeedError.hint` is the actionable half the CLI prints separately. Left the self-describing internals (`placeholders`, `hasTable`, `columnsOf`, `stateDbPath`) alone: the surrounding scripts/ code documents about 2% of its declarations, so padding to an 80% tool default would read as noise rather than convention. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
On the docstring coverage warning: added docs to the exported surface in 33fcf80 — the parts a caller genuinely needs to know. Specifically that I stopped short of the 80% threshold deliberately. The remaining undocumented declarations are things like Happy to revisit if the 80% target is a deliberate repo standard rather than a CodeRabbit default — that's a maintainer call, not one I want to make unilaterally on someone else's codebase. |
Two findings from Macroscope review: - The running-server guard was check-then-act. A copy takes seconds, and nothing stopped a server starting after the check returned None — the seed would then replace the projections and event log underneath it, which is exactly what the guard exists to prevent. `seedDevDatabase` now takes an optional probe and re-runs it immediately before COMMIT, where a rollback still costs nothing. This narrows the window to the commit itself rather than closing it: a filesystem read and a SQLite transaction share no lock, so a server starting in that last instant is still possible. Documented as such rather than claimed as airtight. - The state-dir tiebreak read `state.sqlite` mtime, but the server runs SQLite in WAL mode, where writes land in `state.sqlite-wal` and the main file can stay untouched until a checkpoint. Verified: 50 writes left the main mtime unchanged while the sidecar advanced. So a busy directory lost the tie to a dormant one, and the seed overwrote the wrong database. Recency is now the newer of the two files. Both verified end-to-end: a backdated main file with a fresh -wal now wins the tie, and a probe returning a pid mid-copy leaves the target's rows and event log intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The sync probe added for the pre-commit re-check duplicates the Effect one's rules by hand — positive pid, live process, unparseable means no. Two implementations of one rule drift apart silently, and this branch has already produced several bugs of exactly that shape. Asserts every case against both probes rather than trusting them to agree: live in either state dir, dead pid, pid 0 and -1, malformed JSON, and JSON with no pid. Confirmed the parity test fails when the sync probe's positive-pid guard is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test schema omitted `projection_turns.checkpoint_turn_count`, which is the column `dropPendingTurnStarts` keys its "spare checkpoint rows" guard on. Every existing test therefore ran the degraded path where the clause is dropped for schema drift, so the guard's real behavior was never exercised. Added the column (migration 005 has it) and a case covering what the clause is for: a pending row carrying a checkpoint survives the delete. Confirmed the case fails when the clause is removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Auditing the fixture against the real migrations (the method that found the missing checkpoint column) turned up a second gap: the test schema omitted `projection_thread_activities.sequence`, and the copy's per-thread cap ordered on `created_at` alone. The app reads these back ordered by `sequence` first, then `created_at` (ProjectionSnapshotQuery). A burst of tool activity lands many rows on one timestamp, so capping on the timestamp alone cuts arbitrarily among them — demonstrated with four tied rows where "keep the newest two" kept the two the timeline treats as oldest. Ordered to match, falling back to the timestamp when the source predates migration 008. Fixture gained the column, and the regression test fails against the old ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completing the fixture audit: `orchestration_command_receipts` is one of the three tables the seed clears, but the fixture never created it, so that delete only ever ran its table-missing skip path. Added the table (migration 002) with a receipt row, and extended the event-log test to assert receipts go too — a receipt left behind records a command whose events the seed deleted, so re-issuing that command would be treated as already applied and produce nothing. Confirmed the assertion fails when the delete is removed. The audit is now complete: every table the seed writes or clears exists in the fixture, and every column it keys on is present. The remaining schema gaps are columns copied generically by column intersection, where no seeder behavior depends on them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cb1da9c. Configure here.
| orderBy: hasColumn(source, "projection_thread_activities", "sequence") | ||
| ? `"created_at" DESC, "sequence"` | ||
| : `"created_at"`, | ||
| limit: options.activityLimit, |
There was a problem hiding this comment.
Activity cap wrong sort keys
Medium Severity
The activity cap for projection_thread_activities sorts by created_at then sequence, but the app timeline ranks activities by sequence then created_at. This mismatch can cause seeded threads to display older activities and drop newer, high-sequence ones, making the timeline inconsistent with the source.
Reviewed by Cursor Bugbot for commit cb1da9c. Configure here.
| return yield* new DevSeedTargetError({ | ||
| reason: "server-running", | ||
| detail: String(runningPid.value), | ||
| }); |
There was a problem hiding this comment.
Seed guard scans wrong directory
Low Severity
dev:seed resolves a specific target state directory for state.sqlite, but findRunningServerPid treats a live server in either dev or userdata under the same base as blocking. A process using only the other database can incorrectly refuse the seed.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit cb1da9c. Configure here.
1 participant
What
dev:pairto find a live dev server’s recorded state and issue a fresh pairing URL with the correct web origin.dev:seedto copy recent projection data into an isolated dev database while refusing to write to the shared home.Why
Isolated worktree databases are intentionally empty, and replacing a consumed pairing URL previously required reconstructing internal ports, state paths, and origins by hand. These are useful developer conveniences, but they are not required for the hosting fix itself.
Impact
This is PR 3 of 3 and is stacked on #4556. Keeping these optional utilities last lets the core isolation and sharing changes land without the larger data-copy implementation.
Verification
vp test run scripts/lib/dev-seed.test.ts apps/server/src/cli/auth.test.ts apps/server/src/serverRuntimeState.test.ts(24 tests)Note
Add
dev:pairanddev:seedCLI utilities for dev environment setupbun run dev:pair(t3 auth pairing url) to print a ready-to-open pairing URL for a running dev server, auto-detecting the live server state fromdev/oruserdata/runtime files.bun run dev:seed(scripts/dev-seed.ts) to copy recent threads and projects from a source T3 data directory into a target dev database, with configurable thread/activity limits, schema-drift tolerance, and attachment file copying.isPersistedServerRuntimeStateLiveto distinguish live from stale server runtime state files, and extends persisted state to include an optionaldevUrlfield used for dev server preference.seedDevDatabaseis destructive — it clears all projection tables in the target before copying; a concurrent server check just before commit reduces but does not eliminate the race window.Macroscope summarized cb1da9c.
Note
Medium Risk
dev:seeddestructively replaces target projections and clears orchestration history; guards reduce but do not eliminate races if a server starts during the copy. Pairing URL discovery touches auth credential issuance against the correct dev database.Overview
Adds
bun run dev:pair(auth pairing url) so a running dev server can mint a fresh pairing link fromserver-runtime.jsonwithout manually matching port,devvsuserdata, or web origin. The command probes both state dirs, ignores stale PIDs, prefers a live instance withdevUrl, defaults to the worktree.t3when run from a worktree, and issues credentials against the same state directory the server actually uses.Runtime state now optionally records
devUrl, requires a positive PID, and addsisPersistedServerRuntimeStateLiveso tooling does not trust crash-leftover files.deriveServerPathsaccepts an explicitstateDirfor callers that already located the live server.Adds
bun run dev:seedto copy recent projection data from~/.t3(or--from) into an isolated worktree database: caps threads/activities, tolerates schema drift, copies attachment files, clears the target event log and resets projector cursors, and normalizes agent-dependent state (stopped sessions, interrupted turns, no streaming flags). It refuses the shared home, same source/target, a missing DB, or seeding while a server holds the target open (with a pre-commit re-check).Docs and agent skills now describe these workflows;
projection-tables.tsis shared with mobile showcase seeding. Covered by new tests for auth CLI discovery, runtime liveness, server probes, and the seeder.Reviewed by Cursor Bugbot for commit cb1da9c. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Documentation