Repository navigation
Conversation
Record operation intent and verified results in a session-local action log so maintainers can revisit earlier PR decisions and recover reversible changes without discarding unrelated edits. Keep uncertain writes and irreversible effects explicit, preserve legacy session caches, and reuse the existing readiness guards and atomic writes. Closes apache#74 Generated-by: Codex (GPT-6)
Kaap10
left a comment
Kaap10
left a comment
There was a problem hiding this comment.
Thanks for working on this! Adding structured action journaling and guarded recovery directly addresses #74 and is a great ergonomics and safety improvement.
The overall design - evidence-based receipts, reusing readiness guards, digest verification, and dual-tree parity is very solid.
I tested the branch locally and left a few inline comments on edge cases (mainly a Windows test assertion failure and minor recovery/formatting details).
| result = recovery.history(path, REPO, VIEWER) | ||
| assert [a["pr"] for a in result["actions"]] == [12, 13] | ||
| assert "Please fix CI" not in json.dumps(result) | ||
| assert path.stat().st_mode & 0o077 == 0 |
There was a problem hiding this comment.
This assertion fails when running tests on Windows / NTFS:
assert path.stat().st_mode & 0o077 == 0
# AssertionError: assert (33206 & 63) == 0On Windows, os.stat().st_mode only reflects read-only flags and does not support POSIX group/other permission bits (0o077).
Could we guard this check for POSIX platforms?
if sys.platform != "win32":
assert path.stat().st_mode & 0o077 == 0(Also mirror the same guard to any other file permission assertions in this suite).
| if current != expected: | ||
| raise ValueError("the Magpie block changed after this action") | ||
| if current is not None: | ||
| return body.replace(current, previous or "", 1) |
There was a problem hiding this comment.
When recovering from an action that added a brand new managed block (i.e. previous=None or ""), replacing current directly with "" leaves behind the two leading newlines (\n\n) that splice() originally prepended.
While this doesn't break parsing, it can cause trailing blank lines to accumulate at the bottom of the PR description if an issue undergoes multiple triage and recovery cycles. Consider stripping trailing whitespace or normalizing trailing newlines when previous is falsy.
| for a in log["actions"] | ||
| ] | ||
| recorded = {str(a["pr"]) for a in log["actions"]} | ||
| legacy = {**data["prs"], **log.get("legacy_prs", {})} |
There was a problem hiding this comment.
if pr not in recorded or pr in log.get("legacy_prs", {}):If a PR was previously recorded in data["prs"] as a legacy entry and then acted upon in the current session (adding it to both log["actions"] and log["legacy_prs"]), checking pr in log.get("legacy_prs", {}) will append a duplicate row with status: "legacy" alongside the action entry.
Is this intentional so maintainers can see the pre-journal historical state, or should active session actions take precedence in the history listing?
| raise ValueError("--revisit of a journaled PR needs upstream_repo in project.md") | ||
| log = triage_recovery.history(Path(args.session), cfg.upstream_repo, args.viewer) | ||
| logged_prs = {entry["pr"] for entry in log["actions"]} | ||
| if str(args.revisit) not in session and args.revisit not in logged_prs: |
There was a problem hiding this comment.
if str(args.revisit) not in session and args.revisit not in logged_prs:logged_prs is populated with entry["pr"] which are ints, whereas keys in session are strings (e.g. "12").
The membership checks str(args.revisit) not in session and args.revisit not in logged_prs handle the types correctly here, but for consistency and defensive parsing across legacy caches, casting both to str (or both to int) prevents any subtle KeyError edge cases when loading handwritten or migrated session caches.
| or comment.get("updated_at") != effect["comment_updated_at"] | ||
| ): | ||
| raise ValueError(f"comment {ident} changed or belongs to another actor/PR") | ||
| return {"argv": ["gh", "api", f"repos/{repo}/issues/comments/{ident}", "-X", "DELETE"]} |
There was a problem hiding this comment.
Verifying all four invariants (issue_url, user.login, body, and updated_at) before returning the DELETE command is an excellent safety guard. It ensures we never accidentally delete a comment that was subsequently edited by the author or that belongs to a different thread. Nicely done!
Summary
Type of change
Test plan
Independent correctness, security, and conventions reviews completed with no remaining findings.
RFC-AI-0004 compliance
Linked issues
Closes #74
Gen-AI disclosure
Codex assisted with implementation, testing.
I reviewed and responsible for the contribution.