feat(storage): define quiescent session snapshot boundary - #2968
feat(storage): define quiescent session snapshot boundary#2968MicroGery wants to merge 2 commits into
Conversation
|
I reviewed this from three angles: what problem this slice solves, why the approach can solve it, and whether the problem is minimally/correctly defined. The core problem is real and correctly identified: However, this PR currently implements a coordinator framework rather than solving #2369 end to end: there is no production quiescence authority, state exporter, workspace copier, call site, codec round trip, or real writer-race test yet. CI validates the self-contained contract/fakes, not the production guarantee. I think this should be described explicitly as a foundation slice. I also see two blocking correctness/security gaps:
There is also a policy-definition inconsistency: From an Occam’s razor perspective, I would define the minimum problem as:
Then keep these as separate concerns:
So the architectural direction is good, but I would not treat the issue as solved yet. The main adjustment is to align the claimed trust/security boundaries with what the code can actually enforce, and avoid combining consistency, portability, secret governance, and adversarial cleanup into one oversized problem definition. |
📝 WalkthroughWhat problem this PR solvesThis PR defines the foundation for quiescent Session Bundle snapshots. It adds:
The PR has no production call site. Production quiescence and workspace/state adapters, codec integration, and mutation-race end-to-end coverage remain planned for PR 2. Source-of-truth relationshipThis PR extends the storage package with a new public coordinator API. It does not replace an existing snapshot path or add a production call site. The API is exported from Scope and complexityThe solution is coherent for the contract and foundation slice. The added complexity supports required guarantees for synchronization, cancellation, staging ownership, filesystem identity checks, cleanup, and platform privacy validation. The implementation remains incomplete until PR 2 supplies trusted concrete preparers and production quiescence integration. The following areas should be simplified or separated where possible:
Risks and validationThe coordinator trusts the injected workspace preparer to enforce the workspace policy. The coordinator validates returned roots and counters, but it cannot independently prove that the preparer applied all policy rules. Cleanup still has a race between path verification and recursive removal. An attacker or same-principal process may replace the path after verification. The current tests cover shared quiescence, serialization, concurrency, cancellation, deadlines, cleanup retries, staging ownership, root verification, privacy requirements, policy behavior, and stable errors. They do not fully cover the cleanup replacement interval or production mutation races. The provided summary reports passing storage tests, typecheck, formatting, lint, and diff checks. Direct evidence for those results is not available here, so the final status of required checks is unverified. Review-relevant risks
The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughThis change adds a file-backed quiescent session snapshot coordinator. It enforces workspace and staging-root policies, coordinates preparation and cancellation, publishes owned snapshots, verifies filesystem identities, supports retryable release, and exports the new API. ChangesQuiescent Session Snapshot
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: ⚪ Minimal · up to This PR adds foundation contracts without a production call site. The remaining items are localized cleanup, test-strengthening, and future operational follow-up, so no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant SnapshotCaller
participant SessionSnapshotCoordinator
participant QuiescenceAuthority
participant StateWorkspacePreparer
participant FilesystemStaging
SnapshotCaller->>SessionSnapshotCoordinator: request snapshot
SessionSnapshotCoordinator->>QuiescenceAuthority: acquire session quiescence
SessionSnapshotCoordinator->>StateWorkspacePreparer: prepare state and workspace
StateWorkspacePreparer-->>SessionSnapshotCoordinator: preparation result
SessionSnapshotCoordinator->>FilesystemStaging: publish owned staging directory
SessionSnapshotCoordinator->>QuiescenceAuthority: release session quiescence
SessionSnapshotCoordinator-->>SnapshotCaller: return bundle handle
SnapshotCaller->>SessionSnapshotCoordinator: release bundle
SessionSnapshotCoordinator->>FilesystemStaging: verify and remove staging data
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/storage/src/quiescent-session-snapshot.ts (3)
655-673: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelete the dead
.awsand.cargobranches.Line 70 already matches the bare basename
credentialsthrough/^(?:credentials?|secrets?)(?:\..*)?$/i.isKnownSecretPaththerefore returnstruebefore reaching lines 666-667. The.docker,.kube, andgcloudbranches stay reachable because their basenames are not matched by any pattern.The matching test cases at
packages/storage/src/__tests__/quiescent-session-snapshot.test.tslines 567-568 assert the generic pattern, not these branches.♻️ Proposed simplification
return ( (lowerSegments.at(-2) === '.docker' && lowerName === 'config.json') || - (lowerSegments.at(-2) === '.aws' && lowerName === 'credentials') || - (lowerSegments.at(-2) === '.cargo' && lowerName === 'credentials') || (lowerSegments.at(-2) === '.kube' && lowerName === 'config') ||As per path instructions: "Flag concrete cases where code can be deleted or simplified."
Source: Path instructions
1015-1039: 🩺 Stability & Availability | 🔵 TrivialPlan orphan recovery for a lost owner record.
Line 1021 requires a valid owner record before any removal, and no other path removes a published root. If the owner file is deleted or corrupted,
release()fails on every retry and the snapshot root keeps state and workspace copies indefinitely.Consider a reaper or a documented operator procedure for staging roots whose owner record cannot be verified, plus a metric for staging entries older than a bound.
373-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn the cleanup promise from the catch block.
cleanupAfterPreparationFailurealways throws. Withoutreturn, TypeScript infers the callback as returningOwnedPreparedSessionBundleHandle | undefined, which widensrunQuiescent<T>.♻️ Proposed change
} catch (error) { - await cleanupAfterPreparationFailure(staging, error); + return await cleanupAfterPreparationFailure(staging, error); }packages/storage/src/__tests__/quiescent-session-snapshot.test.ts (1)
92-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese assertions cannot fail, so the quiescence claim stays untested.
The fake preparers at lines 56-75 write the JS variables
liveStateandliveWorkspaceinto the staging roots. Reassigning those variables at lines 92-93 cannot change already written files, and lines 83-90 already assert the same two reads. The test name promises isolation from live writers, but no live source is read after preparation.Write the content into
fixture.liveStateRootandfixture.liveWorkspaceRoot, copy from those roots inside the preparers, then mutate the live files afterprepareresolves. The assertions then protect the observable isolation guarantee.As per path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fd805a31-4648-4d3a-802a-754500c7ef52
📒 Files selected for processing (3)
packages/storage/src/__tests__/quiescent-session-snapshot.test.tspackages/storage/src/index.tspackages/storage/src/quiescent-session-snapshot.ts
|
Thanks for the work — the module is well-structured (contract + quiescence/lifetime abstraction + policy + deterministic fakes, matching the issue's approved two-PR split), the second commit correctly moved secret classification after the log exclusion and made Conclusion: FAIL — P1 blocks merge (the V1 policy is fail-open for the most common secret directory conventions), plus two P2s and P3s. P1 — P2-1 — the mirror-image false positive: common non-secret files ( P2-2 — 1977 lines with no production callers, and the ownership/inode/cleanup machinery belongs to PR 2 by the issue's own split (and is unverifiable here). #2369 explicitly splits PR 2 into "filesystem/state integration, cleanup, race tests, codec round-trip E2E" — this PR delivers PR 1 plus ~500 lines of filesystem integration (inode binding, owner record, cleanup protocol, Windows ACL) verified only against fakes. Also, the error-code union (222-232) declares P2-3 — the headline isolation test is tautological; "later live mutations cannot change the snapshot" is not actually tested. The fake preparer writes JS-variable values into staging files; the test then reassigns those JS variables — which cannot change the already-written files — and re-reads them. No live source is read after prepare, so the test would pass even if PR 2's coordinator exposed live paths. Make the fake copy from P3 (optional): the AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：FAIL（P1 阻塞合并）。模块结构好（契约+静默/生命周期抽象+策略+确定性 fake，符合 issue 批准的两 PR 拆分），第二 commit 已把 secret 判定移到 log 排除之后并让证书类改 include（修复 likun 点 3），协调器设计（逻辑时间点+私有 staging 副本后放行 live writers）贴合问题。P1：secrets/、credentials/、keys/、private/ 目录完全放行进快照——isKnownSecretPath 只做 basename 模式+.ssh 段+特定文件检查，无目录段语义检查；node 复刻 classify 实测 secrets/token→include、secrets/api-key.txt→include、credentials/foo.txt→include、目录项 secrets→include，而 .ssh/config 被拒——语义上更明确的 secrets/ 类目录整体放行是明显不对称，测试表（540-575 行）从没覆盖 secrets/ 目录；违反 PR 自身契约（"只拒绝已知用户自写机密材料的名称"——secrets/ 正是此类）与 #2369 验收（注入凭据必须排除、user-authored secret 需显式决策）。预测：最常见的 secrets/ 目录约定的 workspace 私密内容进 bundle 外泄（正是 cloud fork/归档要防的）。修复：isKnownSecretPath 增加 secrets/credentials/keys/private 目录段（与 .ssh 对称）+ 测试表补三个用例。P2-1（镜像误伤）：secrets.ts/.env.example/*.pub 公钥被拒导致整个快照 policy_rejected——/^(?:credentials?|secrets?).../ 匹配任意扩展名（secrets.ts/secrets.md 是普通源码/文档名）、.env.example 是标准提交模板、id_ 模式匹配 id_ed25519.pub 公钥，而 PR 自称"公钥证书编码不按扩展名拒"（certs/client.crt include）——普通 TS 项目快照整体被拒，功能对常见项目不可用。建议：机密模式限定常见机密扩展名或放行 .ts/.js/.md/.example/.pub。P2-2：1977 行全新增无生产调用点，ownership/inode/cleanup 机制按 issue 拆分属于 PR 2（且这里无法验证）；错误码联合类型声明的 snapshot_busy/session_not_quiescent/source_changed/quota_exceeded 本文件一个都没抛过——死契约面，PR 2 形状不匹配则 ~1200 行返工。建议显式标注为 PR 2 前置验证项或压缩到"契约+策略+极简协调器"。P2-3：头条隔离测试是重言式——fake preparer 写 JS 变量值进 staging 文件后重赋值 JS 变量不可能改变已写文件，无 live 源在 prepare 后被读，PR 2 真把 live 路径暴露给 pack 该测试也通过。修复：fake 从 fixture 根拷贝、prepare 后改 live 文件再断言不变。P3（可选）：.aws/.cargo 分支不可达（basename 已被通用模式命中）删之；catch 缺 return 加宽返回类型；孤儿 owner record 使 release() 永久失败（已声明另行机制，建议补 reaper）；inode→rm 末段窗口已声明接受。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The foundation is structurally sound: state/workspace preparation and publication occur inside one trusted quiescence callback, publication is an atomic rename, and inode-bound ownership makes release idempotent, retryable, and fail-closed against replacement. I found no P0/P1 implementation issue in this PR1 scope.
The primary isolation test does not yet prove its stated property, as detailed inline. The simplest useful proof is to make preparers copy real live roots, mutate those roots after the boundary, and assert the published snapshot remains the boundary version. A later production-integration PR must additionally exercise a real writer during the boundary and add owner-record/inode-based orphan recovery; those are production-readiness gates, not reasons to add speculative machinery to this foundation.
Review performed with three Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I verified the finding against the latest head and live green CI.
中文评论
foundation 的结构是可靠的：state/workspace prepare 与 publish 位于同一 trusted quiescence callback，publish 使用原子 rename，inode-bound ownership 让 release 具备幂等、可重试和 replacement fail-closed。PR1 范围内未发现 P0/P1 实现问题。
主 isolation test 尚未证明其声明属性，具体见行内。最简单有效的证明方式是让 preparer 从真实 live roots 复制，在 boundary 后修改这些 roots，再断言 published snapshot 保持 boundary 版本。后续生产接入 PR 还必须让真实 writer 在 boundary 内尝试写入，并增加基于 owner record + inode 的 orphan recovery；这些是 production readiness gate，不需要在当前 foundation 中预设复杂实现。
本次审查使用了三位 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已依据最新 head 和实时绿色 CI 复核问题。
| liveWorkspace, | ||
| ); | ||
|
|
||
| liveState = 'later-state'; |
There was a problem hiding this comment.
P2 — This does not exercise snapshot isolation. The preparers already copied the current JavaScript string values directly into staging at lines 60/73; reassigning those variables cannot mutate any live source, so the assertions would pass even if production read from the wrong root or outside the intended boundary. Seed fixture.liveStateRoot/liveWorkspaceRoot, make the preparers copy from them, then mutate the live files after prepare() and assert the snapshot retains the boundary contents.
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
15359f9 to
0482ee6
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Defining the quiescent boundary before adding production adapters is a sensible split, and pinning one policy object avoids a caller-controlled safety downgrade. The contract is not yet internally closed, though: the secret-name policy has both unsafe gaps and common false positives, and its accepted path language is wider than the existing V1 bundle codec. The simplest solution is to reuse one canonical portable-path predicate and express secret handling as a small reviewed set of exact path/name rules rather than increasingly broad filename regexes.
AI-assisted review disclosure: Codex verified these findings against the current head, the V1 bundle path contract, policy classifier, ownership/cleanup flow, and tests. Two independent reviewer-agent passes and an OpenCode Go DeepSeek V4 Flash (high) adversarial pass were used as inputs. No local tests were run.
中文复核
先定义 quiescent boundary、后接生产 adapter 的拆分是合理的，固定单一 policy 也避免调用方降低安全标准。但当前契约还没有闭合：secret 规则既漏掉常见私钥/凭据路径，也误伤常见公开或模板文件；同时它接受的路径集合比现有 V1 bundle codec 更宽。更简单的方案是复用唯一的 portable-path 判定，并用小而明确的路径/文件名规则表达 secret policy，避免继续扩张宽泛正则。
本次为 AI 辅助审查：Codex 在最新 head 上核验 V1 bundle 路径契约、policy classifier、ownership/cleanup 与测试；另使用两次独立 reviewer 及一次 OpenCode Go DeepSeek V4 Flash（high）对抗审查。未运行本地测试。
| /^\.git-credentials(?:\.lock)?$/i, | ||
| /^(?:credentials?|secrets?)(?:\..*)?$/i, | ||
| /(?:^|[-_.])(?:id_(?:rsa|dsa|ecdsa|ed25519)|private[-_.]?key)(?:$|[-_.])/i, | ||
| /\.(?:key|p12|pfx)$/i, |
There was a problem hiding this comment.
[P1] Cover known private-key and credential paths that this allowlist currently exports. Common names such as privkey.pem and private.pem do not match this pattern or the extension list, and nested paths such as credentials/oauth.json or private/token are included because only the basename is checked. A Session bundle prepared under this policy can therefore contain user credentials despite the contract claiming fail-closed rejection of known secret material. Add exact high-confidence names/directories (without banning every .pem, since certificates are public) and focused include/reject cases.
| // names that identify known secret material; public certificate encodings and | ||
| // other ambiguous formats are not rejected by extension alone. | ||
| const KNOWN_SECRET_WORKSPACE_FILE_PATTERNS = [ | ||
| /^\.env(?:\..*)?$/i, |
There was a problem hiding this comment.
[P2] Narrow the patterns so ordinary portable inputs are not rejected. ^\.env(?:\..*)?$ rejects .env.example/.env.template, and the id_ed25519 pattern also rejects id_ed25519.pub; both are commonly intentional project inputs and the latter is explicitly public. Prefer exact private-key names plus explicit template/public exceptions, and add regression cases for these two paths.
| ) { | ||
| return { kind: 'reject', category: 'unsafe_path' }; | ||
| } | ||
| const segments = entry.relativePath.split('/'); |
There was a problem hiding this comment.
[P2] Validate the same portable segment contract as the V1 bundle codec here. This decoder currently accepts paths such as workspace/CON, foo:bar, and name., but session-bundle-contract.ts deliberately rejects Windows device names, reserved characters, and trailing dots/spaces on every host. The preparer can therefore successfully publish a snapshot that the next pack stage must reject. Reuse one shared path predicate at the earlier policy boundary and test the codec's reserved-name matrix here.
| liveWorkspace, | ||
| ); | ||
|
|
||
| liveState = 'later-state'; |
There was a problem hiding this comment.
[P2] This does not exercise source isolation or a quiescence boundary: it only reassigns two JavaScript strings after the fake preparers have already written copies, while liveStateRoot and liveWorkspaceRoot are never read or mutated. The test would still pass if a future adapter accidentally read live files after quiescence. Either make this an honest coordinator-ordering test and remove the isolation claim, or mutate real source roots around a controlled release point and assert the staged bytes stay pinned.
|
/agentic_review |
Code Review by Qodo
1. Nonportable paths reach packing
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if (entry.kind === 'file' && isKnownSecretPath(lowerSegments, lowerName)) { | ||
| return { kind: 'reject', category: 'known_secret_file' }; | ||
| } | ||
| return INCLUDE; |
There was a problem hiding this comment.
1. Nonportable paths reach packing 🐞 Bug ≡ Correctness
The V1 “portable-workspace” policy includes names such as a:b, CON, and foo., although the downstream Session Bundle USTAR codec rejects them. A snapshot can therefore prepare successfully but deterministically fail when packed.
Agent Prompt
## Issue description
`SESSION_SNAPSHOT_WORKSPACE_POLICY_V1` includes filesystem paths that the Session Bundle USTAR codec later rejects, so a successfully prepared snapshot may not be packable.
## Issue Context
Consolidate portable-path authority by reusing or extracting the existing Session Bundle path-validation seam rather than duplicating its Windows and USTAR restrictions. Deletion cannot satisfy the policy contract, and no new public surface or state is needed; the existing `unsafe_path` rejection can represent these failures.
## Fix Focus Areas
- packages/storage/src/quiescent-session-snapshot.ts[630-653]
- packages/storage/src/session-bundle-ustar.ts[118-159]
- packages/storage/src/__tests__/quiescent-session-snapshot.test.ts[536-586]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /^\.(?:npmrc|netrc|pypirc|terraformrc)$/i, | ||
| /^\.git-credentials(?:\.lock)?$/i, | ||
| /^(?:credentials?|secrets?)(?:\..*)?$/i, | ||
| /(?:^|[-_.])(?:id_(?:rsa|dsa|ecdsa|ed25519)|private[-_.]?key)(?:$|[-_.])/i, |
There was a problem hiding this comment.
2. Public ssh keys rejected 🐞 Bug ≡ Correctness
The secret-name regex matches public SSH key files such as keys/id_ed25519.pub because . satisfies the boundary after id_ed25519. These non-secret files are rejected as known_secret_file, preventing otherwise valid workspaces from being snapshotted.
Agent Prompt
## Issue description
Public SSH key filenames such as `id_rsa.pub` and `id_ed25519.pub` match the private-key-name rejection pattern and are rejected as known secrets.
## Issue Context
Make the smallest local classification correction by exempting explicit public-key filenames before broad private-key and `.ssh` checks. Reusing the current classifier and rejection categories is sufficient; no new state, configuration, authority, or public API is needed.
## Fix Focus Areas
- packages/storage/src/quiescent-session-snapshot.ts[66-73]
- packages/storage/src/quiescent-session-snapshot.ts[655-673]
- packages/storage/src/__tests__/quiescent-session-snapshot.test.ts[536-586]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const timeout = | ||
| remaining === undefined || remaining <= 0 | ||
| ? undefined | ||
| : setTimeout(abort, Math.min(remaining, 2_147_483_647)); |
There was a problem hiding this comment.
3. Long deadlines expire early 🐞 Bug ☼ Reliability
Deadlines more than 2,147,483,647 milliseconds away are scheduled once at that maximum delay, so cancellation occurs about 24.8 days after preparation begins rather than at deadlineAt. This violates the absolute-deadline contract for otherwise valid safe-integer timestamps.
Agent Prompt
## Issue description
A deadline farther away than Node's maximum timer delay aborts at the timer limit instead of at the requested absolute timestamp.
## Issue Context
A single timer cannot represent the full supported deadline domain. Recompute the remaining duration when each bounded timer fires and abort only once the actual deadline is reached; this introduces only a private rescheduling branch and timer handle, with no new public surface or configuration.
## Fix Focus Areas
- packages/storage/src/quiescent-session-snapshot.ts[769-815]
- packages/storage/src/__tests__/quiescent-session-snapshot.test.ts[280-309]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Two reviewers went over this independently. Neither found a correctness defect, and I checked the disagreement-prone parts myself. This is careful work — the staging tri-state, the dev/ino owner binding before cleanup, the atomic publish rename, and the fail-closed release path all hold together, and the error codes are bounded rather than open-ended.
I want to flag one thing that is a decision rather than a defect, and one piece of housekeeping.
The merge premise. There is no production consumer today — quiescent-session-snapshot is referenced only by its own module and its tests. That is exactly what a contract slice looks like, and I'm not treating it as a defect, because #2369 names the follow-up concretely. But it does mean merging this is a bet that PR 2 lands. If it doesn't, 1227 lines of implementation become pure carrying cost, and a publish protocol change in PR 2 would rewrite much of it rather than build on it. Worth stating in the PR body so the premise is on the record rather than implied.
I looked hard at whether the policy pin is speculative generality and concluded it isn't: SESSION_SNAPSHOT_WORKSPACE_POLICY_V1 being non-replaceable protects a real invariant. A snapshot carries a workspace copy, so a caller that could weaken the policy could route around the secret exclusions. 'policy' in options → throw is the enforcement point, and that's a security boundary rather than a hypothetical extension seam.
One reviewer raised whether the coordinator could mistake a non-quiescent session for a quiescent one. It can't on its own account — and the interface doc already says so plainly ("this interface does not make a process-local mutex authoritative by itself; every real writer must already be governed by the supplied Host/Owner implementation"). Correctness is delegated, and the delegation is explicit. Noting it only because it's the load-bearing assumption for whoever implements the Authority in PR 2.
CI needs a fresh run. Details inline. Not approving on this head for that reason alone — I have no blocking findings on the code.
| * responsible for applying every decision and rejecting symlinks, hard links, | ||
| * special files, path races, case conflicts, and quota violations. | ||
| */ | ||
| export const SESSION_SNAPSHOT_WORKSPACE_POLICY_V1: SessionSnapshotWorkspacePolicy = Object.freeze({ |
There was a problem hiding this comment.
The checks on this head are from 2026-08-18 and can't speak for the current tree.
#3397 landed on 2026-08-22 and added ASF license headers across ~2685 files. Any run that started before that predates the gate — a green from 08-18 is a true result about a different repository.
There's also an e2e failure on this head. I attributed it: the failing case is slash-command-menu-compacts-the-active-session, a locator.click timeout in the desktop suite, 1 failed of 42. This PR adds three files under packages/storage and touches nothing in desktop, CLI, or the slash-command path, so the failure isn't yours. But a five-day-old failure that happens to be unrelated still isn't current evidence of anything.
A rebase onto current main and a fresh run would settle both at once.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head 0482ee65047bc4fb177b4ef415f46558c201faed. Verdict: NO-GO, 1×P1. The two-PR split itself is not a simplification finding: #2369 explicitly authorizes this contract/fake slice before the production adapters, and this coordinator is an orthogonal staging/lifetime layer rather than a second archive codec or state exporter.
CI attribution: the sole red check is the Desktop slash-command E2E timing out while clicking /compact because the root <html> intercepted pointer events and the option then detached. This PR adds an otherwise unconsumed Storage module plus its export/tests and does not touch that Desktop path; all affected build, typecheck, workspace, Runtime Host, package, and Storybook lanes passed. I therefore treat that red as unrelated/intermittent, not as a P-level PR regression. It still means the exact-head all-green approval gate is not met.
Local exact-head verification: dependency-order Core→Storage and full Desktop dependency/build chain passed; focused snapshot tests passed 17/17 with 1 platform skip; Biome and diff-check passed. Local Electron replay was unavailable because this runner lacks xvfb-run.
| ) { | ||
| return { kind: 'exclude', category: 'cache' }; | ||
| } | ||
| if ( |
There was a problem hiding this comment.
[P1] Reject secret-shaped files before the generic log exclusion. In this order, .env.log, secrets.log, credentials.log, and keys/private-key.log all return { kind: 'exclude', category: 'log' }, so snapshot creation succeeds and silently omits them; the added test even pins secrets.log to that outcome. That conflicts with #2369's maintainer decision that known user-authored secret files must fail the complete snapshot rather than silently change workspace semantics, and the bounded counters cannot tell the user which important file disappeared. Please make the known-secret rule win on overlaps (and cover at least .env.log/secrets.log) while retaining ordinary debug.log as an exclusion.
3 participants
Summary
Define the contract/foundation slice of the quiescent Session Bundle snapshot coordinator from #2369.
PreparedSessionBundleSnapshotThis is PR 1 of the implementation split described in #2369. It establishes contracts and deterministic fakes, but has no production call site and does not solve #2369 end to end. Production quiescence/state/workspace adapters, codec round-trip integration, and mutation-race E2E coverage remain for PR 2.
Refs #2369
Trust and security boundaries
SessionSnapshotQuiescenceAuthorityis a trusted Host/Owner boundary; this PR intentionally does not present a process-local mutex as authoritative.SESSION_SNAPSHOT_WORKSPACE_POLICY_V1.SessionSnapshotWorkspacePrepareris the trusted enforcement point: it must apply every decision without downgrading it. The coordinator validates the returned root and bounded counters, but does not re-traverse the result to attest policy compliance. PR 2 must provide and test the production preparer before that guarantee is claimed end to end.Remaining PR 2 integration
prepare -> pack -> inspect -> hydrateround-trip coverageVerification
npm test -w @maka/storage— 801 tests, 786 passed, 15 skipped, 0 failednpm run typecheck -w @maka/storagenpx biome check packages/storage/src/quiescent-session-snapshot.ts packages/storage/src/__tests__/quiescent-session-snapshot.test.ts packages/storage/src/index.tsgit diff --checkAI use
Select exactly one:
Tool(s) and scope: OpenAI Codex assisted with contract design, implementation, regression tests, review remediation, and local verification. The contributor reviewed the resulting changes and remains responsible for their accuracy, provenance, and licensing.
Checklist
Does this PR entail a change in behavior?