Activity

1. ableinc

   added

   agent-ready

   agent-working

   on Aug 26, 2026
2. ableinc commented on Aug 26, 2026

   

   ableinc

   on Aug 26, 2026

   OwnerAuthor

   More actions

   Plan

   Fix PR-comment pickup: stop discarding every comment written by the account the daemon runs as, and make each skip visible

   Context

   Issue #14 reports that @coding-agent mentions on the daemon's own pull requests are never acted on, draft or not.

   The root cause is in pendingMentions (internal/orchestrator/prcomments.go:120):

   if botLogin != "" && strings.EqualFold(c.Author, botLogin) {
       continue
   }

   botLogin is gh api user --jq .login — the account the daemon's gh is authenticated as. The documented deployment is that this is your own account (README.md:88 "authenticated with … gh auth login", and README.md:678 / the systemd section: "run this via sudo from your own already-authenticated account … normally your own account, not a separate one"). So in the default single-account setup the reviewer's login is botLogin, and every comment the reviewer writes is dropped as "the daemon's own comment" before the mention, association, and age checks ever run. Nothing is logged when this happens, which is why the report reads as "it just doesn't pick up comments".

   The filter is also redundant for its stated purpose. Every comment the harness posts is built by internal/orchestrator/report.go and carries markerPrefix (<!-- coding-agent-loop: — phase.go:28); pendingMentions already drops those via isAgentComment one line earlier. All six comment builders (planComment, issueComment, adoptedComment, failureComment, prCommentComment, prCommentFailureComment) emit a marker, and they are the only bodies passed to GH.Comment/GH.CommentOnPR. Runaway self-triggering is additionally bounded by the pr_comment_tasks table, which marks each comment id done exactly once.

   Two secondary defects found while tracing the path, both of which block diagnosing this class of bug:

   1. Every rejection in tickPRComments is a silent continue — repo busy, PR not open, branch prefix mismatch, no pending mentions. An operator has no way to see which filter fired.
   2. --dry-run does not apply to the PR-comment path. Options.Rehearse is only consulted in work() (loop.go:408); tickPRComments/workPRComments ignore it, so coding-agent-loop --once --dry-run will really clone a worktree and really run Claude against a PR comment, contradicting README.md ("no Claude run, no clone, no mutation, no cost") and making the obvious way to test this fix unsafe.

   Intended outcome: a mention from the repo owner on an agent-opened PR is picked up on the next poll, in the default single-account setup; and when it still isn't, the reason is one --log-level debug line or one --explain-pr invocation away.

   Approach

   1. Stop dropping self-authored comments — internal/orchestrator/prcomments.go

   • Delete the unconditional botLogin skip in pendingMentions. isAgentComment(c.Body) remains and is the accurate test for "the harness wrote this".
   • Keep the botLogin parameter, but apply it only when the new cfg.IgnoreSelf knob is set (below), preserving strict behaviour for anyone running a dedicated bot account.
   • Update the doc comment on pendingMentions to say why: the daemon's own comments are recognised by their marker, not by their author, because daemon and reviewer are normally the same GitHub account.

   2. New config knob — internal/config/config.go, config.example.json, README.md

   Add to PRCommentsConfig:

   // IgnoreSelf drops comments authored by the account the daemon is
   // authenticated as. Default false: the daemon normally runs under the
   // reviewer's own account, so dropping those drops every real request.
   // Set true only when a dedicated bot account is used.
   IgnoreSelf bool `json:"ignore_self"`

   Zero value is the fixed behaviour, so existing config.json files pick up the fix with no edit. config.Load decodes over config.Default() with DisallowUnknownFields, so adding a field is backward compatible; leave it out of config.Default() (false) and add "ignore_self": false to config.example.json (the tracked template that embedded.go compiles in as config.json). No Validate() change needed.

   3. Make every skip visible — internal/orchestrator/prcomments.go

   • Change pendingMentions to return ([]gh.PRComment, []skippedMention) where type skippedMention struct { Comment gh.PRComment; Reason string }, with one reason string per filter ("own marker comment", "no mention of @coding-agent", "author association NONE not permitted", "older than max_age", "already addressed", "in back-off until …", "authored by the daemon and ignore_self is set").
   • Extract the PR-level checks currently inlined in tickPRComments into a pure, testable prSkipReason(pr gh.PullRequest, cfg config.Config) string covering state != OPEN and the workspace.branch_prefix rule (the hard safety rule stays exactly as it is — this is a refactor for testability and logging, not a relaxation).
   • In tickPRComments, log each rejection at Debug with repo/pr/reason, and emit one summary line per PR examined: o.log.Debug("pr comments scanned", "repo", …, "pr", …, "comments", len(comments), "pending", len(pending), "skipped", len(skipped)). Log the repo-busy and excluded-repo skips too.

   4. Second discovery path so search-index gaps can't hide a PR — prcomments.go, internal/store/store.go

   gh search prs --author <login> is the only way a PR is currently found; it depends on the GitHub search index (which lags), on --author matching exactly, and on CurrentLogin succeeding at all (today a CurrentLogin error aborts the whole pass with an Error log).

   • Add func (s *Store) RecentRepos(ctx context.Context, since time.Time, limit int) ([]string, error) to internal/store/store.go, using SELECT repo, MAX(started_at) AS last FROM runs WHERE started_at > ? GROUP BY repo ORDER BY last DESC LIMIT ? (started_at is unix seconds, matching ListRuns; the runs_started_at index already exists).

   • In tickPRComments, build the candidate set as the union of:

     • GH.SearchPRs(login, owners, cfg.SearchLimit) (as today), and
     • for each repo from RecentRepos(now-30d, cfg.SearchLimit) that passes Config.GitHub.Owned and !Excluded, the existing GH.ListPRs(ctx, repo, "open", 100).

     Dedupe on repo#number. ListPRs already selects number,url,body,title,headRefName,state,…, so those candidates need no ViewPR call at all; only search hits (number/title/url only) need the existing ViewPR to fill in headRefName/state. Authorship is not needed for the fallback: the branch_prefix rule already restricts it to branches this daemon created.

   • Downgrade the currentLogin failure from "log Error and return" to "log Warn, skip the search leg, continue with the fallback leg" — a bot/app token where gh api user returns no login should not disable the feature outright.

   5. Honour --dry-run on the PR-comment path — prcomments.go

   In workPRComments, before TryClaim, add:

   if o.opts.Rehearse { o.rehearsePRComments(log, cand, pr, pending); return }

   rehearsePRComments logs the PR, the branch it would push to, and one line per pending comment (id, author, first line of the body) and returns — no claim, no run row, no reactions, no clone, no Claude. Mirrors rehearse() (loop.go:482) in shape and log wording.

   6. Operator diagnostic — --explain-pr owner/name#N

   • Export from prcomments.go:

     type MentionDecision struct { Kind string; ID int64; Author, Association string
                                   CreatedAt time.Time; Pending bool; Reason, Excerpt string }
     type PRExplanation struct { Repo string; Number int; State, HeadRefName, Login string
                                Eligible bool; Reason string; Comments []MentionDecision }
     func (o *Orchestrator) ExplainPR(ctx context.Context, repo string, number int) (PRExplanation, error)

     It reuses currentLogin, GH.ViewPR, GH.PRComments, Store.PRCommentTasks, prSkipReason, and pendingMentions — read-only, no claim, no mutation, no Claude.

   • In cmd/agent.go: add flag.StringVar(&f.explainPR, "explain-pr", "", "explain why a pull request's comments are or aren't picked up (owner/name#123), then exit"), a line in usageHeader, and a branch just before if f.once that parses owner/name#N, calls orch.ExplainPR, prints a table, and returns. Reuse the existing wiring above it verbatim.

   • Also extend the existing log.Info("start-up checks passed", …) in bootCheck with "pr_comments", cfg.GitHub.PRComments.Enabled, "mention", cfg.GitHub.PRComments.Mention, and the resolved gh login (via ghClient.CurrentLogin, logged as a non-fatal Warn if it fails), so the daemon states who it thinks it is at boot.

   7. Docs

   • README.md "Responding to PR comments" step 2: replace "the daemon skips its own comments and marker-tagged replies" with the accurate rule — replies the harness itself wrote are recognised by their marker, so a mention still works when the daemon runs under your own account; ignore_self restores the strict behaviour.
   • Config reference table: add a github.pr_comments.ignore_self row.
   • Safety boundaries bullet list: adjust the self-comment sentence, note that --dry-run now rehearses PR-comment work too.
   • Troubleshooting table: add a row — "A @coding-agent comment on an agent PR is never picked up" → run coding-agent-loop --explain-pr owner/name#123, or restart with --log-level debug and look for pr comments scanned.
   • CLI usage block: add the --explain-pr line.

   Tests

   • internal/orchestrator/prcomments_test.go
     • TestPendingMentions: comment id 6 ("bot-login", "@coding-agent from myself") flips from "must not be pending" to must be pending — this is the BugFix: Reading PR Comments Broken #14 regression test. Add an explicit case that a marker comment authored by bot-login is still skipped.
     • New TestPendingMentionsIgnoreSelf: with IgnoreSelf: true, the same comment is skipped with the self-authored reason.
     • New test that every skipped comment comes back with a non-empty, distinct reason.
     • New TestPRSkipReason: closed PR, branch outside branch_prefix, and a good PR.
     • New test for the candidate merge/dedupe helper: a PR present in both the search leg and the ListPRs leg is examined once, and the ListPRs copy (with headRefName) is preferred so no ViewPR is needed.
   • internal/store/store_test.go: TestRecentRepos — repos ordered by most recent run, older than the window excluded, limit honoured.
   • internal/config/config_test.go: ignore_self defaults to false and round-trips from JSON.
   • internal/orchestrator/realdata_sanity_test.go: add a SANITY_PR=owner/name#123 opt-in check in the same style as TestAgainstRealRepository (skipped unless the env var is set, dry-run gh client) that prints the ExplainPR result for a live PR. This is the direct confirmation against the PR that prompted BugFix: Reading PR Comments Broken #14.

   Verification

   1. make test (or go test ./...) — new and updated unit tests pass; make lint (vet + fmt) clean.
   2. Against the real PR from the issue, read-only:
      • go run ./cmd --explain-pr ableinc/coding-agent-loop#<pr> — expect the reviewer's @coding-agent comment to show pending=true. On the current main build the same command (once it exists) would show the self-authored skip; that contrast is the proof.
      • SANITY_PR=ableinc/coding-agent-loop#<pr> go test ./internal/orchestrator -run TestAgainstRealPR -v.
   3. coding-agent-loop --once --dry-run --log-level debug — confirm the PR is discovered, the pr comments scanned line shows pending>=1, and that nothing is cloned and Claude never starts (this exercises item 5; verify no worktree appears under workspace.root and no run row is created).
   4. Live end-to-end: comment @coding-agent <small request> on an open agent PR, run coding-agent-loop --once, and confirm 👀 → reply → 👍 and a push to the PR branch. Re-run --once and confirm the comment is not picked up a second time (the pr_comment_tasks dedupe still holds, now with a self-authored comment).

   Risks and decisions for the reviewer

   • Security posture change. After this, a comment from the daemon's own account can trigger a bypassPermissions Claude run. That is the intended fix (in the default setup that account is the human reviewer), and the association/allowlist check still gates everyone else, but it does widen who can trigger a run by exactly one account. The loop-safety argument rests on two existing properties: every harness-authored comment carries a marker, and pr_comment_tasks acts on each comment id at most once. ignore_self: true is the escape hatch. If the reviewer would rather not widen the default, the alternative is to honour the self-author skip unless the login is explicitly listed in allowed_authors — but note that would leave the out-of-the-box config still broken, which is what BugFix: Reading PR Comments Broken #14 reports.
   • Scope. Items 1–3 fix and expose the reported bug. Items 4 (second discovery path) and 6 (--explain-pr) are robustness/diagnosability and can be cut without affecting the fix; item 5 (--dry-run) is a separate real bug that I'd keep, since verification step 3 depends on it.
   • API cost of item 4. One extra gh pr list per recently-active repo per poll (default 5m), bounded by pr_comments.search_limit repos and a 30-day window. Reusing ListPRs output avoids a ViewPR per PR, so for the fallback leg it is roughly cost-neutral against today's search-then-ViewPR shape. The 30-day/search_limit bounds are hardcoded deliberately rather than adding two more knobs — say so if you'd prefer them configurable.
   • Unverified assumption. I could not reproduce against live GitHub from this worktree. The self-author filter is the only unconditional, silent, always-on rejection on the path and it fires by construction in the documented single-account deployment, so it is almost certainly the cause — but items 3 and 6 exist precisely so that if something else is also wrong, the next run says what. I also confirmed that draft status is never consulted anywhere on this path (IsDraft is decoded but unused in discovery), so the "draft or not" detail in the report is consistent with this root cause rather than a second one.

   ---

   Reply with exactly implement to approve this plan and start the change. Reply with anything else and the plan will be revised to address it.

   _{coding-agent-loop run 0e132686-b428-468f-a4dd-94cc7f8ca795, model claude-opus-5, cost $2.3114}
3. ableinc

   added

   agent-plannedManaged by coding-agent-loopManaged by coding-agent-loop

   and removed

   agent-working

   on Aug 26, 2026