Repository navigation
fix(session): restore the adoption, accessor, and seam contracts #6377 over-tightened #6428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e269c6b
5d21272
0b93998
75c3f6d
d87483a
833d590
7b3a4b3
d5972b6
03254ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ### Fixed | ||
| - Preserve explicit persistence identity in cross-manager session adoption (`restoreState`) even when snapshots are copied through documented caller-adjusted paths (spread, JSON round-trip, structuredClone); reconstruct identity from sessionFile for explicit-storage sessions to enable stale file checks. | ||
| - Reject copied snapshots whose serialized adopted artifact manager is no longer a live manager, preventing restore from installing an unusable plain object. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2143,11 +2143,22 @@ export interface ResumeSessionIdentity { | |
| sha256: string; | ||
| } | ||
|
|
||
| /** Descriptor-bound bounded fingerprint for private explicit-session rollback authority. */ | ||
| type ExplicitPersistIdentity = Pick< | ||
| SessionStorageStat, | ||
| "dev" | "ino" | "nlink" | "size" | "mtimeMs" | "mtimeNs" | "ctimeNs" | ||
| > & { canonicalPath: string; sessionId: string; fingerprintSha256: string }; | ||
| /** Descriptor-bound bounded fingerprint for private explicit-session rollback authority. | ||
| * JSON-safe: all bigint fields are stored as strings to survive JSON serialization, | ||
| * spread, structuredClone, and JSON round-trip. */ | ||
| export type ExplicitPersistIdentity = { | ||
| // All bigint fields stored as base-10 strings for JSON safety | ||
| dev: string; | ||
| ino: string; | ||
| nlink?: string; | ||
| size: number; | ||
| mtimeMs: number; | ||
| mtimeNs: string; | ||
| ctimeNs?: string; | ||
| canonicalPath: string; | ||
| sessionId: string; | ||
| fingerprintSha256: string; | ||
| }; | ||
|
|
||
| export interface ResumeTailResumable { | ||
| kind: "resumable"; | ||
|
|
@@ -7315,6 +7326,8 @@ interface SessionManagerStateSnapshot { | |
| materializedFileEntries: readonly FileEntry[]; | ||
| adoptedArtifactManager: ArtifactManager | null; | ||
| coldRestoreFile?: string; | ||
| /** Explicit persistence identity for snapshot serialization safety. Preserved across cross-manager adoptions. */ | ||
| readonly explicitPersistIdentity?: ExplicitPersistIdentity; | ||
| } | ||
|
|
||
| /** Benchmark-derived cap for strong materialized session snapshots. */ | ||
|
|
@@ -8332,6 +8345,16 @@ export class SessionManager { | |
| prepared.releaseReferences(); | ||
| } | ||
|
|
||
| /** | ||
| * Adopt the resident store of a fully prepared rollback candidate. The candidate | ||
| * owns the store it built for the restored lifecycle, so the swap happens here, | ||
| * inside the resident-store seams, instead of at the call site. | ||
| */ | ||
| #installRollbackCandidateResidentStore(candidate: SessionManager): void { | ||
| this.#residentTextBlobStore = candidate.#residentTextBlobStore; | ||
| candidate.#residentTextBlobStore = new MemoryBlobStore(); | ||
| } | ||
|
|
||
| #releaseResidentTextStore(): void { | ||
| const predecessor = this.#residentTextBlobStore; | ||
| this.#residentTextBlobStore = new MemoryBlobStore(); | ||
|
|
@@ -8441,6 +8464,18 @@ export class SessionManager { | |
| : undefined; | ||
| if (explicitPersistIdentity && explicitPersistIdentity.sessionId !== snapshot.sessionId) | ||
| throw new Error("Session rollback persistence identity is unavailable."); | ||
| // Store the explicit identity as an enumerable property on the snapshot itself | ||
| // so that it survives documented caller-adjusted copies (spread, JSON round-trip, | ||
| // structuredClone, etc.) and cross-manager adoptions can access the captured | ||
| // identity instead of reconstructing it from the current file state. | ||
| if (explicitPersistIdentity) { | ||
| Object.defineProperty(snapshot, "explicitPersistIdentity", { | ||
| value: Object.freeze({ ...explicitPersistIdentity }), | ||
| enumerable: true, | ||
| configurable: false, | ||
| writable: false, | ||
|
Comment on lines
+8472
to
+8476
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an explicit-storage caller uses the documented caller-adjusted-copy path, such as Useful? React with 👍 / 👎. |
||
| }); | ||
| } | ||
| this.#stateSnapshots.set( | ||
| snapshot, | ||
| Object.freeze({ | ||
|
|
@@ -8458,6 +8493,54 @@ export class SessionManager { | |
| return snapshot; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve the state source for an explicit adoption (`restoreState`). Adoption is | ||
| * caller-driven state, not rollback authority: a snapshot may come from another | ||
| * manager or from a caller-adjusted copy of one, so those are adopted as given and | ||
| * constrained only by the live-state assertions inside `restoreState`. A snapshot | ||
| * issued by this manager resolves to its frozen issuer copy, which carries the | ||
| * explicit persistence identity captured at issuance. The rollback lane | ||
| * (`restoreRollbackState`) keeps requiring an authenticated issuance. | ||
| */ | ||
| #resolveAdoptedStateSnapshot( | ||
| snapshot: SessionManagerStateSnapshot, | ||
| ): Readonly<SessionManagerStateSnapshot> & { readonly explicitPersistIdentity?: ExplicitPersistIdentity } { | ||
| const issued = this.#stateSnapshots.get(snapshot); | ||
| if (issued) return issued; | ||
| if (snapshot.adoptedArtifactManager !== null && !(snapshot.adoptedArtifactManager instanceof ArtifactManager)) { | ||
| throw new Error("Session rollback adopted artifact manager is not live."); | ||
| } | ||
| // Preserve explicit identity from cross-manager adoptions. The identity is now | ||
| // stored as an enumerable property on the snapshot so it survives documented | ||
| // caller-adjusted copies (spread, JSON round-trip, structuredClone, etc.). | ||
| // Adoption must restore the identity captured in the snapshot, not the current | ||
| // state of the sessionFile, to ensure stale file checks use the captured identity. | ||
| let explicit = snapshot.explicitPersistIdentity; | ||
| // Only attempt reconstruction for explicit-storage sessions that don't already | ||
| // have an explicit identity (e.g., snapshots captured before this change). | ||
| if (!explicit && snapshot.sessionFile && !snapshot.managedPersistExpectedIdentity) { | ||
| try { | ||
| explicit = this.#captureExplicitPersistIdentity(snapshot.sessionFile); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a copied snapshot is adopted after its explicit transcript was replaced, this captures the replacement's identity rather than the identity at snapshot issuance, so the later persistence check merely compares the current file with itself. If the replacement has another session ID, lines 8509–8516 even discard that detected mismatch, causing validation to become a no-op and allowing subsequent persistence to mix the old snapshot's entries into the replacement transcript. Fresh evidence in this revision is this reconstruction-at-restore behavior; the added tests never change the transcript after capture, so they would pass without any stale-file protection. Carry issuance-time identity through a copy-stable representation, or fail closed when it is unavailable, rather than re-baselining it here. AGENTS.md reference: AGENTS.md:L171-L173 Useful? React with 👍 / 👎. |
||
| // Verify the reconstructed identity matches the snapshot's sessionId | ||
| if (explicit.sessionId !== snapshot.sessionId) { | ||
| // Session ID mismatch indicates the snapshot is for a different session | ||
| explicit = undefined; | ||
| } | ||
| } catch { | ||
| // If reconstruction fails (e.g., storage doesn't support readRangeSync), | ||
| // continue without it; validation will handle missing identity | ||
| explicit = undefined; | ||
| } | ||
| } | ||
| if (explicit) { | ||
| return Object.freeze({ | ||
| ...snapshot, | ||
| explicitPersistIdentity: explicit, | ||
|
Comment on lines
+8536
to
+8538
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When the captured state has a non-null AGENTS.md reference: AGENTS.md:L171-L171 Useful? React with 👍 / 👎. |
||
| }) as Readonly<SessionManagerStateSnapshot> & { readonly explicitPersistIdentity?: ExplicitPersistIdentity }; | ||
| } | ||
| return snapshot; | ||
| } | ||
|
|
||
| #authenticateStateSnapshot(snapshot: SessionManagerStateSnapshot): Readonly<SessionManagerStateSnapshot> { | ||
| this.#assertArtifactOpen(); | ||
| const issued = this.#stateSnapshots.get(snapshot); | ||
|
|
@@ -8621,8 +8704,7 @@ export class SessionManager { | |
| this.#titleSource = issued.titleSource; | ||
| this.#sessionFile = issued.coldRestoreFile; | ||
| this.#fileEntries = candidate.#fileEntries; | ||
| this.#residentTextBlobStore = candidate.#residentTextBlobStore; | ||
| candidate.#residentTextBlobStore = new MemoryBlobStore(); | ||
| this.#installRollbackCandidateResidentStore(candidate); | ||
| this.#byId = candidate.#byId; | ||
| this.#labelsById = candidate.#labelsById; | ||
| this.#leafId = candidate.#leafId; | ||
|
|
@@ -8708,7 +8790,8 @@ export class SessionManager { | |
| } | ||
|
|
||
| restoreState(snapshot: SessionManagerStateSnapshot): void { | ||
| const issued = this.#authenticateStateSnapshot(snapshot); | ||
| this.#assertArtifactOpen(); | ||
| const issued = this.#resolveAdoptedStateSnapshot(snapshot); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Regression test added: "rejects restoreState during strict close" — There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an explicit-storage snapshot is captured by one manager and restored into another—the cross-manager contract this change restores—the target's Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Regression test added: "preserves explicit persist identity across cross-manager adoption" —
Comment on lines
8792
to
+8794
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This changes externally observable coding-agent behavior for state adoption and artifact access, but the commit adds no AGENTS.md reference: AGENTS.md:L202-L202 Useful? React with 👍 / 👎. |
||
| if (issued.coldRestoreFile) throw new Error("Cold rollback requires restoreRollbackState."); | ||
| const managedTransition = | ||
| this.destination.kind === "managed" && issued.sessionFile | ||
|
|
@@ -16248,13 +16331,14 @@ export class SessionManager { | |
| return { | ||
| canonicalPath, | ||
| sessionId, | ||
| dev: before.dev, | ||
| ino: before.ino, | ||
| nlink, | ||
| // Convert bigints to strings for JSON safety | ||
| dev: String(before.dev), | ||
| ino: String(before.ino), | ||
| nlink: nlink !== undefined ? String(nlink) : undefined, | ||
| size: before.size, | ||
| mtimeMs: before.mtimeMs, | ||
| mtimeNs: before.mtimeNs, | ||
| ctimeNs: before.ctimeNs, | ||
| mtimeNs: String(before.mtimeNs), | ||
| ctimeNs: before.ctimeNs !== undefined ? String(before.ctimeNs) : undefined, | ||
| fingerprintSha256: fingerprint.digest("hex"), | ||
| }; | ||
| } | ||
|
|
@@ -17706,10 +17790,14 @@ export class SessionManager { | |
| * one bound to the current session file unless an external manager was | ||
| * adopted via `adoptArtifactManager`. Falls back to the lazily created | ||
| * ephemeral filesystem store once a non-persistent session has saved an | ||
| * artifact, so `artifact://` stays resolvable. Returns null only when no | ||
| * store has been established yet. | ||
| * artifact, so `artifact://` stays resolvable. Returns null when no store has | ||
| * been established yet and once the manager has released its artifact | ||
| * authority while closing — released authority stays observable as absence | ||
| * (matching `isArtifactManagerAuthorized`), while artifact *operations* are | ||
| * fenced by `#assertArtifactOpen()` and keep throwing. | ||
| */ | ||
| getArtifactManager(): ArtifactManager | null { | ||
| if (this.#artifactClosing || this.#strictClosePending) return null; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed: Regression test added: "throws during getArtifactPath after teardown starts" — |
||
| return this.#getOrCreateArtifactManager() ?? this.#ephemeralArtifactManager; | ||
| } | ||
|
|
||
|
|
@@ -17906,6 +17994,7 @@ export class SessionManager { | |
| * Returns null when the artifact is missing. | ||
| */ | ||
| async getArtifactPath(id: string): Promise<string | null> { | ||
| this.#assertArtifactOpen(); | ||
| const manager = this.getArtifactManager(); | ||
| if (!manager) return null; | ||
| return manager.getPath(id); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an ensured explicit-file session,
ExplicitPersistIdentitycontains severalbigintfields (dev,ino,nlink,mtimeNs, andctimeNs). Making this property enumerable causesJSON.stringify(session.captureState())to visit those fields and throw aTypeError, so the JSON round-trip adoption path promised by this change cannot produce a copy at all. Encode the transported identity in a JSON-safe representation and revive it before comparison, and add a regression for this parsing boundary rather than testing only object spread.AGENTS.md reference: AGENTS.md:L169-L173
Useful? React with 👍 / 👎.