fix(daemon): reject unsupported raw --save-script flags at request ingress

[thymikee]

Implements P4-pre from #1478. Scoped strictly to that brief: no tagged aggregate, no supported-writer migration.

Why

flags.saveScript arms session-script publication as soon as a successful handler records the request's action (recordActionEntry sets recordSession and the target path). Any recordable command that reaches a handler with the raw flag set can therefore arm publication and later write a .ad artifact — record stop writes one immediately through record-only cleanup, while trace and the interaction commands publish later on close.

The CLI, Node client, and MCP surfaces only ever emit the flag for its released owners, so this arming path was reachable only by a hand-built wire request.

The seam

Rejection lives in createRequestHandler's runRequestWithinScope (src/daemon/request-router.ts), directly after the existing recordingFlagsResponse sibling and before createRequestExecutionScope — so it precedes lease admission, session lookup, device binding, provider scope, and handler dispatch.

daemon-runtime.ts builds exactly one handleRequest and passes the same function to both createSocketServer(...) and createDaemonHttpServer({ handleRequest }), so the socket wire, HTTP /rpc, and nested batch steps all meet it. Validating commandRpcParamsSchema alone would have left the socket transport open.

Rule: flags.saveScript !== undefined on a command whose descriptor is not a saveScriptFlagOwner → INVALID_ARGS, normalized through normalizeError with an explicit hint, plus diagnosticId and logPath from a forced diagnostics flush (ADR 0010 decision 6), and a save_script_flag_rejected diagnostic.

Presence is rejected, not truthiness — a raw saveScript: false is as unsupported on record as true is, mirroring the maestro replay guard's !== undefined reading of the same flag. Ownership is derived from the command descriptor registry rather than a hand-written string set, and resolves to exactly open, close, replay.

Behavior deleted

releaseRecordOnlySession's writeLog option and the { writeLog: true } call at record stop — i.e. sessionStore.writeSessionLog(session) for record-only sessions. Deleted only after the seam tests were green.

Files

Created

• src/daemon/request-save-script-policy.ts — unsupportedSaveScriptFlagResponse(req) + SAVE_SCRIPT_FLAG_OWNER_COMMANDS
• src/daemon/__tests__/request-save-script-policy.test.ts
• src/daemon/__tests__/request-save-script-transports.test.ts

Modified

• src/daemon/request-router.ts — policy call in runRequestWithinScope
• src/daemon/daemon-command-registry.ts — saveScriptFlagOwner trait + ownsSaveScriptFlag / listSaveScriptFlagOwnerCommands
• src/core/command-descriptor/registry.ts — saveScriptFlagOwner: true on open, close, replay only
• src/daemon/handlers/record-trace-recording.ts — dormant writer removal
• CHANGELOG.md

Tests

request-save-script-transports.test.ts drives the real createRequestHandler behind real socket and HTTP servers, parameterized over both transports:

• unsupported command + flag → rejected; no trace started, no action recorded, recordSession/saveScriptPath untouched, writeSessionLog returns {written:false}, no .ad anywhere under the temp root;
• the identical request without the flag succeeds and records its action, proving the rejection is the flag and not the environment;
• pre-admission proof: the request also carries sessionIsolation: 'tenant' without a tenant, which admission rejects with its own message — the save-script message wins, so the seam is genuinely ahead of admission;
• owner passthrough: replay --save-script reaches its handler;
• a batch step carrying the flag on trace is rejected;
• an owner-armed session still records its target and publishes a real .ad.

