j4k-oss/agent-skills
Watch
1
0
Fork
You've already forked agent-skills
 0
Code Issues 6 Pull requests Releases Packages Activity Actions

test: move-codex-session editor helper should wait for the script #146

Merged
jercik merged 2 commits from test/move-codex-session-editor-helper into main 2026-10-10 07:45:03 +00:00
Conversation 2 Commits 2 Files changed 1 +71 -11
jercik commented 2026-10-10 07:33:19 +00:00
Owner
Copy link

Closes #141. The editor helper could throw before awaiting the script, and its callers then deleted the fixture under a live script. It now settles the editor and the script together. If both fail, it throws an AggregateError. If only the editor fails, the error carries the script's exit status and stderr, with the editor error as cause. If only the script fails, its own error propagates.

Beyond #141, the history and goals tests no longer rely on a fixed 3 s editor window. The editor holds the edited database's write lock and polls the destination state database with a timeout: 0 BEGIN IMMEDIATE. A busy result means the script's final re-check holds the state lock, so the comparison is over and the editor commits.

In 82 traced runs (up to 16 in parallel), the probe always fired after the script took the state lock and before it took the edited database's lock. Removing the re-check, reading digests before locking, and using a plain BEGIN in the script each make these tests fail. The three callers passed 10 of 10 runs, and 5 of 5 under 32 CPU hogs.

