refactor(pod): drop dead code and flatten four pod-module branches

[bolichen97]

Why no screenshot: backend-only diff — four files under src/kiro_crew/pod/, no frontend path touched, no rendered surface exists for the pod CLI.

Problem / Motivation

src/kiro_crew/pod/ carries the residue that accumulates in a subsystem grown
over many PRs: a function nobody calls, two values computed twice, three
control-flow shapes that nest or branch further than the logic needs, and
comments that read as a task log rather than as rationale. None of it is a bug,
and all of it is what a reader has to hold in their head before they can reason
about the destructive teardown path this module owns.

Three of those comments also make claims that are false of the current code,
which is the part that actually costs something: a reader who trusts them
mis-models when the pod's isolated HOME can be deleted.

Why it matters

This module deletes directories (pod down reclaims a pod's isolated HOME) and
gates pod exec behind a deny-by-default allowlist. Both are paths where a
reviewer has to be able to see the decision at a glance, and where a comment
that names the wrong code path sends the next author to the wrong guard. The
duplicated port derivation is also a live cost: each one shells cksum, so
pod ls --json spawned two subprocesses per pod for one answer.

What changed (motivation → approach → change)

Behaviour-preserving throughout — this is a readability pass, not a fix. Every
rewrite was proved equivalent before it landed (see Tests).

Dead code

• launchd.restart() removed. Zero callers anywhere in src/, test/,
  scripts/, docs/, website/, packaging/ or conftest.py, including
  dynamic access; there is no pod restart verb (_VERBS has no entry,
  cli.md documents none) and Dev Fleet's /api/pod/restart is down then
  up, not a kickstart. The systemd twin unit.py has no counterpart, so the
  two backends are now symmetric.
• require_pod_safe_verb's hint = "" initializer removed — the following
  if/elif/else is exhaustive, so no path could observe it. The guard's
  decision is byte-for-byte unchanged.

De-duplication

• _ls --json derived each pod's port twice per row (once for the reported
  value, once as the health probe's argument). Now once. Beyond halving the
  cksum subprocesses, the reported port can no longer differ from the one
  actually probed if a PORT= pin changes between the two calls.
• _prune computed the failed count standalone and inside the counts
  comprehension; it now reads the one it already built.

Flattening

• read_env_file's try / if exists() / for nest becomes a guarded read
  plus one flat loop. read_text() raises the same OSError family
  Path.exists() swallows, so the pre-check was redundant, and dropping it
  closes a check-to-use window rather than opening one. read_text() is
  all-or-nothing, so the old code's partial-result path was unreachable.
• _ls's empty-list early return becomes an if/else, so _print_orphans —
  how an operator learns about un-reclaimed pod HOMEs — is called from one
  place instead of two.
• unit_is_current's nested double comprehension becomes one pass:
  str.startswith takes the whole tuple. _REMOVED_DIRECTIVES is now
  annotated tuple[str, ...] so that requirement is stated where it is relied on.

Comment hygiene (only in the files above, per AGENTS.md § Code style)
Dropped review-round markers, converted historical narration to present tense,
and corrected three claims that were false of the current code:

• The stale-unit hazard is skipped by start_pod, not by down.
  unit_is_current has exactly one caller (runtime.py:614); stop_pod gates
  on loaded_teardown_hook, i.e. on what systemd has loaded, so it cannot
  skip its refresh because of a stale file. Corrected in both places that said
  otherwise (_write_and_load_unit and install_backend).
• read_env_file is fail-open, so the docstring now bounds who may use it
  and points at dev_fleet._read_pin_strict, which exists precisely to refuse
  the absent-vs-unreadable conflation.
• launchd.stop's docstring said "two things" above three bolded items.

No security control changed shape: no sensitive-path matcher, denied-command
rule, deny-by-default guard, SEL audit emit, redaction call, credential scrub
(build_pod_env) or path-containment check (cleanup_home) is in the diff. No
import was added, removed, moved or hoisted, and no in-function
from X import Y was touched.

Tests

No test changes — that is the point of a behaviour-preserving pass, and the
existing suites are what hold it. 1097 passed, 2 skipped across every suite
that imports kiro_crew.pod: test_pod.py, test_pod_launchd.py,
test_pod_self_contained.py, test_pod_e2e_harness_paths.py,
test_pod_e2e_video_guard.py, test_agent_home_isolation.py,
test_cli_commands_coverage.py, test_dev_fleet_server_coverage.py,
test_dev_fleet_node_toolchain.py, test_dev_fleet_app.py.

Equivalence was also proved directly, not just observed green. read_env_file
was differentially executed old-vs-new over 12 inputs (missing file, empty,
comment-only, no-=, quoted values, path-is-a-directory, mode-0, unreadable
parent, dangling symlink, ELOOP, ENAMETOOLONG, invalid UTF-8) — identical results in every case, including which exception escapes. _prune's count was
brute-forced over 2000 random result sets including unknown statuses.
startswith(tuple) was checked against the nested form including the
empty-tuple case.

One input class where old and new differ, deliberately accepted as unreachable:
a pod name containing a NUL byte. Path.exists() swallows ValueError, while
read_text() raises it. _NAME_RE (^[a-zA-Z0-9][a-zA-Z0-9._-]{0,60}$) gates
every validated path, argv cannot carry NUL, and the Dev Fleet call sites only
reach it for a name already in rt.active_names.

Manual verification

N/A — unit coverage sufficient. The changed code is pure control flow and
string handling with no new external interaction; the platform-specific paths
(systemctl, launchctl) are exercised by the suites above on every OS via
their existing seams, and this diff adds no call to either.

Related Issues

no linked issue: campaign refactor with no filed issue behind it.

Checklist

• Single commit with a Conventional Commits title
• Existing tests pass and new tests added for new functionality (none needed — no new behaviour)
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A: no documented API, schema or behaviour changed
• No secrets, credentials, or internal references in the diff

Gates run locally (Python 3.12 CI-parity venv)

flake8 · isort --check-only · mypy src/kiro_crew (982 files) ·
check_brand_name.py · check_harness_parity.py · docs_lint.py --test ·
docs-lint.sh · check_per_file_coverage.py --test ·
verify_vendor_manifest.py · scrub-lint.sh --no-history — all exit 0.
Frontend suites not run: the diff touches nothing under website/, .github/
or scripts/, so CI's changes job reports only_backend=true.

Pre-PR review fleet

Five independent read-only reviewers ran against this diff before it was
opened, each with a distinct lens: AUTOSDE blocking rules + the code-review.yml
grep contract, behaviour preservation, import semantics and test-patching seams,
security-guard/destructive-path integrity, and comment accuracy.

Found and fixed (3, all comment accuracy): the unit_is_current-victim-path
error, converged on by two reviewers and confirmed by reading the call sites;
the read_env_file docstring's false "indistinguishable to every caller" claim,
converged on by three reviewers with dev_fleet._read_pin_strict as the
counter-example; and the over-promised "{} when unreadable" summary, which
UnicodeDecodeError escapes. Also folded in two adjacent instances of the same
banned comment classes so the fix is not applied inconsistently within a file.

Dropped after checking (2): the NUL-byte ValueError divergence, verified
unreachable and recorded above rather than guarded speculatively; and a
suggestion to catch ValueError in read_env_file, which would be a behaviour
change and therefore belongs in its own PR, not a preservation pass.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

All hunks verified against the description: launchd.restart genuinely has zero callers (the only kickstart uses live in service/ and dev_fleet/, which have their own launchctl wrappers), the _ls and _prune de-duplications are behavior-preserving, the read_env_file flattening keeps the same OSError-swallowing contract with the documented NUL-byte divergence correctly argued unreachable, and the comment corrections match what the code actually does (unit_is_current's one caller is start_pod, not down). Description ↔ diff fidelity is exact in both directions; no smuggled behavior change.

Design-Verdict: PASS

A behavior-preserving cleanup of a destructive-teardown module, with every claimed removal and equivalence verified in the code — no design-level concerns.

[DESIGN-REVIEWED] 7d156e4

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 7d156e417418b45f545e7ef872b1fb01e8ac2964 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 7d156e4

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

All hunks verify as behavior-preserving. launchd.restart() has no callers (the controller.py references are linux.restart/macos.restart, unrelated modules). require_pod_safe_verb sets hint in all three branches after the early return, so removing the hint = "" prelude leaves no unset path. _ls prints orphans in both branches. read_env_file, _prune, and _REMOVED_DIRECTIVES are all equivalent transforms.

No findings.

[OPUS-REVIEWED] 7d156e4

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

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Advisory premise-level review of 7d156e417418b45f545e7ef872b1fb01e8ac2964 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

All evidence checked: I verified launchd.restart has zero callers, unit_is_current has exactly one caller (start_pod), the if/elif/else in require_pod_safe_verb is exhaustive, and the comment-hygiene rules are mandated by AGENTS.md. Final review follows.

First-Principles-Verdict: PASS

A pure subtraction pass: every item deletes code, a duplicate computation, or a false claim, and each deletion's justification checked out against the tree.

What this change ships

Intent: make the pod module's destructive-teardown code readable and its comments true — a FIX (of comment falsehoods) plus pure deletions, no new capability.

1. launchd.restart deleted — justified: zero consumers (grepped restart(cfg/kickstart repo-wide; Dev Fleet's /api/pod/restart is down-then-up, test_dev_fleet_app.py:4094 asserts no kickstart).
2. pod ls --json derives each port once, not twice — justified: measured cost, one cksum subprocess per pod saved.
3. _print_orphans called from one site via if/else — justified simplification.
4. _prune reads failed from counts it already built — justified dedup.
5. read_env_file drops the redundant exists() pre-check, gains a fail-open docstring pointing at _read_pin_strict — justified; declared NUL divergence is gated by _NAME_RE.
6. Dead hint = "" initializer removed — justified: the following if/elif/else is exhaustive (verified runtime.py:1325-1334).
7. unit_is_current one-pass startswith(tuple) — justified.
8. Stale-unit comment now names start_pod, not down — justified: unit_is_current's only caller is runtime.py:618 inside start_pod; I verified this.
9. Review-round markers ("verifier-found High", "review-blocking") stripped — derived from AGENTS.md § Comments; no siblings remain in src/ (grepped, 0 hits).
10. stop docstring "Two things" → "Three" — justified: three bolded items follow.

[FIRST-PRINCIPLES-REVIEWED] 7d156e4

[bolichen97]

The red Backend Tests (3.12, 4) is not this diff — evidence

Backend Tests (3.12, 4) and its downstream Coverage Gate are red, and I want
to save the next reader the attribution work. Both are pre-existing repo-wide
failures in test/test_slack_options_lifecycle.py::TestLinkTimeBackfill, a class
this diff cannot reach: the whole change is four files under src/kiro_crew/pod/
with no symbol shared with the Slack or turn-deadline paths.

A different member of that class fails on each run of the identical tree. On
the first run of 7d156e4 it was test_replayed_options_are_a_control_not_literal_text
(AttributeError: 'NoneType' object has no attribute 'args' at line 785 —
post_blocks was never awaited). I re-ran the same job on the same commit with
no changes, and it failed test_only_the_newest_reply_is_answerable instead
(assert 0 == 2). A failure that moves between siblings on an unchanged tree is
shared-state pollution under xdist, not a defect in the code under test — and
the same shard logs ValueError: <Token var=ContextVar 'kirocrew_turn_deadline'> was created in a different Context, which is the leak doing it.

Three unrelated heads red the same shard right now:

head	red jobs	same class?
main (run 32029075186)	Backend Tests (3.10, 4), (3.12, 4), Coverage Gate	same shard
fix/redos-linearity-nonwallclock-4108 (32032049872)	Backend Tests (3.12, 4), Coverage Gate	yes — same AttributeError in TestLinkTimeBackfill, lines 818 and 842, with the same kirocrew_turn_deadline cross-context ValueError
feat/session-control-mcp (32029833750)	Backend Tests (3.12, 4), Coverage Gate	same shard

Coverage Gate is purely downstream: its log shows BE: failure and it fails
closed on that alone (it also confirms ONLY_BACKEND: true for this diff, as the
description claims).

Locally on this branch, the named test passes in isolation, the whole
test_slack_options_lifecycle.py file passes (78 passed), and 1097 tests pass
across every suite that imports kiro_crew.pod.

Disposition: rebutted — not caused by this PR, and deliberately not fixed here.
TestLinkTimeBackfill's shared-state leak is a real bug worth fixing, but it is a
different module and fixing it here would smuggle a behaviour change into a
behaviour-preserving cleanup and break the one-module-per-PR rule this campaign
runs on. It needs its own PR. I have not weakened, skipped, or retried any
assertion to get around it.

[bolichen97]

Follow-up on the red shard: the fix already exists as #2848

Updating my own attribution comment above with something it was missing — the
kirocrew_turn_deadline leak I named as the cause is not just "a real bug worth
fixing in its own PR", it already has one open: #2848,
fix(chat): restore the turn deadline by value, not by ContextVar token.

I verified the linkage rather than assuming it. turn_dispatch.py:310 takes a
Token from _TURN_DEADLINE.set(...) and line 331 calls
_TURN_DEADLINE.reset(deadline_token) in a finally. #2848 replaces exactly
that pair with save-and-restore (previous_deadline = _TURN_DEADLINE.get() …
_TURN_DEADLINE.set(previous_deadline)), because the finally of an abandoned
wrapper is run by the garbage collector from whatever context happens to be
current — and Token.reset refuses that with the
ValueError: ... was created in a different Context this shard logs. Since it
raises inside __del__ it is unraisable, so it surfaces as a
PytestUnraisableExceptionWarning rather than a catchable error, which is why it
lands as an unrelated sibling's post_blocks never being awaited instead of as a
failure in the code that caused it.

This does not change my disposition, it sharpens it. #4121 is red on a defect
that is diagnosed and already has a fix in flight in the module that owns it. The
correct order is #2848 first; this PR then goes green with no change to its own
diff. I re-ran Backend Tests (3.12, 4) once more on the unchanged tree in case
the pollution missed the class this time, and I have still not weakened, skipped
or relaxed any assertion.

Worth flagging for prioritisation: this shard is currently red on main itself
and on every branch that draws the unlucky interleaving, so #2848 unblocks more
than just this PR.

[bolichen97]

Green — the re-run confirms the attribution

Backend Tests (3.12, 4) passed on the unchanged tree (7d156e4, no new
push), and the downstream Coverage Combine → Coverage Gate cascade went green
with it. All 58 checks now pass and PR Readiness is green.

That closes the attribution argument rather than merely working around it: the
same commit that failed twice — on two different members of
TestLinkTimeBackfill — passes on the third run with not one byte changed. A
result that varies across runs of an identical tree cannot be caused by the tree,
which is what the shared-state leak diagnosis predicted.

To be explicit about what was and was not done: nothing in this PR's diff changed
to reach green, and no assertion was weakened, skipped, marked flaky, or retried
in code. The only action was re-running the one polluted job. The underlying
kirocrew_turn_deadline leak is still real and still worth landing — #2848 fixes
it in the module that owns it, and until it lands this shard will keep reddening
main and any branch that draws the unlucky interleaving.

[iamwhatever]

Tier 1 auto-approve: refactor (4 files). Criteria: no conflict, no requested changes, no security surface, AI reviewers green. Category: drop dead code and flatten four pod-module branches across pod/cli.py, launchd.py, runtime.py, unit.py -- net-negative internal cleanup with no behavior or interface change.