fix: move-codex-session should handle Codex 0.160.1 and large native homes #132

jercik · 2026-10-09T19:33:48Z

jercik commented

2026-10-09 19:33:48 +00:00

The move-codex-session script refused every move on Codex 0.160.1 and could not run against a large native home. It now moves the per-thread tables 0.160.1 added (attachments, realtime items, queue revisions, stage-1 memory jobs), streams its unmoved-row check so a 1.6 GB history database works, and tolerates the legacy rollouts Codex records as skipped.

It also fixes four ways a move could end half-done: a concurrent destination write misread as a trigger side effect, a masked disk-full error, history stranded around reverts and forks, and a destination change between copy and source deletion.

Moving a 32-thread tree out of a clone of a real native home and back left every row and rollout intact, apart from regenerated log row IDs.

The `move-codex-session` script refused every move on Codex 0.160.1 and could not run against a large native home. It now moves the per-thread tables 0.160.1 added (attachments, realtime items, queue revisions, stage-1 memory jobs), streams its unmoved-row check so a 1.6 GB history database works, and tolerates the legacy rollouts Codex records as skipped. It also fixes four ways a move could end half-done: a concurrent destination write misread as a trigger side effect, a masked disk-full error, history stranded around reverts and forks, and a destination change between copy and source deletion. Moving a 32-thread tree out of a clone of a real native home and back left every row and rollout intact, apart from regenerated log row IDs.

jercik added 1 commit

2026-10-09 19:33:48 +00:00

fix: move-codex-session should handle Codex 0.160.1 and large native homes

commit-msg / commitlint (pull_request) Successful in 22s

Details

Node tests / node:test (pull_request) Successful in 3m41s

Details

Review / Review (pull_request_target) Successful in 5m7s

Details

170aee6a7a

Move the per-thread tables Codex 0.160.1 added, stream the unmoved-row
check, tolerate legacy rollouts Codex recorded as skipped, and close
failure paths that could leave a move half-done.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

forgejo-actions commented

2026-10-09 19:38:54 +00:00

Review 01M4H2GP9Y7V9GND1K25M4P77M — head 170aee6a7ac2affb4b5cef2699a6192ba8abac65

Review — j4k-oss/agent-skills @ `40154724b6`

Scope: diff against base tree 21b532de61b1
Status: dispatched — coverage complete (5/5 slots terminal)
Facts: current review-wide projection

Computed under:

{
  "abandonment": "abandonment-v1",
  "anchor_recipe": 1,
  "batch_policy": "batch-v1",
  "coverage": "coverage-v3",
  "dispatch_policy": "dispatch-v2",
  "grounder_version": 1,
  "grounding_read_rule": "grounding-read-v1",
  "promotion_policy": "promotion-v1",
  "report": "report-v4",
  "tally": "tally-v1",
  "triage_settle": "triage-settle-v2"
}

Findings (6)

critical — Rollback deletes destination history even when the history copy failed

claim: 01M4H2MB9PP48NTKF5J0NPDZ7V
anchor: skills/move-codex-session/scripts/move-codex-session.ts (snippet)

        stateCopied ? threadIds : [],
        historyIds,
        stateCopied ? newSectionIds : [],

lens: general-bug · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M4H2QF754GRV534VQE42HTMF · valid: The exact-grounded cleanup arguments gate state IDs but pass history IDs unconditionally. The reviewer's concrete trace explains that a history INSERT conflict rolls back the copy before historyCopied becomes true, while stateCopied still enables cleanup and ownership checking is skipped for history. Its reported probe shows the independently inserted conflicting row survives the failed copy and is then deleted by cleanup. The related process-boundary claim still permits ancestor-process activity, so its narrower allowance does not refute this interleaving. Gate history deletion on historyCopied. This supports high severity for conditional data loss; critical overstates the demonstrated scope and reach. The probe is reviewer-reported, not independently reproduced.
disposition: none

A failed move can delete history written by the permitted destination process. If state copying succeeds, then a destination writer inserts a conflicting history row before history copying, the history INSERT fails and rolls back. Cleanup nevertheless deletes that writer's row, even though this move never committed any history rows.

main sets historyCopied only after copyHistory returns. cleanupDestination checks history ownership only when historyCopied is true, but passes all historyIds to deleteDestinationDatabases whenever either stateCopied or historyCopied is true. That function deletes matching rows from all four history tables. Gate the history IDs passed to deletion on historyCopied, just as the state IDs are gated on stateCopied.

I traced main, copyHistory, cleanupDestination, and deleteDestinationDatabases and ran node /app/probe-rollback.ts against unmodified function bodies extracted from the subject, using createFixture from the subject tests. After copyState, the probe inserted a conflicting destination thread_turns row. copyHistory reported UNIQUE constraint failed: thread_turns.thread_id, thread_turns.turn_id; the row remained before cleanup and the query returned an empty array after cleanup. The SKILL.md and assertHomesAvailable explicitly permit destination activity from the invoking process. Full CLI execution was unavailable because lsof is missing.

Passing an empty history-ID list when historyCopied is false should preserve the conflicting row while removing copied state. A CLI reproduction with a destination writer committing between preflight and copyHistory would establish the complete timing path; the direct probe establishes the destructive cleanup mechanism.

high — SKILL.md's list of refusals omits the new project-membership refusal and other refusals the script enforces

claim: 01M4H2KA4PZDF580K9XHQD4C3T
anchor: skills/move-codex-session/SKILL.md (snippet)

and refuses incompatible schemas, destination conflicts, child-only moves, an active source home, or a destination home active outside the invoking process tree

lens: restated-sets · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M4H2QF754GRV534VQE42HTMF · valid: The exact-grounded passage enumerates the script's refusals. The reviewer names the defining script and compares its conditions with the copy, including the project-membership refusal and three other omitted conditions. This satisfies the required set comparison and establishes an already inaccurate inventory, warranting high severity. Reported error messages supply the cause when a move fails, so a full refusal inventory adds no distinct operational value. Preserve the actionable source-idleness and destination-activity prerequisites, using the precise ancestry boundary established by the related claim; replace the remaining enumeration with a purpose statement and a precise script pointer. The proposed rewrite's possible loss of those prerequisites does not negate the established inventory defect.
disposition: none

The sentence presents five refusal conditions as what the script refuses, and the script now refuses more than that. A reader who trusts the list will not expect a move of a project-bound thread to fail, or will believe that a failure outside the five is an unexpected malfunction rather than a designed refusal.

The defining source is skills/move-codex-session/scripts/move-codex-session.ts. The copy lists: incompatible schemas, destination conflicts, child-only moves, an active source home, and a destination home active outside the invoking process tree. The script's fail(...) calls cover those five: schema checks (schema mismatch for table, unsupported thread table), destination conflicts (destination state already contains thread, destination index already contains thread), child-only (is a child of ... move the top-level session instead), source Codex home is active in process, and destination Codex home is active in unrelated process. It also refuses cases the copy does not name. The diff adds requireThreadsOutsideProjects, which fails with thread ${id} belongs to project ${thread.project_id}; project rows are not moved, and the accompanying test "refuses to move a thread that belongs to a project". It also refuses a rollout without session_meta that Codex did not record in rollout_migration_skipped_rollouts, a rollout path outside its Codex home, and a rollout checksum mismatch. So the copy already omits members of its source; at least the project refusal arrived in the same change that edited this sentence.

No exception applies. The sentence is orientation for an agent that runs the script and then reads the reported error; the closing clause already tells the reader to resolve the reported blocker, and the script's message names the specific cause. Exact members are not needed to run the command, and a list that is incomplete misleads more than it helps.

Correction: replace the enumeration with the outcome and let the error message carry the member, for example "...and refuses a move it cannot complete without losing or overwriting data, naming the blocker in its error; resolve the reported blocker instead of editing the databases by hand." If an enumerated condition is needed, keep only the one the invoker can act on before running (the destination home must be idle apart from the invoking process tree) and point to the script's error output for the rest.

