[danii1]

Why

Task local-fix-code-do-not-unwatch-agent-prs-when-g: fix(code): do not unwatch agent PRs when GitHub state is missing

applyAgentPrFetch treated every PR state that was not literally "open" as terminal. A payload that omitted state (or returned null, an empty string, or an unknown value) therefore called markAgentPrClosed, permanently removing a still-open PR from the watch list. Because listOpenAgentPrs() then stopped returning the row, review comments and CI feedback were never polled again and the row could not self-correct. The fix makes only explicitly terminal states close a row; unusable state leaves it watched and retried on the next tick.

What changed

• packages/code/src/lib/review/agent-pr-reconciler.ts
  • PolledPr.state is now optional (state?: string), so the type system forces an explicit decision about a missing value.
  • Added classifyAgentPrState(state: unknown): "open" | "closed" | "merged" | null: only "closed" and "merged" are terminal, "open" is alive, and everything else (missing, null, empty, non-string, unrecognised) is unusable.
  • AgentPrClosure.reason is now the TerminalAgentPrState union, so the poller's is ${closure.reason}; unwatching log can no longer receive undefined.
  • applyAgentPrFetch still writes the ETag cursor first, then returns null for unsupported data, unusable state, and "open"; it closes the row only for terminal state.
  • Added a bounded diagnostic: one console.warn per repo#PR per process for unusable state, naming the repo, PR number, and a short value description (missing, null, empty string, unrecognised "...", or unexpected <type>), never the full payload.
• packages/code/tests/agent-pr-reconciler.test.ts
  • Added coverage for missing/undefined/null/empty/unknown state leaving the row open while persisting the ETag, the once-per-PR warning bound, "open" staying watched without a warning, "closed"/"merged" closing the row, and reconcileOpenAgentPrs keeping the row in listOpenAgentPrs() with closed: [] while still sharing the fetch via fresh.

Scope

Included only the reconciler validation, its unit tests, and the bounded warning. review-polling.ts needed no signature change: it already consumes AgentPrClosure.reason and PolledPr types. No queue-database edits, no GitHub auth/fetch/ETag/404 changes, no schedule changes, no dashboard work, and no commit/push/PR actions.

Tradeoffs

The warning dedupe lives in module-level state, so it is per process and intentionally not persisted: a worker restart can re-emit one warning per affected PR, which matches the requirement. Successful reconciliation of a PR closes the wrong-state window only via the next tick; no extra GitHub request was added. Terminal-state matching is case-insensitive and trims whitespace, which is harmless for GitHub but slightly looser than an exact comparison.

Blast Radius

Callers inspected: reconcileOpenAgentPrs (shared fresh map now receives results for unusable state, unchanged behavior for open/404/304), ReviewPollingAcquirer.pollPr in packages/code/src/lib/acquirers/review-polling.ts (closure path now only logs terminal reasons; prefetched results still skip a second fetch), and the fleet GitHub client wiring in packages/code/src/lib/workspace/fleet-event-acquirers.ts (return type stays compatible). The only behavioral regression risk is intentional: PRs whose GitHub state cannot be read now remain watched instead of being silently dropped. refetchForBaseSync still compares fresh.state !== "open" and continues to stop on missing state, which is the safe branch for base syncs.

Verification

• cd packages/code && bun run test agent-pr-reconciler.test.ts — 13 pass, 0 fail (initially one new assertion failed because the table test accumulated rows; fixed the expectation and re-ran green).
• cd packages/code && bun run test — 2533 pass, 0 fail across 168 files.
• cd packages/code && bun run --filter @getdevintern/code typecheck (via bun run typecheck filter form) — exit 0.
• Root bun run typecheck — all 12 package tasks successful; the trailing typecheck:examples step first hit an environmental nono sandbox IO/permission error, then bun run typecheck:examples re-ran successfully.
• bunx oxfmt --check on the reconciler, review-polling, and test files — all matched.
• bunx oxlint packages/code/src/lib/review packages/code/src/lib/acquirers packages/code/tests — no findings.
• git status confirms only the two intended files are modified; the queue database was not touched.

