feat: AWS Bedrock provider (LLM + embeddings) with SSO + auth-refresh#741
Open
acpiper wants to merge 12 commits into
Open
feat: AWS Bedrock provider (LLM + embeddings) with SSO + auth-refresh#741acpiper wants to merge 12 commits into
acpiper wants to merge 12 commits into
Conversation
… SSO Add a `bedrock` provider so agentmemory can use Anthropic models hosted on AWS Bedrock for compression/summarization/image description. Bedrock uses SigV4 signing rather than an x-api-key header, so the stock Anthropic SDK with a base-URL override cannot work; this wraps @anthropic-ai/bedrock-sdk. - Credentials resolve via the AWS default provider chain (env / IAM role / SSO cache selected by AWS_PROFILE); no static keys required. Explicit static keys are used only when both AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY are set (CI escape hatch). - Detection is gated strictly on AWS_BEDROCK=true and placed first, so it never fires for existing OpenAI/Ollama users. - Default model is Claude Haiku 4.5 (bare on-demand ID); docs cover the us./eu.-prefixed cross-region inference-profile forms. - Opaque Bedrock 4xx errors are wrapped with an actionable hint (model access / region / inference profile). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When a Bedrock call fails with an expired-token error, run a user-configured command (e.g. `aws sso login --profile foo`) and retry once. Equivalent in spirit to Claude Code's awsAuthRefresh setting. - isAuthExpiry classifies expiry errors via a narrow allow-list so genuine failures (validation, throttling, access denials) never trigger a refresh. - The retry runs inside ResilientProvider.call BEFORE recordFailure(), so a recoverable expiry doesn't count toward opening the circuit breaker. - AuthRefresh runs the command with no shell (execFile on tokenized argv), single-flighted, cooldown-limited, and timeout-bounded. Only the literal configured string is executed — no untrusted data is interpolated. - Wired for the bedrock provider only, gated on AWS_AUTH_REFRESH; the mechanism itself is generic. Configurable timeout via AWS_AUTH_REFRESH_TIMEOUT_MS. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The provider.summarize() call in mem::compress-file was the one provider
invocation in the function not wrapped in try/catch. On a provider throw the
error escaped the handler, and the iii engine serialized the Error object as
the opaque `{"error":"[object Object]"}` with an error_id — hiding actionable
messages such as the Bedrock provider's model-access / cross-region
inference-profile guidance.
Wrap the call and return a structured { success: false, error: <message> },
matching the convention already used in compress.ts and summarize.ts. The
original file is left untouched and no backup is written on failure.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a `bedrock` embedding provider so memory vectors can be generated by Cohere / Amazon Titan embedding models on AWS Bedrock, alongside the existing Bedrock LLM provider. - Uses the AWS Bedrock Runtime InvokeModel API (@aws-sdk/client-bedrock-runtime, now a direct dependency) — the Anthropic bedrock-sdk has no embeddings. - Credentials resolve via the AWS provider chain (env / IAM role / SSO cache via AWS_PROFILE), reusing AWS_REGION; no key env var. Static keys honored only when both AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY are set. - Config mirrors the OPENAI_ embedding knobs for parity: AWS_BEDROCK_EMBEDDING_MODEL (default cohere.embed-v4:0) and AWS_BEDROCK_EMBEDDING_DIMENSIONS (default 1024). No API-key or base-URL knob — creds come from the AWS chain and the endpoint is region-derived. - Selected ONLY via explicit EMBEDDING_PROVIDER=bedrock; AWS_BEDROCK=true does not auto-switch embeddings, preserving Bedrock-LLM + local-embeddings setups. - Table-driven model families (cohere. vs amazon.titan-embed): Cohere native batch (v4 keyed-by-type float response; v3 bare-array), Titan one-call-per-text fan-out with bounded concurrency. Unknown model without a dimensions override throws rather than risk silent index corruption. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@acpiper is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds opt-in AWS Bedrock LLM and embedding providers, credential-refresh tooling, resilient retry wiring, embedding batching for Cohere/Titan, error mapping, docs, and tests. ChangesAWS Bedrock Provider Integration
Sequence Diagram(s)sequenceDiagram
participant Client
participant ResilientProvider
participant AuthRefresh
participant BedrockProvider
participant BedrockRuntime
Client->>ResilientProvider: call compress/summarize/embed
ResilientProvider->>BedrockProvider: invoke operation
BedrockProvider->>BedrockRuntime: messages.create / InvokeModel
alt Runtime returns auth expiry error
BedrockRuntime-->>BedrockProvider: Error (expired token)
BedrockProvider-->>ResilientProvider: throw auth-expiry
ResilientProvider->>AuthRefresh: run()
AuthRefresh->>AuthRefresh: spawn configured refresh command
AuthRefresh-->>ResilientProvider: success/failure
alt refresh success
ResilientProvider->>BedrockProvider: retry operation
BedrockProvider->>BedrockRuntime: messages.create / InvokeModel
BedrockRuntime-->>BedrockProvider: Response
BedrockProvider-->>ResilientProvider: Result
else refresh failed
ResilientProvider-->>Client: rethrow original error
end
else Runtime returns success
BedrockRuntime-->>BedrockProvider: Response
BedrockProvider-->>ResilientProvider: Result
end
ResilientProvider-->>Client: final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/config.ts (1)
213-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep LLM-capability detection in sync with Bedrock validation.
This now reports
"llm"for anyAWS_BEDROCK=true, even whenAWS_REGIONis missing and Bedrock cannot actually be constructed. Reuse the same completeness check asdetectProvider()so LLM-only flows are not enabled against an unusable config.🤖 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/config.ts` around lines 213 - 220, The LLM-capability check is currently treating AWS_BEDROCK="true" as sufficient even when required Bedrock config (like AWS_REGION) is missing; update the boolean expression used to detect LLM capability to reuse the same completeness/provider validation as detectProvider() instead of just env["AWS_BEDROCK"] === "true". Locate the LLM detection expression in src/config.ts and replace the raw AWS_BEDROCK string check with the same/provider completeness check used by detectProvider() (or call a helper used by detectProvider() such as isBedrockConfigured()/detectProvider() and confirm it reports Bedrock usable) so that Bedrock only enables LLM flows when its full configuration is present. Ensure other provider checks (ANTHROPIC_API_KEY, GEMINI_API_KEY, etc.) remain unchanged.
🧹 Nitpick comments (9)
README.md (1)
1165-1186: 💤 Low valueConsider reordering SSO and auth-refresh bullets for clarity.
Line 1177 states "Run
aws sso login --profile <name>first, and again when the session expires," which may suggest manual re-login is always required. The auth-refresh automation is introduced in the next bullet (line 1178), but presenting it as an afterthought may cause users to miss the automation option. Reordering or cross-referencing would improve clarity.Suggested reordering
Move the auth-refresh bullet immediately after the SSO bullet and add a forward reference:
-- **SSO** works out of the box, but agentmemory only *reads* the cached token — it cannot perform the login. Run `aws sso login --profile <name>` first, and again when the session expires. +- **SSO** works out of the box, but agentmemory only *reads* the cached token — it cannot perform the login. Run `aws sso login --profile <name>` first. When the session expires, either re-run it manually or configure the auth-refresh hook (below) to automate re-authentication. - **Auth-refresh hook** (optional): when a Bedrock call fails with an expired-token error, agentmemory can run a command of your choosing and retry once:🤖 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 `@README.md` around lines 1165 - 1186, Reorder the two bullets so the "Auth-refresh hook" bullet appears immediately after the "SSO" bullet and update the "SSO" text to reference the auth-refresh option (mention AWS_AUTH_REFRESH and AWS_AUTH_REFRESH_TIMEOUT_MS) to avoid implying manual re-login is always required; specifically, adjust the "SSO" paragraph to say you can run aws sso login manually or configure the auth-refresh hook to refresh automatically, and ensure the "Auth-refresh hook" bullet includes the single-flight, cooldown and execFile security notes unchanged..env.example (1)
47-71: 💤 Low valueConsider reordering auth-refresh documentation for clarity.
The initial NOTE (lines 50-52) states "run
aws sso login --profile <name>first, and re-run it when the session expires," which may lead users to believe manual re-login is always required. The auth-refresh automation (lines 63-70) is introduced later as an optional feature. Reordering or cross-referencing the hook in the initial NOTE would set clearer expectations upfront.Suggested reordering
# AWS Bedrock (Anthropic models on Bedrock). Opt in with AWS_BEDROCK=true; takes # precedence over the keys above when set. Credentials come from the standard AWS # provider chain — environment creds, IAM roles, or an SSO profile cached under # ~/.aws/sso/cache/ (select with AWS_PROFILE). NOTE: agentmemory reads the cached -# SSO token but cannot perform the login — run `aws sso login --profile <name>` -# first, and re-run it when the session expires. +# SSO token but cannot perform the login — run `aws sso login --profile <name>` +# first. When the session expires, either re-run it manually or configure the +# AWS_AUTH_REFRESH hook below to automate re-authentication.🤖 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 @.env.example around lines 47 - 71, The NOTE about running `aws sso login` can be misleading because the optional automated refresh hook (AWS_AUTH_REFRESH and AWS_AUTH_REFRESH_TIMEOUT_MS) is documented later; update the .env.example so the NOTE either (a) cross-references the auth-refresh option by name (mention AWS_AUTH_REFRESH and AWS_AUTH_REFRESH_TIMEOUT_MS and that it can re-establish SSO sessions automatically) or (b) move the AWS_AUTH_REFRESH block directly above/beside the NOTE; ensure the NOTE clearly states manual login is only required if no auth-refresh is configured and keep the existing auth-refresh usage and example intact (AWS_AUTH_REFRESH=... and AWS_AUTH_REFRESH_TIMEOUT_MS=...).test/bedrock-provider.test.ts (1)
41-61: ⚡ Quick winAvoid asserting on
AnthropicBedrockinternals in the test.These checks depend on SDK instance fields like
awsAccessKeyandawsRegion, which are outsideBedrockProvider's contract. Mock the constructor and assert the config passed intonew AnthropicBedrock(...)instead, so a bedrock-sdk implementation change does not create false failures here.🤖 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 `@test/bedrock-provider.test.ts` around lines 41 - 61, The tests directly inspect AnthropicBedrock internals (client.awsAccessKey, client.awsRegion) which ties them to SDK internals; instead, mock the AnthropicBedrock constructor and assert what config was passed to it in the BedrockProvider tests ("constructs with explicit static keys when present", "ignores a lone access key...", "threads the region through to the client"). Replace the current extraction of provider.client with a spy/mock on AnthropicBedrock (e.g., jest.mock or jest.spyOn the constructor) and verify the constructor was called with the expected config object reflecting AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY presence or absence and the awsRegion value, leaving BedrockProvider constructor and behavior unchanged.src/config.ts (1)
55-59: ⚡ Quick winDrop the explanatory detection comments.
This block is describing behavior rather than relying on naming/structure, which conflicts with the repo standard for
src/code.As per coding guidelines,
src/**/*.{ts,js}: No code comments explaining WHAT — use clear naming instead.🤖 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/config.ts` around lines 55 - 59, Remove the multi-line explanatory comment block in src/config.ts that describes AWS_BEDROCK, Bedrock vs Ollama/OpenAI detection; instead rely on clear naming of the config/flag (e.g., AWS_BEDROCK, bedrockRegion, enableBedrock) and, if a comment is necessary, replace it with a single concise line stating intent (e.g., "AWS Bedrock opt-in flag; credentials use AWS provider chain") without describing detection behavior or rationale.src/providers/embedding/bedrock.ts (1)
79-97: ⚡ Quick winTrim the inline provider/env documentation.
This is WHAT-level documentation inside
src/rather than self-describing code, which goes against the repo rule.As per coding guidelines,
src/**/*.{ts,js}: No code comments explaining WHAT — use clear naming instead.🤖 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/providers/embedding/bedrock.ts` around lines 79 - 97, Remove the long WHAT-level block comment at the top of the Bedrock embedding provider and replace it with a one-line purpose summary (e.g., "Bedrock embedding provider for Cohere/Amazon Titan via AWS Bedrock Runtime") while moving the detailed env/usage notes (AWS_REGION, AWS_BEDROCK_EMBEDDING_MODEL, AWS_BEDROCK_EMBEDDING_DIMENSIONS, AWS_PROFILE/AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY/AWS_SESSION_TOKEN) into external docs/README; keep only a short, self-describing comment in the module that names the provider and any non-obvious behavior, and ensure code and environment variables remain clearly named so no WHAT-level explanations are in src.src/providers/bedrock.ts (1)
5-34: ⚡ Quick winMove this WHAT-level provider documentation out of the implementation.
The class header is mostly restating behavior and environment wiring inline. In this repo, that should live in naming or external docs rather than
src/comments.As per coding guidelines,
src/**/*.{ts,js}: No code comments explaining WHAT — use clear naming instead.🤖 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/providers/bedrock.ts` around lines 5 - 34, Remove the large WHAT-level block comment at the top of src/providers/bedrock.ts and replace it with a one-line functional summary (e.g., "Bedrock LLM provider for Anthropic models via AWS Bedrock") plus an optional pointer to external docs; move the detailed environment, credential, and model-ID guidance into the repo's naming/docs area (README or docs/), leaving only minimal implementation notes in the file header so the provider implementation remains self-explanatory without verbose "what" documentation in source.src/providers/index.ts (1)
30-33: ⚡ Quick winDrop the explanatory comments here and rely on naming.
These comments describe what the code is doing rather than documenting non-obvious rationale, which is out of step with the repo rule for
src/**/*.ts. As per coding guidelines,src/**/*.{ts,js}: No code comments explaining WHAT — use clear naming instead.Also applies to: 102-104
🤖 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/providers/index.ts` around lines 30 - 33, Remove the explanatory "Build the optional credential-refresh hook..." comment block (and the similar comment at lines ~102-104) in src/providers/index.ts; instead rely on clear identifiers (e.g., the exported builder function or constant that constructs the credential-refresh hook) to convey intent, leaving only comments for non-obvious rationale if any. Locate the comment blocks around the credential-refresh hook build logic (the code that wires the bedrock provider and checks AWS_AUTH_REFRESH) and delete those "what the code is doing" comments so names and function signatures carry the meaning.src/functions/compress-file.ts (1)
144-147: ⚡ Quick winPlease remove this WHAT-comment.
The surrounding code is already clear enough; keeping this inline explanation violates the repo rule for
src/**/*.ts. As per coding guidelines,src/**/*.{ts,js}: No code comments explaining WHAT — use clear naming instead.🤖 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/functions/compress-file.ts` around lines 144 - 147, Remove the WHAT-style inline comment that starts with "// Surface the provider's message as a structured error..." in src/functions/compress-file.ts; locate that comment block (the three-line explanatory comment shown in the diff) and delete it so the code follows the repo rule disallowing WHAT-comments in src/**/*.ts while leaving surrounding logic and any necessary TODO/WHY comments intact.src/providers/auth-refresh.ts (1)
3-8: ⚡ Quick winRemove the WHAT-comments and let the names/helpers carry the behavior.
These docblocks and inline comments mostly restate the implementation, which conflicts with the repo rule for
src/**/*.ts. Please trim them down or move any non-obvious rationale into tests or external docs. As per coding guidelines,src/**/*.{ts,js}: No code comments explaining WHAT — use clear naming instead.Also applies to: 15-29, 33-38, 58-69
🤖 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/providers/auth-refresh.ts` around lines 3 - 8, Remove the “WHAT” docblocks and inline comments in src/providers/auth-refresh.ts (the top-of-file classifier comment and the comment blocks around lines 15–29, 33–38, and 58–69) and instead rely on clear naming for the classifier/allow-list helpers (e.g., the error classifier function and allowList constants) — keep at most a one-line JSDoc that states purpose, drop verbose restatements of the implementation, and move any non-obvious rationale into tests or external docs so the code comments no longer repeat what functions/variables already convey.
🤖 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 `@src/config.ts`:
- Around line 52-72: detectProvider currently returns a bedrock ProviderConfig
even when AWS_BEDROCK=true but AWS_REGION is missing; change detectProvider so
that if env["AWS_BEDROCK"] === "true" and !hasRealValue(env["AWS_REGION"]) you
reject the config immediately (e.g., throw a descriptive Error or return a
failure sentinel) instead of only writing to stderr — update the AWS_BEDROCK
handling block in detectProvider (and the error text) so the function does not
return a ProviderConfig with provider: "bedrock" when AWS_REGION is unset.
In `@src/providers/embedding/bedrock.ts`:
- Around line 164-171: The code currently builds Float32Array rows from the
Bedrock response but doesn't verify the number of returned embeddings against
the input batch, so add an explicit cardinality check: after computing rows
(from json.embeddings) and before the loop that pushes to out, verify
rows.length equals slice.length (the input batch slice passed into this
embedding routine) and throw a clear error if they differ; keep the existing
withDimensionGuard usage for per-vector length checks but fail fast here on
batch size mismatch to avoid silent misalignment.
In `@src/providers/index.ts`:
- Around line 34-44: createAuthRefresh currently checks only config.provider and
skips creating an AuthRefresh when bedrock appears only in a fallback chain;
update the logic so it inspects the full provider set used to build the chain
(not just the primary provider). Specifically, change createAuthRefresh to
accept or compute the effective providers list (e.g., accept ProviderConfig |
FallbackConfig or an array of provider names), detect if "bedrock" appears
anywhere in that set (including fallbackConfig.providers used by
createFallbackProvider), and return the AuthRefresh when any provider is
"bedrock"; apply the same change to the other spot mentioned (the similar logic
around the other block that handles fallback providers). Ensure you reference
and update createAuthRefresh and createFallbackProvider usage so the full
provider list is available to createAuthRefresh.
In `@src/providers/resilient.ts`:
- Around line 31-38: The catch currently swallows errors from the retry call as
well as authRefresh.run(), so change the flow in the resilient call logic
(symbols: alreadyRetried, authRefresh, isAuthExpiry, authRefresh.run(),
this.call(fn, true)) to only catch failures from authRefresh.run(): call await
this.authRefresh.run() inside a try/catch and if that fails handle/log and fall
through, but perform return await this.call(fn, true) outside that catch so any
error from the retried call propagates normally and is not swallowed or
double-recorded by the breaker.
In `@src/types.ts`:
- Line 150: VALID_PROVIDERS in src/config.ts doesn't include "bedrock" so
loadFallbackConfig() filters out FALLBACK_PROVIDERS=bedrock even though
ProviderType now lists "bedrock"; update the VALID_PROVIDERS array (or its
source of truth used by loadFallbackConfig) to include the "bedrock" string so
that loadFallbackConfig() will accept and return Bedrock when parsing
FALLBACK_PROVIDERS, and ensure any related validation logic referencing
VALID_PROVIDERS or loadFallbackConfig() is updated to recognize "bedrock".
In `@test/auth-refresh.test.ts`:
- Around line 143-164: The test for single-flight is flawed because it spies on
AuthRefresh.run (the public wrapper) instead of the underlying execFile; update
the tests to mock node:child_process.execFile and assert its invocation count:
for the concurrent case create an AuthRefresh({ command: "true" }) and call
refresh.run() three times concurrently then expect execFile to have been called
exactly once; for the cooldown test mock execFile to succeed once then on the
immediate second call assert execFile was not invoked and refresh.run() rejects
with /cooldown/; ensure mocks are restored between tests.
---
Outside diff comments:
In `@src/config.ts`:
- Around line 213-220: The LLM-capability check is currently treating
AWS_BEDROCK="true" as sufficient even when required Bedrock config (like
AWS_REGION) is missing; update the boolean expression used to detect LLM
capability to reuse the same completeness/provider validation as
detectProvider() instead of just env["AWS_BEDROCK"] === "true". Locate the LLM
detection expression in src/config.ts and replace the raw AWS_BEDROCK string
check with the same/provider completeness check used by detectProvider() (or
call a helper used by detectProvider() such as
isBedrockConfigured()/detectProvider() and confirm it reports Bedrock usable) so
that Bedrock only enables LLM flows when its full configuration is present.
Ensure other provider checks (ANTHROPIC_API_KEY, GEMINI_API_KEY, etc.) remain
unchanged.
---
Nitpick comments:
In @.env.example:
- Around line 47-71: The NOTE about running `aws sso login` can be misleading
because the optional automated refresh hook (AWS_AUTH_REFRESH and
AWS_AUTH_REFRESH_TIMEOUT_MS) is documented later; update the .env.example so the
NOTE either (a) cross-references the auth-refresh option by name (mention
AWS_AUTH_REFRESH and AWS_AUTH_REFRESH_TIMEOUT_MS and that it can re-establish
SSO sessions automatically) or (b) move the AWS_AUTH_REFRESH block directly
above/beside the NOTE; ensure the NOTE clearly states manual login is only
required if no auth-refresh is configured and keep the existing auth-refresh
usage and example intact (AWS_AUTH_REFRESH=... and
AWS_AUTH_REFRESH_TIMEOUT_MS=...).
In `@README.md`:
- Around line 1165-1186: Reorder the two bullets so the "Auth-refresh hook"
bullet appears immediately after the "SSO" bullet and update the "SSO" text to
reference the auth-refresh option (mention AWS_AUTH_REFRESH and
AWS_AUTH_REFRESH_TIMEOUT_MS) to avoid implying manual re-login is always
required; specifically, adjust the "SSO" paragraph to say you can run aws sso
login manually or configure the auth-refresh hook to refresh automatically, and
ensure the "Auth-refresh hook" bullet includes the single-flight, cooldown and
execFile security notes unchanged.
In `@src/config.ts`:
- Around line 55-59: Remove the multi-line explanatory comment block in
src/config.ts that describes AWS_BEDROCK, Bedrock vs Ollama/OpenAI detection;
instead rely on clear naming of the config/flag (e.g., AWS_BEDROCK,
bedrockRegion, enableBedrock) and, if a comment is necessary, replace it with a
single concise line stating intent (e.g., "AWS Bedrock opt-in flag; credentials
use AWS provider chain") without describing detection behavior or rationale.
In `@src/functions/compress-file.ts`:
- Around line 144-147: Remove the WHAT-style inline comment that starts with "//
Surface the provider's message as a structured error..." in
src/functions/compress-file.ts; locate that comment block (the three-line
explanatory comment shown in the diff) and delete it so the code follows the
repo rule disallowing WHAT-comments in src/**/*.ts while leaving surrounding
logic and any necessary TODO/WHY comments intact.
In `@src/providers/auth-refresh.ts`:
- Around line 3-8: Remove the “WHAT” docblocks and inline comments in
src/providers/auth-refresh.ts (the top-of-file classifier comment and the
comment blocks around lines 15–29, 33–38, and 58–69) and instead rely on clear
naming for the classifier/allow-list helpers (e.g., the error classifier
function and allowList constants) — keep at most a one-line JSDoc that states
purpose, drop verbose restatements of the implementation, and move any
non-obvious rationale into tests or external docs so the code comments no longer
repeat what functions/variables already convey.
In `@src/providers/bedrock.ts`:
- Around line 5-34: Remove the large WHAT-level block comment at the top of
src/providers/bedrock.ts and replace it with a one-line functional summary
(e.g., "Bedrock LLM provider for Anthropic models via AWS Bedrock") plus an
optional pointer to external docs; move the detailed environment, credential,
and model-ID guidance into the repo's naming/docs area (README or docs/),
leaving only minimal implementation notes in the file header so the provider
implementation remains self-explanatory without verbose "what" documentation in
source.
In `@src/providers/embedding/bedrock.ts`:
- Around line 79-97: Remove the long WHAT-level block comment at the top of the
Bedrock embedding provider and replace it with a one-line purpose summary (e.g.,
"Bedrock embedding provider for Cohere/Amazon Titan via AWS Bedrock Runtime")
while moving the detailed env/usage notes (AWS_REGION,
AWS_BEDROCK_EMBEDDING_MODEL, AWS_BEDROCK_EMBEDDING_DIMENSIONS,
AWS_PROFILE/AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY/AWS_SESSION_TOKEN) into
external docs/README; keep only a short, self-describing comment in the module
that names the provider and any non-obvious behavior, and ensure code and
environment variables remain clearly named so no WHAT-level explanations are in
src.
In `@src/providers/index.ts`:
- Around line 30-33: Remove the explanatory "Build the optional
credential-refresh hook..." comment block (and the similar comment at lines
~102-104) in src/providers/index.ts; instead rely on clear identifiers (e.g.,
the exported builder function or constant that constructs the credential-refresh
hook) to convey intent, leaving only comments for non-obvious rationale if any.
Locate the comment blocks around the credential-refresh hook build logic (the
code that wires the bedrock provider and checks AWS_AUTH_REFRESH) and delete
those "what the code is doing" comments so names and function signatures carry
the meaning.
In `@test/bedrock-provider.test.ts`:
- Around line 41-61: The tests directly inspect AnthropicBedrock internals
(client.awsAccessKey, client.awsRegion) which ties them to SDK internals;
instead, mock the AnthropicBedrock constructor and assert what config was passed
to it in the BedrockProvider tests ("constructs with explicit static keys when
present", "ignores a lone access key...", "threads the region through to the
client"). Replace the current extraction of provider.client with a spy/mock on
AnthropicBedrock (e.g., jest.mock or jest.spyOn the constructor) and verify the
constructor was called with the expected config object reflecting
AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY presence or absence and the awsRegion
value, leaving BedrockProvider constructor and behavior unchanged.
🪄 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: c5a32342-22e2-4e3c-aa21-605c92640cc5
📒 Files selected for processing (16)
.env.exampleREADME.mdpackage.jsonsrc/config.tssrc/functions/compress-file.tssrc/providers/auth-refresh.tssrc/providers/bedrock.tssrc/providers/embedding/bedrock.tssrc/providers/embedding/index.tssrc/providers/index.tssrc/providers/resilient.tssrc/types.tstest/auth-refresh.test.tstest/bedrock-embedding-provider.test.tstest/bedrock-provider.test.tstest/compress-file.test.ts
…meout The refresh path was entirely silent: a swallowed catch meant there was no way to tell whether the hook fired, was skipped by cooldown, failed, or timed out — the original error just propagated (e.g. background consolidation showing a bare "Token is expired" with no sign the refresh ran). - Log every refresh attempt and outcome: running, succeeded, command-failed, timed-out, cooldown-skipped, and whether the post-refresh retry recovered the call (in resilient.ts). - After a timeout, suppress re-runs for postTimeoutCooldownMs (default 15m). A timeout means an interactive login (browser device-auth) was likely left open awaiting approval; relaunching on every background trigger would fill the browser with stale login pages. Timeout is detected via execFile's killed/SIGTERM signal. Ordinary non-zero exits keep the short cooldown. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address PR review: - detectProvider no longer returns a bedrock config when AWS_REGION is unset; it falls through to the next provider instead of deferring an unconstructable config to runtime. New isBedrockUsable() helper is shared with detectLlmProviderKind so capability detection can't report "llm" for a Bedrock config that cannot be built. - Add "bedrock" to VALID_PROVIDERS so FALLBACK_PROVIDERS=bedrock is honored instead of silently dropped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… chains Address PR review: - resilient.ts: scope the inner catch to authRefresh.run() only. Previously it also trapped the post-refresh retry, so a failed retry surfaced the stale auth-expiry error and double-counted the circuit breaker. The retry now runs outside the try and propagates (and accounts for the breaker) normally. - index.ts: derive the auth-refresh hook from every provider built (primary + fallback chain), not just config.provider, so a bedrock provider reachable only via FALLBACK_PROVIDERS still refreshes expired credentials. - test: mock node:child_process and assert the real execFile call count for the single-flight, cooldown, and post-timeout-suppression cases (the old spy on run() only counted wrapper calls and depended on a host `true` binary). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address PR review: if the Cohere response returns fewer rows than the input batch, the texts↔vectors mapping silently misaligns downstream (withDimensionGuard only checks each vector's length, not batch cardinality). Throw on a row-count mismatch instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
AWS_BEDROCK was gated on an exact `=== "true"` match, so a natural `AWS_BEDROCK=True`/`TRUE` silently skipped the Bedrock branch and fell through to the no-op provider with a misleading "No LLM provider key found" warning. Normalize the flag via a case-insensitive isEnvTrue helper (trims whitespace too) shared by the detectProvider gate and isBedrockUsable, so True/TRUE/" true " all opt in while non-boolean values like "1" are still rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/config.ts (1)
52-68: ⚡ Quick winReduce comments to focus on WHY, not WHAT.
Both function comments explain what the functions do, which the clear naming already conveys. Per coding guidelines, comments should explain why the abstraction exists rather than what it does.
Suggested comment refinement
/** - * Case-insensitive boolean-env check. Accepts "true" in any case with - * surrounding whitespace (e.g. "True", "TRUE", " true ") so a natural - * capitalization of AWS_BEDROCK=True doesn't silently disable Bedrock. + * Prevents case-sensitivity footguns where AWS_BEDROCK=True or TRUE + * would silently disable Bedrock. */ function isEnvTrue(v: string | undefined): boolean { return typeof v === "string" && v.trim().toLowerCase() === "true"; } /** - * Bedrock is opted in when AWS_BEDROCK is truthy. The AWS_BEDROCK === "true" - * gate in detectProvider and the usability check below share this so both - * accept the same set of values. + * Ensures detectProvider and isBedrockUsable apply the same opt-in logic. */ function isBedrockOptIn(env: Record<string, string>): boolean { return isEnvTrue(env["AWS_BEDROCK"]); }As per coding guidelines, "
src/**/*.{ts,js}: No code comments explaining WHAT — use clear naming instead".🤖 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/config.ts` around lines 52 - 68, Remove or replace the existing "what" comments for isEnvTrue and isBedrockOptIn with brief "why" comments: delete the explanatory lines that restate the function names' behavior and instead add a short note explaining the rationale for the abstractions (e.g., why case-insensitive truth parsing is needed and why a centralized AWS_BEDROCK opt-in check exists for consistent provider gating) so the code documents intent rather than duplicating the implementation; reference the functions isEnvTrue and isBedrockOptIn when updating the comments.
🤖 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/config.ts`:
- Around line 52-68: Remove or replace the existing "what" comments for
isEnvTrue and isBedrockOptIn with brief "why" comments: delete the explanatory
lines that restate the function names' behavior and instead add a short note
explaining the rationale for the abstractions (e.g., why case-insensitive truth
parsing is needed and why a centralized AWS_BEDROCK opt-in check exists for
consistent provider gating) so the code documents intent rather than duplicating
the implementation; reference the functions isEnvTrue and isBedrockOptIn when
updating the comments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ff90447-7df8-49c0-94cd-75e6f2a58fd6
📒 Files selected for processing (2)
src/config.tstest/bedrock-provider.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/bedrock-provider.test.ts
Per repo guideline (no WHAT-comments in src/), reduce the docblocks on isEnvTrue/isBedrockOptIn/isBedrockUsable to one-line WHY notes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
Adds AWS Bedrock as both an LLM provider (compression / summarization / image description) and an embedding provider, as an alternative to the OpenAI-compatible (Ollama) and Anthropic-API providers.
Bedrock authenticates with AWS SigV4, so the stock Anthropic SDK with an
ANTHROPIC_BASE_URLoverride cannot work — the LLM path wraps@anthropic-ai/bedrock-sdk, and embeddings use the AWS Bedrock RuntimeInvokeModelAPI.Commits
AWS_PROFILE); no static keys required. Opt-in viaAWS_BEDROCK=true(gated, placed first in detection so it never fires for existing OpenAI/Ollama users). Default model Claude Haiku 4.5. Opaque Bedrock 4xx wrapped with actionable hints (model access / region /us./eu.inference profile).aws sso login --profile foo) and retry once. Single-flight, cooldown- and timeout-bounded, no shell (execFileon tokenized argv), only the literal configured string is executed. Gated to bedrock viaAWS_AUTH_REFRESH.[object Object]—mem::compress-filewas the one provider call not wrapped in try/catch; a throw escaped and the engine serialized it opaquely. Now returns a structured{success:false, error:<message>}, matchingcompress.ts/summarize.ts.@aws-sdk/client-bedrock-runtime. Selected explicitly viaEMBEDDING_PROVIDER=bedrock(not auto fromAWS_BEDROCK, preserving Bedrock-LLM + local-embeddings setups). Defaultcohere.embed-v4:0@ 1024 dims. Table-driven model families; geo-prefixed inference-profile IDs (us./eu./global.) supported.Config
Some models aren't available on-demand in every Region (e.g.
cohere.embed-v4:0is inference-profile-only in several Regions) — set the geo-prefixed ID there, e.g.us.cohere.embed-v4:0.Testing
aws sso login→ retry), and embedding round-trip (store + vector search) all confirmed end-to-end.Notes for reviewers
npm install --legacy-peer-deps— a pre-existing peer conflict (repo pins typescript ^6,tsdownpeers want ^5), not introduced here.AWS_BEDROCK_EMBEDDING_DIMENSIONSlater requires re-embedding stored memories.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests