Repository navigation
feat: add shared PR babysitting and idempotent thread sends - #12
Conversation
- Add the babysit skill and helper for tracking PR evidence and delivering resumable events - Add idempotency keys to thread sends and document their retry behavior
Mira PR WalkthroughThis PR adds a shared PR babysitting helper and skill covering GitHub evidence collection, resumable state, event delivery, and scheduling. It also adds idempotency keys to thread sends so retries can reuse an existing delivery. Documentation and tests cover the new workflows, but no test execution results were provided. graph LR
babysit["skills/babysit/scripts/babysit.mjs"] --> github["skills/babysit/scripts/github.mjs"]
babysit --> state["skills/babysit/scripts/state.mjs"]
wake["skills/babysit/scripts/wake.mjs"] --> state
wake --> cli["src/cli.ts"]
cli --> service["src/service.ts"]
Confidence: 3/5 ◉◉◉○○ Merge safety unverified
Key files to review:
27 files reviewed · 2 comments (
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![clark-review[bot]](https://avatars.githubusercontent.com/u/8057288?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mira Review Summary
The check evaluation at skills/babysit/scripts/inspect.mjs:190 can skip explicitly required checks while reporting the PR ready. At skills/babysit/scripts/inspect.mjs:148, readiness can ignore merge gates returned by the final GitHub read.
Key Issues
| Issue | Location | |
|---|---|---|
| 🔴 | Explicitly required checks can be skipped while readiness reports ready. | skills/babysit/scripts/inspect.mjs:190 |
| 🔴 | Readiness can ignore merge gates returned by the final GitHub read. | skills/babysit/scripts/inspect.mjs:148 |
| for (const name of policy.requiredChecks ?? []) { | ||
| if (!checks.some((check) => check.name === name)) add("pending", "REQUIRED_CHECK_MISSING", name); | ||
| } |
There was a problem hiding this comment.
Bug
Enforce explicitly required checks when they are present
policy.requiredChecks only checks whether a named check exists; its outcome still uses GitHub's required flag. A check explicitly required through --required-check can therefore be SKIPPED or NEUTRAL with required: false and leave readiness ready when the rollup is successful. Apply the configured requirement before classifying outcomes so explicitly required checks must pass.
Prompt for AI Agents
In skills/babysit/scripts/inspect.mjs, update the checks mapping and requiredChecks handling at lines 163–192 so names in policy.requiredChecks have effective required status true regardless of GitHub's isRequired value. Preserve missing-check detection and add regression tests in tests/babysit/inspect.test.mjs for explicitly required skipped and failed checks reported by GitHub as optional.
Not useful? Reply
@miracodeai rejectto dismiss this suggestion.
There was a problem hiding this comment.
Fixed in 4885c22. Checks named by --required-check now have effective required status true before outcome classification, regardless of GitHub's isRequired value. Added regression cases for SKIPPED, NEUTRAL, and FAILURE reported as optional; pnpm check passes (367 passed, 2 skipped).
| }; | ||
| } | ||
|
|
||
| const pr = snapshot.pr; |
There was a problem hiding this comment.
Bug
Evaluate readiness against the final core fields
The returned PR view uses snapshot.finalCore, but readiness evaluates mergeability, draft status, and review decision from the initial snapshot.pr. The fetcher's coreChanged comparison does not compare those fields, so it can return a complete snapshot with different final merge gates. For example, an initial CLEAN state followed by final BLOCKED with the same compared fields produces ready alongside a returned PR that says BLOCKED. Use the final core fields for readiness while retaining the collected lists.
| const pr = snapshot.pr; | |
| const pr = { ...snapshot.pr, ...(snapshot.finalCore ?? {}) }; |
Prompt for AI Agents
In skills/babysit/scripts/inspect.mjs at line 148, combine snapshot.pr with snapshot.finalCore so final core fields override initial fields while contexts, reviews, threads, comments, and reactions remain available. Add regression tests in tests/babysit/inspect.test.mjs where finalCore changes mergeStateStatus or reviewDecision without changing the head, base, state, or updatedAt, and assert readiness uses the final gates.
Apply this code change:
const pr = { ...snapshot.pr, ...(snapshot.finalCore ?? {}) };
Not useful? Reply
@miracodeai rejectto dismiss this suggestion.
There was a problem hiding this comment.
Fixed in 4885c22. Readiness now overlays the final core fields onto the collected snapshot, retaining checks, reviews, threads, comments, and reactions. Added regression cases for final BLOCKED, REVIEW_REQUIRED, draft, and conflict gates while head/base/state/updatedAt remain unchanged; pnpm check passes (367 passed, 2 skipped).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe4ed1a968
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (decision && decision.threadVersion === version) { | ||
| discharged.push({ ...entry, decision: decision.decision, evidence: decision.evidence, commit: decision.commit ?? null }); |
There was a problem hiding this comment.
When a previously decided thread is resolved and later reopened without a new or edited comment, its threadVersion remains unchanged, so this branch continues to classify the reopened finding as discharged and can report the PR ready. Reopening is itself a reviewer signal that the finding is not settled; track the resolved-to-unresolved transition or otherwise invalidate the stored decision when it occurs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93e707f. Complete helper observations now invalidate prior decisions when a thread is resolved; reopening unchanged comments remains blocked until an explicit new decision. Decisions made while already resolved also cannot cover reopening. Added a tick -> resolve -> reopen -> inspect -> re-decide regression. GitHub exposes current resolution state rather than resolution history; cycles entirely between observations remain a documented limitation, and inspect stays read-only. pnpm check passed: 376 tests passed, 2 skipped, typecheck and build passed.
| const activity = [ | ||
| ...reviewsAtHead.map((review) => latest(review.submittedAt, review.lastEditedAt)), | ||
| ...reviewerComments.map((comment) => latest(comment.createdAt, comment.lastEditedAt)), | ||
| ...pr.threads.flatMap((thread) => thread.comments.filter((comment) => isReviewer(comment.author)).map((comment) => latest(comment.createdAt, comment.lastEditedAt))), | ||
| ]; |
There was a problem hiding this comment.
When review evidence already exists for the head, the post-evidence activity check includes reviews and comments but omits unboundSignals from reactions. Consequently, if the configured reviewer adds a new reaction after evidence was recorded—even a thumbs-down—the helper emits a changed-news event but still classifies the PR as ready. Persist the reaction set covered by the evidence or conservatively invalidate evidence when a new unbound reviewer reaction appears.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93e707f. New review evidence captures the configured reviewer's reaction set from a complete live snapshot of the recorded head. Any later reaction outside the newest evidence's set keeps readiness unknown, including thumbs-down; truncated reaction lists also cannot establish readiness. Added reaction invalidation and covered-reaction regressions. pnpm check passed: 376 tests passed, 2 skipped, typecheck and build passed.
| const pr = snapshot.pr; | ||
| const head = pr.headSha; |
There was a problem hiding this comment.
During a multi-page read, GitHub can recompute mergeable or mergeStateStatus between the initial and final core queries without changing the head, base, state, or updatedAt; coreChanged therefore accepts the snapshot, but readiness is still evaluated from the initial snapshot.pr. For example, the returned inspection.pr can show the final DIRTY state while this logic reports ready from the earlier CLEAN state. Overlay snapshot.finalCore onto the paginated PR data, or compare every readiness-relevant core field before marking the read complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in 4885c22 before this review of fe4ed1a finished. Classification overlays snapshot.finalCore onto the collected PR data. Regression cases cover final BLOCKED, REVIEW_REQUIRED, draft, and CONFLICTING/DIRTY with unchanged head/base/state/updatedAt. They remain passing. pnpm check passed: 376 tests passed, 2 skipped, typecheck and build passed.
| const ghPath = values.gh === undefined ? null : text(values.gh, "--gh", 1000); | ||
| const cliPath = path.resolve(deps.cwd, values["t3code-cli"] ?? deps.env.T3CODE_CLI ?? deps.bundledCli); |
There was a problem hiding this comment.
If --gh is supplied as a relative executable path such as ./tools/gh, initialization stores it unchanged, but an OS-scheduled tick normally runs with a different working directory. The later execFile then cannot find the executable, so durable monitoring degrades with GH_NOT_FOUND even though initialization succeeded. Preserve bare command names for PATH lookup, but resolve path-like relative values against the initialization directory before saving them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93e707f. Path-like --gh values are resolved against the initialization working directory before persistence; bare command names retain PATH lookup. Regression tests cover ./tools/gh, tools\gh.exe, gh, and gh-custom, then tick from a different working directory and verify the same configured executable is used. pnpm check passed: 376 tests passed, 2 skipped, typecheck and build passed.
| entry = { head, url, note, at }; | ||
| state.reviewEvidence ??= []; | ||
| state.reviewEvidence.push(entry); |
There was a problem hiding this comment.
Re-running the same record --review-evidence command appends an identical source with a new at value, and classification uses the maximum evidence timestamp. If reviewer activity occurred after the original task evidence, retrying or resuming this record command later makes that activity appear older than the evidence and can incorrectly restore ready without a new review. Deduplicate the same head/source evidence while preserving its original timestamp, or require genuinely new evidence before advancing the timestamp.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93e707f. Evidence is deduplicated by full head and source URL, preserving the first timestamp, note, and covered reaction set even when a retry changes the note. A regression retries after new reviewer comments/reactions and confirms readiness stays unknown; a genuinely new completed task URL can record fresh evidence. pnpm check passed: 376 tests passed, 2 skipped, typecheck and build passed.
|
@codex review Please review the current head, 93e707f, after the fixes for all five findings from the opening review. The final-core fix is in 4885c22; reaction evidence, duplicate-source timestamps, observed thread reopening, and custom gh paths are fixed in 93e707f. Each finding has a reply and regression coverage. pnpm check passed: 376 tests passed, 2 skipped, typecheck and build passed. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Adds
t3code babysitto collect GitHub review and CI evidence, preserve resumable PR state, and deliver change events to T3 threads. Addsthreads send --idempotency-keyand--no-start-desktopso background delivery retries preserve the exact message and avoid launching the desktop.Readiness requires completion evidence for the reviewed head and successful local verification. Configured required checks must pass, and readiness uses the final GitHub merge gates. Review evidence captures the observed reaction set; repeat records of a head/source preserve their original timestamp. Observed thread resolutions invalidate previous decisions, and custom gh paths remain stable across scheduler working directories. Includes the bundled skill, documentation, and regression coverage.
Validation:
pnpm checkpassed on 93e707f (typecheck, 376 tests passed, 2 skipped, build). CLI integration tests exercise helper commands and keyed message delivery against an isolated fake protocol 2 server. The live helper reads this PR successfully. GitHub CI passed on Node 22.16.0 and Node 24.10.0 for the final head. Codex code review completed on 93e707f with no major issues; completion is corroborated by the service summary and reviewed-commit comment. An additional final-head security review was requested, but startup/completion is unverified; the visible security summary covers fe4ed1a. This additional review is not a required repository check, and the service reports mergeGateEnabled=false. No final-head security-review pass is claimed. All five opening Codex findings have replies explaining their fixes.Limitations: the helper does not merge, request reviews, or register schedules. T3 native watching is preferred. Retry deduplication depends on retained T3 receipts or message/run history; delivery acceptance is separate from acknowledgment that work was handled. GitHub's current thread state cannot reveal a resolution/reopening cycle entirely between helper observations, so live threads must be rechecked before merging.