Add configurable env tail append option#76
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces static DEFAULT_CONFIG with a getDefaultConfig() that reads CODEX_APPEND_ENV_CONTEXT and adds an appendEnvContext flag to PluginConfig; propagates that flag through request transformation (transformRequestForCodex → transformRequestBody → transformInputForCodex) and updates tests to pass the flag explicitly. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Test / Caller
participant Fetch as fetch-helpers
participant Transformer as request-transformer
participant Config as config
Caller->>Fetch: transformRequestForCodex(body, pluginConfig?)
Fetch->>Config: (if needed) getDefaultConfig()
Fetch->>Fetch: resolve appendEnvContext = pluginConfig?.appendEnvContext ?? envFlag
Fetch->>Transformer: transformRequestBody(body, { appendEnvContext })
Transformer->>Transformer: transformInputForCodex(..., appendEnvContext)
alt appendEnvContext == true
Transformer->>Transformer: append env/files context to prompt tail
else
Transformer->>Transformer: omit env/files tail
end
Transformer-->>Fetch: transformed body
Fetch-->>Caller: final transformed request
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)test/cache-e2e.test.ts (1)
🔇 Additional comments (1)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
spec/append-env-context-config.mdis excluded by none and included by none
📒 Files selected for processing (7)
lib/config.ts(3 hunks)lib/request/fetch-helpers.ts(2 hunks)lib/request/request-transformer.ts(4 hunks)lib/types.ts(1 hunks)test/cache-e2e.test.ts(3 hunks)test/plugin-config.test.ts(7 hunks)test/request-transformer.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
test/request-transformer.test.ts (1)
lib/request/request-transformer.ts (1)
transformRequestBody(101-159)
test/plugin-config.test.ts (1)
lib/config.ts (1)
loadPluginConfig(32-74)
test/cache-e2e.test.ts (1)
lib/types.ts (1)
PluginConfig(6-30)
lib/request/fetch-helpers.ts (1)
lib/types.ts (1)
PluginConfig(6-30)
lib/config.ts (3)
lib/types.ts (1)
PluginConfig(6-30)lib/utils/file-system-utils.ts (1)
safeReadFile(56-62)lib/logger.ts (1)
logWarn(160-162)
🪛 GitHub Check: Lint & Typecheck
test/cache-e2e.test.ts
[warning] 1-1:
'beforeEach' is defined but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (11)
lib/types.ts (1)
20-24: LGTM! Clean addition of the configuration field.The
appendEnvContextfield is properly typed and documented. The JSDoc clearly communicates the default source and purpose.test/request-transformer.test.ts (1)
717-791: LGTM! Tests are now explicit and deterministic.The tests now explicitly control the
appendEnvContextflag rather than relying on environment variable side effects. This improves test clarity and reduces the risk of test pollution.test/cache-e2e.test.ts (1)
33-47: LGTM! Clean integration of pluginConfig.The
runTransformhelper now accepts apluginConfigparameter with a sensible default (appendEnvContext: false) that ensures deterministic test behavior. The config is correctly propagated totransformRequestForCodex.test/plugin-config.test.ts (2)
32-53: LGTM! Proper test isolation.The setup and teardown properly save and restore the
CODEX_APPEND_ENV_CONTEXTenvironment variable to prevent test pollution.
102-119: LGTM! Good coverage of appendEnvContext behavior.The new tests verify both:
- Environment-based default when config is missing
- Config override takes precedence over environment variable
This ensures the priority order is correct: explicit config > environment variable > default.
lib/request/fetch-helpers.ts (1)
120-167: LGTM! Correct priority order for appendEnvContext.The
appendEnvContextflag is sourced with the correct priority:pluginConfig?.appendEnvContext(explicit config) falls back toprocess.env.CODEX_APPEND_ENV_CONTEXT === "1"(environment variable). The parameter rename from_pluginConfigtopluginConfigcorrectly indicates it's now being used.lib/config.ts (2)
12-22: LGTM! Clean refactor to enable runtime configuration.Converting
DEFAULT_CONFIGfrom a static constant to thegetDefaultConfig()function allows theappendEnvContextdefault to be derived from the environment variable at runtime. The function returns a fresh object each time, avoiding shared state issues.
32-73: LGTM! All call sites updated correctly.The migration from
DEFAULT_CONFIGtogetDefaultConfig()is complete and consistent throughout the function. The merge logic correctly usesdefaults.loggingin all paths (default, success, and error).lib/request/request-transformer.ts (3)
25-30: LGTM! Clear documentation of the new option.The
appendEnvContextoption is properly documented and typed inTransformRequestOptions.
37-99: LGTM! Clean propagation of appendEnvContext flag.The
appendEnvContextflag is correctly added totransformInputForCodexand used to control the environment/context reattachment logic. The implementation maintains backward compatibility while enabling explicit control.
101-159: LGTM! Correct fallback pattern for appendEnvContext.The
transformRequestBodyfunction correctly computesappendEnvContextwith the proper priority:options.appendEnvContext(explicit) falls back toprocess.env.CODEX_APPEND_ENV_CONTEXT === "1"(environment variable). The flag is then threaded through totransformInputForCodex.
…tion or class' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
Summary
Testing