Skip to content

Inbox posting, day one: quiet once commented, top severity only, post from a moved-on checkout, ↑ on a just-posted row, description for the live agent #111

Description

@fiddur

Why

The first day of postAlerts on a real inbox showed six things, all seen on NCBackend3#13866, #14530 and #14605:

  1. The daemon posted an alert on a pull request the reviewer had already commented on — the reviewer's own routine had flagged #13866 a month earlier, and on #14530 the reviewer had posted a review from diffity at the very head the daemon then posted to.
  2. A pull request that keeps moving got a fresh automated review at every head: #14605 has three. Once per head is too often when the same person is on the thread already.
  3. The agent named P2 and P3 findings in ALERT-FINDINGS, so they went out too. Only the top severity should ever be posted automatically.
  4. A review prepared at head A cannot be posted once the author pushes B: create-review answers 409 "out of sync", and so does the comment pull. The findings are anchored to A and GitHub accepts a review on A.
  5. A row the reviewer has just posted from moves to Handled at once but has no ↑ until the next poll flips its status.
  6. The agent answering questions in an opened review never gets the description or the discussion file, which feat: a posted review can end with the reviewer's own footer, and the preparing agent reads the pull request's description and discussion #109 gave only to the preparing agent.

One pull request, six changes.

1. quietOnceCommented — the reviewer's own comment means they have seen it

Config quietOnceCommented: boolean (default false), in InboxSettings, SETTINGS_KEYS, settingsHost.get, the Settings panel (a checkbox under "Notify me if": "Once I have commented on a pull request, stop alerting me about it"), the README table.

When on: after the agent has prepared the review and the discussion is known, if any issue comment, review or inline review comment on the pull request is authored by the viewer (viewerLogin), the alert is dropped — alert: null, alertFindings: [] on the result, nothing posted, one log line <id>: alert dropped — you have commented on this pull request — and the review is still prepared, so it lands under Ready rather than Alerted and the tab stays quiet. The daemon's own posts are in the reviewer's name, so they count: a pull request is alerted at most once, which is what the reviewer's routine did by editing its one comment in place.

Plumbing: PrepareDeps.prContext returns { path, context: PrContext } | null instead of the path alone (the file is still written); PrepareOpts.viewerLogin: string | null from the tick (deps.forge.viewerLogin() is already read once per tick — pass it through prepareOne and prepareBumped); a pure commentedBy(context, login): boolean in prepare.ts or a small helper module, tested on its own.

2. No automated post where the reviewer has already reviewed that head

Independent of the setting: postAlertFindings skips the post when store.latestHandled(id) has headSha === head — passed in as PrepareOpts.reviewedHead: string | null by prepareOne — logging <id>: not posted — you reviewed this head yourself. The alert still shows.

3. Only the top severity goes out

Config postSeverities: string[] (default ["P1", "must-fix"]), editable in the Settings panel beside the prefix as a comma-separated field, README row. alertComments keeps only named findings whose first comment opens with one of those labels — reuse the label parsing summarizeFindings in summary.ts already does (P1: … or [must-fix]), do not write a second parser. Dropped findings are logged by count. If the agent named findings and none is left after the severity and status filters, nothing is posted (the existing rule); a named-none alert still posts body-only.

4. Posting and pulling from a checkout the pull request has moved past

packages/cli/src/server.ts, both /api/github/create-review and /api/github/pull-comments: when localHead !== details.headSha, ask the forge whether the local head is a commit of the pull request (gh api repos/{owner}/{repo}/pulls/{n}/commits --paginate, new prCommits(owner, repo, n) in packages/github/src/pr.ts, cached per request). If it is not, keep the 409 exactly as it is. If it is:

  • create-review posts with commit_id = localHead. createReview gains an options argument { commitSha, patch }: the commentable lines come from the diff of that commit against the base branch (gh api -H "Accept: application/vnd.github.diff" repos/{owner}/{repo}/compare/{baseRefName}...{sha}, new getCompareDiff), not from the pull request's current patch. ReviewResult gains commitSha: string so the UI can say "posted against the commit you reviewed; the pull request has moved on since" in its success message (packages/ui/src/lib/review-submission.ts / the submit dialog — one line, no redesign). GitHub marks any of those lines the author has since changed as outdated, which is the correct outcome.
  • pull-comments pulls, and for each incoming remote thread checks that the file exists in the checkout and has at least endLine lines before creating it; the ones that do not map are counted and reported in the response as unmapped instead of being created at a wrong line. Threads already known re-pull as before.

Tests: route tests with a fake gh for both paths — local head is an older PR commit (posts with that commit_id, uses the compare diff), local head is not a PR commit (409 unchanged), pull with one mappable and one unmappable thread.

5. ↑ on a row the reviewer has just posted from

toRow/buildView: a prepared or stale row whose latest handled mark is newer than preparedAt (the same test isHandled makes) is bumpable — prepareUrl set — and resolveBump accepts it (it takes the store, so it can read latestHandled). The bump then behaves as on a handled row: a fresh preparation at the current head. Tests in inbox-open.test.ts and the view tests.

6. The live agent gets the description and the discussion

composeLivePrompt gains body: string (cut at MAX_PROMPT_BODY, framed exactly as the prepare prompt frames it) and contextPath: string | null. The attendant reads them from <dataDirFor(worktree)>/pr-context.json when the file exists — the description is the body field there — and passes null/'' otherwise. Tests in inbox-attendant.test.ts.

Not in scope

  • Moving an opened checkout to the pull request's new head with the threads carried over. Separate design.
  • Editing an earlier automated review in place.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions