Skip to content

fix(pr-monitor): decide comment new-ness by id and timestamp, not body text - #13

Merged
aliasunder merged 3 commits into
mainfrom
pr-monitor-both-endpoints
Sep 6, 2026
Merged

fix(pr-monitor): decide comment new-ness by id and timestamp, not body text#13
aliasunder merged 3 commits into
mainfrom
pr-monitor-both-endpoints

Conversation

@aliasunder

Copy link
Copy Markdown
Owner

Summary

Closes the comment-reading gap that caused a real monitoring misfire on vault-cortex PR #538: after a re-review event, an issue-comments-only query re-read the round-1 comment as a "duplicate" on two consecutive passes while the bot's actual new findings — posted as inline review comments — went unseen.

  • Step 2d gains a "new-ness is decided by timestamps and ids, never by body text" rule: after any re-review event, list BOTH endpoints (inline review comments and issue comments) with id and created_at, and treat a comment as new only if it postdates the run that produced it. A "duplicate posting" verdict requires two distinct comment ids.
  • Step 4.3 (follow-up comparison) now names ids and created_at as the comparison keys.
  • Step 3 opens with a bolded callout for the pipeline attribution footer — the reply templates already carried it, but the transition from direct posting into a pipeline cycle was easy to miss (also observed on #538).

Tests

Prose-only skill change — no executable surface. Verified the three edited sections render correctly and the surrounding step numbering and flow are unchanged.

🤖 Generated with Claude Code

…y text

After a re-review event, the comment-reading step now requires listing
both endpoints (inline review comments and issue comments) with id and
created_at, and treating a comment as new only if it postdates the run
that produced it. Identical text with a familiar marker means the same
comment was re-read, not re-posted. Also bolds the pipeline attribution
footer at the top of Step 3 — the reply templates carried it, but the
transition from direct posting into a pipeline cycle was easy to miss.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread plugins/ship-check/skills/pr-monitor/SKILL.md
@umm-actually

umm-actually Bot commented Sep 6, 2026

Copy link
Copy Markdown

umm-actually re-reviewed at 5fd6e9f

No new findings (3 tracked finding(s) across all runs).

Context notes
  • Priority docs already in context: plugins/ship-check/skills/pr-monitor/SKILL.md

umm-actually · deepseek/deepseek-v4-flash-0731

The new-ness rule requires listing inline review comments with id and
created_at, but 2d's fetch block never queried /pulls/N/comments — an
agent following the skill literally still could not see them. Adds the
endpoint to the fetch block and the proof-of-work listing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Comment thread plugins/ship-check/skills/pr-monitor/SKILL.md
Comment thread plugins/ship-check/skills/pr-monitor/SKILL.md Outdated
… per-surface timestamps

The empty-listing self-report now names all three fetched surfaces, and
Step 4.3 scopes the timestamp comparison by surface — review bodies
expose submitted_at, not created_at.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@aliasunder
aliasunder merged commit e07c6f4 into main Sep 6, 2026
7 checks passed
@aliasunder
aliasunder deleted the pr-monitor-both-endpoints branch September 6, 2026 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant