Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughUnread stale completions for the active session now retain a durable transcript entry and trigger a compact steer notice. The notice identifies the task outcome and age, and directs retrieval through ChangesStale Completion Delivery
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Unread stale completions remain retrievable, and the parent is woken to act on the notice. No material merge-blocking risk is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Unread completions gain a compact notification without automatically replaying their reports or granting new privileges. Session and consumption checks remain in place. Delivery-failure recovery remains uncertain, but no new security defect was demonstrated. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for this fix. We independently reviewed head Component verification used exact-SHA source reads and production queue/delivery snippets in memory, with a mocked host and clock:
Update: independently executed the focused suite on the same pinned head. With Node 24.14.1 and pnpm 11.1.1, frozen-lockfile installation with lifecycle scripts disabled succeeded. In an isolated temporary environment with network disabled during tests, this command passed: node --experimental-strip-types --test tests/agents-completion-delivery.test.ts tests/gentle-agents.test.ts138 passed, 0 failed, 0 skipped; exit 0. An earlier sandbox attempt had nine Git-fixture failures because This verifies the focused suite, not the full suite, typecheck, packaging, official CI or live Pi integration. Hidden-message semantics were checked statically against Pi 0.99.1, not exercised in a live host. Could you confirm the repository CI results and a real-host case where an unread completion ages past 90 seconds during a long parent tool call? This evidence supports the scoped fix, but is not a formal approval or a merge-readiness claim. |
|
Thanks for the careful independent run, @MarsSall. Here are the two things you asked for. Repository CINeither workflow has run on Until someone approves it, I ran the Linux
Real-host cases (Pi 0.99.1)Setup. Pi 0.99.1 with gentle-pi 3.7.0. Its Before the patch: 9 casesEach background completion settled while the parent was inside one long tool call:
The result was flushed at the next What happened to them:
After the patch: 3 casesEach produced exactly one hidden
One caveat on my hostMy install also carries a separate local workaround for the idle-wake problem (earendil-works/pi#5581 / #1528). It wraps
To be clear about scope, this is the same as yours: evidence for the scoped fix, not a merge-readiness claim. |
|
Follow-up on the caveat in my previous comment. The ~10 minute delay in the 101 s case came from my local idle-wake wrapper, which decided "idle" by counting It touches this PR in one place. The stale notice here is also a |
Gentleman-Programming#1092) A stale completion was delivered only as a TUI transcript entry. An unread first completion that settled during one long parent tool call therefore never reached the model and requested no wake, so the parent could report the work as still running. The stale path now also sends one compact model-facing notice, without the report, telling the parent to pull the result. The notice takes the same idle/run route as other child content (Gentleman-Programming#1528), so an idle parent gets a coalesced wake through the prompt lifecycle instead of a direct turn. Closes Gentleman-Programming#1092
443dc46 to
9fe8b2e
Compare
|
Updated the branch by merging Another real-host case without the patch (gentle-pi 4.0.0)This adds a case on the current release to the real-host evidence @MarsSall asked for. The installed
This is #1092 on the current release. It shows only the base behaviour; the after-patch real-host cases are in my earlier comment. |
|
Thanks for updating this to use the shared idle/run/hold router. We independently reviewed head node --experimental-strip-types --test tests/agents-completion-delivery.test.ts tests/gentle-agents.test.ts207 passed, 0 failed, 0 skipped, using Node 24.14.1 and pnpm 11.1.1. Tests ran in a network-disabled Bubblewrap sandbox with candidate source read-only. No blocking candidate-caused routing defects were identified in this scope. One non-blocking suggestion at This verifies the focused suites, not the full suite, typecheck, official CI, or live-provider integration. It is scoped verification evidence, not a formal approval or merge-readiness claim. |
Gentleman-Programming#1092) subagent_status returns task metadata, not the report, so a parent that followed it would still leave the result unread, with no further notice. Suggested in review.
|
Thanks a lot for this, @BGamboa13. Your diagnosis of the stale path and the idea of a compact model-facing notice routed through the idle/run/hold router were exactly right, and #1833 keeps that approach. I'm closing this in favor of #1833 because #1821 showed the stale path was only one of three ways a finished task could be lost silently. Forwarding errors were swallowed after the task was marked delivered, and a rejected idle wake was never retried. #1833 fixes all three in one place, so landing both PRs would conflict in the same functions. If you have a moment, a review on #1833 would be very welcome since you know this code well. |
|
Thanks, @Alan-TheGentleman, closing this in favor of #1833 makes sense. I read its diff: the stale path now sends a compact |
Linked issue
Closes #1092
Problem
On
main, a completion that settles as stale is delivered only as a TUI transcript entry:deliverStalecallspi.appendEntry(AGENTS_STALE_RESULT_TYPE, …)and nothing else. When an unread first completion settles while the parent is inside one tool call longer thanSTALE_COMPLETION_MS(90 s), it never reaches the model and requests no wake. The parent can keep reporting the task as running.Approach
gentle-agents.stale-notice,display: false, no report text). The notice names the task and tells the parent to callsubagent_result.consume()) and tasks from other sessions still produce neither the entry nor the notice.The production change is +29/−12, in
extensions/gentle-agents.ts; the change inlib/agents-completion-delivery.tsis a comment update. The rest is tests.Verification
Branch updated by merging
main@2fb7700a.Six new
issue #1092tests intests/gentle-agents.test.ts. Againstmain's sources, four fail:With the change, all six pass. The other two are guards (a consumed result stays silent; another session's task stays silent) and pass both ways.
The CI steps were run locally on Node 24.21.0 with a clean
HOME:pnpm run typecheck,check:runtime-modules,scripts/verify-package-files.mjsandtest:packed-packagepass.pnpm test: provider-contract and runtime-harness pass. In unit-tests, only the fouron a TTY …launcher tests fail, and they fail identically on a cleanmaincheckout in the same environment (WSL, no real TTY).@MarsSall independently reviewed the earlier head
19297002and found no blocking defects. Since then, the notice has been moved onto fix(agents): preserve prompt lifecycle for idle child delivery #1631's router.Out of scope
/treeholds (bug(agents): session messages, /tree holds and pre-run prompts still escape the prompt-lifecycle delivery after #1631 #1638) are covered separately by fix(agents): route incoming session messages through the prompt-lifecycle delivery router (#1638) #1639 and fix(agents): release held content after /tree and keep idle wakes from racing a starting prompt (#1638) #1640.Summary by CodeRabbit