T-5VN7-pr-check-cursor-bootstrap-misses-existing-comments

Status: closed/done · Impact: high · Complexity: small

Goal

/sdlc:pr-check is the orchestrator’s only sensor for “has the human left feedback on this PR?” Two cursor-related gaps still let the orchestrator silently rebase over outstanding feedback or waste dispatches on its own replies:

1. The bootstrap path (cursor file absent) correctly fires NEEDS-RESPONSE for pre-existing comments today — but no fixture pins that behavior, so a future refactor could regress it silently.
2. Orchestrator-authored comments (gh pr comment <N> --body ..., authenticated as the user’s GitHub identity) are indistinguishable from real user feedback. On the next tick pr-check sees the orchestrator’s own reply, returns NEEDS-RESPONSE, and a dispatch is wasted on a no-op.

Today

Cursor file lives at .sdlc/pr-cursors/<N>.json (post the 2026-05-22-move-plugin-runtime-state-to-sdlc-dir migration). Shape:

{
  "last_seen_comment_at": "2026-05-21T17:58:04Z",
  "last_invoked_at":      "2026-05-21T18:20:49Z"
}

Location	Role today
plugin/skills/pr-check/classify_pr.py#read_cursor	Returns last_seen_comment_at: None when cursor file absent — the bootstrap path
plugin/skills/pr-check/classify_pr.py#_is_later	_is_later(at, None) returns True — so every comment counts as “new” on bootstrap
plugin/skills/pr-check/classify_pr.py#classify	When _new_entries(...) is non-empty, emits NEEDS-RESPONSE with the newest author + count
plugin/skills/pr-check/test_classify_pr.py	Tests cursor_seen_at="2026-05-21T10:00:00Z" (steady-state); no bootstrap-with-None test
plugin/skills/pr-check/fixtures/	Seven fixtures, none exercising the bootstrap (cursor-absent + comments) path
plugin/skills/pr-check/SKILL.md	Section 4 documents the cursor schema; no self_posted_at field
plugin/skills/orchestrate/SKILL.md	NEEDS-RESPONSE dispatch launches ad-hoc resolution sub-agents; their prompts post comments via raw gh pr comment

So the bootstrap behavior is correct in the current code; it’s just not pinned by a test. The self_posted_at filter is entirely unshipped.

The bootstrap incident on 2026-05-21 (cursor files seeded to the latest comment’s timestamp, hiding pre-existing comments) was incidentally fixed by commit 6541a60 (the extraction of classify_pr.py from the SKILL.md prose) — the script’s read_cursor returns None for absent cursors, not “latest comment’s timestamp.” That commit’s message claimed “first-call behavior is unchanged” but in fact the rewrite shipped the fix as a side effect. This task now exists to (a) pin the fixed behavior with a regression fixture and (b) ship the self-posted filter the original task body also called for.

Proposed

Two work items, both small:

Bootstrap regression fixture. Add a fixture exercising the “cursor file absent + PR has pre-existing comments” path and a test that runs through the CLI surface with a freshly-empty --cursor-dir. The test asserts:

• Verdict is NEEDS-RESPONSE.
• The reason cites the newest commenter’s login and the count.
• The cursor file is written with last_seen_comment_at equal to the newest comment’s createdAt.

Self-posted filter. Extend the cursor schema with self_posted_at: list[str]. Add a wrapper at plugin/skills/pr-check/post_self_comment.sh that calls gh pr comment <N> --body ..., captures the returned timestamp, and appends it to that PR’s cursor’s self_posted_at array. Update _new_entries in classify_pr.py to filter entries whose createdAt/submittedAt matches any timestamp in self_posted_at. Update plugin/skills/orchestrate/SKILL.md’s NEEDS-RESPONSE dispatch brief to require the wrapper.

Approach

1. Add plugin/skills/pr-check/fixtures/bootstrap.json mirroring the clean.json shape plus one pre-existing third-party comment with a timestamp like 2026-05-21T17:58:04Z.
2. Add a test in test_classify_pr.py that invokes the CLI with --mock-state pointing at bootstrap.json and --cursor-dir pointing at an empty temp dir, then asserts the NEEDS-RESPONSE verdict and the cursor file’s last_seen_comment_at matches the fixture’s comment timestamp.
3. Update classify_pr.py:
   • read_cursor returns self_posted_at: [] when the field is absent (forward-compatible read of pre-extension cursor files).
   • write_cursor writes the self_posted_at array through unchanged.
   • _new_entries accepts a self_posted_at: list[str] arg and filters out entries whose createdAt/submittedAt is in that list.
   • classify(...) accepts and threads the new arg.
   • main() reads cursor["self_posted_at"] and passes it through.
