Skip to content

refactor(daemon): consolidate session-script test factories and drop saveScriptDefaultedHealedPath - #1508

Open
thymikee wants to merge 1 commit into
mainfrom
p4a/prep-test-factories
Open

refactor(daemon): consolidate session-script test factories and drop saveScriptDefaultedHealedPath#1508
thymikee wants to merge 1 commit into
mainfrom
p4a/prep-test-factories

Conversation

@thymikee

Copy link
Copy Markdown
Member

Preparatory slice for P4a (#1478). This introduces no aggregate. No ReplaySessionTransaction, no SessionScriptPublication, no tagged session-script cluster, and no production publication writer is migrated — all of that is the separate P4a PR. This exists so that PR is reviewable.

1. Session-script test factories

~250 direct writes of saveScript*/recordSession were spread across the daemon test tree as inline object literals. P4a proper has to change the shape of every one of them, so they get a name first.

Three factories next to the existing makeIosSession in src/__tests__/test-utils/session-factories.ts:

factory state
makeAuthoringSession ADR 0016 ordinary authoring recording — recordSession, no repair boundary
makeRepairArmedSession ADR 0012 decision 6 ARMED — plus saveScriptBoundary, the flag the writer's repairArmed keys off
makeRepairCompleteSession ARMED -> COMPLETE — committable

40 inline session literals across 11 test files now go through them. The heaviest is session-script-writer.test.ts (24 literals, 20+ of which spelled out the same three-to-five-field combination). Named "authoring" rather than "recording" because session.recording is the unrelated screen-capture state, and daemon-runtime-recording-teardown.test.ts already has a local makeRecordingSession meaning that.

The factories write today's fields — pure refactor, no production change, no behavior change, no assertion weakened.

Deliberately left explicit

  • session-action-recorder.test.ts / selector-recording.test.ts (9 sites) set saveScriptBoundary: 0 with no recordSession. That is the repair-armed marker in isolation, which is exactly the predicate those tests exercise; forcing them through a factory would add a field production couples but these tests deliberately don't.
  • request-router-repair-expired.test.ts's tombstonedSession — a boundary plus repairSourcePath with no recording, one site, no repetition to remove.
  • session-store.test.ts keeps its own makeSession: one test mutates session.device.name, and the shared factories hand back the module-level IOS_SIMULATOR by reference, so routing it through them would make that mutation leak across tests. Only the dead-field lines were removed there.
  • request-router-typed-error.test.ts keeps its tenant-scoped simulator (simulatorSetPath), now passed into the shared factories as TENANT_SESSION_DEFAULTS rather than re-spelled per test.
  • Assertion-only references (session-replay-repair-loop/-empty-tail/-acceptance, the two integration scenarios, request-save-script-transports.test.ts) were left alone — they read the fields, they don't construct them.

2. saveScriptDefaultedHealedPath deleted

Zero production readers, verified. A repo-wide grep finds only three writers (session-replay-runtime.ts:867/870, session-action-recorder.ts:83), the type declaration, the R7 ownership row, and one test assertion. Nothing branches on it: the writer's refuse-on-exist guard (publishHealedScriptAtomically) has been uniform since #1235 — an explicit --save-script=<path> and the default .healed.ad sibling are refused identically — so the flag has been bookkeeping with no consumer.

Deleting it required, in the same commit:

  • dropping its row from SESSION_STATE_FIELD_OWNERS in scripts/layering/session-state.ts;
  • lowering the R10 baseline in scripts/layering/daemon-modularity.ts: writerOwnedFields 30 -> 29, ownerFileClaims 42 -> 40 (the field claimed two owner files). The ratchet fails on a drop as well as a growth, so this could not be deferred.

scripts/layering/daemon-modularity.test.ts needed no change — it asserts against DAEMON_MODULARITY_BASELINE dynamically rather than against literals. scripts/layering/model.test.ts was checked: its AST fixture string uses saveScriptComplete, not the deleted field, and its field-count assertions derive from the tables, so it is unaffected.

The one test that asserted on the flag (session-script-writer.test.ts) kept its other concern — that close --save-script=<explicit path> re-points a defaulted-healed repair and publishes cleanly to the new target — and lost only the marker assertion.

Gates

pnpm check:layering
Layering guard: OK — ... all 40 SessionState fields are classified and every write is
inside its declared owner (R7); ... R10 pins R7 at 29 writer-owned fields / 40 owner
claims, ...

pnpm check:affected --base origin/main --run: format, lint, typecheck, layering, fallow, mcp-metadata and build all pass. vitest-related runs 236 files / 2269 tests with 1 pre-existing failure: src/daemon/handlers/__tests__/session-test-runner.test.ts ("test dispatches per-platform scripts", ['ios','android'] vs ['android','ios']). It reproduces unchanged on origin/main (32b9db2d7) with this branch stashed, is in the P3 worker's session-test*.ts territory, and is pulled into the related set only because this PR touches src/daemon/types.ts. Every test this PR actually edits passes.

Coordination

The only shared-file edit is the two-line writerOwnedFields/ownerFileClaims baseline in scripts/layering/daemon-modularity.ts — localized, well away from the R10 logical-module policies the P3 worker is editing. session-replay-runtime.ts is touched only at ~867 (the arming step), disjoint from P3's ~461.

Refs #1478

🤖 Generated with Claude Code

https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8


Generated by Claude Code

…saveScriptDefaultedHealedPath

Preparatory slice for P4a (#1478). No aggregate, no transaction type, no
publication-writer migration — those land separately.

Two things:

1. Name the session-script session states in the shared test factories
   (`makeAuthoringSession`, `makeRepairArmedSession`,
   `makeRepairCompleteSession`) and route 40 inline session literals across
   11 test files through them. The `saveScript*` fields are not independent
   — recording without a boundary is ordinary authoring, a boundary without
   `saveScriptComplete` is an ARMED-but-uncommittable repair, and only the
   COMPLETE combination publishes — so re-deriving the combination per test
   buried the distinction each test was actually about. Pure refactor: the
   factories write today's fields and no assertion was weakened.

2. Delete `saveScriptDefaultedHealedPath`. It had zero production readers:
   the writer's refuse-on-exist guard has been uniform since #1235, so the
   flag was written in three places and never consulted. Removing it takes
   its R7 owner row with it and lowers the R10 baseline from 30/42 to 29/40
   (the ratchet fails on a drop too, so this cannot be deferred).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8
@github-actions

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.91 MB 1.91 MB -109 B
JS gzip 611.8 kB 611.7 kB -35 B
npm tarball 729.4 kB 729.4 kB -33 B
npm unpacked 2.56 MB 2.55 MB -109 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.7 ms 27.8 ms -1.0 ms
CLI --help 59.4 ms 58.9 ms -0.5 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/screenshot-result.js -37 B -18 B
dist/src/session.js -72 B -17 B

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head f1be82ebaf1e3f07a9b243441e29656767f11b75 against #1478 P4a and ADRs 0012/0016. No blocking findings.

What I verified:

  • This is genuinely preparatory only: no aggregate, publication writer migration, or production state-machine redesign is smuggled into the slice.
  • saveScriptDefaultedHealedPath has no production reader. Its three writes only tracked whether saveScriptPath came from the healed-sibling default, while publication collision/force behavior is uniformly driven by the target path and writer policy. Removing the type field, both writers, R7 ownership row, and the one marker assertion together is behavior-neutral.
  • makeAuthoringSession, makeRepairArmedSession, and makeRepairCompleteSession reproduce today’s real combinations (recordSession; then boundary; then completion) and make the ARMED/COMPLETE distinction visible. Tests that intentionally exercise malformed/marker-only states remain explicit, and the mutable-device test remains outside the shared factories.
  • Converted tests retain their original behavioral assertions; the writer suite becomes materially easier to audit without hiding target paths, action history, force state, or terminal state in the factory.
  • R7/R10 move monotonically and consistently: 40 SessionState fields remain fully classified; writer-owned fields drop 30 → 29 and owner claims 42 → 40.

Local exact-head checks:

  • focused writer/publication/replay/router set: 4 files, 59 tests passed
  • pnpm check:layering: green, including R7 at 29/40 and R9 at 76
  • git diff --check: clean

GitHub checks are green so far, including integration, layering, Fallow, FreeRange, packaged CLI, typecheck, and platform-independent smokes. Coverage plus Android/iOS smoke/release jobs were still pending at review time, so merge readiness is conditional on those exact-head jobs finishing green.

One scope clarification for the landing record: despite the P4a terminology, this PR is the test-factory/dead-field prep slice, not P4a’s aggregate or ownership migration itself. The body already states that clearly.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head f1be82ebaf1e3f07a9b243441e29656767f11b75 against #1478 and ADRs 0012/0016. No findings.

saveScriptDefaultedHealedPath has no production reader; its removal preserves publication collision/force behavior, which is uniformly decided from the target path and writer policy. The new authoring, repair-armed, and repair-complete factories reproduce the current state combinations without masking intentional malformed-state tests.

Exact-head CI is green, including integration, coverage, layering, typecheck, platform smokes, and native releases. This is ready to merge as the stated P4a prep slice (not the P4a ownership/state-machine migration itself).

Residual risk: no separately authorized cross-vendor review was performed.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants