feat: expand env references in agent target headers#169
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe HTTP agent target now expands ChangesHeader expansion
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant AgentTarget
participant expandEnvInHeaders
participant getEnv
AgentTarget->>expandEnvInHeaders: config.headers
expandEnvInHeaders->>getEnv: resolve ${VAR} names
getEnv-->>expandEnvInHeaders: value or ""
expandEnvInHeaders-->>AgentTarget: expanded headers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/targets/agentTarget.ts (2)
55-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSilent empty-string fallback on missing env var can mask config errors.
When a referenced
${VAR}is undefined, the header value silently becomes an empty string instead of surfacing feedback. A misspelled var name or missing env var will produce a working-looking request with a blank header (e.g., emptyx-api-key), causing confusing downstream 401s that are hard to trace back to the config. This exact failure mode (silent empty value → confusing auth failure) is a common recurring issue in similar${VAR}expansion implementations across other tools.Consider logging a warning (or throwing, matching the strictness elsewhere in this file) that names the missing variable, so failures are actionable per the error-message guideline.
💡 Proposed fix to warn on unresolved variables
function expandEnvInHeaders(headers: Record<string, string>): Record<string, string> { const out: Record<string, string> = {}; for (const [k, v] of Object.entries(headers)) { - out[k] = v.replace(/\$\{([^}]+)\}/g, (_, name) => getEnv(name.trim()) ?? ""); + out[k] = v.replace(/\$\{([^}]+)\}/g, (_, name) => { + const trimmed = name.trim(); + const value = getEnv(trimmed); + if (value === undefined) { + console.warn(`[agentTarget] header "${k}" references undefined env var "${trimmed}"`); + } + return value ?? ""; + }); } return out; }As per coding guidelines, "Error messages must be actionable: tell the user what to fix, not just what went wrong."
🤖 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 `@core/src/targets/agentTarget.ts` around lines 55 - 63, The expandEnvInHeaders helper is silently replacing unresolved ${VAR} references with an empty string, which can hide missing or misspelled env vars. Update expandEnvInHeaders to either warn or throw when getEnv(name.trim()) returns undefined, and include the missing variable name in the message so the issue is actionable. Keep the behavior consistent with the strictness used elsewhere in agentTarget, and make sure the fix is applied in the header expansion path rather than at individual call sites.Source: Coding guidelines
55-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the
${VAR}header expansion helpercore/src/targets/agentTarget.tsandcore/src/targets/mcpTarget.tsboth implement the same regex/replace logic. Move it intocore/src/lib/env.tsso header/env expansion stays consistent in one place.🤖 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 `@core/src/targets/agentTarget.ts` around lines 55 - 63, The `${VAR}` header expansion logic is duplicated in the agent and MCP target code, so centralize it in the shared env utilities. Move the helper currently named expandEnvInHeaders into core/src/lib/env.ts, keep its behavior unchanged, and update the callers in AgentTarget and the MCP target to use the shared function so both targets rely on the same implementation.
🤖 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 `@core/src/targets/agentTarget.ts`:
- Around line 55-63: The expandEnvInHeaders helper is silently replacing
unresolved ${VAR} references with an empty string, which can hide missing or
misspelled env vars. Update expandEnvInHeaders to either warn or throw when
getEnv(name.trim()) returns undefined, and include the missing variable name in
the message so the issue is actionable. Keep the behavior consistent with the
strictness used elsewhere in agentTarget, and make sure the fix is applied in
the header expansion path rather than at individual call sites.
- Around line 55-63: The `${VAR}` header expansion logic is duplicated in the
agent and MCP target code, so centralize it in the shared env utilities. Move
the helper currently named expandEnvInHeaders into core/src/lib/env.ts, keep its
behavior unchanged, and update the callers in AgentTarget and the MCP target to
use the shared function so both targets rely on the same implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5af9a9f1-541a-4363-977e-38844a8ac52e
📒 Files selected for processing (2)
core/src/targets/agentTarget.tsdocs/cli.md
|
@CodeRabbit review |
✅ Action performedReview finished.
|
2 participants
Problem
target.headerson agent (HTTP) targets only accepted literal strings, so theonly way to send an auth header (e.g.
x-api-key) was to paste the raw secretinto the opfor config file. MCP targets already supported
${VAR}expansionfor
urlHeaders— agent targets were the odd one out, and there was no way tokeep secrets out of version-controlled config.
Solution
Added the same
${VAR}expansion agent targets already lacked but MCP targetshad, so header values can reference environment variables instead of hardcoding
secrets.
Changes
core/src/targets/agentTarget.ts: addedexpandEnvInHeaders()and applied itto
config.headersbefore merging into the outgoing request (mirrors theexisting MCP
urlHeaderspattern, via the sharedgetEnvprovider so it alsoworks for browser-extension targets).
docs/cli.md: documented${VAR}expansion support next to thetarget.headersfield description.Summary by CodeRabbit
${VAR}, allowing dynamic values to be inserted before requests are sent.