fix(cli): authenticate apiFetch when AGENTMEMORY_SECRET is set#380
fix(cli): authenticate apiFetch when AGENTMEMORY_SECRET is set#380eleboucher wants to merge 1 commit into
Conversation
|
@eleboucher is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR adds conditional Authorization header support to the ChangesAPI Authentication Header Support
🎯 1 (Trivial) | ⏱️ ~3 minutes
🚥 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 unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
eleboucher
force-pushed
the
fix/cli-auth-headers
branch
from
99c4a72 to
ceb80e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.ts (1)
479-486:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCRITICAL: PR description does not match actual code changes.
The PR description states: "The doctor and status CLI commands call protected endpoints (config/flags, health) using apiFetch but do not send the bearer authorization header. When deployments set AGENTMEMORY_SECRET, the server returns 401 responses."
However, the actual code changes in this PR do NOT fix this issue. The
apiFetchfunction at lines 479-486 still lacks Authorization header support, yet the PR introduces onlyAGENTMEMORY_VIEWER_HOSTandAGENTMEMORY_III_CONFIGfeatures. The AI-generated summary also describes config/host binding changes, not auth header fixes.Action required: Either update the PR description to match the actual changes (environment variable support for viewer host and iii-config override), or implement the missing auth header fix described in the current PR description.
If the auth header fix is still needed,
apiFetchshould readAGENTMEMORY_SECRETand send it as a Bearer token:🔒 Proposed auth header fix (if intended)
async function apiFetch<T = unknown>(base: string, path: string, timeoutMs = 5000): Promise<T | null> { try { - const res = await fetch(`${base}/agentmemory/${path}`, { signal: AbortSignal.timeout(timeoutMs) }); + const headers: Record<string, string> = {}; + const secret = process.env["AGENTMEMORY_SECRET"]; + if (secret) { + headers["Authorization"] = `Bearer ${secret}`; + } + const res = await fetch(`${base}/agentmemory/${path}`, { + headers, + signal: AbortSignal.timeout(timeoutMs) + }); return (await res.json()) as T; } catch { return null;🤖 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 `@src/cli.ts` around lines 479 - 486, The apiFetch function still omits the Authorization header; modify apiFetch to read process.env.AGENTMEMORY_SECRET and, if present, include an Authorization: Bearer <secret> header in the fetch options (preserving the AbortSignal.timeout usage and JSON response handling) so protected endpoints (config/flags, health) receive the token; update the fetch call within async function apiFetch to build a headers object and attach it to the request when AGENTMEMORY_SECRET is set.
🧹 Nitpick comments (1)
src/cli.ts (1)
176-186: ⚡ Quick winAbsolute path handling in config override is ambiguous.
When
AGENTMEMORY_III_CONFIGis set to an absolute path that doesn't exist, Line 178's early return is skipped, and Line 180 treats the full absolute path as a filename to search in__dirname, its parent, andprocess.cwd(). On Unix,join(__dirname, "/absolute/path")returns"/absolute/path", so this might accidentally work, but the behavior is subtle and platform-dependent.♻️ Clearer override handling
Distinguish between absolute and relative paths to make the fallback behavior explicit:
function findIiiConfig(): string { const override = process.env["AGENTMEMORY_III_CONFIG"]; - if (override && existsSync(override)) return override; + if (override) { + // Absolute path: check existence, don't fall back to search + if (path.isAbsolute(override)) { + return existsSync(override) ? override : ""; + } + // Relative/bare filename: check existence, then search + if (existsSync(override)) return override; + } const name = override || "iii-config.yaml"; for (const dir of [__dirname, join(__dirname, ".."), process.cwd()]) {Import
path.isAbsoluteat the top if not already imported.🤖 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 `@src/cli.ts` around lines 176 - 186, The override handling in findIiiConfig is ambiguous for absolute vs relative AGENTMEMORY_III_CONFIG values; update findIiiConfig to explicitly check path.isAbsolute(override) (importing isAbsolute from 'path' if needed) and if absolute: return it only when existsSync(override) (otherwise skip fallback), and if non-absolute: resolve it against process.cwd() (or treat as a filename to search in __dirname, join(__dirname,".."), and process.cwd()) before existsSync checks; ensure the function still falls back to searching for "iii-config.yaml" when no valid override is provided.
🤖 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.
Outside diff comments:
In `@src/cli.ts`:
- Around line 479-486: The apiFetch function still omits the Authorization
header; modify apiFetch to read process.env.AGENTMEMORY_SECRET and, if present,
include an Authorization: Bearer <secret> header in the fetch options
(preserving the AbortSignal.timeout usage and JSON response handling) so
protected endpoints (config/flags, health) receive the token; update the fetch
call within async function apiFetch to build a headers object and attach it to
the request when AGENTMEMORY_SECRET is set.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 176-186: The override handling in findIiiConfig is ambiguous for
absolute vs relative AGENTMEMORY_III_CONFIG values; update findIiiConfig to
explicitly check path.isAbsolute(override) (importing isAbsolute from 'path' if
needed) and if absolute: return it only when existsSync(override) (otherwise
skip fallback), and if non-absolute: resolve it against process.cwd() (or treat
as a filename to search in __dirname, join(__dirname,".."), and process.cwd())
before existsSync checks; ensure the function still falls back to searching for
"iii-config.yaml" when no valid override is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58d43960-a4c1-440d-b58e-1a0b171daf48
📒 Files selected for processing (3)
README.mdsrc/cli.tssrc/viewer/server.ts
eleboucher
changed the title
eleboucher
force-pushed
the
fix/cli-auth-headers
branch
from
ceb80e4 to
78d6d51
Compare
eleboucher
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli.ts (1)
476-478: 💤 Low valueConsider standardizing authorization header casing across the codebase.
The new code uses
"Authorization"(capitalized), which follows the standard HTTP header convention. However,runImportJsonl()at line 1202 uses"authorization"(lowercase) for the same purpose. While HTTP headers are case-insensitive and both work, consistent casing improves maintainability.♻️ Suggested consistency fix for runImportJsonl()
At line 1202, update the existing code to match the capitalization:
- if (secret) headers["authorization"] = `Bearer ${secret}`; + if (secret) headers["Authorization"] = `Bearer ${secret}`;🤖 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 `@src/cli.ts` around lines 476 - 478, The headers creation uses "Authorization" while runImportJsonl() uses "authorization" — standardize to the same casing; update the header key in runImportJsonl() from "authorization" to "Authorization" (the secret retrieval logic using process.env["AGENTMEMORY_SECRET"] and the headers: Record<string,string> pattern should remain unchanged) so both places use Authorization consistently.
🤖 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 `@src/cli.ts`:
- Around line 476-478: The headers creation uses "Authorization" while
runImportJsonl() uses "authorization" — standardize to the same casing; update
the header key in runImportJsonl() from "authorization" to "Authorization" (the
secret retrieval logic using process.env["AGENTMEMORY_SECRET"] and the headers:
Record<string,string> pattern should remain unchanged) so both places use
Authorization consistently.
1 participant
The doctor and status commands call protected endpoints (config/flags, health) via apiFetch but never send the bearer header. Deployments that set AGENTMEMORY_SECRET get 401 responses, parsed as flags=null, which the doctor renders as 'LLM provider: not set' and 'Embedding provider: not set' regardless of actual server-side config.
Summary by CodeRabbit