fix: self-heal dangling storageState in playwright config at load time (#2491)

[bolichen97]

Summary

Fixes the rc.8 regression where Browser Mode hard-breaks forever when playwright-config.json carries a contextOptions.storageState pointing at a file that no longer exists (#2209 reproduced). #2209 fixed generation — generate_playwright_config() only attaches the key when the file exists — but nothing re-validated an already-written config, so one generated before that fix (or whose storage-state file was later removed) kept the dangling reference forever. Playwright raises ENOENT at context creation for it, breaking every browser_* call, and the running playwright-mcp process caches the config, so regeneration alone couldn't recover a live session.

The fix: repair_playwright_config() (browser/setup.py) runs at the config's load moment — proxy startup in run_proxy(), the one place every config-mode launch passes through (both --config <path> and --config=<path> forms). When the referenced storage-state file is missing, it drops the key — converging on the exact behaviour a fresh generation produces, so Browser Mode degrades to an unauthenticated context instead of dying — rewrites the config atomically so the repair survives restarts, and logs one WARNING naming the missing file and the action taken.

Deliberately NOT done (per the issue): no process-killing, no transport-restart — those were symptoms; once a stale config self-heals at load, the manual recovery sequence is unnecessary. No re-seeding via refresh_storage_state() — it is a documented no-op without a cookie source (the OSS default); if a cookie source ever writes the file back, the untouched-when-present branch keeps the key on the next load.

Hardening (pre-push review fleet findings, all applied)

• Symlinked configs are repaired through the link (path.resolve() before read/write) — os.replace would otherwise swap the user's link for a regular file.
• Relative storageState is judged from the config file's directory (Playwright's vantage), not the proxy CWD — a working hand-edited relative reference is never dropped.
• File mode preserved on rewrite (an operator-tightened 0600 doesn't widen to umask default).
• Interprocess lock: a new _playwright_config_locked flock sidecar (pattern from the module's existing _kiro_mcp_locked) serializes BOTH writers of playwright-config.json — generate_playwright_config() and the repair's read-validate-write. The sidecar is derived from the resolved path (link and target converge on one lock), created with O_NOFOLLOW + regular-file re-check so a pre-planted symlink at the sidecar name cannot forge a keystone flag. A re-read stand-down inside the locked region additionally yields to unlocked MANUAL edits (decode errors count as changed).
• A config that is valid, keyless, absent, or unparseable is left byte-for-byte untouched (unparseable falls through to Playwright MCP's own config error, which names the file — never guess-rewrite a hand-edited config).

Testing

• 12 new tests: TestRepairPlaywrightConfig (repair + persist + WARNING; present-file byte-identical; keyless untouched; idempotent across two loads; missing/unparseable/non-dict shapes untouched; relative-path both branches; 0600 preserved (POSIX); symlink write-through (POSIX); concurrent-writer stand-down; default-path resolution; generate→orphan→repair round-trip converges with fresh generation) and TestRunProxyConfigRepair (repaired before spawn; --config= form; no-config skip; trailing-flag safe + repair not invoked).
• Full backend suite: 40,276 passed; the 78 residual failures were re-run against pristine origin/main on the same host — identical failure sets (host forbids user namespaces → SandboxUnavailableError; zero net-new failures).
• isort / flake8 clean; mypy: 0 errors in touched files.

Review fleet

Pre-push model-pinned reviewers: GPT 5.6 Sol (3 findings — mode preservation, symlink clobber, lost-update race) and Opus 5 (0 blocking, 4 advisories — relative-path vantage, --config= form, mode preservation, test strengthening). All applied; the interprocess-lock suggestion for the race was scoped down to the read-recheck stand-down documented above, since no lock exists for this file today and adding one across all generation paths exceeds this fix's blast radius.

Closes #2491

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed ccc0fe42b4aa281a9d3865cf9feefb815f5c0e25 — this comment is updated in place on each push.

Review details

I've reviewed the diff thoroughly, including the lock helper, the repair logic, sensitive-path handling, all callers of generate_playwright_config, and the blocking-rule concerns.

Key checks:

• Event-loop blocking (no-blocking-call-on-event-loop): generate_playwright_config now acquires a blocking flock. Its only async caller (_persist_browser_preferences in messaging.py:2409) dispatches through asyncio.to_thread; the other callers (cli.py, cli_setup.py) are pre-loop. repair_playwright_config runs in the separate proxy process (run_proxy), not the gateway loop. No loop stall introduced.
• Sensitive-path keystone (backend-security-controls): ~/.kiro/crew/playwright-config.json is not in _CREW_SECRET_LEAVES, so the normal path proceeds; a hand-edited --config into .ssh/etc. is refused before lock/read. The sidecar O_NOFOLLOW + samestat re-check correctly blocks a planted symlink from forging a keystone flag.
• Symlink write-through, relative-path vantage, mode preservation, manual-edit stand-down are all handled coherently and match the atomic-write / lock semantics.
• Circular import at mcp_playwright_proxy.py is annotated and matches the top-level-imports exception.
• Repair converges on fresh-generation output; idempotent; degrades to unauth context rather than crashing.

No semantic defect on changed lines completes a consequence chain to user- or system-visible harm.

No findings.

[OPUS-REVIEWED] ccc0fe4

Verdict parsed from the review's SHA-scoped output markers for commit ccc0fe42b4aa281a9d3865cf9feefb815f5c0e25.

False positive or not applicable? A repository writer can comment:
/ai-review override fable ccc0fe42b4aa281a9d3865cf9feefb815f5c0e25: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of ccc0fe42b4aa281a9d3865cf9feefb815f5c0e25 — updated in place on each push; does not block merge.

Design review complete. I examined the full diff (the repair helper, the new interprocess lock, the proxy-startup hook, and both test suites) against the stated regression.

Design-Verdict: PASS

Load-time self-heal at the single spawn chokepoint is the right layer; repair converges with generation semantics, and every hardening layer traces to a named failure.

[DESIGN-REVIEWED] ccc0fe4

[github-actions]

GPT 5.6 Review — ✅ human override accepted

Human judgment by @bolichen97 overrides the GPT 5.6 finding for ccc0fe42b4aa281a9d3865cf9feefb815f5c0e25; the recorded reason is authoritative for this commit.

This comment is updated in place on each push.

The model was not re-run because an authorized human decision supersedes it.

False positive or not applicable? A repository writer can comment:
/ai-review override gpt ccc0fe42b4aa281a9d3865cf9feefb815f5c0e25: <one-sentence reason>

[bolichen97]

Re: GPT 5.6 BLOCKING — lost concurrent updates (setup.py read-modify-write)

Finding accepted, but fixed forward rather than reverting the persistent rewrite: dropping the rewrite would reopen the exact trap this PR closes (the running playwright-mcp process caches the config, so an in-memory-only repair leaves the on-disk ENOENT bomb armed for every restart — spec item 2 of the issue).

Fix applied in 7e54638: a shared interprocess advisory lock (_playwright_config_locked, flock-sidecar pattern lifted from _kiro_mcp_locked in the same module) now serializes both writers of playwright-config.json:

• generate_playwright_config() takes it around its write (all call sites are off-loop: Settings saves run via asyncio.to_thread under _get_config_lock, browse setup/cli_setup are pre-loop CLI),
• repair_playwright_config() runs its entire read-validate-write under it (proxy startup, pre-loop process).

The repair can no longer install a stale snapshot over a concurrent generation — the Settings-save scenario in the finding now blocks on the lock and whichever side runs second sees the other's completed write (a fresh generation carries no dangling key, so the repair no-ops). Two new tests pin the lock: write-inside-locked-region ordering for the repair, and lock acquisition by the generator.

[bolichen97]

Re: GPT 5.6 round-2 BLOCKING x2 (symlink interactions) — both fixed in 64c9d77

1. Symlinked configs use different lock files — accepted. _playwright_config_locked now resolves the config path before deriving the .lock sidecar, so a writer addressing the link and one addressing the target converge on one lock. Pinned by test_link_and_target_share_one_lock_sidecar.

2. Symlink resolution changes the relative storage-state base — accepted. The repair captures the lexical config directory (the link's dir, i.e. the config path AS GIVEN to Playwright) before resolving for write purposes, and judges a relative storageState from that base. A valid state file beside the link is no longer misjudged as missing from the target's directory. Pinned by test_relative_state_beside_symlink_is_kept.

Local gates re-ran green (450 tests across the three touched/adjacent files, isort/flake8/mypy clean).

[bolichen97]

Re: GPT 5.6 round-3 BLOCKING (sensitive-path guard) — fixed in 8410e1c

Accepted as defense-in-depth. repair_playwright_config() now refuses a resolved path matching security.is_sensitive_path() before locking or reading — notably the lock helper creates a .lock sidecar next to the target, so the guard precedes that write too. In the normal flow the --config argv is Kiro Crew's own fixed registration (config_dir()/playwright-config.json), but a hand-edited mcp.json could point the proxy anywhere; the repo rule is that file access of externally-influenced paths must respect the sensitive-path contract.

Pinned by test_sensitive_path_refused_before_lock_or_read (asserts refusal, byte-identity, and no sidecar creation inside the credential dir). Gates green: 159 targeted tests, isort/flake8/mypy clean.

[bolichen97]

Re: GPT 5.6 round-4 BLOCKING (unresolvable config crashes the proxy) — fixed in ac127ed

Accepted: path.resolve() is now wrapped, returning False on OSError (ELOOP from a symlink loop) or RuntimeError — an unrepairable config is Playwright MCP's own error to report, same posture as an unparseable one. The proxy always reaches its spawn. Pinned by test_symlink_loop_is_safe_noop.

Note on Backend Tests (3.10, 2): the shard failed on two DIFFERENT tests across two runs (test_a_sharp_s_case_variant_inserts_rather_than_enriching, then test_distinct_words_differing_only_by_sharp_s_are_not_conflated) in test_lesson_contradiction.py — untouched by this PR. That test class builds one-hot embeddings via seedless hash(t) % 384, so two rules can collide into identical vectors depending on the process hash seed, which makes it intermittently order-of-similarity-flippable. Unrelated to this diff; the new push re-runs the shard. If it keeps flaking I'll file it as a follow-up flake issue rather than chase it here.

[bolichen97]

Re: GPT 5.6 round-6 — decode-error accepted (fixed in 62ab731); lock-removal rebutted

1. Uncaught decode error on the pre-write re-read — accepted. A concurrent save of non-UTF-8 bytes now counts as "changed": the repair stands down instead of raising UnicodeDecodeError pre-spawn. Pinned by test_concurrent_undecodable_save_stands_down_without_crash. (The "or be overwritten" half of the finding is the window the re-read already narrows to microseconds under the lock; a byte-compare cannot shrink it further without fs transactions.)

2. Remove _playwright_config_locked and its call sites — rebutted: this contradicts this bot's own round-1 adjudication on this PR. Round 1 (head 86fcbdf) blocked with: "Serialize all playwright-config.json read-modify-write/generation paths with one interprocess lock, then reread and validate under that lock before repairing" — the lock layer IS that fix, and round 2-5 reviews accepted it while probing it (symlink sidecar convergence, round 2). The PR-intent clause it cites ("do not restructure the browser setup flow") is from the issue's blast-radius note, which is about not touching process-killing/transport-restart behaviour — a 10-line lock helper in the same module, following the existing _kiro_mcp_locked sidecar pattern, is not a setup-flow restructure. Removing it would reintroduce the exact lost-update defect round 1 blocked on.

If the round-6 lock-removal position is maintained on the next pass, I'll treat it as adjudicated and use the override mechanism with this comment as the reference.

[bolichen97]

Re: GPT 5.6 round-7 — both items addressed in 4d7d06a

1. Lock sidecar follows symlinks into security keystones — accepted (same agent-plantable-destination class as prior audits). The sidecar is now opened with os.open(O_RDWR|O_CREAT|O_NOFOLLOW, 0o600) and an fstat regular-file re-check (covering platforms without O_NOFOLLOW): a pre-planted symlink is refused at the kernel with ELOOP, the repair degrades to a safe no-op, and generate_playwright_config fails loudly rather than writing unlocked — writing without the guard would defeat it. Pinned by test_planted_sidecar_symlink_is_refused_and_target_untouched (asserts the planted link's keystone target is neither created nor truncated).

2. PR intent doesn't describe the lock — accepted: the PR description's hardening section now documents the lock layer (it had gone stale from the pre-round-1 design). Description updated in place.

Gates green: 455 tests across touched + adjacent handler files, isort/flake8 clean, 0 mypy errors in touched files.

[bolichen97]

Re: GPT 5.6 round-8 BLOCKING (Windows follows planted sidecar symlink) — fixed in ccc0fe4

Accepted: Windows has no O_NOFOLLOW and Developer Mode allows unprivileged symlink creation, so the POSIX-only kernel refusal left a real gap there. When O_NOFOLLOW is unavailable the helper now (a) refuses a symlink via lstat BEFORE opening, and (b) re-verifies the opened fd's identity against the directory entry AFTER opening with os.path.samestat — closing the lstat→open swap window the naive pre-check alone would leave. The existing fstat regular-file check remains for all platforms. Pinned by test_planted_sidecar_refused_without_o_nofollow (simulates the no-O_NOFOLLOW path by deleting the constant). 164 targeted tests green, lint clean.

[bolichen97]

Re: GPT 5.6 round-9 BLOCKING (recheck/replace not atomic) — rebutted + overridden as re-litigation

Adjudication history on this exact residual:

• Round 1 blocked on lost updates and prescribed: interprocess lock + reread and validate under that lock → implemented (_playwright_config_locked, read-validate-write inside it).
• Round 5 blocked on the unlocked-manual-edit case and prescribed: re-read path immediately before writing and return if contents differ → implemented verbatim.
• Round 6 demanded reverting the rewrite; rebutted (contradicts round 1 and the issue's binding requirement that the repair persist to disk — the cached-config trap is the bug). I stated the override would follow if the position was maintained.
• Round 9 (this one) demands revert the persistent repair until replacement can be conditional on the original snapshot.

Why the demand is not implementable: no portable OS primitive provides atomic compare-and-swap on file CONTENTS (POSIX rename is atomic on the name, not conditional on the prior bytes; Windows likewise). Every lock-taking writer is already fully serialized; the only remaining writer class is an unlocked manual editor, whose save can land in the microseconds between recheck and rename — and that same editor can equally overwrite the config one microsecond AFTER any "conditional" replacement completes, so the demanded construct would not change the reachable outcomes. The residual is: an operator hand-editing the config at the exact instant of a proxy launch on a machine whose config ALSO carries a dangling storageState — and the loss is bounded to that one save, recoverable by re-saving.

Reverting persistence instead would reopen #2491's core defect for every user (the running playwright-mcp process caches the config; an in-memory repair dies with the process).

Override follows per the repo's adjudication mechanism.

[bolichen97]

/ai-review override gpt ccc0fe4: Round-9 revert demand re-litigates rounds 1/5/6 — the lock + pre-write recheck it prescribed are implemented, atomic content-conditional replacement does not exist portably, and reverting persistence would reopen #2491's cached-config trap (see adjudication-history comment above).

[github-actions]

Human judgment recorded

@bolichen97 marked the gpt AI finding as false positive, not applicable, or explicitly accepted for ccc0fe42b4aa281a9d3865cf9feefb815f5c0e25.

Round-9 revert demand re-litigates rounds 1/5/6 — the lock + pre-write recheck it prescribed are implemented, atomic content-conditional replacement does not exist portably, and reverting persistence would reopen #2491's cached-config trap (see adjudication-history comment above).

This decision applies only to this commit. A new push requires a new judgment.