feat(goal-conductor): durable inbox and dispatch record

[iamwhatever]

Problem

The goal-conductor skill holds two facts nowhere but in the conductor's own
prompt and context:

1. A user message that arrives mid-round. The skill's "Goal changes
   mid-flight" section told the conductor to remember the message and apply it at
   the next round boundary. Remembering is context, and context is what
   compaction drops. The user typed a steering instruction and gets no
   acknowledgement that it was lost.
2. Whether an item was already dispatched. Dispatch is two MCP calls —
   session_create then session_send — ordered only by prose ("send the seed
   BEFORE recording the ledger row"). A turn lost between them leaves a session
   with no seed, and the next patrol cycle cannot tell that from a session that
   is merely quiet. The recovery a conductor actually reaches for is a second
   session_create, which is how one item becomes two sessions.

Why it matters

Both failures are silent and both are user-visible. The first discards
something a human typed. The second doubles work under a lost turn — the exact
condition a long-running conductor hits most, since it deliberately ends its turn
and comes back through monitor_start.

Fix

Symptom: a mid-flight message and a half-finished dispatch both live only in
context.

Root cause: the skill has no durable store. accept_eval.py holds the
acceptance verdict and ledger_entry.py holds the entry format, but nothing
holds the queue, so the prose had to carry it.

Change: a third bundled script, scripts/dispatch_queue.py, owning one state
file per goal at <data_home>/conductor/<goal>/queue.json. Seven modes:
enqueue, claim, done, release, dispatch_begin, dispatch_sent,
status. Same invocation contract as its siblings — argv mode, one JSON on stdin,
one JSON on stdout, exit 0 for "ran" and 2 for a malformed call.

The design choices that carry the properties:

• A claim does not delete. It marks and timestamps. A turn that dies after
  claiming leaves the message on disk, and a later claim past the staleness
  window re-serves it with a reclaimed count. Only an explicit done removes
  it.
• A full inbox refuses. At MAX_INBOX the call returns inbox_full rather
  than evicting the oldest — every entry is a message a human typed, so a
  backlog is the better failure. MAX_TEXT refuses rather than truncating, for
  the same reason: half a steering instruction can invert its meaning.
• dispatch_begin is idempotent by id. A second call for the same item
  returns the SAME dispatch id and replay: true, plus a warning naming the
  consequence when no seed was recorded yet. A fresh id per attempt is precisely
  what opens a second session.
• An unreadable state file refuses. It does not read as blank. A blank read
  would be written back by the next mode, destroying the parked messages the
  script exists to keep.
• A corrupt or structurally invalid state file is moved aside, not deleted,
  to queue.corrupt-<ts>.json, so the run continues and the bytes stay
  inspectable. Valid JSON with the wrong shape is quarantined on the same path
  rather than being substituted with a blank record. The shape check goes one
  level deep, not just the containers: {"inbox": [null]} has a list where a
  list belongs, so a container-only check accepts it and every mode then calls
  entry.get(...) on a non-mapping and dies with an uncaught AttributeError —
  the exact crash the quarantine exists to prevent.
• Writes are all-or-nothing, and a successful one is durable. os.write is
  looped until every byte lands and os.replace is skipped entirely after a
  short write, so a full disk produces a structured refusal and leaves the
  previous record byte-identical rather than publishing truncated JSON. After the
  replace, the parent directory is fsynced — fsync on the file commits the
  bytes, not the rename that points at them, so power loss between the two loses
  an enqueue the script already reported as succeeded. Best-effort by design: a
  directory is not openable on Windows and some filesystems refuse to sync one,
  neither of which is a reason to fail a write whose data is already committed.
• O_EXCL lockfile with a liveness-gated, fenced steal, not fcntl.flock —
  the backend test suite runs on Windows, and flock is not there. A stale lock
  is stolen only once its holder is proven dead; a holder that is still running
  is never stolen from, and one that cannot be judged waits for a 900s backstop.
  Ownership is fenced by a token, so a writer whose lock was stolen refuses at
  publish time instead of clobbering the newer state. The fence is checked
  twice, and the second check is the load-bearing one: checking only on entry
  leaves the whole write + fsync window — the slowest part, and exactly where
  a writer stalls long enough to look dead — unguarded, so a writer that passed
  the entry check could still resume after the steal and os.replace its stale
  record over the newer one. The re-check sits immediately before the rename,
  unlinks the temp file, and refuses with lock_lost.
• Nothing reads the record outside the lock, including status. status
  writes nothing, but _read can quarantine — so an unlocked read of old
  corruption, racing an enqueue that publishes valid state, renames that new
  state aside. A mode that only means to look still needs the lock, because its
  read is not side-effect-free.
• Every path through the acquire loop converges. A lock whose staleness
  cannot be judged times out with locked rather than spinning, and so does one
  the steal decides on but cannot carry out — a lock in a directory this
  process may not write to (a goal dir owned by another uid, a read-only mount)
  still stats fine and still looks steal-worthy, so retrying on a failed
  unlink re-decides the same steal every pass with no sleep and no deadline
  check. Only a successful unlink retries; a failure falls through to the
  deadline.
• No identity, no network, no subprocess. A script under execute_bash gets
  no verifiable identity from the gateway, so anything it read would be an
  assertion. Keeping it a plain local state machine is what makes it safe as a
  script rather than an MCP tool, and a source ratchet in the tests pins that.

Goal ids are validated against ^[A-Za-z0-9][A-Za-z0-9._-]{0,63}$, not
sanitized: a goal id names a directory, and sanitizing two different ids into one
merges two goals' queues.

Supporting edits, all queue-only:

• SKILL.md — dispatch_begin before the dispatch steps and dispatch_sent
  right after the seed; the enqueue/claim/done/release cycle in "Goal
  changes mid-flight"; the added approval cost stated in "Known limits". The
  dispatch list still OPENS with session_create, so the atomic-filing property
  session_create should accept a folder so filing is atomic with creation #6118 established is untouched.
• agent.py — the conductor prompt names three bundled scripts instead of two.

Tests

test/test_conductor_queue.py, 50 cases, one class per load-bearing property:

• TestInvocationContract — unknown mode and a JSON array on stdin are exit 2 on
  stdout, not a traceback; every mode reports state_path.
• TestGoalIdIsValidatedNotSanitized — seven rejected shapes, and ../../etc
  never reaches a path.
• TestAParkedMessageSurvives — the dead-turn case asserted directly: claim,
  die, re-claim, get the text back. Plus idempotent done, release, the
  refusal at the cap with the oldest still present, oversize refusal, and the
  corrupt-file sidecar.
• TestARetriedDispatchConverges — same id and replay: true on the second
  begin, the warning naming "no seed", no warning once sent, unsent_items in
  status, and a dispatch_sent with no begin recorded-and-flagged rather than
  refused (the send already happened; refusing would make the record less true).
• TestItTouchesNoIdentity — source ratchet on named session-key and credential
  symbols, and on subprocess/urllib/socket.
• TestUnreadableIsNotAbsent — the unreadable file refuses and the bytes are
  byte-identical afterwards; absent still reads as blank.
• TestTheLockAlwaysConvergesOnAnAnswer — a live lock times out, a stat that
  always fails still honours the deadline without stealing, a stale lock whose
  unlink keeps failing times out instead of spinning, a stale lock from a
  proven-dead holder is stolen, one whose holder still runs is not, and a
  holder that cannot be judged is stolen only past the 900s backstop. The
  liveness cases are POSIX-gated and use a genuinely reaped pid, because
  os.kill(pid, 0) is only a non-destructive probe on POSIX. The un-removable
  case asserts both halves of the bug it pins: the wall-clock stays inside the
  wait budget and the retry count stays bounded, since the failure mode is an
  unbounded retry loop rather than a slow one.
• TestAPartialWriteIsNeverPublished — a short write refuses and leaves the
  previous record byte-identical; a simulated full disk is reported as a
  structured refusal rather than a crash. A stolen lock makes the in-flight write
  refuse instead of clobbering the newer state. The fence re-check is driven by
  stealing the lock from inside a patched os.fsync, so the entry check has
  already passed when the theft lands — the only arrangement that can distinguish
  one fence check from two; it asserts lock_lost, that the newer record
  survives, and that no queue.tmp-* is left behind. Directory durability is
  asserted by recording fsyncs whose fd fstats as S_IFDIR, because a plain
  call-count assertion passes against the bug, and a companion case parametrized
  over "the directory will not open" and "it opens but will not sync" pins the
  best-effort half.
• TestStatusNeverDestroysWhatItOnlyMeantToRead — a live foreign lock makes
  status return the structured locked refusal, and the quarantine case
  asserts the invariant rather than a timing coincidence: _quarantine records
  whether queue.lock exists and carries this pid at the moment of the rename.
• test_structurally_invalid_state_is_quarantined_not_silently_dropped —
  parametrized over six malformed bodies, three of them nested (inbox: [null],
  inbox: ["a string"], dispatch: {"item-1": "abc"}); asserts the
  .corrupt-<epoch> sidecar's bytes equal the original. A sibling case drives
  status over a malformed record through the real subprocess, so a traceback
  fails the test rather than being interpreted as output.

The module runs on Windows rather than being collect-ignored, because
portability is the reason _Lock uses O_EXCL at all — excluding it would leave
that claim untested. Only the cases that need a POSIX-only primitive to mean what
they test are skipped there: 0o000 to MEAN unreadable (a mode-0 file stays
readable by its owner on Windows), and the non-destructive liveness probe (on
Windows os.kill terminates the target, so the probe returns "cannot be
judged" and the backstop path covers it instead).

Manual verification

$ pytest test/test_conductor_queue.py -q
50 passed

$ pytest test/test_builtin_skill_packaging.py test/test_builtin_skill_scope.py \
    test/test_builtin_skill_sync_safety.py test/test_conductor_agent.py \
    test/test_conductor_ledger_entry.py test/test_conductor_queue.py \
    test/test_conductor_skill.py test/test_conductor_skill_cov80.py \
    test/test_goal_command.py test/test_prepare_pr_profiles.py -q
291 passed

$ isort --check-only  # clean
$ python scripts/check_black_formatting.py
black gate passed: nothing in scope is unformatted outside the baseline
$ flake8              # clean
$ mypy src/kiro_crew/
Success: no issues found in 1289 source files
$ python scripts/check_subprocess_encoding.py       # clean
$ python scripts/check_builtin_skill_scope.py       # clean
$ python scripts/check_testpaths_coverage.py        # clean
$ python scripts/docs_lint.py                       # clean

Why the script is named dispatch_queue.py, not queue.py

The obvious name shadows the stdlib queue module. Because the script lives
inside the package tree, mypy resolved import queue in six unrelated files
(embeddings.py, cli.py and others) to this file instead of the stdlib,
producing 21 errors across the repo. The rename is what makes mypy src/kiro_crew/
green; nothing else about the script changed with it.

Relation to #6237

This is the queue half of #6237, rebased onto current main and split out on its
own. #6237 also collapsed the eight session-control verb tools into two
op-shaped tools, which #6109 has since made expensive: per-verb auto-approval now
matches on tool NAME, so collapsing the names re-opens a security decision that
is out of scope for a durable queue. That half is being abandoned; this half is
independent of it and does not touch mcp_dashboard.py, channel.py or
validation.py.

no linked issue: this is a split-out half of #6237 (a PR, not a tracked issue), and the durable-queue gap it closes was found while reviewing that PR rather than filed separately.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound durability mechanics, but the dispatch record has no lifecycle: no round scoping and no removal verb, so it eventually mis-replays or wedges.

Watch

• Dispatch records are write-only and un-scoped by round. The record is keyed by bare item while the ledger entry it mirrors carries round, and the modes are only begun→sent — nothing ever removes a record ("Close the round" never mentions the queue). Consequence, whichever way item keys are read: if item-<n> restarts per round, a new round's dispatch_begin hits last round's sent record and returns replay: true with a stale session key, steering the conductor away from dispatching a genuinely new item; if keys are goal-unique, records accumulate to MAX_ITEMS (200) and dispatch_begin refuses ("a goal that was never closed out, not a cap to raise") with no in-band remedy — the skill forbids fs_write and non-bundled shell, so the conductor cannot clear its own record. Fix: key or field the record by round, and give "Close the round" a prune step (a dispatch_done/round_close mode).
• The recovery still rests on model discipline at the exact moments the failure strikes — enqueue must be called before the compaction that would lose the message, and dispatch_begin before the create. The docstring discloses this honestly ("detectable-and-convergent, NOT atomic") and scopes the gateway-side fix out; humans should just know the window is narrowed, not closed.

Suggestions

• Have status (already run each patrol cycle) surface dispatch records whose ledger row disagrees or is absent — the two durable stores now share session/status fields with only prose ordering reconciling them.

[DESIGN-REVIEWED] 908384b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've validated all four candidates against the code and looked for grounded defects of my own.

Candidate 1 (dispatch MAX_ITEMS never pruned): The cap is 200; a round is 2–3 items per the skill. Reaching a lifetime 200 live dispatch records requires assuming goals that never close out — the candidate itself cannot establish (a) that this occurs in practice. Capacity design limitation, not a concrete defect. Killed.

Candidate 2 (full inbox counts claimed entries): Requires 200 residual claimed-but-never-done entries accumulated via repeated lost-turn-after-claim. No concrete input path establishes this happens; done/release drain the normal flow. Speculative accumulation. Killed.

Candidate 3 (same-second sidecar collision): Only affects forensic recovery of already-corrupt bytes, and the normal path (quarantine → write fresh valid file) makes back-to-back same-second corruption unnecessary to reach. No observable wrong outcome for live data. Killed.

Candidate 4 (small stale_secs re-serves a live claim): Requires an out-of-contract stale_secs far below a round's duration; default is 900 and the documented pattern is one claim per round. The re-serve on elapsed staleness is the designed at-least-once semantics, and the write is fenced. No in-contract trigger. Killed.

Step 2: I examined the lock steal/fence logic (TOCTOU on steal is made non-destructive by the token fence and double-check in _write), the atomic-write path, the shape validation in _read, and the quarantine-under-lock in mode_status. Each apparent hazard has a compensating guard in the same diff. Nothing grounded to the 80+ bar.

No findings.

[OPUS-REVIEWED] 908384b

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

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

[github-actions]

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 908384b265e001a4362c84d36d818d665d81737a. 4 of 4 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/builtin_skills/goal-conductor/SKILL.md:108 -- Dispatch IDs cannot recover the created session (origin: validation)
It pre-assigns the item's dispatch id / session_create with a title that says what the item is FOR
Lost turn after session_create -> queue retains an ID absent from the session -> replay cannot identify it and may create a duplicate.
Anchor: residual/crash-data-loss-corruption
Fix: Include the dispatch ID in the session title and require replay recovery through list_sessions.

BLOCKING -- src/kiro_crew/builtin_skills/goal-conductor/scripts/dispatch_queue.py:248 -- Malformed mapping entries still crash queue operations (origin: validation)
or not all(isinstance(entry, dict) for entry in data["inbox"])
Pending entry missing id or text -> _read accepts it -> mode_claim raises uncaught KeyError.
Anchor: residual/crash-data-loss-corruption
Fix: Validate required nested fields and types before accepting state; quarantine invalid records.

BLOCKING -- src/kiro_crew/builtin_skills/goal-conductor/scripts/dispatch_queue.py:496 -- Stale-lock takeover can unlink a new owner's lock (origin: validation)
self.path.unlink()
Old holder releases after staleness check -> another caller acquires -> stale contender unlinks its lock -> concurrent critical sections can move a newly written queue aside.
Anchor: residual/crash-data-loss-corruption
Fix: Make takeover atomically remove only the exact owner token that was inspected.

BLOCKING -- src/kiro_crew/builtin_skills/goal-conductor/scripts/dispatch_queue.py:202 -- Quarantine names can overwrite recoverable state
path.replace(path.with_suffix(f".corrupt-{int(_now())}"))
Two corruptions within one second -> identical destination -> the earlier sidecar and its parked messages are overwritten.
Anchor: residual/crash-data-loss-corruption
Fix: Generate a collision-proof sidecar name and refuse replacement.

FINDING -- src/kiro_crew/builtin_skills/goal-conductor/scripts/dispatch_queue.py:687 -- "len(data['dispatch']) >= MAX_ITEMS" counts sent records, so a 201-item sequential goal permanently refuses new dispatches -> Fix: exclude terminal records from the capacity check.

FINDING -- src/kiro_crew/builtin_skills/goal-conductor/scripts/dispatch_queue.py:44 -- "Every response carries state_path" contradicts validation errors returned directly by _error -> Fix: qualify the documented contract or add state_path to those responses.

[BLOCK-MERGE] 908384b
[GPT-REVIEWED] 908384b
False positive or not applicable? A repository writer can comment:
/ai-review override gpt 908384b265e001a4362c84d36d818d665d81737a: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 908384b265e001a4362c84d36d818d665d81737a — 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 are done. Producing the review.

First-Principles-Verdict: CONCERNS

Both durable records answer a named, real loss and declare their costs; the one straggler is a status mode the skill prose never tells the conductor to call.

What this change ships

Intent: stop a compaction or lost turn from silently discarding a user's mid-flight steering message or double-dispatching a work item — an ADDITION (a durable store) motivated by two named defects.

1. Mid-flight user messages now park on disk (enqueue/claim/done/release) — justified
2. A retried dispatch converges on one id, replay flagged (dispatch_begin) — justified; mechanism level, gateway-side atomicity explicitly named as out of scope
3. dispatch_sent closes the replay window after the seed — justified
4. status mode with unsent_items/exists — zero consumers
5. Two extra approval prompts per dispatched item, one per parked message — justified, declared in Known limits
6. New persisted state under <data_home>/conductor/<goal>/ (queue, lock, corrupt sidecars) — justified
7. Corrupt/wrong-shape state quarantined to a sidecar, never blanked — justified
8. Stale lock stolen only from a proven-dead holder, fenced at publish — justified
9. Conductor prompt names three scripts instead of two — justified rider
10. 50 tests including an identity/network source ratchet — justified

Watch

• status is the only mode SKILL.md never instructs: I grepped the skill for the seven mode names — dispatch_begin, dispatch_sent, enqueue, claim, done, release each appear in an instruction; status appears only as the ledger's unrelated field. The LLM operates from the skill, so unsent_items — the mode's one distinctive output — has no reachable reader; recovery is already carried by replay: true on dispatch_begin.
• The lock/liveness code parallels platform_compat.pid_exists/file_lock, but the standalone constraint is derived (skills are copied into ~/.kiro/crew/skills/ where kiro_crew is not importable), so it is not a duplicate — noted so nobody "deduplicates" it into a package import.

Subtractions

• Drop mode_status (and its unsent_items/exists fields) from dispatch_queue.py until something is instructed to call it — 0 prose consumers counted; the six instructed modes already carry both recovery paths.

[FIRST-PRINCIPLES-REVIEWED] 908384b

[iamwhatever]

• Structurally invalid state is silently discarded (span=bd5779688197) — fixed in 1add78a61069c583dfe886d2588f1a81f9fd9d6a.

Both questions answered yes: the finding holds against the code, and the fix is
proportional — it is the anti-data-loss guarantee this script exists to provide,
not speculative hardening.

Valid but malformed state -> _read substitutes empty data -> next mutation overwrites recoverable dispatch records.

_read now validates the record's shape before it can be replaced — dict at the
top, inbox a list, dispatch a dict — and anything that fails is moved aside by
a new _quarantine() to a .corrupt-<epoch> sidecar before a blank is
substituted. The bytes are never destroyed, so the dispatch records stay
recoverable by hand.

Coverage: test_structurally_invalid_state_is_quarantined_not_silently_dropped,
parametrized over three malformed bodies, asserts the sidecar's bytes are
byte-identical to the original.

Note: the script was renamed queue.py -> dispatch_queue.py in this round —
the old name shadowed the stdlib queue module and broke mypy across six
unrelated files.

[iamwhatever]

• Short writes can publish truncated state (span=bd5779688197) — fixed in 1add78a61069c583dfe886d2588f1a81f9fd9d6a.

Both questions answered yes: os.write is documented to write fewer bytes than
requested, so this is reachable rather than theoretical, and looping to completion
is the minimum correct form of the write — nothing wider was added.

Low disk space -> _write accepts a partial write -> os.replace publishes truncated JSON and loses the prior queue.

A new _write_all() loops until every byte is written and raises OSError on a
short or zero-byte write. _write now calls it inside a try/except BaseException
that closes the fd, unlinks the temp file, and re-raises — so os.replace is
never reached and the previous record stays intact. main() reports the failure
as a structured state_write_failed error naming that the previous record is
unmodified, rather than crashing.

Fixing this also closed a latent bug the finding did not name: the lock's own
token write went through bare os.write too, and a half-written token can never
match still_held(), which would have wedged every later fenced write on that
goal. Lock acquisition now uses _write_all and fails cleanly instead.

Coverage: class TestAPartialWriteIsNeverPublished —
test_a_short_write_refuses_and_leaves_the_previous_record and
test_a_full_disk_is_reported_as_a_refusal_not_a_crash.

[iamwhatever]

• A slow live writer can have its lock stolen (span=bd5779688197) — fixed in 1add78a61069c583dfe886d2588f1a81f9fd9d6a.

Both questions answered yes. The fix does exactly the two things the finding
asked for and stops there — it does not reach for a distributed lock, which would
be disproportional for a single-host JSON record.

Writer stalls beyond 60 seconds -> another process steals its live lock and writes -> the resumed writer replaces that newer state, losing updates.

Verify the owner is dead. A steal past LOCK_STALE_SECS (60s) now requires
proving the holder is gone: _holder_is_alive() reads the recorded pid and probes
it. The probe is deliberately three-valued — True, False, or None for
"cannot be judged" — because os.kill(pid, 0) is only a non-destructive probe on
POSIX; on Windows os.kill terminates the target, so that platform returns
None and never signals. A holder that cannot be judged (unparseable pid, or any
platform without a safe probe) keeps its lock until the separate
LOCK_ABANDON_SECS backstop at 900s.

Fence lock ownership. Each holder writes a <pid> <random> token. _write
takes the lock as a fence= argument and re-checks still_held() immediately
before publishing; if the token no longer matches it raises _LockLost and writes
nothing, so main() reports lock_lost and states that the other writer's state
stands. __exit__ only unlinks a lock it still owns. Together: the liveness gate
makes a wrong steal rare, and the fence makes losing that race non-destructive —
refuse rather than clobber.

Coverage: test_a_stale_lock_from_a_dead_holder_is_stolen and
test_a_stale_lock_whose_holder_still_runs_is_not_stolen (both POSIX-gated, using
a genuinely reaped pid rather than a hardcoded one),
test_a_holder_that_cannot_be_judged_is_stolen_only_past_the_backstop
(portable), and test_a_stolen_lock_makes_the_write_refuse_instead_of_clobbering.

[iamwhatever]

• File-creating child inherits the repository CWD (span=afff629800ab) — fixed in 1add78a61069c583dfe886d2588f1a81f9fd9d6a.

Both questions answered yes, and the fix is literally the one the finding named —
one keyword argument, no test restructuring.

Queue mode test -> child starts from the checkout and creates files -> any relative child artifact escapes tmp_path into repository state.

Every subprocess.run that launches the script now passes an explicit cwd=
pointing inside tmp_path: the shared _run helper passes cwd=str(home), and
the two TestInvocationContract cases that call the script directly gained a
tmp_path fixture and cwd=str(tmp_path). No child can now write a relative
artifact into the checkout.

The same pass added encoding="utf-8" to those calls, which the repo's
scripts/check_subprocess_encoding.py gate independently requires for text-mode
subprocess.run — that gate was the other red on the previous head and is now
green.

[iamwhatever]

• Cross-Platform Portability: POSIX-only process/signal primitive on an added line — rebutted, and the posix-only-approved label applied so the reviewed finding stops blocking.

Rule 3 of Scan added lines for non-portable patterns matched os.kill seven times in this diff. Six are docstring and comment prose that exists specifically to explain the Windows caveat — the scan greps added lines without distinguishing code from prose, so the text warning against a portability trap is itself reported as one. The seventh is the only real call site.

That call site is already gated the way the check's own error message prescribes:

POSIX-only process/signal primitive -- absent or different on Windows.
Gate on sys.platform or route through platform_compat.

src/kiro_crew/builtin_skills/goal-conductor/scripts/dispatch_queue.py:327-332:

if not hasattr(os, "kill") or sys.platform == "win32":
    return None
if pid <= 0:
    return None
try:
    os.kill(pid, 0)

_holder_is_alive is deliberately three-valued — True / False / None — because "cannot tell" must not read as "dead". On Windows it returns None before reaching os.kill, precisely because os.kill there ignores the signal and terminates the target; answering a bookkeeping question destructively is the bug the gate is meant to catch, and this is the code that avoids it. The lock's 900s LOCK_ABANDON_SECS backstop covers the None case, and test_conductor_queue.py asserts the Windows path separately rather than skipping the module.

Routing through platform_compat is not available: this is a bundled skill script executed standalone under execute_bash, restricted to the stdlib with no product imports — a source ratchet in the tests pins that, since importing kiro_crew is what would give the script ambient authority it must not have. The two stdlib alternatives are worse: reading /proc/<pid> is Linux-only and trips rule 2 (absolute POSIX path literal), and psutil is third-party.

So the line is correct as written and there is no in-scope change that makes the scan pass honestly. Per the check's own remedy — "if this code path is intentionally POSIX-only and documented as such -- add the posix-only-approved label and re-run" — the label is applied. Recording it here rather than letting the label silently flip a red check green.

[iamwhatever]

• _Lock.__enter__ hot-spins forever when a steal is decided but unlink keeps failing — fixed in b74424c13b0c (span=3c793f28e5c8)

Confirmed, and worse than advisory: the loop never terminates. The continue after the try/except OSError was unconditional, so a lock that is statable, stale, and un-removable — a goal dir created under another uid, a read-only mount — re-decided the same steal on every pass with no time.sleep and no _now() >= deadline check. That is exactly the hot spin the FileNotFoundError branch immediately above already refuses; the steal path just did not inherit the reasoning.

Fix: only a successful unlink retries.

if self._may_steal(age):
    try:
        self.path.unlink()
    except OSError:
        # The steal was decided but could not be carried out ...
        # An un-removable lock must converge on `locked`, not pin a core forever.
        pass
    else:
        continue
if _now() >= deadline:
    raise TimeoutError(...)
time.sleep(0.05)

Covered by test_a_stale_lock_that_cannot_be_unlinked_times_out_instead_of_spinning, which asserts both halves of the failure rather than just wall-clock: elapsed stays inside the wait budget and the unlink attempt count stays bounded, because the bug's signature is an unbounded retry loop, not a slow one. It also asserts the lock it could not remove is left in place.

The test was verified against the bug before being trusted: with the unconditional continue restored it did not fail on an assertion — it was killed by pytest's 45s timeout after 44.82s, having never reached a deadline check. With the fix it converges in ~5s (LOCK_WAIT_SECS). Module: 40 passed.

[iamwhatever]

• Nested malformed records crash queue operations — fixed in 908384b26 (span=942100f03ebb)

Confirmed, and it defeats the quarantine's whole purpose. The shape check went one
level deep only on the containers:

Valid JSON with inbox: [null] -> _read accepts it -> status calls e.get -> uncaught exception instead of structured JSON.

{"version": 1, "inbox": [null], "dispatch": {}} has a list where a list belongs,
so the check passed and every mode then reached for entry.get(...) on a
non-mapping. That is exactly the uncaught traceback the sidecar exists to replace
with a structured refusal — the guard was there and simply did not look far enough.

Fix: the entries are checked too, not just the shape holding them.

if (
    not isinstance(data, dict)
    or not isinstance(data.get("inbox"), list)
    or not isinstance(data.get("dispatch"), dict)
    or not all(isinstance(entry, dict) for entry in data["inbox"])
    or not all(isinstance(rec, dict) for rec in data["dispatch"].values())
):
    _quarantine(path)
    return _blank(goal)

Coverage: three nested bodies added to
test_structurally_invalid_state_is_quarantined_not_silently_dropped —
inbox: [null], inbox: ["just a string"], dispatch: {"item-1": "abc"} — each
asserting the .corrupt-<epoch> sidecar's bytes equal the original. Plus
test_a_malformed_entry_is_a_structured_refusal_not_a_traceback, which drives
status through the real subprocess, so a traceback fails the test rather than
being parsed as output.

Verified against the bug: with the nested checks reverted, the parametrized cases
fail with AttributeError: 'NoneType' object has no attribute 'get' and no sidecar
written. Module: 50 passed.

[iamwhatever]

• Successful writes are not durable across power loss — fixed in 908384b26 (span=942100f03ebb)

Confirmed. The write path already did the hard part — loop os.write to completion,
fsync the fd, os.replace — and then stopped one step short of the idiom it was
implementing:

Power loss before parent-directory metadata commits -> enqueue returns success after replacement -> reboot loses the queue update.

fsync on the file commits the bytes. It says nothing about the directory entry
that points at them, and os.replace is a directory operation. So the script could
answer ok: true for an enqueue — a message the user typed — that a reboot then
loses. Same class as the truncation the short-write guard already refuses, only the
loss happens after the answer instead of before.

Fix: a _fsync_dir helper, called on path.parent immediately after the replace.

os.replace(str(tmp), str(path))
_fsync_dir(path.parent)

Deliberately best-effort, and the reasoning is in the docstring: a directory is not
openable on Windows, and some filesystems refuse to sync one. Neither is a reason to
fail a write whose data is already committed and whose rename has already landed, so
sys.platform == "win32" returns early and both os.open and os.fsync swallow
OSError.

Coverage: test_the_parent_directory_is_synced_so_the_rename_survives_power_loss
(POSIX-gated) records fsyncs and filters them by
os.fstat(fd).st_mode & 0o170000 == 0o040000 — a plain call-count assertion passes
against the bug, since the file fsync was always there, so the test has to prove a
directory fd was synced. test_a_directory_that_cannot_be_synced_does_not_fail_the_write
is parametrized over both refusal points (will not open / opens but will not sync)
and asserts the write still publishes.

Verified against the bug: with _fsync_dir reverted, the directory case fails with
"the rename was never committed". Module: 50 passed.

[iamwhatever]

• Lock ownership is checked too early — fixed in 908384b26 (span=942100f03ebb)

Confirmed, and this one hollows out the fence rather than merely narrowing it.

Writer pauses after this check -> another invocation steals and publishes -> writer resumes through os.replace -> newer queue state is overwritten.

The single check sat on entry to _write, before the payload was written and
fsynced. That window is the slowest part of the whole operation — and it is
precisely the window in which a writer looks stalled long enough for its lock to be
judged stale and stolen. So the fence guarded the interval where a steal is least
likely and left unguarded the one where it is most likely. A writer that passed the
entry check could resume after the theft and os.replace its stale record over the
newer state, which is the exact clobber the token fencing was introduced to prevent.

Fix: the load-bearing check is the second one, immediately before the rename.

os.close(fd)
if fence is not None and not fence.still_held():
    try:
        tmp.unlink()
    except OSError:
        pass
    raise _LockLost(f"{fence.path} was taken while this write was being flushed")
os.replace(str(tmp), str(path))

The temp file is unlinked on the refusal path, so a lost fence does not leave
queue.tmp-* litter behind, and main() already maps _LockLost to the structured
lock_lost refusal.

Coverage: test_the_fence_is_rechecked_after_the_flush_not_only_on_entry steals the
lock from inside a patched os.fsync. That timing is the whole point — the entry
check has already passed when the theft lands, so the test can distinguish one fence
check from two, which a steal arranged before the call cannot. It asserts
_LockLost, that the newer record survives intact, and that no queue.tmp-*
remains.

Verified against the bug: with the second check reverted, the test fails
DID NOT RAISE _LockLost. Module: 50 passed.

[iamwhatever]

• Status can quarantine a concurrent writer's valid state — fixed in 908384b26 (span=942100f03ebb)

Confirmed. mode_status was the one mode that took no lock, on the reasoning that a
read needs none — and that reasoning is wrong here, because _read is not
side-effect-free:

Malformed state plus concurrent status and enqueue -> unlocked status reads old corruption -> enqueue publishes valid state -> status quarantines that new state.

_read renames. An unlocked status that observes old corruption, then loses the
race to an enqueue that publishes a good record, moves that good record aside to
.corrupt-<epoch>. A read-only mode destroying a just-published record is a
data-loss path, so the disproportionality argument does not apply.

Fix: mode_status holds the lock across both the read and the existence check.

path = _state_path(goal)
with _Lock(path):
    data = _read(path, goal)
    exists = path.exists()

Checked deadlock-free before applying: every _Lock( site takes the lock exactly
once at top level and never nests, _with_state is pure, and main() already maps
TimeoutError to the structured locked refusal — so contention degrades to an
answer rather than a hang.

Coverage: new TestStatusNeverDestroysWhatItOnlyMeantToRead.
test_status_takes_the_lock_before_it_reads plants a lock held by a live pid, so
the liveness probe reports the holder still running and no steal is possible, and
asserts ok: False / error.code == "locked".
test_the_quarantine_decision_is_made_under_the_lock asserts the invariant rather
than a timing coincidence: _quarantine is patched to record whether queue.lock
exists and carries this pid at the moment of the rename, and asserts
held_at_rename == [True] with no lock left behind. An earlier draft of this test
published from inside _quarantine, which no implementation could pass — the rename
happens whether or not the lock is held — so it was replaced with one that pins the
real property.

Verified against the bug: with the with _Lock(path): reverted, the two cases fail
with assert True is False and 'NoneType' object has no attribute 'get'.
Module: 50 passed.

[iamwhatever]

I don't think this is the right way to coordinate agents, closing it.