chore: add real session bundle measurement harness (#1336) - #1353
Conversation
|
This spike does not answer #1336 yet. The harness is reproducible, but the two measurements behind the decision record are not measuring the system the issue asks about. The bundle distribution comes entirely from The cold-start number has the same problem. There are two smaller correctness issues. Directory filtering only classifies paths by their first segment, so nested I would keep the harness as a synthetic smoke benchmark, but I would not close #1336 or turn these numbers into v1 limits yet. The spike needs sanitized real session exports, the target archive layout, and fresh-process activation through the closest real bootstrap with a fake backend. Until then, the document should call these candidate assumptions rather than measured decisions. |
|
Implemented and reviewed in commit c7e91dc.\n\n- Replaced synthetic state samples with real sanitized session-export inputs and a real POSIX tar.zst archive (manifest, state, workspace).\n- Added fresh-process bootstrap through the real SessionStore, RuntimeEventStore, Harbor cell, and FakeBackend path.\n- Recursively excludes nested .git and node_modules entries, supports UTF-8 paths, validates manifest/path safety, archive-entry equality, byte counts, and SHA-256 digests.\n- Added defense-in-depth JSON/JSONL redaction without replacing schema fields such as tokenUsage. Smoke-only runs are explicitly marked decisionReady=false and cannot be used as percentile evidence.\n- Removed the invalid synthetic decision numbers and stale external links from the decision record.\n\nValidation: npm run build:test passed; measurement tests pass (2/2); Biome and syntax checks pass. |
c7e91dc to
01fdc9c
Compare
|
Rebased onto current This revision addresses the measurement concerns without retaining the old synthetic conclusions:
The PR body and title now state the current evidence honestly: this supplies the real measurement harness, but does not claim release percentiles or close #1336 without a real sanitized session corpus. Validation: format, lint, full workspace build/typecheck, script suite 130/130, measurement tests 14/14, and the first-event timing regression all pass. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving because the remaining findings are non-blocking P2 follow-ups. One metadata issue cannot be attached inline: GitHub currently reports #1336 in this PR's closingIssuesReferences, despite the body saying that this harness does not close the issue. Please remove that closing linkage before merge so the issue remains open for the real-corpus measurement. I also left the timing-boundary issue inline.
Astro-Han
left a comment
There was a problem hiding this comment.
Five inline findings below. The P1 and timing-boundary findings affect whether a report can be treated as trustworthy decision evidence; the P3 covers archive write integrity.
dba10d1 to
779a968
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
I did not find a P0 or P1 issue, so I am approving this PR. I found these follow-ups:
- [P2] A workspace may contain its paired session export. The state is then archived once as workspace data and again under
state, while the report may still saydecisionReady=true. - [P2] The bootstrap sample does not prove a successful restore. It performs extra full reads before the production path and does not check whether the resumed invocation completed.
- [P2]
decisionReadydoes not qualify the runtime environment. A local Darwin/Node 26 run can satisfy the flag even though the decision document requires a controlled Node 24 image. - [P2] JSON and JSONL preparation uses several full-size in-memory copies. Concurrent workspace traversal also retains every entry list, so memory use is not bounded for the largest samples.
- [P2] The public
resumeSessionIdpath can run with a stored header that disagrees withinput.cwdorinput.config. - [P3] The script copies session-bundle policy constants and path rules from Storage, creating a second policy source.
- [P3] Tar creation does not detect a source file changing between stat and streaming.
- [P3] JSON sanitization pretty-prints compact production files, which biases the size measurement upward.
These do not need to block a measurement-only PR, but they should be resolved or documented before the harness is used to set production budgets.
Please push back if any input or environment above is intentionally unsupported. These comments should be checked against the intended measurement contract, not accepted by default.
|
Addressed the remaining review feedback in a53cf1d as one consolidated change.
Validation: CI |
|
Reviewed The earlier evidence gaps are closed. Overlapping roots are rejected, restore samples require a completed invocation, readiness checks the runtime environment, resume identity is validated before execution, and large inputs no longer require unbounded in-memory copies. CI was green. No P0–P3. |
Refs #1336
Summary
This keeps the timeboxed work as a reproducible measurement harness and an evidence-status decision record. It does not implement the production Session Bundle activation or control plane.
tar.zstlayout with a versioned manifest, exact entry equality, byte counts, SHA-256 digests, and path-safety checks..git,node_modules, common credential files, and the source storage-root authority marker.decisionReady=false; require a real sanitized corpus before reporting decision percentiles.Current decision status
The document retains the storage/image/compression recommendations that are supported by implementation constraints, but it does not present the old synthetic 24/32 MiB limits or 250 ms budget as measured decisions. No real user session corpus is available in this checkout, so this PR intentionally leaves #1336 open.
Validation
npm run format:checknpm run lintnpm run build:testnpm run typechecknpm run test:scripts: 130/130 passedgit diff --check