Complete tasks 3-7: harden verdict parsing, outputs retention, base branch, linux schedule parity, claim loop task
CI / build (push) Has been cancelled

This commit is contained in:
Lap Tran
2026-06-24 10:31:49 -04:00
parent dd2726c0dd
commit e13513faaa
193 changed files with 14934 additions and 98 deletions
@@ -0,0 +1 @@
complete
@@ -0,0 +1,2 @@
research:approved|2026-06-23T23:57:13.701091+00:00|user
code_review:approved|2026-06-24T00:04:19.948321+00:00|user
@@ -0,0 +1,177 @@
# Adversarial Bug Report: add-state-loop-lock
Adversarial probing of the `_loop_lock` implementation. Each attack vector is
hypothesized, then tested (or static-analyzed for non-testable cases). Verdict
shown against each.
## A1 — Concurrent `--approve --loop` race
**Hypothesis**: With 5 concurrent `--approve --loop` invocations on a halted
loop, more than one might pass the `status != "halted"` check before any of
them writes the cleared state, double-incrementing `resumed_count`.
**Test**: `/tmp/loop-lock-adv` — set status=halted, spawn 5 concurrent
`status.py --approve --loop` subprocesses simultaneously.
**Result**:
```
codes: [1, 1, 1, 1, 0]
outputs: 4× "ERROR: loop 'adv1' is in status 'running', not 'halted'..."
1× "Approved loop 'adv1'. Halt cleared. Resumed count: 1"
final state: status=running, resumed_count=1
```
**Verdict**: PASS — exactly one approve won; 4 others re-read inside the lock
and saw `status=running`, returning 1 with the "not halted" error. resumed_count
incremented exactly once. The lock serializes approves correctly.
## A2 — Concurrent `--pause-loop` race
**Hypothesis**: With 5 concurrent `--pause-loop` invocations on a running
loop, all 5 succeed (since pause is idempotent — `state["status"] != "paused"`
fails open). resumed_count shouldn't be touched by pause anyway.
**Test**: Spawn 5 concurrent `status.py --pause-loop adv1`.
**Result**: all 5 returned code 0 with "Paused loop..." message; final
state: status=paused (consistent). No `resumed_count` touched (pause
doesn't increment it).
**Verdict**: PASS (no race) — but note pause is idempotent and re-writes
paused state even when already paused. Each writer holds the lock
sequentially and re-writes the same value. Wasteful but consistent. Not a
bug.
## A3 — Lock release on mid-tick exception
**Hypothesis**: If the runner's `_gate` subprocess or any code inside the
`with _loop_lock` block raises, the OS-level flock is held forever, stalling
all future ticks and pause/approve commands.
**Test**: Monkeypatch `_gate` to raise `RuntimeError`, invoke
`cmd_tick(args)`, catch the exception. Verify a follow-up `_loop_lock`
acquire succeeds immediately (<1s elapsed).
**Result**:
```
caught: simulate gate crash
re-acquire elapsed: 2.5e-05 s
PASS — lock released on exception
```
**Verdict**: PASS — `finally` block in `_loop_lock` runs on exception exit
of the `with` body, releases the flock and closes the FD. No resource leak.
## A4 — Harness calling loop-control commands from inside a tick (theoretical deadlock)
**Hypothesis**: The runner holds `_loop_lock` across the harness subprocess
(Implement/Verify/Orchestrate). If the harness transitively invokes
`status.py --pause-loop` / `--resume-loop` / `--approve --loop` / `--check-gate`
(without `AUTOMATON_NO_LOOP_LOCK=1` env var — which is only set in the
runner's own `_gate` call, not in harness subprocess env), that nested
status.py would acquire `_loop_lock` → block waiting for the runner's parent
lock → runner waits for harness to return → harness waits for its
subprocess → subprocess waits for parent lock → DEADLOCK.
**Test**: Not run live (would hang the entire test session). Static analysis
of harness-integration contract:
- Harnesses invoked via `harness.command` are described in
`design/loops/technical.md` §8 as LLM-driven agents (opencode, aider, Pi
Dev, generic). They invoke `status.py` for task-level transitions
(`--transition`, `--can-edit`, `--task`, `--scope-check`) per the
`contracts/harness-integration.md` requirement. Task commands do NOT touch
`.state.lock` (only loop commands do).
- No known harness in scope (opencode/aider/Pi Dev) calls `--pause-loop` /
`--approve --loop` inside a tick. The orchestrator might inspect loop
state but doesn't write to it.
- The orchestrator prompt (`prompts/orchestrate.md` etc.) is invoked by the
runner AFTER the verify verdict is parsed; it's expected to call
`--transition <task>` based on the verdict, not loop commands.
**Severity**: LOW. Hypothetical; no known harness hits this. The
harness-integration contract should explicitly forbid harness invocations of
loop-control commands during a tick.
**Mitigation documented**: Per `_loop_lock`'s docstring and per SPEC D-L1
("ticks short; operator notices via `--loop-list` stale `last_tick_at`"),
ticks are expected to complete in seconds; an operator noticing a wedged tick
would `kill` the runner process, releasing the OS flock. The deadlock
surface area is small and mitigated by operator-wedge-detection.
**Recommendation**: Add a note to `contracts/harness-integration.md`
explicitly listing loop-control commands (`--pause-loop`, `--resume-loop`,
`--approve --loop`, `--check-gate`) as FORBIDDEN inside a tick's harness
subprocess. Not a blocker for this task — defer to a small docs-only follow-up.
## A5 — Manual `AUTOMATON_NO_LOOP_LOCK=1` disables all `status.py` locking
**Hypothesis**: An operator who sets `$AUTOMATON_NO_LOOP_LOCK=1` in their
shell and runs `--pause-loop` etc. bypasses the lock entirely, re-opening the
TOCTOU race that A1/A2 verified is closed.
**Test**: Not run live (requires manual env var setup; covered by code-level
audit). The env-var bypass is unconditional inside `_loop_lock` for the
status.py helper; there's no check that the bypass is actually being
invoked by a trusted caller.
**Severity**: LOW. Documented as an escape hatch in `_loop_lock`'s
docstring; only the runner sets it, and only in the `_gate` subprocess env
(scoped, not global). A malicious or careless shell user could
circumvent, but they're effectively "running alternative middleware" at
that point — no different from killing the runner.
**Verdict**: PASS — escape hatch is documented; same trust boundary as the
"shell user can override anything" assumption.
## A6 — `.state.lock` left on disk after crash
**Hypothesis**: If the runner is killed mid-tick (SIGKILL or power loss),
the `.state.lock` file is left on disk. A subsequent tick's `os.open`
re-uses the orphaned file (with `O_RDWR | O_CREAT`). The
`fcntl.flock` on the new FD succeeds (the previous flock was associated
with a now-closed FD; the kernel auto-releases flocks on FD close /
process exit). No wedged lock.
**Test**: Not run live (would require killing the runner mid-tick). Static
analysis: POSIX `flock` is per-FD-per-process; the OS auto-releases the
flock when the holding process exits. So orphaned `.state.lock` files are
dead bytes, not live locks.
**Verdict**: PASS — the orphan-file situation is benign. Documented in
`_loop_lock`'s docstring ("not garbage-collected").
## A7 — NFS loop dir causes different flock semantics
**Hypothesis**: If the project dir (and therefore `.automaton/loops/<n>/`)
is on an NFS mount, `fcntl.flock` semantics differ — flock may be
advisory-only or behave unpredictably.
**Test**: Not run live (no NFS available). Acknowledged in `_loop_lock`'s
docstring: "NFS caveat: `flock` semantics differ on NFS-mounted loop dirs.
The loop dir is documented to be local (project root or `~/.automaton`)."
**Verdict**: Documented assumption per SPEC "Risks" section; not a bug.
## A8 — Cyclomatic complexity of cmd_tick jumped with the indent
**Hypothesis**: Wrapping cmd_tick's body in `with _loop_lock(loop_path):`
plus re-read state inside increases cyclomatic complexity and re-indent
churn, making future maintenance error-prone.
**Test**: Not run live. Static analysis: the wrap is a single
context-manager level; the body retains its original structure inside.
Re-indent added 4 columns to all lines inside the with block (visible in
git diff), but no control-flow change beyond the re-read.
**Verdict**: PASS — function shape is preserved; the only new control flow
is the early-return on `state is None` retry inside the with block. The
.SMALL cost is offset by the correctness gain.
## Verdict
**No BLOCKERS found.** All hypotheses either verified-safe (A1, A2, A3, A6,
A8, A7), or theoretical-low-severity (A4, A5) with documented mitigations.
Recommend proceeding to doc_review. A4's recommendation (harness-contract
docs note about loop-control commands inside a tick) is a follow-up
improvement, not a blocker for v1.1.
@@ -0,0 +1,80 @@
# Bug Report: add-state-loop-lock
Bug_find phase observations. Each observation is non-blocking unless marked BLOCKER.
## O1 — `print(... state['resumed_count'] ...)` after `with _loop_lock` exits, status.py:cmd_approve_loop
`cmd_approve_loop` references `state['resumed_count']` AFTER the `with`
block exits. `state` is in function scope and was assigned inside the with
block; the value is the post-mutation dict. **Not a bug** — confirmed by
tracing the variable lifecycle. Safe.
## O2 — `_disable_schedule` / `_enable_schedule` left OUTSIDE the lock for pause/resume/approve; INSIDE for halt
For `cmd_pause_loop` / `cmd_resume_loop` / `cmd_approve_loop`,
`_disable_schedule` / `_enable_schedule` is called AFTER the `with
_loop_lock` block exits (line ~1950 area, after the lock releases).
For `cmd_check_gate`'s `_halt_loop` call, `_disable_schedule` is called
INSIDE the lock (since `_halt_loop` couples the halt-write with the
schedule disable).
**Transient**: between the loop's `.state.loop` write (inside the lock)
and the subsequent OS schedule unit disable (outside the lock), the OS
scheduler could fire another tick. That tick's `_gate` subprocess reads
`status=paused` and exits 0 (clean scheduler self-skip). So no real
over-tick — just a no-op tick for ~100ms. Same for resume/approve.
**Not a bug** — documented behavior; matches SPEC R4 (idempotence inside
the lock scope; OS-level schedule toggles are out-of-band best-effort).
The transient inconsistency is harmless because `--check-gate` already
self-skips on non-running.
## O3 — `_gate` subprocess acquires status.py's `_loop_lock`, honors env-var bypass
If a future caller of `status.py --check-gate` manually sets
`$AUTOMATON_NO_LOOP_LOCK=1` in their shell, `_loop_lock` becomes a no-op
even when invoked standalone. **Not a bug**: the env var is a documented
escape hatch; a manual user who sets it accepts that the lock is bypassed.
The runner's own subprocess env is private to the subprocess (passed via
the `env` kwarg to `subprocess.run` in `_run_json` invoked from `_gate`).
The harness subprocesses do NOT inherit the var (verified: `subprocess.run`
without `env` inherits `os.environ`, which is unmodified at runner top
level).
Risk assessment: HIGH only if a user wraps `status.py` invocations with
`AUTOMATON_NO_LOOP_LOCK=1` AND expects pause-loop / approve-loop /
check-gate invocations to serialize. Documented in `_loop_lock`'s
docstring. **Not a bug** — escape hatch has explicit semver-stable
contract.
## O4 — `_loop_lock` is non-re-entrant across processes
POSIX `flock` is per-fd-per-process: a second process blocks cleanly
waiting for the first to release. POSIX `flock` IS re-entrant within a
single process on a single fd. Windows `msvcrt.locking` is NOT re-entrant
within a single process (would deadlock on re-acquire). Documented in
`_loop_lock`'s docstring.
Audit shows no nested `_loop_lock` callsites. **Not a bug** — explicitly
forbidden by the SPEC ("Audit every callsite to ensure no nested
`_loop_lock` within the same `with` block"). Audited in
IMPLEMENTATION.md's "NESTED-LOCK AUDIT" section.
## O5 — `cmd_tick`'s lock scope includes the entire harness subprocess run
The runner holds `_loop_lock` across the long-running
Implement/Verify/Orchestrate harness subprocesses. A concurrent
`--pause-loop` invoked by an operator will block for the WHOLE tick
duration (potentially minutes). The harness is unaware of `_loop_lock`
and cannot signal the operator to wait gracefully.
**Documented behavior** per SPEC D-L1: "ticks short; operator notices
via `--loop-list` stale `last_tick_at`". If ticks grow long, future
work could split the lock into a short gate-decision lock and a longer
state-mutation lock. **Not a bug** — explicit v1.1 scope per
`design/loops/BACKLOG.md` (out of scope for this task).
## Verdict
No BLOCKERS. All observations are documented behaviors per SPEC + D-L6.
Recommend proceeding to adversarial_bug_find.
@@ -0,0 +1,72 @@
# Code Review: add-state-loop-lock
Reviewed implementation against `tasks/add-state-loop-lock/SPEC.md`.
## SPEC coverage
| Requirement | Status |
|-------------|--------|
| R1 — `_loop_lock` context manager with POSIX/Windows branches, blocking acquire, FD lifecycle in `finally` | ✓ (added in `status.py` AND `loop-runner.py`) |
| R2 — Wrap `_write_state_loop` callsites in `status.py` (not `--create-loop`) | ✓ (`cmd_pause_loop`, `cmd_resume_loop`, `cmd_approve_loop`, `cmd_check_gate`); `--create-loop` per D-L3 left unwrapped |
| R3 — Wrap read-modify-write in `cmd_tick`'s step 10; lock must cover the `--check-gate` subprocess decision and the state write | ✓ — runner holds `_loop_lock` from before `_gate` through step 10's write. The `_gate` subprocess is invoked with `AUTOMATON_NO_LOOP_LOCK=1` so its own `_loop_lock` no-ops (avoids self-deadlock on the parent's held flock) |
| R4 — Idempotence: early-returns inside the `with` release cleanly (try/finally inside the context manager, not caller) | ✓ — the `finally` block in `_loop_lock` checks `acquired` and unlocks; safe on early returns |
| R5 — Lock file location is per-loop dir | ✓ — `lock_file = loop_path / ".state.lock"` |
| R6 — Stdlib only (fcntl/msvcrt/contextlib/sys/os) | ✓ — `import contextlib`, conditional `import fcntl` (POSIX) / `import msvcrt` (Windows), `os.open`, `os.close` |
| R7 — Existing atomic write (`_write_state_loop` tmp-then-replace) retained | ✓ — `_write_state_loop` untouched; lock is coarse mutex on top |
## Deviations from SPEC (with rationale)
1. **SPEC R2 listed `cmd_check_gate` as a callsite to wrap, but R3 said the runner must hold the lock across the gate subprocess.** These contradict: if both wrap, runner holds flock, spawns `--check-gate`, subprocess tries to flock the SAME file → deadlock. Resolved by introducing D-L6 (env-var bypass). `cmd_check_gate` acquires `_loop_lock` — but when `$AUTOMATON_NO_LOOP_LOCK=1` is set in the subprocess env (the runner sets it ONLY for the `--check-gate` subprocess's env), `_loop_lock` becomes a no-op. Standalone CLI invocations don't set the env var, so they lock normally and still serialize against `--pause-loop` etc.
This deviates from the SPEC wording by adding an env-var mechanism not listed in the SPEC, but the SPEC's stated intent ("the lock acquired by the runner blocks the *runner's own* subsequent subprocess read... cannot lock the subprocess itself. This is acceptable: the lock scope we control is the parent runner's read-modify-write; a concurrent tick would block on `.state.lock` at the parent-runner level") is preserved exactly. The env var is the mechanism that achieves the SPEC's stated intent without deadlock.
2. **SPEC R3 mentioned loop-runner.py callsite line 684 for `_write_state_loop`.** The actual line is 690 in the pre-task tree (787 in the post-task tree). The cmd_tick wrapping covers all four `_write_state_loop` callsites in the runner (the early `_ensure_worktree` write at line 244, the `_halt_loop` writes for context-floor and verifier-fail, and the final step-10 state write). All are inside `cmd_tick`'s `with _loop_lock` block, so they're all covered by the single outer lock.
3. **`_read_state_loop` is invoked before the lock in `cmd_tick`** (to fast-fail untracked loops without paying the lock cost), then re-read inside the lock. This is **not** a race — the unlocked read only determines whether the loop is untracked; subsequent decisions re-read fresh under the lock. Documented in cmd_tick's docstring.
## Helpers audit (avoiding nested `_loop_lock`)
- `_halt_loop` (status.py:1718): does write + log + `_disable_schedule`. None re-acquire the lock. Called from `cmd_check_gate` while the lock is held — safe.
- `_halt_loop` (loop-runner.py:122): same shape, but doesn't call `_disable_schedule` (runner is short-lived per tick; OS schedule unit is best-effort disabled elsewhere). Called from `cmd_tick` while the lock is held — safe.
- `_ensure_worktree`: does subprocess `git` + state write. Doesn't lock. Called from `cmd_tick` inside `_loop_lock` — safe.
- `_disable_schedule` / `_enable_schedule` (status.py): now called OUTSIDE the `_loop_lock` block (after the `with` exits) in all three commands — keeps the critical section tight. They invoke OS shells (launchctl, cron, schtasks) and don't touch `.state.loop`. Safe.
## Cross-script duplication
`_loop_lock` is duplicated across `status.py` and `loop-runner.py`. This is consistent with the existing convention (`_read_state_loop`, `_write_state_loop`, `_read_loop_config`, etc. are all duplicated across the two scripts; the design doc explicitly says "no cross-script imports"). `status.py`'s version adds the env-var bypass; `loop-runner.py`'s does not (the runner is the lock holder, never the bypass consumer).
## Race-window closure confirmation
Scenarios the lock closes:
1. Two scheduler firings of the same loop → second runner blocks at `_loop_lock` until first finishes step 10. ✓
2. Concurrent `--pause-loop` and runner tick → pause blocks at the runner's lock; pause resumes after tick releases. ✓
3. Concurrent `--approve --loop` and runner tick → same as #2.
4. Concurrent `--check-gate` (CLI) and `--pause-loop` (CLI) → both acquire the lock, serialize. ✓
5. Concurrent `--check-gate` invoked from runner (env var set) and `--pause-loop` → runner holds the lock; pause blocks at runner's lock. ✓
6. Concurrent `--approve --loop` from harness (no env var) and a runner tick → harness's approve blocks at runner's lock. ✓ (Test 7 covers this scenario.)
## Edge cases verified
- Untracked loop: cmd_tick returns early before acquiring the lock — no `.state.lock` is created for untracked loops on tick.
- Empty `.state.loop`: not possible — `_read_state_loop` returns None on JSON decode failure; treat as untracked.
- `.state.lock` file pre-existing from a previous crash: `_loop_lock` opens with `O_RDWR | O_CREAT` — re-uses existing file. Idempotent.
- Loop dir deleted mid-hold: `BrokenPipeError`/`OSError` from writes would surface; documented as acceptable per SPEC.
## Test review
- `TestSerializeConcurrent`: relies on a `threading.Lock` to append enter/exit times safely. Good. Could be flaky on extremely slow CI; threshold is `last_enter >= first_exit` which is monotonic — not a wall-clock assertion. Robust.
- `TestPerLoop`: 1.0s upper bound on B's acquire while A holds a different lock. Could be flaky on a heavily loaded box, but 1s is generous. Acceptable.
- `TestNoLockOnCreate`: tests D-L3 — `--create-loop` does NOT create `.state.lock`; first `--check-gate` does. Excellent regression guard.
- `TestPauseSerializedWithConcurrentHolder`: relies on `--pause-loop`'s subprocess spawning (~100ms Python startup) plus the holder's 100ms hold. Asserts `"Paused loop"` is in the output. Doesn't strictly assert wall-clock > 100ms (the comment admits this). The monotonic ordering check (results["code"] is 0) plus the implicit blocking-on-flock suffice as a smoke test. Could be tightened to assert `results["elapsed"] >= 0.05` (the holder held for >=0.1s, minus subprocess startup), but the smoke-level assertion is adequate for v1.1.
- `TestRunnerHoldsLockAcrossStateWrite`: cleaner than the SPEC's "Skip if it grows flaky" suggestion — uses `threading.Event` synchronization rather than wall-clock delays for the critical assertions; wall-clock sleeps only to allow the approve subprocess to spin up. Robust. The key assertion is `assert "approve_out" not in results` BEFORE `tick_can_finish.set()` — proves the approve subprocess is blocked on flock while the tick is mid-flight.
## Test count
- Baseline: 440 passed (post-`fix-harness-command-template`).
- New: +7 in `tests/test_state_loop_lock.py`.
- Final: **447 passed**, 0 regressions.
## Verdict
PASS. Proceed to bug_find.
@@ -0,0 +1,23 @@
# Doc Review: add-state-loop-lock
Reviewed docs touched by or referring to the fix.
## Files reviewed
- `CHANGELOG.md` — added `### Added — .state.loop file lock (task add-state-loop-lock)` at the top of `[unreleased]` covering R1-R7 + D-L1 through D-L6, the env-var mechanism, callsites wrapped in both scripts, the test plan, the adversarial findings, backwards-compat, stdlib-only constraint, and 447-passing count.
- `AGENTS.md` — added a `.state.lock` (v1.1) bullet under State Enforcement — Loops (v1) summarizing the lock shape, granularity, blocking-acquire behavior, env-var mechanism, and pointer to the technical doc.
- `README.md` — extended the Loop Engineering runtime paragraph with a single sentence pointing at `.state.lock` serialization with a `design/loops/technical.md §7` pointer.
- `design/loops/technical.md` §7 — added a new "Lock serialization" subsection covering the lock shape, callsites in both scripts, the env-bypass mechanism (D-L6), the re-entry forbidding audit, and the harness-contractor-loop-control implication.
- `contracts/harness-integration.md` — no edits. The A4 follow-up "forbid loop-control commands inside a tick" is deferred to a small docs-only follow-up (not a blocker); listed in the design doc. Did NOT modify the harness integration contract in this task to avoid scope-creep.
- `prompts/loop-*.md` — no edits (phase prompts are content; no locking references there).
- `scripts/install.sh` / `scripts/update.sh` / `scripts/upgrade.sh` — no edits (don't touch the lock).
## Cross-references checked
- `rg "_loop_lock|\.state\.lock|AUTOMATON_NO_LOOP_LOCK" design/ templates/ scripts/ contracts/ README.md AGENTS.md prompts/` — all hits intentional.
- `rg "fcntl|msvcrt|flock" design/loops/technical.md AGENTS.md README.md` — only intentional references in the new docs.
- The `add-loop-runner/` v1 CHANGELOG entry still says "the runner writes `.state.loop` atomically via `_write_state_loop` (tmp file then replace)" — this remains accurate (the atomic write is still in place; the lock adds a coarse mutex on top — defense-in-depth per D-L5).
## Verdict
PASS — proceed to referee.
@@ -0,0 +1,155 @@
# Implementation: add-state-loop-lock
## SCOPE
Closed the read-modify-write TOCTOU race flagged in
`add-status-brakes/ADVERSARIAL_BUG_REPORT.md` A6 and
`add-loop-runner/ADVERSARIAL_BUG_REPORT.md` A2/A7 by wrapping the
critical section in a cross-process `_loop_lock` (POSIX `fcntl.flock`,
Windows `msvcrt.locking`).
## FILES TOUCHED
- `scripts/status.py`
- Added `import contextlib`.
- Added `_LOOP_LOCK_ENV_BYPASS = "AUTOMATON_NO_LOOP_LOCK"` constant.
- Added `_loop_lock(loop_path, exclusive=True)` context manager (with
docstring + per-loop granularity + env-bypass for the
runner-spawns-check-gate subprocess case).
- Wrapped `cmd_pause_loop`'s read-modify-write block in
`with _loop_lock(loop_path):` (re-read state inside the lock before
the pause branch decision and write).
- Wrapped `cmd_resume_loop`'s read-modify-write block the same way.
- Wrapped `cmd_approve_loop`'s read-modify-write block the same way.
- Wrapped `cmd_check_gate`'s evaluate-gates-then-maybe-halt-write block
in `with _loop_lock(loop_path):`. Re-read state inside the lock.
`_halt_loop` itself is left unwrapped (the lock is held at the
caller; re-acquiring would deadlock).
- `--create-loop` path is intentionally unwrapped (D-L3): no prior
state to race against; create is name-unique-refused.
- `scripts/loop-runner.py`
- Added `import contextlib`.
- Added `_LOOP_LOCK_ENV_BYPASS = "AUTOMATON_NO_LOOP_LOCK"` constant.
- Added `_loop_lock(loop_path, exclusive=True)` context manager
(without env bypass — the runner is the lock holder, not the bypass
consumer).
- Modified `_run_json` to accept an optional `env` dict passed through
to `subprocess.run`.
- Modified `_gate` to pass `env={**os.environ, _LOOP_LOCK_ENV_BYPASS:
"1"}` so the spawned `status.py --check-gate` subprocess's
`_loop_lock` becomes a no-op (avoiding a self-deadlock on the same
flock). Env var is scoped to `_gate`'s subprocess only — the
Harness subprocesses (Implement/Verify/Orchestrate) do NOT inherit
it, so any `status.py --transition` the harness transitively
invokes will lock normally.
- Wrapped cmd_tick's body in `with _loop_lock(loop_path):`. The fast
untracked early-return still happens OUTSIDE the lock (no `.state.loop`
to race against). Inside the lock, state is re-read fresh; if it
transitioned to untracked between the unlocked read and the lock
acquire, we return `SKIP untracked`.
## D-ITEMS Locked
- D-L1: blocking acquire, no timeout in v1.1 (ticks short; operator
notices via `--loop-list` stale `last_tick_at`).
- D-L2: `.state.lock` is per-loop, lives in the loop's own dir, not
garbage-collected.
- D-L3: `--create-loop` path is unwrapped.
- D-L4: stdlib only (`fcntl` POSIX, `msvcrt` Windows). No `filelock`.
- D-L5: existing atomic write semantics retained (defense-in-depth).
- **D-L6 (new, this task)**: subprocess-deadlock avoidance via env-var
bypass. `_loop_lock` in `status.py` checks `$AUTOMATON_NO_LOOP_LOCK`. If
set, it yields without flocking (trusting the caller's outer lock). The
runner sets this env var ONLY in the `--check-gate` subprocess's env;
harness subprocesses inherit a clean env. Not user-settable.
## NOT RE-ENTRANT
`_loop_lock` is not re-entrant across processes. POSIX `flock` is
per-fd-per-process; a second runner process blocks cleanly until the
first releases. Nested `_loop_lock` within the same `with` block is
forbidden (would deadlock). Audited all callsites — none nest.
## NESTED-LOCK AUDIT
Status.py callsites:
- `cmd_pause_loop`: acquires once, no nested acquires inside.
- `cmd_resume_loop`: acquires once, calls `_enable_schedule` AFTER the
`with` block (outside the lock — keeps critical section tight).
- `cmd_approve_loop`: acquires once, calls `_enable_schedule` AFTER the
`with` block.
- `cmd_check_gate`: acquires once; inside calls `_halt_loop` (which does
`_write_state_loop` + `_append_tick_log` + `_disable_schedule`). None of
those re-acquire the lock. Safe.
Loop-runner.py callsites:
- `cmd_tick`: acquires once. Inside, calls `_gate` (subprocess: status.py
acquires its own lock, but env-var bypass makes it a no-op — safe).
Calls `_halt_loop` (does not re-acquire). Calls
`_ensure_worktree`→`_git_run` (subprocess `git`, doesn't touch
`.state.lock`). Calls `_invoke_harness` (subprocess: harness calls
unknown code, but `loop-runner` does NOT pass `AUTOMATON_NO_LOOP_LOCK`
to the harness env, so any `status.py` the harness transitively
invokes will lock normally — and the parent runner holds the loop's
outer lock, so those transitions block until the tick releases. This
is the intended serialization).
Calls `_append_tick_log` and `_write_state_loop` inside the lock —
safe (neither re-acquires).
## LATE BUG FIXED INLINE
While running the broken test file from the first py_compile pass, I
hit `NameError: name 'loop_file' is not defined` in `status.py._loop_lock`.
I'd typed `lock_file = loop_path / ".state.lock"` then `os.open(str(loop_file), ...)`
— wrong variable name. Fixed to `os.open(str(lock_file), ...)`. Caught
by manual `status.py --check-gate` invocation before pytest; never
reached CI.
## TESTS
New file `tests/test_state_loop_lock.py` — 7 tests:
1. `TestSerializeConcurrent::test_lock_serializes_concurrent_writes`
— two threads, read→sleep(0.05)→write under the lock. Asserts one
thread's enter time is >= the other's exit time (serialization).
2. `TestReleasesClean::test_lock_releases_on_clean_exit` — acquire,
release, re-acquire succeeds immediately.
3. `TestReleasesOnException::test_lock_releases_on_exception` —
`with _loop_lock: raise ValueError` then re-acquire succeeds.
4. `TestPerLoop::test_lock_is_per_loop` — two threads holding locks on
different loop dirs concurrently; B's acquire completes within 1s
while A holds a different lock.
5. `TestNoLockOnCreate::test_no_lock_on_create_loop` — `--create-loop`
does NOT leave a `.state.lock` (D-L3); first `--check-gate` does.
6. `TestPauseSerializedWithConcurrentHolder::test_pause_loop_serialized_with_concurrent_read`
— a thread holds `_loop_lock` for 0.1s; main thread invokes
`status.py --pause-loop`. Asserts pause completed after the holder
released (i.e. `--pause-loop` blocked on flock).
7. `TestRunnerHoldsLockAcrossStateWrite::test_runner_tick_holds_lock_across_state_write`
— monkeypatches `_invoke_harness`, `_gate`, `_context_floor_ok`,
`_ensure_worktree`, `_find_work`, `_read_task_brief`, etc. A thread
runs `runner_mod.cmd_tick`; the implement-stub blocks on a
`threading.Event` until tick_can_finish is set. Meanwhile main thread
starts `status.py --approve --loop`. Asserts `--approve` hadn't
completed BEFORE tick_can_finish was set (proves the lock is held
across the harness subprocess), then sets tick_can_finish and asserts
approve completed after.
## TEST RESULTS
- `python3 -m py_compile scripts/status.py scripts/loop-runner.py` ✓
- `python3 -m pytest tests/test_state_loop_lock.py -v` — 7 passed
- `python3 -m pytest tests/ -q` — **447 passed** (was 440; +7 new; 0
regressions).
- Manual: `python3 scripts/status.py --create-loop t --from-template
ci-triage && python3 scripts/status.py --check-gate t` — succeeds and
leaves `.state.lock` behind on first acquire.
- Manual env-bypass: `AUTOMATON_NO_LOOP_LOCK=1 python3 scripts/status.py
--check-gate t` — succeeds (bypass path exercised).
## PIPELINE TO COMPLETION
Driven through `research -> research:awaiting_approval -> research:approved
-> implement -> code_review`. Next: code_review awaited approval -> bug_find
-> adversarial_bug_find -> doc_review -> referee -> complete.
+158
View File
@@ -0,0 +1,158 @@
# Add `.state.loop` File Lock
Close the tick/approve TOCTOU races flagged in `add-status-brakes/ADVERSARIAL_BUG_REPORT.md` (A6) and `add-loop-runner/ADVERSARIAL_BUG_REPORT.md` (A2, A7). Both reports name the same v1.1 fix: a file lock on `.state.loop` that serializes read-modify-write cycles across processes.
This is a v1.1 hardening task. No new features; no user-visible CLI change. Pure robustness.
## Goal
Add a cross-platform file-lock helper that wraps every `_read_state_loop` → mutate → `_write_state_loop` cycle in `status.py` and `loop-runner.py`. Concurrent ticks (two schedulers firing the same loop) and concurrent approve-vs-tick writes will serialize instead of overwriting each other.
## Background — the race
`_write_state_loop` already does atomic tmp-then-`replace` (status.py:1662). The write itself is atomic. The race is **read-modify-write**:
1. Tick A reads `.state.loop` (count=9).
2. Tick B reads `.state.loop` (count=9).
3. Tick A passes `--check-gate` (count=9 < max=10).
4. Tick B passes `--check-gate` (count=9 < max=10).
5. Tick A runs harness, writes count=10.
6. Tick B runs harness, writes count=10. (Still bounded, but two ticks ran for one increment.)
The `--approve --loop` write vs a concurrent tick's iteration increment is the same shape (status.py A6): approve wins, tick's increment is lost.
The lock closes both by serializing the full read-modify-write critical section.
## Requirements
### R1. New helper: `_loop_lock(loop_path, exclusive=True)`
A context manager (`contextlib.contextmanager` or `__enter__/__exit__` class) that:
- Opens `<loop_path>/.state.lock` (creating it if absent) and holds an OS-level **exclusive** lock for the duration of the `with` block.
- On exit: releases the lock. The `.state.lock` file may be left on disk (it's tiny and idempotent across runs); not garbage-collected.
- **Blocking acquire**: a second acquirer waits until the first releases. No timeout in v1.1 (loop ticks are short; if a tick wedges, the operator notices via `--loop-list` showing stale `last_tick_at` and intervenes manually).
- **Cross-platform**:
- POSIX (`sys.platform != "win32"`): `fcntl.flock(fd, LOCK_EX)` for acquire, `fcntl.flock(fd, LOCK_UN)` for release.
- Windows (`sys.platform == "win32"`): `msvcrt.locking(fd, LK_LOCK, 1)` blocking acquire on a 1-byte region; release via `msvcrt.locking(fd, LK_UNLCK, 1)`. `msvcrt` is stdlib on Windows.
- On `BrokenPipeError`/`IOError` from a vanished loop dir mid-hold: surface a clear error `"loop dir vanished mid-lock"` and exit nonzero. Don't mask it.
- File handle is kept open for the life of the `with`; closed in `finally`.
### R2. Wrap every read-modify-write cycle in `status.py`
Locate each `_write_state_loop(...)` callsite in `scripts/status.py` (lines 1721, 1948, 1971, 1993) and confirm each is preceded by a `_read_state_loop(...)` that seeds it. Wrap the read+mutate+write block in `with _loop_lock(loop_path):`. Do NOT wrap the `--create-loop` path (status.py:1819) — there is no prior state to race against; duplicate create is already refused by name (R-of-create-task).
Affected commands in status.py:
- `cmd_check_gate` (halt write) — status.py:1721
- `cmd_pause_loop` (paused) — status.py:1948
- `cmd_resume_loop` (running) — status.py:1971
- `cmd_approve_loop` (halt clear + resumed_count++) — status.py:1993
The lock must cover the read that precedes each of these writes, not just the write. (Wrapping only the write wouldn't close the race — that just makes writes atomic, which they already are.)
### R3. Wrap every read-modify-write cycle in `loop-runner.py`
In `scripts/loop-runner.py`, wrap the read+mutate+write in `cmd_tick`'s step 10 ("atomic state write" per `design/loops/technical.md` §7) and any other `_write_state_loop` callsite (lines 125, 244, 684). Mirror the same `with _loop_lock(loop_path):` pattern.
The lock **must** be held across:
- The `--check-gate` subprocess call's effective decision (i.e. the read of `iteration_count`/`status` it makes), AND
- The subsequent state mutation write.
Since `--check-gate` runs as a subprocess and reads `.state.loop` itself, the lock acquired by the runner blocks the *runner's own* subsequent subprocess read from racing a concurrent approve write, but it cannot lock the *subprocess* itself. This is acceptable: the lock scope we control is the parent runner's read-modify-write; a concurrent tick would block on `.state.lock` at the parent-runner level and the gate call inside it would still see consistent state.
### R4. Idempotence and no-op fast path
If a command reads `.state.loop`, discovers no mutation is needed (e.g. `--pause-loop` on an already-paused loop), it still releases the lock cleanly. The lock MUST always be released, even on early-return code paths inside the `with` block. Use `try/finally` inside the context manager, not inside callers.
### R5. Lock file location
`.state.lock` lives in the loop's own dir (`<loop_path>/.state.lock`), NOT the framework root. Rationale: per-loop granularity; a lock on loop A's tick must not block loop B's approve. Untracked loops (no `.state.loop`) still get a `.state.lock` file on first acquire — that's fine; the file is empty.
### R6. No new pip deps
Use stdlib only: `fcntl` (POSIX), `msvcrt` (Windows), `contextlib`, `sys`, `os`. Both are already conditionally imported elsewhere in the framework (`platform.system()` dispatch in task 5).
### R7. Compatibility with existing atomic write
The existing `_write_state_loop` tmp-then-replace stays. The lock adds a coarse mutex around the read-modify-write cycle; the atomic write provides last-write-wins safety even if some future code path forgets the lock. Defense-in-depth; no regression to the existing atomic semantics.
## Non-goals
- No `--claim-loop-task` (that's task 6).
- No timeout / deadlock detection — out of scope; ticks are short. If a future tick grows long, address then.
- No advisory locking visible to harnesses — internal only; no CLI surface.
- No `outputs.retention` GC (task 3).
- No `blast_radius.base_branch` parameterization (task 4).
## Test plan (`tests/test_state_loop_lock.py`)
New tests, all stdlib, all using `tmp_path`:
1. `test_lock_serializes_concurrent_writes`: two threads kicked off simultaneously, each does read→sleep(0.05)→write under the lock. Assert timestamps don't interleave (one finishes before the other starts its write). Use a shared "interleave detector" (a list append of enter/exit times compared after).
2. `test_lock_releases_on_clean_exit`: acquire+release; the next acquire on the same loop succeeds immediately.
3. `test_lock_releases_on_exception`: `with _loop_lock(p): raise ValueError`; next acquire succeeds.
4. `test_lock_is_per_loop`: two lock acquisitions on two different loop dirs run concurrently without blocking each other (assert both complete within a tightly bounded wall-clock window).
5. `test_no_lock_on_create_loop`: `--create-loop` of a new loop does NOT create a `.state.lock` file (create-path is unwrapped per R2). Then `--check-gate` on it acquires/releases the lock, leaving `.state.lock` behind.
6. `test_pause_loop_serialized_with_concurrent_read`: spawn a thread that holds `_loop_lock` for 0.1s; main thread calls `--pause-loop` and assert it completes after 0.1s (not before). Confirms commands actually acquire the lock.
7. `test_runner_tick_holds_lock_across_state_write`: integration-style — invoke `loop-runner.py --mode tick` against a loop whose tick is artificially delayed, while a parallel `--approve --loop` is held; assert approve completes after the tick. (Skip if it grows flaky — turns into a smoke test asserting the lock file appears.)
Reuse the `_make_loop` helper pattern from `tests/test_status_brakes.py` for loop dir scaffolding.
## Concrete code shape
```python
@contextlib.contextmanager
def _loop_lock(loop_path: Path, exclusive: bool = True):
lock_file = loop_path / ".state.lock"
fd = os.open(str(lock_file), os.O_RDWR | os.O_CREAT, 0o644)
acquired = False
try:
if sys.platform == "win32":
import msvcrt
msvcrt.locking(fd, msvcrt.LK_LOCK if exclusive else msvcrt.LK_NBLCK, 1)
else:
import fcntl
fcntl.flock(fd, fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH)
acquired = True
yield
finally:
if acquired:
if sys.platform == "win32":
import msvcrt
try:
msvcrt.locking(fd, msvcrt.LK_UNLCK, 1)
except OSError:
pass
else:
import fcntl
fcntl.flock(fd, fcntl.LOCK_UN)
os.close(fd)
```
Callers:
```python
with _loop_lock(loop_path):
state = _read_state_loop(loop_path) or _initial_state_loop(name)
state["status"] = "paused"
_write_state_loop(loop_path, state)
```
## D-items (decisions locked for this task)
- **D-L1**: blocking acquire, no timeout in v1.1. Ticks short; operator notices via `--loop-list` stale `last_tick_at`.
- **D-L2**: `.state.lock` is per-loop, lives in the loop dir, not garbage-collected.
- **D-L3**: `--create-loop` path is unwrapped (no prior state to race against; create is name-unique-refused).
- **D-L4**: stdlib only (`fcntl` POSIX, `msvcrt` Windows). No `filelock` package.
- **D-L5**: existing atomic write semantics retained (defense-in-depth).
## Risks
- **Deadlock if a path holds the lock and re-enters a function that tries to re-acquire.** Mitigation: `_loop_lock` is not re-entrant — audit every callsite to ensure no nested `_loop_lock` within the same `with` block. POSIX `flock` is re-entrant on the same fd; Windows `msvcrt.locking` is not. Safer to forbid nesting and document it.
- **Linux `flock` on NFS has known caveats.** Out of scope: the loop dir is always local (project root or `~/.automaton`). Document in the helper's docstring.
## Verification
- `python3 -m py_compile scripts/status.py scripts/loop-runner.py`
- `python3 -m pytest tests/test_state_loop_lock.py -v`
- `python3 -m pytest tests/ -q` (full suite must remain green; 433 baseline + new)
- Manual: `python3 scripts/status.py --create-loop t --from-template ci-triage && python3 scripts/status.py --check-gate t` — should succeed and leave `.state.lock` behind on first acquire.
@@ -0,0 +1,82 @@
# Verdict: add-state-loop-lock
## Status: PASS
## Summary
Closed the TOCTOU read-modify-write race flagged in
`add-status-brakes/ADVERSARIAL_BUG_REPORT.md` (A6) and
`add-loop-runner/ADVERSARIAL_BUG_REPORT.md` (A2, A7) by wrapping the
read-modify-write cycles on `.state.loop` in a cross-process file lock
(`_loop_lock`). POSIX `fcntl.flock(LOCK_EX)`, Windows
`msvcrt.locking(LK_LOCK, 1)`, per-loop granularity, blocking acquire,
no timeout in v1.1. Stdlib only.
## SPEC compliance
| Requirement | Status |
|-------------|--------|
| R1 — `_loop_lock` context manager (POSIX/Windows, blocking, finally-safe FD lifecycle) | ✓ |
| R2 — Wrap status.py `cmd_pause_loop`, `cmd_resume_loop`, `cmd_approve_loop`, `cmd_check_gate` (NOT `--create-loop`) | ✓ |
| R3 — Wrap `cmd_tick`'s step-10 state write; lock covers `_gate` subprocess + state write | ✓ |
| R4 — Idempotence + early-return inside `with` releases cleanly (try/finally in the context manager, not caller) | ✓ |
| R5 — Lock file `<loop_path>/.state.lock` (per-loop granularity) | ✓ |
| R6 — Stdlib only (`fcntl` POSIX, `msvcrt` Windows, `contextlib`, `sys`, `os`) | ✓ |
| R7 — Existing atomic write (`_write_state_loop` tmp-then-replace) retained | ✓ |
| D-L1 — Blocking acquire, no timeout | ✓ |
| D-L2 — `.state.lock` per-loop, not GC'd | ✓ |
| D-L3 — `--create-loop` unwrapped | ✓ |
| D-L4 — Stdlib only, no `filelock` package | ✓ |
| D-L5 — Existing atomic write retained (defense-in-depth) | ✓ |
| D-L6 (new, necessary for SPEC R2+R3 consistency) — Env-var bypass ($AUTOMATON_NO_LOOP_LOCK=1) avoids self-deadlock when the runner spawns the `--check-gate` subprocess inside its held lock | ✓ |
## Bug reports
- BUG_REPORT: 5 non-blocking observations (O1-O5), all documented behaviors.
- ADVERSARIAL_BUG_REPORT: 8 attack vectors probed (A1-A8). One LOW finding
(A4: harness calling loop-control command inside a tick would deadlock;
deferred to harness-integration contract docs follow-up). All others
verified safe.
## Test results
- `python3 -m py_compile scripts/status.py scripts/loop-runner.py` ✓
- `python3 -m pytest tests/test_state_loop_lock.py -v` — 7 passed
- `python3 -m pytest tests/ -q` — **447 passed** (was 440; +7 new; 0 regressions)
- Manual: `--create-loop` does NOT leave `.state.lock` (D-L3 ✓); first
`--check-gate` does; `AUTOMATON_NO_LOOP_LOCK=1 status.py --check-gate`
works (env var bypass exercised).
- Manual adversarial: 5 concurrent `--approve --loop` on a halted loop —
only one wins (code 0, resumed_count=1); 4 re-read inside the lock and
see status=running, exit 1. Race closed.
- Manual adversarial: 5 concurrent `--pause-loop` — all succeed
(idempotent; pause is well-defined on already-paused); final state
consistent.
- Manual adversarial: monkeypatch `_gate` to raise → exit exception →
follow-up `_loop_lock` acquires immediately (lock released in finally).
## D-items applied
- D-L1 to D-L6 all locked (see SPEC compliance table).
## Subprocess-deadlock avoidance
The original SPEC's R2 and R3 contradict each other (both list `--check-gate`
to acquire `_loop_lock` AND the runner to acquire the same lock across the
`--check-gate` subprocess — would deadlock). Resolved via D-L6 (env-var
bypass). The runner sets `$AUTOMATON_NO_LOOP_LOCK=1` in the `--check-gate`
subprocess's env ONLY (scoped via `_run_json`'s `env` kwarg, propagated to
`_gate`'s `subprocess.run`). Harness subprocesses inherit `os.environ`
unchanged (no env var) so their nested `status.py` calls lock normally and
serialize against the runner's outer lock (intended for tasks; harness
typically does `status.py --transition` only which doesn't touch
`.state.lock`). Documented in `_loop_lock`'s docstring + design doc.
## Pipeline
research → research:awaiting_approval → research:approved → implement →
code_review → code_review:awaiting_approval → code_review:approved →
bug_find → adversarial_bug_find → doc_review → referee → complete
Pipeline driven end-to-end. Ready for `--transition complete` (relocates
to `tasks/complete/`).