4. Add plugin/skills/pr-check/post_self_comment.sh:
   • Args: <pr-number> <body> (body via a --body-file - heredoc equivalent so newlines are preserved).
   • Calls gh pr comment <N> --body "$body".
   • Reads back the comment’s createdAt via gh pr view <N> --json comments --jq '.comments[-1].createdAt' (or parses the URL output if simpler).
   • Appends that timestamp to <project-root>/.sdlc/pr-cursors/<N>.json’s self_posted_at array, creating the file with the new field if absent (last_seen_comment_at: null placeholder).
5. Add plugin/skills/pr-check/fixtures/self-posted-filter.json mirroring a PR with one comment at T and one cursor file with self_posted_at: [T]. (The fixture is the PR-state JSON only; the cursor file is set up by the test.)
6. Add a test that exercises the self-posted filter path through the CLI: pre-populate a cursor file with self_posted_at carrying the fixture’s only comment timestamp, run the CLI, assert verdict is CLEAN (or whatever the appropriate non-NEEDS-RESPONSE verdict is given the rest of the fixture state).
7. Update plugin/skills/pr-check/SKILL.md:
   • Section 4 cursor-schema example gains the self_posted_at field.
   • The cursor-update procedure mentions that the field is preserved across writes.
   • A new “Self-posted comment filter” subsection documents the wrapper and the dispatch convention.
8. Update plugin/skills/orchestrate/SKILL.md’s NEEDS-RESPONSE dispatch (Step 2 of the resolution sub-agent brief) to require the wrapper instead of raw gh pr comment.

Files to touch

Location	Kind	Change
plugin/skills/pr-check/classify_pr.py	modify	Thread self_posted_at through read_cursor, write_cursor, _new_entries, classify, main
plugin/skills/pr-check/test_classify_pr.py	modify	Add bootstrap-fixture test (AC-1/2/3); add self-posted-filter test (AC-5)
plugin/skills/pr-check/fixtures/bootstrap.json	new	PR state with pre-existing third-party comments — bootstrap regression fixture
plugin/skills/pr-check/fixtures/self-posted-filter.json	new	PR state with one comment whose timestamp matches a self_posted_at cursor entry
plugin/skills/pr-check/post_self_comment.sh	new	Wrapper around gh pr comment that records the posted timestamp into the cursor’s self_posted_at
plugin/skills/pr-check/SKILL.md	modify	Document the self_posted_at field and the wrapper convention
plugin/skills/orchestrate/SKILL.md	modify	Require the wrapper for NEEDS-RESPONSE-dispatched sub-agent replies

Acceptance criteria

• AC-1: bootstrap.json exists and contains at least one pre-existing third-party comment with a known createdAt.
• AC-2: test_classify_pr.py runs the CLI with --mock-state bootstrap.json and an empty --cursor-dir, asserts the verdict is NEEDS-RESPONSE, and asserts the cursor file is written with last_seen_comment_at equal to that fixture’s newest comment timestamp.
• AC-3: post_self_comment.sh posts a comment via gh pr comment AND appends the resulting timestamp to the PR’s cursor’s self_posted_at array (verified by an integration-style test that monkeypatches gh or a shell-level test using a fake gh on $PATH).
• AC-4: classify_pr.py’s classify(...) accepts a self_posted_at: list[str] argument and filters entries whose createdAt/submittedAt matches any entry in that list, so they do not contribute to the new-entries set.
• AC-5: Running the CLI against self-posted-filter.json with a cursor file carrying self_posted_at: [<that-comment-timestamp>] returns the appropriate non-NEEDS-RESPONSE verdict.
• AC-6: plugin/skills/orchestrate/SKILL.md’s NEEDS-RESPONSE dispatch brief instructs sub-agents to post comments via ${CLAUDE_PLUGIN_ROOT}skills/pr-check/post_self_comment.sh rather than raw gh pr comment — verified by a grep-style assertion in the test suite (or by reading the file in the post-mortem).
• AC-7: plugin/skills/pr-check/SKILL.md Section 4 documents the self_posted_at cursor field and references the wrapper.

