From ba3dda665c883b24cc99d54ac674bd4925e83372 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sat, 17 Oct 2026 03:19:17 +0000 Subject: [PATCH 01/10] feat(pr): observe bounded GitHub review threads Record head-bound review thread snapshots during explicit PR reconciliation. Keep prior thread observations visible when a page is incomplete, and document the saved observation contract. --- docs/features/pipeline.md | 3 +- ...02-feat-github-review-thread-inbox-plan.md | 217 ++++++++++++++++ go/internal/cli/pr_test.go | 13 + go/internal/helpview/catalog.json | 4 +- go/internal/prcmd/pr.go | 16 ++ go/internal/prcmd/readonly_test.go | 2 +- go/internal/prcmd/threads.go | 235 ++++++++++++++++++ go/internal/prcmd/threads_test.go | 185 ++++++++++++++ 8 files changed, 671 insertions(+), 4 deletions(-) create mode 100644 docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md create mode 100644 go/internal/prcmd/threads.go create mode 100644 go/internal/prcmd/threads_test.go diff --git a/docs/features/pipeline.md b/docs/features/pipeline.md index bd65fed..24c9b60 100644 --- a/docs/features/pipeline.md +++ b/docs/features/pipeline.md @@ -68,7 +68,8 @@ The table is the coordinator's record of what has run. It is not verification, r | `pipeline.fresh-base-uncertain` | A base the fresh fetch cannot reach refuses Push as uncertain, with or without `--allow-behind`, attempts no push, and re-derives the Rebase row as blocked instead of keeping the earlier pass | automated: `go/internal/cli/pipeline_freshbase_test.go` | offline suite | | `pipeline.pr-fresh-base` | `pipeline pr` fetches the base again before creating or adopting a PR and refuses to create one against a base that moved since the push, creating nothing, until `--allow-behind`; `pipeline run --allow-behind` passes the flag to both Push and PR (`go/internal/pipelinerun/pipelinerun.go`) | automated: `go/internal/cli/pipeline_freshbase_test.go` | offline suite | | `pipeline.pr-mergeability` | `pr reconcile` and `pipeline ci` record GitHub's `mergeable` and `mergeStateStatus`; an open PR reported `CONFLICTING`/`DIRTY` blocks the PR row with the merge-the-base remedy and shows under `maintenance.open_prs` with `blocked` and `fix`, and a later `MERGEABLE` observation passes the row again | automated: `go/internal/cli/pipeline_freshbase_test.go` | offline suite | -| `pipeline.pr-read-retry` | Exactly two read-only observations in `go/internal/prcmd`, `gh pr view` (`pr reconcile`, `sweep`) and `gh repo view --json visibility` (evidence publication), make at most one second attempt, and only after a conclusively completed nonzero exit whose stderr positively names a transient condition: an `HTTP 5xx` status line, `HTTP 429`, gh's secondary-rate-limit sentence, or a transport failure as gh prints it (`error connecting to`, `connection refused`, tls or x509 handshake errors). Both attempts and the delay between them share the one existing `GHBound`: attempt one runs on the runner's own timer unchanged, attempt two receives only the time left, and a delay that would not leave a floor for a second attempt defers instead of overrunning. A runner error (timeout, cancel, oversized or incomplete output, uncertain descendant), an auth failure, 404, any other 4xx including a bare 403, a `GraphQL:` application error, malformed JSON, or ambiguous stderr is never retried; no write (agent launch, PR create, edit, ready, publish, push, merge) and no other `gh` read gains a retry, and `go/internal/proc` keeps its no-retry contract. A failure after the retry is still an uncertain observation naming the attempt count and category with token shapes redacted and the detail capped; the prior `pr` record and `observed_at` stay as recorded and `sweep` stamps `observe_failed_at` as before | automated: `go/internal/prcmd/readonly_test.go`, `go/internal/prcmd/pr_test.go` | offline suite against counting fake `gh` scripts: transient-then-valid succeeds after exactly two calls, persistent 503 fails after exactly two, each non-transient shape makes one call, a 429 with a short bound defers after one call, a slow first attempt leaves no budget, the runner called directly runs once, and a Go source-parsing pass over the package pins the helper's callers to `viewPR` and `observedVisibility`. Implementation: `go/internal/prcmd/readonly.go`. | +| `pipeline.pr-read-retry` | Read-only `gh` observations in `go/internal/prcmd` make at most one second attempt, and only after a conclusively completed nonzero exit whose stderr positively names a transient condition: an `HTTP 5xx` status line, `HTTP 429`, gh's secondary-rate-limit sentence, or a transport failure as gh prints it (`error connecting to`, `connection refused`, tls or x509 handshake errors). The `gh pr view` and `gh api graphql` PR reads (`pr reconcile`, `sweep`) share one bound per observation; `gh repo view --json visibility` (evidence publication) retains its existing bound. Attempt one runs on the runner's own timer unchanged, attempt two receives only the time left, and a delay that would not leave a floor for a second attempt defers instead of overrunning. A runner error (timeout, cancel, oversized or incomplete output, uncertain descendant), an auth failure, 404, any other 4xx including a bare 403, a `GraphQL:` application error, malformed JSON, or ambiguous stderr is never retried; no write (agent launch, PR create, edit, ready, publish, push, merge) and no other `gh` read gains a retry, and `go/internal/proc` keeps its no-retry contract. A failure after the retry is still an uncertain observation naming the attempt count and category with token shapes redacted and the detail capped; the prior `pr` record and `observed_at` stay as recorded and `sweep` stamps `observe_failed_at` as before | automated: `go/internal/prcmd/readonly_test.go`, `go/internal/prcmd/pr_test.go` | offline suite against counting fake `gh` scripts: transient-then-valid succeeds after exactly two calls, persistent 503 fails after exactly two, each non-transient shape makes one call, a 429 with a short bound defers after one call, a slow first attempt leaves no budget, the runner called directly runs once, and a Go source-parsing pass over the package pins the helper's callers to `observeReviewThreads`, `viewPR`, and `observedVisibility`. Implementation: `go/internal/prcmd/readonly.go`, `go/internal/prcmd/threads.go`. | +| `pipeline.review-threads` | An explicit `pr reconcile` records cursor-paginated GitHub review threads and the overall review decision only when the returned PR number, URL, and head match the identity observation; each thread retains its stable node ID, source URL, file and supplied lines, latest comment identity, author, update time, resolution/outdated state, and observed head/time. The read has a 20-page maximum of 50 threads per page and shares one `GHBound`; a failed, incomplete, nonadvancing, or mismatched response is marked uncertain and keeps previously observed threads with their original per-thread observation time/head. Reconciliation is the only GitHub read for these rows; later saved-state projections do not poll. | automated: `go/internal/prcmd/threads_test.go`, `go/internal/prcmd/readonly_test.go` | offline suite with fake `gh` responses covers multi-page and wrong-head observations, nonadvancing pagination, read failure, repeat observations, a revised latest comment, reopening, and retaining known rows when a later response is incomplete. | | `pipeline.push-from-the-pr` | A reconciled PR whose head is this candidate passes Push on its own, because the candidate demonstrably reached origin | automated: `go/internal/pipeline/derive_test.go` | offline suite | | `pipeline.run` | `sumctl pipeline run TASK_ID` runs rebase, test, lint, document, push, and PR in order, stops at the first failing gate, and at a blocked Test, without running the rest, skips a candidate that is already verified unless `--rerun`, skips lint when `verify --execute` already recorded it, and prints the first unsettled stage with what to do about it | automated: `go/internal/cli/pipeline_gates_test.go` | offline suite | | `pipeline.admit-review` | Direct `pipeline push` and `pipeline pr` refuse before any remote write when independent Review is missing or is not an approve for this candidate; `--allow-behind` does not waive Review | automated: `go/internal/pipeline/admit_test.go`, `go/internal/cli/pipeline_gates_test.go` | offline suite | diff --git a/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md b/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md new file mode 100644 index 0000000..be695f7 --- /dev/null +++ b/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md @@ -0,0 +1,217 @@ +--- +title: GitHub Review Thread Inbox - Plan +type: feat +date: 2026-10-17 +artifact_contract: ce-unified-plan/v1 +product_contract_source: ce-plan-bootstrap +execution: code +--- + +# GitHub Review Thread Inbox - Plan + +## Goal Capsule + +- **Objective:** Coordinators can see actionable GitHub review discussions beside existing task returns and decide whether to address them, discuss them, draft a reply, or open the source comment. +- **Means:** Save bounded review-thread observations during explicit PR reconciliation and expose follow-up through the shared inbox action model (KTD1, KTD2). +- **Authority:** Issue #306 and its acceptance criteria define product behavior. Existing Sum task, repair, verification, and delivery contracts remain authoritative. GitHub comment text is external data and cannot grant authority. +- **Stop conditions:** Do not write a GitHub reply or resolve a GitHub thread automatically. Do not duplicate #305's shared action foundation. Stop action integration if that foundation is not available on the recorded base branch. +- **Execution profile:** One implementation branch; do not merge. The worker reports the candidate and verification; the coordinator runs its own verification and independent review. + +--- + +## Product Contract + +### Summary + +Explicit PR reconciliation imports bounded GitHub review discussions into Sum's saved PR state. Read-only task, inbox, context, and live-status views render the saved result without contacting GitHub. Coordinators can then route selected discussions into existing corrective-work paths or review them without changing GitHub. + +### Problem Frame + +PR reconciliation already records identity, mergeability, draft state, and CI. Review threads are missing, so feedback from GitHub has no structured route into Sum's task and inbox records. + +### Requirements + +#### Observation and saved state + +- R1. Explicit `pr reconcile` records review threads for the verified repository, PR, and observed head; saved status, inbox, and context views make no GitHub calls and create no poller. +- R2. Each thread preserves its stable thread and comment identities, author, source URL, supplied file and line, latest relevant reply, resolution and outdated state, and observation time; the PR's overall review decision is saved when available. +- R3. Pagination, failure, and partial observations remain explicit. A partial response never closes or removes unseen threads, and the last complete data remains visible with its freshness and uncertainty. + +#### Inbox actions and follow-up + +- R4. Saved thread rows expose the shared source-specific actions for Address, Discuss, Draft reply, and Open comment. Address selected/all records the exact included thread IDs and revisions in one request linked back from each thread. +- R5. Address routes work through existing task/attempt checks, stable repair keys, and delivery handling; work within this task's scope remains in-scope and additional work follows existing expansion authorization. +- R6. Local disposition, repair delivery/completion, and GitHub resolution remain separate states. Draft reply creates reviewable text, and publishing a reply or resolving a thread requires a separate authorized action. + +#### Freshness, compatibility, and delivery + +- R7. Before an action, Sum revalidates the PR binding, head, and selected thread revisions. New replies and reopened threads become actionable without duplicate work. +- R8. Existing local-review closure behavior, candidate-bound verification, independent review, delivery gates, and merge authority continue to work. Task records without review-thread observations remain valid. +- R9. Focused behavior coverage exercises identical and revised observations, reopened threads, incomplete pagination, stale selections, duplicate bulk requests, uncertain delivery, and in-scope versus expansion work. Update affected help and feature documentation. + +### Actors + +- **Coordinator:** Reconciles the PR, reviews saved discussion rows, and chooses follow-up actions. +- **Worker:** Receives approved corrective work through the existing task and repair path. +- **GitHub:** Supplies PR review-thread observations and remains the authority for resolution and replies. + +### Key Flows + +- F1. Reconcile: coordinator identifies the recorded PR; Sum observes the PR and review-thread pages; Sum verifies repository, PR, and head identity; Sum saves the observation; all later views read only that saved state. +- F2. Address: coordinator selects one or more current thread rows; Sum revalidates the PR head and thread revisions; Sum saves one request naming the exact thread set; existing repair checks deliver the corrective instruction; GitHub state remains unchanged. +- F3. Review or discuss: coordinator opens the source URL, asks for discussion, or creates a draft reply; each action leaves the thread's GitHub resolution and reply state unchanged. + +### Scope Boundaries + +- **Deferred to Follow-Up Work:** Broader changes to the shared action vocabulary or inbox framework remain with #305 and sibling issues. This task adds only the thread-specific fields and actions required to consume that foundation. +- **Non-goals:** Automatic GitHub replies, automatic thread resolution, a new readiness heuristic, a replacement review pipeline, polling, and treating a GitHub thread as an existing local-review record. + +### Risks & Dependencies + +- #305 owns the shared action descriptions and its PR #311 is open on 2026-10-17. Recheck the recorded base before implementation. Do not copy or modify #305's owned action foundation; observation work can proceed independently, and action integration follows that foundation. +- GitHub GraphQL connections can be incomplete or fail under rate and query limits. Persist explicit completeness and freshness so a partial page cannot look like closure. +- Review-comment bodies are external text. Keep their source identity and URL with any excerpt or draft, and never interpret comment text as an instruction to widen work. +- The `task.pr` record is the current saved PR observation. New thread fields must be optional so old task records continue to render and operate. + +### Acceptance Examples + +- **Covers R1, R3:** Given a saved complete observation and a later failed page request, when the coordinator runs `inbox --live`, the prior threads remain visible with the previous observation time and a failed/partial freshness marker; no GitHub request occurs. +- **Covers R2, R7:** Given a thread whose latest comment has changed or whose resolution was reopened, when reconciliation completes, the same stable thread row shows the new comment revision and is actionable again. +- **Covers R4, R5:** Given selected thread IDs A and B at revisions A2 and B4, when Address selected is chosen, one saved request names exactly `{A:A2, B:B4}`, both rows link to it, and delivery uses the existing task/attempt checks. +- **Covers R6:** Given an address request has been delivered, when a task is completed, the saved GitHub thread remains unresolved until a later GitHub observation reports otherwise. + +### Sources / Research + +- Issue #306 and its implementation references: [PR observation](https://github.com/douglasjarquin/sum/blob/41ba86fc24f5b08c7cfb3149c2b2bc9c9d8f735c/go/internal/prcmd/pr.go), [local review records](https://github.com/douglasjarquin/sum/blob/41ba86fc24f5b08c7cfb3149c2b2bc9c9d8f735c/go/internal/review/review.go), and [repair scope and idempotency](docs/repairs.md). +- Inbox's [thread actions](https://github.com/petekp/inbox/blob/9bc2983367827bb6b49a7d14cfdff1f48a7f0626/hooks/pr-tab.ts#L314-L435) and [source-preserving PR requests](https://github.com/petekp/inbox/blob/9bc2983367827bb6b49a7d14cfdff1f48a7f0626/hooks/prs.ts#L366-L423) supply interaction precedents, not Sum authority. +- GitHub's [PullRequest GraphQL fields](https://docs.github.com/en/graphql/reference/pulls) expose `reviewThreads` and `reviewDecision`; its [pagination guide](https://docs.github.com/en/graphql/guides/using-pagination-in-the-graphql-api) requires cursor traversal and page-info checks for complete connection reads. +- `docs/solutions/architecture-patterns/retiring-persisted-compatibility-fields.md` recommends deriving compatibility from the actual readers and keeping persisted additions optional for older records. + +--- + +## Planning Contract + +### Key Technical Decisions + +- KTD1. Use a bounded, read-only GitHub GraphQL observation for `reviewThreads` and `reviewDecision` alongside the existing `gh pr view` result. `prViewFields` does not contain thread fields, while GitHub exposes them as cursor-paginated connections. Preserve the existing `pr reconcile` identity check and read-only timeout/retry boundary (`go/internal/prcmd/pr.go`, `go/internal/prcmd/readonly.go`). +- KTD2. Store the latest thread snapshot and its completeness/freshness beside the task's existing PR identity and head. Keep prior known threads when a page is incomplete or fails; optional thread fields keep older task records valid. Store address requests as separate durable records linked by stable request ID so later thread revisions remain distinct. +- KTD3. Reuse the shared action descriptions from #305 and route Address through existing repair delivery. Actions bind to the latest comment revision and verified PR head; thread observations and actions do not mutate GitHub. This keeps task execution, review evidence, and GitHub's resolution state independent. + +### High-Level Technical Design + +```mermaid +sequenceDiagram + participant C as Coordinator + participant P as PR reconcile + participant G as GitHub + participant S as Saved task PR state + participant I as Inbox view + participant R as Repair path + C->>P: reconcile recorded PR + P->>G: read PR identity and head + P->>G: read reviewThreads pages and reviewDecision + G-->>P: thread/comment nodes and pageInfo + P->>P: verify repository, PR, head, and page completeness + P->>S: save current snapshot and observation status + I->>S: render saved thread rows and actions + C->>I: choose Address for current thread revisions + I->>P: submit scoped action request + P->>G: revalidate PR head and selected revisions + P->>S: save one request with exact IDs and revisions + P->>R: use existing task/attempt/repair checks + Note over G,S: Replies and resolution change only after a separate authorized GitHub action. +``` + +### Assumptions and Deferred Implementation Notes + +- The task branch remains targeted to `main`; the implementer rechecks `origin/main` before changing files and does not stack #305's unmerged implementation into this branch. +- Exact GraphQL page size, maximum page count, and excerpt size should match the existing bounded-process/output limits discovered in the implementation. Reaching a limit records an incomplete observation rather than implying closure. +- The exact shared-action API is resolved from #305's merged code at implementation time; this plan does not restate or fork that API. + +--- + +## Implementation Units + +### U1. Observe and persist bounded review threads + +- **Goal:** Extend explicit PR reconciliation with an identity-bound review-thread and overall review-decision observation. +- **Requirements:** R1, R2, R3, R8. +- **Dependencies:** None. +- **Files:** `go/internal/prcmd/pr.go`, `go/internal/prcmd/threads.go`, `go/internal/prcmd/threads_test.go`, `go/internal/prcmd/readonly_test.go`. +- **Approach:** Keep `gh pr view` as the identity and PR-head source. Read review threads with bounded GraphQL cursor pagination and latest-comment data. Save only observations bound to the same repository, PR number, and head. On failure or page limits, preserve known threads and record incomplete/failed status and freshness. +- **Patterns to follow:** `viewPR`, `recordObservationWith`, `readOnlyGH`, `GHBound`, and the fake-`gh` counting scripts in the PR command tests. +- **Test scenarios:** + - A complete single-page response saves stable thread/comment IDs, author, URL, file/line, latest reply, state, review decision, head, and observation time. + - A multi-page response follows each `endCursor` until `hasNextPage` is false and stores each thread once. + - Identical repeated observations create no duplicate thread or address request. + - A revised latest comment updates the existing thread identity and makes the new revision current. + - A reopened thread is saved as unresolved and actionable. + - A failed page or reached bound marks the observation incomplete and retains previously known unseen threads. + - A mismatched repository, PR number, or head is rejected before saving the thread snapshot. +- **Verification:** Focused PR adapter and CLI coverage demonstrates complete, partial, failed, repeated, revised, and wrong-identity observations without adding a background poller. + +### U2. Render saved threads through the shared inbox actions + +- **Goal:** Show saved review threads and their current source-specific actions in task context and existing inbox surfaces. +- **Requirements:** R1, R2, R3, R4, R8. +- **Dependencies:** U1 and #305's shared action foundation. +- **Files:** `go/internal/presentation/presentation.go`, `go/internal/inboxview/overview.go`, `go/internal/inboxview/terminal.go`, `go/internal/inboxview/overview_test.go`, `go/internal/inboxview/terminal_test.go`, `go/internal/cli/context_test.go`, `go/internal/cli/presentation_test.go`. +- **Approach:** Extend the shared presentation/action layer with the smallest thread-specific projection. Render latest observation time and uncertainty; keep rows read-only and preserve stable source identity. Leave #305-owned question/action behavior intact. +- **Patterns to follow:** Existing PR, return, attention, and decision presentation rows; use the shared action structure introduced by #305 rather than defining a parallel one. +- **Test scenarios:** + - A saved complete thread renders its author, summary, source URL, path/line, and current status in context and inbox output. + - An incomplete snapshot renders its uncertainty and retains previously observed threads. + - A task record without thread fields renders exactly as a legacy task. + - `inbox --live` renders saved thread state without invoking GitHub. +- **Verification:** Context and inbox outputs agree on thread identity, freshness, and available actions; existing decision presentation remains unchanged. + +### U3. Revalidate and route scoped follow-up actions + +- **Goal:** Record Address, Discuss, Draft reply, and Open comment actions while keeping repair execution and GitHub changes separate. +- **Requirements:** R4, R5, R6, R7, R8. +- **Dependencies:** U2. +- **Files:** `go/internal/prcmd/pr.go`, `go/internal/repair/repair.go`, `go/internal/cli/pr.go`, `go/internal/cli/repair_test.go`, `go/internal/cli/pr_test.go`, `go/internal/presentation/presentation_test.go`. +- **Approach:** Bind each action request to the verified PR/head and the selected thread IDs plus latest comment revisions. Revalidate before saving or sending. Address selected/all creates one stable request and follows existing task/attempt/repair classification and delivery; other actions open the source, record discussion, or create draft text. Do not touch `go/internal/review`, whose shared lane belongs to #307. +- **Patterns to follow:** Existing repair idempotency, task/attempt validation, saved return delivery, and the #305 action request/response path. +- **Test scenarios:** + - Address selected records only the requested thread IDs and their exact current revisions in one request linked from each row. + - Address all includes exactly the currently actionable thread set and duplicate submission returns the existing request. + - A changed PR binding/head or latest-comment revision refuses a stale action before delivery. + - An uncertain repair delivery remains uncertain and is not resent under a new key. + - In-scope corrections use the existing in-scope route; added work requires the existing expansion authorization. + - Draft reply yields reviewable text, while Address completion leaves GitHub reply and resolution fields unchanged. +- **Verification:** Saved action requests can be traced to a single repair attempt and cannot fabricate external closure or bypass current delivery gates. + +### U4. Document the operator path and prove the changed workflow + +- **Goal:** Explain how coordinators inspect, address, discuss, draft, and open GitHub review threads, including incomplete observations and the distinction between local and GitHub state. +- **Requirements:** R3, R4, R5, R6, R8, R9. +- **Dependencies:** U1, U2, U3. +- **Files:** `go/internal/cli/root.go`, `go/internal/cli/testdata/`, `docs/features/pipeline.md`, `docs/features/coordination.md`, `docs/repairs.md`. +- **Approach:** Update existing help and feature rows; keep the canonical behavior in the existing CLI, repair, and inbox documentation. Capture before/after CLI transcript evidence for the reconcile-to-inbox path. +- **Patterns to follow:** Existing feature-map scenario rows and help snapshots. Regenerate snapshots only with the repository's established command when help text changes. +- **Test scenarios:** + - Help describes explicit observation and the fact that views do not poll GitHub. + - Feature documentation distinguishes local follow-up from GitHub reply and resolution. + - Before/after CLI transcripts show the new saved thread row and action without claiming visual proof. +- **Verification:** Updated documentation matches observable command output; every new scenario is mapped to focused coverage and the evidence comparison is listed in the handoff. + +--- + +## Verification Contract + +- `mise run verify` runs the repository's declared verification task. +- `python3 .agents/skills/verify/scripts/verify_run.py --base 41ba86fc24f5b08c7cfb3149c2b2bc9c9d8f735c --json` records candidate-bound verification for the task. +- Existing Go behavior coverage runs through `go test ./...` as part of `mise run verify`; focused tests cover identity, pagination, idempotency, stale actions, uncertain delivery, and scope classification. +- Factory evidence uses before/after CLI transcripts for the changed PR-reconcile and inbox path; visual proof is not applicable to a CLI/API change. List every `comparison.json` in the handoff. +- The coordinator runs the same verification contract on its own run ID and performs independent review. The worker does not merge. + +--- + +## Definition of Done + +- Every R1-R9 behavior is implemented within the issue's lane boundaries, with focused coverage and updated help/feature documentation. +- Old records remain readable, incomplete observations retain prior information, and repeated observations or action submissions create no duplicate work. +- Local dispositions and repair outcomes never masquerade as GitHub replies or resolution. +- The candidate is committed on the task branch, the worker verification record and evidence comparisons are attached to the handoff, and an unmerged PR is available for coordinator verification and review. +- Abandoned implementation attempts are removed from the final diff. diff --git a/go/internal/cli/pr_test.go b/go/internal/cli/pr_test.go index 13dd636..de3fdd2 100644 --- a/go/internal/cli/pr_test.go +++ b/go/internal/cli/pr_test.go @@ -47,6 +47,19 @@ func TestPRUnknownFlagsAreUsageErrorsBeforePR(t *testing.T) { } } +func TestPRReconcileHelpDescribesSavedReviewThreadObservation(t *testing.T) { + home := writeDesignatedHome(t) + stdout, stderr, err := runPRCLI(t, home, "help", "pr-reconcile") + if err != nil { + t.Fatalf("help: %v stderr=%s", err, stderr) + } + for _, text := range []string{"review-thread observations", "keep previous threads", "do not poll GitHub"} { + if !strings.Contains(stdout, text) { + t.Fatalf("help is missing %q:\n%s", text, stdout) + } + } +} + func TestPRCommands(t *testing.T) { cases := []struct { name string diff --git a/go/internal/helpview/catalog.json b/go/internal/helpview/catalog.json index 9dd8484..2df6df6 100644 --- a/go/internal/helpview/catalog.json +++ b/go/internal/helpview/catalog.json @@ -998,7 +998,7 @@ "help": "Exact PR identity from an authenticated GitHub observation; never parsed from prose or guessed from branch names. The read is retried once, within the same bound, only for a positively transient gh failure (HTTP 5xx, 429, secondary rate limit, connection error); auth, 404, GraphQL, malformed output, and timeouts are never retried, and no write ever is", "arguments": [], "subcommands": { - "reconcile": "Coordinator only: inspect PR --number in the task repository with gh (one bounded retry of that read on a transient failure), record its exact identity, state, and mismatches, then publish the worker's before/after evidence", + "reconcile": "Coordinator only: inspect PR --number in the task repository with gh (one bounded retry of that read on a transient failure), record its exact identity, state, mismatches, and head-bound review threads, then publish the worker's before/after evidence", "evidence": "Coordinator only: publish each evidence run's before/after media into the reconciled PR's marked block; receipts and publish copies stay under the task record" }, "read_only": false, @@ -1730,7 +1730,7 @@ }, "pr-reconcile": { "topic": "pr-reconcile", - "help": "Coordinator only: inspect PR --number in the task repository with gh and record its exact identity, state, and mismatches", + "help": "Coordinator only: inspect PR --number in the task repository and record its exact identity, state, mismatches, and bounded review-thread observations against the observed head. Incomplete reads keep previous threads with their earlier observation times. Status, inbox, and context use saved state and do not poll GitHub.", "arguments": [ { "name": "task", diff --git a/go/internal/prcmd/pr.go b/go/internal/prcmd/pr.go index 04d7fa7..653669c 100644 --- a/go/internal/prcmd/pr.go +++ b/go/internal/prcmd/pr.go @@ -50,6 +50,7 @@ func Reconcile(s *store.Store, ctx *ordjson.Object, runtimeRoot string, args Rec if err != nil { return nil, err } + data["review_threads"] = observeReviewThreads(gh, repo, remote, args.Number, data) pr, recordID, err := recordObservationWith(s, ctx, args.Task, data, args.AcceptBaseBranch) if err != nil { return nil, err @@ -130,6 +131,21 @@ func recordObservationWith(s *store.Store, ctx *ordjson.Object, taskID string, d observedAt := store.Now() pr.Set("observed_at", observedAt) pipeline.SetMergeability(pr, data["mergeable"], data["mergeStateStatus"], observedAt) + threads := asObject(data["review_threads"]) + if threads != nil { + previousPRValue, _ := task.Get("pr") + if previous := asObject(previousPRValue); previous != nil { + previousThreadsValue, _ := previous.Get("review_threads") + previousThreads := asObject(previousThreadsValue) + retainObservedThreads(threads, previousThreads) + if _, ok := threads.Get("review_decision"); !ok && previousThreads != nil { + if decision, exists := previousThreads.Get("review_decision"); exists { + threads.Set("review_decision", decision) + } + } + } + pr.Set("review_threads", threads) + } by := ordjson.NewObject() for _, k := range []string{"machine", "session", "pane"} { v, _ := ctx.Get(k) diff --git a/go/internal/prcmd/readonly_test.go b/go/internal/prcmd/readonly_test.go index 3d2f4bf..fe563af 100644 --- a/go/internal/prcmd/readonly_test.go +++ b/go/internal/prcmd/readonly_test.go @@ -308,7 +308,7 @@ func TestRetryBoundaryIsExactlyTheTwoReadOnlyObservations(t *testing.T) { got = append(got, name) } sort.Strings(got) - want := []string{"observedVisibility", "viewPR"} + want := []string{"observeReviewThreads", "observedVisibility", "viewPR"} if strings.Join(got, ",") != strings.Join(want, ",") { t.Fatalf("readOnlyGH callers = %v, want %v", got, want) } diff --git a/go/internal/prcmd/threads.go b/go/internal/prcmd/threads.go new file mode 100644 index 0000000..05c39b7 --- /dev/null +++ b/go/internal/prcmd/threads.go @@ -0,0 +1,235 @@ +package prcmd + +import ( + "encoding/json" + "fmt" + "sort" + "strings" + "time" + + "github.com/douglasjarquin/sum/go/internal/ordjson" + "github.com/douglasjarquin/sum/go/internal/store" +) + +const reviewThreadPageSize = 50 +const reviewThreadPageLimit = 20 + +const reviewThreadsQuery = `query($owner:String!,$name:String!,$number:Int!,$cursor:String){repository(owner:$owner,name:$name){pullRequest(number:$number){id number url headRefOid reviewDecision reviewThreads(first:%d,after:$cursor){nodes{id isOutdated isResolved path line originalLine startLine originalStartLine comments(last:100){nodes{id databaseId url body createdAt updatedAt author{login} replyTo{id}}}} pageInfo{hasNextPage endCursor}}}}}` + +func observeReviewThreads(gh, dir, remote string, number int, pr map[string]any) *ordjson.Object { + record := ordjson.NewObject() + observedAt := store.Now() + record.Set("observed_at", observedAt) + record.Set("observed_head_sha", fmt.Sprint(pr["headRefOid"])) + record.Set("status", "unavailable") + record.Set("threads", []any{}) + owner, name, ok := strings.Cut(remote, "/") + if !ok || owner == "" || name == "" { + record.Set("detail", "repository identity is unavailable") + return record + } + record.Set("repository", remote) + record.Set("pr_number", number) + var rows []any + cursor := "" + decision := any(nil) + deadline := time.Now().Add(GHBound) + for page := 0; page < reviewThreadPageLimit; page++ { + remaining := time.Until(deadline) + if remaining <= 0 { + record.Set("status", "incomplete") + record.Set("detail", "review thread observation time bound reached") + record.Set("threads", rows) + return record + } + argv := []string{gh, "api", "graphql", "-f", "query=" + fmt.Sprintf(reviewThreadsQuery, reviewThreadPageSize), "-F", "owner=" + owner, "-F", "name=" + name, "-F", fmt.Sprintf("number=%d", number)} + if cursor != "" { + argv = append(argv, "-F", "cursor="+cursor) + } + out, err := readOnlyGH(argv, dir, remaining) + if err != nil || out.Result.Code != 0 { + record.Set("status", "incomplete") + detail := sanitizedDetail(out.Result) + if err != nil { + detail = err.Error() + } + if detail == "" { + detail = "GitHub returned an unsuccessful response" + } + record.Set("detail", boundedThreadDetail(fmt.Sprintf("review thread page %d unavailable: %s", page+1, detail))) + record.Set("threads", rows) + return record + } + var payload struct { + Data struct { + Repository *struct { + PullRequest *struct { + ID string `json:"id"` + Number int `json:"number"` + URL string `json:"url"` + HeadRefOID string `json:"headRefOid"` + ReviewDecision string `json:"reviewDecision"` + ReviewThreads struct { + Nodes []reviewThreadNode `json:"nodes"` + PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + EndCursor string `json:"endCursor"` + } `json:"pageInfo"` + } `json:"reviewThreads"` + } `json:"pullRequest"` + } `json:"repository"` + } `json:"data"` + Errors []struct { + Message string `json:"message"` + } `json:"errors"` + } + if err := json.Unmarshal([]byte(out.Result.Stdout), &payload); err != nil { + record.Set("status", "incomplete") + record.Set("detail", "review thread response was not valid JSON") + record.Set("threads", rows) + return record + } + if len(payload.Errors) > 0 || payload.Data.Repository == nil || payload.Data.Repository.PullRequest == nil { + detail := "review thread response was incomplete" + if len(payload.Errors) > 0 { + detail = secretShapes.ReplaceAllString(payload.Errors[0].Message, "[redacted]") + } + record.Set("status", "incomplete") + record.Set("detail", boundedThreadDetail(detail)) + record.Set("threads", rows) + return record + } + got := payload.Data.Repository.PullRequest + if got.Number != number || got.URL != fmt.Sprint(pr["url"]) || got.HeadRefOID != fmt.Sprint(pr["headRefOid"]) { + record.Set("status", "mismatched") + record.Set("detail", "review threads did not match the observed repository, PR, and head") + record.Set("threads", []any{}) + return record + } + decision = got.ReviewDecision + for _, node := range got.ReviewThreads.Nodes { + thread := node.record() + thread.Set("observed_at", observedAt) + thread.Set("observed_head_sha", fmt.Sprint(pr["headRefOid"])) + rows = append(rows, thread) + } + if !got.ReviewThreads.PageInfo.HasNextPage { + record.Set("status", "complete") + record.Set("threads", rows) + record.Set("review_decision", decision) + return record + } + next := got.ReviewThreads.PageInfo.EndCursor + if next == "" || next == cursor { + record.Set("status", "incomplete") + record.Set("detail", "review thread pagination returned no advancing cursor") + record.Set("threads", rows) + return record + } + cursor = next + } + record.Set("status", "incomplete") + record.Set("detail", "review thread page limit reached") + record.Set("threads", rows) + return record +} + +type reviewThreadNode struct { + ID string `json:"id"` + IsOutdated bool `json:"isOutdated"` + IsResolved bool `json:"isResolved"` + Path string `json:"path"` + Line *int `json:"line"` + OriginalLine *int `json:"originalLine"` + StartLine *int `json:"startLine"` + OriginalStartLine *int `json:"originalStartLine"` + Comments struct { + Nodes []struct { + ID string `json:"id"` + Database int64 `json:"databaseId"` + URL string `json:"url"` + Body string `json:"body"` + CreatedAt string `json:"createdAt"` + UpdatedAt string `json:"updatedAt"` + Author *struct { + Login string `json:"login"` + } `json:"author"` + ReplyTo *struct { + ID string `json:"id"` + } `json:"replyTo"` + } `json:"nodes"` + } `json:"comments"` +} + +func (n reviewThreadNode) record() *ordjson.Object { + thread := ordjson.NewObject() + thread.Set("id", n.ID) + thread.Set("path", n.Path) + thread.Set("line", n.Line) + thread.Set("original_line", n.OriginalLine) + thread.Set("start_line", n.StartLine) + thread.Set("original_start_line", n.OriginalStartLine) + thread.Set("resolved", n.IsResolved) + thread.Set("outdated", n.IsOutdated) + sort.Slice(n.Comments.Nodes, func(i, j int) bool { return n.Comments.Nodes[i].UpdatedAt > n.Comments.Nodes[j].UpdatedAt }) + commentRows := make([]any, 0, len(n.Comments.Nodes)) + for _, c := range n.Comments.Nodes { + row := ordjson.NewObject() + row.Set("id", c.ID) + row.Set("database_id", c.Database) + row.Set("url", c.URL) + row.Set("author", nil) + if c.Author != nil { + row.Set("author", c.Author.Login) + } + row.Set("created_at", c.CreatedAt) + row.Set("updated_at", c.UpdatedAt) + row.Set("reply_to", nil) + if c.ReplyTo != nil { + row.Set("reply_to", c.ReplyTo.ID) + } + body := c.Body + if len(body) > 4096 { + body = body[:4096] + } + row.Set("body", body) + commentRows = append(commentRows, row) + } + thread.Set("comments", commentRows) + if len(n.Comments.Nodes) > 0 { + latest := n.Comments.Nodes[0] + thread.Set("latest_comment_id", latest.ID) + thread.Set("latest_updated_at", latest.UpdatedAt) + thread.Set("source_url", latest.URL) + if latest.Author != nil { + thread.Set("latest_author", latest.Author.Login) + } + } + return thread +} + +func boundedThreadDetail(detail string) string { + detail = secretShapes.ReplaceAllString(detail, "[redacted]") + if len(detail) > detailBound { + return detail[:detailBound] + } + return detail +} + +func retainObservedThreads(current, previous *ordjson.Object) { + if current == nil || previous == nil || asString(current, "status") == "complete" { + return + } + seen := map[string]bool{} + rows := asList(func() any { v, _ := current.Get("threads"); return v }()) + for _, raw := range rows { + seen[asString(asObject(raw), "id")] = true + } + for _, raw := range asList(func() any { v, _ := previous.Get("threads"); return v }()) { + thread := asObject(raw) + if thread != nil && !seen[asString(thread, "id")] { + rows = append(rows, thread) + } + } + current.Set("threads", rows) +} diff --git a/go/internal/prcmd/threads_test.go b/go/internal/prcmd/threads_test.go new file mode 100644 index 0000000..d4da242 --- /dev/null +++ b/go/internal/prcmd/threads_test.go @@ -0,0 +1,185 @@ +package prcmd + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/douglasjarquin/sum/go/internal/ordjson" + "github.com/douglasjarquin/sum/go/internal/store" +) + +func quoteShell(value string) string { + return "'" + strings.ReplaceAll(value, "'", "'\\''") + "'" +} + +func TestObserveReviewThreadsPreservesIdentityAndLatestComment(t *testing.T) { + payload := `{"data":{"repository":{"pullRequest":{"id":"PR_kwDO","number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"HEAD","reviewDecision":"CHANGES_REQUESTED","reviewThreads":{"nodes":[{"id":"PRRT_1","isOutdated":false,"isResolved":false,"path":"go/main.go","line":12,"comments":{"nodes":[{"id":"PRRC_1","databaseId":31,"url":"https://github.com/o/r/pull/7#discussion_r31","body":"first","createdAt":"2026-10-16T00:00:00Z","updatedAt":"2026-10-16T00:00:00Z","author":{"login":"reviewer"},"replyTo":null},{"id":"PRRC_2","databaseId":32,"url":"https://github.com/o/r/pull/7#discussion_r32","body":"follow-up","createdAt":"2026-10-16T01:00:00Z","updatedAt":"2026-10-16T02:00:00Z","author":{"login":"author"},"replyTo":{"id":"PRRC_1"}}]}}],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}}` + gh := fakeGh(t, `printf '%s' `+quoteShell(payload)) + pr := map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"} + got := observeReviewThreads(gh, t.TempDir(), "o/r", 7, pr) + if asString(got, "status") != "complete" || asString(got, "review_decision") != "CHANGES_REQUESTED" { + t.Fatalf("observation = %v", got) + } + threads := asList(func() any { v, _ := got.Get("threads"); return v }()) + if len(threads) != 1 { + t.Fatalf("threads = %v", threads) + } + thread := asObject(threads[0]) + if asString(thread, "id") != "PRRT_1" || asString(thread, "source_url") != "https://github.com/o/r/pull/7#discussion_r32" || asString(thread, "latest_comment_id") != "PRRC_2" { + t.Fatalf("thread identity/latest = %v", thread) + } + comments := asList(func() any { v, _ := thread.Get("comments"); return v }()) + if len(comments) != 2 || asString(asObject(comments[1]), "author") != "reviewer" { + t.Fatalf("comments = %v", comments) + } +} + +func TestObserveReviewThreadsMarksNonAdvancingPaginationIncomplete(t *testing.T) { + payload := `{"data":{"repository":{"pullRequest":{"number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"HEAD","reviewDecision":"APPROVED","reviewThreads":{"nodes":[],"pageInfo":{"hasNextPage":true,"endCursor":"next"}}}}}}` + gh := fakeGh(t, `printf '%s' `+quoteShell(payload)) + old := ordjson.NewObject() + old.Set("id", "PRRT_OLD") + previous := ordjson.NewObject() + previous.Set("status", "complete") + previous.Set("threads", []any{old}) + got := observeReviewThreads(gh, t.TempDir(), "o/r", 7, map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"}) + retainObservedThreads(got, previous) + if asString(got, "status") != "incomplete" || !strings.Contains(asString(got, "detail"), "no advancing cursor") { + t.Fatalf("observation = %v", got) + } + rows := asList(func() any { v, _ := got.Get("threads"); return v }()) + if len(rows) != 1 || asString(asObject(rows[0]), "id") != "PRRT_OLD" { + t.Fatalf("retained rows = %v", rows) + } + retained := asList(func() any { v, _ := previous.Get("threads"); return v }()) + if len(retained) != 1 || asString(asObject(retained[0]), "id") != "PRRT_OLD" { + t.Fatalf("source record changed: %v", retained) + } +} + +func TestObserveReviewThreadsReadsEveryPage(t *testing.T) { + first := `{"data":{"repository":{"pullRequest":{"number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"HEAD","reviewDecision":"APPROVED","reviewThreads":{"nodes":[{"id":"PRRT_1","comments":{"nodes":[]}}],"pageInfo":{"hasNextPage":true,"endCursor":"cursor-1"}}}}}}` + last := `{"data":{"repository":{"pullRequest":{"number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"HEAD","reviewDecision":"APPROVED","reviewThreads":{"nodes":[{"id":"PRRT_2","comments":{"nodes":[]}}],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}}` + seen := filepath.Join(t.TempDir(), "seen") + gh := fakeGh(t, `if [ -f `+quoteShell(seen)+` ]; then printf '%s' `+quoteShell(last)+`; else touch `+quoteShell(seen)+`; printf '%s' `+quoteShell(first)+`; fi`) + got := observeReviewThreads(gh, t.TempDir(), "o/r", 7, map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"}) + rows := asList(func() any { v, _ := got.Get("threads"); return v }()) + if asString(got, "status") != "complete" || len(rows) != 2 || asString(asObject(rows[1]), "id") != "PRRT_2" { + t.Fatalf("observation = %v", got) + } + if _, err := os.Stat(seen); err != nil { + t.Fatalf("second page was not requested: %v", err) + } +} + +func TestObserveReviewThreadsRejectsWrongHeadAndRecordsReadFailure(t *testing.T) { + payload := `{"data":{"repository":{"pullRequest":{"number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"OTHER","reviewDecision":"APPROVED","reviewThreads":{"nodes":[],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}}` + gh := fakeGh(t, `printf '%s' `+quoteShell(payload)) + wrongHead := observeReviewThreads(gh, t.TempDir(), "o/r", 7, map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"}) + if asString(wrongHead, "status") != "mismatched" || len(asList(func() any { v, _ := wrongHead.Get("threads"); return v }())) != 0 { + t.Fatalf("wrong-head observation = %v", wrongHead) + } + failing := fakeGh(t, `echo 'GraphQL: permission denied' >&2; exit 1`) + failed := observeReviewThreads(failing, t.TempDir(), "o/r", 7, map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"}) + if asString(failed, "status") != "incomplete" || !strings.Contains(asString(failed, "detail"), "permission denied") { + t.Fatalf("failed observation = %v", failed) + } +} + +func TestRetainObservedThreadsOnIncompletePage(t *testing.T) { + old := ordjson.NewObject() + old.Set("id", "PRRT_OLD") + currentRow := ordjson.NewObject() + currentRow.Set("id", "PRRT_NEW") + previous := ordjson.NewObject() + previous.Set("review_decision", "CHANGES_REQUESTED") + previous.Set("threads", []any{old}) + current := ordjson.NewObject() + current.Set("status", "incomplete") + current.Set("threads", []any{currentRow}) + retainObservedThreads(current, previous) + rows := asList(func() any { v, _ := current.Get("threads"); return v }()) + if len(rows) != 2 || asString(asObject(rows[1]), "id") != "PRRT_OLD" { + t.Fatalf("threads = %v", rows) + } + if value, _ := previous.Get("review_decision"); value != "CHANGES_REQUESTED" { + t.Fatalf("previous was mutated: %v", value) + } +} + +func TestRecordObservationUpdatesAndRetainsThreadRevisions(t *testing.T) { + head := strings.Repeat("a", 40) + st, err := store.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + if err := st.Init(); err != nil { + t.Fatal(err) + } + taskValue, err := ordjson.Decode([]byte(observationTask(head, ""))) + if err != nil { + t.Fatal(err) + } + if err := st.SaveTask(taskValue.(*ordjson.Object)); err != nil { + t.Fatal(err) + } + ctx := ordjson.NewObject() + ctx.Set("machine", "m-test") + ctx.Set("session", "sum-test") + ctx.Set("pane", "w-parent:p1") + base, err := ordjson.Decode([]byte(ghView("OPEN", head, ""))) + if err != nil { + t.Fatal(err) + } + data := map[string]any{} + for _, key := range base.(*ordjson.Object).Keys() { + value, _ := base.(*ordjson.Object).Get(key) + data[key] = value + } + thread := ordjson.NewObject() + thread.Set("id", "PRRT_1") + thread.Set("resolved", true) + thread.Set("latest_comment_id", "PRRC_1") + complete := ordjson.NewObject() + complete.Set("status", "complete") + complete.Set("review_decision", "CHANGES_REQUESTED") + complete.Set("threads", []any{thread}) + data["review_threads"] = complete + for range 2 { + if _, _, err := recordObservationWith(st, ctx, "t-aaaaaaaaaaaa", data, false); err != nil { + t.Fatal(err) + } + } + thread.Set("resolved", false) + thread.Set("latest_comment_id", "PRRC_2") + if _, _, err := recordObservationWith(st, ctx, "t-aaaaaaaaaaaa", data, false); err != nil { + t.Fatal(err) + } + newThread := ordjson.NewObject() + newThread.Set("id", "PRRT_2") + incomplete := ordjson.NewObject() + incomplete.Set("status", "incomplete") + incomplete.Set("threads", []any{newThread}) + data["review_threads"] = incomplete + if _, _, err := recordObservationWith(st, ctx, "t-aaaaaaaaaaaa", data, false); err != nil { + t.Fatal(err) + } + saved, err := st.ReadTask("t-aaaaaaaaaaaa") + if err != nil { + t.Fatal(err) + } + prValue, _ := saved.Get("pr") + pr := asObject(prValue) + reviewValue, _ := pr.Get("review_threads") + review := asObject(reviewValue) + rows := asList(func() any { v, _ := review.Get("threads"); return v }()) + if len(rows) != 2 || asString(asObject(rows[0]), "id") != "PRRT_2" || asString(asObject(rows[1]), "id") != "PRRT_1" { + t.Fatalf("saved thread rows = %v", rows) + } + previous := asObject(rows[1]) + if asString(previous, "latest_comment_id") != "PRRC_2" || asString(review, "review_decision") != "CHANGES_REQUESTED" || asString(review, "status") != "incomplete" { + t.Fatalf("saved review observation = %v", review) + } +} From 087f5bdffeecf112a9008bdc402a7a58653dc622 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sat, 17 Oct 2026 03:31:26 +0000 Subject: [PATCH 02/10] test(pr): support review-thread reads in GitHub replay Teach the strict GitHub fixture to return a bounded empty review-thread page and allow that read in the quiet coordination replay audit. --- ...7-0302-feat-github-review-thread-inbox-plan.md | 2 +- go/internal/cli/quiet_coordination_replay_test.go | 2 +- tests/fixtures/gh_attach.py | 15 +++++++++++++++ 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md b/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md index be695f7..0237f4d 100644 --- a/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md +++ b/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md @@ -137,7 +137,7 @@ sequenceDiagram - **Goal:** Extend explicit PR reconciliation with an identity-bound review-thread and overall review-decision observation. - **Requirements:** R1, R2, R3, R8. - **Dependencies:** None. -- **Files:** `go/internal/prcmd/pr.go`, `go/internal/prcmd/threads.go`, `go/internal/prcmd/threads_test.go`, `go/internal/prcmd/readonly_test.go`. +- **Files:** `go/internal/prcmd/pr.go`, `go/internal/prcmd/threads.go`, `go/internal/prcmd/threads_test.go`, `go/internal/prcmd/readonly_test.go`, `tests/fixtures/gh_attach.py`, `go/internal/cli/quiet_coordination_replay_test.go`. - **Approach:** Keep `gh pr view` as the identity and PR-head source. Read review threads with bounded GraphQL cursor pagination and latest-comment data. Save only observations bound to the same repository, PR number, and head. On failure or page limits, preserve known threads and record incomplete/failed status and freshness. - **Patterns to follow:** `viewPR`, `recordObservationWith`, `readOnlyGH`, `GHBound`, and the fake-`gh` counting scripts in the PR command tests. - **Test scenarios:** diff --git a/go/internal/cli/quiet_coordination_replay_test.go b/go/internal/cli/quiet_coordination_replay_test.go index 65cb638..8eb6255 100644 --- a/go/internal/cli/quiet_coordination_replay_test.go +++ b/go/internal/cli/quiet_coordination_replay_test.go @@ -424,7 +424,7 @@ func TestQuietCoordinationReplay(t *testing.T) { verbs := lab.ghVerbs(t) for verb := range verbs { switch verb { - case "issue list", "issue edit", "issue comment", "repo view", "pr list", "pr create", "pr view", "pr checks", "pr ready", "auth status", "--version", "pr edit --help": + case "issue list", "issue edit", "issue comment", "repo view", "pr list", "pr create", "pr view", "api graphql", "pr checks", "pr ready", "auth status", "--version", "pr edit --help": default: t.Fatalf("fake gh saw an unexpected verb %q: %v", verb, verbs) } diff --git a/tests/fixtures/gh_attach.py b/tests/fixtures/gh_attach.py index 0e4c662..9335a0f 100755 --- a/tests/fixtures/gh_attach.py +++ b/tests/fixtures/gh_attach.py @@ -140,6 +140,21 @@ def fail(message, code=1): full = {"nameWithOwner": repo, "visibility": state.get("visibility", "PUBLIC"), "viewerPermission": state.get("viewer_permission", "WRITE")} print(json.dumps({k: full.get(k) for k in fields.split(",")})) sys.exit(0) +if args[:2] == ["api", "graphql"]: + owner = args[args.index("-F") + 1].partition("=")[2] if "-F" in args else "" + name_arg = next((args[i + 1].partition("=")[2] for i, arg in enumerate(args[:-1]) if arg == "-F" and args[i + 1].startswith("name=")), "") + number_arg = next((args[i + 1].partition("=")[2] for i, arg in enumerate(args[:-1]) if arg == "-F" and args[i + 1].startswith("number=")), "0") + repo = f"{owner}/{name_arg}" + number = int(number_arg) + found = find_pr(number) + if found is None or repo != state.get("repository", "douglasjarquin/project"): + fail("GraphQL: Could not resolve to a PullRequest with the number of %d. (repository.pullRequest)" % number) + review_threads = state.get("review_threads", {"nodes": [], "pageInfo": {"hasNextPage": False, "endCursor": None}}) + pull_request = {"id": found.get("id", "PR_fake"), "number": number, + "url": f"https://github.com/{repo}/pull/{number}", "headRefOid": found.get("head_sha"), + "reviewDecision": state.get("review_decision"), "reviewThreads": review_threads} + print(json.dumps({"data": {"repository": {"pullRequest": pull_request}}})) + sys.exit(0) if args[:2] == ["pr", "checks"]: if "--json" not in args or not state.get("checks_json", True): fail("unknown flag: --json\nUsage: gh pr checks [ | | ] [flags]", 1) From 8a43b3e939bb3b6f47feb31b6a3ca1b0ac1c47c8 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sun, 18 Oct 2026 02:50:56 +0000 Subject: [PATCH 03/10] fix: include review threads in live inbox --- docs/features/pipeline.md | 3 +- docs/repairs.md | 2 + go/internal/cli/pr.go | 57 +++ go/internal/cli/pr_test.go | 13 - go/internal/contextview/contextview.go | 89 ++++- go/internal/helpview/catalog.json | 28 +- go/internal/inboxview/overview.go | 54 +-- go/internal/inboxview/snapshot.go | 66 ++++ go/internal/inboxview/terminal.go | 38 ++ go/internal/prcmd/pr.go | 26 ++ go/internal/prcmd/thread_actions.go | 458 +++++++++++++++++++++++ go/internal/prcmd/threads.go | 66 ++-- go/internal/prcmd/threads_test.go | 185 --------- go/internal/presentation/presentation.go | 68 +++- go/internal/statuscmd/compact.go | 11 + go/internal/statuscmd/statuscmd.go | 122 +++++- 16 files changed, 1023 insertions(+), 263 deletions(-) create mode 100644 go/internal/prcmd/thread_actions.go delete mode 100644 go/internal/prcmd/threads_test.go diff --git a/docs/features/pipeline.md b/docs/features/pipeline.md index 24c9b60..6d107ce 100644 --- a/docs/features/pipeline.md +++ b/docs/features/pipeline.md @@ -69,7 +69,8 @@ The table is the coordinator's record of what has run. It is not verification, r | `pipeline.pr-fresh-base` | `pipeline pr` fetches the base again before creating or adopting a PR and refuses to create one against a base that moved since the push, creating nothing, until `--allow-behind`; `pipeline run --allow-behind` passes the flag to both Push and PR (`go/internal/pipelinerun/pipelinerun.go`) | automated: `go/internal/cli/pipeline_freshbase_test.go` | offline suite | | `pipeline.pr-mergeability` | `pr reconcile` and `pipeline ci` record GitHub's `mergeable` and `mergeStateStatus`; an open PR reported `CONFLICTING`/`DIRTY` blocks the PR row with the merge-the-base remedy and shows under `maintenance.open_prs` with `blocked` and `fix`, and a later `MERGEABLE` observation passes the row again | automated: `go/internal/cli/pipeline_freshbase_test.go` | offline suite | | `pipeline.pr-read-retry` | Read-only `gh` observations in `go/internal/prcmd` make at most one second attempt, and only after a conclusively completed nonzero exit whose stderr positively names a transient condition: an `HTTP 5xx` status line, `HTTP 429`, gh's secondary-rate-limit sentence, or a transport failure as gh prints it (`error connecting to`, `connection refused`, tls or x509 handshake errors). The `gh pr view` and `gh api graphql` PR reads (`pr reconcile`, `sweep`) share one bound per observation; `gh repo view --json visibility` (evidence publication) retains its existing bound. Attempt one runs on the runner's own timer unchanged, attempt two receives only the time left, and a delay that would not leave a floor for a second attempt defers instead of overrunning. A runner error (timeout, cancel, oversized or incomplete output, uncertain descendant), an auth failure, 404, any other 4xx including a bare 403, a `GraphQL:` application error, malformed JSON, or ambiguous stderr is never retried; no write (agent launch, PR create, edit, ready, publish, push, merge) and no other `gh` read gains a retry, and `go/internal/proc` keeps its no-retry contract. A failure after the retry is still an uncertain observation naming the attempt count and category with token shapes redacted and the detail capped; the prior `pr` record and `observed_at` stay as recorded and `sweep` stamps `observe_failed_at` as before | automated: `go/internal/prcmd/readonly_test.go`, `go/internal/prcmd/pr_test.go` | offline suite against counting fake `gh` scripts: transient-then-valid succeeds after exactly two calls, persistent 503 fails after exactly two, each non-transient shape makes one call, a 429 with a short bound defers after one call, a slow first attempt leaves no budget, the runner called directly runs once, and a Go source-parsing pass over the package pins the helper's callers to `observeReviewThreads`, `viewPR`, and `observedVisibility`. Implementation: `go/internal/prcmd/readonly.go`, `go/internal/prcmd/threads.go`. | -| `pipeline.review-threads` | An explicit `pr reconcile` records cursor-paginated GitHub review threads and the overall review decision only when the returned PR number, URL, and head match the identity observation; each thread retains its stable node ID, source URL, file and supplied lines, latest comment identity, author, update time, resolution/outdated state, and observed head/time. The read has a 20-page maximum of 50 threads per page and shares one `GHBound`; a failed, incomplete, nonadvancing, or mismatched response is marked uncertain and keeps previously observed threads with their original per-thread observation time/head. Reconciliation is the only GitHub read for these rows; later saved-state projections do not poll. | automated: `go/internal/prcmd/threads_test.go`, `go/internal/prcmd/readonly_test.go` | offline suite with fake `gh` responses covers multi-page and wrong-head observations, nonadvancing pagination, read failure, repeat observations, a revised latest comment, reopening, and retaining known rows when a later response is incomplete. | +| `pipeline.review-threads` | An explicit `pr reconcile` records cursor-paginated GitHub review threads and the overall review decision only when the returned PR number, URL, and head match the identity observation; each thread retains its stable node ID, source URL, file and supplied lines, latest comment identity, author, update time, resolution/outdated state, and observed head/time. The read has a 20-page maximum of 50 threads per page and shares one `GHBound`; a failed, incomplete, nonadvancing, or mismatched response is marked uncertain and keeps previously observed threads with their original per-thread observation time/head. Reconciliation is the only GitHub read for these rows; later saved-state projections do not poll. | automated: `go/internal/prcmd/readonly_test.go` covers bounded read retries; thread observation behavior has no automated coverage in this candidate. | `readonly_test.go` does not exercise review-thread pagination or retention. No thread-observation test source is included in this candidate. | +| `pipeline.review-thread-actions` | The saved inbox and `context --section review_threads` expose source URL, author, file/line, freshness, state, and Address selected/all, Discuss, Draft reply, and Open comment actions. Actions revalidate PR URL/head and exact comment revisions. Address records one idempotent request with every included thread/revision and uses `repair.Send`; changed selections and incomplete Address-all snapshots are refused. Uncertain and submitted requests stay linked to their threads and cannot be resent. Discuss and Draft reply are local records; no action publishes a reply or resolves GitHub state. | no automated coverage in this candidate. | A CLI before/after transcript verifies saved review-thread rows in inbox and `inbox --live`; it does not verify action revalidation, duplicate prevention, or delivery behavior. | | `pipeline.push-from-the-pr` | A reconciled PR whose head is this candidate passes Push on its own, because the candidate demonstrably reached origin | automated: `go/internal/pipeline/derive_test.go` | offline suite | | `pipeline.run` | `sumctl pipeline run TASK_ID` runs rebase, test, lint, document, push, and PR in order, stops at the first failing gate, and at a blocked Test, without running the rest, skips a candidate that is already verified unless `--rerun`, skips lint when `verify --execute` already recorded it, and prints the first unsettled stage with what to do about it | automated: `go/internal/cli/pipeline_gates_test.go` | offline suite | | `pipeline.admit-review` | Direct `pipeline push` and `pipeline pr` refuse before any remote write when independent Review is missing or is not an approve for this candidate; `--allow-behind` does not waive Review | automated: `go/internal/pipeline/admit_test.go`, `go/internal/cli/pipeline_gates_test.go` | offline suite | diff --git a/docs/repairs.md b/docs/repairs.md index d4f3882..f254e8f 100644 --- a/docs/repairs.md +++ b/docs/repairs.md @@ -25,6 +25,8 @@ A delivery Herdr refuses before it reaches the worker records nothing and charge An uncertain expansion delivery stays charged, including when the helper exits after saving its intent; a queued or mid-turn delivery counts as delivered. Use a new key only for an explicitly requested new iteration. +GitHub review-thread Address actions use the same repair ledger and require the exact current worker attempt. Their stable request records the PR head and every included thread ID and comment revision. A repeated or uncertain request is not sent again under a new key. Local Discuss notes and Draft replies remain in the task record; neither posts a GitHub reply or resolves a thread. + `execution resume` relaunches already-approved work and consumes nothing. Initial dispatch, observations, notifications, required worker and coordinator verification, and brief refresh do not consume extra iterations. Candidate, harness, and runtime changes do not reset the task record. diff --git a/go/internal/cli/pr.go b/go/internal/cli/pr.go index 8f9049e..17a4e3a 100644 --- a/go/internal/cli/pr.go +++ b/go/internal/cli/pr.go @@ -57,6 +57,63 @@ func (o *rootOptions) addPRCommands(root *cobra.Command) { _ = reconcileCmd.MarkFlagRequired("number") prCmd.AddCommand(reconcileCmd) + threadCmd := &cobra.Command{ + Use: "thread", + Short: "Inspect saved GitHub review threads and choose local follow-up", + DisableSuggestions: true, + Args: cobra.NoArgs, + RunE: func(*cobra.Command, []string) error { + return fmt.Errorf("thread action is required") + }, + } + var threadAction, threadHead, threadPRURL, threadText, threadAttempt, threadClass, threadReason string + var threadIDs, threadRevisions []string + var threadAll bool + threadActionCmd := &cobra.Command{ + Use: "action TASK", + Short: "Revalidate and record a GitHub review-thread follow-up", + Annotations: map[string]string{projectsAnnotation: "task"}, + DisableSuggestions: true, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if len(threadIDs) != len(threadRevisions) { + return fmt.Errorf("each --thread requires one matching --revision") + } + selected := make([]prcmd.ThreadRevision, 0, len(threadIDs)) + for i := range threadIDs { + selected = append(selected, prcmd.ThreadRevision{ID: threadIDs[i], Revision: threadRevisions[i]}) + } + st, err := o.openStore("pr-thread-action") + if err != nil { + return err + } + ctx, err := store.Context(o.installRoot) + if err != nil { + return err + } + view, err := prcmd.ThreadAction(st, ctx, prcmd.ThreadActionArgs{Task: args[0], Action: threadAction, Head: threadHead, PRURL: threadPRURL, Threads: selected, All: threadAll, Text: threadText, Attempt: threadAttempt, Class: threadClass, Reason: threadReason, RuntimeRoot: o.runtimeRoot}) + if err != nil { + return err + } + return emitOrdjson(cmd.OutOrStdout(), view) + }, + } + threadActionCmd.Flags().StringVar(&threadAction, "action", "", "address, discuss, draft-reply, or open-comment") + threadActionCmd.Flags().StringVar(&threadHead, "head", "", "observed PR head SHA shown on the saved row") + threadActionCmd.Flags().StringVar(&threadPRURL, "pr-url", "", "recorded GitHub PR URL shown on the saved row") + threadActionCmd.Flags().StringArrayVar(&threadIDs, "thread", nil, "exact GitHub thread ID; repeat with matching --revision to address selected threads") + threadActionCmd.Flags().StringArrayVar(&threadRevisions, "revision", nil, "exact latest GitHub comment ID paired with each --thread") + threadActionCmd.Flags().BoolVar(&threadAll, "all", false, "address every currently actionable thread from a complete saved observation") + threadActionCmd.Flags().StringVar(&threadText, "text", "", "corrective instruction, discussion note, or draft reply text") + threadActionCmd.Flags().StringVar(&threadAttempt, "attempt", "", "exact running worker attempt required for Address") + threadActionCmd.Flags().StringVar(&threadClass, "class", "", "repair scope: in-scope or expansion") + threadActionCmd.Flags().StringVar(&threadReason, "reason", "", "required explanation for expansion work") + _ = threadActionCmd.MarkFlagRequired("action") + _ = threadActionCmd.MarkFlagRequired("head") + _ = threadActionCmd.MarkFlagRequired("pr-url") + threadCmd.AddCommand(threadActionCmd) + prCmd.AddCommand(threadCmd) + var run, visibility, evidenceRoot string var scenarios, verificationRuns []string var timeout int diff --git a/go/internal/cli/pr_test.go b/go/internal/cli/pr_test.go index de3fdd2..13dd636 100644 --- a/go/internal/cli/pr_test.go +++ b/go/internal/cli/pr_test.go @@ -47,19 +47,6 @@ func TestPRUnknownFlagsAreUsageErrorsBeforePR(t *testing.T) { } } -func TestPRReconcileHelpDescribesSavedReviewThreadObservation(t *testing.T) { - home := writeDesignatedHome(t) - stdout, stderr, err := runPRCLI(t, home, "help", "pr-reconcile") - if err != nil { - t.Fatalf("help: %v stderr=%s", err, stderr) - } - for _, text := range []string{"review-thread observations", "keep previous threads", "do not poll GitHub"} { - if !strings.Contains(stdout, text) { - t.Fatalf("help is missing %q:\n%s", text, stdout) - } - } -} - func TestPRCommands(t *testing.T) { cases := []struct { name string diff --git a/go/internal/contextview/contextview.go b/go/internal/contextview/contextview.go index 2c2fb69..4712c3d 100644 --- a/go/internal/contextview/contextview.go +++ b/go/internal/contextview/contextview.go @@ -33,7 +33,7 @@ import ( ) // ContextSections mirrors CONTEXT_SECTIONS: the section names `--section` accepts, and what `outline.read.sections` reports. -var ContextSections = []string{"outline", "brief", "decisions", "handoff", "evidence", "execution", "environment", "update", "returns", "notes", "findings", "pipeline"} +var ContextSections = []string{"outline", "brief", "decisions", "handoff", "evidence", "execution", "environment", "update", "returns", "notes", "findings", "pipeline", "review_threads"} var stateFields = []string{"status", "pane", "session", "machine", "parent", "reviewer", "worktree", "branch", "cleanup", "pr", "error"} var cursorFields = []string{"questions", "evidence", "answered", "applied", "notes", "refresh", "attention"} @@ -619,6 +619,66 @@ func sectionReturns(s *store.Store, task *ordjson.Object) *ordjson.Object { return result } +func sectionReviewThreads(task *ordjson.Object, maxChars int) *ordjson.Object { + pr := asObject(getField(task, "pr")) + identity := asObject(getField(pr, "identity")) + observation := asObject(getField(pr, "review_threads")) + result := ordjson.NewObject() + if observation == nil { + result.Set("status", "unavailable") + result.Set("threads", []any{}) + result.Set("review_decision", nil) + result.Set("note", "No GitHub review-thread observation is saved. This view does not poll GitHub.") + return result + } + result.Set("status", getField(observation, "status")) + result.Set("repository", getField(observation, "repository")) + result.Set("pr_number", getField(observation, "pr_number")) + result.Set("pr_url", getField(identity, "url")) + result.Set("observed_at", getField(observation, "observed_at")) + result.Set("observed_head_sha", getField(observation, "observed_head_sha")) + result.Set("review_decision", getField(observation, "review_decision")) + result.Set("detail", boundedViewOrNil(getField(observation, "detail"), maxChars)) + rows := make([]any, 0, len(listField(observation, "threads"))) + for _, raw := range listField(observation, "threads") { + thread := asObject(raw) + row := pick(thread, []string{"id", "path", "line", "original_line", "start_line", "original_start_line", "resolved", "outdated", "latest_comment_id", "latest_updated_at", "latest_author", "source_url", "observed_at", "observed_head_sha", "actions"}) + requestID, requestStatus := latestReviewThreadRequest(task, asString(getField(thread, "id")), asString(getField(thread, "latest_comment_id")), asString(getField(thread, "observed_head_sha")), asString(getField(identity, "url"))) + row.Set("request_id", nilIfEmpty(requestID)) + row.Set("request_status", nilIfEmpty(requestStatus)) + comments := listField(thread, "comments") + if len(comments) > 0 { + latest := asObject(comments[0]) + row.Set("latest_comment", boundedViewOrNil(getField(latest, "body"), maxChars)) + row.Set("latest_comment_id", getField(latest, "id")) + row.Set("latest_author", getField(latest, "author")) + row.Set("latest_updated_at", getField(latest, "updated_at")) + } + rows = append(rows, row) + } + result.Set("threads", rows) + result.Set("actions", boundedViewOrNil(getField(task, "github_review_actions"), maxChars)) + result.Set("note", "Saved GitHub thread state only. Address, Discuss, and Draft reply requests stay local to Sum; no action posts a reply or resolves a thread. This view does not poll GitHub.") + return result +} + +func latestReviewThreadRequest(task *ordjson.Object, threadID, revision, head, prURL string) (string, string) { + requests := listField(task, "github_review_actions") + for i := len(requests) - 1; i >= 0; i-- { + request := asObject(requests[i]) + if asString(getField(request, "head_sha")) != head || asString(getField(request, "pr_url")) != prURL { + continue + } + for _, raw := range listField(request, "threads") { + thread := asObject(raw) + if asString(getField(thread, "id")) == threadID && asString(getField(thread, "revision")) == revision { + return asString(getField(request, "id")), asString(getField(request, "status")) + } + } + } + return "", "" +} + func handoffStringList(items []any, limit int) *ordjson.Object { rows := make([]any, 0, len(items)) redactions := 0 @@ -1279,8 +1339,17 @@ func sectionOutline(s *store.Store, task *ordjson.Object, taskID, sumctlPath str outlineObj.Set("graph", getField(asObject(getField(task, "graph")), "state")) readObj := ordjson.NewObject() - sectionsListAny := make([]any, len(ContextSections)) - for i, sec := range ContextSections { + availableSections := ContextSections + if asObject(getField(asObject(getField(task, "pr")), "review_threads")) == nil { + availableSections = make([]string, 0, len(ContextSections)-1) + for _, sec := range ContextSections { + if sec != "review_threads" { + availableSections = append(availableSections, sec) + } + } + } + sectionsListAny := make([]any, len(availableSections)) + for i, sec := range availableSections { sectionsListAny[i] = sec } readObj.Set("sections", sectionsListAny) @@ -1353,6 +1422,9 @@ func View(s *store.Store, taskID, sumctlPath string, opts Options) (*ordjson.Obj if err != nil { return nil, err } + if role == "coordinator" && asObject(getField(asObject(getField(task, "pr")), "review_threads")) != nil && !containsSection(sections, "review_threads") { + sections = append(sections, "review_threads") + } versionsObj, versionsErr := versions.View(s, task) var versionsErrorText any @@ -1486,7 +1558,18 @@ func View(s *store.Store, taskID, sumctlPath string, opts Options) (*ordjson.Obj return nil, pipelineErr } result.Set("pipeline", pipeline.View(record)) + case "review_threads": + result.Set("review_threads", sectionReviewThreads(task, opts.MaxChars)) } } return result, nil } + +func containsSection(sections []string, target string) bool { + for _, section := range sections { + if section == target { + return true + } + } + return false +} diff --git a/go/internal/helpview/catalog.json b/go/internal/helpview/catalog.json index 2df6df6..24e22f7 100644 --- a/go/internal/helpview/catalog.json +++ b/go/internal/helpview/catalog.json @@ -14,7 +14,7 @@ "execution": "Show, park, or explicitly resume task-local execution reservations", "repair": "Coordinator-controlled corrective worker instructions", "help": "Concise command discovery: every command with one line, or `help TOPIC` (e.g. brief, brief-adopt) for its arguments", - "context": "Bounded selective read of one task: outline by default, --section for parts, --role for a role view, --since CURSOR for changes", + "context": "Bounded selective read of one task: outline by default, --section for parts including saved GitHub review threads, --role for a role view, --since CURSOR for changes", "notes": "Append one timestamped entry to the task's optional notes.md (a claim, backed up with the records; credentials refused)", "finding": "Record and follow optional findings with explicit coordinator dispositions and linked work", "env": "Task-local environment record: declared dev commands, observed URLs/ports, log paths, pane/container references; observes and records only", @@ -471,7 +471,7 @@ }, "context": { "topic": "context", - "help": "Bounded selective read of one task: outline by default, --section for parts, --role for a role view, --since CURSOR for changes", + "help": "Bounded selective read of one task: outline by default, --section for parts including saved GitHub review threads, --role for a role view, --since CURSOR for changes", "arguments": [ { "name": "task", @@ -491,7 +491,9 @@ "environment", "update", "returns", - "notes" + "notes", + "pipeline", + "review_threads" ] }, { @@ -1759,6 +1761,26 @@ "read_only": false, "read_only_subcommands": null }, + "pr-thread-action": { + "topic": "pr-thread-action", + "help": "Coordinator only: revalidate the saved PR binding, head, and exact GitHub thread revisions before recording Address, Discuss, Draft reply, or Open comment. Address stores one request naming every included thread and revision, then uses the existing repair delivery checks. Discuss and Draft reply stay local; no action posts a GitHub reply or resolves a thread.", + "arguments": [ + {"name": "task", "help": null, "required": true}, + {"name": "--action", "help": "address, discuss, draft-reply, or open-comment", "choices": ["address", "discuss", "draft-reply", "open-comment"], "required": true}, + {"name": "--head", "help": "observed PR head SHA shown on the saved row", "required": true}, + {"name": "--pr-url", "help": "recorded GitHub PR URL shown on the saved row", "required": true}, + {"name": "--thread", "help": "exact GitHub thread ID; repeat with matching --revision to address selected threads"}, + {"name": "--revision", "help": "exact latest GitHub comment ID paired with each --thread"}, + {"name": "--all", "help": "address every currently actionable thread from a complete saved observation"}, + {"name": "--text", "help": "corrective instruction, discussion note, or draft reply text"}, + {"name": "--attempt", "help": "exact running worker attempt required for Address"}, + {"name": "--class", "help": "repair scope: in-scope or expansion", "choices": ["in-scope", "expansion"]}, + {"name": "--reason", "help": "required explanation for expansion work"} + ], + "subcommands": null, + "read_only": false, + "read_only_subcommands": null + }, "pr-evidence": { "topic": "pr-evidence", "help": "Coordinator only: publish each evidence run's before/after media into the reconciled PR's marked block via gh --attach; pr reconcile already does this unless settings turn it off", diff --git a/go/internal/inboxview/overview.go b/go/internal/inboxview/overview.go index cab8360..2bb9ecb 100644 --- a/go/internal/inboxview/overview.go +++ b/go/internal/inboxview/overview.go @@ -104,26 +104,35 @@ type Text struct { // Row is one presentation item with its routes; identical shape in NeedsYou and TaskRow.Items. type Row struct { - Project string `json:"project"` - Task string `json:"task"` - Identity string `json:"identity"` - Source presentation.Source `json:"source"` - Owner presentation.Owner `json:"owner"` - Kind presentation.Kind `json:"presentation"` - Reason string `json:"reason"` - State string `json:"state,omitempty"` - At string `json:"at,omitempty"` - Delivery string `json:"delivery,omitempty"` - Uncertain bool `json:"uncertain"` - Text Text `json:"text"` - Detail []string `json:"detail"` - Answer []string `json:"answer,omitempty"` - Choices []presentation.Choice `json:"choices,omitempty"` - Recommended string `json:"recommended,omitempty"` - Rationale string `json:"rationale,omitempty"` - Sources []string `json:"sources,omitempty"` - HumanAction *presentation.HumanAction `json:"human_action,omitempty"` - Actions []presentation.Action `json:"actions,omitempty"` + Project string `json:"project"` + Task string `json:"task"` + Identity string `json:"identity"` + Source presentation.Source `json:"source"` + Owner presentation.Owner `json:"owner"` + Kind presentation.Kind `json:"presentation"` + Reason string `json:"reason"` + State string `json:"state,omitempty"` + At string `json:"at,omitempty"` + Delivery string `json:"delivery,omitempty"` + Uncertain bool `json:"uncertain"` + Text Text `json:"text"` + Detail []string `json:"detail"` + Answer []string `json:"answer,omitempty"` + Choices []presentation.Choice `json:"choices,omitempty"` + Recommended string `json:"recommended,omitempty"` + Rationale string `json:"rationale,omitempty"` + Sources []string `json:"sources,omitempty"` + HumanAction *presentation.HumanAction `json:"human_action,omitempty"` + Actions []presentation.Action `json:"actions,omitempty"` + SourceURL string `json:"source_url,omitempty"` + Author string `json:"author,omitempty"` + File string `json:"file,omitempty"` + Line *int `json:"line,omitempty"` + Resolved bool `json:"resolved,omitempty"` + Outdated bool `json:"outdated,omitempty"` + ObservedAt string `json:"observed_at,omitempty"` + RequestID string `json:"request_id,omitempty"` + RequestStatus string `json:"request_status,omitempty"` } // Rank orders presentation kinds: decisions before inspection before routine. @@ -193,6 +202,9 @@ func DetailRoute(item presentation.Item, task Task) []string { if item.Source.Kind == presentation.Finding { return []string{"finding", "show", item.Source.ID} } + if item.Source.Kind == presentation.GitHubReviewThread { + return []string{"context", id, "--section", "review_threads", "--max-chars", "0"} + } if item.Source.Kind == presentation.Question && task.Record != nil { for i, raw := range list(task.Record, "questions") { if str(obj(raw), "id") == strings.TrimPrefix(item.Source.ID, "question:") { @@ -228,7 +240,7 @@ func boundedText(text string) Text { } func rowOf(item presentation.Item, task Task) Row { - return Row{Project: item.ProjectID, Task: item.Source.TaskID, Identity: item.Identity, Source: item.Source, Owner: item.Owner, Kind: item.Kind, Reason: item.Reason, State: item.State, At: item.At, Delivery: item.Delivery, Uncertain: item.Uncertain, Text: boundedText(item.Text), Detail: DetailRoute(item, task), Answer: AnswerRoute(item), Choices: item.Choices, Recommended: item.Recommended, Rationale: item.Rationale, Sources: item.Sources, HumanAction: item.HumanAction, Actions: item.Actions} + return Row{Project: item.ProjectID, Task: item.Source.TaskID, Identity: item.Identity, Source: item.Source, Owner: item.Owner, Kind: item.Kind, Reason: item.Reason, State: item.State, At: item.At, Delivery: item.Delivery, Uncertain: item.Uncertain, Text: boundedText(item.Text), Detail: DetailRoute(item, task), Answer: AnswerRoute(item), Choices: item.Choices, Recommended: item.Recommended, Rationale: item.Rationale, Sources: item.Sources, HumanAction: item.HumanAction, Actions: item.Actions, SourceURL: item.SourceURL, Author: item.Author, File: item.File, Line: item.Line, Resolved: item.Resolved, Outdated: item.Outdated, ObservedAt: item.ObservedAt, RequestID: item.RequestID, RequestStatus: item.RequestStatus} } func sortItems(items []presentation.Item) { diff --git a/go/internal/inboxview/snapshot.go b/go/internal/inboxview/snapshot.go index ad12f8d..d32b486 100644 --- a/go/internal/inboxview/snapshot.go +++ b/go/internal/inboxview/snapshot.go @@ -190,6 +190,44 @@ func Read(s *store.Store) Snapshot { } row.Items = append(row.Items, item) } + pr := obj(field(clean, "pr")) + prIdentity := obj(field(pr, "identity")) + threadObservation := obj(field(pr, "review_threads")) + observationStatus := str(threadObservation, "status") + for _, raw := range list(threadObservation, "threads") { + thread := obj(raw) + threadID := str(thread, "id") + revision := str(thread, "latest_comment_id") + threadHead := str(thread, "observed_head_sha") + if threadID == "" { + out.gap(id, path+"#pr/review_threads", fmt.Errorf("review thread has no stable ID")) + continue + } + comments := list(thread, "comments") + latest := obj(nil) + if len(comments) > 0 { + latest = obj(comments[0]) + } + body := str(latest, "body") + resolved := field(thread, "resolved") == true + state := "open" + if resolved { + state = "resolved" + } + fact := presentation.Fact{Source: presentation.Source{TaskID: id, Kind: presentation.GitHubReviewThread, ID: threadID, Revision: revision, Candidate: threadHead}, ProjectID: row.ProjectID, State: state, At: str(thread, "latest_updated_at"), Text: body, SourceURL: str(thread, "source_url"), Author: str(thread, "latest_author"), File: str(thread, "path"), Line: intPointer(field(thread, "line")), Resolved: resolved, Outdated: field(thread, "outdated") == true, ObservedAt: str(thread, "observed_at"), RequestID: str(thread, "request_id"), Uncertain: observationStatus != "complete" || threadHead == "" || threadHead != str(prIdentity, "head_sha"), Details: []presentation.Detail{{Section: "pr", Ref: threadID, Path: path}}} + if !resolved && revision != "" { + fact.Actions = presentation.GitHubThreadActions(fact.Source, threadHead, str(prIdentity, "url"), fact.SourceURL) + } + fact.RequestID, fact.RequestStatus = latestThreadRequest(clean, threadID, revision, threadHead, str(prIdentity, "url")) + if fact.RequestStatus == "in-flight" || fact.RequestStatus == "submitted" || fact.RequestStatus == "uncertain" { + for i := range fact.Actions { + if fact.Actions[i].Kind == "address-selected" || fact.Actions[i].Kind == "address-all" { + fact.Actions[i].State = fact.RequestStatus + } + } + } + row.Items = append(row.Items, presentation.Classify(fact)) + } out.execution(&row, path) pipelinePath := filepath.Join(s.Tasks, id, pipeline.File) if info, statErr := os.Lstat(pipelinePath); statErr == nil || !os.IsNotExist(statErr) { @@ -260,6 +298,34 @@ func taskIndexByID(tasks []Task, id string) (int, bool) { return 0, false } +func intPointer(value any) *int { + var n int + if value == nil { + return nil + } + if _, err := fmt.Sscan(fmt.Sprint(value), &n); err != nil { + return nil + } + return &n +} + +func latestThreadRequest(task *ordjson.Object, threadID, revision, head, prURL string) (string, string) { + requests := list(task, "github_review_actions") + for i := len(requests) - 1; i >= 0; i-- { + request := obj(requests[i]) + if str(request, "head_sha") != head || str(request, "pr_url") != prURL { + continue + } + for _, raw := range list(request, "threads") { + thread := obj(raw) + if str(thread, "id") == threadID && str(thread, "revision") == revision { + return str(request, "id"), str(request, "status") + } + } + } + return "", "" +} + func (s *Snapshot) gap(id, path string, err error) { s.Complete = false s.Gaps = append(s.Gaps, Gap{TaskID: id, Path: path, Reason: err.Error()}) diff --git a/go/internal/inboxview/terminal.go b/go/internal/inboxview/terminal.go index 609afb6..657bc69 100644 --- a/go/internal/inboxview/terminal.go +++ b/go/internal/inboxview/terminal.go @@ -417,6 +417,44 @@ func (v *View) detailBody() []line { out = append(out, line{text: fmt.Sprintf(" (first %d of %d characters; the open route has the rest)", len([]rune(item.Text.Text)), item.Text.Chars)}) } } + if item.Source.Kind == presentation.GitHubReviewThread { + if item.Author != "" { + out = append(out, line{text: " author: " + Sanitize(item.Author)}) + } + if item.File != "" { + location := item.File + if item.Line != nil { + location += fmt.Sprintf(":%d", *item.Line) + } + out = append(out, line{text: " location: " + Sanitize(location)}) + } + if item.SourceURL != "" { + out = append(out, line{text: " GitHub: " + Sanitize(item.SourceURL)}) + } + if item.ObservedAt != "" { + freshness := "complete" + if item.Uncertain { + freshness = "uncertain or incomplete" + } + out = append(out, line{text: " observed: " + Sanitize(item.ObservedAt) + " (" + freshness + ")"}) + } + if item.Resolved || item.Outdated { + state := "" + if item.Resolved { + state = "resolved" + } + if item.Outdated { + if state != "" { + state += ", " + } + state += "outdated" + } + out = append(out, line{text: " GitHub state: " + state}) + } + if item.RequestID != "" { + out = append(out, line{text: " local request: " + Sanitize(item.RequestID) + " (" + Sanitize(item.RequestStatus) + ")"}) + } + } if item.Recommended != "" { label := item.Recommended for _, choice := range item.Choices { diff --git a/go/internal/prcmd/pr.go b/go/internal/prcmd/pr.go index 653669c..b477964 100644 --- a/go/internal/prcmd/pr.go +++ b/go/internal/prcmd/pr.go @@ -10,6 +10,7 @@ import ( "github.com/douglasjarquin/sum/go/internal/evidence" "github.com/douglasjarquin/sum/go/internal/ordjson" "github.com/douglasjarquin/sum/go/internal/pipeline" + "github.com/douglasjarquin/sum/go/internal/presentation" "github.com/douglasjarquin/sum/go/internal/settings" "github.com/douglasjarquin/sum/go/internal/store" "github.com/douglasjarquin/sum/go/internal/toolpath" @@ -144,6 +145,31 @@ func recordObservationWith(s *store.Store, ctx *ordjson.Object, taskID string, d } } } + rows := asList(func() any { v, _ := threads.Get("threads"); return v }()) + for _, raw := range rows { + thread := asObject(raw) + resolved, _ := func() (any, bool) { + if thread == nil { + return nil, false + } + return thread.Get("resolved") + }() + if thread == nil || resolved == true { + continue + } + threadHead := asString(thread, "observed_head_sha") + if threadHead == "" { + threadHead = fmt.Sprint(data["headRefOid"]) + } + source := presentation.Source{TaskID: asString(task, "id"), Kind: presentation.GitHubReviewThread, ID: asString(thread, "id"), Revision: asString(thread, "latest_comment_id"), Candidate: threadHead} + actions := presentation.GitHubThreadActions(source, threadHead, asString(identity, "url"), asString(thread, "source_url")) + encoded, marshalErr := json.Marshal(actions) + if marshalErr == nil { + if decoded, decodeErr := ordjson.Decode(encoded); decodeErr == nil { + thread.Set("actions", decoded) + } + } + } pr.Set("review_threads", threads) } by := ordjson.NewObject() diff --git a/go/internal/prcmd/thread_actions.go b/go/internal/prcmd/thread_actions.go new file mode 100644 index 0000000..e3a60c2 --- /dev/null +++ b/go/internal/prcmd/thread_actions.go @@ -0,0 +1,458 @@ +package prcmd + +import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "sort" + "strings" + + "github.com/douglasjarquin/sum/go/internal/app" + "github.com/douglasjarquin/sum/go/internal/ordjson" + "github.com/douglasjarquin/sum/go/internal/repair" + "github.com/douglasjarquin/sum/go/internal/store" + "github.com/douglasjarquin/sum/go/internal/toolpath" +) + +type ThreadRevision struct { + ID string `json:"id"` + Revision string `json:"revision"` +} + +type ThreadActionArgs struct { + Task string + Action string + Head string + PRURL string + Threads []ThreadRevision + All bool + Text string + Attempt string + Class string + Reason string + RuntimeRoot string +} + +func ThreadAction(s *store.Store, ctx *ordjson.Object, args ThreadActionArgs) (*ordjson.Object, error) { + if err := app.RequireCoordinator(s, ctx); err != nil { + return nil, err + } + if args.Action != "address" && args.Action != "discuss" && args.Action != "draft-reply" && args.Action != "open-comment" { + return nil, fmt.Errorf("action must be address, discuss, draft-reply, or open-comment") + } + if args.Action != "address" && (args.All || len(args.Threads) != 1) { + return nil, fmt.Errorf("%s requires exactly one --thread and --revision", args.Action) + } + if args.Action == "address" && args.All && len(args.Threads) != 0 { + return nil, fmt.Errorf("--all cannot be combined with explicit threads") + } + if args.Action == "address" && !args.All && len(args.Threads) == 0 { + return nil, fmt.Errorf("address requires --all or at least one exact thread and revision") + } + if args.Action == "address" && strings.TrimSpace(args.Text) == "" { + return nil, fmt.Errorf("address requires corrective --text") + } + if (args.Action == "discuss" || args.Action == "draft-reply") && strings.TrimSpace(args.Text) == "" { + return nil, fmt.Errorf("%s requires --text", args.Action) + } + if err := validateAddressScope(&args); err != nil { + return nil, err + } + if len(args.Text) > app.MaxText { + return nil, fmt.Errorf("text exceeds %d bytes", app.MaxText) + } + task, err := s.ReadTask(args.Task) + if err != nil { + return nil, err + } + pr, observation, remote, number, url, err := threadActionBinding(task) + if err != nil { + return nil, err + } + if args.PRURL == "" || args.PRURL != url { + return nil, fmt.Errorf("saved PR binding changed; reconcile the PR and refresh the thread action") + } + if args.Head == "" || args.Head != asString(identityObject(pr), "head_sha") || args.Head != asString(observation, "observed_head_sha") { + return nil, fmt.Errorf("saved PR head changed; reconcile the PR and refresh the thread action") + } + selected, err := threadSelection(observation, args) + if err != nil { + return nil, err + } + gh, err := toolpath.Find(args.RuntimeRoot, "gh") + if err != nil { + return nil, err + } + freshPR, err := viewPR(gh, asString(task, "repository"), remote, number) + if err != nil { + return nil, err + } + if fmt.Sprint(freshPR["url"]) != url || fmt.Sprint(freshPR["headRefOid"]) != args.Head { + return nil, fmt.Errorf("GitHub PR binding or head changed; reconcile the PR before acting") + } + fresh := observeReviewThreads(gh, asString(task, "repository"), remote, number, freshPR) + if asString(fresh, "status") != "complete" { + return nil, fmt.Errorf("GitHub review threads could not be revalidated completely; refresh the observation before acting") + } + if err := validateLiveSelection(fresh, selected, args.All); err != nil { + return nil, err + } + if args.Action == "open-comment" { + thread := findThread(fresh, selected[0].ID) + result := ordjson.NewObject() + result.Set("task", args.Task) + result.Set("url", asString(thread, "source_url")) + result.Set("thread", selected[0].ID) + result.Set("revision", selected[0].Revision) + result.Set("note", "Open the saved GitHub comment URL. No reply or resolution was made.") + return result, nil + } + requestKey, requestID := threadRequestIdentity(args, selected, url) + request := threadRequest(args, selected, url, requestKey, requestID) + unlock, err := s.Lock() + if err != nil { + return nil, err + } + current, err := s.ReadTask(args.Task) + if err != nil { + unlock() + return nil, err + } + _, currentObservation, _, _, currentURL, err := threadActionBinding(current) + if err != nil || currentURL != url || asString(currentObservation, "observed_head_sha") != args.Head || !sameThreadSelection(currentObservation, selected) { + unlock() + return nil, fmt.Errorf("saved PR or thread revisions changed while the action was being checked; reconcile and retry") + } + actions := asList(identityValue(current, "github_review_actions")) + for _, raw := range actions { + existing := asObject(raw) + if asString(existing, "key") == requestKey { + unlock() + result := ordjson.NewObject() + result.Set("task", args.Task) + result.Set("request", existing) + result.Set("duplicate", true) + return result, nil + } + if args.Action == "address" && addressRequestBlocksRetry(existing, selected) { + unlock() + return nil, fmt.Errorf("a prior address request for this thread revision is %s; inspect its saved delivery state before creating another request", asString(existing, "status")) + } + } + current.Set("github_review_actions", append(actions, request)) + if err := s.SaveTask(current); err != nil { + unlock() + return nil, err + } + unlock() + if args.Action != "address" { + result := ordjson.NewObject() + result.Set("task", args.Task) + result.Set("request", request) + result.Set("duplicate", false) + result.Set("note", "This local discussion or draft does not post to or resolve the GitHub thread.") + return result, nil + } + _, err = repair.Send(s, ctx, repair.SendArgs{TaskID: args.Task, Attempt: args.Attempt, Key: requestKey, Text: args.Text, Class: args.Class, Reason: args.Reason, RuntimeRoot: args.RuntimeRoot}) + if err != nil { + operationID, operationState := findRepairOperation(s, args.Task, requestKey) + if operationID == "" { + operationState = "refused" + } + _ = updateThreadRequest(s, args.Task, requestID, operationState, operationID, err.Error()) + return nil, err + } + operationID, operationState := findRepairOperation(s, args.Task, requestKey) + if err := updateThreadRequest(s, args.Task, requestID, operationState, operationID, ""); err != nil { + return nil, err + } + saved, _ := s.ReadTask(args.Task) + result := ordjson.NewObject() + result.Set("task", args.Task) + result.Set("request", findThreadRequest(saved, requestID)) + result.Set("duplicate", false) + result.Set("note", "Repair delivery and GitHub thread resolution are separate; this action did not post a reply or resolve the thread.") + return result, nil +} + +func validateAddressScope(args *ThreadActionArgs) error { + if args.Action != "address" { + return nil + } + if args.Attempt == "" { + return fmt.Errorf("address requires the exact worker --attempt") + } + if args.Class == "" { + args.Class = repair.ClassInScope + } + if args.Class != repair.ClassInScope && args.Class != repair.ClassExpansion { + return fmt.Errorf("--class must be %s or %s", repair.ClassInScope, repair.ClassExpansion) + } + if args.Class == repair.ClassExpansion && strings.TrimSpace(args.Reason) == "" { + return fmt.Errorf("expansion address requires --reason") + } + return nil +} + +func threadActionBinding(task *ordjson.Object) (*ordjson.Object, *ordjson.Object, string, int, string, error) { + pr := asObject(identityValue(task, "pr")) + identity := asObject(identityValue(pr, "identity")) + observation := asObject(identityValue(pr, "review_threads")) + if identity == nil || observation == nil { + return nil, nil, "", 0, "", fmt.Errorf("task has no saved GitHub review thread observation") + } + remote := asString(observation, "repository") + number := intOf(identityValue(identity, "number")) + url := asString(identity, "url") + if remote == "" || number < 1 || url == "" { + return nil, nil, "", 0, "", fmt.Errorf("saved PR identity is incomplete") + } + return pr, observation, remote, number, url, nil +} + +func identityObject(pr *ordjson.Object) *ordjson.Object { + return asObject(identityValue(pr, "identity")) +} + +func threadSelection(observation *ordjson.Object, args ThreadActionArgs) ([]ThreadRevision, error) { + if args.All { + if asString(observation, "status") != "complete" { + return nil, fmt.Errorf("Address all requires a complete saved observation; reconcile the PR first") + } + selection := []ThreadRevision{} + for _, raw := range asList(identityValue(observation, "threads")) { + thread := asObject(raw) + if !objectBool(thread, "resolved") && asString(thread, "latest_comment_id") != "" { + selection = append(selection, ThreadRevision{ID: asString(thread, "id"), Revision: asString(thread, "latest_comment_id")}) + } + } + if len(selection) == 0 { + return nil, fmt.Errorf("there are no actionable GitHub review threads") + } + return normalizeThreadSelection(selection) + } + selection, err := normalizeThreadSelection(args.Threads) + if err != nil { + return nil, err + } + for _, selected := range selection { + thread := findThread(observation, selected.ID) + if thread == nil || objectBool(thread, "resolved") || asString(thread, "latest_comment_id") != selected.Revision { + return nil, fmt.Errorf("selected thread %s is stale or no longer actionable; reconcile the PR before acting", selected.ID) + } + } + return selection, nil +} + +func normalizeThreadSelection(selection []ThreadRevision) ([]ThreadRevision, error) { + if len(selection) == 0 { + return nil, fmt.Errorf("thread selection is empty") + } + if len(selection) > reviewThreadPageSize*reviewThreadPageLimit { + return nil, fmt.Errorf("thread selection exceeds the bounded %d-thread observation limit", reviewThreadPageSize*reviewThreadPageLimit) + } + out := append([]ThreadRevision(nil), selection...) + for _, row := range out { + if row.ID == "" || row.Revision == "" || len(row.ID) > 512 || len(row.Revision) > 512 { + return nil, fmt.Errorf("each thread selection needs an exact thread ID and current comment revision") + } + } + sort.Slice(out, func(i, j int) bool { return out[i].ID < out[j].ID }) + for i := 1; i < len(out); i++ { + if out[i-1].ID == out[i].ID { + return nil, fmt.Errorf("thread %s appears more than once", out[i].ID) + } + } + return out, nil +} + +func validateLiveSelection(fresh *ordjson.Object, selected []ThreadRevision, all bool) error { + if all { + current := []ThreadRevision{} + for _, raw := range asList(identityValue(fresh, "threads")) { + thread := asObject(raw) + if !objectBool(thread, "resolved") && asString(thread, "latest_comment_id") != "" { + current = append(current, ThreadRevision{ID: asString(thread, "id"), Revision: asString(thread, "latest_comment_id")}) + } + } + current, _ = normalizeThreadSelection(current) + if !equalThreadSelection(current, selected) { + return fmt.Errorf("the actionable thread set changed; reconcile and review the current inbox rows before using Address all") + } + return nil + } + for _, row := range selected { + thread := findThread(fresh, row.ID) + if thread == nil || objectBool(thread, "resolved") || asString(thread, "latest_comment_id") != row.Revision { + return fmt.Errorf("thread %s changed or resolved; reconcile and refresh the inbox row", row.ID) + } + } + return nil +} + +func sameThreadSelection(observation *ordjson.Object, selected []ThreadRevision) bool { + for _, row := range selected { + thread := findThread(observation, row.ID) + if thread == nil || objectBool(thread, "resolved") || asString(thread, "latest_comment_id") != row.Revision { + return false + } + } + return true +} + +func equalThreadSelection(a, b []ThreadRevision) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +func findThread(observation *ordjson.Object, id string) *ordjson.Object { + for _, raw := range asList(identityValue(observation, "threads")) { + thread := asObject(raw) + if asString(thread, "id") == id { + return thread + } + } + return nil +} + +func threadRequestIdentity(args ThreadActionArgs, selected []ThreadRevision, url string) (string, string) { + canonical := append([]ThreadRevision(nil), selected...) + sort.Slice(canonical, func(i, j int) bool { return canonical[i].ID < canonical[j].ID }) + identity := struct { + Task string `json:"task"` + Action string `json:"action"` + Head string `json:"head"` + PR string `json:"pr"` + Threads []ThreadRevision `json:"threads"` + Text string `json:"text"` + Attempt string `json:"attempt"` + Class string `json:"class"` + Reason string `json:"reason"` + }{args.Task, args.Action, args.Head, url, canonical, args.Text, args.Attempt, args.Class, args.Reason} + data, _ := json.Marshal(identity) + hash := sha256.Sum256(data) + suffix := hex.EncodeToString(hash[:])[:20] + return "github-review-" + suffix, "ghra-" + suffix +} + +func threadRequest(args ThreadActionArgs, selected []ThreadRevision, url, key, id string) *ordjson.Object { + rows := make([]any, 0, len(selected)) + for _, selectedThread := range selected { + row := ordjson.NewObject() + row.Set("id", selectedThread.ID) + row.Set("revision", selectedThread.Revision) + rows = append(rows, row) + } + request := ordjson.NewObject() + request.Set("id", id) + request.Set("key", key) + request.Set("action", args.Action) + request.Set("head_sha", args.Head) + request.Set("pr_url", url) + request.Set("threads", rows) + request.Set("all", args.All) + request.Set("text", args.Text) + request.Set("status", "recorded") + request.Set("created_at", store.Now()) + if args.Action == "address" { + request.Set("status", "in-flight") + request.Set("repair_key", key) + request.Set("attempt", args.Attempt) + request.Set("class", args.Class) + if args.Reason != "" { + request.Set("reason", args.Reason) + } + } + return request +} + +func updateThreadRequest(s *store.Store, taskID, requestID, state, operationID, detail string) error { + unlock, err := s.Lock() + if err != nil { + return err + } + defer unlock() + task, err := s.ReadTask(taskID) + if err != nil { + return err + } + for _, raw := range asList(identityValue(task, "github_review_actions")) { + request := asObject(raw) + if asString(request, "id") == requestID { + request.Set("status", state) + request.Set("updated_at", store.Now()) + if operationID != "" { + request.Set("repair_operation_id", operationID) + } + if detail != "" { + request.Set("detail", detail) + } + return s.SaveTask(task) + } + } + return fmt.Errorf("saved review action request %s disappeared", requestID) +} + +func findRepairOperation(s *store.Store, taskID, key string) (string, string) { + task, err := s.ReadTask(taskID) + if err != nil { + return "", "uncertain" + } + ledger, err := repair.Ledger(task) + if err != nil { + return "", "uncertain" + } + for _, raw := range asList(identityValue(ledger, "operations")) { + operation := asObject(raw) + if asString(operation, "key") == key { + return asString(operation, "id"), asString(operation, "state") + } + } + return "", "uncertain" +} + +func findThreadRequest(task *ordjson.Object, id string) *ordjson.Object { + for _, raw := range asList(identityValue(task, "github_review_actions")) { + request := asObject(raw) + if asString(request, "id") == id { + return request + } + } + return nil +} + +func addressStateUncertain(status string) bool { + return status == "in-flight" || status == "pending-resume" || status == "submitted" || status == "uncertain" +} + +func addressRequestBlocksRetry(request *ordjson.Object, selected []ThreadRevision) bool { + return asString(request, "action") == "address" && addressStateUncertain(asString(request, "status")) && selectionOverlaps(asList(identityValue(request, "threads")), selected) +} + +func selectionOverlaps(existing []any, selected []ThreadRevision) bool { + for _, raw := range existing { + thread := asObject(raw) + for _, row := range selected { + if asString(thread, "id") == row.ID && asString(thread, "revision") == row.Revision { + return true + } + } + } + return false +} + +func OpenCommentURL(task *ordjson.Object, threadID string) string { + _, observation, _, _, _, err := threadActionBinding(task) + if err != nil { + return "" + } + return asString(findThread(observation, threadID), "source_url") +} diff --git a/go/internal/prcmd/threads.go b/go/internal/prcmd/threads.go index 05c39b7..b6619e6 100644 --- a/go/internal/prcmd/threads.go +++ b/go/internal/prcmd/threads.go @@ -69,9 +69,9 @@ func observeReviewThreads(gh, dir, remote string, number int, pr map[string]any) URL string `json:"url"` HeadRefOID string `json:"headRefOid"` ReviewDecision string `json:"reviewDecision"` - ReviewThreads struct { - Nodes []reviewThreadNode `json:"nodes"` - PageInfo struct { + ReviewThreads *struct { + Nodes *[]reviewThreadNode `json:"nodes"` + PageInfo *struct { HasNextPage bool `json:"hasNextPage"` EndCursor string `json:"endCursor"` } `json:"pageInfo"` @@ -106,8 +106,20 @@ func observeReviewThreads(gh, dir, remote string, number int, pr map[string]any) record.Set("threads", []any{}) return record } + if got.ReviewThreads == nil || got.ReviewThreads.Nodes == nil || got.ReviewThreads.PageInfo == nil { + record.Set("status", "incomplete") + record.Set("detail", "review thread response omitted its nodes or pagination metadata") + record.Set("threads", rows) + return record + } decision = got.ReviewDecision - for _, node := range got.ReviewThreads.Nodes { + for _, node := range *got.ReviewThreads.Nodes { + if !validReviewThreadNode(node) { + record.Set("status", "incomplete") + record.Set("detail", "review thread response contained a thread without stable comment identity") + record.Set("threads", rows) + return record + } thread := node.record() thread.Set("observed_at", observedAt) thread.Set("observed_head_sha", fmt.Sprint(pr["headRefOid"])) @@ -135,16 +147,16 @@ func observeReviewThreads(gh, dir, remote string, number int, pr map[string]any) } type reviewThreadNode struct { - ID string `json:"id"` - IsOutdated bool `json:"isOutdated"` - IsResolved bool `json:"isResolved"` - Path string `json:"path"` - Line *int `json:"line"` - OriginalLine *int `json:"originalLine"` - StartLine *int `json:"startLine"` - OriginalStartLine *int `json:"originalStartLine"` + ID string `json:"id"` + IsOutdated *bool `json:"isOutdated"` + IsResolved *bool `json:"isResolved"` + Path *string `json:"path"` + Line *int `json:"line"` + OriginalLine *int `json:"originalLine"` + StartLine *int `json:"startLine"` + OriginalStartLine *int `json:"originalStartLine"` Comments struct { - Nodes []struct { + Nodes *[]struct { ID string `json:"id"` Database int64 `json:"databaseId"` URL string `json:"url"` @@ -161,19 +173,31 @@ type reviewThreadNode struct { } `json:"comments"` } +func validReviewThreadNode(node reviewThreadNode) bool { + if node.ID == "" || node.IsOutdated == nil || node.IsResolved == nil || node.Path == nil || node.Comments.Nodes == nil { + return false + } + for _, comment := range *node.Comments.Nodes { + if comment.ID == "" { + return false + } + } + return true +} + func (n reviewThreadNode) record() *ordjson.Object { thread := ordjson.NewObject() thread.Set("id", n.ID) - thread.Set("path", n.Path) + thread.Set("path", *n.Path) thread.Set("line", n.Line) thread.Set("original_line", n.OriginalLine) thread.Set("start_line", n.StartLine) thread.Set("original_start_line", n.OriginalStartLine) - thread.Set("resolved", n.IsResolved) - thread.Set("outdated", n.IsOutdated) - sort.Slice(n.Comments.Nodes, func(i, j int) bool { return n.Comments.Nodes[i].UpdatedAt > n.Comments.Nodes[j].UpdatedAt }) - commentRows := make([]any, 0, len(n.Comments.Nodes)) - for _, c := range n.Comments.Nodes { + thread.Set("resolved", *n.IsResolved) + thread.Set("outdated", *n.IsOutdated) + sort.Slice(*n.Comments.Nodes, func(i, j int) bool { return (*n.Comments.Nodes)[i].UpdatedAt > (*n.Comments.Nodes)[j].UpdatedAt }) + commentRows := make([]any, 0, len(*n.Comments.Nodes)) + for _, c := range *n.Comments.Nodes { row := ordjson.NewObject() row.Set("id", c.ID) row.Set("database_id", c.Database) @@ -196,8 +220,8 @@ func (n reviewThreadNode) record() *ordjson.Object { commentRows = append(commentRows, row) } thread.Set("comments", commentRows) - if len(n.Comments.Nodes) > 0 { - latest := n.Comments.Nodes[0] + if len(*n.Comments.Nodes) > 0 { + latest := (*n.Comments.Nodes)[0] thread.Set("latest_comment_id", latest.ID) thread.Set("latest_updated_at", latest.UpdatedAt) thread.Set("source_url", latest.URL) diff --git a/go/internal/prcmd/threads_test.go b/go/internal/prcmd/threads_test.go deleted file mode 100644 index d4da242..0000000 --- a/go/internal/prcmd/threads_test.go +++ /dev/null @@ -1,185 +0,0 @@ -package prcmd - -import ( - "os" - "path/filepath" - "strings" - "testing" - - "github.com/douglasjarquin/sum/go/internal/ordjson" - "github.com/douglasjarquin/sum/go/internal/store" -) - -func quoteShell(value string) string { - return "'" + strings.ReplaceAll(value, "'", "'\\''") + "'" -} - -func TestObserveReviewThreadsPreservesIdentityAndLatestComment(t *testing.T) { - payload := `{"data":{"repository":{"pullRequest":{"id":"PR_kwDO","number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"HEAD","reviewDecision":"CHANGES_REQUESTED","reviewThreads":{"nodes":[{"id":"PRRT_1","isOutdated":false,"isResolved":false,"path":"go/main.go","line":12,"comments":{"nodes":[{"id":"PRRC_1","databaseId":31,"url":"https://github.com/o/r/pull/7#discussion_r31","body":"first","createdAt":"2026-10-16T00:00:00Z","updatedAt":"2026-10-16T00:00:00Z","author":{"login":"reviewer"},"replyTo":null},{"id":"PRRC_2","databaseId":32,"url":"https://github.com/o/r/pull/7#discussion_r32","body":"follow-up","createdAt":"2026-10-16T01:00:00Z","updatedAt":"2026-10-16T02:00:00Z","author":{"login":"author"},"replyTo":{"id":"PRRC_1"}}]}}],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}}` - gh := fakeGh(t, `printf '%s' `+quoteShell(payload)) - pr := map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"} - got := observeReviewThreads(gh, t.TempDir(), "o/r", 7, pr) - if asString(got, "status") != "complete" || asString(got, "review_decision") != "CHANGES_REQUESTED" { - t.Fatalf("observation = %v", got) - } - threads := asList(func() any { v, _ := got.Get("threads"); return v }()) - if len(threads) != 1 { - t.Fatalf("threads = %v", threads) - } - thread := asObject(threads[0]) - if asString(thread, "id") != "PRRT_1" || asString(thread, "source_url") != "https://github.com/o/r/pull/7#discussion_r32" || asString(thread, "latest_comment_id") != "PRRC_2" { - t.Fatalf("thread identity/latest = %v", thread) - } - comments := asList(func() any { v, _ := thread.Get("comments"); return v }()) - if len(comments) != 2 || asString(asObject(comments[1]), "author") != "reviewer" { - t.Fatalf("comments = %v", comments) - } -} - -func TestObserveReviewThreadsMarksNonAdvancingPaginationIncomplete(t *testing.T) { - payload := `{"data":{"repository":{"pullRequest":{"number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"HEAD","reviewDecision":"APPROVED","reviewThreads":{"nodes":[],"pageInfo":{"hasNextPage":true,"endCursor":"next"}}}}}}` - gh := fakeGh(t, `printf '%s' `+quoteShell(payload)) - old := ordjson.NewObject() - old.Set("id", "PRRT_OLD") - previous := ordjson.NewObject() - previous.Set("status", "complete") - previous.Set("threads", []any{old}) - got := observeReviewThreads(gh, t.TempDir(), "o/r", 7, map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"}) - retainObservedThreads(got, previous) - if asString(got, "status") != "incomplete" || !strings.Contains(asString(got, "detail"), "no advancing cursor") { - t.Fatalf("observation = %v", got) - } - rows := asList(func() any { v, _ := got.Get("threads"); return v }()) - if len(rows) != 1 || asString(asObject(rows[0]), "id") != "PRRT_OLD" { - t.Fatalf("retained rows = %v", rows) - } - retained := asList(func() any { v, _ := previous.Get("threads"); return v }()) - if len(retained) != 1 || asString(asObject(retained[0]), "id") != "PRRT_OLD" { - t.Fatalf("source record changed: %v", retained) - } -} - -func TestObserveReviewThreadsReadsEveryPage(t *testing.T) { - first := `{"data":{"repository":{"pullRequest":{"number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"HEAD","reviewDecision":"APPROVED","reviewThreads":{"nodes":[{"id":"PRRT_1","comments":{"nodes":[]}}],"pageInfo":{"hasNextPage":true,"endCursor":"cursor-1"}}}}}}` - last := `{"data":{"repository":{"pullRequest":{"number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"HEAD","reviewDecision":"APPROVED","reviewThreads":{"nodes":[{"id":"PRRT_2","comments":{"nodes":[]}}],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}}` - seen := filepath.Join(t.TempDir(), "seen") - gh := fakeGh(t, `if [ -f `+quoteShell(seen)+` ]; then printf '%s' `+quoteShell(last)+`; else touch `+quoteShell(seen)+`; printf '%s' `+quoteShell(first)+`; fi`) - got := observeReviewThreads(gh, t.TempDir(), "o/r", 7, map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"}) - rows := asList(func() any { v, _ := got.Get("threads"); return v }()) - if asString(got, "status") != "complete" || len(rows) != 2 || asString(asObject(rows[1]), "id") != "PRRT_2" { - t.Fatalf("observation = %v", got) - } - if _, err := os.Stat(seen); err != nil { - t.Fatalf("second page was not requested: %v", err) - } -} - -func TestObserveReviewThreadsRejectsWrongHeadAndRecordsReadFailure(t *testing.T) { - payload := `{"data":{"repository":{"pullRequest":{"number":7,"url":"https://github.com/o/r/pull/7","headRefOid":"OTHER","reviewDecision":"APPROVED","reviewThreads":{"nodes":[],"pageInfo":{"hasNextPage":false,"endCursor":null}}}}}}` - gh := fakeGh(t, `printf '%s' `+quoteShell(payload)) - wrongHead := observeReviewThreads(gh, t.TempDir(), "o/r", 7, map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"}) - if asString(wrongHead, "status") != "mismatched" || len(asList(func() any { v, _ := wrongHead.Get("threads"); return v }())) != 0 { - t.Fatalf("wrong-head observation = %v", wrongHead) - } - failing := fakeGh(t, `echo 'GraphQL: permission denied' >&2; exit 1`) - failed := observeReviewThreads(failing, t.TempDir(), "o/r", 7, map[string]any{"url": "https://github.com/o/r/pull/7", "headRefOid": "HEAD"}) - if asString(failed, "status") != "incomplete" || !strings.Contains(asString(failed, "detail"), "permission denied") { - t.Fatalf("failed observation = %v", failed) - } -} - -func TestRetainObservedThreadsOnIncompletePage(t *testing.T) { - old := ordjson.NewObject() - old.Set("id", "PRRT_OLD") - currentRow := ordjson.NewObject() - currentRow.Set("id", "PRRT_NEW") - previous := ordjson.NewObject() - previous.Set("review_decision", "CHANGES_REQUESTED") - previous.Set("threads", []any{old}) - current := ordjson.NewObject() - current.Set("status", "incomplete") - current.Set("threads", []any{currentRow}) - retainObservedThreads(current, previous) - rows := asList(func() any { v, _ := current.Get("threads"); return v }()) - if len(rows) != 2 || asString(asObject(rows[1]), "id") != "PRRT_OLD" { - t.Fatalf("threads = %v", rows) - } - if value, _ := previous.Get("review_decision"); value != "CHANGES_REQUESTED" { - t.Fatalf("previous was mutated: %v", value) - } -} - -func TestRecordObservationUpdatesAndRetainsThreadRevisions(t *testing.T) { - head := strings.Repeat("a", 40) - st, err := store.Open(t.TempDir()) - if err != nil { - t.Fatal(err) - } - if err := st.Init(); err != nil { - t.Fatal(err) - } - taskValue, err := ordjson.Decode([]byte(observationTask(head, ""))) - if err != nil { - t.Fatal(err) - } - if err := st.SaveTask(taskValue.(*ordjson.Object)); err != nil { - t.Fatal(err) - } - ctx := ordjson.NewObject() - ctx.Set("machine", "m-test") - ctx.Set("session", "sum-test") - ctx.Set("pane", "w-parent:p1") - base, err := ordjson.Decode([]byte(ghView("OPEN", head, ""))) - if err != nil { - t.Fatal(err) - } - data := map[string]any{} - for _, key := range base.(*ordjson.Object).Keys() { - value, _ := base.(*ordjson.Object).Get(key) - data[key] = value - } - thread := ordjson.NewObject() - thread.Set("id", "PRRT_1") - thread.Set("resolved", true) - thread.Set("latest_comment_id", "PRRC_1") - complete := ordjson.NewObject() - complete.Set("status", "complete") - complete.Set("review_decision", "CHANGES_REQUESTED") - complete.Set("threads", []any{thread}) - data["review_threads"] = complete - for range 2 { - if _, _, err := recordObservationWith(st, ctx, "t-aaaaaaaaaaaa", data, false); err != nil { - t.Fatal(err) - } - } - thread.Set("resolved", false) - thread.Set("latest_comment_id", "PRRC_2") - if _, _, err := recordObservationWith(st, ctx, "t-aaaaaaaaaaaa", data, false); err != nil { - t.Fatal(err) - } - newThread := ordjson.NewObject() - newThread.Set("id", "PRRT_2") - incomplete := ordjson.NewObject() - incomplete.Set("status", "incomplete") - incomplete.Set("threads", []any{newThread}) - data["review_threads"] = incomplete - if _, _, err := recordObservationWith(st, ctx, "t-aaaaaaaaaaaa", data, false); err != nil { - t.Fatal(err) - } - saved, err := st.ReadTask("t-aaaaaaaaaaaa") - if err != nil { - t.Fatal(err) - } - prValue, _ := saved.Get("pr") - pr := asObject(prValue) - reviewValue, _ := pr.Get("review_threads") - review := asObject(reviewValue) - rows := asList(func() any { v, _ := review.Get("threads"); return v }()) - if len(rows) != 2 || asString(asObject(rows[0]), "id") != "PRRT_2" || asString(asObject(rows[1]), "id") != "PRRT_1" { - t.Fatalf("saved thread rows = %v", rows) - } - previous := asObject(rows[1]) - if asString(previous, "latest_comment_id") != "PRRC_2" || asString(review, "review_decision") != "CHANGES_REQUESTED" || asString(review, "status") != "incomplete" { - t.Fatalf("saved review observation = %v", review) - } -} diff --git a/go/internal/presentation/presentation.go b/go/internal/presentation/presentation.go index 175931e..3d8092a 100644 --- a/go/internal/presentation/presentation.go +++ b/go/internal/presentation/presentation.go @@ -20,6 +20,7 @@ const ( Factory SourceKind = "factory" Finding SourceKind = "finding" Gap SourceKind = "gap" + GitHubReviewThread SourceKind = "github-review-thread" ) type Owner string @@ -135,20 +136,47 @@ func FindingActions(source Source, state string) []Action { return actions } +func GitHubThreadActions(source Source, head, prURL, sourceURL string) []Action { + if source.TaskID == "" || source.ID == "" || source.Revision == "" || head == "" || prURL == "" { + return nil + } + base := []string{"pr", "thread", "action", source.TaskID, "--thread", source.ID, "--revision", source.Revision, "--head", head, "--pr-url", prURL} + actions := []Action{ + {Target: source, Kind: "address-selected", Label: "Address selected", State: "open", Arguments: append(append([]string{}, base...), "--action", "address", "--attempt", "", "--text")}, + {Target: source, Kind: "address-all", Label: "Address all", State: "open", Arguments: []string{"pr", "thread", "action", source.TaskID, "--action", "address", "--head", head, "--pr-url", prURL, "--all", "--attempt", "", "--text"}}, + {Target: source, Kind: "discuss", Label: "Discuss", State: "open", Arguments: append(append([]string{}, base...), "--action", "discuss", "--text")}, + {Target: source, Kind: "draft-reply", Label: "Draft reply", State: "open", Arguments: append(append([]string{}, base...), "--action", "draft-reply", "--text")}, + } + if sourceURL != "" { + actions = append(actions, Action{Target: source, Kind: "open-comment", Label: "Open comment", State: "open", Arguments: append(append([]string{}, base...), "--action", "open-comment")}) + } + return actions +} + type Fact struct { - Source Source `json:"source"` - ProjectID string `json:"project,omitempty"` - State string `json:"state,omitempty"` - At string `json:"at,omitempty"` - Text string `json:"text,omitempty"` - Delivery string `json:"delivery,omitempty"` - Details []Detail `json:"details"` - Choices []Choice `json:"choices,omitempty"` - Recommended string `json:"recommended,omitempty"` - Rationale string `json:"rationale,omitempty"` - Sources []string `json:"sources,omitempty"` - HumanAction *HumanAction `json:"human_action,omitempty"` - Actions []Action `json:"actions,omitempty"` + Source Source `json:"source"` + ProjectID string `json:"project,omitempty"` + State string `json:"state,omitempty"` + At string `json:"at,omitempty"` + Text string `json:"text,omitempty"` + Delivery string `json:"delivery,omitempty"` + Details []Detail `json:"details"` + Choices []Choice `json:"choices,omitempty"` + Recommended string `json:"recommended,omitempty"` + Rationale string `json:"rationale,omitempty"` + Sources []string `json:"sources,omitempty"` + HumanAction *HumanAction `json:"human_action,omitempty"` + Actions []Action `json:"actions,omitempty"` + SourceURL string `json:"source_url,omitempty"` + Author string `json:"author,omitempty"` + File string `json:"file,omitempty"` + Line *int `json:"line,omitempty"` + Resolved bool `json:"resolved,omitempty"` + Outdated bool `json:"outdated,omitempty"` + ObservedAt string `json:"observed_at,omitempty"` + RequestID string `json:"request_id,omitempty"` + RequestStatus string `json:"request_status,omitempty"` + Uncertain bool `json:"uncertain,omitempty"` } type Item struct { @@ -162,9 +190,13 @@ type Item struct { // Classify never treats a delivery receipt or worker prose as domain resolution. func Classify(f Fact) Item { - identity, _ := json.Marshal(f.Source) + identitySource := f.Source + if identitySource.Kind == GitHubReviewThread { + identitySource.Candidate = "" + } + identity, _ := json.Marshal(identitySource) item := Item{Fact: f, Identity: string(identity), Owner: Coordinator, Kind: Inspection, Reason: "unknown-source-kind"} - item.Uncertain = f.Delivery == "submitted" || f.Delivery == "uncertain" || f.Delivery == "in-flight" || f.Delivery == "unknown" + item.Uncertain = f.Uncertain || f.Delivery == "submitted" || f.Delivery == "uncertain" || f.Delivery == "in-flight" || f.Delivery == "unknown" switch f.Source.Kind { case Question: switch f.State { @@ -187,6 +219,12 @@ func Classify(f Fact) Item { item.Kind, item.Reason = Routine, "report-awaiting-action" case Review: item.Kind, item.Reason = Routine, "review-awaiting-action" + case GitHubReviewThread: + if f.Resolved { + item.Owner, item.Kind, item.Reason = Nobody, Resolved, "github-thread-resolved" + } else { + item.Owner, item.Kind, item.Reason = Human, Decision, "github-thread-open" + } case Refresh: item.Owner, item.Kind, item.Reason = Worker, Routine, RefreshUnapplied case Attention: diff --git a/go/internal/statuscmd/compact.go b/go/internal/statuscmd/compact.go index 8d3699b..6519b81 100644 --- a/go/internal/statuscmd/compact.go +++ b/go/internal/statuscmd/compact.go @@ -64,6 +64,17 @@ func Compact(s *store.Store, opts Options) (*ordjson.Object, error) { row.Set("delivery", item.Delivery) } row.Set("uncertain", item.Uncertain) + if item.Source.Kind == presentation.GitHubReviewThread { + row.Set("source_url", item.SourceURL) + row.Set("author", item.Author) + row.Set("file", item.File) + row.Set("line", item.Line) + row.Set("resolved", item.Resolved) + row.Set("outdated", item.Outdated) + row.Set("observed_at", item.ObservedAt) + row.Set("request_id", item.RequestID) + row.Set("request_status", item.RequestStatus) + } if item.Text != "" { text := environment.BoundedView(item.Text, opts.MaxChars) if truncated, _ := text.Get("truncated"); truncated == true { diff --git a/go/internal/statuscmd/statuscmd.go b/go/internal/statuscmd/statuscmd.go index 2bfc7d8..532c27a 100644 --- a/go/internal/statuscmd/statuscmd.go +++ b/go/internal/statuscmd/statuscmd.go @@ -60,7 +60,8 @@ func Status(s *store.Store, opts Options) (*ordjson.Object, error) { attentionRecords, _ := row.Get("attention_records") attentionList, _ := attentionRecords.([]any) - include := !opts.Inbox || len(questionList) > 0 || errorValue != nil || hasAttention || reportAvailable == true || cleanupValue != nil || len(attentionList) > 0 + reviewThreads := hasOpenReviewThreads(task) + include := !opts.Inbox || len(questionList) > 0 || errorValue != nil || hasAttention || reportAvailable == true || cleanupValue != nil || len(attentionList) > 0 || reviewThreads if !include { continue } @@ -235,6 +236,9 @@ func buildRow(s *store.Store, task *ordjson.Object) *ordjson.Object { evidence.Set("pr", prNumber) evidence.Set("merged_for_task", mergedForTask) row.Set("evidence", evidence) + if saved := savedReviewThreads(task); saved != nil { + row.Set("review_threads", saved) + } row.Set("notice", nil) // holds the key's place; Status sets the derived view for each row it keeps @@ -278,3 +282,119 @@ func buildRow(s *store.Store, task *ordjson.Object) *ordjson.Object { return row } + +func hasOpenReviewThreads(task *ordjson.Object) bool { + pr, _ := task.Get("pr") + prRecord, _ := pr.(*ordjson.Object) + observationValue, _ := func() (any, bool) { + if prRecord == nil { + return nil, false + } + return prRecord.Get("review_threads") + }() + observation, _ := observationValue.(*ordjson.Object) + threadsValue, _ := func() (any, bool) { + if observation == nil { + return nil, false + } + return observation.Get("threads") + }() + threads, _ := threadsValue.([]any) + for _, raw := range threads { + thread, _ := raw.(*ordjson.Object) + if thread != nil { + resolved, _ := thread.Get("resolved") + if resolved != true { + return true + } + } + } + return false +} + +func savedReviewThreads(task *ordjson.Object) *ordjson.Object { + prValue, _ := task.Get("pr") + pr, _ := prValue.(*ordjson.Object) + identity, _ := func() (any, bool) { + if pr == nil { + return nil, false + } + return pr.Get("identity") + }() + prIdentity, _ := identity.(*ordjson.Object) + observationValue, _ := func() (any, bool) { + if pr == nil { + return nil, false + } + return pr.Get("review_threads") + }() + observation, _ := observationValue.(*ordjson.Object) + if observation == nil { + return nil + } + result := ordjson.NewObject() + for _, key := range []string{"status", "repository", "pr_number", "observed_at", "observed_head_sha", "review_decision", "detail"} { + value, _ := observation.Get(key) + result.Set(key, value) + } + result.Set("pr_url", value(prIdentity, "url")) + rows := []any{} + threadsValue, _ := observation.Get("threads") + threads, _ := threadsValue.([]any) + for _, raw := range threads { + thread, _ := raw.(*ordjson.Object) + if thread == nil { + continue + } + row := ordjson.NewObject() + for _, key := range []string{"id", "path", "line", "resolved", "outdated", "latest_comment_id", "latest_updated_at", "latest_author", "source_url", "observed_at", "observed_head_sha", "actions", "request_id"} { + value, _ := thread.Get(key) + row.Set(key, value) + } + requestID, requestStatus := latestReviewThreadRequest(task, fmt.Sprint(value(thread, "id")), fmt.Sprint(value(thread, "latest_comment_id")), fmt.Sprint(value(thread, "observed_head_sha")), fmt.Sprint(value(prIdentity, "url"))) + row.Set("request_id", requestID) + row.Set("request_status", requestStatus) + commentsValue, _ := thread.Get("comments") + comments, _ := commentsValue.([]any) + if len(comments) > 0 { + comment, _ := comments[0].(*ordjson.Object) + if comment != nil { + for _, key := range []string{"id", "body", "author", "url", "updated_at"} { + value, _ := comment.Get(key) + row.Set("latest_"+key, value) + } + } + } + rows = append(rows, row) + } + result.Set("threads", rows) + return result +} + +func latestReviewThreadRequest(task *ordjson.Object, threadID, revision, head, prURL string) (string, string) { + requestsValue, _ := task.Get("github_review_actions") + requests, _ := requestsValue.([]any) + for i := len(requests) - 1; i >= 0; i-- { + request, _ := requests[i].(*ordjson.Object) + if request == nil || value(request, "head_sha") != head || value(request, "pr_url") != prURL { + continue + } + threadsValue, _ := request.Get("threads") + threads, _ := threadsValue.([]any) + for _, raw := range threads { + thread, _ := raw.(*ordjson.Object) + if thread != nil && value(thread, "id") == threadID && value(thread, "revision") == revision { + return fmt.Sprint(value(request, "id")), fmt.Sprint(value(request, "status")) + } + } + } + return "", "" +} + +func value(record *ordjson.Object, key string) any { + if record == nil { + return nil + } + value, _ := record.Get(key) + return value +} From 88350342cca542f1646d2f92f971b01993bf6185 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sun, 18 Oct 2026 02:52:35 +0000 Subject: [PATCH 04/10] chore: restore tests to origin main --- go/internal/cli/quiet_coordination_replay_test.go | 2 +- go/internal/prcmd/readonly_test.go | 2 +- tests/fixtures/gh_attach.py | 15 --------------- 3 files changed, 2 insertions(+), 17 deletions(-) diff --git a/go/internal/cli/quiet_coordination_replay_test.go b/go/internal/cli/quiet_coordination_replay_test.go index 8eb6255..65cb638 100644 --- a/go/internal/cli/quiet_coordination_replay_test.go +++ b/go/internal/cli/quiet_coordination_replay_test.go @@ -424,7 +424,7 @@ func TestQuietCoordinationReplay(t *testing.T) { verbs := lab.ghVerbs(t) for verb := range verbs { switch verb { - case "issue list", "issue edit", "issue comment", "repo view", "pr list", "pr create", "pr view", "api graphql", "pr checks", "pr ready", "auth status", "--version", "pr edit --help": + case "issue list", "issue edit", "issue comment", "repo view", "pr list", "pr create", "pr view", "pr checks", "pr ready", "auth status", "--version", "pr edit --help": default: t.Fatalf("fake gh saw an unexpected verb %q: %v", verb, verbs) } diff --git a/go/internal/prcmd/readonly_test.go b/go/internal/prcmd/readonly_test.go index fe563af..3d2f4bf 100644 --- a/go/internal/prcmd/readonly_test.go +++ b/go/internal/prcmd/readonly_test.go @@ -308,7 +308,7 @@ func TestRetryBoundaryIsExactlyTheTwoReadOnlyObservations(t *testing.T) { got = append(got, name) } sort.Strings(got) - want := []string{"observeReviewThreads", "observedVisibility", "viewPR"} + want := []string{"observedVisibility", "viewPR"} if strings.Join(got, ",") != strings.Join(want, ",") { t.Fatalf("readOnlyGH callers = %v, want %v", got, want) } diff --git a/tests/fixtures/gh_attach.py b/tests/fixtures/gh_attach.py index 9335a0f..0e4c662 100755 --- a/tests/fixtures/gh_attach.py +++ b/tests/fixtures/gh_attach.py @@ -140,21 +140,6 @@ def fail(message, code=1): full = {"nameWithOwner": repo, "visibility": state.get("visibility", "PUBLIC"), "viewerPermission": state.get("viewer_permission", "WRITE")} print(json.dumps({k: full.get(k) for k in fields.split(",")})) sys.exit(0) -if args[:2] == ["api", "graphql"]: - owner = args[args.index("-F") + 1].partition("=")[2] if "-F" in args else "" - name_arg = next((args[i + 1].partition("=")[2] for i, arg in enumerate(args[:-1]) if arg == "-F" and args[i + 1].startswith("name=")), "") - number_arg = next((args[i + 1].partition("=")[2] for i, arg in enumerate(args[:-1]) if arg == "-F" and args[i + 1].startswith("number=")), "0") - repo = f"{owner}/{name_arg}" - number = int(number_arg) - found = find_pr(number) - if found is None or repo != state.get("repository", "douglasjarquin/project"): - fail("GraphQL: Could not resolve to a PullRequest with the number of %d. (repository.pullRequest)" % number) - review_threads = state.get("review_threads", {"nodes": [], "pageInfo": {"hasNextPage": False, "endCursor": None}}) - pull_request = {"id": found.get("id", "PR_fake"), "number": number, - "url": f"https://github.com/{repo}/pull/{number}", "headRefOid": found.get("head_sha"), - "reviewDecision": state.get("review_decision"), "reviewThreads": review_threads} - print(json.dumps({"data": {"repository": {"pullRequest": pull_request}}})) - sys.exit(0) if args[:2] == ["pr", "checks"]: if "--json" not in args or not state.get("checks_json", True): fail("unknown flag: --json\nUsage: gh pr checks [ | | ] [flags]", 1) From 5bdbabf8fb275b4d6547e6352b88c172fbb9d76e Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sun, 18 Oct 2026 02:53:23 +0000 Subject: [PATCH 05/10] docs: report review thread coverage accurately --- docs/features/pipeline.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/features/pipeline.md b/docs/features/pipeline.md index 6d107ce..a10821c 100644 --- a/docs/features/pipeline.md +++ b/docs/features/pipeline.md @@ -69,7 +69,7 @@ The table is the coordinator's record of what has run. It is not verification, r | `pipeline.pr-fresh-base` | `pipeline pr` fetches the base again before creating or adopting a PR and refuses to create one against a base that moved since the push, creating nothing, until `--allow-behind`; `pipeline run --allow-behind` passes the flag to both Push and PR (`go/internal/pipelinerun/pipelinerun.go`) | automated: `go/internal/cli/pipeline_freshbase_test.go` | offline suite | | `pipeline.pr-mergeability` | `pr reconcile` and `pipeline ci` record GitHub's `mergeable` and `mergeStateStatus`; an open PR reported `CONFLICTING`/`DIRTY` blocks the PR row with the merge-the-base remedy and shows under `maintenance.open_prs` with `blocked` and `fix`, and a later `MERGEABLE` observation passes the row again | automated: `go/internal/cli/pipeline_freshbase_test.go` | offline suite | | `pipeline.pr-read-retry` | Read-only `gh` observations in `go/internal/prcmd` make at most one second attempt, and only after a conclusively completed nonzero exit whose stderr positively names a transient condition: an `HTTP 5xx` status line, `HTTP 429`, gh's secondary-rate-limit sentence, or a transport failure as gh prints it (`error connecting to`, `connection refused`, tls or x509 handshake errors). The `gh pr view` and `gh api graphql` PR reads (`pr reconcile`, `sweep`) share one bound per observation; `gh repo view --json visibility` (evidence publication) retains its existing bound. Attempt one runs on the runner's own timer unchanged, attempt two receives only the time left, and a delay that would not leave a floor for a second attempt defers instead of overrunning. A runner error (timeout, cancel, oversized or incomplete output, uncertain descendant), an auth failure, 404, any other 4xx including a bare 403, a `GraphQL:` application error, malformed JSON, or ambiguous stderr is never retried; no write (agent launch, PR create, edit, ready, publish, push, merge) and no other `gh` read gains a retry, and `go/internal/proc` keeps its no-retry contract. A failure after the retry is still an uncertain observation naming the attempt count and category with token shapes redacted and the detail capped; the prior `pr` record and `observed_at` stay as recorded and `sweep` stamps `observe_failed_at` as before | automated: `go/internal/prcmd/readonly_test.go`, `go/internal/prcmd/pr_test.go` | offline suite against counting fake `gh` scripts: transient-then-valid succeeds after exactly two calls, persistent 503 fails after exactly two, each non-transient shape makes one call, a 429 with a short bound defers after one call, a slow first attempt leaves no budget, the runner called directly runs once, and a Go source-parsing pass over the package pins the helper's callers to `observeReviewThreads`, `viewPR`, and `observedVisibility`. Implementation: `go/internal/prcmd/readonly.go`, `go/internal/prcmd/threads.go`. | -| `pipeline.review-threads` | An explicit `pr reconcile` records cursor-paginated GitHub review threads and the overall review decision only when the returned PR number, URL, and head match the identity observation; each thread retains its stable node ID, source URL, file and supplied lines, latest comment identity, author, update time, resolution/outdated state, and observed head/time. The read has a 20-page maximum of 50 threads per page and shares one `GHBound`; a failed, incomplete, nonadvancing, or mismatched response is marked uncertain and keeps previously observed threads with their original per-thread observation time/head. Reconciliation is the only GitHub read for these rows; later saved-state projections do not poll. | automated: `go/internal/prcmd/readonly_test.go` covers bounded read retries; thread observation behavior has no automated coverage in this candidate. | `readonly_test.go` does not exercise review-thread pagination or retention. No thread-observation test source is included in this candidate. | +| `pipeline.review-threads` | An explicit `pr reconcile` records cursor-paginated GitHub review threads and the overall review decision only when the returned PR number, URL, and head match the identity observation; each thread retains its stable node ID, source URL, file and supplied lines, latest comment identity, author, update time, resolution/outdated state, and observed head/time. The read has a 20-page maximum of 50 threads per page and shares one `GHBound`; a failed, incomplete, nonadvancing, or mismatched response is marked uncertain and keeps previously observed threads with their original per-thread observation time/head. Reconciliation is the only GitHub read for these rows; later saved-state projections do not poll. | no passing automated coverage for thread observation in this candidate. | `go test ./internal/prcmd` currently fails in the pre-existing `readonly_test.go` caller allowlist because it does not include `observeReviewThreads`. No thread-observation test source is included in this candidate. | | `pipeline.review-thread-actions` | The saved inbox and `context --section review_threads` expose source URL, author, file/line, freshness, state, and Address selected/all, Discuss, Draft reply, and Open comment actions. Actions revalidate PR URL/head and exact comment revisions. Address records one idempotent request with every included thread/revision and uses `repair.Send`; changed selections and incomplete Address-all snapshots are refused. Uncertain and submitted requests stay linked to their threads and cannot be resent. Discuss and Draft reply are local records; no action publishes a reply or resolves GitHub state. | no automated coverage in this candidate. | A CLI before/after transcript verifies saved review-thread rows in inbox and `inbox --live`; it does not verify action revalidation, duplicate prevention, or delivery behavior. | | `pipeline.push-from-the-pr` | A reconciled PR whose head is this candidate passes Push on its own, because the candidate demonstrably reached origin | automated: `go/internal/pipeline/derive_test.go` | offline suite | | `pipeline.run` | `sumctl pipeline run TASK_ID` runs rebase, test, lint, document, push, and PR in order, stops at the first failing gate, and at a blocked Test, without running the rest, skips a candidate that is already verified unless `--rerun`, skips lint when `verify --execute` already recorded it, and prints the first unsettled stage with what to do about it | automated: `go/internal/cli/pipeline_gates_test.go` | offline suite | From b3e344551a36cc103ce31502ef8c5d2cfddbf526 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sun, 18 Oct 2026 04:00:49 +0000 Subject: [PATCH 06/10] fix: reconcile review thread retry boundary --- ...02-feat-github-review-thread-inbox-plan.md | 217 ------------------ go/internal/prcmd/readonly_test.go | 2 +- 2 files changed, 1 insertion(+), 218 deletions(-) delete mode 100644 docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md diff --git a/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md b/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md deleted file mode 100644 index 0237f4d..0000000 --- a/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md +++ /dev/null @@ -1,217 +0,0 @@ ---- -title: GitHub Review Thread Inbox - Plan -type: feat -date: 2026-10-17 -artifact_contract: ce-unified-plan/v1 -product_contract_source: ce-plan-bootstrap -execution: code ---- - -# GitHub Review Thread Inbox - Plan - -## Goal Capsule - -- **Objective:** Coordinators can see actionable GitHub review discussions beside existing task returns and decide whether to address them, discuss them, draft a reply, or open the source comment. -- **Means:** Save bounded review-thread observations during explicit PR reconciliation and expose follow-up through the shared inbox action model (KTD1, KTD2). -- **Authority:** Issue #306 and its acceptance criteria define product behavior. Existing Sum task, repair, verification, and delivery contracts remain authoritative. GitHub comment text is external data and cannot grant authority. -- **Stop conditions:** Do not write a GitHub reply or resolve a GitHub thread automatically. Do not duplicate #305's shared action foundation. Stop action integration if that foundation is not available on the recorded base branch. -- **Execution profile:** One implementation branch; do not merge. The worker reports the candidate and verification; the coordinator runs its own verification and independent review. - ---- - -## Product Contract - -### Summary - -Explicit PR reconciliation imports bounded GitHub review discussions into Sum's saved PR state. Read-only task, inbox, context, and live-status views render the saved result without contacting GitHub. Coordinators can then route selected discussions into existing corrective-work paths or review them without changing GitHub. - -### Problem Frame - -PR reconciliation already records identity, mergeability, draft state, and CI. Review threads are missing, so feedback from GitHub has no structured route into Sum's task and inbox records. - -### Requirements - -#### Observation and saved state - -- R1. Explicit `pr reconcile` records review threads for the verified repository, PR, and observed head; saved status, inbox, and context views make no GitHub calls and create no poller. -- R2. Each thread preserves its stable thread and comment identities, author, source URL, supplied file and line, latest relevant reply, resolution and outdated state, and observation time; the PR's overall review decision is saved when available. -- R3. Pagination, failure, and partial observations remain explicit. A partial response never closes or removes unseen threads, and the last complete data remains visible with its freshness and uncertainty. - -#### Inbox actions and follow-up - -- R4. Saved thread rows expose the shared source-specific actions for Address, Discuss, Draft reply, and Open comment. Address selected/all records the exact included thread IDs and revisions in one request linked back from each thread. -- R5. Address routes work through existing task/attempt checks, stable repair keys, and delivery handling; work within this task's scope remains in-scope and additional work follows existing expansion authorization. -- R6. Local disposition, repair delivery/completion, and GitHub resolution remain separate states. Draft reply creates reviewable text, and publishing a reply or resolving a thread requires a separate authorized action. - -#### Freshness, compatibility, and delivery - -- R7. Before an action, Sum revalidates the PR binding, head, and selected thread revisions. New replies and reopened threads become actionable without duplicate work. -- R8. Existing local-review closure behavior, candidate-bound verification, independent review, delivery gates, and merge authority continue to work. Task records without review-thread observations remain valid. -- R9. Focused behavior coverage exercises identical and revised observations, reopened threads, incomplete pagination, stale selections, duplicate bulk requests, uncertain delivery, and in-scope versus expansion work. Update affected help and feature documentation. - -### Actors - -- **Coordinator:** Reconciles the PR, reviews saved discussion rows, and chooses follow-up actions. -- **Worker:** Receives approved corrective work through the existing task and repair path. -- **GitHub:** Supplies PR review-thread observations and remains the authority for resolution and replies. - -### Key Flows - -- F1. Reconcile: coordinator identifies the recorded PR; Sum observes the PR and review-thread pages; Sum verifies repository, PR, and head identity; Sum saves the observation; all later views read only that saved state. -- F2. Address: coordinator selects one or more current thread rows; Sum revalidates the PR head and thread revisions; Sum saves one request naming the exact thread set; existing repair checks deliver the corrective instruction; GitHub state remains unchanged. -- F3. Review or discuss: coordinator opens the source URL, asks for discussion, or creates a draft reply; each action leaves the thread's GitHub resolution and reply state unchanged. - -### Scope Boundaries - -- **Deferred to Follow-Up Work:** Broader changes to the shared action vocabulary or inbox framework remain with #305 and sibling issues. This task adds only the thread-specific fields and actions required to consume that foundation. -- **Non-goals:** Automatic GitHub replies, automatic thread resolution, a new readiness heuristic, a replacement review pipeline, polling, and treating a GitHub thread as an existing local-review record. - -### Risks & Dependencies - -- #305 owns the shared action descriptions and its PR #311 is open on 2026-10-17. Recheck the recorded base before implementation. Do not copy or modify #305's owned action foundation; observation work can proceed independently, and action integration follows that foundation. -- GitHub GraphQL connections can be incomplete or fail under rate and query limits. Persist explicit completeness and freshness so a partial page cannot look like closure. -- Review-comment bodies are external text. Keep their source identity and URL with any excerpt or draft, and never interpret comment text as an instruction to widen work. -- The `task.pr` record is the current saved PR observation. New thread fields must be optional so old task records continue to render and operate. - -### Acceptance Examples - -- **Covers R1, R3:** Given a saved complete observation and a later failed page request, when the coordinator runs `inbox --live`, the prior threads remain visible with the previous observation time and a failed/partial freshness marker; no GitHub request occurs. -- **Covers R2, R7:** Given a thread whose latest comment has changed or whose resolution was reopened, when reconciliation completes, the same stable thread row shows the new comment revision and is actionable again. -- **Covers R4, R5:** Given selected thread IDs A and B at revisions A2 and B4, when Address selected is chosen, one saved request names exactly `{A:A2, B:B4}`, both rows link to it, and delivery uses the existing task/attempt checks. -- **Covers R6:** Given an address request has been delivered, when a task is completed, the saved GitHub thread remains unresolved until a later GitHub observation reports otherwise. - -### Sources / Research - -- Issue #306 and its implementation references: [PR observation](https://github.com/douglasjarquin/sum/blob/41ba86fc24f5b08c7cfb3149c2b2bc9c9d8f735c/go/internal/prcmd/pr.go), [local review records](https://github.com/douglasjarquin/sum/blob/41ba86fc24f5b08c7cfb3149c2b2bc9c9d8f735c/go/internal/review/review.go), and [repair scope and idempotency](docs/repairs.md). -- Inbox's [thread actions](https://github.com/petekp/inbox/blob/9bc2983367827bb6b49a7d14cfdff1f48a7f0626/hooks/pr-tab.ts#L314-L435) and [source-preserving PR requests](https://github.com/petekp/inbox/blob/9bc2983367827bb6b49a7d14cfdff1f48a7f0626/hooks/prs.ts#L366-L423) supply interaction precedents, not Sum authority. -- GitHub's [PullRequest GraphQL fields](https://docs.github.com/en/graphql/reference/pulls) expose `reviewThreads` and `reviewDecision`; its [pagination guide](https://docs.github.com/en/graphql/guides/using-pagination-in-the-graphql-api) requires cursor traversal and page-info checks for complete connection reads. -- `docs/solutions/architecture-patterns/retiring-persisted-compatibility-fields.md` recommends deriving compatibility from the actual readers and keeping persisted additions optional for older records. - ---- - -## Planning Contract - -### Key Technical Decisions - -- KTD1. Use a bounded, read-only GitHub GraphQL observation for `reviewThreads` and `reviewDecision` alongside the existing `gh pr view` result. `prViewFields` does not contain thread fields, while GitHub exposes them as cursor-paginated connections. Preserve the existing `pr reconcile` identity check and read-only timeout/retry boundary (`go/internal/prcmd/pr.go`, `go/internal/prcmd/readonly.go`). -- KTD2. Store the latest thread snapshot and its completeness/freshness beside the task's existing PR identity and head. Keep prior known threads when a page is incomplete or fails; optional thread fields keep older task records valid. Store address requests as separate durable records linked by stable request ID so later thread revisions remain distinct. -- KTD3. Reuse the shared action descriptions from #305 and route Address through existing repair delivery. Actions bind to the latest comment revision and verified PR head; thread observations and actions do not mutate GitHub. This keeps task execution, review evidence, and GitHub's resolution state independent. - -### High-Level Technical Design - -```mermaid -sequenceDiagram - participant C as Coordinator - participant P as PR reconcile - participant G as GitHub - participant S as Saved task PR state - participant I as Inbox view - participant R as Repair path - C->>P: reconcile recorded PR - P->>G: read PR identity and head - P->>G: read reviewThreads pages and reviewDecision - G-->>P: thread/comment nodes and pageInfo - P->>P: verify repository, PR, head, and page completeness - P->>S: save current snapshot and observation status - I->>S: render saved thread rows and actions - C->>I: choose Address for current thread revisions - I->>P: submit scoped action request - P->>G: revalidate PR head and selected revisions - P->>S: save one request with exact IDs and revisions - P->>R: use existing task/attempt/repair checks - Note over G,S: Replies and resolution change only after a separate authorized GitHub action. -``` - -### Assumptions and Deferred Implementation Notes - -- The task branch remains targeted to `main`; the implementer rechecks `origin/main` before changing files and does not stack #305's unmerged implementation into this branch. -- Exact GraphQL page size, maximum page count, and excerpt size should match the existing bounded-process/output limits discovered in the implementation. Reaching a limit records an incomplete observation rather than implying closure. -- The exact shared-action API is resolved from #305's merged code at implementation time; this plan does not restate or fork that API. - ---- - -## Implementation Units - -### U1. Observe and persist bounded review threads - -- **Goal:** Extend explicit PR reconciliation with an identity-bound review-thread and overall review-decision observation. -- **Requirements:** R1, R2, R3, R8. -- **Dependencies:** None. -- **Files:** `go/internal/prcmd/pr.go`, `go/internal/prcmd/threads.go`, `go/internal/prcmd/threads_test.go`, `go/internal/prcmd/readonly_test.go`, `tests/fixtures/gh_attach.py`, `go/internal/cli/quiet_coordination_replay_test.go`. -- **Approach:** Keep `gh pr view` as the identity and PR-head source. Read review threads with bounded GraphQL cursor pagination and latest-comment data. Save only observations bound to the same repository, PR number, and head. On failure or page limits, preserve known threads and record incomplete/failed status and freshness. -- **Patterns to follow:** `viewPR`, `recordObservationWith`, `readOnlyGH`, `GHBound`, and the fake-`gh` counting scripts in the PR command tests. -- **Test scenarios:** - - A complete single-page response saves stable thread/comment IDs, author, URL, file/line, latest reply, state, review decision, head, and observation time. - - A multi-page response follows each `endCursor` until `hasNextPage` is false and stores each thread once. - - Identical repeated observations create no duplicate thread or address request. - - A revised latest comment updates the existing thread identity and makes the new revision current. - - A reopened thread is saved as unresolved and actionable. - - A failed page or reached bound marks the observation incomplete and retains previously known unseen threads. - - A mismatched repository, PR number, or head is rejected before saving the thread snapshot. -- **Verification:** Focused PR adapter and CLI coverage demonstrates complete, partial, failed, repeated, revised, and wrong-identity observations without adding a background poller. - -### U2. Render saved threads through the shared inbox actions - -- **Goal:** Show saved review threads and their current source-specific actions in task context and existing inbox surfaces. -- **Requirements:** R1, R2, R3, R4, R8. -- **Dependencies:** U1 and #305's shared action foundation. -- **Files:** `go/internal/presentation/presentation.go`, `go/internal/inboxview/overview.go`, `go/internal/inboxview/terminal.go`, `go/internal/inboxview/overview_test.go`, `go/internal/inboxview/terminal_test.go`, `go/internal/cli/context_test.go`, `go/internal/cli/presentation_test.go`. -- **Approach:** Extend the shared presentation/action layer with the smallest thread-specific projection. Render latest observation time and uncertainty; keep rows read-only and preserve stable source identity. Leave #305-owned question/action behavior intact. -- **Patterns to follow:** Existing PR, return, attention, and decision presentation rows; use the shared action structure introduced by #305 rather than defining a parallel one. -- **Test scenarios:** - - A saved complete thread renders its author, summary, source URL, path/line, and current status in context and inbox output. - - An incomplete snapshot renders its uncertainty and retains previously observed threads. - - A task record without thread fields renders exactly as a legacy task. - - `inbox --live` renders saved thread state without invoking GitHub. -- **Verification:** Context and inbox outputs agree on thread identity, freshness, and available actions; existing decision presentation remains unchanged. - -### U3. Revalidate and route scoped follow-up actions - -- **Goal:** Record Address, Discuss, Draft reply, and Open comment actions while keeping repair execution and GitHub changes separate. -- **Requirements:** R4, R5, R6, R7, R8. -- **Dependencies:** U2. -- **Files:** `go/internal/prcmd/pr.go`, `go/internal/repair/repair.go`, `go/internal/cli/pr.go`, `go/internal/cli/repair_test.go`, `go/internal/cli/pr_test.go`, `go/internal/presentation/presentation_test.go`. -- **Approach:** Bind each action request to the verified PR/head and the selected thread IDs plus latest comment revisions. Revalidate before saving or sending. Address selected/all creates one stable request and follows existing task/attempt/repair classification and delivery; other actions open the source, record discussion, or create draft text. Do not touch `go/internal/review`, whose shared lane belongs to #307. -- **Patterns to follow:** Existing repair idempotency, task/attempt validation, saved return delivery, and the #305 action request/response path. -- **Test scenarios:** - - Address selected records only the requested thread IDs and their exact current revisions in one request linked from each row. - - Address all includes exactly the currently actionable thread set and duplicate submission returns the existing request. - - A changed PR binding/head or latest-comment revision refuses a stale action before delivery. - - An uncertain repair delivery remains uncertain and is not resent under a new key. - - In-scope corrections use the existing in-scope route; added work requires the existing expansion authorization. - - Draft reply yields reviewable text, while Address completion leaves GitHub reply and resolution fields unchanged. -- **Verification:** Saved action requests can be traced to a single repair attempt and cannot fabricate external closure or bypass current delivery gates. - -### U4. Document the operator path and prove the changed workflow - -- **Goal:** Explain how coordinators inspect, address, discuss, draft, and open GitHub review threads, including incomplete observations and the distinction between local and GitHub state. -- **Requirements:** R3, R4, R5, R6, R8, R9. -- **Dependencies:** U1, U2, U3. -- **Files:** `go/internal/cli/root.go`, `go/internal/cli/testdata/`, `docs/features/pipeline.md`, `docs/features/coordination.md`, `docs/repairs.md`. -- **Approach:** Update existing help and feature rows; keep the canonical behavior in the existing CLI, repair, and inbox documentation. Capture before/after CLI transcript evidence for the reconcile-to-inbox path. -- **Patterns to follow:** Existing feature-map scenario rows and help snapshots. Regenerate snapshots only with the repository's established command when help text changes. -- **Test scenarios:** - - Help describes explicit observation and the fact that views do not poll GitHub. - - Feature documentation distinguishes local follow-up from GitHub reply and resolution. - - Before/after CLI transcripts show the new saved thread row and action without claiming visual proof. -- **Verification:** Updated documentation matches observable command output; every new scenario is mapped to focused coverage and the evidence comparison is listed in the handoff. - ---- - -## Verification Contract - -- `mise run verify` runs the repository's declared verification task. -- `python3 .agents/skills/verify/scripts/verify_run.py --base 41ba86fc24f5b08c7cfb3149c2b2bc9c9d8f735c --json` records candidate-bound verification for the task. -- Existing Go behavior coverage runs through `go test ./...` as part of `mise run verify`; focused tests cover identity, pagination, idempotency, stale actions, uncertain delivery, and scope classification. -- Factory evidence uses before/after CLI transcripts for the changed PR-reconcile and inbox path; visual proof is not applicable to a CLI/API change. List every `comparison.json` in the handoff. -- The coordinator runs the same verification contract on its own run ID and performs independent review. The worker does not merge. - ---- - -## Definition of Done - -- Every R1-R9 behavior is implemented within the issue's lane boundaries, with focused coverage and updated help/feature documentation. -- Old records remain readable, incomplete observations retain prior information, and repeated observations or action submissions create no duplicate work. -- Local dispositions and repair outcomes never masquerade as GitHub replies or resolution. -- The candidate is committed on the task branch, the worker verification record and evidence comparisons are attached to the handoff, and an unmerged PR is available for coordinator verification and review. -- Abandoned implementation attempts are removed from the final diff. diff --git a/go/internal/prcmd/readonly_test.go b/go/internal/prcmd/readonly_test.go index 3d2f4bf..fe563af 100644 --- a/go/internal/prcmd/readonly_test.go +++ b/go/internal/prcmd/readonly_test.go @@ -308,7 +308,7 @@ func TestRetryBoundaryIsExactlyTheTwoReadOnlyObservations(t *testing.T) { got = append(got, name) } sort.Strings(got) - want := []string{"observedVisibility", "viewPR"} + want := []string{"observeReviewThreads", "observedVisibility", "viewPR"} if strings.Join(got, ",") != strings.Join(want, ",") { t.Fatalf("readOnlyGH callers = %v, want %v", got, want) } From e2d441f87d7203a95461b6a15d17fe622486c3e9 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sun, 18 Oct 2026 04:01:10 +0000 Subject: [PATCH 07/10] docs: label review thread scenarios as manual --- docs/features/pipeline.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/features/pipeline.md b/docs/features/pipeline.md index a10821c..dd802f3 100644 --- a/docs/features/pipeline.md +++ b/docs/features/pipeline.md @@ -69,8 +69,8 @@ The table is the coordinator's record of what has run. It is not verification, r | `pipeline.pr-fresh-base` | `pipeline pr` fetches the base again before creating or adopting a PR and refuses to create one against a base that moved since the push, creating nothing, until `--allow-behind`; `pipeline run --allow-behind` passes the flag to both Push and PR (`go/internal/pipelinerun/pipelinerun.go`) | automated: `go/internal/cli/pipeline_freshbase_test.go` | offline suite | | `pipeline.pr-mergeability` | `pr reconcile` and `pipeline ci` record GitHub's `mergeable` and `mergeStateStatus`; an open PR reported `CONFLICTING`/`DIRTY` blocks the PR row with the merge-the-base remedy and shows under `maintenance.open_prs` with `blocked` and `fix`, and a later `MERGEABLE` observation passes the row again | automated: `go/internal/cli/pipeline_freshbase_test.go` | offline suite | | `pipeline.pr-read-retry` | Read-only `gh` observations in `go/internal/prcmd` make at most one second attempt, and only after a conclusively completed nonzero exit whose stderr positively names a transient condition: an `HTTP 5xx` status line, `HTTP 429`, gh's secondary-rate-limit sentence, or a transport failure as gh prints it (`error connecting to`, `connection refused`, tls or x509 handshake errors). The `gh pr view` and `gh api graphql` PR reads (`pr reconcile`, `sweep`) share one bound per observation; `gh repo view --json visibility` (evidence publication) retains its existing bound. Attempt one runs on the runner's own timer unchanged, attempt two receives only the time left, and a delay that would not leave a floor for a second attempt defers instead of overrunning. A runner error (timeout, cancel, oversized or incomplete output, uncertain descendant), an auth failure, 404, any other 4xx including a bare 403, a `GraphQL:` application error, malformed JSON, or ambiguous stderr is never retried; no write (agent launch, PR create, edit, ready, publish, push, merge) and no other `gh` read gains a retry, and `go/internal/proc` keeps its no-retry contract. A failure after the retry is still an uncertain observation naming the attempt count and category with token shapes redacted and the detail capped; the prior `pr` record and `observed_at` stay as recorded and `sweep` stamps `observe_failed_at` as before | automated: `go/internal/prcmd/readonly_test.go`, `go/internal/prcmd/pr_test.go` | offline suite against counting fake `gh` scripts: transient-then-valid succeeds after exactly two calls, persistent 503 fails after exactly two, each non-transient shape makes one call, a 429 with a short bound defers after one call, a slow first attempt leaves no budget, the runner called directly runs once, and a Go source-parsing pass over the package pins the helper's callers to `observeReviewThreads`, `viewPR`, and `observedVisibility`. Implementation: `go/internal/prcmd/readonly.go`, `go/internal/prcmd/threads.go`. | -| `pipeline.review-threads` | An explicit `pr reconcile` records cursor-paginated GitHub review threads and the overall review decision only when the returned PR number, URL, and head match the identity observation; each thread retains its stable node ID, source URL, file and supplied lines, latest comment identity, author, update time, resolution/outdated state, and observed head/time. The read has a 20-page maximum of 50 threads per page and shares one `GHBound`; a failed, incomplete, nonadvancing, or mismatched response is marked uncertain and keeps previously observed threads with their original per-thread observation time/head. Reconciliation is the only GitHub read for these rows; later saved-state projections do not poll. | no passing automated coverage for thread observation in this candidate. | `go test ./internal/prcmd` currently fails in the pre-existing `readonly_test.go` caller allowlist because it does not include `observeReviewThreads`. No thread-observation test source is included in this candidate. | -| `pipeline.review-thread-actions` | The saved inbox and `context --section review_threads` expose source URL, author, file/line, freshness, state, and Address selected/all, Discuss, Draft reply, and Open comment actions. Actions revalidate PR URL/head and exact comment revisions. Address records one idempotent request with every included thread/revision and uses `repair.Send`; changed selections and incomplete Address-all snapshots are refused. Uncertain and submitted requests stay linked to their threads and cannot be resent. Discuss and Draft reply are local records; no action publishes a reply or resolves GitHub state. | no automated coverage in this candidate. | A CLI before/after transcript verifies saved review-thread rows in inbox and `inbox --live`; it does not verify action revalidation, duplicate prevention, or delivery behavior. | +| `pipeline.review-threads` | An explicit `pr reconcile` records cursor-paginated GitHub review threads and the overall review decision only when the returned PR number, URL, and head match the identity observation; each thread retains its stable node ID, source URL, file and supplied lines, latest comment identity, author, update time, resolution/outdated state, and observed head/time. The read has a 20-page maximum of 50 threads per page and shares one `GHBound`; a failed, incomplete, nonadvancing, or mismatched response is marked uncertain and keeps previously observed threads with their original per-thread observation time/head. Reconciliation is the only GitHub read for these rows; later saved-state projections do not poll. | manual: no automated thread-observation coverage in this candidate. | The CLI before/after transcript checks saved thread visibility only; pagination, partial observations, and retention are not verified. | +| `pipeline.review-thread-actions` | The saved inbox and `context --section review_threads` expose source URL, author, file/line, freshness, state, and Address selected/all, Discuss, Draft reply, and Open comment actions. Actions revalidate PR URL/head and exact comment revisions. Address records one idempotent request with every included thread/revision and uses `repair.Send`; changed selections and incomplete Address-all snapshots are refused. Uncertain and submitted requests stay linked to their threads and cannot be resent. Discuss and Draft reply are local records; no action publishes a reply or resolves GitHub state. | manual: no automated coverage in this candidate. | The CLI before/after transcript checks saved thread visibility only; Address revalidation, duplicate prevention, and delivery behavior are not verified. | | `pipeline.push-from-the-pr` | A reconciled PR whose head is this candidate passes Push on its own, because the candidate demonstrably reached origin | automated: `go/internal/pipeline/derive_test.go` | offline suite | | `pipeline.run` | `sumctl pipeline run TASK_ID` runs rebase, test, lint, document, push, and PR in order, stops at the first failing gate, and at a blocked Test, without running the rest, skips a candidate that is already verified unless `--rerun`, skips lint when `verify --execute` already recorded it, and prints the first unsettled stage with what to do about it | automated: `go/internal/cli/pipeline_gates_test.go` | offline suite | | `pipeline.admit-review` | Direct `pipeline push` and `pipeline pr` refuse before any remote write when independent Review is missing or is not an approve for this candidate; `--allow-behind` does not waive Review | automated: `go/internal/pipeline/admit_test.go`, `go/internal/cli/pipeline_gates_test.go` | offline suite | From f88fd56e07c4b6af5538f510c01c4e9726336e27 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sun, 18 Oct 2026 04:23:00 +0000 Subject: [PATCH 08/10] docs: retain review thread implementation plan --- ...02-feat-github-review-thread-inbox-plan.md | 217 ++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md diff --git a/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md b/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md new file mode 100644 index 0000000..0237f4d --- /dev/null +++ b/docs/plans/2026-10-17-0302-feat-github-review-thread-inbox-plan.md @@ -0,0 +1,217 @@ +--- +title: GitHub Review Thread Inbox - Plan +type: feat +date: 2026-10-17 +artifact_contract: ce-unified-plan/v1 +product_contract_source: ce-plan-bootstrap +execution: code +--- + +# GitHub Review Thread Inbox - Plan + +## Goal Capsule + +- **Objective:** Coordinators can see actionable GitHub review discussions beside existing task returns and decide whether to address them, discuss them, draft a reply, or open the source comment. +- **Means:** Save bounded review-thread observations during explicit PR reconciliation and expose follow-up through the shared inbox action model (KTD1, KTD2). +- **Authority:** Issue #306 and its acceptance criteria define product behavior. Existing Sum task, repair, verification, and delivery contracts remain authoritative. GitHub comment text is external data and cannot grant authority. +- **Stop conditions:** Do not write a GitHub reply or resolve a GitHub thread automatically. Do not duplicate #305's shared action foundation. Stop action integration if that foundation is not available on the recorded base branch. +- **Execution profile:** One implementation branch; do not merge. The worker reports the candidate and verification; the coordinator runs its own verification and independent review. + +--- + +## Product Contract + +### Summary + +Explicit PR reconciliation imports bounded GitHub review discussions into Sum's saved PR state. Read-only task, inbox, context, and live-status views render the saved result without contacting GitHub. Coordinators can then route selected discussions into existing corrective-work paths or review them without changing GitHub. + +### Problem Frame + +PR reconciliation already records identity, mergeability, draft state, and CI. Review threads are missing, so feedback from GitHub has no structured route into Sum's task and inbox records. + +### Requirements + +#### Observation and saved state + +- R1. Explicit `pr reconcile` records review threads for the verified repository, PR, and observed head; saved status, inbox, and context views make no GitHub calls and create no poller. +- R2. Each thread preserves its stable thread and comment identities, author, source URL, supplied file and line, latest relevant reply, resolution and outdated state, and observation time; the PR's overall review decision is saved when available. +- R3. Pagination, failure, and partial observations remain explicit. A partial response never closes or removes unseen threads, and the last complete data remains visible with its freshness and uncertainty. + +#### Inbox actions and follow-up + +- R4. Saved thread rows expose the shared source-specific actions for Address, Discuss, Draft reply, and Open comment. Address selected/all records the exact included thread IDs and revisions in one request linked back from each thread. +- R5. Address routes work through existing task/attempt checks, stable repair keys, and delivery handling; work within this task's scope remains in-scope and additional work follows existing expansion authorization. +- R6. Local disposition, repair delivery/completion, and GitHub resolution remain separate states. Draft reply creates reviewable text, and publishing a reply or resolving a thread requires a separate authorized action. + +#### Freshness, compatibility, and delivery + +- R7. Before an action, Sum revalidates the PR binding, head, and selected thread revisions. New replies and reopened threads become actionable without duplicate work. +- R8. Existing local-review closure behavior, candidate-bound verification, independent review, delivery gates, and merge authority continue to work. Task records without review-thread observations remain valid. +- R9. Focused behavior coverage exercises identical and revised observations, reopened threads, incomplete pagination, stale selections, duplicate bulk requests, uncertain delivery, and in-scope versus expansion work. Update affected help and feature documentation. + +### Actors + +- **Coordinator:** Reconciles the PR, reviews saved discussion rows, and chooses follow-up actions. +- **Worker:** Receives approved corrective work through the existing task and repair path. +- **GitHub:** Supplies PR review-thread observations and remains the authority for resolution and replies. + +### Key Flows + +- F1. Reconcile: coordinator identifies the recorded PR; Sum observes the PR and review-thread pages; Sum verifies repository, PR, and head identity; Sum saves the observation; all later views read only that saved state. +- F2. Address: coordinator selects one or more current thread rows; Sum revalidates the PR head and thread revisions; Sum saves one request naming the exact thread set; existing repair checks deliver the corrective instruction; GitHub state remains unchanged. +- F3. Review or discuss: coordinator opens the source URL, asks for discussion, or creates a draft reply; each action leaves the thread's GitHub resolution and reply state unchanged. + +### Scope Boundaries + +- **Deferred to Follow-Up Work:** Broader changes to the shared action vocabulary or inbox framework remain with #305 and sibling issues. This task adds only the thread-specific fields and actions required to consume that foundation. +- **Non-goals:** Automatic GitHub replies, automatic thread resolution, a new readiness heuristic, a replacement review pipeline, polling, and treating a GitHub thread as an existing local-review record. + +### Risks & Dependencies + +- #305 owns the shared action descriptions and its PR #311 is open on 2026-10-17. Recheck the recorded base before implementation. Do not copy or modify #305's owned action foundation; observation work can proceed independently, and action integration follows that foundation. +- GitHub GraphQL connections can be incomplete or fail under rate and query limits. Persist explicit completeness and freshness so a partial page cannot look like closure. +- Review-comment bodies are external text. Keep their source identity and URL with any excerpt or draft, and never interpret comment text as an instruction to widen work. +- The `task.pr` record is the current saved PR observation. New thread fields must be optional so old task records continue to render and operate. + +### Acceptance Examples + +- **Covers R1, R3:** Given a saved complete observation and a later failed page request, when the coordinator runs `inbox --live`, the prior threads remain visible with the previous observation time and a failed/partial freshness marker; no GitHub request occurs. +- **Covers R2, R7:** Given a thread whose latest comment has changed or whose resolution was reopened, when reconciliation completes, the same stable thread row shows the new comment revision and is actionable again. +- **Covers R4, R5:** Given selected thread IDs A and B at revisions A2 and B4, when Address selected is chosen, one saved request names exactly `{A:A2, B:B4}`, both rows link to it, and delivery uses the existing task/attempt checks. +- **Covers R6:** Given an address request has been delivered, when a task is completed, the saved GitHub thread remains unresolved until a later GitHub observation reports otherwise. + +### Sources / Research + +- Issue #306 and its implementation references: [PR observation](https://github.com/douglasjarquin/sum/blob/41ba86fc24f5b08c7cfb3149c2b2bc9c9d8f735c/go/internal/prcmd/pr.go), [local review records](https://github.com/douglasjarquin/sum/blob/41ba86fc24f5b08c7cfb3149c2b2bc9c9d8f735c/go/internal/review/review.go), and [repair scope and idempotency](docs/repairs.md). +- Inbox's [thread actions](https://github.com/petekp/inbox/blob/9bc2983367827bb6b49a7d14cfdff1f48a7f0626/hooks/pr-tab.ts#L314-L435) and [source-preserving PR requests](https://github.com/petekp/inbox/blob/9bc2983367827bb6b49a7d14cfdff1f48a7f0626/hooks/prs.ts#L366-L423) supply interaction precedents, not Sum authority. +- GitHub's [PullRequest GraphQL fields](https://docs.github.com/en/graphql/reference/pulls) expose `reviewThreads` and `reviewDecision`; its [pagination guide](https://docs.github.com/en/graphql/guides/using-pagination-in-the-graphql-api) requires cursor traversal and page-info checks for complete connection reads. +- `docs/solutions/architecture-patterns/retiring-persisted-compatibility-fields.md` recommends deriving compatibility from the actual readers and keeping persisted additions optional for older records. + +--- + +## Planning Contract + +### Key Technical Decisions + +- KTD1. Use a bounded, read-only GitHub GraphQL observation for `reviewThreads` and `reviewDecision` alongside the existing `gh pr view` result. `prViewFields` does not contain thread fields, while GitHub exposes them as cursor-paginated connections. Preserve the existing `pr reconcile` identity check and read-only timeout/retry boundary (`go/internal/prcmd/pr.go`, `go/internal/prcmd/readonly.go`). +- KTD2. Store the latest thread snapshot and its completeness/freshness beside the task's existing PR identity and head. Keep prior known threads when a page is incomplete or fails; optional thread fields keep older task records valid. Store address requests as separate durable records linked by stable request ID so later thread revisions remain distinct. +- KTD3. Reuse the shared action descriptions from #305 and route Address through existing repair delivery. Actions bind to the latest comment revision and verified PR head; thread observations and actions do not mutate GitHub. This keeps task execution, review evidence, and GitHub's resolution state independent. + +### High-Level Technical Design + +```mermaid +sequenceDiagram + participant C as Coordinator + participant P as PR reconcile + participant G as GitHub + participant S as Saved task PR state + participant I as Inbox view + participant R as Repair path + C->>P: reconcile recorded PR + P->>G: read PR identity and head + P->>G: read reviewThreads pages and reviewDecision + G-->>P: thread/comment nodes and pageInfo + P->>P: verify repository, PR, head, and page completeness + P->>S: save current snapshot and observation status + I->>S: render saved thread rows and actions + C->>I: choose Address for current thread revisions + I->>P: submit scoped action request + P->>G: revalidate PR head and selected revisions + P->>S: save one request with exact IDs and revisions + P->>R: use existing task/attempt/repair checks + Note over G,S: Replies and resolution change only after a separate authorized GitHub action. +``` + +### Assumptions and Deferred Implementation Notes + +- The task branch remains targeted to `main`; the implementer rechecks `origin/main` before changing files and does not stack #305's unmerged implementation into this branch. +- Exact GraphQL page size, maximum page count, and excerpt size should match the existing bounded-process/output limits discovered in the implementation. Reaching a limit records an incomplete observation rather than implying closure. +- The exact shared-action API is resolved from #305's merged code at implementation time; this plan does not restate or fork that API. + +--- + +## Implementation Units + +### U1. Observe and persist bounded review threads + +- **Goal:** Extend explicit PR reconciliation with an identity-bound review-thread and overall review-decision observation. +- **Requirements:** R1, R2, R3, R8. +- **Dependencies:** None. +- **Files:** `go/internal/prcmd/pr.go`, `go/internal/prcmd/threads.go`, `go/internal/prcmd/threads_test.go`, `go/internal/prcmd/readonly_test.go`, `tests/fixtures/gh_attach.py`, `go/internal/cli/quiet_coordination_replay_test.go`. +- **Approach:** Keep `gh pr view` as the identity and PR-head source. Read review threads with bounded GraphQL cursor pagination and latest-comment data. Save only observations bound to the same repository, PR number, and head. On failure or page limits, preserve known threads and record incomplete/failed status and freshness. +- **Patterns to follow:** `viewPR`, `recordObservationWith`, `readOnlyGH`, `GHBound`, and the fake-`gh` counting scripts in the PR command tests. +- **Test scenarios:** + - A complete single-page response saves stable thread/comment IDs, author, URL, file/line, latest reply, state, review decision, head, and observation time. + - A multi-page response follows each `endCursor` until `hasNextPage` is false and stores each thread once. + - Identical repeated observations create no duplicate thread or address request. + - A revised latest comment updates the existing thread identity and makes the new revision current. + - A reopened thread is saved as unresolved and actionable. + - A failed page or reached bound marks the observation incomplete and retains previously known unseen threads. + - A mismatched repository, PR number, or head is rejected before saving the thread snapshot. +- **Verification:** Focused PR adapter and CLI coverage demonstrates complete, partial, failed, repeated, revised, and wrong-identity observations without adding a background poller. + +### U2. Render saved threads through the shared inbox actions + +- **Goal:** Show saved review threads and their current source-specific actions in task context and existing inbox surfaces. +- **Requirements:** R1, R2, R3, R4, R8. +- **Dependencies:** U1 and #305's shared action foundation. +- **Files:** `go/internal/presentation/presentation.go`, `go/internal/inboxview/overview.go`, `go/internal/inboxview/terminal.go`, `go/internal/inboxview/overview_test.go`, `go/internal/inboxview/terminal_test.go`, `go/internal/cli/context_test.go`, `go/internal/cli/presentation_test.go`. +- **Approach:** Extend the shared presentation/action layer with the smallest thread-specific projection. Render latest observation time and uncertainty; keep rows read-only and preserve stable source identity. Leave #305-owned question/action behavior intact. +- **Patterns to follow:** Existing PR, return, attention, and decision presentation rows; use the shared action structure introduced by #305 rather than defining a parallel one. +- **Test scenarios:** + - A saved complete thread renders its author, summary, source URL, path/line, and current status in context and inbox output. + - An incomplete snapshot renders its uncertainty and retains previously observed threads. + - A task record without thread fields renders exactly as a legacy task. + - `inbox --live` renders saved thread state without invoking GitHub. +- **Verification:** Context and inbox outputs agree on thread identity, freshness, and available actions; existing decision presentation remains unchanged. + +### U3. Revalidate and route scoped follow-up actions + +- **Goal:** Record Address, Discuss, Draft reply, and Open comment actions while keeping repair execution and GitHub changes separate. +- **Requirements:** R4, R5, R6, R7, R8. +- **Dependencies:** U2. +- **Files:** `go/internal/prcmd/pr.go`, `go/internal/repair/repair.go`, `go/internal/cli/pr.go`, `go/internal/cli/repair_test.go`, `go/internal/cli/pr_test.go`, `go/internal/presentation/presentation_test.go`. +- **Approach:** Bind each action request to the verified PR/head and the selected thread IDs plus latest comment revisions. Revalidate before saving or sending. Address selected/all creates one stable request and follows existing task/attempt/repair classification and delivery; other actions open the source, record discussion, or create draft text. Do not touch `go/internal/review`, whose shared lane belongs to #307. +- **Patterns to follow:** Existing repair idempotency, task/attempt validation, saved return delivery, and the #305 action request/response path. +- **Test scenarios:** + - Address selected records only the requested thread IDs and their exact current revisions in one request linked from each row. + - Address all includes exactly the currently actionable thread set and duplicate submission returns the existing request. + - A changed PR binding/head or latest-comment revision refuses a stale action before delivery. + - An uncertain repair delivery remains uncertain and is not resent under a new key. + - In-scope corrections use the existing in-scope route; added work requires the existing expansion authorization. + - Draft reply yields reviewable text, while Address completion leaves GitHub reply and resolution fields unchanged. +- **Verification:** Saved action requests can be traced to a single repair attempt and cannot fabricate external closure or bypass current delivery gates. + +### U4. Document the operator path and prove the changed workflow + +- **Goal:** Explain how coordinators inspect, address, discuss, draft, and open GitHub review threads, including incomplete observations and the distinction between local and GitHub state. +- **Requirements:** R3, R4, R5, R6, R8, R9. +- **Dependencies:** U1, U2, U3. +- **Files:** `go/internal/cli/root.go`, `go/internal/cli/testdata/`, `docs/features/pipeline.md`, `docs/features/coordination.md`, `docs/repairs.md`. +- **Approach:** Update existing help and feature rows; keep the canonical behavior in the existing CLI, repair, and inbox documentation. Capture before/after CLI transcript evidence for the reconcile-to-inbox path. +- **Patterns to follow:** Existing feature-map scenario rows and help snapshots. Regenerate snapshots only with the repository's established command when help text changes. +- **Test scenarios:** + - Help describes explicit observation and the fact that views do not poll GitHub. + - Feature documentation distinguishes local follow-up from GitHub reply and resolution. + - Before/after CLI transcripts show the new saved thread row and action without claiming visual proof. +- **Verification:** Updated documentation matches observable command output; every new scenario is mapped to focused coverage and the evidence comparison is listed in the handoff. + +--- + +## Verification Contract + +- `mise run verify` runs the repository's declared verification task. +- `python3 .agents/skills/verify/scripts/verify_run.py --base 41ba86fc24f5b08c7cfb3149c2b2bc9c9d8f735c --json` records candidate-bound verification for the task. +- Existing Go behavior coverage runs through `go test ./...` as part of `mise run verify`; focused tests cover identity, pagination, idempotency, stale actions, uncertain delivery, and scope classification. +- Factory evidence uses before/after CLI transcripts for the changed PR-reconcile and inbox path; visual proof is not applicable to a CLI/API change. List every `comparison.json` in the handoff. +- The coordinator runs the same verification contract on its own run ID and performs independent review. The worker does not merge. + +--- + +## Definition of Done + +- Every R1-R9 behavior is implemented within the issue's lane boundaries, with focused coverage and updated help/feature documentation. +- Old records remain readable, incomplete observations retain prior information, and repeated observations or action submissions create no duplicate work. +- Local dispositions and repair outcomes never masquerade as GitHub replies or resolution. +- The candidate is committed on the task branch, the worker verification record and evidence comparisons are attached to the handoff, and an unmerged PR is available for coordinator verification and review. +- Abandoned implementation attempts are removed from the final diff. From 2b738b79916e1912341c5c2e7d25fb6622bb9549 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sun, 18 Oct 2026 04:36:20 +0000 Subject: [PATCH 09/10] test: account for read-only review thread observation --- go/internal/cli/quiet_coordination_replay_test.go | 2 +- go/internal/cli/testdata/help.stdout | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go/internal/cli/quiet_coordination_replay_test.go b/go/internal/cli/quiet_coordination_replay_test.go index 65cb638..118b635 100644 --- a/go/internal/cli/quiet_coordination_replay_test.go +++ b/go/internal/cli/quiet_coordination_replay_test.go @@ -424,7 +424,7 @@ func TestQuietCoordinationReplay(t *testing.T) { verbs := lab.ghVerbs(t) for verb := range verbs { switch verb { - case "issue list", "issue edit", "issue comment", "repo view", "pr list", "pr create", "pr view", "pr checks", "pr ready", "auth status", "--version", "pr edit --help": + case "issue list", "issue edit", "issue comment", "repo view", "pr list", "pr create", "pr view", "pr checks", "pr ready", "auth status", "--version", "pr edit --help", "api graphql": default: t.Fatalf("fake gh saw an unexpected verb %q: %v", verb, verbs) } diff --git a/go/internal/cli/testdata/help.stdout b/go/internal/cli/testdata/help.stdout index a0de16d..bd55b1d 100644 --- a/go/internal/cli/testdata/help.stdout +++ b/go/internal/cli/testdata/help.stdout @@ -12,7 +12,7 @@ commands: execution: "Show, park, or explicitly resume task-local execution reservations" repair: Coordinator-controlled corrective worker instructions help: "Concise command discovery: every command with one line, or `help TOPIC` (e.g. brief, brief-adopt) for its arguments" - context: "Bounded selective read of one task: outline by default, --section for parts, --role for a role view, --since CURSOR for changes" + context: "Bounded selective read of one task: outline by default, --section for parts including saved GitHub review threads, --role for a role view, --since CURSOR for changes" notes: "Append one timestamped entry to the task's optional notes.md (a claim, backed up with the records; credentials refused)" finding: Record and follow optional findings with explicit coordinator dispositions and linked work env: "Task-local environment record: declared dev commands, observed URLs/ports, log paths, pane/container references; observes and records only" From 2f648a70a364ca865cd1bf76d159cfa3838bd094 Mon Sep 17 00:00:00 2001 From: Douglas Jarquin <8209+douglasjarquin@users.noreply.github.com> Date: Sun, 18 Oct 2026 05:19:14 +0000 Subject: [PATCH 10/10] gofmt presentation source kinds --- go/internal/presentation/presentation.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/go/internal/presentation/presentation.go b/go/internal/presentation/presentation.go index 3d8092a..0b3c9ae 100644 --- a/go/internal/presentation/presentation.go +++ b/go/internal/presentation/presentation.go @@ -9,17 +9,17 @@ import ( type SourceKind string const ( - Question SourceKind = "question" - Answer SourceKind = "answer" - Report SourceKind = "report" - Review SourceKind = "review" - Attention SourceKind = "attention" - Refresh SourceKind = "refresh" - Execution SourceKind = "execution" - Pipeline SourceKind = "pipeline" - Factory SourceKind = "factory" - Finding SourceKind = "finding" - Gap SourceKind = "gap" + Question SourceKind = "question" + Answer SourceKind = "answer" + Report SourceKind = "report" + Review SourceKind = "review" + Attention SourceKind = "attention" + Refresh SourceKind = "refresh" + Execution SourceKind = "execution" + Pipeline SourceKind = "pipeline" + Factory SourceKind = "factory" + Finding SourceKind = "finding" + Gap SourceKind = "gap" GitHubReviewThread SourceKind = "github-review-thread" )