fix(sparc-service): strip agent-injected keys before SPARC evaluates tool calls - #738
Conversation
…KEYS env vars SPARC_LOG_REQUESTS=true logs the full incoming ReflectRequest JSON at INFO level so unexpected tool argument keys can be diagnosed without rebuilding. SPARC_STRIP_TOOL_ARG_KEYS=<comma-separated keys> removes the named keys from every tool_calls[].function.arguments before the request reaches SPARC. Needed as a configurable hotfix for Exgentic sending session_id in tool arguments — a key not declared in the tool spec that causes SPARC to reject the call. Both vars are no-ops when unset. No image rebuild required to toggle them; set via kubectl set env or the sparc-service ConfigMap. Signed-off-by: Vitaly Zabershinsky <VITALYZ@il.ibm.com>
…utput is visible uvicorn.run() with log_level="info" only configures the uvicorn logger, not the Python root logger — application loggers (sparc_service.api) had no handler and were silently dropped. Adding basicConfig before uvicorn.run() ensures all INFO+ log lines reach stdout. Signed-off-by: Vitaly Zabershinsky <VITALYZ@il.ibm.com>
📝 WalkthroughWalkthroughThe service configures INFO-level logging at startup. The ChangesRequest processing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 2
🤖 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 `@authbridge/sparc-service/sparc_service/__main__.py`:
- Around line 11-12: Update the logging setup in the module-level initialization
around logging.basicConfig so application logs are explicitly routed to
sys.stdout by configuring its stream, while preserving the existing INFO level.
Ensure the deployed entry point’s direct sparc_service.logger usage follows this
stdout configuration rather than defaulting to stderr.
In `@authbridge/sparc-service/sparc_service/api.py`:
- Around line 43-44: Update _strip_tool_arg_keys to validate that each tool call
and its function value are mappings before invoking .get on them. Handle
malformed entries such as null or scalar function values consistently with the
existing stripping behavior, and keep argument processing inside the endpoint’s
established error-handling path.
🪄 Autofix
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 Plus
Run ID: 79b39f85-6f17-4735-b5f0-926a0b71b7eb
📒 Files selected for processing (2)
authbridge/sparc-service/sparc_service/__main__.pyauthbridge/sparc-service/sparc_service/api.py
| import logging | ||
| logging.basicConfig(level=logging.INFO) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'basicConfig|sys\.stdout|sys\.stderr|stream=|log_config' \
authbridge/sparc-serviceRepository: rossoctl/cortex
Length of output: 1023
🌐 Web query:
Python logging.basicConfig default stream stderr documentation
💡 Result:
Yes, the Python logging.basicConfig function defaults to using sys.stderr [1]. When basicConfig is called without a filename argument, it configures the logging system by creating a StreamHandler [2][1]. If the optional stream keyword argument is not provided to basicConfig, this StreamHandler defaults to sys.stderr [1]. This behavior is inherited from the StreamHandler class itself, which defaults to sys.stderr when no stream is explicitly specified [3][1]. If you wish to send logs to standard output instead, you can explicitly pass sys.stdout to the stream argument [4]: import logging import sys logging.basicConfig(stream=sys.stdout)
Citations:
- 1: https://github.com/python/cpython/blob/3.11/Lib/logging/__init__.py
- 2: https://docs.python.org/3/library/logging.html
- 3: https://stackoverflow.com/questions/56496458/does-python-logging-write-to-stdout-or-stderr-by-default
- 4: https://stackoverflow.com/questions/58718153/python3-logging-basicconfig-sends-everything-to-stderr
Route application logs to stdout.
logging.basicConfig() defaults to sys.stderr, so the sparc_service.logger call in this module does not meet the stdout contract. Add stream=sys.stdout, or route this logger through Uvicorn’s logging configuration. Verify the deployed entry point uses loguru or sparc_service.logger directly.
🤖 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 `@authbridge/sparc-service/sparc_service/__main__.py` around lines 11 - 12,
Update the logging setup in the module-level initialization around
logging.basicConfig so application logs are explicitly routed to sys.stdout by
configuring its stream, while preserving the existing INFO level. Ensure the
deployed entry point’s direct sparc_service.logger usage follows this stdout
configuration rather than defaulting to stderr.
91262d0 to
651bb04
Compare
|
Good catch on the root cause — a spurious static-layer reject from an undeclared 1. Overlap with #739 — pick one home for this codeThe 2. Config bypasses
|
…s integration, null function guard
- settings.py: add log_requests (bool) and strip_tool_arg_keys (frozenset)
fields to Settings dataclass; parse them from SPARC_LOG_REQUESTS and
SPARC_STRIP_TOOL_ARG_KEYS in Settings.from_env() using _truthy()
- api.py: remove module-level os.getenv for _LOG_REQUESTS/_STRIP_KEYS;
thread settings into the reflect endpoint so it reads settings.log_requests
and settings.strip_tool_arg_keys instead of module globals; drop unused
os import
- api.py: guard _strip_tool_arg_keys against {"function": null} — use
`fn = tc.get("function") or {}` and skip the entry (not crash) when fn
is not a dict, so a null function key returns 400 not 500
- __main__.py: hoist `import logging` to module top (was inside main())
Signed-off-by: Vitaly Zabershinsky <VITALYZ@il.ibm.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
authbridge/sparc-service/sparc_service/settings.py (1)
159-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
SPARC_STRIP_TOOL_ARG_KEYSparsing.Cover whitespace trimming, duplicate keys, and an unset variable. Assert the resulting
frozensetbefore the API consumes this setting.🤖 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 `@authbridge/sparc-service/sparc_service/settings.py` around lines 159 - 163, Add focused tests for the settings parsing that initializes strip_tool_arg_keys, covering whitespace trimming, duplicate-key deduplication, and an unset SPARC_STRIP_TOOL_ARG_KEYS variable. Assert the resulting frozenset directly before any API consumption.authbridge/sparc-service/sparc_service/api.py (1)
27-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression tests for the sanitizer.
Add or verify tests for JSON strings, dictionary arguments, invalid JSON preservation, configured-key removal, missing
function,function: null, and scalar function values. The malformed-call test must assert the endpoint returns a 4xx response, not only that the helper avoids an exception.As stated in the PR objectives: add unit tests for argument stripping.
🤖 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 `@authbridge/sparc-service/sparc_service/api.py` around lines 27 - 45, Add regression tests for _strip_tool_arg_keys covering JSON-string and dictionary arguments, configured-key removal, invalid JSON preservation, missing or null function values, and scalar function values. Also add a malformed tool-call endpoint test that asserts a 4xx response, ensuring endpoint validation is verified rather than only helper exception avoidance.
🤖 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 `@authbridge/sparc-service/sparc_service/api.py`:
- Around line 78-87: Update the request logging in the visible reflect-request
handling block so both payload logs use log.debug rather than log.info, while
preserving the existing SPARC_LOG_REQUESTS setting as the gate for DEBUG output.
Redact configured sensitive tool-argument keys before logging the incoming
request, and ensure neither the pre-strip request nor post-strip tool_calls
exposes raw sensitive values.
- Around line 81-83: Restrict the stripping performed by the request handling
flow around _strip_tool_arg_keys to a protected allowlist of harmless internal
logging metadata, rather than arbitrary operator-provided keys. Preserve
/reflect request headers and all authorization, session, IBAC, token-exchange,
and policy-related arguments; add a regression test proving those keys remain
intact while approved metadata is removed.
In `@authbridge/sparc-service/sparc_service/settings.py`:
- Around line 184-185: Update the SPARC_LOG_REQUESTS handling in the settings
construction flow to validate unrecognized non-empty values instead of silently
treating them as false. When the value is not an accepted boolean
representation, append a descriptive error to the existing errors collection
while preserving valid true, false, and unset behavior.
---
Nitpick comments:
In `@authbridge/sparc-service/sparc_service/api.py`:
- Around line 27-45: Add regression tests for _strip_tool_arg_keys covering
JSON-string and dictionary arguments, configured-key removal, invalid JSON
preservation, missing or null function values, and scalar function values. Also
add a malformed tool-call endpoint test that asserts a 4xx response, ensuring
endpoint validation is verified rather than only helper exception avoidance.
In `@authbridge/sparc-service/sparc_service/settings.py`:
- Around line 159-163: Add focused tests for the settings parsing that
initializes strip_tool_arg_keys, covering whitespace trimming, duplicate-key
deduplication, and an unset SPARC_STRIP_TOOL_ARG_KEYS variable. Assert the
resulting frozenset directly before any API consumption.
🪄 Autofix
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 Plus
Run ID: 5c0486ac-075a-4a62-a90d-fc81c77dda25
📒 Files selected for processing (3)
authbridge/sparc-service/sparc_service/__main__.pyauthbridge/sparc-service/sparc_service/api.pyauthbridge/sparc-service/sparc_service/settings.py
🚧 Files skipped from review as they are similar to previous changes (1)
- authbridge/sparc-service/sparc_service/main.py
| if settings.log_requests: | ||
| log.info("incoming reflect request: %s", request.model_dump_json()) | ||
|
|
||
| if settings.strip_tool_arg_keys and request.tool_calls: | ||
| request = request.model_copy( | ||
| update={"tool_calls": _strip_tool_arg_keys(request.tool_calls, settings.strip_tool_arg_keys)} | ||
| ) | ||
| if settings.log_requests: | ||
| log.info("after strip (%s): tool_calls=%s", sorted(settings.strip_tool_arg_keys), request.tool_calls) | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log raw request payloads at INFO level.
log.info logs the complete request before stripping and logs tool calls again afterward. With SPARC_LOG_REQUESTS enabled, session identifiers and other tool arguments can enter normal production logs. Use log.debug, redact sensitive configured keys, and ensure the setting enables DEBUG output without promoting payloads to INFO.
As stated in the PR objectives: SPARC_LOG_REQUESTS must log incoming requests at DEBUG level.
🤖 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 `@authbridge/sparc-service/sparc_service/api.py` around lines 78 - 87, Update
the request logging in the visible reflect-request handling block so both
payload logs use log.debug rather than log.info, while preserving the existing
SPARC_LOG_REQUESTS setting as the gate for DEBUG output. Redact configured
sensitive tool-argument keys before logging the incoming request, and ensure
neither the pre-strip request nor post-strip tool_calls exposes raw sensitive
values.
| if settings.strip_tool_arg_keys and request.tool_calls: | ||
| request = request.model_copy( | ||
| update={"tool_calls": _strip_tool_arg_keys(request.tool_calls, settings.strip_tool_arg_keys)} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'SPARC_STRIP_TOOL_ARG_KEYS|strip_tool_arg_keys|session_id|IBAC|token|authorization' \
authbridge/sparc-service \
--glob '*.py' \
--glob '*.yaml'Repository: rossoctl/cortex
Length of output: 16840
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementations and nearby tests without running repository code.
sed -n '1,130p' authbridge/sparc-service/sparc_service/api.py
printf '\n--- settings.py relevant section ---\n'
sed -n '130,195p' authbridge/sparc-service/sparc_service/settings.py
printf '\n--- tests around strip/logging ---\n'
sed -n '1,180p' authbridge/sparc-service/tests/test_api.py
printf '\n--- config/docs references ---\n'
rg -n -C 3 'SPARC_STRIP_TOOL_ARG_KEYS|strip_tool_arg_keys|SECURE|SESSION|IBAC|Authorization|Authorization|session_id' . \
--glob '*.md' --glob '*.txt' --glob '*.yaml' --glob '*.yml' --glob '*.py' | sed -n '1,240p'Repository: rossoctl/cortex
Length of output: 40077
Restrict stripping to harmless request metadata.
The strip setting accepts operator-provided key names, and _strip_tool_arg_keys() removes them from reflection arguments before SPARC evaluates the tool call. If this list includes authorization/session keys used by IBAC, token exchange, or policy decisions, it can bypass controls. Keep /reflect request headers intact, and only strip request-specific logging metadata through a protected allowlist or fixed internal keys, plus a regression test.
🤖 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 `@authbridge/sparc-service/sparc_service/api.py` around lines 81 - 83, Restrict
the stripping performed by the request handling flow around _strip_tool_arg_keys
to a protected allowlist of harmless internal logging metadata, rather than
arbitrary operator-provided keys. Preserve /reflect request headers and all
authorization, session, IBAC, token-exchange, and policy-related arguments; add
a regression test proving those keys remain intact while approved metadata is
removed.
Source: Coding guidelines
| log_requests=_truthy(os.getenv("SPARC_LOG_REQUESTS", "")), | ||
| strip_tool_arg_keys=strip_tool_arg_keys, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid SPARC_LOG_REQUESTS values.
_truthy() returns False for every unrecognized value. A typo such as SPARC_LOG_REQUESTS=treu therefore disables request logging without reporting a configuration error. Validate the value and append an error to errors, consistent with the existing settings validation flow.
🤖 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 `@authbridge/sparc-service/sparc_service/settings.py` around lines 184 - 185,
Update the SPARC_LOG_REQUESTS handling in the settings construction flow to
validate unrecognized non-empty values instead of silently treating them as
false. When the value is not an accepted boolean representation, append a
descriptive error to the existing errors collection while preserving valid true,
false, and unset behavior.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
3 participants
Problem
Exgentic wraps every agent response as a fake
messagetool call andinjects a
session_idkey into its JSON arguments (e.g.{"content": "...", "session_id": "912ebc98-..."}). This key is notdeclared in any tool spec, so SPARC's static layer correctly rejects the
call — but the rejection is spurious: it's a structural technicality, not
a real policy violation, and it short-circuits SPARC's semantic evaluation
before it ever runs.
Verified live: a real Tau2
exchange_delivered_order_itemsWRITE call wasstatic-layer-rejected purely because of the injected
session_id(
decision=reject score=- ms=4.1).Fix
SPARC_STRIP_TOOL_ARG_KEYS=<comma-separated keys>— removes the namedkeys from every
tool_calls[].function.argumentsbefore SPARC evaluatesthe call. No-op when unset.
SPARC_LOG_REQUESTS=true— logs the full incomingReflectRequestJSONat DEBUG level, for diagnosing unexpected argument keys without
rebuilding. No-op when unset.
log.info()/log.debug()output fromsparc_service.*loggers actually reaches stdout (uvicorn'slog_level="info"only configures uvicorn's own logger, not the Pythonroot logger, so application-level logs were silently dropped).
Verification
Set
SPARC_STRIP_TOOL_ARG_KEYS=session_id, re-ran the same tool call:decision=approve score=1.00 ms=6990.7— nosession_idin theevaluated args, and the ~7s latency confirms a genuine LLM-backed semantic
evaluation ran this time (vs. the ~4ms static-layer short-circuit before).
Summary by CodeRabbit
New Features
Bug Fixes