The state test keeps a bounded 15 s wait (STATE_EDIT_WINDOW_MS, under the script's 30 s lock wait). State is the first lock, so nothing is observable there, and polling for copied rows fails an earlier verification. The suite is about 12 s slower. A deterministic wait would need a pause hook in the script, which this PR leaves out.

🤖 Generated with Claude Code

Closes #141. The editor helper could throw before awaiting the script, and its callers then deleted the fixture under a live script. It now settles the editor and the script together. If both fail, it throws an `AggregateError`. If only the editor fails, the error carries the script's exit status and stderr, with the editor error as `cause`. If only the script fails, its own error propagates. Beyond #141, the history and goals tests no longer rely on a fixed 3 s editor window. The editor holds the edited database's write lock and polls the destination state database with a `timeout: 0` `BEGIN IMMEDIATE`. A busy result means the script's final re-check holds the state lock, so the comparison is over and the editor commits. In 82 traced runs (up to 16 in parallel), the probe always fired after the script took the state lock and before it took the edited database's lock. Removing the re-check, reading digests before locking, and using a plain `BEGIN` in the script each make these tests fail. The three callers passed 10 of 10 runs, and 5 of 5 under 32 CPU hogs. The state test keeps a bounded 15 s wait (`STATE_EDIT_WINDOW_MS`, under the script's 30 s lock wait). State is the first lock, so nothing is observable there, and polling for copied rows fails an earlier verification. The suite is about 12 s slower. A deterministic wait would need a pause hook in the script, which this PR leaves out. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
`moveWhileDestinationIsEdited` could throw before it awaited the script, and its callers then
removed the fixture under a live script. It now waits for the editor and the script together and
reports both errors when both fail.

The editor also held its edit for a fixed 3 s, which failed under heavy load. For the history and
goals databases the edit now commits once the script holds the state database's write lock. The
script takes the destination locks in order, state first, so that lock proves its comparison is
over while it waits on the edited database. The state database is the first lock, so a script
waiting on it holds nothing to observe. That edit waits out a 15 s window, below the script's 30 s
lock wait.

Closes #141

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test: move-codex-session editor helper should report the script's result and take an explicit wait
All checks were successful
commit-msg / commitlint (pull_request) Successful in 19s
Details
Node tests / node:test (pull_request) Successful in 3m22s
Details
Review / Review (pull_request_target) Successful in 4m4s
Details
2f54a7801a
 
When the editor failed but the script had run, the thrown error dropped the script's exit status and
stderr, so a fault run did not say the move had completed. The error now carries both and keeps the
editor's error as its `cause`.

The helper also chose between waiting for the state lock and waiting out a window by comparing the
edited path with the state database path. The caller now picks: `moveWhileStateIsEdited` for the
first-locked state database and `moveWhileDestinationIsEdited` for the databases locked after it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
forgejo-actions commented 2026-10-10 07:37:22 +00:00
Copy link

Review 01M4JC2KV7J7ENY61TKM3DP5X5 — head 3a20e5f00b34e1315e5d30c6e5225035f766c863

Review — j4k-oss/agent-skills @ aaa8e581a8

Scope: diff against base tree e33d8035a418
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 (0)

No findings survived.

Reviewed:

  • general-bug (whole/default): no-claims
  • writing-quality (whole/default): no-claims
  • test-trimming (whole/default): no-claims
  • restated-sets (whole/default): no-claims
  • project-docs (whole/default): no-claims

Other claims

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

Coverage

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

lens part arm unit status runs loss
general-bug whole default no-claims 1 no
writing-quality whole default no-claims 1 no
test-trimming whole default no-claims 1 no
restated-sets whole default no-claims 1 no
project-docs whole default no-claims 1 no
<!-- review:summary --> **Review** `01M4JC2KV7J7ENY61TKM3DP5X5` — head `3a20e5f00b34e1315e5d30c6e5225035f766c863` # Review — j4k-oss/agent-skills @ aaa8e581a81e Scope: diff against base tree `e33d8035a418` 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 (0) No findings survived. Reviewed: - general-bug (whole/default): no-claims - writing-quality (whole/default): no-claims - test-trimming (whole/default): no-claims - restated-sets (whole/default): no-claims - project-docs (whole/default): no-claims ## Other claims - grounding-pending (0) - ungrounded (0) - rejected (0) - duplicate-of (0) - unadjudicated (0) ## Coverage Coverage pass: 01M4JC2Z39FFK7RVRXXQ6G5HR4 Accounting: complete Slot health: healthy | lens | part | arm | unit status | runs | loss | | --- | --- | --- | --- | --- | --- | | general-bug | whole | default | no-claims | 1 | no | | writing-quality | whole | default | no-claims | 1 | no | | test-trimming | whole | default | no-claims | 1 | no | | restated-sets | whole | default | no-claims | 1 | no | | project-docs | whole | default | no-claims | 1 | no |
🚀 1
forgejo-actions reviewed 2026-10-10 07:37:22 +00:00
skills/move-codex-session/scripts/move-codex-session.test.ts Outdated
@ -480,0 +497,4 @@
// The state database is the first lock, so a script waiting on it holds nothing to observe; the edit waits out a window.
function moveWhileStateIsEdited(fixture: Fixture, statement: string): Promise<MoveResult> {
return moveWhileEditing(fixture, fixture.destinationStatePath, statement, async (scriptHasExited) => {
const windowEnd = Date.now() + STATE_EDIT_WINDOW_MS;
forgejo-actions commented 2026-10-10 07:37:22 +00:00
Copy link

low — The state deletion race test relies on an arbitrary 15-second window

On a slow or heavily loaded runner, this test can commit its deletion before the mover reaches the destination comparison, so it no longer exercises the claimed after-comparison race and can fail with a different result. It also holds the mover behind the state lock for up to 15 seconds on every normal run of this test.

moveWhileStateIsEdited starts the window after the destination index contains the moved ID. In the mover, index publication is followed by rollout and database rechecks before deleteSourceDatabases; that function compares destination rows before it reaches the final BEGIN IMMEDIATE on destination databases. Because the test itself owns the state write lock, it cannot observe that final state-lock attempt and instead commits when this timer expires. If the mover has not reached the comparison by then, it sees the deleted rows as its initial comparison state, so the expected changed during source deletion diagnostic is not guaranteed.

Use an observable synchronization point for the state case, or make the test explicitly detect and fail if it cannot establish that the edit landed after the comparison. I traced moveWhileStateIsEdited, editDestinationBeforeRecheck, and deleteSourceDatabases plus the call sequence after index publication in move-codex-session.ts. I attempted the focused Node test; it could not reach the race because this environment lacks lsof (spawnSync lsof ENOENT). Thus the timing failure is a static consequence of the synchronization, not an observed flaky run. A deterministic signal that the mover completed its comparison while the editor still held the lock would refute this concern.

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

<!-- review:claim:01M4JBRRGQV1NXA6SD25A537DC --> **low** — The state deletion race test relies on an arbitrary 15-second window > On a slow or heavily loaded runner, this test can commit its deletion before the mover reaches the destination comparison, so it no longer exercises the claimed after-comparison race and can fail with a different result. It also holds the mover behind the state lock for up to 15 seconds on every normal run of this test. > > `moveWhileStateIsEdited` starts the window after the destination index contains the moved ID. In the mover, index publication is followed by rollout and database rechecks before `deleteSourceDatabases`; that function compares destination rows before it reaches the final `BEGIN IMMEDIATE` on destination databases. Because the test itself owns the state write lock, it cannot observe that final state-lock attempt and instead commits when this timer expires. If the mover has not reached the comparison by then, it sees the deleted rows as its initial comparison state, so the expected `changed during source deletion` diagnostic is not guaranteed. > > Use an observable synchronization point for the state case, or make the test explicitly detect and fail if it cannot establish that the edit landed after the comparison. I traced `moveWhileStateIsEdited`, `editDestinationBeforeRecheck`, and `deleteSourceDatabases` plus the call sequence after index publication in `move-codex-session.ts`. I attempted the focused Node test; it could not reach the race because this environment lacks `lsof` (`spawnSync lsof ENOENT`). Thus the timing failure is a static consequence of the synchronization, not an observed flaky run. A deterministic signal that the mover completed its comparison while the editor still held the lock would refute this concern. lens `general-bug` · arm `default` · tally 1 valid / 0 invalid / 0 uncertain claim `01M4JBRRGQV1NXA6SD25A537DC` of review `01M4JBP5GFGV3DP31BC6RBFQBV`
🚀 1
forgejo-actions commented 2026-10-10 07:41:54 +00:00
Copy link

superseded by review 01M4JC2KV7J7ENY61TKM3DP5X5 for head 3a20e5f00b34e1315e5d30c6e5225035f766c863

<!-- review:superseded:01M4JC2KV7J7ENY61TKM3DP5X5 --> superseded by review `01M4JC2KV7J7ENY61TKM3DP5X5` for head `3a20e5f00b34e1315e5d30c6e5225035f766c863`
🚀 1
Ghost marked this conversation as resolved
jercik force-pushed test/move-codex-session-editor-helper from 2f54a7801a
All checks were successful
commit-msg / commitlint (pull_request) Successful in 19s
Details
Node tests / node:test (pull_request) Successful in 3m22s
Details
Review / Review (pull_request_target) Successful in 4m4s
Details
to 3a20e5f00b
All checks were successful
commit-msg / commitlint (pull_request) Successful in 13s
Details
Review / Review (pull_request_target) Successful in 1m48s
Details
Node tests / node:test (pull_request) Successful in 3m41s
Details
2026-10-10 07:40:07 +00:00 Compare
jercik merged commit 7307509a82 into main 2026-10-10 07:45:03 +00:00
jercik deleted branch test/move-codex-session-editor-helper 2026-10-10 07:45:03 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
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!146
No description provided.