[codex] Guard dev session schemas against silent drift#431
Merged
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/package.json">
<violation number="1" location="app/package.json:52">
P2: The new `db:schema:*` scripts use single-quoted `--compiler-options` JSON, which is not cross-shell safe and can fail on Windows npm runs. Use escaped double quotes so ts-node receives valid JSON on all platforms.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| "visual:qa": "npm run visual:capture && npm run visual:diff", | ||
| "qa:review": "open tests/visual/review.html", | ||
| "qa": "npm run lint && npm run typecheck && npm run test", | ||
| "db:schema:check": "ts-node --project tsconfig.json --compiler-options '{\"module\":\"CommonJS\"}' scripts/session-schema-manifest.ts --check && vitest run tests/unit/sessions/SessionDb.schemaIdentity.test.ts", |
There was a problem hiding this comment.
P2: The new db:schema:* scripts use single-quoted --compiler-options JSON, which is not cross-shell safe and can fail on Windows npm runs. Use escaped double quotes so ts-node receives valid JSON on all platforms.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/package.json, line 52:
<comment>The new `db:schema:*` scripts use single-quoted `--compiler-options` JSON, which is not cross-shell safe and can fail on Windows npm runs. Use escaped double quotes so ts-node receives valid JSON on all platforms.</comment>
<file context>
@@ -49,6 +49,8 @@
"visual:qa": "npm run visual:capture && npm run visual:diff",
"qa:review": "open tests/visual/review.html",
"qa": "npm run lint && npm run typecheck && npm run test",
+ "db:schema:check": "ts-node --project tsconfig.json --compiler-options '{\"module\":\"CommonJS\"}' scripts/session-schema-manifest.ts --check && vitest run tests/unit/sessions/SessionDb.schemaIdentity.test.ts",
+ "db:schema:update": "ts-node --project tsconfig.json --compiler-options '{\"module\":\"CommonJS\"}' scripts/session-schema-manifest.ts --write",
"start:reset-onboarding": "node scripts/reset-onboarding.mjs",
</file context>
Cheggin
force-pushed
the
codex/session-schema-identity-guard
branch
from
394af67 to
567d721
Compare
Collaborator
Author
|
@cubic review |
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/scripts/dev-profile.mjs">
<violation number="1" location="app/scripts/dev-profile.mjs:379">
P1: Guard against copying a profile onto itself. With `--force`, the current logic can delete the source path when source and target resolve to the same location.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| ensureAppStopped(productName, opts); | ||
| const source = resolveProfileRef(opts.from ?? 'default', productName); | ||
| const target = resolveProfileRef(opts.to ?? opts.name ?? currentBranchName(), productName); | ||
| copyProfile(source, target, opts); |
There was a problem hiding this comment.
P1: Guard against copying a profile onto itself. With --force, the current logic can delete the source path when source and target resolve to the same location.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/scripts/dev-profile.mjs, line 379:
<comment>Guard against copying a profile onto itself. With `--force`, the current logic can delete the source path when source and target resolve to the same location.</comment>
<file context>
@@ -0,0 +1,419 @@
+ ensureAppStopped(productName, opts);
+ const source = resolveProfileRef(opts.from ?? 'default', productName);
+ const target = resolveProfileRef(opts.to ?? opts.name ?? currentBranchName(), productName);
+ copyProfile(source, target, opts);
+ const result = {
+ message: `copied ${opts.dbOnly ? 'session DB' : 'profile'} from ${source} to ${target}`,
</file context>
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/sessions/schemaIdentity.ts">
<violation number="1" location="app/src/main/sessions/schemaIdentity.ts:35">
P2: Global whitespace collapsing can hide real schema differences by altering quoted SQL literals before hashing.</violation>
</file>
<file name="app/scripts/dev-profile.mjs">
<violation number="1" location="app/scripts/dev-profile.mjs:121">
P1: Refs containing `/` are misclassified as filesystem paths, so common branch-style names are resolved to unintended directories.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| } | ||
|
|
||
| function looksLikePath(ref) { | ||
| return ref.startsWith('.') || ref.startsWith('~') || isAbsolute(ref) || ref.includes('/') || ref.includes('\\'); |
There was a problem hiding this comment.
P1: Refs containing / are misclassified as filesystem paths, so common branch-style names are resolved to unintended directories.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At app/scripts/dev-profile.mjs, line 121:
<comment>Refs containing `/` are misclassified as filesystem paths, so common branch-style names are resolved to unintended directories.</comment>
<file context>
@@ -0,0 +1,419 @@
+}
+
+function looksLikePath(ref) {
+ return ref.startsWith('.') || ref.startsWith('~') || isAbsolute(ref) || ref.includes('/') || ref.includes('\\');
+}
+
</file context>
Suggested change
| return ref.startsWith('.') || ref.startsWith('~') || isAbsolute(ref) || ref.includes('/') || ref.includes('\\'); | |
| return ref.startsWith('.') || ref.startsWith('~') || isAbsolute(ref); |
Worktree-local copies of sessions.db can be copied between branches, so DB_SCHEMA_VERSION alone does not prove the actual SQLite schema shape still matches the checkout. This adds a deterministic schema identity derived from sqlite_schema, stores the expected identity in a repo manifest, and gives developers task commands to copy a stopped sessions.db into the active branch profile and diagnose schema mismatches. Constraint: Existing production databases should keep opening through the current SessionDb migration path without a new metadata table requirement. Constraint: task agent:run must use the same userData root as the running app because it reads local-task-server.json. Rejected: Adopt Drizzle migration tooling | new dependency and ORM/tooling migration is broader than the current hand-written SessionDb migration surface needs. Rejected: Store schema ID in production databases | requires a metadata migration and can change runtime behavior for existing installs. Rejected: Copy whole runtime profiles by default | local-task-server.json, logs, harness, and crash data are volatile per-run state. Confidence: high Scope-risk: moderate Directive: Keep worktree profile commands aligned with AGB_USER_DATA_DIR and do not copy SQLite files while the app is running. Tested: yarn db:schema:check; task db:schema:check; task db:schema:update; task worktree:profile:path; slash-containing FROM branch profile copy against a throwaway DB; task db:worktree:doctor against a throwaway DB; task worktree:profile:clean against a throwaway profile; task typecheck; task lint; git diff --check Not-tested: Full npm run test remains blocked by existing BrowserPool zoom-fit assertion failures. Co-authored-by: OmX <omx@oh-my-codex.dev>
Cheggin
force-pushed
the
codex/session-schema-identity-guard
branch
from
567d721 to
002e37c
Compare
Collaborator
Author
|
@cubic review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sqlite_schemasrc/main/sessions/schema-manifest.jsondb:schema:check/db:schema:updatecommands and a CI check before coveragetask worktree:profile:pathtask worktree:uptask db:worktree:copy FROM=defaulttask db:worktree:doctortask worktree:profile:clean FORCE=1AGENTS.mdplus the app-spawned harnessAGENTS.mdWhy
Worktree-local
sessions.dbcopies can be moved between branches.DB_SCHEMA_VERSION/user_versioncatches newer-vs-older DBs, but it does not prove two branches with the same version have the same actual schema shape.This PR gives developers a direct workflow to copy a stopped
sessions.dbinto the active branch profile and immediately diagnose schema/version drift. It also keepstask agent:runaligned with the sameAGB_USER_DATA_DIR, which is required because the task runner reads<userData>/local-task-server.json.Notes
sessions.dbplus WAL/SHM companions, not volatile runtime files likelocal-task-server.json, logs, harness output, or crash data.Validation
yarn db:schema:checktask db:schema:checktask db:schema:updatetask worktree:profile:pathtask db:worktree:copyagainst a throwaway DBtask db:worktree:doctoragainst a throwaway DBtask worktree:profile:cleanagainst a throwaway profiletask typechecktask lint(0 errors; existing warnings remain)git diff --checkKnown baseline
npm run testis not clean in this checkout becausetests/unit/sessions/BrowserPool.test.tshas two existing zoom-fit assertion failures unrelated to this DB/schema change.