From 3866bc8efb776b791cf57b08443d0cd0831c5861 Mon Sep 17 00:00:00 2001 From: Bart van der Meeren Date: Thu, 8 Oct 2026 17:01:20 +0200 Subject: [PATCH 1/2] feat(babysit): add a release threshold so P2/P3 findings stop blocking merges Babysit runs treated every valid review finding as a merge blocker and had no way to record a deferral, so Rukoo #2 ran 14 Codex rounds (54 P2, 10 P1) and optimal-hermes #1 ran 12 before the user stepped in. Helper: - Read the Codex P0-P3 badge for each thread, only when a configured reviewer opened it. No other comment text is kept. - Add `decide --decision deferred`, with batch --finding. P0, P1, and unrated findings need --user-approved, recorded only on those findings. - Report codeReview.rounds and `next` actions, including a head-aware prompt to request review of a repaired head. - Show open findings by severity, deferrals, and rounds in wake messages. Skill: - Severity triage table: P0/P1 block; P2/P3 are deferred unless a commit is already going out for another reason. - Round budget: ask the user before pushing after a fourth review that still raises new P0/P1 findings. - List the only merge gates; optional checks and local reviews are not gates. - Request re-review in the same step as each push, run inspect after pushes, merge without asking again, one agent per branch. --- skills/babysit/SKILL.md | 69 +++++---- skills/babysit/references/helper.md | 10 +- skills/babysit/scripts/README.md | 6 +- skills/babysit/scripts/babysit.mjs | 68 ++++++--- skills/babysit/scripts/github.mjs | 11 ++ skills/babysit/scripts/inspect.mjs | 44 +++++- skills/babysit/scripts/wake.mjs | 19 ++- tests/babysit/github.test.mjs | 4 +- tests/babysit/threshold.test.mjs | 211 ++++++++++++++++++++++++++++ 9 files changed, 386 insertions(+), 56 deletions(-) create mode 100644 tests/babysit/threshold.test.mjs diff --git a/skills/babysit/SKILL.md b/skills/babysit/SKILL.md index 4e8ed6e..202d6e0 100644 --- a/skills/babysit/SKILL.md +++ b/skills/babysit/SKILL.md @@ -5,16 +5,18 @@ description: Commit intended changes, open or update a GitHub PR, and babysit re # Babysit -Own the requested PR workflow through verified merge. Opening the PR or enabling auto-merge is not completion. +Own the requested PR workflow through verified merge. Opening the PR or enabling auto-merge is not completion. The goal is a merged PR with no open P0/P1 findings. A review with nothing left to say is not the goal. ## Scope and authorization Invoking this skill alone requests the full workflow through verified merge. Infer the target repository, branch or existing PR, and intended changes from the active task, conversation, and working tree. Begin at the first incomplete step. Do not require the user to restate the workflow or add instructions. Ask only when the target or intended scope cannot be determined safely from available context. -This invocation, or a request to commit, create a PR, and babysit until merge, authorizes committing and pushing the intended changes, opening or updating the PR, posting task-related PR comments and review requests, fixing in-scope issues, and merging after all gates pass. Apply the same workflow to an existing PR when asked to babysit it until merge. Respect any narrower scope or explicit stop-before-merge instruction. +This invocation, or a request to commit, create a PR, and babysit until merge, authorizes committing and pushing the intended changes, opening or updating the PR, posting task-related PR comments and review requests, fixing in-scope issues, deferring non-blocking findings, and merging after all gates pass. When the gates in section 7 pass, merge. Do not ask for merge permission again. Apply the same workflow to an existing PR when asked to babysit it until merge. Respect any narrower scope or explicit stop-before-merge instruction. Handle routine in-scope repairs without asking for permission again. Escalate missing permissions, required human approvals, legal agreements, paid usage, material scope changes, and decisions that require the user. Never sign a CLA, enable paid review credits, change branch protection, or use administrator bypass. Use another reviewer only if the user authorizes it. Do not spawn subagents unless the user explicitly requests them. +One agent owns the PR branch. Do not let two agents edit it at once. If you delegated work and the delegate stopped without a result, check its state, take the work back, and continue. Do not wait on a delegate that is no longer running. + ## 1. Verify and commit - Read the repository instructions and follow its branch, commit, test, and merge conventions. Confirm the repository, intended base branch, and task scope. @@ -44,29 +46,42 @@ Use these user-provided settings unless the user updates them: These settings are expected behavior, not proof that the current repository is connected or that a review has started. -Let the automatic opening review run before requesting another review. Verify actual startup through live GitHub review activity or the review service's task state. If the opening review does not start, check access, configuration, and failure messages. After confirming no review is running or queued, request `@codex review` once if appropriate; repeated comments do not repair missing access or quota. +Let the automatic opening review run before requesting another review. Verify actual startup through live GitHub review activity or the review service's task state. If the opening review does not start, check access, configuration, and failure messages. After confirming no review is running or queued, request `@codex review` once; repeated comments do not repair missing access or quota. + +Wait for the review to finish, including its summary, inline comments, and threads, then re-read them. An early summary or a thumbs-up without commit metadata is not completion. Bind review evidence to the exact PR head SHA: a clean review of an older commit does not cover a newer head. If the evidence cannot establish completion, keep the review gate pending and say so; silence is not proof. + +`t3code babysit inspect --pr ` gathers the evidence. A submitted review at the head proves submission, not completion, so readiness stays unknown until you record completion: `t3code babysit record --pr --review-evidence --head --url --note `. + +## 4. Triage findings -Wait for the full exhaustive review, including review summaries, inline comments, and review threads. An early summary, completed check, or thumbs-up alone is insufficient while further findings may still arrive. Re-read comments and threads after completion. If the available evidence cannot establish full completion, keep the review gate pending and report the uncertainty; silence or an arbitrary quiet period is not proof. +Every finding gets one decision: fixed, refuted, or deferred. Whether it blocks the merge depends on its severity. Codex marks each inline finding P0 to P3; `inspect` reports it as `severity`, and only for threads the configured reviewer opened. -Associate all review evidence with the exact PR head SHA. Prefer review or task metadata that identifies the reviewed commit. Record the review/task identifiers, URLs, and completion evidence. When a comment or reaction lacks commit metadata, seek corroborating task evidence instead of assigning it to the current head from its timestamp alone. A clean review of an older commit does not approve a newer head. +| Severity | Blocks merge | What to do | +| --- | --- | --- | +| P0, P1 | Yes | Fix it, or refute it with concrete evidence. Defer only with the user's approval. | +| P2, P3 | No, once deferred | Fix it only when you already push a commit for another reason and the fix is small, in scope, and covered by tests. Otherwise defer it. | +| Unrated (humans, other bots) | Yes | Fix it, or refute it with evidence the commenter accepts. Defer only with the user's approval. | -Use `t3code babysit inspect --pr ` for deterministic evidence gathering. A submitted review on the current commit proves submission, not exhaustive completion. The helper deliberately leaves readiness unknown until corroborated completion evidence is recorded. After checking the review service's completed task and reviewed commit, run `t3code babysit record --pr --review-evidence --head --url --note `. Never record that evidence from silence, an early summary, or an unbound thumbs-up. +Raise a P2/P3 finding to blocking when it is actually a security hole, data loss, or a broken main user journey. Never lower a severity without the user, and never edit a reviewer's comment. Assess each finding against the code before acting; refute false positives instead of changing code blindly. -## 4. Repair and re-review +To defer, post one PR comment that lists the deferred findings with their links, the reason, and where the follow-up lives, such as an issue. Do not reply in each thread. Then run `decide --pr --finding [--finding …] --decision deferred --evidence `. The helper refuses to defer P0, P1, or unrated findings without `--user-approved`. Pass that flag only after the user actually approved the deferral; the helper cannot check it. Record fixes and refutations with `--decision fixed|refuted`. A new or edited comment reopens the finding. Do not resolve a thread only to clear a gate; respect repository rules about who may resolve it. + +Never push a commit, and so start another review round, only to fix P2/P3 findings. + +## 5. Repair and re-review - Inspect CI logs, review summaries, inline comments, and unresolved threads. Do not rely on the check rollup alone. Fetch all pages when listing review evidence and discussions. -- Assess each finding against the code. Fix confirmed issues. Refute false positives with concrete evidence instead of changing code blindly. -- Address actionable in-scope findings. Block merge on unresolved correctness, security, data-integrity, or required-review issues. Explain deferred non-blocking suggestions and why they do not block this PR. Escalate blocking issues outside the authorized scope. -- Batch related repairs, add focused regression tests, rerun relevant checks, commit, and push to the same branch. Record the new head SHA and invalidate review evidence for the previous head. -- Automatic review is configured for PR opening. Do not assume a repair push triggers another review. If no review of the new head is already running or queued, comment `@codex review` once. Record the request comment and head SHA so retries or resumed sessions do not duplicate it. -- Wait for review and CI on the new head. Repeat until the final head has a completed review and no actionable blocking findings. -- Do not resolve a thread merely to clear a gate. Record how the finding was fixed or refuted, including the relevant commit or evidence, and respect repository rules about who may resolve it. +- Batch the round's repairs, add focused regression tests, rerun relevant checks, commit, and push to the same branch. Record `t3code babysit record --pr --tested --result pass --command ` for the new head. +- In the same step as the push, request re-review. A repair push does not trigger Codex automatically. If no review of the new head is running or queued, comment `@codex review` once and record it with `record --pr --review-request --head --url `. Check saved requests first so resumed sessions do not duplicate it. +- Run `inspect` after every push and before every yield, and do its `next` actions before you wait. +- Wait for review and CI on the new head and triage again. The new review of the final head is required; its P2/P3 findings are deferred like any other. +- Verify repairs with targeted tests. Local reviews, delegated reviews, and second opinions are optional. They never gate a merge, and a repair never needs one before the Codex re-review. -If Codex is rate-limited or unavailable, keep the PR unmerged and report the blocker. Rate limits are not approval. Do not enable paid credits, bypass review, or silently substitute another reviewer. If a reset or recovery time is known, retain the pending gate and resume at that time without repeatedly requesting reviews. A retry after an explicit failure is distinct from duplicating a running or queued review. +**Round budget.** `inspect` reports `codeReview.rounds`, the number of commits Codex submitted a review for. Repairing the findings of the first three reviews is routine. If the fourth review still raises new P0/P1 findings, do not push again on your own. Send the user the open blocking findings, what the repairs keep breaking, and a recommendation: keep fixing, defer with approval, or split the PR. Continue once they answer. -Record successful local verification with `t3code babysit record --pr --tested --result pass --command `. Bind every review request to its commit with `record --pr --review-request --head --url `. Check saved requests before posting another one. Use `decide --pr --finding --decision fixed|refuted --evidence ` for assessed findings; a new or edited comment reopens the finding. These records support resumption and do not replace live GitHub verification. +If Codex is rate-limited or unavailable, keep the PR unmerged and report the blocker. Rate limits are not approval. Do not enable paid credits, bypass review, or silently substitute another reviewer. If a reset time is known, keep the gate pending and resume then without repeating requests. A retry after an explicit failure is distinct from duplicating a running or queued review. -## 5. Babysit actively, with low noise +## 6. Babysit actively, with low noise Continue through verified merge or the user's narrower stopping point. A genuine blocker must name the missing gate and preserve resumable state. Do not write custom pollers or create a new conversation to deliver a wakeup. @@ -85,34 +100,36 @@ Add `--thread ` when fallback delivery to the original conve **Without durable delivery:** use `wait --pr --timeout ` within the active session. It makes no model call while sleeping. Disclose that this wait ends with the session; it is not durable monitoring. -**On helper-delivered wakes:** run `inspect`, handle the event or record a concrete blocker, then `ack --pr --event --note `. Delivery acceptance is not acknowledgment of handled work. Interrupted work stays unacknowledged in `status` and later wakes; deliberate recovery uses `wake --pr --redeliver `. Ambiguous delivery retries reuse the persisted text and event key. Rejected sends require diagnosis and explicit redelivery. Never start a second repair loop for the same PR. +**On helper-delivered wakes:** run `inspect`, handle the event or record a concrete blocker, then `ack --pr --event --note `. Delivery acceptance is not acknowledgment of handled work. Interrupted work stays unacknowledged in `status` and later wakes; deliberate recovery uses `wake --pr --redeliver `. Never start a second repair loop for the same PR. Keep unchanged observations silent and send brief updates for material changes. Unknown or incomplete evidence keeps the relevant gate pending. If the helper cannot express a gate, report the missing evidence rather than substituting an ad hoc parser. -## 6. Merge only after final verification +## 7. Merge only after final verification -Immediately before merging, re-read the live PR state and verify every gate: +These are the only merge gates. Immediately before merging, re-read the live PR and verify each one: - The head is the exact commit tested and reviewed. -- Required checks have passed for the applicable commit or merge candidate. Missing, skipped, or cancelled checks are not automatically passing evidence; establish whether repository policy requires them and whether the applicable checks actually ran. -- Codex has completed the full review of the final head, and comments and threads have been rechecked after completion. -- No actionable blocking findings remain. +- Required checks passed for the applicable commit. Missing, skipped, or cancelled checks are not passing evidence; establish whether repository policy requires them. +- Codex finished reviewing the final head, and comments and threads were rechecked after it finished. +- No P0/P1 finding is open, and every finding has a decision. - Required human approvals and repository rules are satisfied. - The PR is mergeable, and no new commits, comments, or review activity invalidate readiness. -If the head changes, rerun affected checks and obtain Codex review of the new head. If the base changes, reassess integration and refresh checks as required by repository policy. Resolve in-scope conflicts, then verify and review the resulting head. Re-read the state after any readiness-changing event. +Optional CI checks, security reviews that repository policy does not require, extra reviewers, and local reviews are not gates. Do not wait for them and do not invent new ones. Pass `--security-check` to `init` only for a security check that branch protection requires; the helper keeps readiness unknown until a configured security check passes. + +If the head changes, rerun affected checks and obtain Codex review of the new head. If the base changes, reassess integration and refresh checks as required by repository policy. Resolve in-scope conflicts, then verify and review the resulting head. Use the repository's merge method, defaulting to squash when no convention exists. Bind the merge to the verified head SHA where supported. Never use administrator bypass. Do not enable auto-merge before review gates are satisfied when branch protection does not enforce those gates. Continue monitoring while auto-merge or a merge queue is pending; enabling either is not completion. -## 7. Prove completion +## 8. Prove completion Verify through GitHub that the PR is actually merged. Report: - PR URL. - Verified pre-merge head SHA and resulting merge commit SHA. - Local test results and final CI results, including material checks not run. -- Final Codex review outcome and any required human approval outcome. -- Remaining non-blocking limitations, if any. +- Final Codex review outcome, the number of review rounds, and any required human approval outcome. +- Deferred findings, each with its severity, link, and follow-up. If GitHub shows the PR closed without merge, report that outcome accurately. A blocked handoff must name the unsatisfied gate, relevant evidence, the decision or external change needed, and the actual monitoring status. diff --git a/skills/babysit/references/helper.md b/skills/babysit/references/helper.md index 7a74014..b3c017e 100644 --- a/skills/babysit/references/helper.md +++ b/skills/babysit/references/helper.md @@ -6,7 +6,15 @@ Use the helper shipped with the modified t3code CLI: `t3code babysit --help`. Di ## Evidence -Readiness is an aid to final verification, never merge authorization. A submitted review must match the full head commit and configured reviewer; it does not establish exhaustive completion. Record corroborating completed-task evidence explicitly. Code review and security checks remain separate. Unbound reactions, missing checks, incomplete pages, failed API reads, unknown mergeability, and stale tested commits cannot establish readiness. All unresolved findings need a recorded decision; later edits or new comments reopen them. +Readiness is an aid to final verification, never merge authorization. A submitted review must match the full head commit and configured reviewer; it does not establish exhaustive completion. Record corroborating completed-task evidence explicitly. Code review and security checks remain separate. Unbound reactions, missing checks, incomplete pages, failed API reads, unknown mergeability, and stale tested commits cannot establish readiness. All unresolved findings need a recorded decision: `fixed`, `refuted`, or `deferred`. Later edits or new comments reopen them. + +## Findings and rounds + +Each open finding carries `severity`, read from the Codex priority badge (`P0` to `P3`) on the thread's first comment. It is null when there is no badge or when someone other than a configured reviewer opened the thread. The helper keeps that token and no other comment text. Reason details start with the severity, such as `P2 src/app.ts:12`. + +`decide --decision deferred` discharges a finding without a code change, so it stops blocking readiness. It accepts several `--finding` ids at once and records all or none. It works on its own for P2 and P3. For P0, P1, and unrated findings it also needs `--user-approved`, which is recorded as `userApproved` on those findings only. The helper cannot verify the approval; pass the flag only after the user gave it. + +`codeReview.rounds` counts the commits the configured reviewer submitted a review for. A clean pass that Codex reports only as a comment or reaction is not counted. The skill's round budget uses it. `next` lists actions for reasons agents tend to stall on, such as requesting a review of a repaired head. ## Durable delivery diff --git a/skills/babysit/scripts/README.md b/skills/babysit/scripts/README.md index 7294cb3..624e9c8 100644 --- a/skills/babysit/scripts/README.md +++ b/skills/babysit/scripts/README.md @@ -16,7 +16,7 @@ node babysit.mjs --pr owner/repo#N [--state-dir DIR] [options] | `record --review-request --head SHA --url COMMENT_URL` | Records an `@codex review` request. The URL must point into this PR. | | `record --review-evidence --head SHA --url SOURCE --note TEXT` | Records corroborating evidence that the review of that exact head is complete. | | `record --watcher t3-native\|os-schedule\|session-wait\|none [--watcher-id ID] [--cancel-command CMD]` | Records how the watch wakes the agent. | -| `decide --finding THREAD_ID --decision fixed\|refuted --evidence TEXT [--commit SHA]` | Discharges a review thread as it reads now. A later comment or an edit reopens it. | +| `decide --finding THREAD_ID… --decision fixed\|refuted\|deferred --evidence TEXT [--commit SHA] [--user-approved]` | Discharges review threads as they read now, all or none. A later comment or an edit reopens one. Deferring a P0, P1, or unrated finding also needs `--user-approved`. | | `tick` | One scheduled poll. Reads GitHub, creates an event when there is news, and delivers any due event. It never acknowledges an event. | | `wait [--timeout 30m] [--interval 60s]` | In-session polling. Prints the next event; printing it counts as delivery. | | `ack --event ID… [--note TEXT]` | Marks events handled once the agent has finished the work they called for. | @@ -33,9 +33,11 @@ node babysit.mjs --pr owner/repo#N [--state-dir DIR] [options] - No required check is missing, and none was skipped. - The tested SHA is the head. - Recorded review evidence names the exact head, and the reviewer has not been active since it was recorded. -- No unresolved thread lacks a matching decision. +- No unresolved thread lacks a matching decision. A deferred finding counts as decided. - GitHub's merge state is clean. +Open findings report their Codex priority (`P0` to `P3`) as `severity`, null when the thread has no badge or a non-reviewer opened it; only that token is read from comment bodies. `codeReview.rounds` counts the commits the reviewer reviewed, and `next` lists the actions the current reasons call for. + Anything not finished is `pending`. Anything unexplained is `unknown`. A review submitted at the head proves only that a review was submitted. Comments and reactions carry no commit, so they are reported as unbound signals and are never assigned to a head. Readiness is news, not merge approval; skill section 6 still applies. Recording new review evidence reads a complete live snapshot of the same head and captures its reviewer reactions. A later reaction outside that set withdraws readiness. Repeating a head/source URL keeps its original timestamp, note, and reaction set; a new completed review needs a new source URL. diff --git a/skills/babysit/scripts/babysit.mjs b/skills/babysit/scripts/babysit.mjs index 47f97de..6002109 100644 --- a/skills/babysit/scripts/babysit.mjs +++ b/skills/babysit/scripts/babysit.mjs @@ -9,7 +9,7 @@ import { fileURLToPath } from "node:url"; import { parseArgs } from "node:util"; import { fetchPullRequestSnapshot, ghRunner } from "./github.mjs"; -import { classify, invalidateResolvedDecisions, reviewerReactions, threadVersion } from "./inspect.mjs"; +import { classify, DEFERRABLE_SEVERITIES, findingSeverity, invalidateResolvedDecisions, reviewerReactions, threadVersion } from "./inspect.mjs"; import { quotePowerShell, quoteShell, scheduleCommands } from "./schedule.mjs"; import { describeLock, @@ -38,6 +38,7 @@ const RATE_LIMIT_FLOOR = 100; const RATE_LIMIT_FALLBACK_MS = 15 * 60_000; const WATCHERS = new Set(["t3-native", "os-schedule", "session-wait", "none"]); const STOP_REASONS = new Set(["merged", "closed", "cancelled"]); +const DECISIONS = new Set(["fixed", "refuted", "deferred"]); const LOGIN = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})(?:\[bot\])?$/u; const OPTIONS = { @@ -64,8 +65,9 @@ const OPTIONS = { watcher: { type: "string" }, "watcher-id": { type: "string" }, "cancel-command": { type: "string" }, - finding: { type: "string" }, + finding: { type: "string", multiple: true }, decision: { type: "string" }, + "user-approved": { type: "boolean" }, evidence: { type: "string" }, commit: { type: "string" }, event: { type: "string", multiple: true }, @@ -82,7 +84,7 @@ const ALLOWED = { init: [...COMMON, ...POLICY, "thread", "wake-settled", "t3code-cli", "gh", "task", "cwd", "branch"], inspect: [...COMMON, ...POLICY, "gh"], record: [...COMMON, "tested", "result", "command", "review-request", "review-evidence", "head", "url", "note", "watcher", "watcher-id", "cancel-command"], - decide: [...COMMON, "finding", "decision", "evidence", "commit"], + decide: [...COMMON, "finding", "decision", "evidence", "commit", "user-approved"], tick: COMMON, wait: [...COMMON, "timeout", "interval"], ack: [...COMMON, "event", "note"], @@ -569,8 +571,10 @@ async function record(values, deps, context) { } async function decide(values, deps, context) { - const findingId = text(values.finding, "--finding", 300); - if (values.decision !== "fixed" && values.decision !== "refuted") throw usage("DECISION_REQUIRED", "--decision must be fixed or refuted."); + const findingIds = [...new Set((values.finding ?? []).map((finding) => text(finding, "--finding", 300)))]; + if (findingIds.length === 0) throw usage("VALUE_REQUIRED", "--finding needs a review thread id."); + if (!DECISIONS.has(values.decision)) throw usage("DECISION_REQUIRED", "--decision must be fixed, refuted, or deferred."); + if (values["user-approved"] && values.decision !== "deferred") throw usage("INVALID_USAGE", "--user-approved applies only to --decision deferred."); const evidence = text(values.evidence, "--evidence", 2000); const commit = values.commit === undefined ? null : requireFullSha(values.commit, "--commit"); @@ -583,30 +587,49 @@ async function decide(values, deps, context) { details: { error: snapshot.error ?? null }, }); } - const thread = snapshot.pr.threads.find((candidate) => candidate.id === findingId); - if (!thread) { - throw new BabysitError("FINDING_NOT_FOUND", `${displayRef(context.ref)} has no review thread ${findingId}.`, { exitCode: 3, details: { finding: findingId } }); - } + // Check every thread before recording any, so a batch is all or nothing. + const threads = findingIds.map((findingId) => { + const thread = snapshot.pr.threads.find((candidate) => candidate.id === findingId); + if (!thread) { + throw new BabysitError("FINDING_NOT_FOUND", `${displayRef(context.ref)} has no review thread ${findingId}.`, { exitCode: 3, details: { finding: findingId } }); + } + const severity = findingSeverity(thread, state.policy); + if (values.decision === "deferred" && !DEFERRABLE_SEVERITIES.has(severity) && !values["user-approved"]) { + throw new BabysitError( + "DEFERRAL_NEEDS_USER", + `Review thread ${findingId} is ${severity ?? "unrated"}. Fix it, refute it, or defer it only after the user approves, with --user-approved.`, + { exitCode: 4, details: { finding: findingId, severity } }, + ); + } + return thread; + }); // The decision covers the thread exactly as it reads now; a later or edited comment reopens it. invalidateResolvedDecisions(snapshot, state, iso(deps.now())); - const decision = { - decision: values.decision, - evidence, - commit, - at: iso(deps.now()), - threadVersion: threadVersion(thread), - isResolved: thread.isResolved, - comments: thread.comments.length, - headSha: snapshot.pr.headSha, - url: thread.comments[0]?.url ?? null, - }; state.findingDecisions ??= {}; - state.findingDecisions[findingId] = decision; + const findings = threads.map((thread) => { + const decision = { + decision: values.decision, + evidence, + commit, + severity: findingSeverity(thread, state.policy), + // Only findings that needed approval carry it, so the report never claims an approval nobody gave. + ...(values["user-approved"] && !DEFERRABLE_SEVERITIES.has(findingSeverity(thread, state.policy)) ? { userApproved: true } : {}), + at: iso(deps.now()), + threadVersion: threadVersion(thread), + isResolved: thread.isResolved, + comments: thread.comments.length, + headSha: snapshot.pr.headSha, + url: thread.comments[0]?.url ?? null, + }; + state.findingDecisions[thread.id] = decision; + return { threadId: thread.id, ...decision }; + }); await save(context, state, lock, deps); return { data: { statePath: context.statePath, - finding: { threadId: findingId, isResolved: thread.isResolved, ...decision }, + ...(findings.length === 1 ? { finding: findings[0] } : {}), + findings, note: "Reply in the thread before deciding: any later comment or edit on the thread reopens this finding.", }, }; @@ -899,6 +922,7 @@ export async function main(argv, overrides = {}) { "record requires one of --tested, --review-request, --review-evidence, or --watcher. Tests need --result and --command; review evidence needs --head, --url and --note.", "wait has a finite timeout. tick sends only changed news and retries ambiguous delivery with the original key and text.", "schedule-command prints registration/cancellation commands; it does not start a schedule.", + "decide takes one or more --finding ids. --decision deferred works on its own for P2/P3 findings; P0, P1, and unrated findings also need --user-approved.", "Delivered events require explicit ack --event. Readiness never authorizes a merge."] }; const envelope = { ok: true, data }; deps.stdout(`${JSON.stringify(envelope, null, 2)}\n`); diff --git a/skills/babysit/scripts/github.mjs b/skills/babysit/scripts/github.mjs index 1532978..0d2f4df 100644 --- a/skills/babysit/scripts/github.mjs +++ b/skills/babysit/scripts/github.mjs @@ -183,6 +183,7 @@ function normalizeContext(node) { } // Bodies are hashed, not kept: edits stay detectable, and no comment text reaches state or wake messages. +// The one exception is a review comment's priority badge, reduced to P0-P3. function normalizeReview(node) { return { id: node.id, @@ -196,6 +197,15 @@ function normalizeReview(node) { }; } +/** + * The priority badge a Codex review comment opens with, such as `![P2 Badge](…)`: P0 to P3, or null. + * Only this token is kept, never the text around it. + */ +export function reviewSeverity(body) { + const match = /!\[(P[0-3]) Badge\]/u.exec(String(body ?? "").slice(0, 300)); + return match ? match[1] : null; +} + function normalizeComment(node) { return { id: node.id, @@ -203,6 +213,7 @@ function normalizeComment(node) { createdAt: node.createdAt ?? null, lastEditedAt: node.lastEditedAt ?? null, bodySha256: sha256(node.body ?? ""), + severity: reviewSeverity(node.body), url: node.url ?? null, }; } diff --git a/skills/babysit/scripts/inspect.mjs b/skills/babysit/scripts/inspect.mjs index c17301a..92a021e 100644 --- a/skills/babysit/scripts/inspect.mjs +++ b/skills/babysit/scripts/inspect.mjs @@ -10,6 +10,39 @@ const FAILED_CONCLUSIONS = new Set(["FAILURE", "TIMED_OUT", "CANCELLED", "ACTION const SKIPPED_CONCLUSIONS = new Set(["SKIPPED", "NEUTRAL"]); const SUBMITTED_REVIEW_STATES = new Set(["COMMENTED", "APPROVED", "CHANGES_REQUESTED"]); const MERGEABLE_STATES = new Set(["CLEAN", "HAS_HOOKS"]); +/** Reviewer priorities an agent may defer on its own. P0, P1, and unrated findings need the user. */ +export const DEFERRABLE_SEVERITIES = new Set(["P2", "P3"]); + +/** What the agent should do about a reason, for the reasons agents have stalled on. */ +const TRIAGE = + "Triage open findings by severity: fix P0/P1; fix a P2/P3 only when you already push a commit for another reason and the fix is small and safe, otherwise decide --decision deferred."; +const NEXT_ACTIONS = { + REVIEW_NOT_REQUESTED: + "No review of this head is submitted or requested. Unless the automatic review on PR open is still running, comment `@codex review` once now and record it with record --review-request.", + REVIEW_NOT_REQUESTED_REPAIRED: + "This repaired head has no review and none is requested, and a push does not trigger one. Comment `@codex review` once now and record it with record --review-request.", + REVIEW_COMPLETION_UNCORROBORATED: "A review of this head was submitted. Confirm the review task finished, then record --review-evidence.", + FINDING_UNRESOLVED: TRIAGE, + FINDING_REOPENED: TRIAGE, + TESTED_HEAD_MISSING: "Run the verification commands at this head and record --tested.", + TESTED_HEAD_MISMATCH: "Run the verification commands at this head and record --tested.", + BRANCH_BEHIND: "Update the branch from its base, verify, and push.", + CONFLICTS: "Resolve the conflicts with the base, verify, and push.", +}; + +function nextActions(reasons, repairedHead) { + return [...new Set(reasons.map((reason) => NEXT_ACTIONS[reason.code === "REVIEW_NOT_REQUESTED" && repairedHead ? "REVIEW_NOT_REQUESTED_REPAIRED" : reason.code]).filter(Boolean))]; +} + +/** + * A finding's priority: the badge on its first comment, and only when a configured reviewer wrote it. + * A badge quoted by anyone else does not rate the finding. + */ +export function findingSeverity(thread, policy) { + const first = thread.comments[0]; + const reviewers = new Set((policy?.codeReviewers ?? []).map(loginKey)); + return first && reviewers.has(loginKey(first.author)) ? (first.severity ?? null) : null; +} /** pass, fail, skipped, pending, or unknown. Anything not finished is pending, never complete. */ export function checkOutcome(check) { @@ -87,6 +120,7 @@ function emptySections(state, headSha) { completeness: null, caveat: REVIEW_CAVEAT, reviewers: [...(state.policy?.codeReviewers ?? [])], + rounds: null, reviewsAtHead: [], staleReviews: [], requestsAtHead: [], @@ -96,6 +130,7 @@ function emptySections(state, headSha) { securityReview: { state: "unknown", checks: [] }, findings: { open: [], discharged: [] }, tested: testedView(state, headSha), + next: [], }; } @@ -279,8 +314,10 @@ export function classify(snapshot, state) { )) add("unknown", "REVIEW_REACTIONS_AFTER_EVIDENCE"); } const reviewState = reviewsAtHead.length > 0 ? "submitted" : requestsAtHead.length > 0 ? "requested" : unboundSignals.length > 0 ? "unbound_signal" : "none"; + // One round per commit the reviewer submitted a review for. + const rounds = new Set(reviewerReviews.map((review) => review.commitSha).filter(Boolean)).size; - // Findings: every unresolved thread blocks until a decision matches its current content. + // Findings: every unresolved thread blocks until a decision (fixed, refuted, or deferred) matches its current content. const decisions = state.findingDecisions ?? {}; const open = []; const discharged = []; @@ -291,6 +328,7 @@ export function classify(snapshot, state) { const entry = { threadId: thread.id, author: first?.author ?? null, + severity: findingSeverity(thread, policy), path: thread.path, line: thread.line, isOutdated: thread.isOutdated, @@ -305,7 +343,7 @@ export function classify(snapshot, state) { } else { open.push({ ...entry, reopened: Boolean(decision) }); const where = thread.path ? `${thread.path}${thread.line ? `:${thread.line}` : ""}${thread.isOutdated ? " (outdated)" : ""}` : null; - add("blocked", decision ? "FINDING_REOPENED" : "FINDING_UNRESOLVED", thread.id, where); + add("blocked", decision ? "FINDING_REOPENED" : "FINDING_UNRESOLVED", thread.id, [entry.severity ?? "unrated", where].filter(Boolean).join(" ")); } } @@ -383,6 +421,7 @@ export function classify(snapshot, state) { completeness: evidenceAtHead.length > 0 ? "agent_recorded_evidence" : reviewsAtHead.length > 0 ? "submitted_review_at_head" : null, caveat: REVIEW_CAVEAT, reviewers: [...(policy.codeReviewers ?? [])], + rounds, reviewsAtHead: reviewsAtHead.map(reviewView), staleReviews: staleReviews.map(reviewView), requestsAtHead, @@ -396,6 +435,7 @@ export function classify(snapshot, state) { }, findings: { open, discharged }, tested: testedView(state, head), + next: nextActions(reasons, staleReviews.length > 0), summary, newsKey: hashSummary(summary), fingerprint: sha256( diff --git a/skills/babysit/scripts/wake.mjs b/skills/babysit/scripts/wake.mjs index eb266d4..3ac6177 100644 --- a/skills/babysit/scripts/wake.mjs +++ b/skills/babysit/scripts/wake.mjs @@ -119,6 +119,14 @@ export function describeChanges(previous, current) { return lines; } +/** ` (1 P1, 3 P2)` for open findings, or nothing when there are none. */ +function severityBreakdown(findings) { + if (findings.length === 0) return ""; + const counts = new Map(); + for (const finding of findings) counts.set(finding.severity ?? "unrated", (counts.get(finding.severity ?? "unrated") ?? 0) + 1); + return ` (${[...counts].sort(([a], [b]) => a.localeCompare(b)).map(([severity, count]) => `${count} ${severity}`).join(", ")})`; +} + function clipped(lines) { return lines.length <= MAX_LIST_LINES ? lines : [...lines.slice(0, MAX_LIST_LINES), `(${lines.length - MAX_LIST_LINES} more; run inspect)`]; } @@ -144,9 +152,12 @@ export function renderWakeText({ state, eventId, previousId, kind, inspection, c lines.push(`Head: ${inspection.pr.headSha}`); lines.push(`Tested: ${tested.sha ?? "none"}${tested.sha ? ` (${tested.result}${tested.matchesHead ? ", matches head" : ", not the head"})` : ""}`); lines.push( - `Code review at head: ${review.reviewsAtHead.length} submitted, ${review.evidenceAtHead.length} recorded evidence, ${review.unboundSignals.length} unbound signal${review.unboundSignals.length === 1 ? "" : "s"}.`, + `Code review at head: ${review.reviewsAtHead.length} submitted, ${review.evidenceAtHead.length} recorded evidence, ${review.unboundSignals.length} unbound signal${review.unboundSignals.length === 1 ? "" : "s"}. Review rounds so far: ${review.rounds ?? "unknown"}.`, + ); + const deferred = inspection.findings.discharged.filter((finding) => finding.decision === "deferred").length; + lines.push( + `Open findings: ${inspection.findings.open.length}${severityBreakdown(inspection.findings.open)}; deferred: ${deferred}; CI: ${inspection.ci.state}; security review: ${inspection.securityReview.state}.`, ); - lines.push(`Open findings: ${inspection.findings.open.length}; CI: ${inspection.ci.state}; security review: ${inspection.securityReview.state}.`); } lines.push("Changes since the last delivered wake:"); lines.push(...clipped(changes.length > 0 ? changes : ["None beyond the state above."]).map((line) => `- ${line}`)); @@ -160,6 +171,10 @@ export function renderWakeText({ state, eventId, previousId, kind, inspection, c ).map((line) => `- ${line}`), ); } + if (kind === "observation" && (inspection.next ?? []).length > 0) { + lines.push("Next:"); + lines.push(...inspection.next.map((action) => `- ${action}`)); + } lines.push(`Unacknowledged earlier wakes: ${earlier.length > 0 ? earlier.map((event) => event.id).join(", ") : "none"}.`); lines.push("This is news, not merge approval. Re-read the live pull request and verify every gate before merging (babysit skill, section 6)."); lines.push(`Inspect: ${command("inspect")}`); diff --git a/tests/babysit/github.test.mjs b/tests/babysit/github.test.mjs index e269657..ff35bba 100644 --- a/tests/babysit/github.test.mjs +++ b/tests/babysit/github.test.mjs @@ -56,7 +56,7 @@ function lists() { contexts: [checkRun("build"), checkRun("test"), { __typename: "StatusContext", id: "status:deploy", context: "deploy", state: "SUCCESS", isRequired: false, targetUrl: null }], reviews: [reviewNode("review-1"), reviewNode("review-2"), reviewNode("review-3")], threads: [ - { id: "thread-1", isResolved: false, isOutdated: false, path: "a.ts", line: 3, originalLine: 3, comments: [commentNode("t1-c1"), commentNode("t1-c2"), commentNode("t1-c3")] }, + { id: "thread-1", isResolved: false, isOutdated: false, path: "a.ts", line: 3, originalLine: 3, comments: [commentNode("t1-c1", { body: "![P1 Badge](https://img.shields.io/badge/P1-orange) body of t1-c1" }), commentNode("t1-c2"), commentNode("t1-c3")] }, { id: "thread-2", isResolved: true, isOutdated: false, path: "b.ts", line: null, originalLine: 9, comments: [commentNode("t2-c1")] }, { id: "thread-3", isResolved: true, isOutdated: true, path: "c.ts", line: 1, originalLine: 1, comments: [commentNode("t3-c1")] }, ], @@ -142,6 +142,8 @@ describe("fetchPullRequestSnapshot", () => { // Bodies are hashed, never kept. expect(JSON.stringify(snapshot)).not.toContain("body of"); expect(snapshot.pr.comments[0].bodySha256).toMatch(/^[0-9a-f]{64}$/u); + // Only the priority badge survives from a body. + expect(snapshot.pr.threads[0].comments.map((comment) => comment.severity)).toEqual(["P1", null, null]); expect(github.calls.map((call) => call.query)).toEqual([ QUERIES.main, QUERIES.contexts, diff --git a/tests/babysit/threshold.test.mjs b/tests/babysit/threshold.test.mjs new file mode 100644 index 0000000..0aa5281 --- /dev/null +++ b/tests/babysit/threshold.test.mjs @@ -0,0 +1,211 @@ +// The release threshold: severity-rated findings, deferral, review rounds, and next actions. +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { reviewSeverity } from "../../skills/babysit/scripts/github.mjs"; +import { classify, threadVersion } from "../../skills/babysit/scripts/inspect.mjs"; +import { recordObservation } from "../../skills/babysit/scripts/wake.mjs"; +import { codes, comment, harness, HEAD, OLD_HEAD, PR_URL, review, REVIEWER, snapshot, thread, watchState } from "./fixtures.mjs"; + +const PR = ["--pr", "acme/widgets#5"]; + +/** A review thread whose first comment carries Codex's priority badge. */ +function rated(id, severity, overrides = {}) { + return thread({ + id, + comments: [comment({ id: `${id}-c1`, author: REVIEWER, severity, url: `${PR_URL}#discussion_${id}` })], + ...overrides, + }); +} + +describe("reviewSeverity", () => { + it("reads the Codex priority badge and nothing else", () => { + expect(reviewSeverity("**![P2 Badge](https://img.shields.io/badge/P2-yellow?style=flat) Scope the lookup**")).toBe("P2"); + expect(reviewSeverity("![P0 Badge](x) Data loss")).toBe("P0"); + expect(reviewSeverity("This mentions P1 but has no badge.")).toBeNull(); + expect(reviewSeverity("![P7 Badge](x)")).toBeNull(); + expect(reviewSeverity(null)).toBeNull(); + // A badge buried deep in a reply is not the finding's rating. + expect(reviewSeverity(`${"x".repeat(400)} ![P0 Badge](x)`)).toBeNull(); + }); +}); + +describe("classify", () => { + it("reports each open finding's severity in the finding and its reason", () => { + const inspection = classify(snapshot({ threads: [rated("thread-1", "P1"), rated("thread-2", "P2"), rated("thread-3", null)] }), watchState()); + + expect(inspection.readiness).toBe("blocked"); + expect(inspection.findings.open.map((finding) => [finding.threadId, finding.severity])).toEqual([ + ["thread-1", "P1"], + ["thread-2", "P2"], + ["thread-3", null], + ]); + expect(inspection.reasons.filter((reason) => reason.code === "FINDING_UNRESOLVED").map((reason) => reason.detail)).toEqual([ + "P1 src/app.ts:12", + "P2 src/app.ts:12", + "unrated src/app.ts:12", + ]); + expect(inspection.next.some((action) => /decide --decision deferred/u.test(action))).toBe(true); + }); + + it("ignores a priority badge that someone other than the reviewer wrote", () => { + const quoted = thread({ id: "thread-1", comments: [comment({ id: "thread-1-c1", author: "some-human", severity: "P3" })] }); + const inspection = classify(snapshot({ threads: [quoted] }), watchState()); + + expect(inspection.findings.open[0].severity).toBeNull(); + }); + + it("still discharges decisions recorded before severities existed", () => { + const open = rated("thread-1", "P2"); + const state = watchState({ findingDecisions: { "thread-1": { decision: "fixed", evidence: "Fixed.", threadVersion: threadVersion(open), at: "2026-10-07T09:56:00Z" } } }); + + expect(classify(snapshot({ threads: [open] }), state).readiness).toBe("ready"); + }); + + it("asks for a review of a repaired head without the opening-review caveat", () => { + const opening = classify(snapshot({ reviews: [] }), watchState({ reviewEvidence: [] })); + expect(opening.next.join(" ")).toMatch(/automatic review on PR open/u); + const repaired = classify(snapshot({ reviews: [review({ commitSha: OLD_HEAD })] }), watchState({ reviewEvidence: [] })); + expect(repaired.next.join(" ")).toMatch(/repaired head/u); + }); + + it("treats a deferred finding as decided, so it does not block readiness", () => { + const open = rated("thread-1", "P2"); + const state = watchState({ + findingDecisions: { "thread-1": { decision: "deferred", evidence: "Follow-up in #14.", severity: "P2", threadVersion: threadVersion(open), at: "2026-10-07T09:56:00Z" } }, + }); + const inspection = classify(snapshot({ threads: [open] }), state); + + expect(inspection.readiness).toBe("ready"); + expect(inspection.findings.discharged).toMatchObject([{ threadId: "thread-1", decision: "deferred", severity: "P2" }]); + }); + + it("counts one review round per commit the reviewer reviewed", () => { + const reviews = [ + review({ id: "r1", commitSha: OLD_HEAD }), + review({ id: "r2", commitSha: OLD_HEAD }), + review({ id: "r3", commitSha: HEAD }), + review({ id: "r4", author: "someone-else", commitSha: "d".repeat(40) }), + ]; + expect(classify(snapshot({ reviews }), watchState()).codeReview.rounds).toBe(2); + }); + + it("tells the agent to request a review when a repaired head has none", () => { + const inspection = classify(snapshot({ reviews: [review({ commitSha: OLD_HEAD })] }), watchState({ reviewEvidence: [] })); + + expect(codes(inspection)).toContain("REVIEW_NOT_REQUESTED"); + expect(inspection.next.some((action) => /`@codex review`/u.test(action))).toBe(true); + }); + + it("has no next action when the pull request is ready", () => { + expect(classify(snapshot(), watchState()).next).toEqual([]); + }); +}); + +describe("wake text", () => { + it("summarizes open findings by severity, deferrals, rounds, and next actions", () => { + const state = { + ...watchState({ reviewEvidence: [] }), + delivery: { type: "t3-thread", threadId: "thread-a", wakeSettled: false, cliPath: "/opt/t3code/dist/cli.js" }, + events: [], + lastObservation: null, + baselineSummary: null, + health: { consecutiveFailures: 0 }, + }; + const context = { now: () => Date.parse("2026-10-07T10:00:00Z"), command: (name) => `helper ${name}` }; + const { event } = recordObservation(state, classify(snapshot({ threads: [rated("thread-1", "P1"), rated("thread-2", "P2"), rated("thread-3", "P2")] }), state), context); + + expect(event.text).toContain("Open findings: 3 (1 P1, 2 P2); deferred: 0;"); + expect(event.text).toContain("Review rounds so far: 1."); + expect(event.text).toMatch(/Next:\n- /u); + }); +}); + +describe("decide --decision deferred", () => { + let h; + beforeEach(async () => { + h = await harness(); + }); + afterEach(async () => { + await h.remove(); + }); + + async function setup(threads) { + h.snapshots.splice(0, h.snapshots.length, snapshot({ threads })); + expect((await h.run(["init", ...PR, "--code-reviewer", REVIEWER, "--thread", "thread-a", "--wake-settled"])).code).toBe(0); + expect((await h.run(["record", ...PR, "--tested", HEAD, "--result", "pass", "--command", "pnpm check"])).code).toBe(0); + expect((await h.run(["record", ...PR, "--review-evidence", "--head", HEAD, "--url", "https://chatgpt.com/codex/tasks/task_1", "--note", "Review finished."])).code).toBe(0); + } + + it("defers several P2/P3 findings in one call and then reports ready", async () => { + await setup([rated("thread-1", "P2"), rated("thread-2", "P3")]); + + const decided = await h.run(["decide", ...PR, "--finding", "thread-1", "--finding", "thread-2", "--decision", "deferred", "--evidence", "Non-blocking; follow-up in #14."]); + expect(decided).toMatchObject({ code: 0, data: { findings: [{ threadId: "thread-1", decision: "deferred", severity: "P2" }, { threadId: "thread-2", severity: "P3" }] } }); + + const inspection = (await h.run(["inspect", ...PR])).data.inspection; + expect(inspection.readiness).toBe("ready"); + expect(inspection.findings.discharged.map((finding) => finding.decision)).toEqual(["deferred", "deferred"]); + }); + + it("refuses to defer P0, P1, or unrated findings without the user's approval, and records nothing from a refused batch", async () => { + await setup([rated("thread-1", "P2"), rated("thread-2", "P1"), rated("thread-3", null)]); + + const refused = await h.run(["decide", ...PR, "--finding", "thread-1", "--finding", "thread-2", "--decision", "deferred", "--evidence", "Later."]); + expect(refused).toMatchObject({ code: 4, error: { code: "DEFERRAL_NEEDS_USER", details: { finding: "thread-2", severity: "P1" } } }); + expect((await h.run(["inspect", ...PR])).data.inspection.findings.discharged).toEqual([]); + + expect((await h.run(["decide", ...PR, "--finding", "thread-3", "--decision", "deferred", "--evidence", "Later."])).error.code).toBe("DEFERRAL_NEEDS_USER"); + + const approved = await h.run(["decide", ...PR, "--finding", "thread-2", "--finding", "thread-3", "--decision", "deferred", "--user-approved", "--evidence", "Bart approved deferring these to #15."]); + expect(approved).toMatchObject({ code: 0, data: { findings: [{ threadId: "thread-2", userApproved: true }, { threadId: "thread-3", userApproved: true }] } }); + }); + + it("marks only the findings that needed approval as user-approved", async () => { + await setup([rated("thread-1", "P1"), rated("thread-2", "P2")]); + + const decided = await h.run(["decide", ...PR, "--finding", "thread-1", "--finding", "thread-2", "--decision", "deferred", "--user-approved", "--evidence", "Approved for #15."]); + expect(decided.data.findings.map((finding) => finding.userApproved ?? false)).toEqual([true, false]); + }); + + it("refuses to defer a badge quoted by someone other than the reviewer", async () => { + await setup([thread({ id: "thread-1", comments: [comment({ id: "thread-1-c1", author: "some-human", severity: "P3" })] })]); + + expect((await h.run(["decide", ...PR, "--finding", "thread-1", "--decision", "deferred", "--evidence", "Later."])).error.code).toBe("DEFERRAL_NEEDS_USER"); + }); + + it("records nothing when a batch names an unknown thread", async () => { + await setup([rated("thread-1", "P2")]); + + expect((await h.run(["decide", ...PR, "--finding", "thread-1", "--finding", "thread-9", "--decision", "deferred", "--evidence", "Later."])).error.code).toBe("FINDING_NOT_FOUND"); + expect((await h.run(["inspect", ...PR])).data.inspection.findings.discharged).toEqual([]); + }); + + it("reopens a deferred P2 that the reviewer re-rates P1, and then needs approval to defer", async () => { + await setup([rated("thread-1", "P2")]); + expect((await h.run(["decide", ...PR, "--finding", "thread-1", "--decision", "deferred", "--evidence", "Follow-up in #14."])).code).toBe(0); + + const rerated = rated("thread-1", "P1"); + rerated.comments[0] = { ...rerated.comments[0], bodySha256: "edited", lastEditedAt: "2026-10-07T10:05:00Z" }; + h.snapshots.splice(0, h.snapshots.length, snapshot({ threads: [rerated] })); + const inspection = (await h.run(["inspect", ...PR])).data.inspection; + expect(inspection.reasons).toContainEqual(expect.objectContaining({ code: "FINDING_REOPENED", detail: "P1 src/app.ts:12" })); + expect((await h.run(["decide", ...PR, "--finding", "thread-1", "--decision", "deferred", "--evidence", "Later."])).error.code).toBe("DEFERRAL_NEEDS_USER"); + }); + + it("accepts --user-approved only with a deferral", async () => { + await setup([rated("thread-1", "P1")]); + + const result = await h.run(["decide", ...PR, "--finding", "thread-1", "--decision", "fixed", "--user-approved", "--evidence", "Fixed."]); + expect(result).toMatchObject({ code: 2, error: { code: "INVALID_USAGE" } }); + }); + + it("reopens a deferred finding when the reviewer comments again", async () => { + await setup([rated("thread-1", "P2")]); + expect((await h.run(["decide", ...PR, "--finding", "thread-1", "--decision", "deferred", "--evidence", "Follow-up in #14."])).code).toBe(0); + + const replied = rated("thread-1", "P2"); + replied.comments.push(comment({ id: "thread-1-c2", author: REVIEWER })); + h.snapshots.splice(0, h.snapshots.length, snapshot({ threads: [replied] })); + expect(codes((await h.run(["inspect", ...PR])).data.inspection)).toContain("FINDING_REOPENED"); + }); +}); From fb1a3ca2044e2db55a1bc23312ea5e8f69832d2b Mon Sep 17 00:00:00 2001 From: Bart van der Meeren Date: Thu, 8 Oct 2026 21:40:03 +0200 Subject: [PATCH 2/2] fix(babysit): rate findings by opening badge only and recheck deferrals reviewSeverity accepted a priority badge anywhere in the first 300 characters, so a reviewer comment that quoted `![P2 Badge](x)` became P2 and could be deferred without the user's approval. Match only a badge at the start of the comment, inside Codex's bold and wrappers. classify kept a deferral as long as the thread text was unchanged. After `init` changed the reviewer, a deferred P2 became unrated but stayed discharged. A deferral now holds only while it would still be allowed: the finding is P2/P3, or the user approved deferring it at its current severity. Otherwise it reopens. --- skills/babysit/references/helper.md | 2 +- skills/babysit/scripts/github.mjs | 5 ++-- skills/babysit/scripts/inspect.mjs | 14 ++++++++-- tests/babysit/threshold.test.mjs | 41 +++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 5 deletions(-) diff --git a/skills/babysit/references/helper.md b/skills/babysit/references/helper.md index b3c017e..1bc3c20 100644 --- a/skills/babysit/references/helper.md +++ b/skills/babysit/references/helper.md @@ -12,7 +12,7 @@ Readiness is an aid to final verification, never merge authorization. A submitte Each open finding carries `severity`, read from the Codex priority badge (`P0` to `P3`) on the thread's first comment. It is null when there is no badge or when someone other than a configured reviewer opened the thread. The helper keeps that token and no other comment text. Reason details start with the severity, such as `P2 src/app.ts:12`. -`decide --decision deferred` discharges a finding without a code change, so it stops blocking readiness. It accepts several `--finding` ids at once and records all or none. It works on its own for P2 and P3. For P0, P1, and unrated findings it also needs `--user-approved`, which is recorded as `userApproved` on those findings only. The helper cannot verify the approval; pass the flag only after the user gave it. +`decide --decision deferred` discharges a finding without a code change, so it stops blocking readiness. It accepts several `--finding` ids at once and records all or none. It works on its own for P2 and P3. For P0, P1, and unrated findings it also needs `--user-approved`, which is recorded as `userApproved` on those findings only. The helper cannot verify the approval; pass the flag only after the user gave it. A deferral reopens when the finding's severity changes to one it was not allowed for, such as a P2 that becomes unrated after `init` changes the reviewer. `codeReview.rounds` counts the commits the configured reviewer submitted a review for. A clean pass that Codex reports only as a comment or reaction is not counted. The skill's round budget uses it. `next` lists actions for reasons agents tend to stall on, such as requesting a review of a repaired head. diff --git a/skills/babysit/scripts/github.mjs b/skills/babysit/scripts/github.mjs index 0d2f4df..4648a67 100644 --- a/skills/babysit/scripts/github.mjs +++ b/skills/babysit/scripts/github.mjs @@ -199,10 +199,11 @@ function normalizeReview(node) { /** * The priority badge a Codex review comment opens with, such as `![P2 Badge](…)`: P0 to P3, or null. - * Only this token is kept, never the text around it. + * Only an opening badge counts, inside the bold and `` wrappers Codex puts around it. A badge quoted + * in prose, a blockquote, or code does not rate the finding. Only this token is kept, never the text around it. */ export function reviewSeverity(body) { - const match = /!\[(P[0-3]) Badge\]/u.exec(String(body ?? "").slice(0, 300)); + const match = /^\s*(?:(?:\*\*|__|<(?:sub|sup|b|strong)>)\s*)*!\[(P[0-3]) Badge\]\(/u.exec(String(body ?? "")); return match ? match[1] : null; } diff --git a/skills/babysit/scripts/inspect.mjs b/skills/babysit/scripts/inspect.mjs index 92a021e..45e1053 100644 --- a/skills/babysit/scripts/inspect.mjs +++ b/skills/babysit/scripts/inspect.mjs @@ -44,6 +44,15 @@ export function findingSeverity(thread, policy) { return first && reviewers.has(loginKey(first.author)) ? (first.severity ?? null) : null; } +/** + * Whether a recorded deferral would still be allowed at the finding's current severity. A policy or badge + * change can make a deferred P2 unrated or P1; the deferral then needs the user's approval for that severity. + */ +function deferralHolds(decision, severity) { + if (decision.decision !== "deferred" || DEFERRABLE_SEVERITIES.has(severity)) return true; + return decision.userApproved === true && (decision.severity ?? null) === severity; +} + /** pass, fail, skipped, pending, or unknown. Anything not finished is pending, never complete. */ export function checkOutcome(check) { if (check.kind === "status") { @@ -317,7 +326,8 @@ export function classify(snapshot, state) { // One round per commit the reviewer submitted a review for. const rounds = new Set(reviewerReviews.map((review) => review.commitSha).filter(Boolean)).size; - // Findings: every unresolved thread blocks until a decision (fixed, refuted, or deferred) matches its current content. + // Findings: every unresolved thread blocks until a decision (fixed, refuted, or deferred) matches its current content, + // and a deferral also its current severity. const decisions = state.findingDecisions ?? {}; const open = []; const discharged = []; @@ -338,7 +348,7 @@ export function classify(snapshot, state) { version, }; const decision = decisions[thread.id]; - if (decision && !decision.invalidatedAt && decision.isResolved !== true && decision.threadVersion === version) { + if (decision && !decision.invalidatedAt && decision.isResolved !== true && decision.threadVersion === version && deferralHolds(decision, entry.severity)) { discharged.push({ ...entry, decision: decision.decision, evidence: decision.evidence, commit: decision.commit ?? null }); } else { open.push({ ...entry, reopened: Boolean(decision) }); diff --git a/tests/babysit/threshold.test.mjs b/tests/babysit/threshold.test.mjs index 0aa5281..7f48f8e 100644 --- a/tests/babysit/threshold.test.mjs +++ b/tests/babysit/threshold.test.mjs @@ -27,6 +27,14 @@ describe("reviewSeverity", () => { // A badge buried deep in a reply is not the finding's rating. expect(reviewSeverity(`${"x".repeat(400)} ![P0 Badge](x)`)).toBeNull(); }); + + it("ignores a badge quoted in prose, a blockquote, or code", () => { + expect(reviewSeverity("Example: `![P2 Badge](x)` marks a P2 finding.")).toBeNull(); + expect(reviewSeverity("See ![P3 Badge](x) above.")).toBeNull(); + expect(reviewSeverity("> ![P2 Badge](x) quoted from another thread")).toBeNull(); + expect(reviewSeverity("`![P2 Badge](x)`")).toBeNull(); + expect(reviewSeverity("```md\n![P3 Badge](x)\n```")).toBeNull(); + }); }); describe("classify", () => { @@ -79,6 +87,22 @@ describe("classify", () => { expect(inspection.findings.discharged).toMatchObject([{ threadId: "thread-1", decision: "deferred", severity: "P2" }]); }); + it("reopens a deferral that the current reviewer policy no longer allows", () => { + const open = rated("thread-1", "P2"); + const policy = { ...watchState().policy, codeReviewers: ["other-reviewer"] }; + const deferred = (overrides) => ({ decision: "deferred", evidence: "Follow-up in #14.", threadVersion: threadVersion(open), at: "2026-10-07T09:56:00Z", ...overrides }); + + // The P2 is unrated once its author is no longer a configured reviewer. + const unapproved = classify(snapshot({ threads: [open] }), watchState({ policy, findingDecisions: { "thread-1": deferred({ severity: "P2" }) } })); + expect(unapproved.reasons).toContainEqual(expect.objectContaining({ code: "FINDING_REOPENED", detail: "unrated src/app.ts:12" })); + + // The user's approval covers the severity they approved, not another one. + const p1 = rated("thread-1", "P1"); + const approvedP1 = deferred({ severity: "P1", userApproved: true, threadVersion: threadVersion(p1) }); + expect(classify(snapshot({ threads: [p1] }), watchState({ findingDecisions: { "thread-1": approvedP1 } })).readiness).toBe("ready"); + expect(codes(classify(snapshot({ threads: [p1] }), watchState({ policy, findingDecisions: { "thread-1": approvedP1 } })))).toContain("FINDING_REOPENED"); + }); + it("counts one review round per commit the reviewer reviewed", () => { const reviews = [ review({ id: "r1", commitSha: OLD_HEAD }), @@ -173,6 +197,23 @@ describe("decide --decision deferred", () => { expect((await h.run(["decide", ...PR, "--finding", "thread-1", "--decision", "deferred", "--evidence", "Later."])).error.code).toBe("DEFERRAL_NEEDS_USER"); }); + it("refuses to defer a reviewer comment that only quotes a badge", async () => { + await setup([rated("thread-1", reviewSeverity("Example: `![P2 Badge](x)` marks a P2 finding."))]); + + expect((await h.run(["decide", ...PR, "--finding", "thread-1", "--decision", "deferred", "--evidence", "Later."])).error.code).toBe("DEFERRAL_NEEDS_USER"); + }); + + it("reopens a deferral when init changes the reviewer, and then needs approval to defer", async () => { + await setup([rated("thread-1", "P2")]); + expect((await h.run(["decide", ...PR, "--finding", "thread-1", "--decision", "deferred", "--evidence", "Follow-up in #14."])).code).toBe(0); + expect((await h.run(["inspect", ...PR])).data.inspection.readiness).toBe("ready"); + + expect((await h.run(["init", ...PR, "--code-reviewer", "other-reviewer", "--thread", "thread-a", "--wake-settled"])).code).toBe(0); + const inspection = (await h.run(["inspect", ...PR])).data.inspection; + expect(inspection.reasons).toContainEqual(expect.objectContaining({ code: "FINDING_REOPENED", detail: "unrated src/app.ts:12" })); + expect((await h.run(["decide", ...PR, "--finding", "thread-1", "--decision", "deferred", "--evidence", "Later."])).error.code).toBe("DEFERRAL_NEEDS_USER"); + }); + it("records nothing when a batch names an unknown thread", async () => { await setup([rated("thread-1", "P2")]);