feat: --force/--overwrite for --save-script + arm-time EEXIST preflight#1266
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
Review of Conformance verified (traced, not trusted):
Blocker (CI): Fallow's complexity gate is red — Design question (non-blocking, wants an explicit decision): Nit: the shared flag definition means With the Fallow extraction done, this closes both repair-loop UX gaps (#1258 + the arm-time discard trap) as specified. |
…ilure Pure structural refactor, no behavior change: moves the diagnose + classify-and-return / AppError-rethrow logic out of SessionScriptWriter.write into a module-level helper. Drops write()'s cyclomatic below Fallow's threshold (the finding CI flagged on #1266) — the extracted throw still propagates out of the catch exactly as before.
|
Fixed the red Fallow Code Quality check (a5234e5).
Pure structural refactor, zero behavior change — the extracted Verification:
Pushed head: |
|
Re-review of exact head a5234e5: Fallow is fixed, but persisted overwrite authorization is inconsistent. runReplayScriptFile passes only the live request force flag to the EEXIST preflight. A repair armed with save-script plus force and later continued with replay from plus save-script, without repeating force, is rejected by preflight when the target exists, even though the session persisted saveScriptForce and every eventual write path honors it. Use the same effective decision as publication, live force or preRunSession.saveScriptForce, and add the continuation regression. The earlier per-target question also remains: re-arming from forced a.ad to b.ad without a live force currently carries sticky authorization and may overwrite b.ad. Clear force when retargeting without an explicit opt-in, or document and test that broader session-level contract explicitly. All checks are green, but this head is not merge-ready. |
|
Both persisted-force inconsistencies fixed ( Point 1 — preflight now honors persisted force.
Point 2 — force is per-target, cleared on retarget (chose your preferred option (a)). Re-arming a different
Nit (connect --overwrite alias): deliberate — it's the one shared global Verification: Pushed head: |
|
Re-review of exact head 7f3accd: the persisted-force continuation and close-retarget cases are fixed, but replay retarget preflight still has an ordering mismatch. The preflight accepts persisted saveScriptForce before determining whether this request explicitly retargets from forced a.ad to b.ad. Later, applySaveScriptRetarget clears that authorization for b.ad, so replay steps can execute and publication then refuses existing b.ad. Persisted force should bypass preflight only when the computed target is unchanged, unless the current request carries live force. Add a replay continuation regression for prior forced a.ad plus explicit existing b.ad without live force: it must fail before dispatch and must not mutate the session target. Not merge-ready yet. |
|
Preflight ordering mismatch fixed ( The bug you caught: the preflight computed The fix: the effective-force decision now lives inside
Regression added: a Unchanged cases stay correct (all still green): same-target Verification: Pushed head: |
…ilure Pure structural refactor, no behavior change: moves the diagnose + classify-and-return / AppError-rethrow logic out of SessionScriptWriter.write into a module-level helper. Drops write()'s cyclomatic below Fallow's threshold (the finding CI flagged on #1266) — the extracted throw still propagates out of the catch exactly as before.
thymikee
force-pushed
the
feat/save-script-force-and-arm-preflight
branch
from
6618d4f to
83d189f
Compare
|
Rebased onto Clean rebase — no conflicts. All 4 commits (
Re-verified against the merged code:
New head: |
|
Re-review of exact head 83d189f found one remaining correctness bug. The retarget EEXIST preflight now computes per-target force correctly, but saveScriptComplete is cleared before that preflight runs. If the session already holds a completed repair, including a valid #1267 empty-tail completion, a rejected retarget returns before dispatch while leaving saveScriptComplete false. A later close can then treat the repair as incomplete and tear it down without publishing. Move the completion reset after the save-script preflight, or restore the prior value on rejection. Extend the existing no-dispatch and no-mutation regression to assert saveScriptComplete remains true and that a subsequent close can still commit the original target. The current iOS smoke job has also failed and macOS is still running, so this head is not merge-ready. |
…ilure Pure structural refactor, no behavior change: moves the diagnose + classify-and-return / AppError-rethrow logic out of SessionScriptWriter.write into a module-level helper. Drops write()'s cyclomatic below Fallow's threshold (the finding CI flagged on #1266) — the extracted throw still propagates out of the catch exactly as before.
thymikee
force-pushed
the
feat/save-script-force-and-arm-preflight
branch
from
83d189f to
fe87050
Compare
|
Rebased onto Clean rebase — git auto-merged the overlap, and I verified both sides coexist (not one silently dropping the other) on all three flagged files:
No conflict needed a design call — the two changesets touch disjoint concerns (ref-frame lifecycle vs save-script force) in the same files. Re-verified against the merged base:
(Prior head's macOS Smoke Test flake noted — infra, unrelated; the fresh CI run should clear it.) New head: |
…light #1235 made healed-script publication refuse-on-exist. #1258 adds an escape hatch: --force (alias --overwrite) on open/close/replay makes publishHealedScriptAtomically atomically REPLACE an existing target (renameSync) instead of refusing. The flag threads CLI -> daemon request -> SessionState.saveScriptForce (persisted at arm time, like saveScriptPath, so a later close/auto-commit that doesn't repeat the flag still honors it) -> the publish primitive. Default (flag absent) is unchanged: refuse-on-exist. Second half: an arm-time EEXIST preflight in session-replay-runtime.ts now fails a repair-armed replay --save-script BEFORE any step dispatches when its target already exists and --force is not set, instead of only failing at publish time after the whole repair run (and its corrective steps) has already executed against the device.
…ilure Pure structural refactor, no behavior change: moves the diagnose + classify-and-return / AppError-rethrow logic out of SessionScriptWriter.write into a module-level helper. Drops write()'s cyclomatic below Fallow's threshold (the finding CI flagged on #1266) — the extracted throw still propagates out of the catch exactly as before.
…get) Addresses two authorization inconsistencies in the #1258 --force work flagged in re-review: 1. Arm-time EEXIST preflight now uses the SAME effective force decision as publication — `req.flags?.force || preRunSession.saveScriptForce` — instead of the live flag alone. A repair armed with `--save-script --force` and continued via `replay --from … --save-script` (without repeating --force) is no longer rejected on a target the earlier forced leg authorized. 2. Force is now per-target, not sticky-across-retarget. Re-arming a DIFFERENT `--save-script=<other>` without a live --force CLEARS the persisted `saveScriptForce` (new shared `applySaveScriptRetarget`, used by both the replay armer and `recordActionEntry`/close), so a retarget can never silently overwrite a file nobody opted into. A live --force on the retarget re-grants it for the new target. Updated the SessionState doc accordingly. Regressions: persisted-force `--from` continuation is not preflight-rejected; retarget-without-force refuses an existing target (and the --force contrast overwrites it). Both verified to fail without their respective fix.
Re-review ordering-mismatch fix. The preflight computed effective force as `req.flags?.force || preRunSession.saveScriptForce`, accepting persisted force regardless of whether THIS request retargets. So a `--from` continuation with explicit `--save-script=b.ad` (no live force) on a session forced for a.ad would PASS the preflight, run every step, then have `applySaveScriptRetarget` clear the force for b.ad, and finally refuse the existing b.ad at publish time — defeating the arm-time-preflight point and mutating the session mid-run. Now the effective-force decision is computed inside `preflightSaveScriptTarget` against the target THIS request resolves to (the same one the armer will set), matching `applySaveScriptRetarget`'s per-target contract: live force always bypasses; persisted force bypasses ONLY when `targetPath === existingSaveScriptPath` (same target). A retarget to a different path without live force is now refused BEFORE any step dispatches. The preflight stays read-only (runs before the armer; never mutates the session). Regression: a --from continuation retargeting to an existing b.ad without live force fails at the preflight, dispatches zero steps, and leaves saveScriptPath/ saveScriptForce unchanged. Verified it fails (leg passes preflight, runs) when the fix is reverted. Same-target continuation and live-force retarget stay correct.
thymikee
force-pushed
the
feat/save-script-force-and-arm-preflight
branch
from
fe87050 to
20e0542
Compare
|
Rebased onto |
|
Independent re-review of exact head The PR adds Please add The previously reported P1 also remains on this rebased head: |
…et reject Two re-review blockers. BLOCKER 1 (client-contract gap): the typed close APIs accept --save-script/ --force inputs but dropped the daemon's `savedScript` response, so a Node client could request publication but not learn where the file landed. Add `savedScript?: string` to SessionCloseResult and AppCloseResult, and project it via readOptionalString in both close normalizers (sessions.close, apps.close). New public-client coverage asserts it round-trips (and is absent when the daemon published nothing). BLOCKER 2 (P1 ordering): the C2 `saveScriptComplete = false` reset ran BEFORE the EEXIST preflight's early-return, so a retarget REJECTION corrupted a prior COMPLETE transaction — a later close would then refuse to commit the original target. Move the reset to AFTER `if (saveScriptPreflight) return ...` (verified nothing between the two positions reads saveScriptComplete; applySaveScriptRetarget already runs later, so the original saveScriptPath is preserved). The reset is correct only when the run proceeds to re-arm. Strengthened the retarget-rejection regression to assert (a) saveScriptComplete survives the rejection and (b) a later bare close commits the ORIGINAL target, not the rejected new one. Both verified to fail without their respective fix.
|
Both re-review blockers fixed ( BLOCKER 1 — close
BLOCKER 2 — P1 ordering: retarget rejection no longer corrupts a COMPLETE transaction.
Retarget-rejection regression strengthened — now explicitly asserts BOTH (confirmed to fail without the fix):
Verification: New head: |
|
Re-review of exact head |
thymikee
added
the
ready-for-human
|
1 participant
Summary
Closes #1258.
--force/--overwriteflag. fix(replay): repair-transaction lifecycle — keep-alive, no-partial-emit, close-as-lifecycle, atomic publish #1235 made healed-script publication refuse-on-exist (publishHealedScriptAtomicallythrowsCOMMAND_FAILEDon an existing target). This PR adds--force(alias--overwrite) toreplay,close, andopen— when set, the publish primitive atomically REPLACES an existing target (renameSync, same-directory atomic replace) instead of refusing. Default (flag absent) is unchanged: refuse-on-exist.Threaded CLI → daemon request → publish primitive:
forceCLI flag (already used byconnect --force) rather than adding a second flag under the same name — flag names are a single global namespace in this codebase, and--forcewas already taken. Added--overwriteas an alias on that same flag definition and broadened its help text to cover both meanings.forceis captured onSessionState.saveScriptForceat arm time (open --save-script --force, or a repair's firstreplay --save-script --force), mirroring howsaveScriptPathis persisted — so a later write that doesn't repeat the flag (a bareclosefinishing a session opened withopen --save-script --force, a--fromcontinuation leg, or an unattended auto-commit teardown with no live request at all) still honors the overwrite.req.flags?.force || session.saveScriptForce.Arm-time EEXIST preflight (second half of Add --force/--overwrite flag for --save-script now that publish refuses any existing target #1258's follow-up need). Before this change, a
replay --save-scriptrepair run only discovered an existing target at publish time (close/completion) — an agent could run a whole repair (corrective steps included) only to hit EEXIST at the very end.session-replay-runtime.tsnow resolves the same target the armer would (explicit--save-script=<path>, an already-armed session's existing path on a--fromcontinuation, or the default<stem>.healed.adsibling) before the step loop starts, and fails fast with the refuse-on-exist error before any step — including the firstopen— dispatches.--forceskips this preflight (the caller already opted into overwriting).Test plan
npx tsc --noEmitnpx oxlint --deny-warningsnpx oxfmt --checknpx vitest run— fullsrc/daemon,src/commands,src/client,src/clisuites (192 files / 1790 tests), plus the connect/--forceCLI-parsing tests, all passsession-script-writer.test.ts:write(session, { force: true })overwrites an existing target atomically, for both a repair-armed COMPLETE artifact and an ordinary (non-repair) recording; default (noforce) still refusessession-replay-repair-transaction.test.ts: arm-time preflight rejects an existing--save-scripttarget with zero steps dispatched (verified via a request spy);--forceskips the preflight and the replay proceeds;close --save-script --forceoverwrites an existing COMPLETE healed.adpnpm check:fallow --base origin/main— flags one pre-existing complexity finding onSessionScriptWriter.write(cyclomatic 10, unchanged branch structure vsorigin/main); verified via direct diff that my change only added a parameter and a pass-through line, no new branches — not a regression introduced by this PR