Examined: SKILL.md in full, every fail( site in move-codex-session.ts, requireThreadsOutsideProjects and its call site, and the test names added in the diff. Not run: the script itself. The decisive evidence is the project-membership fail(...) at the requireThreadsOutsideProjects function, which the sentence does not cover.

high — Source deletion guards state and history but can discard the only remaining auxiliary rows

claim: 01M4H2P36D6VPE1K504TAQ5E05
anchor: skills/move-codex-session/scripts/move-codex-session.ts (snippet)

        destinationState.exec("BEGIN IMMEDIATE");
        destinationHistory.exec("BEGIN IMMEDIATE");
        const currentDigests: Array<[string, Record<string, string>, Record<string, string>]> = [
          ["state", comparedStateDigests, movedRowDigests(destinationState, STATE_TABLES, threadIds, removableSectionIds)],
          ["history", comparedHistoryDigests, movedRowDigests(destinationHistory, HISTORY_TABLES, historyIds.copy, [])],
        ];

lens: general-bug · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M4H2QF754GRV534VQE42HTMF · valid: The exact-grounded commit guard locks and compares destination state and history. The reviewer's trace establishes that auxiliary destination copies are verified earlier, before an await, but their source rows are subsequently deleted without retaining equivalent destination locks or rechecking their transferred contents. The reported probe deletes a copied goal before source deletion and finds the goal absent from both homes after a successful return. Auxiliary snapshots protecting unrelated source rows do not protect that destination recovery copy. Permitted ancestor-process activity leaves the interleaving possible; its unknown frequency does not refute the mechanism. Extend the commit guard to transferred auxiliary contents with appropriate key normalization and locks held through source commit. High severity is supported.
disposition: none

The move can commit source deletion after destination goal, memory, queue, or log rows disappear or change. A permitted destination writer can modify these rows after main's last comparison; the source then loses its original rows without checking that the destination still holds the recovery copy. This leaves an incomplete moved session despite the successful source-deletion checks.

main verifies each thread database before awaiting verifyRollouts. deleteSourceDatabases subsequently deletes the source rows from every thread database, but compares and write-locks only destination state and history before committing. Its snapshots of auxiliary databases check unrelated source rows, not the destination copy. Extend the commit precondition to the auxiliary destination databases, comparing transferred contents with the appropriate generated-key normalization and retaining their write locks through the source commit.

I traced main, verifyDatabaseContents, copyThreadDatabase, and deleteSourceDatabases and examined the tests that preserve the source after destination state or history deletion. I ran node /app/probe-source-delete.ts using unmodified subject function bodies and the subject's createFixture. The probe copied and verified every auxiliary database, deleted the root's destination thread_goals row, and invoked deleteSourceDatabases. It returned successfully; both source and destination queries for that goal returned empty arrays, and the source retained only the unrelated thread. Full CLI execution was unavailable because lsof is missing.

A CLI test committing a destination goal deletion after the final comparison should leave the source intact or reject the move. The probe establishes that the source-deletion function currently accepts this interleaving; the actual frequency of such destination edits in Codex is not established by this repository.

medium — “Invoking process tree” overstates which active destination processes are allowed

claim: 01M4H2NJXVF1HPQ2QPTEEQYXWX
anchor: skills/move-codex-session/SKILL.md (snippet)

a destination home active outside the invoking process tree

lens: writing-quality · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M4H2QF754GRV534VQE42HTMF · valid: The exact-grounded phrase describes an invoking process tree, whereas the reported implementation walks only the script PID and its ancestors, with a separate allowance for its lock helpers. The reviewer supplies a concrete test comparison: a sibling destination holder is rejected while the invoking parent holding a descriptor is permitted. That establishes a plausible preparation mistake under the broader wording, rather than a merely missing definition. Describe permitted user-controlled activity as the script and its ancestors, retaining the active-source restriction; internal lock-helper allowances need not become another inventory for the reader. No supplied requirement establishes that the broader process-tree policy was an intentional decision overriding this implementation.
disposition: none

An agent preparing the move can leave a destination-writing child or sibling of the invoking Codex running because it belongs to the same process tree, yet the script rejects that process as unrelated. The same phrase appears in the warning about destination database contention, making the allowed activity boundary ambiguous.

In scripts/move-codex-session.ts, ancestorProcessIds starts with process.pid and follows process.ppid upward using ps; it never walks descendants or siblings. assertHomesAvailable permits only those PIDs and the script's lock-helper PIDs. The test “moves a complete session tree and fails closed on unsafe state” starts a destination file-holder child of the test process and expects an “active in unrelated process” failure, whereas a file descriptor held by the test process itself is allowed when it invokes the move. Thus a process in the invoker's wider tree is outside the allowed ancestry chain.

Use “the script or its ancestor processes” for the contention warning and destination activity boundary. Keep the active-source restriction and the instruction to resolve reported blockers. The writing standard's Use Precise Language guidance calls for a verifiable boundary; this detail serves the agent's concrete task of deciding which destination processes may remain active, so replacing it with a source pointer would lose necessary operational meaning.

I traced ancestorProcessIds and assertHomesAvailable, their calls in main, and the corresponding test setup and assertions. This is a static comparison; I did not run the test. A descendant traversal or another PID allowance on that call path would refute the claim, but neither appears in the reviewed implementation.

medium — The rollback test name claims completeness over the script’s database set

claim: 01M4H2PTSH7WV19VM9FTPN47GF
anchor: skills/move-codex-session/scripts/move-codex-session.test.ts (snippet)

test("rolls back every destination database when a step after the queue copy fails", () => {

lens: writing-quality · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M4H2QF754GRV534VQE42HTMF · valid: The exact-grounded test name promises rollback of every destination database. The reviewer names the defining script and lists state, thread_history, logs, goals, memories, and queue, explaining that the manually asserted copy currently matches with no differing member. The rubric expressly excludes completeness claims about a production-defined set from the test-name exception, even when the current body checks each member. This is therefore a medium maintenance-copy defect, not a demand for more test coverage or a naming preference. The proposed failure-trigger and rollback-outcome name preserves useful test-output meaning while leaving all regression assertions intact.
disposition: none

Adding a destination database to the mover creates another place to update: this test name continues promising coverage of every destination database even if its assertions still cover only the old selection. The wording makes test output a separately maintained completeness claim.

The authority is main in scripts/move-codex-session.ts: it selects state and thread_history directly, then maps THREAD_DATABASE_SPECS, whose members are logs, goals, memories, and queue. The source set is therefore state, thread_history, logs, goals, memories, and queue. The name's “every destination database” claims those same members, and the body manually queries each of them. No member differs at this revision; the defect is the independent completeness promise, not a currently missing assertion.

Rename the test to “rolls back copied rows when index publication fails”. This keeps the test-output reader's useful task: identifying the failure trigger and rollback outcome. It removes only the global inventory claim. Leave the setup and assertions intact. The writing standard's One Idea, One Place guidance favors an outcome statement, and the restated-sets guideline explicitly excludes test names that claim completeness about a set defined by the code under test from the test-name exception.

I read this test's symlink setup and database assertions, THREAD_DATABASE_SPECS, and the database selection and copy loop in main. No runtime reproduction is needed for the wording comparison, and none was run. A database selection derived by this test from the authority, together with a test name derived from that same selection, would establish that there is no independent copy; this name and its assertions are literal text.

low — The precision-loss test decrements twice and misses lossy snapshot encoding

claim: 01M4H2NZSQ64ECWNP23YVXHTDS
anchor: skills/move-codex-session/scripts/move-codex-session.test.ts (snippet)

        CREATE TRIGGER nudge_timestamp
        AFTER INSERT ON threads
        BEGIN
          UPDATE rollout_migration_skipped_rollouts SET rollout_modified_at_ns = rollout_modified_at_ns - 1;
        END;

lens: test-trimming · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M4H2QF754GRV534VQE42HTMF · valid: The exact-grounded AFTER INSERT trigger has no single-thread condition. The reported fixture and copy trace insert two threads, decrementing 9007199254740993 twice to 9007199254740991. These endpoints remain distinguishable after Number conversion, so the stated lossy BigInt encoding regression still triggers refusal and leaves the test green. The reported mutation probe corroborates that mechanism; its lsof shim does not affect this arithmetic comparison. Restricting the trigger to one inserted thread makes both endpoints round to 9007199254740992, restoring the promised precision check while retaining refusal and rollback assertions. This is a concrete ineffective-test defect under the supplied test guidelines.
disposition: none

The test promises to detect an unrelated integer change that JavaScript doubles cannot distinguish, but it still passes when snapshot encoding rounds every BigInt through Number. This leaves false confidence that the integer-precision regression is protected.

createFixture inserts both ROOT_ID and CHILD_ID into source threads, and copyState inserts both into destination threads. The anchored AFTER INSERT trigger therefore subtracts one twice: 9007199254740993 becomes 9007199254740991. Number rounds the initial value to 9007199254740992, while the final value is exactly representable, so even a lossy snapshot detects this change. The assertions assert.equal(refused.status, 1) and assert.equal(refused.stderr, "state triggers changed unrelated destination data\n") do not distinguish that implementation from the precise one.

Restrict the trigger to one inserted thread, for example with WHEN NEW.id = '${ROOT_ID}' inside the interpolated SQL. The final integer then becomes 9007199254740992, which rounds to the same Number as the initial value. Keep the refusal and rollback assertions; the repaired input restores the intended precision check.

I traced createFixture, runMove, copyState, snapshotUnmovedRows and encodeSqliteValue in the test and implementation files, and inspected the adjacent "keeps integers above 2**53 in unmoved rows exact" test. In isolated copies, I changed encodeSqliteValue from ["integer", value.toString()] to ["integer", Number(value).toString()]. Running node --test --test-name-pattern='integers above|doubles cannot' passed both existing tests with both implementations. After adding the single-thread trigger condition, the precision test passed the original implementation and failed the mutant: the CLI exited zero instead of the expected one.

The sandbox lacks lsof, so these targeted probes used a PATH shim returning exit one (no open processes); they exercised the real SQLite transfer and snapshot code but did not validate process detection. The unmodified full-suite attempt failed at that missing dependency. A run with real lsof would confirm the same mutation result without the shim.

Other claims

grounding-pending (0)
ungrounded (0)
rejected (0)
duplicate-of (0)
unadjudicated (0)

Coverage

Coverage pass: 01M4H2H5DR4BZ93RFR6BM1D6ND
Accounting: complete
Slot health: healthy

lens	part	arm	unit status	runs	loss
general-bug	whole	default	claims-emitted	1	no
writing-quality	whole	default	claims-emitted	1	no
test-trimming	whole	default	claims-emitted	1	no
restated-sets	whole	default	claims-emitted	1	no
project-docs	whole	default	no-claims	1	no

**Review** `01M4H2GP9Y7V9GND1K25M4P77M` — head `170aee6a7ac2affb4b5cef2699a6192ba8abac65` # Review — j4k-oss/agent-skills @ 40154724b696 Scope: diff against base tree `21b532de61b1` Status: dispatched — coverage complete (5/5 slots terminal) Facts: current review-wide projection Computed under: ```json { "abandonment": "abandonment-v1", "anchor_recipe": 1, "batch_policy": "batch-v1", "coverage": "coverage-v3", "dispatch_policy": "dispatch-v2", "grounder_version": 1, "grounding_read_rule": "grounding-read-v1", "promotion_policy": "promotion-v1", "report": "report-v4", "tally": "tally-v1", "triage_settle": "triage-settle-v2" } ``` ## Findings (6) ### critical — Rollback deletes destination history even when the history copy failed - claim: `01M4H2MB9PP48NTKF5J0NPDZ7V` - anchor: `skills/move-codex-session/scripts/move-codex-session.ts` (snippet) ``` stateCopied ? threadIds : [], historyIds, stateCopied ? newSectionIds : [], ``` - lens: general-bug · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4H2QF754GRV534VQE42HTMF` · valid: The exact-grounded cleanup arguments gate state IDs but pass history IDs unconditionally. The reviewer's concrete trace explains that a history INSERT conflict rolls back the copy before historyCopied becomes true, while stateCopied still enables cleanup and ownership checking is skipped for history. Its reported probe shows the independently inserted conflicting row survives the failed copy and is then deleted by cleanup. The related process-boundary claim still permits ancestor-process activity, so its narrower allowance does not refute this interleaving. Gate history deletion on historyCopied. This supports high severity for conditional data loss; critical overstates the demonstrated scope and reach. The probe is reviewer-reported, not independently reproduced. - disposition: none > A failed move can delete history written by the permitted destination process. If state copying succeeds, then a destination writer inserts a conflicting history row before history copying, the history INSERT fails and rolls back. Cleanup nevertheless deletes that writer's row, even though this move never committed any history rows. > > main sets historyCopied only after copyHistory returns. cleanupDestination checks history ownership only when historyCopied is true, but passes all historyIds to deleteDestinationDatabases whenever either stateCopied or historyCopied is true. That function deletes matching rows from all four history tables. Gate the history IDs passed to deletion on historyCopied, just as the state IDs are gated on stateCopied. > > I traced main, copyHistory, cleanupDestination, and deleteDestinationDatabases and ran `node /app/probe-rollback.ts` against unmodified function bodies extracted from the subject, using createFixture from the subject tests. After copyState, the probe inserted a conflicting destination thread_turns row. copyHistory reported `UNIQUE constraint failed: thread_turns.thread_id, thread_turns.turn_id`; the row remained before cleanup and the query returned an empty array after cleanup. The SKILL.md and assertHomesAvailable explicitly permit destination activity from the invoking process. Full CLI execution was unavailable because lsof is missing. > > Passing an empty history-ID list when historyCopied is false should preserve the conflicting row while removing copied state. A CLI reproduction with a destination writer committing between preflight and copyHistory would establish the complete timing path; the direct probe establishes the destructive cleanup mechanism. ### high — SKILL.md's list of refusals omits the new project-membership refusal and other refusals the script enforces - claim: `01M4H2KA4PZDF580K9XHQD4C3T` - anchor: `skills/move-codex-session/SKILL.md` (snippet) ``` and refuses incompatible schemas, destination conflicts, child-only moves, an active source home, or a destination home active outside the invoking process tree ``` - lens: restated-sets · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4H2QF754GRV534VQE42HTMF` · valid: The exact-grounded passage enumerates the script's refusals. The reviewer names the defining script and compares its conditions with the copy, including the project-membership refusal and three other omitted conditions. This satisfies the required set comparison and establishes an already inaccurate inventory, warranting high severity. Reported error messages supply the cause when a move fails, so a full refusal inventory adds no distinct operational value. Preserve the actionable source-idleness and destination-activity prerequisites, using the precise ancestry boundary established by the related claim; replace the remaining enumeration with a purpose statement and a precise script pointer. The proposed rewrite's possible loss of those prerequisites does not negate the established inventory defect. - disposition: none > The sentence presents five refusal conditions as what the script refuses, and the script now refuses more than that. A reader who trusts the list will not expect a move of a project-bound thread to fail, or will believe that a failure outside the five is an unexpected malfunction rather than a designed refusal. > > The defining source is `skills/move-codex-session/scripts/move-codex-session.ts`. The copy lists: incompatible schemas, destination conflicts, child-only moves, an active source home, and a destination home active outside the invoking process tree. The script's `fail(...)` calls cover those five: schema checks (`schema mismatch for table`, `unsupported thread table`), destination conflicts (`destination state already contains thread`, `destination index already contains thread`), child-only (`is a child of ... move the top-level session instead`), `source Codex home is active in process`, and `destination Codex home is active in unrelated process`. It also refuses cases the copy does not name. The diff adds `requireThreadsOutsideProjects`, which fails with `thread ${id} belongs to project ${thread.project_id}; project rows are not moved`, and the accompanying test "refuses to move a thread that belongs to a project". It also refuses a rollout without `session_meta` that Codex did not record in `rollout_migration_skipped_rollouts`, a rollout path outside its Codex home, and a rollout checksum mismatch. So the copy already omits members of its source; at least the project refusal arrived in the same change that edited this sentence. > > No exception applies. The sentence is orientation for an agent that runs the script and then reads the reported error; the closing clause already tells the reader to resolve the reported blocker, and the script's message names the specific cause. Exact members are not needed to run the command, and a list that is incomplete misleads more than it helps. > > Correction: replace the enumeration with the outcome and let the error message carry the member, for example "...and refuses a move it cannot complete without losing or overwriting data, naming the blocker in its error; resolve the reported blocker instead of editing the databases by hand." If an enumerated condition is needed, keep only the one the invoker can act on before running (the destination home must be idle apart from the invoking process tree) and point to the script's error output for the rest. > > Examined: SKILL.md in full, every `fail(` site in move-codex-session.ts, `requireThreadsOutsideProjects` and its call site, and the test names added in the diff. Not run: the script itself. The decisive evidence is the project-membership `fail(...)` at the `requireThreadsOutsideProjects` function, which the sentence does not cover. ### high — Source deletion guards state and history but can discard the only remaining auxiliary rows - claim: `01M4H2P36D6VPE1K504TAQ5E05` - anchor: `skills/move-codex-session/scripts/move-codex-session.ts` (snippet) ``` destinationState.exec("BEGIN IMMEDIATE"); destinationHistory.exec("BEGIN IMMEDIATE"); const currentDigests: Array<[string, Record<string, string>, Record<string, string>]> = [ ["state", comparedStateDigests, movedRowDigests(destinationState, STATE_TABLES, threadIds, removableSectionIds)], ["history", comparedHistoryDigests, movedRowDigests(destinationHistory, HISTORY_TABLES, historyIds.copy, [])], ]; ``` - lens: general-bug · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4H2QF754GRV534VQE42HTMF` · valid: The exact-grounded commit guard locks and compares destination state and history. The reviewer's trace establishes that auxiliary destination copies are verified earlier, before an await, but their source rows are subsequently deleted without retaining equivalent destination locks or rechecking their transferred contents. The reported probe deletes a copied goal before source deletion and finds the goal absent from both homes after a successful return. Auxiliary snapshots protecting unrelated source rows do not protect that destination recovery copy. Permitted ancestor-process activity leaves the interleaving possible; its unknown frequency does not refute the mechanism. Extend the commit guard to transferred auxiliary contents with appropriate key normalization and locks held through source commit. High severity is supported. - disposition: none > The move can commit source deletion after destination goal, memory, queue, or log rows disappear or change. A permitted destination writer can modify these rows after main's last comparison; the source then loses its original rows without checking that the destination still holds the recovery copy. This leaves an incomplete moved session despite the successful source-deletion checks. > > main verifies each thread database before awaiting verifyRollouts. deleteSourceDatabases subsequently deletes the source rows from every thread database, but compares and write-locks only destination state and history before committing. Its snapshots of auxiliary databases check unrelated source rows, not the destination copy. Extend the commit precondition to the auxiliary destination databases, comparing transferred contents with the appropriate generated-key normalization and retaining their write locks through the source commit. > > I traced main, verifyDatabaseContents, copyThreadDatabase, and deleteSourceDatabases and examined the tests that preserve the source after destination state or history deletion. I ran `node /app/probe-source-delete.ts` using unmodified subject function bodies and the subject's createFixture. The probe copied and verified every auxiliary database, deleted the root's destination thread_goals row, and invoked deleteSourceDatabases. It returned successfully; both source and destination queries for that goal returned empty arrays, and the source retained only the unrelated thread. Full CLI execution was unavailable because lsof is missing. > > A CLI test committing a destination goal deletion after the final comparison should leave the source intact or reject the move. The probe establishes that the source-deletion function currently accepts this interleaving; the actual frequency of such destination edits in Codex is not established by this repository. ### medium — “Invoking process tree” overstates which active destination processes are allowed - claim: `01M4H2NJXVF1HPQ2QPTEEQYXWX` - anchor: `skills/move-codex-session/SKILL.md` (snippet) ``` a destination home active outside the invoking process tree ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4H2QF754GRV534VQE42HTMF` · valid: The exact-grounded phrase describes an invoking process tree, whereas the reported implementation walks only the script PID and its ancestors, with a separate allowance for its lock helpers. The reviewer supplies a concrete test comparison: a sibling destination holder is rejected while the invoking parent holding a descriptor is permitted. That establishes a plausible preparation mistake under the broader wording, rather than a merely missing definition. Describe permitted user-controlled activity as the script and its ancestors, retaining the active-source restriction; internal lock-helper allowances need not become another inventory for the reader. No supplied requirement establishes that the broader process-tree policy was an intentional decision overriding this implementation. - disposition: none > An agent preparing the move can leave a destination-writing child or sibling of the invoking Codex running because it belongs to the same process tree, yet the script rejects that process as unrelated. The same phrase appears in the warning about destination database contention, making the allowed activity boundary ambiguous. > > In scripts/move-codex-session.ts, ancestorProcessIds starts with process.pid and follows process.ppid upward using ps; it never walks descendants or siblings. assertHomesAvailable permits only those PIDs and the script's lock-helper PIDs. The test “moves a complete session tree and fails closed on unsafe state” starts a destination file-holder child of the test process and expects an “active in unrelated process” failure, whereas a file descriptor held by the test process itself is allowed when it invokes the move. Thus a process in the invoker's wider tree is outside the allowed ancestry chain. > > Use “the script or its ancestor processes” for the contention warning and destination activity boundary. Keep the active-source restriction and the instruction to resolve reported blockers. The writing standard's Use Precise Language guidance calls for a verifiable boundary; this detail serves the agent's concrete task of deciding which destination processes may remain active, so replacing it with a source pointer would lose necessary operational meaning. > > I traced ancestorProcessIds and assertHomesAvailable, their calls in main, and the corresponding test setup and assertions. This is a static comparison; I did not run the test. A descendant traversal or another PID allowance on that call path would refute the claim, but neither appears in the reviewed implementation. ### medium — The rollback test name claims completeness over the script’s database set - claim: `01M4H2PTSH7WV19VM9FTPN47GF` - anchor: `skills/move-codex-session/scripts/move-codex-session.test.ts` (snippet) ``` test("rolls back every destination database when a step after the queue copy fails", () => { ``` - lens: writing-quality · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4H2QF754GRV534VQE42HTMF` · valid: The exact-grounded test name promises rollback of every destination database. The reviewer names the defining script and lists state, thread_history, logs, goals, memories, and queue, explaining that the manually asserted copy currently matches with no differing member. The rubric expressly excludes completeness claims about a production-defined set from the test-name exception, even when the current body checks each member. This is therefore a medium maintenance-copy defect, not a demand for more test coverage or a naming preference. The proposed failure-trigger and rollback-outcome name preserves useful test-output meaning while leaving all regression assertions intact. - disposition: none > Adding a destination database to the mover creates another place to update: this test name continues promising coverage of every destination database even if its assertions still cover only the old selection. The wording makes test output a separately maintained completeness claim. > > The authority is main in scripts/move-codex-session.ts: it selects state and thread_history directly, then maps THREAD_DATABASE_SPECS, whose members are logs, goals, memories, and queue. The source set is therefore state, thread_history, logs, goals, memories, and queue. The name's “every destination database” claims those same members, and the body manually queries each of them. No member differs at this revision; the defect is the independent completeness promise, not a currently missing assertion. > > Rename the test to “rolls back copied rows when index publication fails”. This keeps the test-output reader's useful task: identifying the failure trigger and rollback outcome. It removes only the global inventory claim. Leave the setup and assertions intact. The writing standard's One Idea, One Place guidance favors an outcome statement, and the restated-sets guideline explicitly excludes test names that claim completeness about a set defined by the code under test from the test-name exception. > > I read this test's symlink setup and database assertions, THREAD_DATABASE_SPECS, and the database selection and copy loop in main. No runtime reproduction is needed for the wording comparison, and none was run. A database selection derived by this test from the authority, together with a test name derived from that same selection, would establish that there is no independent copy; this name and its assertions are literal text. ### low — The precision-loss test decrements twice and misses lossy snapshot encoding - claim: `01M4H2NZSQ64ECWNP23YVXHTDS` - anchor: `skills/move-codex-session/scripts/move-codex-session.test.ts` (snippet) ``` CREATE TRIGGER nudge_timestamp AFTER INSERT ON threads BEGIN UPDATE rollout_migration_skipped_rollouts SET rollout_modified_at_ns = rollout_modified_at_ns - 1; END; ``` - lens: test-trimming · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4H2QF754GRV534VQE42HTMF` · valid: The exact-grounded AFTER INSERT trigger has no single-thread condition. The reported fixture and copy trace insert two threads, decrementing 9007199254740993 twice to 9007199254740991. These endpoints remain distinguishable after Number conversion, so the stated lossy BigInt encoding regression still triggers refusal and leaves the test green. The reported mutation probe corroborates that mechanism; its lsof shim does not affect this arithmetic comparison. Restricting the trigger to one inserted thread makes both endpoints round to 9007199254740992, restoring the promised precision check while retaining refusal and rollback assertions. This is a concrete ineffective-test defect under the supplied test guidelines. - disposition: none > The test promises to detect an unrelated integer change that JavaScript doubles cannot distinguish, but it still passes when snapshot encoding rounds every BigInt through Number. This leaves false confidence that the integer-precision regression is protected. > > createFixture inserts both ROOT_ID and CHILD_ID into source threads, and copyState inserts both into destination threads. The anchored AFTER INSERT trigger therefore subtracts one twice: 9007199254740993 becomes 9007199254740991. Number rounds the initial value to 9007199254740992, while the final value is exactly representable, so even a lossy snapshot detects this change. The assertions `assert.equal(refused.status, 1)` and `assert.equal(refused.stderr, "state triggers changed unrelated destination data\n")` do not distinguish that implementation from the precise one. > > Restrict the trigger to one inserted thread, for example with `WHEN NEW.id = '${ROOT_ID}'` inside the interpolated SQL. The final integer then becomes 9007199254740992, which rounds to the same Number as the initial value. Keep the refusal and rollback assertions; the repaired input restores the intended precision check. > > I traced createFixture, runMove, copyState, snapshotUnmovedRows and encodeSqliteValue in the test and implementation files, and inspected the adjacent "keeps integers above 2**53 in unmoved rows exact" test. In isolated copies, I changed encodeSqliteValue from `["integer", value.toString()]` to `["integer", Number(value).toString()]`. Running `node --test --test-name-pattern='integers above|doubles cannot'` passed both existing tests with both implementations. After adding the single-thread trigger condition, the precision test passed the original implementation and failed the mutant: the CLI exited zero instead of the expected one. > > The sandbox lacks lsof, so these targeted probes used a PATH shim returning exit one (no open processes); they exercised the real SQLite transfer and snapshot code but did not validate process detection. The unmodified full-suite attempt failed at that missing dependency. A run with real lsof would confirm the same mutation result without the shim. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M4H2H5DR4BZ93RFR6BM1D6ND Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | claims-emitted | 1 | no | | writing-quality | whole | default | claims-emitted | 1 | no | | test-trimming | whole | default | claims-emitted | 1 | no | | restated-sets | whole | default | claims-emitted | 1 | no | | project-docs | whole | default | no-claims | 1 | no |

🚀 1

forgejo-actions reviewed

2026-10-09 19:38:54 +00:00

skills/move-codex-session/SKILL.md

					
				@ -8,3 +8,3 @@

				Move one top-level session and its descendant agent sessions without losing rollout, state, history, or index data. User instructions override this skill's defaults.

				The bundled script supports macOS and Linux and requires Node.js 24+, `lsof`, and Perl. Run it with the session ID and explicit source and destination Codex homes. It holds Codex-compatible thread-writer and rollout-maintenance locks, and refuses incompatible schemas, destination conflicts, child-only moves, an active source home, or a destination home active outside the invoking process tree; resolve the reported blocker instead of editing the databases by hand.

				The bundled script supports macOS and Linux and requires Node.js 24+, `lsof`, and Perl. Run it with the session ID and explicit source and destination Codex homes. It holds Codex-compatible thread-writer and rollout-maintenance locks, write-locks each destination database while copying into it (seconds on a large home, so a Codex writing there from the invoking process tree can see `database is locked`), and refuses incompatible schemas, destination conflicts, child-only moves, an active source home, or a destination home active outside the invoking process tree; resolve the reported blocker instead of editing the databases by hand.

high — SKILL.md's list of refusals omits the new project-membership refusal and other refusals the script enforces

The sentence presents five refusal conditions as what the script refuses, and the script now refuses more than that. A reader who trusts the list will not expect a move of a project-bound thread to fail, or will believe that a failure outside the five is an unexpected malfunction rather than a designed refusal.

The defining source is skills/move-codex-session/scripts/move-codex-session.ts. The copy lists: incompatible schemas, destination conflicts, child-only moves, an active source home, and a destination home active outside the invoking process tree. The script's fail(...) calls cover those five: schema checks (schema mismatch for table, unsupported thread table), destination conflicts (destination state already contains thread, destination index already contains thread), child-only (is a child of ... move the top-level session instead), source Codex home is active in process, and destination Codex home is active in unrelated process. It also refuses cases the copy does not name. The diff adds requireThreadsOutsideProjects, which fails with thread ${id} belongs to project ${thread.project_id}; project rows are not moved, and the accompanying test "refuses to move a thread that belongs to a project". It also refuses a rollout without session_meta that Codex did not record in rollout_migration_skipped_rollouts, a rollout path outside its Codex home, and a rollout checksum mismatch. So the copy already omits members of its source; at least the project refusal arrived in the same change that edited this sentence.

No exception applies. The sentence is orientation for an agent that runs the script and then reads the reported error; the closing clause already tells the reader to resolve the reported blocker, and the script's message names the specific cause. Exact members are not needed to run the command, and a list that is incomplete misleads more than it helps.

Correction: replace the enumeration with the outcome and let the error message carry the member, for example "...and refuses a move it cannot complete without losing or overwriting data, naming the blocker in its error; resolve the reported blocker instead of editing the databases by hand." If an enumerated condition is needed, keep only the one the invoker can act on before running (the destination home must be idle apart from the invoking process tree) and point to the script's error output for the rest.

Examined: SKILL.md in full, every fail( site in move-codex-session.ts, requireThreadsOutsideProjects and its call site, and the test names added in the diff. Not run: the script itself. The decisive evidence is the project-membership fail(...) at the requireThreadsOutsideProjects function, which the sentence does not cover.

lens restated-sets · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4H2KA4PZDF580K9XHQD4C3T of review 01M4H2GP9Y7V9GND1K25M4P77M

**high** — SKILL.md's list of refusals omits the new project-membership refusal and other refusals the script enforces > The sentence presents five refusal conditions as what the script refuses, and the script now refuses more than that. A reader who trusts the list will not expect a move of a project-bound thread to fail, or will believe that a failure outside the five is an unexpected malfunction rather than a designed refusal. > > The defining source is `skills/move-codex-session/scripts/move-codex-session.ts`. The copy lists: incompatible schemas, destination conflicts, child-only moves, an active source home, and a destination home active outside the invoking process tree. The script's `fail(...)` calls cover those five: schema checks (`schema mismatch for table`, `unsupported thread table`), destination conflicts (`destination state already contains thread`, `destination index already contains thread`), child-only (`is a child of ... move the top-level session instead`), `source Codex home is active in process`, and `destination Codex home is active in unrelated process`. It also refuses cases the copy does not name. The diff adds `requireThreadsOutsideProjects`, which fails with `thread ${id} belongs to project ${thread.project_id}; project rows are not moved`, and the accompanying test "refuses to move a thread that belongs to a project". It also refuses a rollout without `session_meta` that Codex did not record in `rollout_migration_skipped_rollouts`, a rollout path outside its Codex home, and a rollout checksum mismatch. So the copy already omits members of its source; at least the project refusal arrived in the same change that edited this sentence. > > No exception applies. The sentence is orientation for an agent that runs the script and then reads the reported error; the closing clause already tells the reader to resolve the reported blocker, and the script's message names the specific cause. Exact members are not needed to run the command, and a list that is incomplete misleads more than it helps. > > Correction: replace the enumeration with the outcome and let the error message carry the member, for example "...and refuses a move it cannot complete without losing or overwriting data, naming the blocker in its error; resolve the reported blocker instead of editing the databases by hand." If an enumerated condition is needed, keep only the one the invoker can act on before running (the destination home must be idle apart from the invoking process tree) and point to the script's error output for the rest. > > Examined: SKILL.md in full, every `fail(` site in move-codex-session.ts, `requireThreadsOutsideProjects` and its call site, and the test names added in the diff. Not run: the script itself. The decisive evidence is the project-membership `fail(...)` at the `requireThreadsOutsideProjects` function, which the sentence does not cover. lens `restated-sets` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4H2KA4PZDF580K9XHQD4C3T` of review `01M4H2GP9Y7V9GND1K25M4P77M`

🚀 1

medium — “Invoking process tree” overstates which active destination processes are allowed

An agent preparing the move can leave a destination-writing child or sibling of the invoking Codex running because it belongs to the same process tree, yet the script rejects that process as unrelated. The same phrase appears in the warning about destination database contention, making the allowed activity boundary ambiguous.

In scripts/move-codex-session.ts, ancestorProcessIds starts with process.pid and follows process.ppid upward using ps; it never walks descendants or siblings. assertHomesAvailable permits only those PIDs and the script's lock-helper PIDs. The test “moves a complete session tree and fails closed on unsafe state” starts a destination file-holder child of the test process and expects an “active in unrelated process” failure, whereas a file descriptor held by the test process itself is allowed when it invokes the move. Thus a process in the invoker's wider tree is outside the allowed ancestry chain.

Use “the script or its ancestor processes” for the contention warning and destination activity boundary. Keep the active-source restriction and the instruction to resolve reported blockers. The writing standard's Use Precise Language guidance calls for a verifiable boundary; this detail serves the agent's concrete task of deciding which destination processes may remain active, so replacing it with a source pointer would lose necessary operational meaning.

I traced ancestorProcessIds and assertHomesAvailable, their calls in main, and the corresponding test setup and assertions. This is a static comparison; I did not run the test. A descendant traversal or another PID allowance on that call path would refute the claim, but neither appears in the reviewed implementation.

lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4H2NJXVF1HPQ2QPTEEQYXWX of review 01M4H2GP9Y7V9GND1K25M4P77M

**medium** — “Invoking process tree” overstates which active destination processes are allowed > An agent preparing the move can leave a destination-writing child or sibling of the invoking Codex running because it belongs to the same process tree, yet the script rejects that process as unrelated. The same phrase appears in the warning about destination database contention, making the allowed activity boundary ambiguous. > > In scripts/move-codex-session.ts, ancestorProcessIds starts with process.pid and follows process.ppid upward using ps; it never walks descendants or siblings. assertHomesAvailable permits only those PIDs and the script's lock-helper PIDs. The test “moves a complete session tree and fails closed on unsafe state” starts a destination file-holder child of the test process and expects an “active in unrelated process” failure, whereas a file descriptor held by the test process itself is allowed when it invokes the move. Thus a process in the invoker's wider tree is outside the allowed ancestry chain. > > Use “the script or its ancestor processes” for the contention warning and destination activity boundary. Keep the active-source restriction and the instruction to resolve reported blockers. The writing standard's Use Precise Language guidance calls for a verifiable boundary; this detail serves the agent's concrete task of deciding which destination processes may remain active, so replacing it with a source pointer would lose necessary operational meaning. > > I traced ancestorProcessIds and assertHomesAvailable, their calls in main, and the corresponding test setup and assertions. This is a static comparison; I did not run the test. A descendant traversal or another PID allowance on that call path would refute the claim, but neither appears in the reviewed implementation. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4H2NJXVF1HPQ2QPTEEQYXWX` of review `01M4H2GP9Y7V9GND1K25M4P77M`

🚀 1

jercik marked this conversation as resolved

skills/move-codex-session/scripts/move-codex-session.test.ts

Lines 1330-1334

					
				@ -751,0 +1327,8 @@

				      `

				        INSERT INTO rollout_migration_skipped_rollouts (migration_id, rollout_path, rollout_modified_at_ns)

				          VALUES ('big', 'beyond-safe-integer.jsonl', 9007199254740993);

				        CREATE TRIGGER nudge_timestamp

				        AFTER INSERT ON threads

				        BEGIN

				          UPDATE rollout_migration_skipped_rollouts SET rollout_modified_at_ns = rollout_modified_at_ns - 1;

				        END;

low — The precision-loss test decrements twice and misses lossy snapshot encoding

The test promises to detect an unrelated integer change that JavaScript doubles cannot distinguish, but it still passes when snapshot encoding rounds every BigInt through Number. This leaves false confidence that the integer-precision regression is protected.

createFixture inserts both ROOT_ID and CHILD_ID into source threads, and copyState inserts both into destination threads. The anchored AFTER INSERT trigger therefore subtracts one twice: 9007199254740993 becomes 9007199254740991. Number rounds the initial value to 9007199254740992, while the final value is exactly representable, so even a lossy snapshot detects this change. The assertions assert.equal(refused.status, 1) and assert.equal(refused.stderr, "state triggers changed unrelated destination data\n") do not distinguish that implementation from the precise one.

Restrict the trigger to one inserted thread, for example with WHEN NEW.id = '${ROOT_ID}' inside the interpolated SQL. The final integer then becomes 9007199254740992, which rounds to the same Number as the initial value. Keep the refusal and rollback assertions; the repaired input restores the intended precision check.

I traced createFixture, runMove, copyState, snapshotUnmovedRows and encodeSqliteValue in the test and implementation files, and inspected the adjacent "keeps integers above 2**53 in unmoved rows exact" test. In isolated copies, I changed encodeSqliteValue from ["integer", value.toString()] to ["integer", Number(value).toString()]. Running node --test --test-name-pattern='integers above|doubles cannot' passed both existing tests with both implementations. After adding the single-thread trigger condition, the precision test passed the original implementation and failed the mutant: the CLI exited zero instead of the expected one.

The sandbox lacks lsof, so these targeted probes used a PATH shim returning exit one (no open processes); they exercised the real SQLite transfer and snapshot code but did not validate process detection. The unmodified full-suite attempt failed at that missing dependency. A run with real lsof would confirm the same mutation result without the shim.

lens test-trimming · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4H2NZSQ64ECWNP23YVXHTDS of review 01M4H2GP9Y7V9GND1K25M4P77M

**low** — The precision-loss test decrements twice and misses lossy snapshot encoding > The test promises to detect an unrelated integer change that JavaScript doubles cannot distinguish, but it still passes when snapshot encoding rounds every BigInt through Number. This leaves false confidence that the integer-precision regression is protected. > > createFixture inserts both ROOT_ID and CHILD_ID into source threads, and copyState inserts both into destination threads. The anchored AFTER INSERT trigger therefore subtracts one twice: 9007199254740993 becomes 9007199254740991. Number rounds the initial value to 9007199254740992, while the final value is exactly representable, so even a lossy snapshot detects this change. The assertions `assert.equal(refused.status, 1)` and `assert.equal(refused.stderr, "state triggers changed unrelated destination data\n")` do not distinguish that implementation from the precise one. > > Restrict the trigger to one inserted thread, for example with `WHEN NEW.id = '${ROOT_ID}'` inside the interpolated SQL. The final integer then becomes 9007199254740992, which rounds to the same Number as the initial value. Keep the refusal and rollback assertions; the repaired input restores the intended precision check. > > I traced createFixture, runMove, copyState, snapshotUnmovedRows and encodeSqliteValue in the test and implementation files, and inspected the adjacent "keeps integers above 2**53 in unmoved rows exact" test. In isolated copies, I changed encodeSqliteValue from `["integer", value.toString()]` to `["integer", Number(value).toString()]`. Running `node --test --test-name-pattern='integers above|doubles cannot'` passed both existing tests with both implementations. After adding the single-thread trigger condition, the precision test passed the original implementation and failed the mutant: the CLI exited zero instead of the expected one. > > The sandbox lacks lsof, so these targeted probes used a PATH shim returning exit one (no open processes); they exercised the real SQLite transfer and snapshot code but did not validate process detection. The unmodified full-suite attempt failed at that missing dependency. A run with real lsof would confirm the same mutation result without the shim. lens `test-trimming` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4H2NZSQ64ECWNP23YVXHTDS` of review `01M4H2GP9Y7V9GND1K25M4P77M`

🚀 1

jercik marked this conversation as resolved

skills/move-codex-session/scripts/move-codex-session.test.ts

					
				@ -751,0 +1671,4 @@

				  }

				});

				test("rolls back every destination database when a step after the queue copy fails", () => {

medium — The rollback test name claims completeness over the script’s database set

Adding a destination database to the mover creates another place to update: this test name continues promising coverage of every destination database even if its assertions still cover only the old selection. The wording makes test output a separately maintained completeness claim.

The authority is main in scripts/move-codex-session.ts: it selects state and thread_history directly, then maps THREAD_DATABASE_SPECS, whose members are logs, goals, memories, and queue. The source set is therefore state, thread_history, logs, goals, memories, and queue. The name's “every destination database” claims those same members, and the body manually queries each of them. No member differs at this revision; the defect is the independent completeness promise, not a currently missing assertion.

Rename the test to “rolls back copied rows when index publication fails”. This keeps the test-output reader's useful task: identifying the failure trigger and rollback outcome. It removes only the global inventory claim. Leave the setup and assertions intact. The writing standard's One Idea, One Place guidance favors an outcome statement, and the restated-sets guideline explicitly excludes test names that claim completeness about a set defined by the code under test from the test-name exception.

I read this test's symlink setup and database assertions, THREAD_DATABASE_SPECS, and the database selection and copy loop in main. No runtime reproduction is needed for the wording comparison, and none was run. A database selection derived by this test from the authority, together with a test name derived from that same selection, would establish that there is no independent copy; this name and its assertions are literal text.

lens writing-quality · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4H2PTSH7WV19VM9FTPN47GF of review 01M4H2GP9Y7V9GND1K25M4P77M

**medium** — The rollback test name claims completeness over the script’s database set > Adding a destination database to the mover creates another place to update: this test name continues promising coverage of every destination database even if its assertions still cover only the old selection. The wording makes test output a separately maintained completeness claim. > > The authority is main in scripts/move-codex-session.ts: it selects state and thread_history directly, then maps THREAD_DATABASE_SPECS, whose members are logs, goals, memories, and queue. The source set is therefore state, thread_history, logs, goals, memories, and queue. The name's “every destination database” claims those same members, and the body manually queries each of them. No member differs at this revision; the defect is the independent completeness promise, not a currently missing assertion. > > Rename the test to “rolls back copied rows when index publication fails”. This keeps the test-output reader's useful task: identifying the failure trigger and rollback outcome. It removes only the global inventory claim. Leave the setup and assertions intact. The writing standard's One Idea, One Place guidance favors an outcome statement, and the restated-sets guideline explicitly excludes test names that claim completeness about a set defined by the code under test from the test-name exception. > > I read this test's symlink setup and database assertions, THREAD_DATABASE_SPECS, and the database selection and copy loop in main. No runtime reproduction is needed for the wording comparison, and none was run. A database selection derived by this test from the authority, together with a test name derived from that same selection, would establish that there is no independent copy; this name and its assertions are literal text. lens `writing-quality` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4H2PTSH7WV19VM9FTPN47GF` of review `01M4H2GP9Y7V9GND1K25M4P77M`

🚀 1

jercik marked this conversation as resolved

skills/move-codex-session/scripts/move-codex-session.ts

Lines 1822-1827

					
				@ -1535,0 +1819,9 @@

				      // The compared destination rows must still be the ones the source commit stands behind: a writer that changed

				      // or deleted them since the comparison would leave the session in neither home.

				      try {

				        destinationState.exec("BEGIN IMMEDIATE");

				        destinationHistory.exec("BEGIN IMMEDIATE");

				        const currentDigests: Array<[string, Record<string, string>, Record<string, string>]> = [

				          ["state", comparedStateDigests, movedRowDigests(destinationState, STATE_TABLES, threadIds, removableSectionIds)],

				          ["history", comparedHistoryDigests, movedRowDigests(destinationHistory, HISTORY_TABLES, historyIds.copy, [])],

				        ];

high — Source deletion guards state and history but can discard the only remaining auxiliary rows

The move can commit source deletion after destination goal, memory, queue, or log rows disappear or change. A permitted destination writer can modify these rows after main's last comparison; the source then loses its original rows without checking that the destination still holds the recovery copy. This leaves an incomplete moved session despite the successful source-deletion checks.

main verifies each thread database before awaiting verifyRollouts. deleteSourceDatabases subsequently deletes the source rows from every thread database, but compares and write-locks only destination state and history before committing. Its snapshots of auxiliary databases check unrelated source rows, not the destination copy. Extend the commit precondition to the auxiliary destination databases, comparing transferred contents with the appropriate generated-key normalization and retaining their write locks through the source commit.

I traced main, verifyDatabaseContents, copyThreadDatabase, and deleteSourceDatabases and examined the tests that preserve the source after destination state or history deletion. I ran node /app/probe-source-delete.ts using unmodified subject function bodies and the subject's createFixture. The probe copied and verified every auxiliary database, deleted the root's destination thread_goals row, and invoked deleteSourceDatabases. It returned successfully; both source and destination queries for that goal returned empty arrays, and the source retained only the unrelated thread. Full CLI execution was unavailable because lsof is missing.

A CLI test committing a destination goal deletion after the final comparison should leave the source intact or reject the move. The probe establishes that the source-deletion function currently accepts this interleaving; the actual frequency of such destination edits in Codex is not established by this repository.

lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4H2P36D6VPE1K504TAQ5E05 of review 01M4H2GP9Y7V9GND1K25M4P77M

**high** — Source deletion guards state and history but can discard the only remaining auxiliary rows > The move can commit source deletion after destination goal, memory, queue, or log rows disappear or change. A permitted destination writer can modify these rows after main's last comparison; the source then loses its original rows without checking that the destination still holds the recovery copy. This leaves an incomplete moved session despite the successful source-deletion checks. > > main verifies each thread database before awaiting verifyRollouts. deleteSourceDatabases subsequently deletes the source rows from every thread database, but compares and write-locks only destination state and history before committing. Its snapshots of auxiliary databases check unrelated source rows, not the destination copy. Extend the commit precondition to the auxiliary destination databases, comparing transferred contents with the appropriate generated-key normalization and retaining their write locks through the source commit. > > I traced main, verifyDatabaseContents, copyThreadDatabase, and deleteSourceDatabases and examined the tests that preserve the source after destination state or history deletion. I ran `node /app/probe-source-delete.ts` using unmodified subject function bodies and the subject's createFixture. The probe copied and verified every auxiliary database, deleted the root's destination thread_goals row, and invoked deleteSourceDatabases. It returned successfully; both source and destination queries for that goal returned empty arrays, and the source retained only the unrelated thread. Full CLI execution was unavailable because lsof is missing. > > A CLI test committing a destination goal deletion after the final comparison should leave the source intact or reject the move. The probe establishes that the source-deletion function currently accepts this interleaving; the actual frequency of such destination edits in Codex is not established by this repository. lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4H2P36D6VPE1K504TAQ5E05` of review `01M4H2GP9Y7V9GND1K25M4P77M`

🚀 1

jercik marked this conversation as resolved

skills/move-codex-session/scripts/move-codex-session.ts

Lines 1927-1929

					
				@ -1645,0 +1924,6 @@

				      deleteDestinationDatabases(

				        statePath,

				        historyPath,

				        stateCopied ? threadIds : [],

				        historyIds,

				        stateCopied ? newSectionIds : [],

critical — Rollback deletes destination history even when the history copy failed

A failed move can delete history written by the permitted destination process. If state copying succeeds, then a destination writer inserts a conflicting history row before history copying, the history INSERT fails and rolls back. Cleanup nevertheless deletes that writer's row, even though this move never committed any history rows.

main sets historyCopied only after copyHistory returns. cleanupDestination checks history ownership only when historyCopied is true, but passes all historyIds to deleteDestinationDatabases whenever either stateCopied or historyCopied is true. That function deletes matching rows from all four history tables. Gate the history IDs passed to deletion on historyCopied, just as the state IDs are gated on stateCopied.

I traced main, copyHistory, cleanupDestination, and deleteDestinationDatabases and ran node /app/probe-rollback.ts against unmodified function bodies extracted from the subject, using createFixture from the subject tests. After copyState, the probe inserted a conflicting destination thread_turns row. copyHistory reported UNIQUE constraint failed: thread_turns.thread_id, thread_turns.turn_id; the row remained before cleanup and the query returned an empty array after cleanup. The SKILL.md and assertHomesAvailable explicitly permit destination activity from the invoking process. Full CLI execution was unavailable because lsof is missing.

Passing an empty history-ID list when historyCopied is false should preserve the conflicting row while removing copied state. A CLI reproduction with a destination writer committing between preflight and copyHistory would establish the complete timing path; the direct probe establishes the destructive cleanup mechanism.

lens general-bug · arm default · tally 1 valid / 0 invalid / 0 uncertain
claim 01M4H2MB9PP48NTKF5J0NPDZ7V of review 01M4H2GP9Y7V9GND1K25M4P77M

**critical** — Rollback deletes destination history even when the history copy failed > A failed move can delete history written by the permitted destination process. If state copying succeeds, then a destination writer inserts a conflicting history row before history copying, the history INSERT fails and rolls back. Cleanup nevertheless deletes that writer's row, even though this move never committed any history rows. > > main sets historyCopied only after copyHistory returns. cleanupDestination checks history ownership only when historyCopied is true, but passes all historyIds to deleteDestinationDatabases whenever either stateCopied or historyCopied is true. That function deletes matching rows from all four history tables. Gate the history IDs passed to deletion on historyCopied, just as the state IDs are gated on stateCopied. > > I traced main, copyHistory, cleanupDestination, and deleteDestinationDatabases and ran `node /app/probe-rollback.ts` against unmodified function bodies extracted from the subject, using createFixture from the subject tests. After copyState, the probe inserted a conflicting destination thread_turns row. copyHistory reported `UNIQUE constraint failed: thread_turns.thread_id, thread_turns.turn_id`; the row remained before cleanup and the query returned an empty array after cleanup. The SKILL.md and assertHomesAvailable explicitly permit destination activity from the invoking process. Full CLI execution was unavailable because lsof is missing. > > Passing an empty history-ID list when historyCopied is false should preserve the conflicting row while removing copied state. A CLI reproduction with a destination writer committing between preflight and copyHistory would establish the complete timing path; the direct probe establishes the destructive cleanup mechanism. lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4H2MB9PP48NTKF5J0NPDZ7V` of review `01M4H2GP9Y7V9GND1K25M4P77M`

🚀 1

jercik marked this conversation as resolved

jercik referenced this pull request

2026-10-09 19:50:02 +00:00

fix: move-codex-session rollback should keep destination history rows it never copied #133

jercik referenced this pull request

2026-10-09 19:59:02 +00:00

fix: move-codex-session skill should state which processes block a move #134

jercik referenced this pull request

2026-10-09 19:59:03 +00:00

test: name the move-codex-session rollback test for its trigger and outcome #135

jercik referenced this pull request

2026-10-09 19:59:05 +00:00

test: make the move-codex-session precision test fire its trigger once #136

jercik commented

2026-10-09 20:01:05 +00:00

Replying to review comment #150303

Confirmed, and it predates this PR: main also passes the moved thread IDs to the destination history rollback whenever the state copy committed. Tracked in #133, which deletes destination history only after the history copy commits. Its regression test has a destination writer commit a history row between the two copies; on this PR's head the rollback deletes that row.

> Replying to review comment #150303 Confirmed, and it predates this PR: `main` also passes the moved thread IDs to the destination history rollback whenever the state copy committed. Tracked in https://code.j4k.dev/j4k-oss/agent-skills/pulls/133, which deletes destination history only after the history copy commits. Its regression test has a destination writer commit a history row between the two copies; on this PR's head the rollback deletes that row.

jercik commented

2026-10-09 20:01:09 +00:00

Replying to review comment #150304

Confirmed: the script also refuses project-bound threads, unrecorded legacy rollouts, rollout paths outside the home and checksum mismatches, and the incomplete list predates this PR. Tracked in #134, which replaces the list with the two home-activity requirements a caller can act on and points to the script's error for every other refusal.

> Replying to review comment #150304 Confirmed: the script also refuses project-bound threads, unrecorded legacy rollouts, rollout paths outside the home and checksum mismatches, and the incomplete list predates this PR. Tracked in https://code.j4k.dev/j4k-oss/agent-skills/pulls/134, which replaces the list with the two home-activity requirements a caller can act on and points to the script's error for every other refusal.

jercik commented

2026-10-09 20:01:13 +00:00

Replying to review comment #150306

Confirmed: ancestorProcessIds allows only the script and its ancestors, so a process the invoking Codex started still blocks the move. The process-tree wording predates this PR. Tracked in #134, which states the ancestor rule in the skill.

> Replying to review comment #150306 Confirmed: `ancestorProcessIds` allows only the script and its ancestors, so a process the invoking Codex started still blocks the move. The process-tree wording predates this PR. Tracked in https://code.j4k.dev/j4k-oss/agent-skills/pulls/134, which states the ancestor rule in the skill.

jercik commented

2026-10-09 20:01:18 +00:00

Replying to review comment #150307

Agreed. Tracked in #135, which renames the test to "rolls back copied rows when index publication fails" and leaves its setup and assertions unchanged.

> Replying to review comment #150307 Agreed. Tracked in https://code.j4k.dev/j4k-oss/agent-skills/pulls/135, which renames the test to "rolls back copied rows when index publication fails" and leaves its setup and assertions unchanged.

jercik commented

2026-10-09 20:01:22 +00:00

Replying to review comment #150308

Confirmed with the mutation you described: with encodeSqliteValue returning Number(value).toString(), the current test passes, and with the trigger limited to the root thread it fails because the move exits 0. Tracked in #136.

> Replying to review comment #150308 Confirmed with the mutation you described: with `encodeSqliteValue` returning `Number(value).toString()`, the current test passes, and with the trigger limited to the root thread it fails because the move exits 0. Tracked in https://code.j4k.dev/j4k-oss/agent-skills/pulls/136.

jercik referenced this pull request

2026-10-09 20:01:33 +00:00

fix: move-codex-session rollback should keep destination history rows it never copied #133

~~jercik referenced this pull request 2026-10-09 20:07:59 +00:00~~

test: check every moved move-codex-session table in the index-publication rollback test #137

jercik referenced this pull request

2026-10-09 20:56:10 +00:00

fix: move-codex-session should re-check goals, memories, queue and logs before deleting the source #138

jercik commented

2026-10-09 20:56:46 +00:00

Replying to review comment #150305

Confirmed, and it predates this PR: main's deleteSourceDatabases also compares and write-locks only the destination state and history databases before committing the source deletion. Tracked in #138, which compares the goals, memories, queue and logs copies too and holds write locks on all six destination databases through the source commit. Its regression test deletes a destination goal after the comparison; on this PR's head the move exits 0 and deletes the source rows.

> Replying to review comment #150305 Confirmed, and it predates this PR: `main`'s `deleteSourceDatabases` also compares and write-locks only the destination state and history databases before committing the source deletion. Tracked in https://code.j4k.dev/j4k-oss/agent-skills/pulls/138, which compares the goals, memories, queue and logs copies too and holds write locks on all six destination databases through the source commit. Its regression test deletes a destination goal after the comparison; on this PR's head the move exits 0 and deletes the source rows.

jercik merged commit 9607aa28cb into main

2026-10-09 20:58:05 +00:00

jercik referenced this pull request from a commit

2026-10-09 20:58:05 +00:00

fix: `move-codex-session` should handle Codex 0.160.1 and large native homes (#132)

jercik deleted branch fix/move-codex-session-codex-0160

2026-10-09 20:58:06 +00:00

jercik referenced this pull request

2026-10-10 03:57:26 +00:00

move-codex-session leaks database connections when a later open throws #139

jercik referenced this pull request

2026-10-10 06:57:38 +00:00

move-codex-session cannot finish a move that fails after the source commit #142

Sign in to join this conversation.

No reviewers

No labels

No milestone

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

j4k-oss/agent-skills!132

Rows
Columns

fix: move-codex-session should handle Codex 0.160.1 and large native homes #132

Review — j4k-oss/agent-skills @ 40154724b6

Findings (6)

critical — Rollback deletes destination history even when the history copy failed

high — SKILL.md's list of refusals omits the new project-membership refusal and other refusals the script enforces

high — Source deletion guards state and history but can discard the only remaining auxiliary rows

medium — “Invoking process tree” overstates which active destination processes are allowed

medium — The rollback test name claims completeness over the script’s database set

low — The precision-loss test decrements twice and misses lossy snapshot encoding

Other claims

Coverage

fix: `move-codex-session` should handle Codex 0.160.1 and large native homes #132

Review — j4k-oss/agent-skills @ `40154724b6`