jercik commented

Owner

Follow-up to #132 from its review (finding).

Renames the rollback test from "rolls back every destination database when a step after the queue copy fails" to "rolls back copied rows when index publication fails". The old name promised coverage of every destination database, which the test's hand-written assertions would have to keep true each time the script gains a database. Setup and assertions are unchanged.

Follow-up to #132 from its review ([finding](https://code.j4k.dev/j4k-oss/agent-skills/pulls/132#issuecomment-150307)). Renames the rollback test from "rolls back every destination database when a step after the queue copy fails" to "rolls back copied rows when index publication fails". The old name promised coverage of every destination database, which the test's hand-written assertions would have to keep true each time the script gains a database. Setup and assertions are unchanged.

jercik added 1 commit

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

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

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

Details

Review / Review (pull_request_target) Successful in 2m25s

Details

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

Details

d3baeebc1e

The name promised coverage of every destination database, a claim the
script's database list would have to keep true. It now names the failure
that triggers the rollback and the outcome.

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

jercik referenced this pull request

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

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

forgejo-actions commented

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

Review 01M4H3Z011KMV9X91RB8GEDBNA — head f4cf20e273e7d893668f32c22bd414146094eda6

Review — j4k-oss/agent-skills @ `5e81dbd147`

Scope: diff against base tree 40154724b696
Status: dispatched — coverage complete (2/2 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 (1)

medium — Index-publication rollback test leaves copied tables unchecked

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

    assert.deepEqual(queryRows(fixture.destinationStatePath, "SELECT id FROM threads"), []);

lens: test-trimming · arm: default
verdicts: 1 valid / 0 invalid / 0 uncertain
- pass 01M4H4269NK1939KFWDQPQZMAQ · valid: The exact-grounded assertion establishes only that destination threads are empty. The reviewer supplies a concrete additional trace: the fixture contains a copied thread_spawn_edges row, index publication fails after copying, rollback removes destination rows, and the failure test never checks that edge. Omitting edge cleanup would therefore leave this existing rollback test green despite stale copied data. The described success test checks copying, and the separate attachments/realtime rollback test does not cover edge cleanup; neither refutes this gap. This meets the ineffective-test standard without an executed mutation. Retain medium severity. The smallest supported repair is a post-failure assertion that the fixture edge is removed while destination-owned rows survive; the evidence does not justify requiring exhaustive checks of every other listed table. Narrowing the title alone would not restore the missing rollback protection.
disposition: none

The test can pass while a failed move leaves copied rows behind in tables it never inspects, so it does not establish the rollback of the rows its title promises. Its fixture includes a root-to-child thread_spawn_edges row, but after the index publication failure it checks only destination threads, history thread_turns, selected logs/goals/memory/queue tables, and queue revisions; it does not query thread_spawn_edges, thread_dynamic_tools, thread_attachments, history items/projection/realtime tables, or goal deferrals. For example, a regression that skips deleting thread_spawn_edges during rollback would leave the fixture's edge referencing the removed root/child while every assertion in this test still passes. The normal-success test checks the edge's copied contents, but not its rollback; the separate rollback test for attachments and realtime items protects only those tables. move-codex-session.ts copies state/history and each thread database before publishing the index, then calls deleteDestinationDatabases and deleteThreadDatabaseRows on failure, so a missed cleanup in one table is a consequential stale-data case on this exact failure path. Add post-failure empty-row checks for every moving-thread/history table (while preserving destination-owned rows), or otherwise narrow the test's stated contract to the subset it verifies. I inspected this test, the successful move assertions, the attachments/realtime rollback test, and the copy/publish/catch rollback path; I did not execute a mutation. A mutation that omits deletion of thread_spawn_edges would establish the gap if this test remains green; equivalent rollback coverage for that table would refute it.

Other claims

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

Coverage

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

lens	part	arm	unit status	runs	loss
general-bug	whole	default	no-claims	1	no
test-trimming	whole	default	claims-emitted	1	no

writing-quality — skipped-by-dispatch: only a test title string changed; no prose or documentation
restated-sets — skipped-by-dispatch: no enumerated sets or lists changed
project-docs — skipped-by-dispatch: no documentation changed

**Review** `01M4H3Z011KMV9X91RB8GEDBNA` — head `f4cf20e273e7d893668f32c22bd414146094eda6` # Review — j4k-oss/agent-skills @ 5e81dbd1471c Scope: diff against base tree `40154724b696` Status: dispatched — coverage complete (2/2 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 (1) ### medium — Index-publication rollback test leaves copied tables unchecked - claim: `01M4H41HT6VPDJWAQEGK3SWH9H` - anchor: `skills/move-codex-session/scripts/move-codex-session.test.ts` (snippet) ``` assert.deepEqual(queryRows(fixture.destinationStatePath, "SELECT id FROM threads"), []); ``` - lens: test-trimming · arm: default - verdicts: 1 valid / 0 invalid / 0 uncertain - pass `01M4H4269NK1939KFWDQPQZMAQ` · valid: The exact-grounded assertion establishes only that destination threads are empty. The reviewer supplies a concrete additional trace: the fixture contains a copied thread_spawn_edges row, index publication fails after copying, rollback removes destination rows, and the failure test never checks that edge. Omitting edge cleanup would therefore leave this existing rollback test green despite stale copied data. The described success test checks copying, and the separate attachments/realtime rollback test does not cover edge cleanup; neither refutes this gap. This meets the ineffective-test standard without an executed mutation. Retain medium severity. The smallest supported repair is a post-failure assertion that the fixture edge is removed while destination-owned rows survive; the evidence does not justify requiring exhaustive checks of every other listed table. Narrowing the title alone would not restore the missing rollback protection. - disposition: none > The test can pass while a failed move leaves copied rows behind in tables it never inspects, so it does not establish the rollback of the rows its title promises. Its fixture includes a root-to-child `thread_spawn_edges` row, but after the index publication failure it checks only destination `threads`, history `thread_turns`, selected logs/goals/memory/queue tables, and queue revisions; it does not query `thread_spawn_edges`, `thread_dynamic_tools`, `thread_attachments`, history items/projection/realtime tables, or goal deferrals. For example, a regression that skips deleting `thread_spawn_edges` during rollback would leave the fixture's edge referencing the removed root/child while every assertion in this test still passes. The normal-success test checks the edge's copied contents, but not its rollback; the separate rollback test for attachments and realtime items protects only those tables. `move-codex-session.ts` copies state/history and each thread database before publishing the index, then calls `deleteDestinationDatabases` and `deleteThreadDatabaseRows` on failure, so a missed cleanup in one table is a consequential stale-data case on this exact failure path. Add post-failure empty-row checks for every moving-thread/history table (while preserving destination-owned rows), or otherwise narrow the test's stated contract to the subset it verifies. I inspected this test, the successful move assertions, the attachments/realtime rollback test, and the copy/publish/catch rollback path; I did not execute a mutation. A mutation that omits deletion of `thread_spawn_edges` would establish the gap if this test remains green; equivalent rollback coverage for that table would refute it. ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M4H3ZJX2HJ3020AV4R55BX8E Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | test-trimming | whole | default | claims-emitted | 1 | no | - writing-quality — skipped-by-dispatch: only a test title string changed; no prose or documentation - restated-sets — skipped-by-dispatch: no enumerated sets or lists changed - project-docs — skipped-by-dispatch: no documentation changed

🚀 1

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 commented

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

Author

Owner

Replying to review comment #150379

The summary's finding (claim 01M4H41HT6VPDJWAQEGK3SWH9H) is valid for this test: with the destination rollback's thread_spawn_edges or thread_dynamic_tools delete removed, it still passes. The suite was not blind to it, since "moves a complete session tree and fails closed on unsafe state" fails both mutants through its retry. The assertions predate this PR, which only renames the test. Tracked in #137, which checks every table the fixture moves and fails both mutants.

> Replying to review comment #150379 The summary's finding (claim `01M4H41HT6VPDJWAQEGK3SWH9H`) is valid for this test: with the destination rollback's `thread_spawn_edges` or `thread_dynamic_tools` delete removed, it still passes. The suite was not blind to it, since "moves a complete session tree and fails closed on unsafe state" fails both mutants through its retry. The assertions predate this PR, which only renames the test. Tracked in https://code.j4k.dev/j4k-oss/agent-skills/pulls/137, which checks every table the fixture moves and fails both mutants.

jercik changed target branch from fix/move-codex-session-codex-0160 to main

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

jercik force-pushed test/move-codex-session-rollback-test-name from d3baeebc1e

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

Details

Review / Review (pull_request_target) Successful in 2m25s

Details

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

Details

to f4cf20e273

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

Details