Out of scope

• Re-classifying every cursor file currently on disk. The forward-compatible read_cursor (treating absent self_posted_at as []) handles the transition; no migration script is needed.
• Distinguishing comment authors purely by GitHub identity (bot-vs-human heuristics). The self_posted_at filter is precise where it applies; broader bot detection is a separate problem.
• Fixing the bootstrap behavior in the code itself — it’s already correct in classify_pr.py (incidentally shipped via 6541a60). This task only adds regression coverage for that behavior.
• Re-running the affected PRs (#74, #75, #76, #77, #80) through pr-check post-fix. Those cursors have already advanced past their bootstrap moment; the relevant signal is whether new comments arrive, not what state pre-existed.

Dependencies

• none

Discovery context

Surfaced 2026-05-21 during a live /sdlc:orchestrate tick. Six open PRs had user-authored change-request comments; only one (#78) was correctly classified NEEDS-RESPONSE. The other five (#80, #77,

76, #75, #74) returned CLEAN because their cursor file already

contained last_seen_comment_at equal to the latest comment’s timestamp — the bootstrap had advanced the cursor past those comments without first firing NEEDS-RESPONSE.

After resolution sub-agents posted reply comments on the same tick, the very next /sdlc:orchestrate returned NEEDS-RESPONSE for PR

74 — but the two “new comments” were the orchestrator’s own

replies, authenticated as the user’s GH identity via the shared gh CLI session. That false-positive motivated extending this task with the self_posted_at field and the post_self_comment.sh wrapper.

Update 2026-05-23 (relevance check during /sdlc:task-work)

The bootstrap bug described above turned out to have been incidentally fixed by commit 6541a60 (feat(pr-check): extract verdict emission into deterministic classify_pr.py, 2026-05-21): the new read_cursor returns last_seen_comment_at=None when the cursor file is absent, and _is_later(at, None) returns True, so every comment counts as “new” on bootstrap. The commit message claimed “first-call behavior is unchanged” — in fact the rewrite shipped the fix as a side effect.

The original task’s AC-1 and AC-2 are therefore already satisfied by behavior, but no regression test pins that behavior. This task now exists to (a) add the regression fixture so a future refactor can’t silently re-break the bootstrap, and (b) ship the self_posted_at filter that the original task body also called for but never landed. The path drift the original noted (.claude/ → .sdlc/pr-cursors/) has also already shipped via 2026-05-22-move-plugin-runtime-state-to-sdlc-dir.

Post-mortem

Captured by /sdlc:task-work on 2026-05-23. PR: pending.

Acceptance criteria coverage

• AC-1: auto — plugin/skills/pr-check/fixtures/bootstrap.json carries one third-party comment by drive-by-reviewer at 2026-05-21T17:58:04Z; pinned by test_cli_bootstrap_path_fires_needs_response.
• AC-2: auto — test_cli_bootstrap_path_fires_needs_response exercises the CLI with --mock-state bootstrap.json + empty --cursor-dir, asserts NEEDS-RESPONSE drive-by-reviewer 1 new comment, and asserts the cursor’s last_seen_comment_at equals the fixture’s comment timestamp.
• AC-3: auto — post_self_comment.sh posts via gh pr comment, re-queries gh pr view --json comments --jq '.comments[-1].createdAt' for the timestamp, and uses jq to append it to the cursor’s self_posted_at array; two tests with a fake gh on $PATH cover fresh-cursor + existing-cursor paths.
• AC-4: auto — classify(...) and _new_entries(...) accept self_posted_at: list[str] | tuple[str, ...] = () and filter entries whose createdAt/submittedAt is in that set; three unit tests pin the filter (comments path, reviews path, full-classify path).
• AC-5: auto — test_cli_self_posted_filter_suppresses_needs_response pre-populates <cursor-dir>/88.json with self_posted_at: ["2026-05-21T18:00:00Z"], runs the CLI against self-posted-filter.json, and asserts the verdict starts with CLEAN (not NEEDS-RESPONSE) and that the field is preserved across the cursor write.
• AC-6: auto — plugin/skills/orchestrate/SKILL.md’s NEEDS-RESPONSE bullet now contains a “For NEEDS-RESPONSE specifically” paragraph instructing sub-agents to post via the wrapper rather than calling the raw GitHub-CLI comment command directly; verifiable via grep post_self_comment.sh plugin/skills/orchestrate/SKILL.md.
• AC-7: auto — plugin/skills/pr-check/SKILL.md Section 4 documents self_posted_at in the cursor schema, mentions field preservation across writes, and adds a “Self-posted comment filter” subsection referencing the wrapper.

What worked

• The relevance check at Step 2 caught that the original task’s AC-1/AC-2 were already shipped (incidentally, via 6541a60) before any worktree was created. The doc rewrite that followed scoped the work to what was actually unshipped (regression coverage + self-posted filter), avoiding a re-implementation of behavior already in the codebase.
• The sub-agent dispatch in Step 6 produced five focused, well-ordered commits (one per AC group). Each commit message accurately describes its scope and matches the AC it satisfies.
• pyyaml block-scalar conversion of relevance_note/completion_note in a closed task (during the drift-fix PR) was byte-identical content — verifiable via git diff against the pre-conversion plain scalar.

Friction and automation gaps

• audit_entities.py quality-check failed on 5 pre-existing drift items unrelated to this branch — required a separate chore/audit-drift-fix PR before this PR could pass the gate. The task 2026-05-21-run-quality-checks-isolates-pre-existing-drift would close this loop by making the gate fail only on drift the current branch introduced. (This is the second consecutive task-work run that hit this same friction — PR #101 in the prior session had the same shape.) → T-H69K-run-quality-checks-isolates-pre-existing-drift
• The sub-agent’s first-pass attempt to wire gh pr comment ... mentions into plugin/skills/pr-check/SKILL.md would have tripped pr-check’s invariants.yaml forbidden-phrase rule. The sub-agent caught it manually and rephrased around “the raw GitHub-CLI comment command”, but a pre-flight lint check during authoring (the linter that already exists, just run inline in the sub-agent’s loop after each Edit) would catch this class of accidental invariant violation faster than the post-hoc gate. → T-407I-subagent-invariant-preflight-lint
• Local main got fast-forwarded mid-session (PR #97 merged externally), leaving the task branch rooted off a stale main commit with foreign files (docs/decisions/0001-github-ref-leases.md flat file vs the new github-ref-leases/ folder structure) in the diff against origin/main. check_ancestry.py reported clean because it only flags chore(tasks): start <other> foreign commits, not unrelated upstream restructures. Step 9’s rebase resolved it cleanly, but the helper’s clean verdict was misleading — diff-against-origin-main was non-trivial yet the verdict didn’t say so. The helper could grow a second mode that distinguishes “clean from parallel task-work contamination” from “task branch’s diff vs origin/main contains only this task’s files.” → T-61OI-check-ancestry-flags-stale-base
• The sub-agent stashed an unrelated working-tree dirty file (docs/decisions/0001-github-ref-leases.md) that turned out to no longer exist on the post-rebase tree. The stash is still in the stash list (stash@{0}) and orphaned. start_task.py (or a Step 4 hook) could check for pre-existing stashes referencing files that no longer exist on the target rebase and prompt to drop them. → T-XKWA-start-task-drops-orphan-stashes

Spawned follow-up tasks

• T-H69K-run-quality-checks-isolates-pre-existing-drift — linked-existing; bullet explicitly named this task as the tracking item for the audit_entities pre-existing-drift gate friction.
• T-407I-subagent-invariant-preflight-lint — created; wire lint_skill_prose.py into the task-work Step 6 sub-agent loop to catch invariants.yaml forbidden-phrase violations pre-flight.
• T-61OI-check-ancestry-flags-stale-base — created; grow check_ancestry.py to detect stale-base contamination (diff vs origin/main contains unrelated upstream files), not just parallel-task chore(tasks): start foreign commits.
• T-XKWA-start-task-drops-orphan-stashes — created; start_task.py scans the stash list post-rebase for entries whose paths are missing on the new tree and prompts to drop them.

---

← Back to Tasks