feat(hitl): expose whether a confirmation is still pending - #1590
Conversation
A HITL confirmation's pending entry holds the `resolve`/`reject` of the suspended tool call, so it exists only in the memory of the manager that issued it. Nothing outside that manager could ask whether it is still there. That gap is not academic, because emitting a response is not proof the decision landed. `setupHITLEventForwarding` installs a permanent listener for `hitl:confirmation-response` at construction, so `emitter.emit(...)` reports a listener was invoked whether or not anything is waiting. A consumer that rebuilds a session from persisted state constructs a NEW NeuroLink instance whose pending set is empty; the emit is accepted, `processUserResponse` logs "No pending confirmation found for ID" and returns, and the consumer has no way to know. The case that motivated this had a merchant told a gated write had been approved when it never ran. Adds `HITLManager.hasPendingConfirmation(confirmationId)` beside the existing `getPendingCount()`, and `NeuroLink.hasPendingHITLConfirmation(confirmationId)` so consumers holding the facade can reach it — `hitlManager` is private, so the manager's own accessors were unreachable from outside. The predicate is also added to the `HITLManager` contract type in `types/hitl.ts`. Without it the method would be invisible through `toolRegistry.getHITLManager()` and `externalServerManager.getHITLManager()`, which are both typed against that type rather than the class. Deliberate limits, documented on the method rather than left to be discovered: - It returns false both when HITL was never configured and when the id is unknown or already settled. Both mean "emitting here achieves nothing", which is the question being asked; a caller needing to tell a config mistake from an expired confirmation should read the config instead. - It is advisory, not atomic — it reports the state at the instant it is called, so callers should emit on the answer without an `await` in between. For the case it exists for, an instance that never issued the id, absence cannot become presence, so it cannot go stale in the unsafe direction. An atomic alternative — making `processUserResponse` report its own outcome, or forwarding an applied/orphaned event — was considered and not taken here. It does not reach a consumer that resolves confirmations by emitting on the event emitter, since the return value is swallowed by the forwarding chain; and it is a larger change to the event contract than this needs to be. docs/api regenerated, as CI checks it for drift.
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change adds pending HITL confirmation checks to ChangesHITL pending confirmation checks
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change exposes whether a HITL confirmation is still pending without altering confirmation execution behavior. The localized API additions are merge-ready after normal checks, with no actionable merge-blocking risk remaining. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. (4 skipped: 3 unsupported, 1 too large.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
src/lib/hitl/hitlManager.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. error TS5012: Cannot read file '/.svelte-kit/tsconfig.json': ENOENT: no such file or directory, open '/.svelte-kit/tsconfig.json'. src/lib/neurolink.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). src/lib/types/hitl.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency). 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 |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
|
🎉 This PR is included in version 12.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The gap
A HITL confirmation's pending entry holds the
resolve/rejectof the suspended tool call, so it lives only in the memory of the manager that issued it. Nothing outside that manager could ask whether it is still there.That matters because emitting a response is not proof the decision landed:
setupHITLEventForwardinginstalls a permanent listener forhitl:confirmation-responseat construction, soemitter.emit(...)reports a listener was invoked whether or not anything is waiting.NeuroLinkinstance whose pending set is empty.processUserResponselogsNo pending confirmation found for IDand returns, and the consumer has no way to know.The case that motivated this: a merchant was told a gated write had been approved when it never ran.
What this adds
HITLManager.hasPendingConfirmation(confirmationId)getPendingCount()NeuroLink.hasPendingHITLConfirmation(confirmationId)hitlManageris private, so the manager's accessors were unreachable from outside the facadeHITLManagercontract type intypes/hitl.tstoolRegistry.getHITLManager()andexternalServerManager.getHITLManager(), which are typed against that type rather than the classThat third one was found by review, not by me — it would have shipped as a method that two of the three access paths could not see.
Deliberate limits, documented on the method
Stated in the JSDoc rather than left to be discovered:
falsefor two different situations — HITL never configured, and id unknown/already settled. Both mean "emitting here achieves nothing", which is the question being asked. A caller needing to tell a config mistake from an expired confirmation should read the config instead.awaitin between. For the case it exists for — an instance that never issued the id — absence cannot become presence, so it cannot go stale in the unsafe direction.An alternative that was considered and not taken
Making
processUserResponsereport its own outcome, or forwarding an applied/orphaned event, would be atomic by construction and is arguably the better general shape.It is not taken here because it does not reach the consumer that motivated this: one that resolves confirmations by emitting on the event emitter never sees a return value, since it is swallowed by the forwarding chain. It is also a larger change to the event contract than this needs to be. Happy to follow up with it if maintainers prefer that direction.
Verification
pnpm run pre-push— 14/14 passpnpm run build— passes, publintAll good!prettier --check— clean on all three source filespnpm run docs:api— regenerated (CI checks for drift). Confirmed the clean branch had no drift beforehand, so everything indocs/apihere is attributable to this change; the third doc file is only source-line links shifting by the 3 lines added totypes/hitl.tstsc --noEmit— 575 pre-existing errors onrelease, 575 with this change, the only diff being line numbers shifting by the added lines. No new errors. (Worth noting separately thattypecheckdoes not currently pass onrelease.)distwith correct.d.tssignaturesNo changeset:
release.ymlrunssemantic-release, so thefeat(hitl):type drives the minor bump..changeset/appears vestigial.Summary by CodeRabbit
New Features
Documentation