Skip to content

fix(test): resolve the store version from the upstream lane in persistedVersion - #382

Merged
ualtinok merged 1 commit into
cortexkit:masterfrom
iceteaSA:fix/persisted-version-fork-floor
Aug 28, 2026
Merged

ualtinok merged 1 commit into
cortexkit:masterfrom
iceteaSA:fix/persisted-version-fork-floor

Conversation

@iceteaSA

@iceteaSA iceteaSA commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

What

persistedVersion in storage-db-initialize-order.test.ts reads the store's schema version with a bare MAX(version). Production doesn't — getPersistedSchemaVersion bounds the same read by the reserved fork lane:

// storage-db.ts:269-274
const row = db
    .prepare("SELECT COALESCE(MAX(version), 0) AS version FROM schema_migrations WHERE version < ?")
    .get(FORK_MIGRATION_VERSION_FLOOR) as { version: number } | undefined;

So for a downstream fork recording its migrations in the documented >= 10_000 lane, production resolves the version correctly while this test helper reads the fork's row as the store version:

opens the repaired legacy store through the shared OpenCode and Pi boot paths
  Expected: 82
  Received: 10100

The test is correct in what it asserts — it's the helper that doesn't match the production rule it's standing in for.

Change

Mirror the production bound in the helper, plus a regression that seeds a fork-lane row and asserts the resolved version is still LATEST_MIGRATION_VERSION.

Red-check: reverting the helper bound while keeping the new test gives Expected: 82 / Received: 10100 — the same symptom, now caught directly rather than as a side effect of an unrelated boot-path assertion.

with the fix              5 pass / 0 fail
helper reverted, test kept 4 pass / 1 fail

Scope

One test file, +35/−2. No production code touched.

tsc --noEmit clean. Lint reports the same 6 pre-existing errors as clean master (verified by stashing), none in this file.

Why it matters

This is the only place I found where the reserved-lane convention from #280 isn't honoured. Production code, the schema fence, and the migration runner all respect the floor; this helper was the one straggler. Without it, any fork using the lane as documented sees a red suite on an upstream test that has nothing to do with its changes — which is exactly the friction the reserved lane was introduced to remove.

Found while rebasing a fork onto master after #376/#378 merged: the failure looked like one of the known full-suite concurrency artifacts, but it reproduced in isolation (2.6s, single file) and passed on clean master, which is what pointed at the helper rather than the environment.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the persistedVersion test helper so it resolves the store version from the upstream migration lane, matching production's getPersistedSchemaVersion. Previously the helper used a bare MAX(version) query, so a downstream fork recording migrations in the reserved 10_000+ lane was treated as the store version and failed the shared boot assertion.

  • Adds a regression test that seeds a fork-lane row and asserts the resolved version is still LATEST_MIGRATION_VERSION.

Written for commit 2580291. Summary will update on new commits.

Review in cubic

Greptile Summary

This PR aligns the test-only persistedVersion helper with production by excluding migration rows in the reserved downstream-fork lane.

  • Imports and applies FORK_MIGRATION_VERSION_FLOOR to the helper query.
  • Adds regression coverage proving a fork-lane row does not change the resolved upstream schema version.

Confidence Score: 5/5

The PR appears safe to merge, with no actionable defects identified in the test-only change.

The updated query uses the same constant and strict boundary as production, while the regression fixture remains isolated and is cleaned up through the existing database lifecycle.

Important Files Changed

Filename Overview
packages/plugin/src/features/magic-context/storage-db-initialize-order.test.ts The helper now mirrors production’s bounded schema-version query, and the added isolated regression test correctly covers the downstream fork-lane case.

Reviews (2): Last reviewed commit: "fix(test): resolve the store version fro..." | Re-trigger Greptile

…tedVersion

`getPersistedSchemaVersion` (storage-db.ts:269-274) already bounds its read with
`WHERE version < FORK_MIGRATION_VERSION_FLOOR`, so a downstream fork recording its
own migrations in the reserved >= 10_000 lane still reports this binary's upstream
schema version. The `persistedVersion` helper in this test did a bare MAX(version),
so it read a fork's 10_000+ row as the store version and failed the shared
OpenCode/Pi boot assertion for any fork using the documented lane.

Mirror the production bound in the helper and add a regression that seeds a
fork-lane row and asserts the resolved version is still LATEST_MIGRATION_VERSION.
Reverting the helper bound makes it fail with Expected 82 / Received 10100.
@iceteaSA
iceteaSA force-pushed the fix/persisted-version-fork-floor branch from 8a6c0dd to 2580291 Compare August 28, 2026 22:03
@iceteaSA

Copy link
Copy Markdown
Contributor Author

Pushed one correction, caught by running this against a real fork rather than only against clean master.

The regression test seeded its fork-lane row with a plain INSERT at FORK_MIGRATION_VERSION_FLOOR + 100. That passes on master (no fork rows) but fails on an actual fork whose own migration already occupies that version:

SQLiteError: UNIQUE constraint failed: schema_migrations.version

Which is a poor property for a test whose entire purpose is to protect forks using the lane. Changed to INSERT OR REPLACE so the fixture is idempotent regardless of what the store already contains.

Verified both directions now:

clean master + fix      5 pass / 0 fail
fork (rows at 10_000-10_002, 10_100) + fix   5 pass / 0 fail
helper bound reverted   4 pass / 1 fail — Expected: 82, Received: 10100

@magic-alfonso

magic-alfonso Bot commented Aug 28, 2026

Copy link
Copy Markdown

Verified and merging. The helper now mirrors getPersistedSchemaVersion's lane-bounded read exactly, and the new regression is the right shape — it pins that a fork row in the reserved lane can never masquerade as the store version, with the OR REPLACE touch keeping the fixture idempotent on a real fork. Ran the file on the PR head: 5/5 green. This closes the last unbounded MAX(version) read we know of outside production code — thank you for sweeping the test surface too, and for the steady stream of precise reports this week. The #381 adjudication is in progress and will get a full per-item reply.

@ualtinok
ualtinok merged commit fa01870 into cortexkit:master Aug 28, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants