Skip to content

fix(record): make an aborted authoring recording terminal by construction - #1712

Merged
thymikee merged 4 commits into
mainfrom
claude/addressable-issues-no-device-twyoip
Aug 10, 2026
Merged

fix(record): make an aborted authoring recording terminal by construction#1712
thymikee merged 4 commits into
mainfrom
claude/addressable-issues-no-device-twyoip

Conversation

@thymikee

@thymikee thymikee commented Aug 10, 2026

Copy link
Copy Markdown
Member

Summary

A second successful open on an open --save-script session aborts the recording: the aggregate goes to authoring{aborted}, recordSession is cleared, and the caller is warned. close --save-script then refuses it with "Retry with plain close; it will tear down the session without writing."

That promise was not kept. recordSession is an evidence-capture flag that several surfaces set directly, without consulting the publication aggregate — so it could contradict the terminal status. When the second open carried --save-script, the recorder's shared flag ingress (applyRecordedSaveScriptFlags) re-armed the boolean while the status stayed ABORTED — session-open.ts aborts at line 454 and records the open action, flags included, at line 461 — and a bare close published the full session log. SessionScriptWriter.write gated only on recordSession and the repair variant, so nothing on the ordinary authoring path refused an aborted lifecycle.

The abort is now terminal by construction rather than inert by ordering:

  • isAuthoringAborted gives the pure aggregate one home for the question, next to the repair-side terminality it mirrors.
  • recordSessionAfterSaveScriptFlag makes --save-script arm recording through the publication projection instead of around it. It answers "not recording" for an ABORTED lifecycle on every surface that handles the flag — the re-open builder (buildNextOpenSession), the close finalizer (finalizeOrdinaryCloseScript), and the recorded-action ingress — so the flag can no longer contradict the status.
  • SessionScriptWriter asks one isPublicationWriteBlocked question covering all three reasons to publish nothing, so every path reaching the writer — bare close, teardown, idle-reap, active publication — refuses it.

Armed recordings, published recordings, and every repair transaction are unaffected. session-close.ts's note that this case was unresolved is updated rather than left stale.

Closes #1533

Why the ingress fix alone was not enough

The first pass fixed the recorded-action ingress and made the writer refuse an ABORTED lifecycle, but left the other two recordSession writers alone and documented the resulting contradiction as acceptable — the writer's own comment observed that "something can re-arm that boolean behind the terminal status".

That something was live. buildNextOpenSession re-armed recording for any open --save-script (existingSession.recordSession || saveScript), and applyOrdinaryScriptRecordingOpenOutcome only aborts a lifecycle that is still ARMED — an already-ABORTED one returns early without clearing anything. A third open --save-script therefore left recordSession true behind the terminal status.

The writer gate hid the publication symptom, so this was not a second publication bug. It was still a real one: recordSession disables the direct iOS selector fast paths for click and get (interaction-touch.ts, selector-runtime.ts), forcing every interaction onto the snapshot route. An aborted session went on paying recording-time costs for a recording that can never publish.

With the rule owned in one place, the writer's gate stands on the aggregate alone rather than as a net under a known drift, and the paragraphs defending that drift are replaced by statements of the rule.

Scope: 12 files (6 implementation, 4 regression tests, ADR 0016, CHANGELOG). The writer change is net-simplifying — the three sequential guards in write() collapse to one predicate, which is also what keeps the function under the fallow complexity threshold it tripped when the gate was added as a fourth branch.

Validation

Red-before-fix evidence, ingress and writer (source fix stashed, tests unchanged):

  • the --save-script ingress does not re-arm an aborted authoring lifecycle — failed; recordSession was true.
  • an aborted lifecycle cannot be retargeted by a later --save-script= either — failed; the target moved to the hijacking path.
  • write() publishes nothing for an aborted authoring lifecycle that is still recording — failed; a script was written.
  • the abort is terminal per-target too — an explicit --save-script path is not written either — failed; the explicit target was written.
  • bare close on an aborted authoring session writes no script — failed; the session log landed on disk.

Red-before-fix evidence, re-open surface (only buildNextOpenSession's one line reverted, new tests in place — stashing everything would have removed the tests too and proved nothing):

  • re-opening an aborted authoring session with --save-script does not re-arm recording — failed; true !== false.
  • an aborted lifecycle that is already recording is corrected, not carried forward — failed; true !== false.

All seven are assertion failures, not timeouts. The paired control tests stayed green against the pre-fix code, which is what pins the fix as scoped rather than a blanket refusal: an ARMED authoring lifecycle still takes the ingress, an ARMED authoring recording is untouched by the abort gate, an ordinary armed authoring session still publishes on bare close, plus the two new re-open controls (a re-open with --save-script still arms recording for a session with no publication yet, a re-open without --save-script leaves an armed authoring session recording).

After the fix:

  • pnpm exec vitest run --project unit-core: 674 files / 5,800 tests passed, 3 skipped.
  • pnpm exec vitest run src/daemon: 222 files / 1,950 tests passed.
  • pnpm format:check, pnpm lint, pnpm typecheck, pnpm check:layering — all passed. Layering includes the R7 ownership check, which is the relevant one here: this moves a SessionState.recordSession write into the publication projection.

No device lane is involved: the bug and all seven regressions are unit-level, reached through the daemon-private publication projections, the open surface, and the writer. Native/device lanes remain GitHub-CI-authoritative.


Generated by Claude Code

…tion

A second successful `open` on an `open --save-script` session aborts the
recording: the aggregate goes to `authoring{aborted}`, `recordSession` is
cleared, and the caller is warned. `close --save-script` then refuses it with
"Retry with plain close; it will tear down the session without writing."

That promise was not kept. When the second `open` itself carried
`--save-script`, the recorder's shared flag ingress re-armed `recordSession`
while leaving the status terminal, and a bare `close` published the full
session log — the writer gated only on `recordSession` and the repair variant,
so nothing on the ordinary authoring path refused an aborted lifecycle.

The abort is now terminal by construction rather than inert by ordering:

- `isAuthoringAborted` gives the pure aggregate one home for the question.
- `applyRecordedSaveScriptFlags` takes no branch for an aborted lifecycle:
  it neither re-arms recording nor retargets the output path.
- `SessionScriptWriter` asks one `isPublicationWriteBlocked` question covering
  all three reasons to publish nothing, so every path reaching the writer
  (bare `close`, teardown, idle-reap, active publication) refuses it.

Armed recordings, published recordings, and every repair transaction are
unaffected; the control tests for those stay green against the pre-fix code
while the five new regressions go red.

Closes #1533
@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.05 MB 2.05 MB +204 B
JS gzip 665.2 kB 665.3 kB +58 B
npm tarball 802.2 kB 802.3 kB +55 B
npm unpacked 2.80 MB 2.80 MB +204 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.1 ms 28.7 ms +0.7 ms
CLI --help 68.9 ms 73.1 ms +4.1 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/screenshot-result.js +200 B +43 B
dist/src/session.js +4 B +14 B
dist/src/internal/daemon.js 0 B +4 B
dist/src/agent-device-client.js 0 B -1 B

@thymikee

Copy link
Copy Markdown
Member Author

Exact-head review at 53bbf5e1c: implementation and regressions are clean; aborted authoring is terminal at both ingress and writer boundaries, with credible red-before coverage and no changed cancellation/device path. One P2 documentation blocker remains: ADR 0016 still says #1533's rearm/bare-close case is unresolved, which this PR now closes. Update the amendment to record the resolution. Also correct the new session-close.ts comment naming nonexistent isAuthoringAbortedWriteBlocked; the actual private writer gate is isPublicationWriteBlocked (or describe the behavior without a symbol). All substantive checks are green; no ready label applied.

…reference

The ADR 0016 close-time amendment still described #1533 as unresolved, and
the session-close.ts note named `isAuthoringAbortedWriteBlocked` — a private
helper that was folded into `isPublicationWriteBlocked` when the writer's
three sequential guards collapsed into one predicate, so the symbol names
nothing in the tree.

Copy link
Copy Markdown
Member Author

Both addressed in f24f472.

ADR 0016. The 2026-08-02 amendment's closing sentence now points forward instead of declaring the case unresolved, and a new #1533, shipped amendment records the resolution: ABORTED terminality was enforced only by abortAuthoringOnSecondOpen clearing recordSession — inert by ordering — and is now terminal by construction at both boundaries. It also records the part worth keeping in the decision log: the gap made an error message this ADR specifies untrue, since the close --save-script refusal promises plain close "tears down without writing". No state name, transition, or entry point in the lifecycle above it is altered.

Stale symbol. Correct, and it was mine — isAuthoringAbortedWriteBlocked existed in the first version of the writer change and was folded into isPublicationWriteBlocked when the fallow complexity gate rejected adding a fourth sequential guard to write(). I updated the prose but not that reference. The comment now describes the behavior without naming a private helper, which also keeps it from going stale again if the gate is refactored. Grepped the tree for the old name: no remaining occurrences.

Re-ran pnpm check:affected --base origin/main --head HEAD --run on the new head: all runnable checks passed, 272 files / 2,682 tests.


Generated by Claude Code

Copy link
Copy Markdown
Member Author

iOS Smoke Tests failed on f24f472RunnerTests.testBareTypeUsesTappedInputWhenSoftwareKeyboardIsHidden(), 34 executed / 1 failure / 0 unexpected. It is not this PR's, and the run history proves it rather than merely arguing it:

Commit iOS run Result
53bbf5e — the entire implementation + all 5 regressions 31388607419 success
f24f472 — ADR markdown + one code comment 31393096243 failure

git diff 53bbf5e f24f472 is 2 files: docs/adr/0016-active-session-script-publication.md and 4 lines in session-close.ts, every one of which is inside a // comment block. So the iOS suite passed with 100% of the executable change present and failed on a commit carrying none. There is also no path from the diff to the failure even in principle — this PR touches TypeScript under src/daemon/ (session script publication), and the failing test is Swift in AgentDeviceRunnerUITests exercising text entry against a hidden software keyboard.

The failure looks like simulator keyboard-state contention rather than a product defect. The test's own precondition is the assertion that fell over:

XCTAssertFalse(
  isKeyboardVisible(app: app),
  "the test must exercise a focused responder with the software keyboard hidden"
)

Software-keyboard visibility is simulator-global (hardware-keyboard attach state), so it is order- and host-sensitive. Corroborating it in the same run: the test immediately preceding it, testTypeWithoutResolvedInputReturnsTypedFailureBeforeDispatchingText, took 12.195s doing repeated Checking existence of Keyboard (First Match) with retry 1/retry 2 cycles across five rounds — the keyboard state on that runner was already unstable in the window right before the failing assertion.

I have re-run the failed job. If it comes back red on the same test I will treat it as real and investigate properly rather than re-running again; if it goes green, this is worth noting as a flake candidate in the iOS lane, since the precondition assertion depends on global simulator state the test does not itself establish.


Generated by Claude Code

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 10, 2026
@thymikee

thymikee commented Aug 10, 2026

Copy link
Copy Markdown
Member Author

Exact-head re-review at f24f47259: clean / ready. ADR 0016 now records #1533’s terminality resolution accurately, and the stale nonexistent writer-symbol reference is gone. Ingress and writer boundaries both enforce aborted authoring terminality, the red-before regressions and armed controls remain load-bearing, and all substantive checks—including iOS smoke—are green. Non-blocking bookkeeping only: the added ADR makes nine unique changed files while the PR body still says eight. No ready label applied by this reviewer.

claude added 2 commits August 10, 2026 15:36
…und it

`recordSession` is an evidence-capture flag, but three surfaces set it
directly without consulting the publication aggregate, so it could
contradict a terminal ABORTED authoring status. The #1533 fix closed the
recorded-action ingress and made the writer refuse an ABORTED lifecycle,
then documented the remaining contradiction as acceptable — the writer's
own comment noted that "something can re-arm that boolean behind the
terminal status".

That something was live: `buildNextOpenSession` re-armed recording for any
`open --save-script`, and `applyOrdinaryScriptRecordingOpenOutcome` only
aborts a lifecycle that is still ARMED. A third `open --save-script` on an
already-ABORTED session therefore left `recordSession` true behind the
terminal status. The writer gate hid the publication symptom, but the
session kept paying recording-time costs for a recording that can never
publish: `recordSession` disables the direct iOS selector fast paths for
click and get, forcing every interaction onto the snapshot route.

Route the flag through one rule owned by the publication projection
(`recordSessionAfterSaveScriptFlag`), which answers "not recording" for an
ABORTED lifecycle on every surface that handles it — the re-open builder,
the close finalizer, and the recorded-action ingress. The writer's gate is
unchanged and still correct; it now stands on the aggregate alone rather
than as a net under a known drift, so the comments defending the drift are
replaced by statements of the rule.

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

Copy link
Copy Markdown
Member Author

Exact-head re-review at 5bec77cc4: clean / ready. The new publication-projection change closes the remaining third-open drift: an already-ABORTED authoring lifecycle cannot be re-armed by buildNextOpenSession, the close finalizer and recorded-action ingress preserve the same terminal rule, and the writer remains the final aggregate publication gate. The aborted reopen regressions are load-bearing, while none→save and armed→no-save controls preserve intended behavior; repair, armed, and published semantics are unchanged. ADR 0016, changelog, and the 12-file scope are accurate. All substantive checks are green, the head is CLEAN/MERGEABLE, and the existing ready-for-human label remains justified.

@thymikee
thymikee merged commit 4f9aded into main Aug 10, 2026
30 checks passed
@thymikee
thymikee deleted the claude/addressable-issues-no-device-twyoip branch August 10, 2026 16:01
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-10 16:02 UTC

thymikee pushed a commit that referenced this pull request Aug 11, 2026
…anch

Two conflicts, both resolved as posted on the PR before the merge landed.

`scripts/layering/daemon-modularity.ts` — the R7 ratchet. #1724 and this
branch each remove a different `SessionState` field, so neither side's
baseline survives. Note git only flagged `ownerFileClaims` (26 vs 28);
`writerOwnedFields: 22` merged silently because both sides wrote 22, yet
the merged manifest holds 21. Counted the auto-merged
`SESSION_STATE_FIELD_OWNERS` rather than doing the subtraction: 21 entries,
25 owner claims. The layering guard confirms — R10 now pins R7 at 21/25.

`src/daemon/handlers/record-trace-recording.ts` — modify/delete. #1724
deletes the module; this branch's only change in it was a doc comment that
stopped naming the removed `recordSession` field, so the deletion wins.
Nothing references the path any more.

`session-state.ts` auto-merged correctly: #1724 predates #1712/#1719 and
still carried the `recordSession` manifest entry, and the merge drops it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFW9gJqz1wEHoowkdd1nFW
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.

Aborted authoring recording still publishes on bare close after a --save-script re-arm

2 participants