fix(verify,diagnostics): dw3-dw5 ledger sweeps (DW-40 … DW-94) - #760
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
19d2a4c to
0ce71f5
Compare
0ce71f5 to
9ae77d6
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
9ae77d6 to
387e394
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 387e3949ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - `git diff --check` -- expected: no whitespace errors. | ||
| - `git diff -- _bmad-output/implementation-artifacts/deferred-work.md` -- expected: empty. | ||
|
|
||
| ## Auto Run Result |
There was a problem hiding this comment.
Remove this generated implementation record from the commit: its Auto Run Result, verification transcript, and review-triage log are session/run artifacts rather than durable documentation, and the repository explicitly prohibits committing these records. Keeping it tracked also bypasses the _bmad-output/ ignore rule and leaves transient execution history in the source tree.
AGENTS.md reference: AGENTS.md:L71-L71
Useful? React with 👍 / 👎.
387e394 to
d540f15
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
d540f15 to
a7bce9f
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
a7bce9f to
85d587a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85d587a5e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state = load_state(run_dir) | ||
| current_engine = read_pid_identity(run_dir) | ||
| current_liveness = engine_liveness(run_dir) | ||
| rival_published_engine = current_liveness != "dead" and current_engine != addressed_engine |
There was a problem hiding this comment.
When the recorded PID still exists but its process identity cannot be read, alive_and_ours() returns false earlier in _stop_run_once and the local pid is replaced with None, while this later read returns current_liveness == "unknown" and retains the recorded PID in current_engine. This condition therefore always identifies the unchanged engine as a rival, returns None, and makes stop_run() retry forever. Preserve the original PID-file tuple for the generation comparison so an unverifiable engine does not create an infinite stop loop.
Useful? React with 👍 / 👎.
| validate_adapter_artifact_paths( | ||
| task_dir, | ||
| (task_dir / "messages.json",), | ||
| ) |
There was a problem hiding this comment.
When a reused OpenCode task directory contains a symlink, hardlink, FIFO, or device named heartbeat.json, resultless-stops.jsonl, or session-lifecycle.jsonl, this validates only messages.json even though the inherited _ResultFileMixin later writes all three omitted files. The heartbeat overwrite can truncate a linked external file, and diagnostic appends can modify or block on redirected entries, so include every adapter-written task artifact in this pre-write validation as the generic adapter does.
Useful? React with 👍 / 👎.
5ca7732 to
bd3b74a
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
bd3b74a to
768516c
Compare
|
@codex review |
953fa94 to
85a1038
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
85a1038 to
ec60b45
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec60b45883
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| resolution_recorded=False, | ||
| ) | ||
| except RearmError as e: | ||
| with state_lock(run_dir): |
There was a problem hiding this comment.
When a concurrent CLI resume has acquired this lock but has not yet published its PID, the preceding liveness check passes and this synchronous modal callback blocks here. _prepare_resume_locked holds the same lock across configuration, skill, profile, and Git preflight, and the POSIX lock wait is unbounded, so the Textual event loop can stop repainting and accepting input for an extended period. Acquire the lock and perform the re-arm in a worker, or use a nonblocking acquisition and report the contention.
Useful? React with 👍 / 👎.
ec60b45 to
afe18c1
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…59, DW-60, DW-61, DW-62, DW-63 via bmad-loop
…POSIX-only imports Two rows fake a non-Windows `sys.platform` to pin the POSIX half of a gate. The faked branch reaches `import fcntl`, which does not exist on Windows, so they raise ModuleNotFoundError there rather than testing anything. They predate this series; the sweep only extended `diagnostics.collect` far enough to reach that import. Skip them off POSIX — the Linux legs still hold the row. The replacement-directory row deleted the run dir and recreated it, assuming the new directory would land on a different inode. A filesystem is free to reuse the inode it just released, and CI's does: `os.path.samestat` then reads the replacement as the very directory the composer claimed, the guard correctly stays silent, and the row failed on an empty warning. Allocate the replacement while the original is still live — two directories that exist at once cannot share an inode — then rename it into place.
Second site with the same version-dependent premise as the resolve row: a hardcoded `getrecursionlimit() * 20` that 3.13 cannot decode but 3.14 parses iteratively, so the row failed on "DID NOT RAISE" rather than on the degradation it exists to pin. Take the depth from the conftest probe.
… not as cleared `_stop_run_once` clears its local `pid` on every path that declines to signal — gone, reused, or an identity it cannot read — and then compared the post-delivery pid file against that cleared tuple. An engine whose pid exists but whose identity cannot be read (win32 ERROR_ACCESS_DENIED) is `"unknown"`, not `"dead"`, so the unchanged file read as a rival that had just published a new engine, `_stop_run_once` answered "retry", and `stop_run` — unbounded by design, so a real rival is always sent the stop — never returned. Keep the tuple as read for the generation compare. A rival is a CHANGED pid file, nothing else; the unverifiable engine then takes the fallback the pre-split `stop_run` always took for it. The test wraps `_stop_run_once` so the livelock reddens instead of hanging the suite.
… writing `OpencodeHttpAdapter.start_session` refused a redirected `messages.json` — its own file — but not a symlinked, hardlinked, FIFO or device `heartbeat.json`, `resultless-stops.jsonl` or `session-lifecycle.jsonl`, all written by the inherited `_ResultFileMixin`. A reused task directory carrying one of those let the heartbeat overwrite truncate a linked external file and the breadcrumb appends block on or redirect into it, which GenericAdapter already refuses. The three names move into `generic.RESULT_FILE_ARTIFACTS`, handed to `validate_adapter_artifact_paths` by both adapters, so a fourth mixin write cannot reach one adapter's validation and miss the other's. The test is parametrized over that tuple for the same reason.
The per-run state lock DW-94/DW-93 added to `delete_run`/`archive_run` is new in this branch — on main neither took a lock — and `file_lock` raises a plain `OSError` when the acquisition fails. That escaped `cmd_clean`'s per-candidate handler and aborted the whole invocation: later candidates were never processed, and runs the loop had already mutated vanished from a report only emitted after the loop. `file_lock` now raises a typed `LockUnavailableError` for a failed acquisition alone (never the locked body, never the lock-file create), and `cmd_clean` catches it beside the lifecycle races, classifying the run by what actually happened and continuing. Typed rather than a bare `OSError` so `UnconfinedWriteError` — also an `OSError` — is never folded into "left untouched". `clean` acquires with `wait_for_lock=False`: `fcntl.flock` never times out, so waiting was unbounded on POSIX, and a lock someone else holds already means what `clean` reports anyway. The flag is threaded through `state_lock` and defaults to blocking, leaving every other caller unchanged.
The entry ran 13 lines against 5- and 9-line neighbours, carrying DW ids, the exception taxonomy and root-cause narration that belong in docstrings and git history. Keep the what and the user-facing why in 5 lines.
…shboard `_do_rearm` took the run's state lock blocking, on Textual's message loop — no `@work`, and its sole caller is the synchronous `push_screen` dismiss callback. On POSIX that wait is unbounded (`fcntl.flock` never times out), and the realistic holder is a rival `resume`, which keeps the lock across config, skills, profiles and a git preflight bounded only by `[limits] git_timeout_s` (120s by default) while publishing its pid last, so the modal's `_engine_possibly_live` gate reads dead for that whole window. Measured: 3.00s against a 3s holder, 8.00s against an 8s holder. Acquire with `blocking=False` and report the contention as a toast. The new `except LockUnavailableError` arm MUST precede the existing `except (RearmError, OSError, runs.StateRootError)`: the error is an `OSError` subclass, so the reverse order makes it dead code and files contention as "re-arm failed". Waiting bought nothing anyway — the post-lock liveness re-check refuses a re-arm against a live rival. The `finally`'s residue echo is skipped on contention: a refused acquisition ran nothing, so any record appended in that window belongs to the holder.
afe18c1 to
f125d52
Compare
1 participant
What
Fourth of five stacked PRs draining Wave 2's deferred-work ledger. Closes DW-40, DW-53, DW-54,
DW-56 … DW-63, DW-67, DW-74 … DW-77, DW-80, DW-84, DW-86, DW-87, DW-89, DW-91, DW-93, DW-94
(24 entries) — the
dw3/dw4/dw5sweep batches.Why
The tail of the ledger: the entries the later sweep passes reached once the earlier slices had
settled the contracts they depend on. Several are documentation-of-record for decisions taken
during Wave 2's review (DW-91, DW-93) rather than code changes.
How
DW-54).
verify.py's testfixtures.
hardening (DW-86, DW-89).
docs (DW-67, DW-75, DW-87); run lifecycle resume exclusion (DW-94).
Testing
uv run pytest -q -n logical,uv run pyright, andtrunk check --all --no-fixall clean atthis tip. Negative assertions ablated before being trusted.
Changelog
Entries land under
## [Unreleased]inCHANGELOG.md.Stack (merge bottom-up): S1 → S2 → S3 → S4 (this) → S5. Base is S3, not
main.