HTTP Header propogation - #1675
Merged
Merged
Conversation
Previously, HTTP headers from incoming requests could only be forwarded to MCP servers via extra_headers templates. This change extends header propagation to all toolset types: - Shared utility: Extract header rendering logic from MCP into holmes/utils/header_rendering.py for reuse across toolset types - Toolset base class: Add extra_headers field and render_extra_headers() method to the Toolset base class so any toolset can declare Jinja2 template headers referencing request_context and env vars - HTTP toolset: Merge rendered extra_headers into outgoing HTTP requests - YAML toolsets: Expose rendered extra_headers as HOLMES_HEADER_* env vars in subprocess execution for safe use in bash commands - Python toolsets: Demonstrate the pattern with ServiceNow, passing rendered_extra_headers through to API request methods - MCP toolset: Refactored to use shared utility; now also merges toolset-level extra_headers alongside config-level ones - ToolInvokeContext: Pre-render extra_headers at invocation time via tool_to_toolset mapping so all tool types get them automatically https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
Create a dedicated documentation page for the extra_headers / request context header propagation feature covering all toolset types (MCP, HTTP connectors, custom YAML, and Python toolsets). - New page: docs/data-sources/header-propagation.md - Condensed the MCP docs inline section to a short summary with link - Added header propagation to HTTP connectors feature list - Added $HOLMES_HEADER_* to custom toolsets variable syntax section - Documented HOLMES_PASSTHROUGH_BLOCKED_HEADERS env var - Added nav entries in mkdocs.yml and .nav.yml - Links to servicenow_tables.py as Python toolset reference impl https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
Resolved conflicts: - mkdocs.yml: kept header-propagation nav entry, dropped removed permissions entry - .nav.yml: same as above - tool_executor.py: adopted master's _tool_to_toolset private naming, kept both get_toolset_for_tool() and ensure_toolset_initialized() https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
extra_headers is connection configuration, not toolset metadata, so it belongs under the config section alongside other connection settings like endpoints, auth, and URLs. Changes: - Remove extra_headers field from Toolset base class - Add extra_headers to ToolsetConfig (base config for Python toolsets) - Add extra_headers to HttpToolsetConfig - Update Toolset.render_extra_headers() to read from self.config (handles both dict configs for YAML toolsets and Pydantic model configs for Python toolsets) - Simplify MCP _render_headers() to only use config-level extra_headers (remove toolset-level merge since field no longer exists there) - Update all tests and docs to use config-level placement https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
…1:58157/git/HolmesGPT/holmesgpt into claude/propagate-http-headers-7KYO8
https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
The example showed a double-indirection pattern (configure extra_headers then reference the env var in the command) that was hard to follow. Replaced with a tip pointing users to HTTP connectors for this use case, and kept the env var naming convention documented for advanced users. https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
The previous wording focused on "per-request auth" which missed the point. The real advantage of HTTP/Python toolsets over YAML for API calls is automatic header merging without env var wiring. https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
…ity, MCP tabs - Add full e2e YAML toolset example showing extra_headers config with env var usage in command - Clarify that Python toolset header propagation is opt-in per toolset, not automatic - Restore 3-tab layout (CLI/Holmes Helm/Robusta Helm) in MCP Advanced Configuration https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
…daction, MCP double-render - Fix precedence docs: extra_headers wins over LLM headers, not vice versa - Fix CaseInsensitiveDict: override __contains__ and get() for full case-insensitive support - Fix _render_single_template: let exceptions propagate so failed templates are skipped instead of sending raw Jinja2 syntax as header values - Fix ToolInvokeContext.model_dump(): redact header values while preserving header names - Fix MCP double-rendering: override render_extra_headers() to return empty since MCP handles headers at connection time via _render_headers() - Fix broken doc link to non-existent ../contributing/python-toolsets.md - Update extract_passthrough_headers docstring to mention all toolset types - Clarify config nesting applies to all toolset types in docs - Add tests for CaseInsensitiveDict.__contains__/get(), MCP render_extra_headers override https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
…s in header propagation docs https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
… headers The config key is the same across all toolset types, but YAML tools deliver header values via HOLMES_HEADER_* environment variables because bash commands cannot receive HTTP headers directly. https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
YAML toolsets expose values as HOLMES_HEADER_* environment variables, not HTTP headers. Using a distinct config key (extra_env_vars) makes the semantics clear and avoids confusion with extra_headers used by MCP, HTTP, and Python toolsets where values become actual HTTP headers. - Override render_extra_headers() on YAMLToolset to read extra_env_vars - Update docs to show extra_env_vars for YAML examples - Update tests to verify YAML ignores extra_headers, reads extra_env_vars https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
…ts own toolset Tools now call self._toolset.render_extra_headers() directly instead of receiving pre-rendered headers through ToolInvokeContext. This removes the middleman pattern where tool_calling_llm.py looked up the toolset and pre-rendered headers for every tool call. - Add _toolset back-reference to YAMLTool, set in YAMLToolset.__init__ - YAMLTool, HttpTool, ServiceNow tools render headers from their own toolset - Remove rendered_extra_headers field from ToolInvokeContext - Remove get_toolset_for_tool (no longer needed) - Fix pre-existing test assertion for granular header redaction https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
…unrelated test change - Fix YAMLTool.__invoke_script signature to match actual 3-tuple return - Remove # type: ignore comments that masked the mismatch - Revert test_mcp_toolset.py change (pre-existing issue, not related to B3) https://claude.ai/code/session_01MqivZae5mKFbitjMFmU3WD Signed-off-by: Claude <noreply@anthropic.com>
…CP override
- Replace custom CaseInsensitiveDict with requests.structures.CaseInsensitiveDict
- Remove MCP render_extra_headers() override that returned {}; _render_headers()
now calls super().render_extra_headers() for template rendering
- Remove unused render_template_headers import from MCP toolset
- Update tests to match new behavior
https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if
Signed-off-by: Claude <noreply@anthropic.com>
Contributor
📂 Previous Runs📜 Run @ c8c33a9 (#22770895613)✅ Results of HolmesGPT evalsAutomatically triggered by commit c8c33a9 on branch Results of HolmesGPT evals
📜 Run @ 445a381 (#22770343625)✅ Results of HolmesGPT evalsAutomatically triggered by commit 445a381 on branch Results of HolmesGPT evals
📜 Run @ ef76fec (#22769990261)✅ Results of HolmesGPT evalsAutomatically triggered by commit ef76fec on branch 📜 Run @ e03c569 (#22769703875)✅ Results of HolmesGPT evalsAutomatically triggered by commit e03c569 on branch Results of HolmesGPT evals
📜 Run @ 35492cf (#22764824498)✅ Results of HolmesGPT evalsAutomatically triggered by commit 35492cf on branch Results of HolmesGPT evals
✅ Results of HolmesGPT evalsAutomatically triggered by commit b0a7185 on branch Results of HolmesGPT evals
📖 Legend
🔄 Re-run evals manually
Option 1: Comment on this PR with Or with more options (one per line): Run evals on a different branch (e.g., master) for comparison:
Quick re-run: Use Option 2: Trigger via GitHub Actions UI → "Run workflow" Option 3: Add PR labels to include extra evals in automatic regression runs:
Examples: 🏷️ Valid tags
Commands: CLI: |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
✅ Docker images ready for
Use these tags to pull the images for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:01bedb6f
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:01bedb6f me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:01bedb6f
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:01bedb6f
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:01bedb6f
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes-operator:01bedb6f me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:01bedb6f
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-operator-dev:01bedb6fPatch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:01bedb6f \
--set operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set operator.image=holmes-operator-dev:01bedb6fRobusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:01bedb6f \
--set holmes.operator.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.operator.image=holmes-operator-dev:01bedb6f |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds HTTP header propagation in server mode: incoming request headers can be rendered (Jinja2 + env vars) and forwarded to backend toolsets (MCP, HTTP, YAML, Python) as HTTP headers or injected as Changes
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant Server as HolmesGPT Server
participant LLM as Tool-Calling LLM
participant Toolset as Toolset (MCP/HTTP/YAML/Python)
participant Renderer as Header Renderer
participant Backend as Backend API / Subprocess
Client->>Server: HTTP request (with headers)
Server->>Server: extract_passthrough_headers()
Server->>LLM: invoke tool (includes request_context)
LLM->>Toolset: _directly_invoke_tool_call(request_context)
Toolset->>Toolset: prepare_invoke_context(context)
Toolset->>Renderer: render_extra_headers(config, request_context)
Renderer-->>Toolset: rendered headers dict
Toolset->>Toolset: convert to HOLMES_HEADER_* env or merge into HTTP headers
Toolset->>Backend: call backend (HTTP request or subprocess) with merged headers / env
Backend-->>Toolset: response
Toolset-->>LLM: tool result
LLM-->>Server: tool result
Server-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Contributor
🔬 CLI Performance Benchmark🟡 Startup Time (no LLM)Measures
🟡 Full CLI with LLMMeasures
PR: |
![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: 5
🧹 Nitpick comments (1)
docs/data-sources/api-toolsets.md (1)
56-56: Prefer linking header propagation outside the “Key Features” list.Please avoid expanding this list section and place the link in configuration/reference flow instead to reduce staleness.
As per coding guidelines, "Skip 'Capabilities', 'Security Best Practices', and similar list sections in documentation. Users discover capabilities by using Holmes and understand security basics. Feature lists become stale quickly."Suggested minimal change
-- **[Header Propagation](header-propagation.md)**: Forward HTTP headers from incoming requests to backend APIs using `extra_headers` templates🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/data-sources/api-toolsets.md` at line 56, Move the "Header Propagation" link out of the "Key Features" list to avoid expanding that feature list; remove the bullet "- **[Header Propagation](header-propagation.md)**: Forward HTTP headers..." from the Key Features section and instead add a single contextual reference/link to header-propagation.md in the configuration or reference flow section (e.g., where API call configuration or extra_headers templates are documented), keeping the description minimal and avoiding adding new bullets to capability lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/data-sources/header-propagation.md`:
- Around line 272-276: Replace the secret-like token placeholder in the curl
example (the -H "X-Auth-Token: your-token-here" header) with a neutral,
non-secret-looking placeholder such as "X-Auth-Token: YOUR_TOKEN" or
"X-Auth-Token: <TOKEN>" so secret scanners stop flagging it; update the curl
command and any related examples in header-propagation.md to use the chosen
neutral placeholder consistently.
- Line 271: Replace the fenced code blocks labeled "bash" with indented code
blocks to satisfy markdownlint MD046: remove the opening ```bash and closing ```
lines around both snippets and indent each line of the code examples by four
spaces (preserving the exact code content and spacing). Ensure both occurrences
(the two fenced blocks highlighted in the comment) are changed so the examples
become indented code blocks rather than fenced blocks.
In `@holmes/core/tools.py`:
- Around line 1027-1049: YAMLToolset.render_extra_headers currently only reads
"extra_env_vars" and ignores the canonical "extra_headers" key; update
render_extra_headers to check both keys (prefer "extra_env_vars" but fall back
to "extra_headers") for dict and object-style configs, assign the found mapping
to extra_env_vars/extra_headers variable, and then pass that mapping into
render_template_headers exactly as existing code does (retain request_context
and source_name=self.name) so legacy configs using extra_headers continue to
work.
- Around line 581-585: In the finally block after calling
self.__execute_subprocess(temp_script_path, extra_env) replace the
subprocess.run(["rm", temp_script_path]) call with os.remove(temp_script_path)
to avoid spawning a subprocess for file deletion; keep using the existing
temp_script_path variable and wrap os.remove(...) in a try/except if you want to
silently ignore missing-file errors (e.g., catch FileNotFoundError) so the
function (which returns output, return_code, rendered_script) behavior is
preserved.
In `@tests/test_header_propagation.py`:
- Line 410: The test assigns a local variable result from the call to
tool._invoke but never uses it, causing a lint error; either remove the
assignment by calling tool._invoke(...) without capturing the return or replace
result with _ if intentionally ignored, or add a meaningful assertion (e.g.,
assert result is not None or assert expected value) to use the value. Locate the
invocation of tool._invoke in tests/test_header_propagation.py (the result
variable) and apply one of these fixes so the variable is either used or not
assigned.
---
Nitpick comments:
In `@docs/data-sources/api-toolsets.md`:
- Line 56: Move the "Header Propagation" link out of the "Key Features" list to
avoid expanding that feature list; remove the bullet "- **[Header
Propagation](header-propagation.md)**: Forward HTTP headers..." from the Key
Features section and instead add a single contextual reference/link to
header-propagation.md in the configuration or reference flow section (e.g.,
where API call configuration or extra_headers templates are documented), keeping
the description minimal and avoiding adding new bullets to capability lists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6920027-7eb4-4730-83d1-4c408bc5522f
📒 Files selected for processing (16)
docs/data-sources/.nav.ymldocs/data-sources/api-toolsets.mddocs/data-sources/custom-toolsets.mddocs/data-sources/header-propagation.mddocs/data-sources/remote-mcp-servers.mddocs/reference/environment-variables.mdholmes/core/tools.pyholmes/plugins/toolsets/http/http_toolset.pyholmes/plugins/toolsets/mcp/toolset_mcp.pyholmes/plugins/toolsets/servicenow_tables/servicenow_tables.pyholmes/utils/header_rendering.pyholmes/utils/pydantic_utils.pymkdocs.ymlserver.pytests/plugins/toolsets/http/test_http_toolset.pytests/test_header_propagation.py
…anges 1. test_tool_invoke_context_sanitizes_request_context: Update assertion to match new per-header redaction (header names preserved, values redacted individually) instead of old blanket redaction. 2. test_nested_config_model_field: Change NestedLabelsConfig to inherit from BaseModel instead of ToolsetConfig, since it's a nested label config not a toolset config. This avoids inheriting the new extra_headers field which caused an unexpected 3rd child node. Also added explanatory comment to model_dump redaction logic. https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
holmes/core/tools.py (2)
585-585:⚠️ Potential issue | 🟡 MinorUse
os.remove()for temp-file cleanup instead of spawningrm.Line 585 still shells out for a local file delete. This is unnecessary, less portable, and keeps the static-analysis security warning alive.
🔧 Proposed fix
try: output, return_code = self.__execute_subprocess(temp_script_path, extra_env) finally: - subprocess.run(["rm", temp_script_path]) + try: + os.remove(temp_script_path) + except FileNotFoundError: + pass return output, return_code, rendered_script🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tools.py` at line 585, Replace the subprocess call that removes the temp file (subprocess.run(["rm", temp_script_path]) in holmes/core/tools.py) with a direct os.remove(temp_script_path) call, add an import os if missing, and wrap the removal in a small try/except (catch OSError) to log or ignore failures instead of shelling out; update any references to temp_script_path and remove the subprocess.run invocation so the file deletion is done natively and portably.
1037-1043:⚠️ Potential issue | 🟠 MajorYAML toolset still ignores canonical
extra_headersconfig.Lines 1039-1043 read only
extra_env_vars. Configs usingextra_headers(the canonical toolset config key) will be silently skipped for YAML toolsets.🔧 Proposed compatibility fix
extra_env_vars: Optional[Dict[str, str]] = None if self.config is not None: if isinstance(self.config, dict): - extra_env_vars = self.config.get("extra_env_vars") + extra_env_vars = self.config.get("extra_env_vars") or self.config.get( + "extra_headers" + ) else: - extra_env_vars = getattr(self.config, "extra_env_vars", None) + extra_env_vars = getattr(self.config, "extra_env_vars", None) or getattr( + self.config, "extra_headers", None + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tools.py` around lines 1037 - 1043, The YAML toolset currently only reads self.config["extra_env_vars"] (or getattr(self.config, "extra_env_vars")) and therefore ignores the canonical key "extra_headers"; update the config extraction logic in the same block that sets extra_env_vars so it also checks for "extra_headers" when self.config is a dict (falling back to "extra_env_vars" if "extra_headers" is absent) and when self.config is an object (use getattr(self.config, "extra_headers", None) before getattr(..., "extra_env_vars", None)); assign the found value to extra_env_vars so downstream code using extra_env_vars will work with configs that provide extra_headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/core/tools.py`:
- Around line 202-205: The redaction currently checks for dict specifically and
misses mapping-like header objects (e.g., CaseInsensitiveDict) stored in
ctx["headers"], so switch the type check to use collections.abc.Mapping (ensure
Mapping is imported) and, when ctx.get("headers") is a Mapping, replace it with
a plain dict mapping each key to "***REDACTED***" (e.g., {k: "***REDACTED***"
for k in ctx["headers"].keys()}) before model_dump() is called; this ensures all
Mapping implementations are redacted while preserving header keys.
---
Duplicate comments:
In `@holmes/core/tools.py`:
- Line 585: Replace the subprocess call that removes the temp file
(subprocess.run(["rm", temp_script_path]) in holmes/core/tools.py) with a direct
os.remove(temp_script_path) call, add an import os if missing, and wrap the
removal in a small try/except (catch OSError) to log or ignore failures instead
of shelling out; update any references to temp_script_path and remove the
subprocess.run invocation so the file deletion is done natively and portably.
- Around line 1037-1043: The YAML toolset currently only reads
self.config["extra_env_vars"] (or getattr(self.config, "extra_env_vars")) and
therefore ignores the canonical key "extra_headers"; update the config
extraction logic in the same block that sets extra_env_vars so it also checks
for "extra_headers" when self.config is a dict (falling back to "extra_env_vars"
if "extra_headers" is absent) and when self.config is an object (use
getattr(self.config, "extra_headers", None) before getattr(...,
"extra_env_vars", None)); assign the found value to extra_env_vars so downstream
code using extra_env_vars will work with configs that provide extra_headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2275eb4c-a029-468c-9c64-fdd30adc1f5a
📒 Files selected for processing (4)
holmes/core/tools.pyholmes/plugins/toolsets/mcp/toolset_mcp.pytests/test_mcp_toolset.pytests/test_toolset_config_tui.py
🚧 Files skipped from review as they are similar to previous changes (1)
- holmes/plugins/toolsets/mcp/toolset_mcp.py
Remove extra_headers from ToolsetConfig base class so it no longer
pollutes the schema of every Python toolset. Only toolsets that
actually use extra_headers now define it:
- ServiceNowTablesConfig (added explicitly)
- MCPConfig (already had its own field)
- HttpToolsetConfig (already had its own field, extends BaseModel)
The base Toolset.render_extra_headers() uses getattr with a default,
so it gracefully returns {} for configs without the field.
https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if
Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_header_propagation.py (1)
377-420:⚠️ Potential issue | 🟡 MinorRemove or assert the unused
resultvariable.Line 412 assigns
resultbut never uses it. The test verifies behavior viamock_request.call_argsrather than the return value.💡 Minimal fix
- result = tool._invoke( + _ = tool._invoke( {"url": "https://api.example.com/test"}, ctx, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_header_propagation.py` around lines 377 - 420, The test test_extra_headers_override_defaults assigns an unused variable result from tool._invoke; remove the unused assignment or replace it with an assertion that uses the return value to avoid dead code. Update the call to tool._invoke in test_extra_headers_override_defaults (the line currently setting result) by either calling tool._invoke(...) without assignment or adding a meaningful assert against its return, ensuring the rest of the test still inspects mock_request.call_args to validate header propagation.
🧹 Nitpick comments (2)
holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py (2)
353-358: Theor Noneis redundant and slightly misleading.Per Context snippet 1,
render_extra_headers()always returns aDict[str, str]—either{}when no headers are configured or the rendered headers. It never returnsNone. Theor Noneconverts an empty dict toNone, which works but is unnecessary since_make_api_requesthandles both cases identically withif extra_headers:.Consider removing
or Nonefor clarity:🔧 Suggested simplification
data, headers = self._toolset._make_api_request( endpoint=endpoint, query_params=query_params, timeout=30, - extra_headers=self._toolset.render_extra_headers(context.request_context) or None, + extra_headers=self._toolset.render_extra_headers(context.request_context), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py` around lines 353 - 358, The call site unnecessarily converts an empty dict to None by appending "or None" when passing extra headers to _make_api_request; since render_extra_headers(context.request_context) always returns a Dict[str, str] (possibly {}), remove the "or None" and pass the dict directly to extra_headers so _make_api_request receives the rendered headers as-is (locate this change around the call to self._toolset._make_api_request and the render_extra_headers invocation).
456-458: Same redundantor Nonepattern as inGetRecords._invoke.For consistency, consider removing
or Nonehere as well.🔧 Suggested simplification
- return self._make_servicenow_request( - endpoint, params, query_params, extra_headers=self._toolset.render_extra_headers(context.request_context) or None - ) + return self._make_servicenow_request( + endpoint, + params, + query_params, + extra_headers=self._toolset.render_extra_headers(context.request_context), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py` around lines 456 - 458, The call to self._make_servicenow_request passes extra_headers=self._toolset.render_extra_headers(context.request_context) or None which redundantly converts falsy values to None; remove the trailing "or None" so extra_headers is passed directly (i.e., extra_headers=self._toolset.render_extra_headers(context.request_context)) for consistency with GetRecords._invoke and to avoid unnecessary conditional casting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_header_propagation.py`:
- Around line 377-420: The test test_extra_headers_override_defaults assigns an
unused variable result from tool._invoke; remove the unused assignment or
replace it with an assertion that uses the return value to avoid dead code.
Update the call to tool._invoke in test_extra_headers_override_defaults (the
line currently setting result) by either calling tool._invoke(...) without
assignment or adding a meaningful assert against its return, ensuring the rest
of the test still inspects mock_request.call_args to validate header
propagation.
---
Nitpick comments:
In `@holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py`:
- Around line 353-358: The call site unnecessarily converts an empty dict to
None by appending "or None" when passing extra headers to _make_api_request;
since render_extra_headers(context.request_context) always returns a Dict[str,
str] (possibly {}), remove the "or None" and pass the dict directly to
extra_headers so _make_api_request receives the rendered headers as-is (locate
this change around the call to self._toolset._make_api_request and the
render_extra_headers invocation).
- Around line 456-458: The call to self._make_servicenow_request passes
extra_headers=self._toolset.render_extra_headers(context.request_context) or
None which redundantly converts falsy values to None; remove the trailing "or
None" so extra_headers is passed directly (i.e.,
extra_headers=self._toolset.render_extra_headers(context.request_context)) for
consistency with GetRecords._invoke and to avoid unnecessary conditional
casting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 698151ce-a77d-4e70-b416-076bb3c24d69
📒 Files selected for processing (2)
holmes/plugins/toolsets/servicenow_tables/servicenow_tables.pytests/test_header_propagation.py
…lset - Revert ToolInvokeContext.model_dump() to original aggressive sanitization (redacts entire request_context, not just headers) since it's only used in tests and the defensive approach is safer - Remove _toolset private attr from YAMLTool; instead add extra_env field to ToolInvokeContext and Toolset.prepare_invoke_context() hook - YAMLToolset.prepare_invoke_context() renders headers and populates context.extra_env before the tool runs - Tool executor calls prepare_invoke_context() via new get_toolset_for_tool() - YAMLTool._invoke() reads context.extra_env instead of reaching back to toolset https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_header_propagation.py (1)
209-210: Move these imports to module scope.Keeping imports inside the test methods violates the repo rule and makes the file harder to scan.
As per coding guidelines, "Always place Python imports at the top of the file, not inside functions or methods".
Also applies to: 353-353, 398-398, 447-447, 465-465, 486-486, 508-508, 524-524
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_header_propagation.py` around lines 209 - 210, Tests contain local imports (e.g., ToolsetYamlFromConfig) placed inside test functions; move these imports to module scope at the top of tests/test_header_propagation.py so they follow the repo rule and improve readability. Locate occurrences where imports are done inside test methods (including the instance of ToolsetYamlFromConfig and the other similar in-function imports noted in the review) and hoist them to the file header, removing the in-function import lines and updating any tests to use the module-level imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/core/tool_calling_llm.py`:
- Around line 702-705: _handle_tool_call_approval is re-invoking the tool via
_directly_invoke_tool_call without propagating invoke_context.request_context,
causing forwarded headers to be lost; update the re-entry path in
_handle_tool_call_approval to pass the original request_context through to
_directly_invoke_tool_call (use the same invoke_context.request_context that
toolset.prepare_invoke_context sets up), ensuring any call like
_directly_invoke_tool_call(..., request_context=invoke_context.request_context)
or equivalent is used so header propagation is preserved for retried tools.
In `@holmes/core/tools.py`:
- Line 194: The model_dump() for ToolInvokeContext is currently only redacting
request_context but not extra_env, so propagated headers like
HOLMES_HEADER_AUTHORIZATION will be serialized in full; update
ToolInvokeContext.model_dump() to also redact extra_env by treating it like
request_context (e.g., when building the dumped dict, replace sensitive
extra_env values with a placeholder such as "<redacted>"), and apply the same
key-based filtering used for request_context (at least redact keys matching
patterns like "HOLMES_HEADER_*" or containing "AUTH") while preserving
non-sensitive entries; locate references to ToolInvokeContext and its
model_dump() implementation and add extra_env to the redaction logic so the
returned serialization never contains raw authorization header values.
---
Nitpick comments:
In `@tests/test_header_propagation.py`:
- Around line 209-210: Tests contain local imports (e.g., ToolsetYamlFromConfig)
placed inside test functions; move these imports to module scope at the top of
tests/test_header_propagation.py so they follow the repo rule and improve
readability. Locate occurrences where imports are done inside test methods
(including the instance of ToolsetYamlFromConfig and the other similar
in-function imports noted in the review) and hoist them to the file header,
removing the in-function import lines and updating any tests to use the
module-level imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15fc8a83-c5b1-44c4-9fb7-88b8ca0b5413
📒 Files selected for processing (4)
holmes/core/tool_calling_llm.pyholmes/core/tools.pyholmes/core/tools_utils/tool_executor.pytests/test_header_propagation.py
…use os.remove - Pass request_context in the "already approved" path of _handle_tool_call_approval so headers aren't lost on retried tools - Redact extra_env values in ToolInvokeContext.model_dump() and __str__ to prevent leaking HOLMES_HEADER_* secrets in serialization - Replace subprocess.run(["rm", ...]) with os.remove() in YAMLTool script cleanup to avoid spawning an unnecessary subprocess https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if Signed-off-by: Claude <noreply@anthropic.com>
…late Base Toolset._get_extra_headers_template() handles both dict configs (.get()) and Pydantic configs (getattr()) so subclasses only need to override when the key name differs (e.g. YAMLToolset uses extra_env_vars). Also update extra_headers Field descriptions to mention both request context and env var templating support. https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if Signed-off-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
holmes/core/tools.py (1)
1036-1040:⚠️ Potential issue | 🟠 MajorPreserve
extra_headerscompatibility for YAML toolsets.This override now ignores the canonical
extra_headerskey and only readsextra_env_vars, so existing YAML toolset configs usingextra_headerswill silently stop propagating headers.💡 Proposed compatibility fix
def _get_extra_headers_template(self) -> Optional[Dict[str, str]]: - """YAML toolsets use ``extra_env_vars`` instead of ``extra_headers``.""" - if isinstance(self.config, dict): - return self.config.get("extra_env_vars") - return None + """YAML toolsets prefer ``extra_env_vars`` but still accept ``extra_headers``.""" + if isinstance(self.config, dict): + return self.config.get("extra_env_vars") or self.config.get("extra_headers") + return getattr(self.config, "extra_env_vars", None) or getattr( + self.config, "extra_headers", None + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holmes/core/tools.py` around lines 1036 - 1040, The _get_extra_headers_template method currently only reads "extra_env_vars" from self.config and drops the canonical "extra_headers" key; update it to preserve backwards compatibility by checking for "extra_headers" first (or merging both) when self.config is a dict and returning a dict of header values, falling back to "extra_env_vars" if "extra_headers" is missing; reference the _get_extra_headers_template method and the keys "extra_headers" and "extra_env_vars" in your change and ensure the return type remains Optional[Dict[str, str]].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@holmes/plugins/toolsets/http/http_toolset.py`:
- Around line 87-91: _health check uses only build_headers(endpoint) and ignores
the new extra_headers field, causing health checks to miss static headers;
update _check_endpoint_health to include extra_headers by rendering and merging
them into the headers used for the health request (the same rendering/templating
used for normal requests) so that build_headers(endpoint) is extended with the
rendered endpoint.extra_headers before sending the health check request.
In `@holmes/plugins/toolsets/servicenow_tables/servicenow_tables.py`:
- Around line 58-64: prerequisites_callable currently calls
_perform_health_check without supplying the rendered extra_headers, causing
startup health checks to miss required headers; update prerequisites_callable to
render self.extra_headers using the same Jinja/env rendering logic used for
actual requests and pass the resulting headers into _perform_health_check so the
health check uses the same headers as normal invocations (i.e., locate
prerequisites_callable and ensure it invokes _perform_health_check(...,
headers=rendered_extra_headers) where rendered_extra_headers is produced from
self.extra_headers).
---
Duplicate comments:
In `@holmes/core/tools.py`:
- Around line 1036-1040: The _get_extra_headers_template method currently only
reads "extra_env_vars" from self.config and drops the canonical "extra_headers"
key; update it to preserve backwards compatibility by checking for
"extra_headers" first (or merging both) when self.config is a dict and returning
a dict of header values, falling back to "extra_env_vars" if "extra_headers" is
missing; reference the _get_extra_headers_template method and the keys
"extra_headers" and "extra_env_vars" in your change and ensure the return type
remains Optional[Dict[str, str]].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a84511b0-b0de-48cb-b842-c24405cfe580
📒 Files selected for processing (5)
holmes/core/tool_calling_llm.pyholmes/core/tools.pyholmes/plugins/toolsets/http/http_toolset.pyholmes/plugins/toolsets/servicenow_tables/servicenow_tables.pytests/test_mcp_toolset.py
Each tool type now handles its own headers without involving tool_calling_llm.py: - YAMLTool: stores extra_env_vars template + toolset name as private attrs, set by YAMLToolset at init. Renders in _invoke() itself. - HTTP/ServiceNow tools: already called _toolset.render_extra_headers() directly (unchanged). Removed from the invocation path: - ToolInvokeContext.extra_env field - Toolset.prepare_invoke_context() hook - ToolExecutor.get_toolset_for_tool() - The hook call in tool_calling_llm._directly_invoke_tool_call() https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if Signed-off-by: Claude <noreply@anthropic.com>
…rnally, drop library tests 1. Inline _get_extra_headers_template into render_extra_headers (single caller) 2. ServiceNow _make_api_request renders extra_headers internally via request_context instead of threading extra_headers through two layers 3. Remove TestCaseInsensitiveDict (tests third-party requests library) https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if Signed-off-by: Claude <noreply@anthropic.com>
…gle-caller helpers - render_template_headers -> render_header_templates (reads better) - Inline _render_extra_env and build_header_env_vars into YAMLTool._invoke (both had exactly 1 caller) - Remove unit tests for deleted build_header_env_vars (covered by existing end-to-end YAML tool tests) Signed-off-by: Claude <noreply@anthropic.com>
…all site The generic render_extra_headers method had to handle both dict and Pydantic configs. Each call site now calls render_header_templates directly with the appropriate config access pattern for its toolset type. https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if Signed-off-by: Claude <noreply@anthropic.com>
…ates directly
Instead of the extra_env_vars → render_header_templates → HOLMES_HEADER_* env
var indirection, request_context and env are now available directly in YAML
tool command/script Jinja2 templates. Users can write
{{ request_context.headers['X-Token'] }} inline in their commands.
https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if
Signed-off-by: Claude <noreply@anthropic.com>
Ensures {{ request_context.headers['x-tenant-id'] }} works case-insensitively
in YAML command templates, consistent with MCP/HTTP/Python toolsets.
https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if
Signed-off-by: Claude <noreply@anthropic.com>
The Pydantic field descriptions showed {{ headers.X_Tenant_Id }} but the
actual template syntax is {{ request_context.headers['X-Tenant-Id'] }}.
https://claude.ai/code/session_01TjsbvuR731LPWcXy2Zg3if
Signed-off-by: Claude <noreply@anthropic.com>
aantn
enabled auto-merge (squash)
March 6, 2026 15:52
arikalon1
approved these changes
Mar 6, 2026
This was referenced Mar 14, 2026
Merged
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
3 participants
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 by CodeRabbit
New Features
Documentation
Tests