diff --git a/.gitignore b/.gitignore index f46d320..b37f8ef 100644 --- a/.gitignore +++ b/.gitignore @@ -25,3 +25,15 @@ # Agent-doc, local-only (untracked 2026-06-21) PLAYBOOK.md + +# Repo-local gstack state stays untracked EXCEPT the QA recipe, which the +# qa:browser recipe schema requires committed (see +# qa/skills/browser/references/recipe-schema.md). The leading `!.gstack/` +# keeps the DIRECTORY includable (the machine-global ignore excludes +# `.gstack/` wholesale, and git cannot re-include children of an excluded +# dir); `.gstack/*` then untracks its contents except the recipe path. +!.gstack/ +.gstack/* +!.gstack/qa-quincey/ +.gstack/qa-quincey/* +!.gstack/qa-quincey/recipe.yml diff --git a/eng/.claude-plugin/plugin.json b/eng/.claude-plugin/plugin.json index 9360831..a2c5ca4 100644 --- a/eng/.claude-plugin/plugin.json +++ b/eng/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "eng", - "version": "2.6.5", + "version": "2.7.0", "description": "Engineer Ernie, the engineering persona. eng:cr is his master code-review skill and the single local review path the ~/dev merge gate keys on: it risk-tiers depth, runs the pr-review-toolkit lenses, and mints the merge-clearance stamp. It routes to cr-teammate (review someone else's PR and post a comment) and to address-pr-feedback / pr-watcher (respond to review feedback). He also spikes the riskiest unknown before building, generates CodeRabbit config, and builds macOS Shortcuts. The plugin also SHIPS Ernie's PR-lifecycle enforcement hooks (hooks/hooks.json): the ship-PR gate (PRs only via /ship), the merge-clearance gate (no merge without the cleared gauntlet AND a /land-and-deploy sentinel, making /land-and-deploy the single CLI merge path), the /ship and /land-and-deploy sentinels, the review stamp recorder, and an after-ship CodeRabbit-watcher nudge (after a genuine /ship opens a PR, points the agent at /eng:pr-watcher, or when CodeRabbit is rate-limited routes to /land-and-deploy if a current /eng:cr review backstops the head, else to /eng:cr and then /land-and-deploy), active in opted-in repos (.ship-gate.json / .merge-clearance.json) under ~/dev. Skills: eng:cr, eng:cr-teammate, eng:address-pr-feedback, eng:pr-watcher, eng:spike, eng:coderabbit-config, eng:shortcut.", "author": { "name": "Mujtaba Badat", diff --git a/eng/hooks/scripts/ship-watch-nudge-lib.sh b/eng/hooks/scripts/ship-watch-nudge-lib.sh index d6e64ab..9c2a673 100755 --- a/eng/hooks/scripts/ship-watch-nudge-lib.sh +++ b/eng/hooks/scripts/ship-watch-nudge-lib.sh @@ -9,8 +9,8 @@ # opens a PR in an opted-in ~/dev repo, it decides WHAT to nudge the main agent # toward and builds the additionalContext string. # -# The "why": a hook cannot launch a foreground skill (/eng:pr-watcher pairs the -# main agent with a sensor subagent), but it CAN inject additionalContext that the +# The "why": a hook cannot launch a foreground skill (/eng:pr-watcher's dispatcher +# loop runs in the main agent's own turn), but it CAN inject additionalContext that the # model reads next turn. So the durable, reliable mechanism is an auto-NUDGE, not # an auto-run. The nudge is rate-limit-aware: it must not push the agent into an # open-ended watch loop when CodeRabbit will not actually review (rate-limited). diff --git a/eng/hooks/scripts/ship-watch-nudge.sh b/eng/hooks/scripts/ship-watch-nudge.sh index 447cae8..0c2cd43 100755 --- a/eng/hooks/scripts/ship-watch-nudge.sh +++ b/eng/hooks/scripts/ship-watch-nudge.sh @@ -5,8 +5,8 @@ # shipped PR unwatched. # # Why a NUDGE and not an auto-run: a hook cannot launch a foreground skill. The -# canonical watcher /eng:pr-watcher pairs the main agent (dispatcher + fix-applier) -# with a passive sensor subagent, so it can only run in the main turn. A PostToolUse +# canonical watcher /eng:pr-watcher runs its dispatcher loop (and its foreground +# sensor script) in the main agent's own turn, so only the model can start it. A PostToolUse # hook CAN return additionalContext that the model reads next turn - verified against # Claude Code 2.1.x: {"hookSpecificOutput":{"hookEventName":"PostToolUse", # "additionalContext":"..."}} on stdout with exit 0 reliably reaches the model. So diff --git a/eng/skills/pr-watcher/CHANGELOG.md b/eng/skills/pr-watcher/CHANGELOG.md index e859d58..aa12099 100644 --- a/eng/skills/pr-watcher/CHANGELOG.md +++ b/eng/skills/pr-watcher/CHANGELOG.md @@ -1,6 +1,34 @@ # eng:pr-watcher changelog -## Unreleased +## v4 - deterministic sensor script (no more sensor subagent) + +The sensor is now `scripts/sensor-poll.sh`, a deterministic bash script the +dispatcher runs in FOREGROUND Bash slices (~9 min each, `continue` outcome + +`sensor-state.json` spanning the 30-minute cycle budget), printing exactly one +JSON object per invocation. The general-purpose sensor subagent is removed. + +Why: the subagent contract ("block 30 minutes in one agent turn, end with one +JSON") was structurally unsatisfiable. Foreground sleep is blocked for agents, +so the model reached for background tasks and Monitor, both of which END the +agent's turn, which the dispatcher reads as the final answer. Observed live on +email-hero PR 79 (2026-07-20): the sensor parked twice on monitors whose +conditions fired correctly within ~1 minute of CodeRabbit finishing, with no +agent left to consume them, while the dispatcher waited 10+ minutes. A script +that sleeps internally satisfies the one-JSON contract by construction and +removes the prompt-drift surface entirely. + +Also in v4: pure decision logic extracted to `scripts/sensor-poll-lib.sh` with +bats coverage (`tests/sensor-poll.bats`); robust gh/jq resolution under Claude +Code's stripped PATH (the incident's first poll script died on a hardcoded +`/opt/homebrew/bin/jq`); persistent-API-failure ticks surface as a new +`outcome: error` with `error_message` instead of hanging. The v3 protocol +semantics (status-primary polling, comment-stream fallback, init-pass +`already_settled` / `cr_failure` / backlog-drain branches, settle conditions) +carry over, with one deliberate broadening: the `Actionable comments posted:` +fallback settle marker now matches any new CR item body, not only review +bodies. + +Also shipping with v4 (was pending as Unreleased): - Dropped the test-command gate from the skill contract. The watcher no longer asks for or runs a test command before pushing a CR-induced fix. Friction diff --git a/eng/skills/pr-watcher/SKILL.md b/eng/skills/pr-watcher/SKILL.md index 4a3239c..8d03a44 100644 --- a/eng/skills/pr-watcher/SKILL.md +++ b/eng/skills/pr-watcher/SKILL.md @@ -1,6 +1,6 @@ --- name: pr-watcher -description: Foreground watcher that pairs the main agent (dispatcher and fix-applier) with a passive polling subagent (sensor) to handle CodeRabbit feedback on a GitHub PR. The sensor blocks silently in one Agent call until CR posts a settled round of feedback, then returns a single JSON blob. The main agent classifies, applies fixes, runs tests, commits, pushes, and replies on the PR itself, then spawns the next sensor. Never merges; never resolves conversations; never pushes without passing tests; never touches files outside the PR's own diff; never parallelizes fixes. Use when asked to "watch the PR", "watch coderabbit", "pr watch", or invoked manually as `/eng:pr-watcher ` after /ship. +description: Foreground watcher that pairs the main agent (dispatcher and fix-applier) with a deterministic polling script (the sensor, scripts/sensor-poll.sh) to handle CodeRabbit feedback on a GitHub PR. The dispatcher runs the script in foreground Bash slices; it blocks until CR posts a settled round of feedback (or a budget expires) and prints exactly one JSON blob per invocation. The main agent classifies, applies fixes, runs tests, commits, pushes, and replies on the PR itself, then starts the next sense cycle. Never merges; never resolves conversations; never pushes without passing tests; never touches files outside the PR's own diff; never parallelizes fixes. Use when asked to "watch the PR", "watch coderabbit", "pr watch", or invoked manually as `/eng:pr-watcher ` after /ship. --- ## Update check (run first) @@ -24,19 +24,20 @@ You are running the `/eng:pr-watcher` skill. It watches a single PR for CodeRabb **Where this sits in the family.** `eng:cr` *performs* reviews; this skill and `eng:address-pr-feedback` *respond* to them. This skill is the **autonomous** responder: it polls and auto-handles each settled CodeRabbit round. `eng:address-pr-feedback` is the **manual** sibling for working comments one at a time with explicit lesson capture. They share the same job (respond to review feedback on your own PR); pick autonomous vs manual by whether you want to walk away or stay in the loop. When the watcher escalates an item it cannot handle (`needs_user_input`), `eng:address-pr-feedback` is the natural follow-up for working it by hand. -## Architecture: dispatcher + sensor +## Architecture: dispatcher + deterministic sensor script This skill splits work between two roles: - **Main agent (you) = dispatcher + gate + fix-applier.** You read the sensor's JSON, classify each finding, apply fixes with Edit/Write, run tests, commit, push, and post PR replies. You hold all user-facing decisions. -- **Sensor subagent = pure polling sensor.** Spawned via the Agent tool, it blocks for up to 30 minutes waiting for CR to post AND settle a new round of feedback, then returns ONE JSON blob. It never edits files, never runs git, never writes on the PR. +- **Sensor = `scripts/sensor-poll.sh`, a deterministic script.** You run it in a FOREGROUND Bash call. It implements the whole polling protocol (init pass, status-primary 15s loop, comment-stream fallback, settle conditions, budgets) and prints EXACTLY ONE JSON object per invocation. It never edits files, never runs git, never writes on the PR (read-only `gh` calls). -Loop: spawn sensor, await return, process batch in the main turn, spawn the next sensor with updated baseline IDs. Repeat until merged / closed / user stops / sensor returns `idle_timeout` and you decide to stop. +Loop: run one sense cycle (foreground script, sliced; see Step 3), process the batch in the main turn, update baselines, run the next sense cycle. Repeat until merged / closed / user stops / the sensor returns `idle_timeout` and you decide to stop. -Why this shape: -- One Agent call can wait up to 30 minutes for real signal, instead of the main agent re-entering a 9-minute Bash poll every cycle. +Why a script and not a subagent (v4; the v2/v3 sensor was a general-purpose subagent): +- The old contract ("block 30 minutes inside one agent turn, then return one JSON") is unsatisfiable with harness primitives: foreground `sleep` is blocked for agents, and both background tasks and Monitor END the agent's turn, which the dispatcher reads as the sensor's final answer. In the 2026-07-20 incident (email-hero PR 79) the sensor parked twice on monitors whose conditions fired correctly within a minute of CR finishing, with no agent left to consume them. +- A script that sleeps INTERNALLY runs fine in one foreground Bash call, so "one command in, one JSON out" holds by construction. No prompt for a model to drift on, no turn to end early, and no second agent burning tokens to babysit a loop. - Main context absorbs one short JSON per cycle, not minutes of "tick" lines. -- One sensor at a time + main-owned git = zero risk of concurrent edits on the branch. +- One sense cycle at a time + main-owned git = zero risk of concurrent edits on the branch. ## What this skill WILL NOT do @@ -204,203 +205,62 @@ Print a one-line start banner: 🐇 Watching PR # (/). Timeout: h. Ctrl-C to stop. ``` -## Step 3: SENSE — spawn one passive polling subagent +## Step 3: SENSE - run the sensor script in the foreground -This is the single Agent call per cycle. Read current baselines, capture the latest pushed SHA for log clarity, then spawn the sensor and await its return. The main agent stays silent until the sensor returns — no per-minute output in the transcript. +Sensing is one deterministic script, run as a foreground Bash command. The script reads the baselines straight from `$STATE_DIR` (no input marshalling) and prints one JSON object per invocation. -Resolve the inputs: +The sensor's primary signal is CodeRabbit's **commit status** (legacy GitHub Statuses API): CR posts a `CodeRabbit` context status on each new HEAD commit that transitions `pending` → `success`/`failure` when its review pass finishes. That single endpoint is cheap, so the script polls it every 15s and fetches the three comment streams only when the status transitions. When there is no CR commit status to watch (a repo whose CR setup never posts one, or a status stuck in `pending` while comments still arrive), it falls back to comment-stream polling every ~60s with the marker / quiet-period settle conditions. The init pass (before the loop) returns immediately when CR is already terminal on the current HEAD: `already_settled` (success, nothing unprocessed), `cr_failure` (failure/error, nothing unprocessed), or `new_cr_feedback` (unprocessed backlog with a settle condition already holding). + +Resolve the script from the installed plugin (repo checkout as fallback) and start the cycle fresh: ```bash -BASE_ISSUE=$(cat "$STATE_DIR/baseline_issue_comments.json") -BASE_REVIEW=$(cat "$STATE_DIR/baseline_reviews.json") -BASE_RCOMMENT=$(cat "$STATE_DIR/baseline_review_comments.json") -HEAD_SHA=$(git rev-parse HEAD 2>/dev/null || echo "unknown") +SENSOR="${CLAUDE_PLUGIN_ROOT:-$HOME/dev/gstack-extensions/eng}/skills/pr-watcher/scripts/sensor-poll.sh" +rm -f "$STATE_DIR/sensor-state.json" # new sense cycle: the init pass runs again ``` -Spawn the sensor via the Agent tool: +Then run the slice loop, repeating this SAME command while the printed outcome is `"continue"`: + +```bash +"$SENSOR" --owner "$OWNER" --repo "$REPO" --pr "$PR_NUM" --state-dir "$STATE_DIR" +``` -- `subagent_type`: `"general-purpose"` -- `description`: `"eng:pr-watcher sensor: PR #"` -- `run_in_background`: omit (must default to false) -- `prompt`: the sensor template below, with placeholders substituted verbatim +Slice mechanics (why "continue" exists): a foreground Bash call caps at 10 minutes, so the script returns within ~9 minutes per invocation (`--slice-seconds 540`) and spans the cycle's 30-minute budget (`--total-seconds 1800`) across at most 4 invocations, persisting its place in `$STATE_DIR/sensor-state.json` between them. Rules: -### Sensor prompt template (paste verbatim, substitute bracketed values) +- Run it FOREGROUND: pass `timeout: 600000` on the Bash call and OMIT `run_in_background`. +- `"continue"` is not a failure and needs no user interaction: immediately run the same command again. +- NEVER wrap the script in a background task, a Monitor, or an Agent subagent. The v3 sensor subagent parked exactly that way (turn ended, JSON never arrived) while CodeRabbit was already finished; sensing is this one repeated foreground command, by design. -The sensor uses CodeRabbit's **commit status** (legacy GitHub Statuses API) as its -primary signal: CR posts a `CodeRabbit` context status on each new HEAD commit -that transitions `pending` → `success`/`failure` when its review pass finishes. -That single endpoint is cheap to poll and gives a clear "review just finished" -edge. Comment-stream fetches happen only when the status transitions. +### Sensor output schema -If a repo's CR setup does not post a commit status (some self-hosted or older -installs), the sensor falls back to comment-stream polling at the original 60s -cadence so the watcher still works. +One JSON object on stdout per invocation. Full comment bodies, no truncation. -```text -You are a passive polling sensor for the /eng:pr-watcher skill. Your only job is to -wait for CodeRabbit (coderabbitai[bot]) to finish a review pass on -PR [PR_URL], then return a short summary. You do NOT edit code, push commits, -or reply on the PR. - -Treat these IDs as already-known baseline (do NOT report them as new): -- issue_comments: [BASE_ISSUE] -- reviews: [BASE_REVIEW] -- review_comments: [BASE_RCOMMENT] - -Most recent pushed commit at watch start (for log context only): [HEAD_SHA] - -Primary signal — CodeRabbit commit status: -CR posts a legacy commit status with context "CodeRabbit", creator -coderabbitai[bot], state pending → success (or failure). Each new push to the -PR head triggers a fresh pending → terminal transition. The endpoint is -GET /repos/[OWNER]/[REPO]/commits//statuses and is cheap, so the -sensor polls it at a tight cadence and only fetches comment streams when the -status flips. - -Polling protocol: -0. Init pass (run ONCE before the 15s loop, immediately on spawn): - a. Resolve current PR head: - gh pr view [PR_NUM] --repo [OWNER]/[REPO] --json state,headRefOid - If state is MERGED or CLOSED, return outcome: pr_closed immediately. - Let CURRENT_SHA = headRefOid. - b. Fetch the commit status list for CURRENT_SHA (same endpoint and filter as - step 2b below). Let INIT_CR_STATUS = (state, updated_at) of the latest - CodeRabbit entry, or null if absent. - c. Fetch the three comment streams once and filter to coderabbitai[bot] - items NOT in the baseline (same as step 3). Call the result INIT_NEW. - d. If INIT_CR_STATUS.state == "success" AND INIT_NEW is empty across all - three streams, CR is already caught up cleanly on this HEAD. Return - outcome: already_settled IMMEDIATELY with cr_status_state = "success", - cr_status_updated_at = INIT_CR_STATUS.updated_at, settled_via: "n/a", - and empty new_* arrays. Do NOT wait 30 minutes. - d2. If INIT_CR_STATUS.state in ("failure", "error") AND INIT_NEW is empty - across all three streams, CR's review on the current HEAD ended in - failure with no actionable comments to drain. Return outcome: - cr_failure IMMEDIATELY with cr_status_state set to the failure state - and cr_status_updated_at = INIT_CR_STATUS.updated_at. Do NOT silently - fall into the 15s loop: CR has emitted its final word on this HEAD and - no new transition will arrive without a new push, so polling would idle - to timeout. The dispatcher surfaces the failure to the user and ends - the skill (see the dispatcher branch table below). If a new push is - made later, the user can re-invoke /eng:pr-watcher to start a fresh watch. - e. If INIT_NEW is non-empty AND at least one settling condition holds for - INIT_NEW, return outcome: new_cr_feedback IMMEDIATELY so the dispatcher - can drain the backlog. The settling conditions are the same ones the - 15s loop applies in steps 3 and 4, evaluated against the current state: - (i) INIT_CR_STATUS.state is terminal (success/failure/error) → return - with settled_via: "status_transition"; OR - (ii) any item in INIT_NEW contains the literal sentinel - `` - → return with settled_via: "marker"; OR - (iii) all items in INIT_NEW have an updated_at at least 180 seconds - older than the current time → return with settled_via: "quiet_period". - If INIT_NEW is non-empty but NONE of (i)/(ii)/(iii) holds, CR is - mid-review and returning now would surface a partial batch. Do NOT - return; fall through to step 0f so the 15s loop can wait for the next - proper settling signal before draining INIT_NEW. (The 15s loop's - step 3 also re-fetches comment streams once a terminal transition is - detected, so no items are lost by waiting.) - f. Otherwise (status is pending or absent, no new items): set - last_terminal_status_updated_at = (INIT_CR_STATUS.updated_at if state - is terminal, else null) and proceed to the 15s loop. Note: when status - is terminal but seeding-baselines-from-current produced an empty INIT_NEW, - we will have already returned via 0d above. -1. Initialize fallback_tick_counter = 0. -2. Every 15 seconds: - a. Resolve current PR head via - gh pr view [PR_NUM] --repo [OWNER]/[REPO] --json state,headRefOid - If state is MERGED or CLOSED, return outcome: pr_closed immediately. - Let CURRENT_SHA = headRefOid. - b. Fetch the commit status list for CURRENT_SHA: - gh api "repos/[OWNER]/[REPO]/commits/$CURRENT_SHA/statuses?per_page=100" - Filter to context == "CodeRabbit" AND creator.login == "coderabbitai[bot]" - (creator may be missing on some entries; treat that as a match too if the - context is "CodeRabbit"). Take the entry with the latest updated_at. - Call its (state, updated_at) the LATEST_CR_STATUS. - c. If LATEST_CR_STATUS is present and state in ("success", "failure", "error") - AND (last_terminal_status_updated_at is null OR - LATEST_CR_STATUS.updated_at > last_terminal_status_updated_at), this is - a fresh review transition. Proceed to step 3. (The null check is - load-bearing: when the watcher enters the 15s loop with a pending or - absent status at init, last_terminal_status_updated_at starts as null, - and `updated_at > null` is falsy in every common runtime. Without - treating null as "no prior terminal seen," the first terminal status - that lands during polling would never trigger a transition, exactly - reproducing the 30-minute stale-wait this version was meant to kill.) - d. If LATEST_CR_STATUS is absent (no CR status on this SHA at all) OR is - present-but-non-terminal (state == "pending"), increment - fallback_tick_counter. Every 4th tick (every ~60s), fall through to - step 4 (comment-stream poll) so we still notice activity when CR - doesn't post a commit status AND when CR posts a `pending` status that - never transitions (rare CR-side hang where comments may still arrive - via the streams even though the status is stuck). - e. Otherwise (status is terminal but not a fresh transition since - last_terminal_status_updated_at) sleep until the next 15s tick. -3. Status just transitioned to terminal. Set - last_terminal_status_updated_at = LATEST_CR_STATUS.updated_at. - Wait 5 seconds to let CR's comment writes settle (status sometimes flips - slightly before the last review_comment write is visible to the API), then - fetch all three streams once: - gh api "repos/[OWNER]/[REPO]/issues/[PR_NUM]/comments?per_page=100" - gh api "repos/[OWNER]/[REPO]/pulls/[PR_NUM]/reviews?per_page=100" - gh api "repos/[OWNER]/[REPO]/pulls/[PR_NUM]/comments?per_page=100" - Filter each to user.login == "coderabbitai[bot]" and to IDs not in the - baseline. If any new items are present, return outcome: new_cr_feedback with - settled_via: "status_transition". If zero new items (status flipped to - success but CR posted nothing actionable, e.g. a 0-finding pass), still - return new_cr_feedback so the dispatcher can mark the round seen; the - dispatcher will classify everything as nitpick_only / status_ping and move - on. -4. Fallback: same comment-stream fetch as step 3, plus a freshness check using - the original quiet_period logic — return new_cr_feedback when there is at - least one new CR item AND either: - (a) a new review body matches ^Actionable comments posted:, OR - (b) a new comment/review body contains the literal sentinel - "", OR - (c) 180 seconds have passed since the latest new item's effective - timestamp with no further changes in a subsequent poll. Effective - timestamp = `updated_at` if present, else `submitted_at` (the field - GitHub review objects expose) as a fallback. For repos that don't - post a CR commit status, batches containing only a review object - depend on this fallback path; without the submitted_at fallback the - age calculation never resolves and the watcher misses the settling. - Reset fallback_tick_counter to 0 after each fallback fetch. -5. After 1800 seconds (30 minutes) wall-clock with no terminal status - transition and no qualifying fallback activity, return outcome: - idle_timeout. - -Emit EXACTLY ONE JSON object as your final message. No prose before or after. -Full comment bodies, no truncation. - -Schema: +```json { - "outcome": "new_cr_feedback" | "pr_closed" | "idle_timeout" | "already_settled" | "cr_failure", - "polled_for_seconds": , - "ticks": , + "outcome": "new_cr_feedback" | "pr_closed" | "idle_timeout" | "already_settled" | "cr_failure" | "continue" | "error", + "polled_for_seconds": 0, + "ticks": 0, "head_sha_at_return": "", - "cr_status_state": "pending" | "success" | "failure" | "error" | null, + "cr_status_state": "pending | success | failure | error | null", "cr_status_updated_at": "", - "settled_via": "status_transition" | "marker" | "quiet_period" | "n/a", - "new_issue_comments": [{"id":"...","updated_at":"...","body":"..."}, ...], - "new_reviews": [{"id":"...","state":"...","submitted_at":"...","body":"..."}, ...], - "new_review_comments": [{"id":"...","path":"...","line":N,"updated_at":"...","body":"..."}, ...] + "settled_via": "status_transition | marker | quiet_period | n/a", + "new_issue_comments": [{"id": "...", "updated_at": "...", "body": "..."}], + "new_reviews": [{"id": "...", "state": "...", "submitted_at": "...", "body": "..."}], + "new_review_comments": [{"id": "...", "path": "...", "line": 0, "updated_at": "...", "body": "..."}], + "error_message": "only present when outcome is error" } - -Hard limits: -- No file edits. No git commands. No PR writes (no gh pr comment, no gh api -X POST). -- Maximum 30 minutes wall-clock. -- One JSON object as your final message, nothing else. ``` After the sensor returns, branch on `outcome`: +- `"continue"` → the slice budget expired before CR settled: run the same sensor command again immediately (already covered by the slice loop above; it is not a failure and does not reach the decisions below). - `"pr_closed"` → print `PR is closed/merged. Watcher exiting.` and end the skill. -- `"already_settled"` → CodeRabbit's review on the current HEAD is terminal `success` and there are no unprocessed CR items. (Sensor returns this only for `success`, never for `failure`/`error`.) Print `🐇 CodeRabbit is caught up on HEAD (status: success). Nothing to address. Watcher exiting.` and end the skill. Do NOT loop again; spawning another sensor would just reproduce this outcome. -- `"cr_failure"` → CodeRabbit's review on the current HEAD ended in `failure` or `error` with no actionable comments to drain. CR has emitted its final word; no new transition will arrive without a new push. **Check for a rate-limit FIRST, before the genuine-failure exit below.** If CR's comments contain the `rate limited by coderabbit.ai` marker, this is not a real CR error: take the Step 4h rate-limited short-circuit instead of exiting to inspect. If a current `/eng:cr` review backstops this HEAD (`review-skill-head` == HEAD) the PR is clear to land via `/land-and-deploy`; otherwise run `/eng:cr` first, then land. Watching will not help, because CR will not review this HEAD without a new push. **Only if it is NOT a rate-limit** (a genuine CR failure): print `⚠️ CodeRabbit review on HEAD ended in (updated_at: ). No comments were posted; this typically indicates a CR-side problem (internal error, repo config). Watcher exiting; please inspect the PR and re-invoke /eng:pr-watcher after the next push.` and end the skill. Do NOT loop; another sensor would reproduce this outcome. -- `"idle_timeout"` → ask the user (via AskUserQuestion) whether to keep watching or stop. Default recommendation: **stop** (long silence after watcher start almost always means CR is done; the user can re-invoke /eng:pr-watcher when there is new activity). If they choose to keep watching, spawn another sensor. +- `"already_settled"` → CodeRabbit's review on the current HEAD is terminal `success` and there are no unprocessed CR items. (Sensor returns this only for `success`, never for `failure`/`error`.) Print `🐇 CodeRabbit is caught up on HEAD (status: success). Nothing to address. Watcher exiting.` and end the skill. Do NOT loop again; another sense cycle would just reproduce this outcome. +- `"cr_failure"` → CodeRabbit's review on the current HEAD ended in `failure` or `error` with no actionable comments to drain. CR has emitted its final word; no new transition will arrive without a new push. **Check for a rate-limit FIRST, before the genuine-failure exit below.** If CR's comments contain the `rate limited by coderabbit.ai` marker, this is not a real CR error: take the Step 4h rate-limited short-circuit instead of exiting to inspect. If a current `/eng:cr` review backstops this HEAD (`review-skill-head` == HEAD) the PR is clear to land via `/land-and-deploy`; otherwise run `/eng:cr` first, then land. Watching will not help, because CR will not review this HEAD without a new push. **Only if it is NOT a rate-limit** (a genuine CR failure): print `⚠️ CodeRabbit review on HEAD ended in (updated_at: ). No comments were posted; this typically indicates a CR-side problem (internal error, repo config). Watcher exiting; please inspect the PR and re-invoke /eng:pr-watcher after the next push.` and end the skill. Do NOT loop; another sense cycle would reproduce this outcome. +- `"idle_timeout"` → ask the user (via AskUserQuestion) whether to keep watching or stop. Default recommendation: **stop** (long silence after watcher start almost always means CR is done; the user can re-invoke /eng:pr-watcher when there is new activity). If they choose to keep watching, start another sense cycle. - `"new_cr_feedback"` → proceed to Step 4. +- `"error"` → the script's `gh` calls failed repeatedly (rate limit, expired auth, network) or its lib is missing; `error_message` says which. Count it as a sensor failure. -If the sensor fails to return parseable JSON, count it as a sensor failure. After **three consecutive sensor failures**, print an error and stop. +If the sensor prints unparseable output or `outcome: error`, count it as a sensor failure. After **three consecutive sensor failures**, print an error (include the last `error_message`) and stop. ## Step 4: PROCESS — classify, fix, push, reply (in the main turn) @@ -499,9 +359,9 @@ Also append any reply IDs you just posted (issue_comments for top-level replies, ### 4h. All-clear exit check -The loop's exit condition is "CodeRabbit has nothing left for us." Detect that here, before spawning the next sensor, so the watcher does not spin a 30-minute idle_timeout waiting for a transition that will never come. +The loop's exit condition is "CodeRabbit has nothing left for us." Detect that here, before starting the next sense cycle, so the watcher does not spin a 30-minute idle_timeout waiting for a transition that will never come. -Exit the skill (do NOT spawn another sensor) when ALL the following hold for the batch you just processed: +Exit the skill (do NOT start another sense cycle) when ALL the following hold for the batch you just processed: - `pushed_commits_this_batch == 0` (no `valid_actionable` finding made it through tests + commit + push this batch). If you pushed even once, CR will re-review the new HEAD, so do not exit. - The sensor returned `cr_status_state == "success"` AND `settled_via == "status_transition"`. (CR's terminal pass on the current HEAD finished cleanly. `failure`/`error` is also "done" in CR's sense, but signals a CR-side problem worth keeping the watcher alive for a human to inspect, so do not auto-exit on those.) @@ -512,7 +372,7 @@ Exit the skill (do NOT spawn another sensor) when ALL the following hold for the - Backstopped (`review-skill-head` == HEAD): a current `/eng:cr` review already covers this HEAD. Print `🐇 CodeRabbit is rate-limited (no real review on HEAD ); a current /eng:cr review backstops it. Nothing to watch. Land via /land-and-deploy.` and end the skill. - Not backstopped: print `🐇 CodeRabbit is rate-limited (no real review on HEAD ) and no current /eng:cr review backstops it. Run /eng:cr on this HEAD, then land via /land-and-deploy. Not watching further.` and end the skill. -Do NOT spawn another sensor in either case. Only when `cr_rate_limited` is NOT set does the genuine clean-exit below apply. +Do NOT start another sense cycle in either case. Only when `cr_rate_limited` is NOT set does the genuine clean-exit below apply. When the condition is met, print: @@ -522,19 +382,19 @@ When the condition is met, print: and end the skill. -Then return to Step 3 and spawn the next sensor. +Then return to Step 3 and run the next sense cycle. ## Stop conditions The skill exits when any of: - Sensor returns `outcome: pr_closed`. -- Sensor returns `outcome: already_settled` (CR already done on the current HEAD at sensor spawn). +- Sensor returns `outcome: already_settled` (CR already done on the current HEAD when the sense cycle started). - Step 4h all-clear check fires (CR's terminal pass on the current HEAD posted nothing actionable and we did not push during the batch). - Sensor returns `outcome: idle_timeout` and the user chooses to stop. - Wall-clock timeout reached (default 8h, override via `PR_WATCHER_TIMEOUT`). - User interrupts the session (Ctrl-C, /exit). -- Three consecutive sensor failures (malformed JSON, agent errors, etc.). +- Three consecutive sensor failures (unparseable output or `outcome: error`). On exit, print: @@ -551,6 +411,10 @@ On exit, print: baseline_reviews.json baseline_review_comments.json escalations.jsonl # append-only, one JSON per line + sensor-state.json # transient: sensor-poll.sh's place within ONE + # sense cycle (survives "continue" slices; removed + # on terminal outcomes; the dispatcher's rm at each + # cycle start is the backstop) ``` State is per-PR and persists across sessions. Re-invoking `/eng:pr-watcher` on the same PR after `/exit` resumes from the saved baselines, never re-processing items already handled. @@ -559,11 +423,11 @@ State is per-PR and persists across sessions. Re-invoking `/eng:pr-watcher` on t | Failure | Response | |---|---| -| `gh` rate-limited (HTTP 403 with `X-RateLimit-Remaining: 0`) | Sensor sleeps until the reset time reported by the header, then resumes. | -| `gh` returns 401 | Print `ERROR: gh auth expired. Run gh auth login.` Exit with status 1. | +| `gh` rate-limited or failing transiently | In the 15s loop the script tolerates failing ticks and keeps polling; after ~10 minutes of consecutive failures it returns `outcome: error` with the captured gh stderr in `error_message`. The init pass is tighter: 3 attempts ~15s apart, then `outcome: error` (a dead API at cycle start is likely auth/config, not weather). The dispatcher counts an `error` as a sensor failure. | +| `gh` returns 401 | Surfaces as `outcome: error` whose `error_message` carries gh's stderr; when it names 401/auth, print `ERROR: gh auth expired. Run gh auth login.` and stop instead of retrying. | | Concurrent push by a human | `git pull --rebase` once and retry; on conflict, revert and escalate that finding. | | PR force-pushed (head SHA changed) | Inline review comment IDs may become stale. On the next cycle, clear `baseline_review_comments.json` and re-seed from the current CR comments. | -| Sensor returns malformed JSON | Count as a sensor failure. After 3 consecutive failures, exit. | +| Sensor prints unparseable output or `outcome: error` | Count as a sensor failure. After 3 consecutive failures, exit. | ## What you (the running session) actually do @@ -571,7 +435,7 @@ State is per-PR and persists across sessions. Re-invoking `/eng:pr-watcher` on t 2. Step 1 → verify prereqs. 3. Step 2 → discover config (timeout, baselines). Seed baselines on first run. 4. Print the start banner. -5. Loop: spawn ONE sensor subagent (Step 3) → await its JSON → process the batch yourself (Step 4) → spawn the next sensor. +5. Loop: run ONE sense cycle (Step 3: the foreground sensor script, re-run while it says `continue`) → read its JSON → process the batch yourself (Step 4) → next sense cycle. 6. On any stop condition, print the summary and end. -Do not edit files in a sensor subagent. Do not call Agent with `run_in_background: true`. Do not spawn more than one sensor at a time. Do not merge the PR. Do not resolve conversations. +Do not run the sensor script in a background task, a Monitor, or an Agent subagent; it runs foreground, in your own turn. Do not run more than one sense cycle at a time. Do not merge the PR. Do not resolve conversations. diff --git a/eng/skills/pr-watcher/scripts/sensor-poll-lib.sh b/eng/skills/pr-watcher/scripts/sensor-poll-lib.sh new file mode 100644 index 0000000..61e9994 --- /dev/null +++ b/eng/skills/pr-watcher/scripts/sensor-poll-lib.sh @@ -0,0 +1,127 @@ +# Pure decision logic for sensor-poll.sh (the /eng:pr-watcher sensing script). +# Sourced by sensor-poll.sh and by tests/sensor-poll.bats. Every function here +# is side-effect free: JSON strings in, JSON/verdict strings out. Resolve +# siblings via BASH_SOURCE so the executing copy binds its own dependencies +# (repo checkout and plugin cache both work), matching the hooks convention. + +# The literal sentinel CodeRabbit embeds in its auto-generated review-status +# comments. Single source of truth for the marker settle condition. +SP_CR_SENTINEL='' + +# sp_latest_cr_status +# From a GET /commits//statuses payload, pick the latest CodeRabbit +# entry: context == "CodeRabbit" AND (creator.login == "coderabbitai[bot]" +# OR creator missing; some entries omit it). Echoes compact JSON +# {"state":...,"updated_at":...} or the literal "null" when absent. +sp_latest_cr_status() { + jq -c ' + [.[] | select(.context == "CodeRabbit") + | select((.creator.login // "coderabbitai[bot]") == "coderabbitai[bot]")] + | sort_by(.updated_at) | last + | if . == null then null else {state, updated_at} end + ' <<<"$1" +} + +# sp_filter_new +# Filter a comment-stream payload to coderabbitai[bot] items whose stringified +# id is NOT in the baseline array, mapped to the sensor schema fields for +# (issue_comments | reviews | review_comments). Echoes a JSON array. +sp_filter_new() { + local kind="$1" stream="$2" baseline="$3" fields + case "$kind" in + issue_comments) fields='{id: (.id|tostring), updated_at, body}' ;; + reviews) fields='{id: (.id|tostring), state, submitted_at, body}' ;; + review_comments) fields='{id: (.id|tostring), path, line, updated_at, body}' ;; + *) echo "sp_filter_new: unknown kind: $kind" >&2; return 2 ;; + esac + jq -c --argjson base "$baseline" \ + "[.[] | select(.user.login == \"coderabbitai[bot]\") + | select((.id|tostring) as \$id | \$base | index(\$id) | not) + | $fields]" <<<"$stream" +} + +# sp_fresh_transition +# Is this status a NEW terminal transition worth draining? Yes when state is +# terminal AND we have not already consumed a terminal status at or after +# this timestamp. The empty-last check is load-bearing: with no prior +# terminal seen, the first terminal status must fire (the v3 sensor's +# "updated_at > null is always false" bug). ISO-8601 Z timestamps compare +# correctly as strings. Echoes yes/no; return code matches. +sp_fresh_transition() { + local state="$1" updated="$2" last="$3" + case "$state" in success|failure|error) ;; *) echo "no"; return 1 ;; esac + if [ -z "$last" ] || [ "$last" = "null" ] || [[ "$updated" > "$last" ]]; then + echo "yes"; return 0 + fi + echo "no"; return 1 +} + +# sp_fallback_settled +# Fallback settle conditions (a)+(b) for repos/rounds without a status +# transition: any new item whose body starts with "Actionable comments +# posted:" or contains the CodeRabbit review-status sentinel. Echoes yes/no. +sp_fallback_settled() { + local verdict + verdict=$(jq -r --arg sentinel "$SP_CR_SENTINEL" ' + any(.[]; (.body // "") | (test("^Actionable comments posted:") or contains($sentinel))) + ' <<<"$1") + if [ "$verdict" = "true" ]; then echo "yes"; return 0; fi + echo "no"; return 1 +} + +# sp_all_quiet +# Fallback settle condition (c): every item's effective timestamp +# (updated_at, else submitted_at; review objects only expose the latter) is +# at least old. Empty arrays are NOT quiet (nothing to +# drain), and an item with no parseable timestamp blocks quietness rather +# than counting as vacuously old. Echoes yes/no. +sp_all_quiet() { + local items="$1" now="$2" quiet="$3" verdict + verdict=$(jq -r --argjson now "$now" --argjson quiet "$quiet" ' + if length == 0 then false + else all(.[]; + ((.updated_at // .submitted_at // null) + | if type == "string" then (try fromdateiso8601 catch null) else null end + ) as $e | $e != null and $e <= ($now - $quiet)) + end + ' <<<"$items") + if [ "$verdict" = "true" ]; then echo "yes"; return 0; fi + echo "no"; return 1 +} + +# sp_fingerprint +# Stable fingerprint of a new-item set (ids + effective timestamps), used to +# detect "no further changes between fallback polls" for the quiet-period +# settle. Echoes a hex digest. +sp_fingerprint() { + printf '%s\n%s\n%s\n' "$1" "$2" "$3" \ + | jq -c '.[] | [.id, (.updated_at // .submitted_at // null)]' \ + | shasum -a 256 | cut -d' ' -f1 +} + +# sp_emit \ +# \ +# [error_message] +# Assemble the sensor's single output JSON. cr_state/cr_updated_at may be +# empty (emitted as null). error_message is included only when non-empty. +sp_emit() { + jq -cn \ + --arg outcome "$1" --argjson polled "$2" --argjson ticks "$3" \ + --arg head "$4" --arg state "$5" --arg updated "$6" --arg via "$7" \ + --argjson ic "$8" --argjson rv "$9" --argjson rc "${10}" \ + --arg err "${11:-}" ' + { + outcome: $outcome, + polled_for_seconds: $polled, + ticks: $ticks, + head_sha_at_return: $head, + cr_status_state: (if $state == "" then null else $state end), + cr_status_updated_at: (if $updated == "" then null else $updated end), + settled_via: $via, + new_issue_comments: $ic, + new_reviews: $rv, + new_review_comments: $rc + } + + (if $err == "" then {} else {error_message: $err} end) + ' +} diff --git a/eng/skills/pr-watcher/scripts/sensor-poll.sh b/eng/skills/pr-watcher/scripts/sensor-poll.sh new file mode 100755 index 0000000..72acbdc --- /dev/null +++ b/eng/skills/pr-watcher/scripts/sensor-poll.sh @@ -0,0 +1,264 @@ +#!/bin/bash +# Deterministic sensing for /eng:pr-watcher. Implements the whole polling +# protocol (init pass, status-primary 15s loop, comment-stream fallback, +# settle conditions, budgets) and prints EXACTLY ONE JSON object to stdout, +# then exits 0. The dispatcher runs this in a FOREGROUND Bash call; a slice +# that runs out of time emits {"outcome":"continue"} and persists its place in +# /sensor-state.json so the next foreground call resumes. +# +# Outcomes: new_cr_feedback | already_settled | cr_failure | pr_closed | +# idle_timeout | continue | error +# +# Usage: +# sensor-poll.sh --owner O --repo R --pr N --state-dir DIR \ +# [--slice-seconds 540] [--total-seconds 1800] +# +# Reads baselines from DIR/baseline_{issue_comments,reviews,review_comments}.json +# (maintained by the dispatcher). Diagnostics go to stderr only. + +set -u + +# Claude Code's Bash tool strips the PATH (no homebrew), so append the usual +# gh/jq homes. Append rather than prepend: a caller-provided PATH entry (e.g. +# a test's stubbed gh) must win over the real binaries. +PATH="$PATH:/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin" + +# Bootstrap guards cannot rely on the lib or jq, so they echo the full output +# schema as a literal (the one-JSON contract holds even here). +bootstrap_error() { + printf '{"outcome":"error","polled_for_seconds":0,"ticks":0,"head_sha_at_return":"","cr_status_state":null,"cr_status_updated_at":null,"settled_via":"n/a","new_issue_comments":[],"new_reviews":[],"new_review_comments":[],"error_message":"%s"}\n' "$1" + exit 0 +} + +LIB="$(cd -P "$(dirname "${BASH_SOURCE[0]}")" && pwd)/sensor-poll-lib.sh" +# shellcheck source=/dev/null +. "$LIB" 2>/dev/null || bootstrap_error "failed to source sensor-poll-lib.sh" +command -v gh >/dev/null && command -v jq >/dev/null \ + || bootstrap_error "gh or jq not on PATH" + +OWNER="" REPO="" PR_NUM="" STATE_DIR="" +SLICE_SECONDS=540 +TOTAL_SECONDS=1800 +QUIET_SECONDS=180 +TICK_SECONDS="${SENSOR_TICK_SECONDS:-15}" # env seam for tests; 15s in real use +FALLBACK_EVERY=4 # comment-stream fallback every Nth tick (~60s) +MAX_CONSEC_FAILURES=40 # ~10 min of consecutive failing ticks -> outcome error + +arg_error() { sp_emit error 0 0 "" "" "" "n/a" '[]' '[]' '[]' "$1"; exit 0; } +while [ $# -gt 0 ]; do + case "$1" in + # A flag with no value would hit an unset "$2" under set -u and crash past + # the one-JSON contract; catch it as a normal arg error first. + --owner|--repo|--pr|--state-dir|--slice-seconds|--total-seconds) + [ $# -ge 2 ] || arg_error "missing value for $1" ;; + esac + case "$1" in + --owner) OWNER="$2"; shift 2 ;; + --repo) REPO="$2"; shift 2 ;; + --pr) PR_NUM="$2"; shift 2 ;; + --state-dir) STATE_DIR="$2"; shift 2 ;; + --slice-seconds) SLICE_SECONDS="$2"; shift 2 ;; + --total-seconds) TOTAL_SECONDS="$2"; shift 2 ;; + *) arg_error "unknown arg: $1" ;; + esac +done +[ -n "$OWNER" ] && [ -n "$REPO" ] && [ -n "$PR_NUM" ] && [ -n "$STATE_DIR" ] \ + || arg_error "--owner/--repo/--pr/--state-dir are required" + +STATE_FILE="$STATE_DIR/sensor-state.json" +GH_ERR_FILE="$STATE_DIR/.gh-stderr" +mkdir -p "$STATE_DIR" + +# baseline : a missing, empty, or corrupt baseline file reads as [] so a +# bad file degrades to one extra processing cycle, never to broken JSON output. +baseline() { + local b + b=$(cat "$STATE_DIR/baseline_$1.json" 2>/dev/null || true) + jq -e . >/dev/null 2>&1 <<<"$b" && printf '%s' "$b" || printf '[]' +} + +last_gh_err() { tr -d '\n' < "$GH_ERR_FILE" 2>/dev/null | tail -c 200; } + +# --- state (persisted across slices; one sense cycle = one state-file lifetime) +POLL_STARTED="" TICKS=0 LAST_TERMINAL="" PREV_FP="" FALLBACK_TICKS=0 CONSEC_FAILURES=0 +load_state() { + [ -f "$STATE_FILE" ] || return 1 + # A corrupt/truncated file (non-numeric poll_started) reads as absent, so the + # init pass simply runs again instead of crashing the arithmetic below. + jq -e '.poll_started | numbers' "$STATE_FILE" >/dev/null 2>&1 || return 1 + POLL_STARTED=$(jq -r '.poll_started' "$STATE_FILE") + TICKS=$(jq -r '.ticks // 0' "$STATE_FILE") + LAST_TERMINAL=$(jq -r '.last_terminal // ""' "$STATE_FILE") + PREV_FP=$(jq -r '.prev_fp // ""' "$STATE_FILE") + FALLBACK_TICKS=$(jq -r '.fallback_ticks // 0' "$STATE_FILE") + CONSEC_FAILURES=$(jq -r '.consecutive_failures // 0' "$STATE_FILE") +} +save_state() { + local tmp="$STATE_FILE.tmp" + jq -cn --argjson started "$POLL_STARTED" --argjson ticks "$TICKS" \ + --arg last "$LAST_TERMINAL" --arg fp "$PREV_FP" \ + --argjson fb "$FALLBACK_TICKS" --argjson fails "$CONSEC_FAILURES" \ + '{poll_started: $started, ticks: $ticks, + last_terminal: (if $last == "" then null else $last end), + prev_fp: (if $fp == "" then null else $fp end), + fallback_ticks: $fb, consecutive_failures: $fails}' > "$tmp" \ + && mv "$tmp" "$STATE_FILE" +} + +# finish [err] +# Terminal emit: the sense cycle is over, so the state file is removed. +CURRENT_SHA="" +finish() { + local outcome="$1" via="$2" cstate="$3" cupdated="$4" ic="$5" rv="$6" rc="$7" err="${8:-}" + local now polled=0 + now=$(date +%s) + [ -n "$POLL_STARTED" ] && polled=$((now - POLL_STARTED)) + rm -f "$STATE_FILE" + sp_emit "$outcome" "$polled" "$TICKS" "$CURRENT_SHA" "$cstate" "$cupdated" "$via" "$ic" "$rv" "$rc" "$err" + exit 0 +} + +# fail_tick : one failing gh tick. Counts toward the error threshold, +# surfaces the captured gh stderr when the threshold trips, else waits out the +# tick. Callers `continue` (loop) or `return 1` propagation is not needed: +# this either exits via finish or sleeps and returns. +fail_tick() { + CONSEC_FAILURES=$((CONSEC_FAILURES + 1)) + echo "sensor-poll: $1 (consecutive failures: $CONSEC_FAILURES)" >&2 + [ "$CONSEC_FAILURES" -ge "$MAX_CONSEC_FAILURES" ] \ + && finish error "n/a" "" "" '[]' '[]' '[]' "$1; gh said: $(last_gh_err)" + sleep "$TICK_SECONDS" +} + +# --- gh fetch helpers (set globals; return non-zero on API failure) +PR_STATE="" LATEST_STATUS="" NEW_IC="" NEW_RV="" NEW_RC="" +fetch_pr() { + local out + out=$(gh pr view "$PR_NUM" --repo "$OWNER/$REPO" --json state,headRefOid 2>"$GH_ERR_FILE") || return 1 + PR_STATE=$(jq -r '.state' <<<"$out") + CURRENT_SHA=$(jq -r '.headRefOid' <<<"$out") +} +fetch_status() { + local out + out=$(gh api "repos/$OWNER/$REPO/commits/$CURRENT_SHA/statuses?per_page=100" 2>"$GH_ERR_FILE") || return 1 + LATEST_STATUS=$(sp_latest_cr_status "$out") +} +# --paginate follows every page (these endpoints return oldest-first, so page +# 1 alone goes blind to NEW items once a stream passes 100); it emits one JSON +# array per page, which `jq -s add` flattens back to a single array. +fetch_page_all() { + ( set -o pipefail + gh api --paginate "$1" 2>"$GH_ERR_FILE" | jq -s 'add // []' ) +} +fetch_streams() { + local ic rv rc + ic=$(fetch_page_all "repos/$OWNER/$REPO/issues/$PR_NUM/comments?per_page=100") || return 1 + rv=$(fetch_page_all "repos/$OWNER/$REPO/pulls/$PR_NUM/reviews?per_page=100") || return 1 + rc=$(fetch_page_all "repos/$OWNER/$REPO/pulls/$PR_NUM/comments?per_page=100") || return 1 + NEW_IC=$(sp_filter_new issue_comments "$ic" "$(baseline issue_comments)") + NEW_RV=$(sp_filter_new reviews "$rv" "$(baseline reviews)") + NEW_RC=$(sp_filter_new review_comments "$rc" "$(baseline review_comments)") +} +new_total() { jq -n --argjson a "$NEW_IC" --argjson b "$NEW_RV" --argjson c "$NEW_RC" '($a|length)+($b|length)+($c|length)'; } +all_new() { jq -cn --argjson a "$NEW_IC" --argjson b "$NEW_RV" --argjson c "$NEW_RC" '$a+$b+$c'; } +status_state() { jq -r 'if . == null then "" else .state end' <<<"$LATEST_STATUS"; } +status_updated() { jq -r 'if . == null then "" else .updated_at end' <<<"$LATEST_STATUS"; } +pr_closed_check() { + case "$PR_STATE" in + MERGED|CLOSED) finish pr_closed "n/a" "" "" '[]' '[]' '[]' ;; + esac +} + +# --- init pass (first slice of a sense cycle only) +if ! load_state; then + POLL_STARTED=$(date +%s) + # Tolerate brief API blips at cycle start (~2 tick-lengths), then error: a + # persistently dead API at init is likely auth/config, not weather. + INIT_OK="" + for attempt in 1 2 3; do + if fetch_pr; then + pr_closed_check + if fetch_status && fetch_streams; then INIT_OK=1; break; fi + fi + [ "$attempt" -lt 3 ] && sleep "$TICK_SECONDS" + done + [ -n "$INIT_OK" ] || finish error "n/a" "" "" '[]' '[]' '[]' \ + "init fetch failed after 3 attempts; gh said: $(last_gh_err)" + STATE=$(status_state); UPDATED=$(status_updated) + if [ "$(new_total)" -eq 0 ]; then + case "$STATE" in + success) finish already_settled "n/a" "$STATE" "$UPDATED" '[]' '[]' '[]' ;; + failure|error) finish cr_failure "n/a" "$STATE" "$UPDATED" '[]' '[]' '[]' ;; + esac + else + # Backlog exists: drain it now only if a settle condition already holds; + # otherwise CR is mid-review and returning would surface a partial batch. + case "$STATE" in + success|failure|error) + finish new_cr_feedback status_transition "$STATE" "$UPDATED" "$NEW_IC" "$NEW_RV" "$NEW_RC" ;; + esac + [ "$(sp_fallback_settled "$(all_new)")" = "yes" ] \ + && finish new_cr_feedback marker "$STATE" "$UPDATED" "$NEW_IC" "$NEW_RV" "$NEW_RC" + [ "$(sp_all_quiet "$(all_new)" "$(date +%s)" "$QUIET_SECONDS")" = "yes" ] \ + && finish new_cr_feedback quiet_period "$STATE" "$UPDATED" "$NEW_IC" "$NEW_RV" "$NEW_RC" + fi + # Only pending/absent statuses reach here (terminal ones finished above), + # so the cycle starts with no consumed terminal. + save_state +fi + +# --- 15s polling loop (one slice) +SLICE_STARTED=$(date +%s) +while true; do + NOW=$(date +%s) + if [ $((NOW - POLL_STARTED)) -ge "$TOTAL_SECONDS" ]; then + finish idle_timeout "n/a" "$(status_state)" "$(status_updated)" '[]' '[]' '[]' + fi + if [ $((NOW - SLICE_STARTED)) -ge "$SLICE_SECONDS" ]; then + save_state + sp_emit continue $((NOW - POLL_STARTED)) "$TICKS" "$CURRENT_SHA" \ + "$(status_state)" "$(status_updated)" "n/a" '[]' '[]' '[]' + exit 0 + fi + + TICKS=$((TICKS + 1)) + fetch_pr || { fail_tick "gh pr view failed"; continue; } + pr_closed_check + fetch_status || { fail_tick "commit status fetch failed"; continue; } + STATE=$(status_state); UPDATED=$(status_updated) + + if [ "$(sp_fresh_transition "$STATE" "$UPDATED" "$LAST_TERMINAL")" = "yes" ]; then + # Let CR's comment writes settle: the status sometimes flips slightly + # before the last review_comment write is visible to the API. + sleep 5 + # Do NOT mark the transition consumed until the streams are in hand: a + # failed fetch here must leave the transition fresh so the next tick + # retries the drain (a consumed-but-undrained round would idle to + # timeout, the exact failure this script exists to prevent). + fetch_streams || { fail_tick "comment stream fetch failed after transition"; continue; } + # Even zero new items is a settled round (0-finding pass): the dispatcher + # marks it seen and runs its all-clear exit check. + finish new_cr_feedback status_transition "$STATE" "$UPDATED" "$NEW_IC" "$NEW_RV" "$NEW_RC" + fi + + if [ -z "$STATE" ] || [ "$STATE" = "pending" ]; then + FALLBACK_TICKS=$((FALLBACK_TICKS + 1)) + if [ "$FALLBACK_TICKS" -ge "$FALLBACK_EVERY" ]; then + FALLBACK_TICKS=0 + fetch_streams || { fail_tick "comment stream fetch failed in fallback"; continue; } + if [ "$(new_total)" -gt 0 ]; then + [ "$(sp_fallback_settled "$(all_new)")" = "yes" ] \ + && finish new_cr_feedback marker "$STATE" "$UPDATED" "$NEW_IC" "$NEW_RV" "$NEW_RC" + FP=$(sp_fingerprint "$NEW_IC" "$NEW_RV" "$NEW_RC") + if [ "$FP" = "$PREV_FP" ] && [ "$(sp_all_quiet "$(all_new)" "$(date +%s)" "$QUIET_SECONDS")" = "yes" ]; then + finish new_cr_feedback quiet_period "$STATE" "$UPDATED" "$NEW_IC" "$NEW_RV" "$NEW_RC" + fi + PREV_FP="$FP" + fi + fi + fi + + # Reaching here means every gh call this tick succeeded. + CONSEC_FAILURES=0 + sleep "$TICK_SECONDS" +done diff --git a/eng/skills/pr-watcher/tests/sensor-poll-integration.bats b/eng/skills/pr-watcher/tests/sensor-poll-integration.bats new file mode 100644 index 0000000..c57213c --- /dev/null +++ b/eng/skills/pr-watcher/tests/sensor-poll-integration.bats @@ -0,0 +1,142 @@ +#!/usr/bin/env bats +# Integration tests for sensor-poll.sh's deterministic init pass, driven by a +# stubbed `gh` on PATH (the script APPENDS its fallback dirs to PATH, so the +# stub wins). Only loop-free branches are exercised; slice/loop timing is +# covered by live QA. Run as an individual file: +# bats eng/skills/pr-watcher/tests/sensor-poll-integration.bats + +SCRIPT="$BATS_TEST_DIRNAME/../scripts/sensor-poll.sh" + +setup() { + WORK="$BATS_TEST_TMPDIR/work" + STATE="$WORK/state" + FIX="$WORK/fixtures" + mkdir -p "$WORK/bin" "$STATE" "$FIX" + + # gh stub: serves fixture files per endpoint, matched on the full argv. + # Stream endpoints REQUIRE --paginate in the argv: if sensor-poll.sh ever + # drops the flag, the fetch falls through to the unmatched branch and the + # suite fails (pagination is a correctness requirement, not a nicety). + # GH_STUB_FAIL=1 fails every call. + cat > "$WORK/bin/gh" <<'STUB' +#!/bin/bash +[ "${GH_STUB_FAIL:-0}" = "1" ] && { echo "HTTP 401: Bad credentials" >&2; exit 1; } +case "$*" in + "pr view "*) cat "$GH_FIXTURE_DIR/pr.json" ;; + *statuses*) cat "$GH_FIXTURE_DIR/statuses.json" ;; + *--paginate*issues*comments*) cat "$GH_FIXTURE_DIR/issue_comments.json" ;; + *--paginate*reviews*) cat "$GH_FIXTURE_DIR/reviews.json" ;; + *--paginate*pulls*comments*) cat "$GH_FIXTURE_DIR/review_comments.json" ;; + *) echo "gh stub: unmatched: $*" >&2; exit 64 ;; +esac +STUB + chmod +x "$WORK/bin/gh" + export PATH="$WORK/bin:$PATH" + export GH_FIXTURE_DIR="$FIX" + export SENSOR_TICK_SECONDS=0 # init retries sleep 0s in tests + + # Default fixtures: open PR, no CR status, empty streams, empty baselines. + echo '{"state":"OPEN","headRefOid":"abc123"}' > "$FIX/pr.json" + echo '[]' > "$FIX/statuses.json" + echo '[]' > "$FIX/issue_comments.json" + echo '[]' > "$FIX/reviews.json" + echo '[]' > "$FIX/review_comments.json" + for k in issue_comments reviews review_comments; do + echo '[]' > "$STATE/baseline_$k.json" + done +} + +run_sensor() { + run "$SCRIPT" --owner o --repo r --pr 1 --state-dir "$STATE" "$@" +} + +@test "merged PR returns pr_closed immediately" { + echo '{"state":"MERGED","headRefOid":"abc123"}' > "$FIX/pr.json" + run_sensor + [ "$status" -eq 0 ] + [ "$(jq -r .outcome <<<"$output")" = "pr_closed" ] +} + +@test "terminal success with nothing unprocessed returns already_settled" { + echo '[{"context":"CodeRabbit","state":"success","updated_at":"2020-01-01T00:05:00Z","creator":{"login":"coderabbitai[bot]"}}]' > "$FIX/statuses.json" + run_sensor + echo "$output" | jq -e '.outcome == "already_settled" and .cr_status_state == "success" + and .cr_status_updated_at == "2020-01-01T00:05:00Z" and .head_sha_at_return == "abc123"' +} + +@test "terminal failure with nothing unprocessed returns cr_failure" { + echo '[{"context":"CodeRabbit","state":"failure","updated_at":"2020-01-01T00:05:00Z","creator":{"login":"coderabbitai[bot]"}}]' > "$FIX/statuses.json" + run_sensor + echo "$output" | jq -e '.outcome == "cr_failure" and .cr_status_state == "failure"' +} + +@test "unprocessed backlog with terminal status drains via status_transition" { + echo '[{"context":"CodeRabbit","state":"success","updated_at":"2020-01-01T00:05:00Z","creator":{"login":"coderabbitai[bot]"}}]' > "$FIX/statuses.json" + echo '[{"id":11,"user":{"login":"coderabbitai[bot]"},"updated_at":"2020-01-01T00:04:00Z","body":"finding"}]' > "$FIX/issue_comments.json" + run_sensor + echo "$output" | jq -e '.outcome == "new_cr_feedback" and .settled_via == "status_transition" + and .new_issue_comments == [{"id":"11","updated_at":"2020-01-01T00:04:00Z","body":"finding"}]' +} + +@test "unprocessed backlog with pending status and review-status sentinel drains via marker" { + echo '[{"context":"CodeRabbit","state":"pending","updated_at":"2020-01-01T00:05:00Z","creator":{"login":"coderabbitai[bot]"}}]' > "$FIX/statuses.json" + body="done ok" + jq -cn --arg b "$body" '[{"id":12,"user":{"login":"coderabbitai[bot]"},"updated_at":"2020-01-01T00:04:00Z","body":$b}]' > "$FIX/issue_comments.json" + run_sensor + echo "$output" | jq -e '.outcome == "new_cr_feedback" and .settled_via == "marker"' +} + +@test "baselined items do not count as backlog (still already_settled)" { + echo '[{"context":"CodeRabbit","state":"success","updated_at":"2020-01-01T00:05:00Z","creator":{"login":"coderabbitai[bot]"}}]' > "$FIX/statuses.json" + echo '[{"id":11,"user":{"login":"coderabbitai[bot]"},"updated_at":"2020-01-01T00:04:00Z","body":"seen"}]' > "$FIX/issue_comments.json" + echo '["11"]' > "$STATE/baseline_issue_comments.json" + run_sensor + [ "$(jq -r .outcome <<<"$output")" = "already_settled" ] +} + +@test "persistent init API failure returns error JSON with gh stderr, exit 0" { + export GH_STUB_FAIL=1 + run_sensor + [ "$status" -eq 0 ] + echo "$output" | jq -e '.outcome == "error" and (.error_message | contains("401"))' +} + +@test "corrupt sensor-state.json is treated as a fresh init pass" { + echo '[{"context":"CodeRabbit","state":"success","updated_at":"2020-01-01T00:05:00Z","creator":{"login":"coderabbitai[bot]"}}]' > "$FIX/statuses.json" + echo 'garbage{{' > "$STATE/sensor-state.json" + run_sensor + [ "$(jq -r .outcome <<<"$output")" = "already_settled" ] + [ ! -f "$STATE/sensor-state.json" ] +} + +@test "empty baseline file reads as [] and output stays valid JSON" { + echo '[{"context":"CodeRabbit","state":"success","updated_at":"2020-01-01T00:05:00Z","creator":{"login":"coderabbitai[bot]"}}]' > "$FIX/statuses.json" + echo '[{"id":11,"user":{"login":"coderabbitai[bot]"},"updated_at":"2020-01-01T00:04:00Z","body":"x"}]' > "$FIX/issue_comments.json" + : > "$STATE/baseline_issue_comments.json" + run_sensor + echo "$output" | jq -e '.outcome == "new_cr_feedback" and (.new_issue_comments | length) == 1' +} + +@test "bad usage still emits one error JSON and exits 0" { + run "$SCRIPT" --bogus + [ "$status" -eq 0 ] + echo "$output" | jq -se 'length == 1 and .[0].outcome == "error" + and (.[0].error_message | contains("unknown arg"))' +} + +@test "flag with no value emits one error JSON, never a set -u crash" { + run "$SCRIPT" --owner + [ "$status" -eq 0 ] + echo "$output" | jq -se 'length == 1 and .[0].outcome == "error" + and (.[0].error_message | contains("missing value for --owner"))' +} + +@test "multi-page stream responses (concatenated arrays) are flattened" { + echo '[{"context":"CodeRabbit","state":"success","updated_at":"2020-01-01T00:05:00Z","creator":{"login":"coderabbitai[bot]"}}]' > "$FIX/statuses.json" + printf '%s\n%s\n' \ + '[{"id":21,"user":{"login":"coderabbitai[bot]"},"updated_at":"2020-01-01T00:01:00Z","body":"page1"}]' \ + '[{"id":22,"user":{"login":"coderabbitai[bot]"},"updated_at":"2020-01-01T00:02:00Z","body":"page2"}]' \ + > "$FIX/issue_comments.json" + run_sensor + echo "$output" | jq -e '.outcome == "new_cr_feedback" and (.new_issue_comments | map(.id)) == ["21","22"]' +} diff --git a/eng/skills/pr-watcher/tests/sensor-poll.bats b/eng/skills/pr-watcher/tests/sensor-poll.bats new file mode 100644 index 0000000..66159c7 --- /dev/null +++ b/eng/skills/pr-watcher/tests/sensor-poll.bats @@ -0,0 +1,151 @@ +#!/usr/bin/env bats +# Unit tests for the pure decision logic in scripts/sensor-poll-lib.sh. +# Run as an individual file (never part of a full-suite invocation): +# bats eng/skills/pr-watcher/tests/sensor-poll.bats + +setup() { + . "$BATS_TEST_DIRNAME/../scripts/sensor-poll-lib.sh" +} + +# --- sp_latest_cr_status + +@test "latest_cr_status picks newest CodeRabbit entry, ignores other contexts" { + statuses='[ + {"context":"ci/build","state":"success","updated_at":"2020-01-01T00:09:00Z","creator":{"login":"github-actions[bot]"}}, + {"context":"CodeRabbit","state":"pending","updated_at":"2020-01-01T00:01:00Z","creator":{"login":"coderabbitai[bot]"}}, + {"context":"CodeRabbit","state":"success","updated_at":"2020-01-01T00:05:00Z","creator":{"login":"coderabbitai[bot]"}} + ]' + run sp_latest_cr_status "$statuses" + [ "$status" -eq 0 ] + [ "$output" = '{"state":"success","updated_at":"2020-01-01T00:05:00Z"}' ] +} + +@test "latest_cr_status treats a missing creator as a match" { + statuses='[{"context":"CodeRabbit","state":"pending","updated_at":"2020-01-01T00:01:00Z"}]' + run sp_latest_cr_status "$statuses" + [ "$output" = '{"state":"pending","updated_at":"2020-01-01T00:01:00Z"}' ] +} + +@test "latest_cr_status returns null when no CodeRabbit status exists" { + run sp_latest_cr_status '[]' + [ "$output" = "null" ] +} + +# --- sp_filter_new + +@test "filter_new keeps only unbaselined coderabbitai[bot] items, stringifies ids" { + stream='[ + {"id":1,"user":{"login":"coderabbitai[bot]"},"updated_at":"2020-01-01T00:00:00Z","body":"old"}, + {"id":2,"user":{"login":"coderabbitai[bot]"},"updated_at":"2020-01-01T00:01:00Z","body":"new"}, + {"id":3,"user":{"login":"mujtaba3B"},"updated_at":"2020-01-01T00:02:00Z","body":"human"} + ]' + run sp_filter_new issue_comments "$stream" '["1"]' + [ "$output" = '[{"id":"2","updated_at":"2020-01-01T00:01:00Z","body":"new"}]' ] +} + +@test "filter_new maps review fields (state, submitted_at)" { + stream='[{"id":9,"user":{"login":"coderabbitai[bot]"},"state":"COMMENTED","submitted_at":"2020-01-01T00:01:00Z","body":"r"}]' + run sp_filter_new reviews "$stream" '[]' + [ "$output" = '[{"id":"9","state":"COMMENTED","submitted_at":"2020-01-01T00:01:00Z","body":"r"}]' ] +} + +@test "filter_new maps review_comment fields and tolerates null line" { + stream='[{"id":7,"user":{"login":"coderabbitai[bot]"},"path":"a.sh","line":null,"updated_at":"2020-01-01T00:01:00Z","body":"c"}]' + run sp_filter_new review_comments "$stream" '[]' + [ "$output" = '[{"id":"7","path":"a.sh","line":null,"updated_at":"2020-01-01T00:01:00Z","body":"c"}]' ] +} + +# --- sp_fresh_transition + +@test "fresh_transition fires on first terminal status when no prior terminal seen" { + # The v3 incident edge: last_terminal unset must NOT suppress the transition. + run sp_fresh_transition success "2020-01-01T00:05:00Z" "" + [ "$output" = "yes" ] + run sp_fresh_transition failure "2020-01-01T00:05:00Z" "null" + [ "$output" = "yes" ] +} + +@test "fresh_transition ignores pending and already-consumed terminals" { + run sp_fresh_transition pending "2020-01-01T00:05:00Z" "" + [ "$output" = "no" ] + run sp_fresh_transition success "2020-01-01T00:05:00Z" "2020-01-01T00:05:00Z" + [ "$output" = "no" ] + run sp_fresh_transition success "2020-01-01T00:04:00Z" "2020-01-01T00:05:00Z" + [ "$output" = "no" ] +} + +@test "fresh_transition fires on a newer terminal than the last consumed one" { + run sp_fresh_transition success "2020-01-01T00:06:00Z" "2020-01-01T00:05:00Z" + [ "$output" = "yes" ] +} + +# --- sp_fallback_settled + +@test "fallback_settled detects the actionable-comments header" { + run sp_fallback_settled '[{"body":"Actionable comments posted: 2\n\ndetail"}]' + [ "$output" = "yes" ] +} + +@test "fallback_settled detects the review-status sentinel" { + items=$(jq -cn --arg s "$SP_CR_SENTINEL" '[{"body":("prefix " + $s + " suffix")}]') + run sp_fallback_settled "$items" + [ "$output" = "yes" ] +} + +@test "fallback_settled says no for plain comments and null bodies" { + run sp_fallback_settled '[{"body":"just a walkthrough"},{"body":null}]' + [ "$output" = "no" ] +} + +# --- sp_all_quiet (2020-01-01T00:00:00Z == epoch 1577836800) + +@test "all_quiet yes when every item is older than the quiet window" { + items='[{"updated_at":"2020-01-01T00:00:00Z"},{"submitted_at":"2020-01-01T00:00:30Z"}]' + run sp_all_quiet "$items" 1577837100 180 + [ "$output" = "yes" ] +} + +@test "all_quiet no when any item is fresh, and no for an empty set" { + items='[{"updated_at":"2020-01-01T00:00:00Z"},{"updated_at":"2020-01-01T00:04:00Z"}]' + run sp_all_quiet "$items" 1577837100 180 + [ "$output" = "no" ] + run sp_all_quiet '[]' 1577837100 180 + [ "$output" = "no" ] +} + +@test "all_quiet no when an item has no parseable timestamp (never vacuously quiet)" { + run sp_all_quiet '[{"updated_at":"2020-01-01T00:00:00Z"},{"foo":1}]' 1577837100 180 + [ "$output" = "no" ] + run sp_all_quiet '[{"updated_at":"not-a-date"}]' 1577837100 180 + [ "$output" = "no" ] +} + +# --- sp_fingerprint + +@test "fingerprint is stable for identical input and changes with timestamps" { + a=$(sp_fingerprint '[{"id":"1","updated_at":"2020-01-01T00:00:00Z"}]' '[]' '[]') + b=$(sp_fingerprint '[{"id":"1","updated_at":"2020-01-01T00:00:00Z"}]' '[]' '[]') + c=$(sp_fingerprint '[{"id":"1","updated_at":"2020-01-01T00:01:00Z"}]' '[]' '[]') + [ "$a" = "$b" ] + [ "$a" != "$c" ] +} + +# --- sp_emit + +@test "emit produces the full schema with nulled empty status fields" { + run sp_emit already_settled 12 3 abc123 "" "" "n/a" '[]' '[]' '[]' + [ "$status" -eq 0 ] + echo "$output" | jq -e ' + .outcome == "already_settled" and .polled_for_seconds == 12 and .ticks == 3 + and .head_sha_at_return == "abc123" and .cr_status_state == null + and .cr_status_updated_at == null and .settled_via == "n/a" + and .new_issue_comments == [] and .new_reviews == [] and .new_review_comments == [] + and (has("error_message") | not)' +} + +@test "emit carries items and error_message when provided" { + run sp_emit error 5 1 abc "pending" "2020-01-01T00:00:00Z" "n/a" '[{"id":"1"}]' '[]' '[]' "boom" + echo "$output" | jq -e ' + .cr_status_state == "pending" and .new_issue_comments == [{"id":"1"}] + and .error_message == "boom"' +}