Negative control: with the seam short-circuited, 4 of these fail (both transports' arm/write plus both pre-admission cases), and trace --save-script armed and started as before.

Gates

• pnpm check:affected --base origin/main --run → pass (format, lint, typecheck, layering, fallow; 298 test files / 2898 tests)
• Focused: vitest --project unit-core src/daemon src/core/command-descriptor → 187 files / 1589 tests
• Provider-integration saveScript/session suites (active-session-script-publication, no-record-recorder-routes, replay-repair-abort-close, replay-repair-record-exclusion, daemon-transport, daemon-http-server) → 19 tests, confirming open/close/replay arm and publish unchanged
• Also standalone: pnpm typecheck, pnpm lint, pnpm check:layering, pnpm check:fallow, pnpm check:production-exports, pnpm format:check, pnpm check:mcp-metadata

rpcProtocolVersion not bumped.

One judgement call for review

DaemonBatchStep.flags is free-form Record<string, unknown> passthrough, so a Node/MCP batch payload could technically have set saveScript on a non-owner step today and armed publication. This PR treats that as the same undeclared raw arming path the brief asks to close — no command schema, CLI grammar, or MCP tool ever advertised it — and pins it with a test.

If you consider free-form batch step flags a released surface, this is the one decision to revisit.

Checked separately: session save-script is positional-based (['save-script', './screen-x.ad']), not flags.saveScript, so it is unaffected.

Left for P4a

No tagged aggregate; ReplaySessionTransaction / SessionScriptPublication untouched. SessionStore.recordAction / recordActionEntry still arm from request flags for the owner commands — that is the P4a migration target.

Parallelism note

Developed concurrently with P2. No P2-owned file is touched: packages/maestro, src/daemon/adapters/maestro/, src/daemon/handlers/session-replay-runtime.ts, and src/daemon/handlers/session-test-discovery.ts are all untouched, so this should merge without interacting with the Maestro extraction.

---

Generated by Claude Code

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.90 MB	1.90 MB	+704 B
JS gzip	609.7 kB	609.8 kB	+172 B
npm tarball	727.4 kB	727.6 kB	+198 B
npm unpacked	2.55 MB	2.55 MB	+704 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	26.2 ms	26.6 ms	+0.5 ms
CLI --help	57.0 ms	57.4 ms	+0.5 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/internal/daemon.js	+635 B	+156 B
dist/src/sdk-batch-runner.js	+69 B	+16 B

[thymikee]

Verdict: approve after one small test amendment. I found no production correctness issue.

The ingress placement is correct, both transports converge there, batch nesting is covered, and deleting the record-only writer is justified once the raw path is closed. I also checked the replay-scoped fast path: replay actions neither inherit saveScript nor parse it from .ad actions, so it does not bypass this policy.

One requested amendment: the HTTP test helper currently projects only code, message, and hint, discarding diagnosticId and logPath. That means the ADR 0010 guarantee could regress while these new transport tests remain green. Please preserve those fields in the helper and assert for both transports that diagnosticId is present and logPath is present and exists. Ideally, also verify that the log contains save_script_flag_rejected.

I tested that amended assertion locally; it passes on both socket and HTTP. The focused exact-head suite passed 18/18, and all completed GitHub checks were green at review time, with one iOS smoke lane still running.

[thymikee]

Amendment pushed in ecd3ec6a7.

The HTTP helper built its error object from only code/message/hint, so diagnosticId and logPath were dropped on that transport. The socket helper forwards the raw JSON, which is why the gap stayed invisible. Both fields are now preserved in the HTTP projection, and each transport asserts that diagnosticId is present, logPath is present and the file exists, and the log contains save_script_flag_rejected.

I also ran the negative control the request implies: restoring the old projection makes the HTTP case fail on logPath while socket still passes, so the assertion guards the ADR 0010 contract rather than just passing alongside it.

Focused suites 18/18, and pnpm check:affected --base origin/main --run is green at the exact head (2898 tests).

Thanks for the replay fast-path check — that replay actions neither inherit saveScript nor parse it from .ad actions was the remaining "is there another arming path" question, and it's a better answer than the ingress test alone gives.

---

Generated by Claude Code

[thymikee]

Clean review for P4-pre at ecd3ec6a. The descriptor-derived open/close/replay allowlist closes raw saveScript arming before admission across socket, HTTP, and nested batch execution; ADR 0010 diagnostics and unsupported no-write counterfactuals are covered. Record-only’s now-unreachable immediate writer is safely removed. All completed exact-head checks are green; Android, iOS, and Linux smoke jobs remain pending. Ready for human review.

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

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-30 16:11 UTC