---

This PR was automatically created by @devintern/code

[danii1]

Artifacts & verification

Run: 57 · Commit: 47f7b7c2c0cab9bb9c571b30f9289f05c0c182f1

Change explanation

Produced by pr-explainer-narrative, execution 1 at 47f7b7c2c0cab9bb9c571b30f9289f05c0c182f1.

Keep agent PRs watched when GitHub omits or changes state

No PR template was found in the repository, so this note uses the requested sections.

Why this change is needed

The task reports that the review poller permanently unwatched a still-open agent PR, getdevintern/devintern#365, because GitHub returned a PR payload whose state was undefined. The reconciler treated that as "not open", marked the registry row closed, and listOpenAgentPrs() stopped returning it, so review and CI feedback were no longer polled and the row could not correct itself.

The task traces the cause to applyAgentPrFetch using state !== "open" as though it were a terminal-state check, a regression introduced by 22ced428 when the previous ?? "closed" fallback was removed. No motivation beyond the task description was inferred.

What changed

Terminal-state validation is now explicit. A new classifyAgentPrState(state: unknown) recognizes only open, closed, and merged; anything missing, null, empty, non-string, or unknown is "unusable" and returns null. Only closed and merged may close a registry row. PolledPr.state, CiChange.state, and PolledCiPr.state are now optional/unknown so consumers cannot silently assume a string.

Closure decisions use the classifier. applyAgentPrFetch leaves the row open and returns null for unusable state, so the poller can no longer emit undefined; unwatching. The CI failure watcher applies the same classifier before unwatching and before attempting a fix.

Unusable responses are retried rather than cached. The reconciler drops or does not persist the PR-state ETag when state is unusable, so the next tick sends an unconditional request instead of risking a 304 with no state to inspect. It still shares the fetched payload through fresh when a payload exists, so the normal path adds no extra request. The review poller also skips feedback collection while state is not open, leaving review/comment cursors and processed markers untouched so events are not deduplicated forever. The CI watcher defers fix attempts and does not consume its retry budget or escalate for an unconfirmed PR.

Diagnostics are bounded. The reconciler warns at most once per PR per process, naming repo#number and the observed shape (missing, null, empty string, unrecognised "...", unexpected <type>), never the full payload. Warning bookkeeping is released when the PR is later observed open or terminal, and pruned when a row leaves the open registry.

Validation actually performed

Observed in the pipeline evidence:

• The __verify-1 step recorded: targeted bun run test agent-pr-reconciler.test.ts (13 pass), full bun run test (2533 pass), package and root bun run typecheck (pass), bunx oxfmt --check on the three named files (pass), and bunx oxlint on review, acquirers, and tests (no output).
• The auto-review step also recorded 83 passing tests across the three affected suites plus clean typecheck, lint, and format.
• The tests added in the diff cover missing/undefined/null/empty/unknown/numeric/object/boolean state, ETag clearing, one-warning-per-PR behavior, re-warning after close and re-registration, open/closed/merged closure, reconciliation leaving rows open while populating fresh, CI watcher unwatch/ETag/deferred-fix behavior, and review polling deferring feedback until state is usable.

Not observed or not claimed:

• I did not independently re-run the suites while writing this note.
• The auto-review step reported success: false after 5 iterations. Its final feedback listed remaining gaps around CI-watcher ETag coverage, provider-neutrality, and a null-payload edge. The final diff adds a CI-watcher ETag regression test that appears to address the first gap; the other points were not declared resolved by the available evidence.

Material risks

• Acceptance-criteria conflict. The task's acceptance criteria say an unusable payload should "still update the ETag cursor." This implementation clears or drops that cursor, and its tests assert the cursor is null. The rationale is that persisting the ETag makes the next request conditional and a 304 carries no state, so the PR would remain watched but never be re-read. Reviewers should confirm this deliberate deviation is preferred over the literal criterion. The trade-off is one unconditional GitHub request per affected PR per tick while state remains unusable.
• Broader than the stated scope. The change also modifies the CI failure watcher and widens shared CI provider types (CiChange.state, PolledCiPr.state), beyond the review-poller boundary described in the task.
• unknown typing is not enforcement. Raw state comparisons remain in other modules (webhook-server.ts, mention-sweep.ts, conflict-resolver.ts, review-adapter.ts, and the poller's base-sync check). On the inspected paths those comparisons skip work or reject an action rather than unwatch, but they do not share the new semantics. This was observed by grep; their runtime behavior was not re-tested here.
• 304 with no cached payload. In review-polling.ts:825-827, a 304 with no entry in prCache defaults the lifecycle to open and proceeds with feedback collection. That preserves prior behavior but is not an explicit confirmation of state. This is an inference from reading the code, not a covered test in the diff.
• CI-only unusable state has no warning. The bounded diagnostic lives in the reconciler; the CI watcher classifies state but does not emit the warning. This is an inference from the diff.

Where reviewers should focus

1. packages/code/src/lib/review/agent-pr-reconciler.ts — classifyAgentPrState, the unusable-state branch (cursor clearing, warning, row left open), and the terminal closure return.
2. packages/code/src/lib/acquirers/ci-failure-watcher.ts — lifecycle handling, ETag persistence guard, and the early return that defers CI fixes.
3. packages/code/src/lib/acquirers/review-polling.ts — the lifecycle !== "open" feedback guard and the 304/no-cache default.
4. Tests — confirm the new assertions match the intended ETag trade-off and that the CI ETag regression test exercises the guard, not only the no-unwatch path.

Suggested checks, not performed here: run the task's verification commands and add coverage for the 304-with-empty-cache path if that edge matters.

Visual explanation

Produced by pr-explainer-visual, execution 1 at 47f7b7c2c0cab9bb9c571b30f9289f05c0c182f1.

Show artifact

Summary

GitHub can return an agent PR payload with a missing, null, or unrecognized state, and the reconciler treated anything other than "open" as terminal, permanently unwatching still-open PRs so review and CI feedback stopped being polled. This change adds classifyAgentPrState, which only treats closed and merged as terminal, and routes closure decisions in the reconciler and CI watcher through it. Unusable state now keeps the row watched, avoids caching an ETag that could produce a state-less 304, and emits at most one bounded warning per PR per process. The review poller also defers feedback collection while state is unconfirmed, so events are not marked processed and then lost.

Evidence

observed state              classifyAgentPrState   registry action
--------------------------  ---------------------  --------------------------------
"open"                      "open"                 keep watched
"closed" / "merged"         "closed" / "merged"    mark closed, stop watching
missing / null / "" /       null                   keep watched, do not cache ETag,
unknown / non-string                               warn once per PR per process

Validation recorded in the pipeline evidence:

• __verify-1: targeted bun run test agent-pr-reconciler.test.ts reported 13 pass; full bun run test reported 2533 pass; package and root bun run typecheck passed; bunx oxfmt --check on the three named files passed; bunx oxlint on review, acquirers, and tests produced no output.
• auto-review: reported 83 passing tests across the three affected suites, plus clean typecheck, lint, and format.
• The same auto-review reported success: false after 5 iterations, citing gaps around CI-watcher ETag coverage, provider-neutrality for non-GitHub states, and a null-payload edge. The final diff includes a CI-watcher ETag regression test that appears to address the first gap; the other items are not shown as resolved.
• Before/after execution results for the original bug were not captured. No screenshots, benchmark numbers, or reproduction logs are available in the evidence.

The new tests in the diff cover missing, undefined, null, empty, unknown, numeric, object, and boolean states; row retention; ETag clearing; one-warning-per-PR behavior; terminal closed/merged; sharing through fresh; CI watcher deferral; and review feedback deferral.

Merge Danger

Reverting the change restores state !== "open" as the closure test, immediately reintroducing the original failure: a live PR with omitted state is marked closed and stops being polled. Rows already marked closed are not automatically reopened, so recovery would require editing the queue database, which this task explicitly excludes. The change touches shared PR/CI types consumed by the review poller, reconciler, and CI watcher, so reverting also reverts the compiler pressure that forces callers to handle absent state.

The change adds no schema, auth, or configuration migration, so it is reasonable to ship without a staged rollout. Operational effects are limited to retaining watched rows, one extra unconditional PR fetch per tick while a PR's state remains unusable, and a single warning per affected PR per process. One deliberate deviation from the task's acceptance criteria should be reviewed: the implementation clears rather than updates the ETag cursor for unusable state, because a cached ETag could yield a 304 carrying no state and leave the PR watched but never re-read. The new tests assert the cursor is null in that case.

Verification report

Produced by __verify-1, execution 1 at 5d30326fa39e5284b4f7943dab41d75a27680d75.

Show artifact

{
  "summary": "Implementation satisfies the functional requirements and acceptance criteria. `applyAgentPrFetch` now validates a narrow terminal-state set, leaves rows watched on missing/null/empty/unknown state, still persists ETags, keeps sharing fetch results through `fresh`, and emits one bounded warning per PR per process. Verified commands: targeted `bun run test agent-pr-reconciler.test.ts` (13 pass), full `bun run test` (2533 pass), package and root `bun run typecheck` (pass), `bunx oxfmt --check` on the three files (pass), and `bunx oxlint` on review/acquirers/tests (no output).",
  "items": [
    {
      "priority": "info",
      "confidence": "high",
      "category": "code-quality",
      "file": "packages/code/src/lib/acquirers/review-polling.ts",
      "line": "920",
      "issue": "Blast-radius safety: widening `PolledPr.state` to optional does not break sibling consumers. The only consumers of `PolledPr` are the reconciler and `review-polling.ts`; `refetchForBaseSync` already treats anything other than `\"open\"` as a stop, and the CI watcher uses the separate `PolledCiPr` type.",
      "evidence": "Ran `grep -rn \"PolledPr\" packages --include=*.ts`: only `review-polling.ts`, the reconciler, and tests reference it. Ran full root `bun run typecheck`: all 12 packages passed. `review-polling.ts:920-926` already guards with `fresh.state !== \"open\"`.",
      "suggestion": "No change needed; the compiler now forces any future consumer to handle absent state explicitly."
    },
    {
      "priority": "info",
      "confidence": "medium",
      "category": "bug",
      "file": "packages/code/src/lib/acquirers/ci-failure-watcher.ts",
      "line": "583",
      "issue": "The same unguarded `state !== \"open\"` pattern remains in the CI-failure watcher, which could still log `undefined; unwatching` if GitHub omits state there. This is pre-existing and outside the task's review-poller scope, but the bug class is not eliminated process-wide.",
      "evidence": "Read `packages/code/src/lib/acquirers/ci-failure-watcher.ts:583-586`: checks `prResult.data.state !== \"open\"` and interpolates `prResult.data.state` directly; `PolledCiPr.state` remains required at `packages/code/src/lib/code-host/github/ci-provider.ts:8`.",
      "suggestion": "Consider a separate follow-up applying the same terminal-state classifier to `PolledCiPr` (and analogous GitLab snapshot state); do not widen this change beyond the review-poller scope."
    },
    {
      "priority": "low",
      "confidence": "medium",
      "category": "testing",
      "file": "packages/code/tests/agent-pr-reconciler.test.ts",
      "line": "177",
      "issue": "The unusable-state test asserts open/ETag behavior for missing, undefined, null, empty, and unknown states, but warning emission/deduplication is asserted only for the missing-state shape. This does not violate acceptance criteria, but warning coverage for each unusable shape is indirect.",
      "evidence": "Read `packages/code/tests/agent-pr-reconciler.test.ts:177-208` (behavior for all shapes) and `:210-231` (warning spy only for `{ data: {} }`, repeated three times for PR 30).",
      "suggestion": "Optionally extend the warning spy test to loop over each unusable shape, or add direct unit tests for `classifyAgentPrState`; not required for acceptance."
    }
  ],
  "approved": true
}

Further artifacts omitted because they exceed the comment preview budget.