Skip to content

fix: guard repair cleanup and capture ownership - #3142

Closed
thymikee wants to merge 2 commits into
refactor/session-snapshot-lifetimesfrom
fix/session-ownership-review
Closed

thymikee wants to merge 2 commits into
refactor/session-snapshot-lifetimesfrom
fix/session-ownership-review

Conversation

@thymikee

@thymikee thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Repair tombstone reads and removal now verify the stored address, including when two names sanitize to the same artifact directory. Malformed evidence stays intact; expired owned markers can be removed. Test publication also respects an explicit scoped address.

Capture controls distinguish a different handle under the same token and generation. Record-only capture publishes a new draft record through its declared owner. Legacy cutover tests wait for a complete, parseable verdict rather than file existence.

10 files; 126 gross lines. Addresses review findings on #3132, #3138 and #3140. Part of #3116, stacked on #3141.

Validation

Head 958983efee: 97 focused tests passed; quick checks and parent-scoped Fallow passed. Seven fault-injection runs failed at the intended assertions, including actual child-process partial publication. Restored focused tests pass. The ownership gate failed before declaring the real draft construction owner and passed afterward.

Independent read-only audit found no actionable findings. pnpm check:affected --base refactor/session-snapshot-lifetimes --run passed on this head, including 2,797 related tests and all selected local gates. CI and live device validation remain pending.

Review in cubic

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.96 MB 4.96 MB +106 B
Package (unpacked) 4.96 MB 4.96 MB +106 B
Package (download) 1.49 MB 1.49 MB +38 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.7 ms 26.2 ms -0.4 ms
CLI --help 83.3 ms 80.0 ms -3.3 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 10 files

Re-trigger cubic

@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

The scoped-session tombstone read at src/daemon/request-router.ts:619 does not match where the tombstone is written, so the cleanup guard misses the case it was meant to fix. I reviewed 958983e. repairExpiredIfTombstoned reads sessionStore.readRepairTombstone(req.session) under the raw request name. For an implicit session with meta.cwd (stored as cwd:<hash>:default) or a tenant-scoped one (<tenant>:<name>), the reaper writes the tombstone in the scoped address's directory with owner = ref.address (session-store.ts:338-346). The router looks in the raw name's directory instead, and with the new owner check it would refuse even a file it found. The owner check fixes sanitized-directory collisions, which is a different problem, so this mismatch is unchanged. No router test publishes at a scoped address. A CLI user whose cwd-scoped or tenant-scoped repair session was reaped, or whose commit failed at teardown, still gets a bare SESSION_NOT_FOUND instead of REPAIR_SESSION_EXPIRED or REPAIR_COMMIT_FAILED with re-run guidance. This is the #3140 finding this PR says it addresses. The rule should be that every error-path marker read, repair and idle, uses the address the request itself resolved to. Could you pull the scopeRequestSession plus resolveEffectiveSessionName({ attachesToSession: false }) block out of readIdleExpiryTombstoneSafely into one total helper, and call it from both repairExpiredIfTombstoned and idleExpiredIfTombstoned? Then please add a test in request-router-repair-expired.test.ts that publishes at cwd:<hash>:default, writes the tombstone, retires the session, sends close with session default and meta.cwd, and expects REPAIR_SESSION_EXPIRED, plus a tenant-scoped variant. Both should fail on the current head.

Not blocking, take or leave: findUnrecoveredRepairCommitFailure in src/session-repair-tombstone.ts:131 still reports entry.name (the sanitized directory) as sessionName when tombstone.owner is now authoritative; the test "test session publication uses its explicit scoped address" only exercises a test helper; R68 catches property construction but not member assignment such as session.screenRecording = x; and no behavior test asserts the caller's draft stays unmutated after record-only adoption.

I did not rerun the tests or the fault injection from the PR body, so the regression conclusions come from reading the code before the change. Repo Guards, Coverage and Integration Tests now pass on 958983e. Smoke Tests is still running, and a failure there could not be treated as unrelated, because it overlaps the record-only recording publication in record-runtime.ts:147. There are no conflicts. Before merge, repairExpiredIfTombstoned must read the tombstone under the request's resolved scoped address, with the scoped router test passing.

@thymikee
thymikee added this pull request to stack #3146 October 3, 2026 08:50
@thymikee
thymikee force-pushed the refactor/session-snapshot-lifetimes branch from 1c3da4c to ede3e35 Compare October 3, 2026 14:41
@thymikee
thymikee force-pushed the fix/session-ownership-review branch 4 times, most recently from f095e6e to 995874e Compare October 3, 2026 16:56
@thymikee
thymikee force-pushed the refactor/session-snapshot-lifetimes branch from 393dcb8 to 672583f Compare October 3, 2026 16:56
@thymikee
thymikee force-pushed the fix/session-ownership-review branch from 995874e to 1a6fe40 Compare October 3, 2026 17:49
@thymikee
thymikee force-pushed the refactor/session-snapshot-lifetimes branch from 672583f to 0cc39c1 Compare October 3, 2026 17:49
@thymikee
thymikee removed this pull request from stack #3146 October 3, 2026 19:38
@thymikee
thymikee force-pushed the refactor/session-snapshot-lifetimes branch from 0cc39c1 to e4ca3d1 Compare October 3, 2026 19:39
@thymikee
thymikee force-pushed the fix/session-ownership-review branch from 1a6fe40 to 6c3e88b Compare October 3, 2026 19:39
@thymikee
thymikee added this pull request to stack #3187 October 3, 2026 19:45
@thymikee
thymikee removed this pull request from stack #3187 October 3, 2026 20:59
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Consolidated into #3140 as part of reducing #3116 to seven PRs. Session/repair changes are in #3140; timeout-result corrections are in #3127. The composition preserves the complete pre-consolidation source tree, including tests and later review corrections. This PR is superseded; its review discussion and native evidence remain available. Outstanding findings transfer to the owning keeper in the implementation record.

@thymikee thymikee closed this Oct 3, 2026
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-03 21:16 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant