fix(SUP-3293): bound jarvos note capture paths - #93
Conversation
Co-Authored-By: Paperclip <noreply@paperclip.ing>
📝 WalkthroughWalkthroughThis PR adds tool-call timeout enforcement and note-intent argument injection to the jarvOS MCP adapter, and adds default trigger inference plus ignored-capture error messaging with nonzero CLI exit codes to the universal-capture bridge, with accompanying tests. ChangesMCP Tool Timeout and Note Intent
Ignored-Capture Handling for Triggerless Payloads
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant JarvosMcp
participant ToolHandler
Client->>JarvosMcp: tools/call request
JarvosMcp->>JarvosMcp: withToolTimeout(callTool(...))
JarvosMcp->>ToolHandler: execute jarvos_create_note(noteCaptureArgs(args))
alt tool completes in time
ToolHandler-->>JarvosMcp: result
JarvosMcp-->>Client: response
else timeout exceeded
JarvosMcp-->>Client: JSON-RPC error -32000 timeout
end
sequenceDiagram
participant CLI
participant CaptureWithJarvos
participant Stdout
participant Stderr
CLI->>CaptureWithJarvos: captureWithJarvos(parsed)
CaptureWithJarvos->>CaptureWithJarvos: defaultProgrammaticTrigger(captureEvent)
CaptureWithJarvos-->>CLI: result with routing.plan.ignored, error
CLI->>Stdout: print result
alt routing.plan.ignored is true
CLI->>Stderr: print result.error (ignoredCaptureMessage)
CLI->>CLI: process.exitCode = 2
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52a3390784
ℹ️ 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".
| function withToolTimeout(name, operation, timeoutMs = toolTimeoutMs()) { | ||
| return new Promise((resolve, reject) => { | ||
| let settled = false; | ||
| const timer = setTimeout(() => { | ||
| if (settled) return; | ||
| settled = true; | ||
| reject(toolTimeoutError(name, timeoutMs)); | ||
| }, timeoutMs); |
There was a problem hiding this comment.
When an MCP tool blocks synchronously, this timer cannot fire until the operation has already returned, so the new timeout does not actually bound all tool calls. For example, jarvos_session_thread_write can sit in the synchronous lock wait in acquireLockFile (modules/jarvos-agent-context/src/index.js, around the sleepSync retry loop), so a stale .lock can still hold the MCP request for that lock timeout instead of returning the configured JARVOS_MCP_TOOL_TIMEOUT_MS error. The timeout needs to be enforced inside those synchronous paths or the work needs to be made cancellable/asynchronous.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/jarvos-agent-context/scripts/jarvos-mcp.js`:
- Around line 190-213: withToolTimeout currently only rejects on timeout and
drops late completions, so side effects like create-note can still happen and be
duplicated on retry. Update withToolTimeout and the callTool path it wraps so
timed-out operations are either actually aborted/cancelled or made idempotent
via a request key, and ensure late resolves/rejects are logged or surfaced
instead of silently ignored. Use the withToolTimeout helper and the
jarvos_create_note/callTool flow as the main places to apply the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c92834eb-81ef-4876-a61c-fcaea8d18b61
📒 Files selected for processing (6)
CHANGELOG.mdmodules/jarvos-agent-context/scripts/jarvos-mcp.jsmodules/jarvos-agent-context/test/agent-context.test.jsmodules/jarvos-secondbrain/bridge/capture/README.mdmodules/jarvos-secondbrain/bridge/capture/src/universal-capture.jsmodules/jarvos-secondbrain/tests/universal-capture.test.js
| function withToolTimeout(name, operation, timeoutMs = toolTimeoutMs()) { | ||
| return new Promise((resolve, reject) => { | ||
| let settled = false; | ||
| const timer = setTimeout(() => { | ||
| if (settled) return; | ||
| settled = true; | ||
| reject(toolTimeoutError(name, timeoutMs)); | ||
| }, timeoutMs); | ||
|
|
||
| Promise.resolve() | ||
| .then(operation) | ||
| .then((result) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timer); | ||
| resolve(result); | ||
| }, (error) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(timer); | ||
| reject(error); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Timeout doesn't cancel the underlying operation — risk of duplicate writes on retry.
withToolTimeout races the operation against a timer but never cancels/aborts the operation itself. If callTool (e.g. createNote via jarvos_create_note) eventually resolves after the timeout fires, the result is silently dropped in the settled-check, and the note may still get created. A caller who sees the timeout error and retries the same create-note call could end up creating a duplicate note, since there's no idempotency guard.
Consider surfacing/logging late completions, or adding an idempotency key so retries after a timeout don't duplicate side effects.
Also applies to: 321-324
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 192-196: Avoid using the initial state variable in setState
Context: setTimeout(() => {
if (settled) return;
settled = true;
reject(toolTimeoutError(name, timeoutMs));
}, timeoutMs)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/jarvos-agent-context/scripts/jarvos-mcp.js` around lines 190 - 213,
withToolTimeout currently only rejects on timeout and drops late completions, so
side effects like create-note can still happen and be duplicated on retry.
Update withToolTimeout and the callTool path it wraps so timed-out operations
are either actually aborted/cancelled or made idempotent via a request key, and
ensure late resolves/rejects are logged or surfaced instead of silently ignored.
Use the withToolTimeout helper and the jarvos_create_note/callTool flow as the
main places to apply the fix.
💡 Codex Review Preserve explicit idea triggers before defaulting to notes
When a programmatic capture includes both a title and text, this line forces ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dc4ff2727
ℹ️ 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".
|
|
||
| function defaultProgrammaticTrigger(event = {}) { | ||
| if (event.trigger || detectTrigger(event)) return event.trigger; | ||
| return isLikelyProgrammaticCapture(event) ? 'note' : event.trigger; |
There was a problem hiding this comment.
For triggerless CLI/library calls that include a title and body, normalizeSource turns the required source into an object with tool, so isLikelyProgrammaticCapture is true for ordinary CaptureEvent v2 inputs and this branch injects trigger: "note". In that scenario text without note:, save this, etc. writes a note and exits 0 instead of taking the new ignored/exit-2 path described by ignoredCaptureMessage and the capture contract's non-capture behavior; require an explicit trigger/capture phrase or a separate opt-in flag before defaulting to note.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/jarvos-secondbrain/bridge/capture/src/universal-capture.js (1)
139-142: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify the discarded
detectTriggerresult.
if (event.trigger || detectTrigger(event)) return event.trigger;callsdetectTrigger(event)only to check truthiness, then returns the original (possibly falsy)event.trigger, silently discarding whateverdetectTriggeractually resolved. Behavior relies on downstream routing re-deriving the trigger from text independently, which isn't obvious from this function alone and reads like a bug (e.g.,event.trigger || detectTrigger(event)).♻️ Suggested clarification
function defaultProgrammaticTrigger(event = {}) { - if (event.trigger || detectTrigger(event)) return event.trigger; - return isLikelyProgrammaticCapture(event) ? 'note' : event.trigger; + // Preserve existing trigger/keyword-detection behavior as-is; only inject a + // 'note' fallback when neither an explicit trigger nor keyword detection + // would resolve one downstream. + const alreadyResolvable = Boolean(event.trigger || detectTrigger(event)); + if (alreadyResolvable) return event.trigger; + return isLikelyProgrammaticCapture(event) ? 'note' : event.trigger; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/jarvos-secondbrain/bridge/capture/src/universal-capture.js` around lines 139 - 142, The defaultProgrammaticTrigger function is discarding the resolved value from detectTrigger(event) by only using it as a truthiness check and then returning event.trigger, which makes the logic unclear. Update this branch so the detected trigger value is actually preserved and returned, and keep the fallback to isLikelyProgrammaticCapture(event) only when no trigger is available. Use defaultProgrammaticTrigger, detectTrigger, and isLikelyProgrammaticCapture as the key symbols while adjusting the control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@modules/jarvos-secondbrain/bridge/capture/src/universal-capture.js`:
- Around line 139-142: The defaultProgrammaticTrigger function is discarding the
resolved value from detectTrigger(event) by only using it as a truthiness check
and then returning event.trigger, which makes the logic unclear. Update this
branch so the detected trigger value is actually preserved and returned, and
keep the fallback to isLikelyProgrammaticCapture(event) only when no trigger is
available. Use defaultProgrammaticTrigger, detectTrigger, and
isLikelyProgrammaticCapture as the key symbols while adjusting the control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f7c380b-2ddb-4b75-ae57-2d426d103a3f
📒 Files selected for processing (4)
modules/jarvos-agent-context/scripts/jarvos-mcp.jsmodules/jarvos-agent-context/test/agent-context.test.jsmodules/jarvos-secondbrain/bridge/capture/src/universal-capture.jsmodules/jarvos-secondbrain/tests/universal-capture.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/jarvos-agent-context/scripts/jarvos-mcp.js
Summary
trigger: "note"intent forjarvos_create_noteMCP calls and wrap MCP tool execution with a bounded timeout error.ok:falseoutput for library callers.Verification
node --test modules/jarvos-agent-context/test/agent-context.test.jsnode --test modules/jarvos-secondbrain/tests/universal-capture.test.js modules/jarvos-secondbrain/tests/keyword-capture-router.test.jstrigger:"note"capture CLI exits 0 and writes 1 note + 1 journal filejarvos_create_notereturns# jarvOS Note Createdgit diff --checknpm testLocal dogfood sync
/Users/andrew/jarvOSis currently on dirty branchSUP-1948/vault-provenance-qmdand lacks the newer universal-capture files fromorigin/main, so the patch cannot be safely applied there without mixing unrelated dogfood work. The clean branch/PR is the safe sync source; local dogfood should update from this PR after the older checkout is reconciled or replaced with a fresh main-based worktree.Documentation impact: changelog + capture bridge operator docs.
Summary by CodeRabbit
New Features
Bug Fixes