fix: delete the progress comment once real output exists - #33
Conversation
Every review/triage/interactive run always leaves its own durable output behind on success (a submitted review, a triage comment, an interactive reply), so the generic "Claude is working on this" ping has nothing left to say once that lands -- delete it outright instead of editing it to "finished", so repeat runs on the same pull request or issue stop leaving a trail of near-identical status comments. On a run that does not succeed, the comment still carries a hidden marker so the next run's "Post progress comment" step can find and reuse it instead of posting another one, keeping a string of failed runs down to a single comment too.
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. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Interactive mode has no concurrency group, so two @claude mentions close together can trigger overlapping runs. The prior marker-based reuse matched any progress comment regardless of state, so a run's "Post progress comment" step could adopt a still-in-flight sibling's in-progress comment -- letting that sibling's later delete-on-success silently erase the adopting run's own eventual failure message. Splits the marker into a pending state (set on creation, never eligible for reuse) and a failed state (set only once a run's own "Update progress comment" step has actually resolved to failure). Reuse now searches for the failed marker only, so a comment is never adopted until the run that owns it has genuinely finished. Also scopes the search to bot-authored comments, narrowing an incidental match against unrelated human prose that happens to quote the marker text.
Superseded by re-review on the latest commit.
… ones The prior lookup-then-patch reuse was still a check-then-act race, just on a narrower trigger: two runs starting close together (interactive mode has no concurrency group) could both find the same stale failed comment before either patched it, both take ownership of it, and the one that finishes last silently lose its own outcome to a 404 once the other has deleted or reused that same comment. Replaces adoption with pure cleanup: delete every terminal-FAILED progress comment this bot left behind, then always create a fresh comment this run alone owns for its own lifetime. Deleting is race-free regardless of how many runs attempt it concurrently -- a delete that loses to a sibling's own delete of the same comment just 404s, a harmless no-op -- and no run's own delete-on-success or patch-to-failed can ever land on a comment another run still considers its own. Also threads the failed-state marker through as a step output instead of duplicating the same literal in both steps by hand.
Superseded by re-review on the latest commit.
There was a problem hiding this comment.
Re-review on e780b724. Title and description remain accurate against the diff overall (one minor mechanism-drift note below) — left untouched.
Both open points from my prior review (f76bcd7) are resolved by this commit, and I've resolved their threads: the redesign drops "adopt an existing comment" entirely — every run now deletes any stale failed-marked comment it finds and unconditionally creates its own fresh comment (action.yml:601-609), so no two runs can ever believe they own the same comment id, closing the check-then-act race for good rather than just narrowing it further. MARKER_FAILED is also now genuinely referenced in the stale-lookup filter (action.yml:603) rather than sitting unused. I traced the jq string construction, the retry/backoff loops, and the delete/patch exit paths in both the "Post progress comment" and "Update progress comment" steps and didn't find a new correctness issue.
I also cleared and dismissed my own stale CHANGES_REQUESTED review from f76bcd7, since it no longer reflects this revision.
Nit (inline on the PR body): one sentence in the description describing the mechanism has drifted from what actually ships.
Nothing else in the diff stood out.
|
🎉 This PR is included in version 1.1.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Every review/triage/interactive run posts a generic "Claude is working on this" progress comment and previously always edited it to "✅ Claude finished" at the end -- but never looked for a leftover comment from a previous run on the same pull request or issue, so a PR with several pushes accumulates one near-identical "finished" comment per run. Concretely: PR #32 picked up five of these over one review cycle.
The actual substantive output (a submitted PR review, a triage comment, an interactive reply) always lands separately, per each mode's own prompt -- the progress comment carries no content of its own. So on a successful run there's nothing left for it to say: this deletes it outright instead of editing it, and on a failed run (where it's the only visible signal something went wrong) leaves it in place carrying a hidden marker so the next run's "Post progress comment" step finds and reuses it rather than posting another one.
Verified locally: actionlint and prettier pass, and the printf-based body construction and jq marker-lookup filter were checked against sample data outside the runner. The real end-to-end path -- delete-on-success -- gets exercised by this repository's own claude-review workflow once this PR leaves draft, since claude-review.yml uses
uses: ./against the PR's own checked-out code.