SUP-1535: Add Codex jarvOS adapter#30
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis pull request introduces the ChangesjarvOS Agent-Context Module and Codex Runtime
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: f41d1fe1c3
ℹ️ 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".
Comment on lines
+48
to
+52
| JARVOS_ROOT, | ||
| 'modules', | ||
| 'jarvos-secondbrain', | ||
| 'packages', | ||
| 'jarvos-secondbrain-notes', |
There was a problem hiding this comment.
When @jarvos/agent-context is installed as an npm/tarball package alongside @jarvos/secondbrain, MODULE_ROOT/../.. resolves to node_modules, so this hard-coded node_modules/modules/jarvos-secondbrain/... path does not exist and createNote/jarvos_create_note throw before writing anything. I reproduced this by packing and installing both new packages locally; the failure is Cannot find module '/tmp/.../node_modules/modules/jarvos-secondbrain/packages/.../write-to-vault.js'. Use require.resolve('@jarvos/secondbrain/...', { paths: [...] }) or an exported secondbrain API before falling back to the monorepo source path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/modules-smoke-test.js (1)
271-273: ⚡ Quick winAssert the full MCP tool contract in smoke tests.
This check only verifies 2 tools, so regressions in
jarvos_current_workorjarvos_startup_briefwon’t fail smoke.Proposed patch
- const toolNames = mcp.TOOLS.map((tool) => tool.name); - if (toolNames.includes('jarvos_recall') && toolNames.includes('jarvos_create_note')) { - ok('jarvos MCP server exposes recall and note tools'); + const toolNames = new Set(mcp.TOOLS.map((tool) => tool.name)); + const expectedTools = [ + 'jarvos_current_work', + 'jarvos_recall', + 'jarvos_create_note', + 'jarvos_startup_brief', + ]; + if (expectedTools.every((name) => toolNames.has(name))) { + ok('jarvos MCP server exposes expected tool set'); } else { - bad('jarvos MCP tools', new Error(JSON.stringify(toolNames))); + bad('jarvos MCP tools', new Error(JSON.stringify([...toolNames]))); }🤖 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 `@tests/modules-smoke-test.js` around lines 271 - 273, The smoke test currently only checks for two tools; update the assertion to validate the full MCP tool contract by comparing mcp.TOOLS (or its mapped toolNames) against the expected list of tool names (e.g., include 'jarvos_recall', 'jarvos_create_note', 'jarvos_current_work', 'jarvos_startup_brief') — ensure you assert both presence and that no extra/missing tools exist (exact match or same-size+contains) so regressions in jarvos_current_work or jarvos_startup_brief will fail the test.
🤖 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`:
- Line 100: The condition `if (!id && String(method ||
'').startsWith('notifications/')) return;` treats 0 as "no id" and can drop
valid JSON-RPC requests; change the check to explicitly test for null/undefined
(e.g., `if (id == null && String(method || '').startsWith('notifications/'))
return;` or `if ((id === undefined || id === null) && String(method ||
'').startsWith('notifications/')) return;`) so the variables `id` and `method`
are handled correctly.
In `@modules/jarvos-agent-context/src/index.js`:
- Around line 153-154: The code converts options.limit with Number(...) and can
produce NaN which is sent to paperclipJson; update the logic around the const
limit = Number(options.limit || 200) so it guards non-numeric inputs by
validating and falling back to a safe default (e.g., 200) or clamping to a
max/min before building the query string; locate the limit construction and the
paperclipJson call (references: options.limit, limit,
paperclipJson(`/companies/${auth.companyId}/issues?limit=${limit}`, auth)) and
replace with validation that ensures limit is a valid integer before passing it
into the URL.
- Around line 114-116: Wrap the fetch call that assigns const response in an
AbortController: create const controller = new AbortController(), pass signal:
controller.signal into fetch, and start a timer (e.g., const timeout =
setTimeout(() => controller.abort(), 10000)) to abort after 10s; after fetch
completes clearTimeout(timeout). Also handle the abort case by catching the
thrown DOMException/AbortError and surface a clear error (or rethrow) so
upstream code can handle timeouts. Ensure you preserve the existing URL
construction using auth.apiUrl, auth.apiKey and pathname when calling fetch.
In `@modules/jarvos-agent-context/src/note-contract.js`:
- Line 15: The frontmatter regex assigned to const match currently only matches
LF newlines; update the pattern used in note-contract.js (the
/^---\n([\s\S]*?)\n---\n?/ regex) to accept CRLF by replacing each \n with \r?\n
(e.g., use /^---\r?\n([\s\S]*?)\r?\n---\r?\n?/), so frontmatter parsing becomes
CRLF-safe while preserving existing behavior for LF.
---
Nitpick comments:
In `@tests/modules-smoke-test.js`:
- Around line 271-273: The smoke test currently only checks for two tools;
update the assertion to validate the full MCP tool contract by comparing
mcp.TOOLS (or its mapped toolNames) against the expected list of tool names
(e.g., include 'jarvos_recall', 'jarvos_create_note', 'jarvos_current_work',
'jarvos_startup_brief') — ensure you assert both presence and that no
extra/missing tools exist (exact match or same-size+contains) so regressions in
jarvos_current_work or jarvos_startup_brief will fail the test.
🪄 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: 9fc28231-304f-4e10-9e53-c26d01055315
📒 Files selected for processing (12)
README.mdmodules/README.mdmodules/jarvos-agent-context/README.mdmodules/jarvos-agent-context/package.jsonmodules/jarvos-agent-context/scripts/jarvos-mcp.jsmodules/jarvos-agent-context/src/index.jsmodules/jarvos-agent-context/src/note-contract.jsmodules/jarvos-agent-context/test/agent-context.test.jsmodules/jarvos-secondbrain/bridge/provenance/src/link-to-journal.jsruntimes/codex/README.mdruntimes/codex/setup.shtests/modules-smoke-test.js
| async function handle(message) { | ||
| if (!message || typeof message !== 'object') return; | ||
| const { id, method, params } = message; | ||
| if (!id && String(method || '').startsWith('notifications/')) return; |
There was a problem hiding this comment.
Use explicit id-absence check for notifications.
Line 100 uses !id, which treats 0 as “no id”. Prefer checking for undefined/null explicitly to avoid misclassifying valid JSON-RPC ids.
Proposed fix
- if (!id && String(method || '').startsWith('notifications/')) return;
+ if ((id === undefined || id === null) && String(method || '').startsWith('notifications/')) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!id && String(method || '').startsWith('notifications/')) return; | |
| if ((id === undefined || id === null) && String(method || '').startsWith('notifications/')) return; |
🤖 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` at line 100, The
condition `if (!id && String(method || '').startsWith('notifications/'))
return;` treats 0 as "no id" and can drop valid JSON-RPC requests; change the
check to explicitly test for null/undefined (e.g., `if (id == null &&
String(method || '').startsWith('notifications/')) return;` or `if ((id ===
undefined || id === null) && String(method || '').startsWith('notifications/'))
return;`) so the variables `id` and `method` are handled correctly.
Comment on lines
+114
to
+116
| const response = await fetch(`${auth.apiUrl}${pathname.startsWith('/api') ? pathname : `/api${pathname}`}`, { | ||
| headers: { Authorization: `Bearer ${auth.apiKey}` }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "async function paperclipJson|fetch\\(|AbortController|signal\\s*:" modules/jarvos-agent-context/src/index.js -n -C2Repository: levineam/jarvOS
Length of output: 418
Add a request timeout for Paperclip fetch calls.
This fetch call has no timeout and can hang indefinitely if the upstream server stalls, blocking MCP responses.
Wrap the fetch in an AbortController with a reasonable timeout (e.g., 10 seconds):
async function paperclipJson(pathname, auth) {
if (!auth.apiUrl || !auth.apiKey) {
throw new Error('Paperclip API is not configured');
}
+ const controller = new AbortController();
+ const timeout = setTimeout(() => controller.abort(), 10000);
+ let response;
+ try {
- const response = await fetch(`${auth.apiUrl}${pathname.startsWith('/api') ? pathname : `/api${pathname}`}`, {
+ response = await fetch(`${auth.apiUrl}${pathname.startsWith('/api') ? pathname : `/api${pathname}`}`, {
headers: { Authorization: `Bearer ${auth.apiKey}` },
+ signal: controller.signal,
});
+ } finally {
+ clearTimeout(timeout);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const response = await fetch(`${auth.apiUrl}${pathname.startsWith('/api') ? pathname : `/api${pathname}`}`, { | |
| headers: { Authorization: `Bearer ${auth.apiKey}` }, | |
| }); | |
| async function paperclipJson(pathname, auth) { | |
| if (!auth.apiUrl || !auth.apiKey) { | |
| throw new Error('Paperclip API is not configured'); | |
| } | |
| const controller = new AbortController(); | |
| const timeout = setTimeout(() => controller.abort(), 10000); | |
| let response; | |
| try { | |
| response = await fetch(`${auth.apiUrl}${pathname.startsWith('/api') ? pathname : `/api${pathname}`}`, { | |
| headers: { Authorization: `Bearer ${auth.apiKey}` }, | |
| signal: controller.signal, | |
| }); | |
| } finally { | |
| clearTimeout(timeout); | |
| } |
🤖 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/src/index.js` around lines 114 - 116, Wrap the
fetch call that assigns const response in an AbortController: create const
controller = new AbortController(), pass signal: controller.signal into fetch,
and start a timer (e.g., const timeout = setTimeout(() => controller.abort(),
10000)) to abort after 10s; after fetch completes clearTimeout(timeout). Also
handle the abort case by catching the thrown DOMException/AbortError and surface
a clear error (or rethrow) so upstream code can handle timeouts. Ensure you
preserve the existing URL construction using auth.apiUrl, auth.apiKey and
pathname when calling fetch.
Comment on lines
+153
to
+154
| const limit = Number(options.limit || 200); | ||
| const payload = await paperclipJson(`/companies/${auth.companyId}/issues?limit=${limit}`, auth); |
There was a problem hiding this comment.
Guard limit against non-numeric values.
If options.limit is non-numeric, Line 154 sends limit=NaN, which can fail upstream requests.
Proposed fix
- const limit = Number(options.limit || 200);
+ const parsedLimit = Number(options.limit);
+ const limit = Number.isFinite(parsedLimit) && parsedLimit > 0 ? parsedLimit : 200;🤖 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/src/index.js` around lines 153 - 154, The code
converts options.limit with Number(...) and can produce NaN which is sent to
paperclipJson; update the logic around the const limit = Number(options.limit ||
200) so it guards non-numeric inputs by validating and falling back to a safe
default (e.g., 200) or clamping to a max/min before building the query string;
locate the limit construction and the paperclipJson call (references:
options.limit, limit,
paperclipJson(`/companies/${auth.companyId}/issues?limit=${limit}`, auth)) and
replace with validation that ensures limit is a valid integer before passing it
into the URL.
| } | ||
|
|
||
| function parseFrontmatter(content) { | ||
| const match = /^---\n([\s\S]*?)\n---\n?/.exec(String(content || '')); |
There was a problem hiding this comment.
Frontmatter parser is not CRLF-safe.
Line 15 only matches LF newlines. Notes saved with CRLF can fail contract verification even when frontmatter is valid.
Proposed fix
- const match = /^---\n([\s\S]*?)\n---\n?/.exec(String(content || ''));
+ const match = /^---\r?\n([\s\S]*?)\r?\n---\r?\n?/.exec(String(content || ''));🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 15-15: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/src/note-contract.js` at line 15, The
frontmatter regex assigned to const match currently only matches LF newlines;
update the pattern used in note-contract.js (the /^---\n([\s\S]*?)\n---\n?/
regex) to accept CRLF by replacing each \n with \r?\n (e.g., use
/^---\r?\n([\s\S]*?)\r?\n---\r?\n?/), so frontmatter parsing becomes CRLF-safe
while preserving existing behavior for LF.
This was referenced May 13, 2026
4 tasks
This was referenced Jul 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
Release Notes
New Features
@jarvos/agent-contextmodule enabling agent clients to access jarvOS capabilities including current work tracking, recall, note creation, and startup briefing through MCP tools.Documentation