fix(connections): migrate the materialized pre-launch opt-out

[pepmach]

What

One-shot migration for the launch upgrade trap: pre-launch builds materialized connections_ui: false into every config.json they saved, and after #9149 that stale key reads as the deliberate opt-out — every upgrading install boots with a silently empty Connections gallery. (Hit the project owner's own gateway on launch day; diagnosis was manual key deletion.)

How

The two readings of false are byte-identical; what distinguishes them is when the key was written. A sidecar boundary marker (connections_ui_migrated.json, beside config.json — same placement rationale as the superseded-defaults ack file) records the first post-launch load:

• No marker + stored false → stale noise: stripped as a delta through the existing _persist_config_migration machinery (re-checked against the on-disk document inside the write lock, other keys untouchable), in-memory value corrected the same load, marker recorded.
• No marker + stored true → the only deliberate pre-launch act: never touched; marker recorded.
• Marker present + false → the deliberate post-launch opt-out: honored, forever.
• Degraded load → skips both strip and marker, retries on the first clean load (matches the existing fix(publish): a malformed publish section silently reopens the destination allowlist #4057 write-back gate).
• Contended lock / already-migrated-by-another-writer → _persist_config_migration returns False with nothing written; the marker defers WITH the strip (gated on the persist outcome) and the whole migration retries on the next load. Marker-without-strip would freeze the stale false as a deliberate opt-out forever.

Why not the superseded-defaults registry

That mechanism (report, never rewrite — #5244's lesson) exists for keys where stale-default and deliberate-choice are permanently indistinguishable. Pre-launch connections_ui: false had no deliberate meaning at all — the flag was the opt-in gate and false was both default and only behavior — so a bounded rewrite with a recorded boundary is sound where the generic rewrite was not. The one lossy edge: a user who upgraded in the launch→migration window and deliberately opted out gets flipped on once and must opt out again; the migration cannot distinguish that write, and the population is bounded by the window.

Verification

• New truth table in test/test_connections_ui_flag.py (red-first): strip-once with delta isolation (auto_update survives untouched), marked false honored, true untouched, absent-key marker-only, second-load no-op, contended-persist defers the marker with the strip — suite 15/15.
• Existing opt-out test updated to the marked semantics (the honest post-migration state of every install).
• test_config_loader.py + test_config_baseline.py + test_config_meta_stamp.py: 529 passed (no schema change — the marker is a sidecar, so no baseline regeneration).
• test_config_superseded_defaults.py: 46 passed (registry untouched).
• test_sandbox_governance_mask.py + test_security_posture.py: 588 passed (new data-home leaf is a non-sensitive sidecar, same class as the ack file).
• isort / flake8 / mypy / black gate clean.

Pattern harvest

Rule candidate: semgrep — a boolean-returning
persistence/write helper called for its side effect with the return value
discarded — _persist_config_migration(...) already encoded
"deferred, nothing written" as False, and the defect was ignoring it while
writing a dependent durable record (the marker). The general shape: any
"record that step X happened" write must be gated on step X's own reported
outcome, not on having reached the code path that requested it. Same family
as fsync-then-rename ordering bugs; a semgrep rule flagging bare-statement
calls to known bool-returning writers would have caught this at authoring
time.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 3244da33454fb321cd7a23e812efa140cfb41a59 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

A real launch-day trap, fixed at the true ambiguity (when the byte was written), with a bounded one-shot boundary, gated durably on the persist outcome, and every failure path pinned.

[DESIGN-REVIEWED] 3244da3

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 3244da33454fb321cd7a23e812efa140cfb41a59 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks complete. My verification confirmed: the superseded-defaults registry deliberately never rewrites (recorded decision at superseded_defaults.py:100-106) so it isn't a duplicate mechanism; the meta.lastTouchedVersion stamp can't serve as the boundary because refresh_config_meta_stamp re-stamps every gateway start regardless of whether the strip landed, which would freeze a deferred strip; the marker's universal write is required so a fresh install's later deliberate false survives; and the marker payload fields have zero readers (grep stripped_stale_false|migrated_at: defining site only, tests use {}).

First-Principles-Verdict: PASS

Verify the #9149-era premise: no pre-launch surface ever let a user deliberately write connections_ui: false — every stripped key rests on that history claim.

What this change ships

Inventory (5 items) — 5 justified

Intent: make upgrading installs stop booting with a silently empty Connections gallery caused by pre-launch builds materializing connections_ui: false — a FIX.

1. Upgrading installs with a stale stored false boot with the gallery on; the key is stripped from config.json once — justified
2. A connections_ui_migrated.json sidecar appears beside config.json in every data home after the first post-launch load — justified
3. A stored false found after the marker exists is honored as the opt-out forever — justified
4. A deliberate launch-window opt-out is flipped on once and must be re-set (declared, bounded loss) — justified
5. On a degraded or lock-contended load, strip and marker both wait for the next clean load — justified

Subtractions

• Shrink the marker payload to {}: migrated_at and stripped_stale_false have zero readers (grepped stripped_stale_false|migrated_at repo-wide — defining site loader.py:3965-3966 only; the tests already treat {} as a valid marker). Only .exists() is consumed.

[FIRST-PRINCIPLES-REVIEWED] 3244da3

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

One advisory finding — nothing blocks: the connections_ui launch migration decides its in-memory override on the merged base+overlay document, contradicting its own "BASE document" comment.

FINDING — src/kiro_crew/config/loader.py:3916 — if data.get("connections_ui") is False: reads the MERGED document (data = _deep_merge(data, local_data) at loader.py:2558), so a deliberate overlay opt-out (connections_ui: false in config.local.json, base key absent or true) forces cfg.connections_ui = True while the base-only strip in _apply_document_migrations finds nothing to remove → _persist_config_migration returns False → persisted is False → the marker is never written, so on a stable install (no other pending migration) every boot re-overrides the opt-out and the migration never converges → Fix: decide the in-memory half on the pre-overlay base value (the base_shadow already captured at loader.py:2557), not merged data, so an overlay-set connections_ui is left alone.

[OPUS-REVIEWED] 3244da3

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 3244da33454fb321cd7a23e812efa140cfb41a59 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/config/loader.py:3916 -- merged data.get("connections_ui") overrides an overlay-only false, enabling the gallery on every load -> Fix: derive the migration predicate from the base document only.
[GPT-REVIEWED] 3244da3

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

[pepmach]

Disposition: FIXED — span=f88546c82bde (marker written on lock-contention deferral)

Correct catch, and the exact failure mode named (marker-without-strip freezes the stale false as a deliberate opt-out forever). Fixed as prescribed: the marker write is now gated on _persist_config_migration's existing return value whenever the load decided any migration (persisted captured at the call site; the marker branch requires connections_migrating and persisted and not cfg._degraded_sections). The already-migrated-by-another-writer path also returns False; the marker then lands on the next load, which finds nothing to strip — one extra boot, same endpoint, documented at the branch.

Contended-lock test added red-first: test_a_deferred_strip_defers_the_marker_too monkeypatches the persist to the BlockingIOError-swallowing outcome (return False) and pins that on-disk false survives, the marker is absent, and the strip was pending — red on the judged head, green on 3244da33454fb321cd7a23e812efa140cfb41a59 with the full battery (flag suite 15/15, loader 529, superseded-defaults 46).

[pepmach]

Disposition: FIXED — span=964a6ea5f43f (loader.py:3954 — marker write ignores _persist_config_migration's bool return)

Same root defect as the design lane's watch item, fixed once: the persist call's return value is captured (persisted) and the marker branch is gated on it, so a BlockingIOError-deferred strip (nothing written, returns False) defers the marker with it and the whole migration retries on the next load. Pinned red-first by test_a_deferred_strip_defers_the_marker_too; fix verified on 3244da33454fb321cd7a23e812efa140cfb41a59 (flag suite 15/15, static gates clean).

[pepmach]

CI triage (head 3244da33454fb321cd7a23e812efa140cfb41a59): Backend Tests (Windows) (4) failed twice on test_work_ledger::test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding — inherited, not this PR's diff (a config-loader migration; zero ledger overlap, Linux shards green, and the same failure hit #9149 pre-dating this change). The #9250 per-thread records identify a real Windows-only product bug: a concurrent bind intermittently rejects a valid worker key as path traversal — diagnosed and tracked as #9612. Rerunning the shard for this round; the ~1-in-N race passes on most attempts.

[iamwhatever]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: root cause is that pre-launch builds materialized connections_ui: false into every saved config as default noise, and post-launch the same bytes read as a deliberate opt-out; a one-shot marker-gated migration strips exactly that stale false once, leaves true alone, and honours every later false forever.

[chenmingwei23]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix - one-shot config migration stripping pre-launch materialized connections_ui:false so it is not misread as a post-launch opt-out; root cause clear, config/loader.py plus its test only.

[dwu96]

Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: fix with a clear root cause — pre-launch builds materialized connections_ui: false into every saved config, so post-launch that byte pattern read as a deliberate opt-out; a one-shot sidecar marker strips exactly the stale false once and honours every later false, config-loader migration plus its tests only.