fix(sparc-service): WatsonX reasoning-model support + Dockerfile fix + SPARC_SKIP_TOOLS - #739
fix(sparc-service): WatsonX reasoning-model support + Dockerfile fix + SPARC_SKIP_TOOLS#739vz-ibm wants to merge 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe SPARC service adds configurable logging, tool sanitization and skipping, expanded reflection telemetry, LLM schema handling and retries, Watsonx provider support, empty-response regression tests, and container deployment updates. ChangesSPARC service changes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SPARCAPI
participant ReflectionEngine
participant ProviderClient
participant LiteLLM
Client->>SPARCAPI: Submit request with tool calls
SPARCAPI->>SPARCAPI: Sanitize configured arguments
alt Tool is configured to skip
SPARCAPI-->>Client: Return approval response
else Reflection required
SPARCAPI->>ReflectionEngine: Evaluate sanitized request
ReflectionEngine->>ProviderClient: Request structured evaluation
ProviderClient->>LiteLLM: Send schema-injected request
LiteLLM-->>ProviderClient: Return result or empty-response error
ProviderClient->>LiteLLM: Retry matching empty-response errors
LiteLLM-->>ProviderClient: Return evaluation result
ProviderClient-->>ReflectionEngine: Return validated result
ReflectionEngine-->>SPARCAPI: Return reflection outcome
SPARCAPI-->>Client: Return approval response
end
Possibly related PRs
🚥 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| tool_name = request.tool_calls[0].get("function", {}).get("name", "") | ||
| if tool_name in _SKIP_TOOLS: | ||
| # DEBUG: per-call skip entry — visible only at DEBUG level | ||
| log.debug("reflect tool=%s skipped (SPARC_SKIP_TOOLS)", tool_name) |
e2ab362 to
24b6405
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/deploy/Makefile`:
- Around line 54-59: Update the fallback in the kind image-loading command to
save $(IMAGE) to a temporary archive first, chaining the container runtime save
with && before invoking kind load image-archive on that file. Preserve the
existing fallback behavior and ensure the temporary archive is cleaned up after
loading.
In `@authbridge/sparc-service/sparc_service/api.py`:
- Around line 113-118: The skip fast path in the request handling logic must
approve a batch only when every tool call is in the fixed infrastructure
allowlist and `_SKIP_TOOLS`; otherwise continue normal evaluation or rejection.
Add startup validation ensuring `_SKIP_TOOLS` contains only allowlisted
infrastructure tools, preserving authentication, IBAC, token-exchange, plugin,
and session-recording enforcement for all other traffic.
- Around line 29-32: Update the logging configuration in __main__.py so
LOG_LEVEL controls the root logger level, allowing DEBUG when configured, and
ensure sparc_service.api debug logging is enabled when SPARC_LOG_REQUESTS is
true. Preserve the existing SPARC_DEBUG_LLM-specific behavior and default INFO
level.
In `@authbridge/sparc-service/sparc_service/engine.py`:
- Around line 124-138: Remove raw user-controlled data from diagnostics: in
authbridge/sparc-service/sparc_service/engine.py lines 124-138, stop logging
first_tc or unredacted serialized arguments; in lines 151-163, remove raw args
from INFO and DEBUG telemetry. In authbridge/sparc-service/sparc_service/api.py
lines 102-111, apply the approved redaction and allowlisted metadata before the
first request log event. In authbridge/sparc-service/sparc_service/providers.py
lines 134-151, redact prompt, schema, and result fields before diagnostic
logging, preserving only safe allowlisted metadata.
In `@authbridge/sparc-service/sparc_service/providers.py`:
- Around line 67-68: Make wrapper installation idempotent per shared client
class: at authbridge/sparc-service/sparc_service/providers.py lines 67-68,
install the schema wrapper only once; at line 112, install the retry wrapper
only once; and at line 154, install the debug wrapper only once. Use per-class
patch markers or an instance-local wrapper/subclass, preserving the existing
wrapper behavior without stacking duplicate layers, backoff, or logs.
In `@authbridge/sparc-service/tests/test_haiku_empty_response.py`:
- Around line 62-66: Replace the production conversation data in the test
fixture for the empty-response case with a synthetic, minimized conversation
that preserves only the inputs needed to reproduce the failure. Remove all real
account identifiers, names, birth dates, payment identifiers, and other
production-derived content while keeping the test’s expected behavior unchanged.
- Around line 222-267: Update test_response_format_intermittent_empty and
test_system_prompt_mode_no_empty to be opt-in integration probes: skip unless
RUN_HAIKU_TESTS=1 and the required OAIKEY/OAIBASE credentials are present, using
the project’s pytest skip/marker conventions. Preserve their live-call
assertions when enabled, and add deterministic unit tests that mock empty
responses and exercise the retry and schema-prompt wrappers without external
credentials.
🪄 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: adbe454f-0615-4513-898e-ca48c8a30b94
📒 Files selected for processing (8)
authbridge/sparc-service/Dockerfileauthbridge/sparc-service/deploy/Makefileauthbridge/sparc-service/sparc_service/__main__.pyauthbridge/sparc-service/sparc_service/api.pyauthbridge/sparc-service/sparc_service/engine.pyauthbridge/sparc-service/sparc_service/providers.pyauthbridge/sparc-service/sparc_service/settings.pyauthbridge/sparc-service/tests/test_haiku_empty_response.py
| @# kind load docker-image fails on Linux with rootful Podman — use image-archive instead. | ||
| if kind load docker-image $(IMAGE) --name $(KIND_CLUSTER_NAME) 2>/dev/null; then \ | ||
| true; \ | ||
| else \ | ||
| $(CONTAINER_RUNTIME) save $(IMAGE) | kind load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME); \ | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
set -eu
kind version
nodes="$(kind get nodes --name rossoctl | sed '/^[[:space:]]*$/d' | wc -l | tr -d ' ')"
printf 'rossoctl node count: %s\n' "$nodes"
if [ "$nodes" -gt 1 ]; then
echo "The /dev/stdin fallback must use a temporary archive for this cluster."
exit 1
fiRepository: rossoctl/cortex
Length of output: 195
🏁 Script executed:
#!/bin/sh
set -eu
# Locate and inspect the referenced Makefile and relevant kind load behavior.
if [ -f authbridge/sparc-service/deploy/Makefile ]; then
sed -n '1,90p' authbridge/sparc-service/deploy/Makefile
else
printf 'authbridge/sparc-service/deploy/Makefile not found\n'
find . -path '*/deploy/Makefile' -maxdepth 5
fi
printf '\n--- kind references ---\n'
rg -n "kind load image-archive|kind load docker-image|image-archive|/dev/stdin|KIND_CLUSTER_NAME|CONTAINER_RUNTIME" .Repository: rossoctl/cortex
Length of output: 18619
🌐 Web query:
kubernetes-sigs kind image-archive load archive opens archive once per node kind v0.32.0
💡 Result:
In kind v0.32.0, the kind load image-archive command processes each specified archive file individually [1][2]. When multiple archives are provided, kind opens and loads each archive file sequentially for the target nodes [1]. For each node selected to receive an image, the implementation opens the archive file on the host using os.Open and streams it into the node via a pipe (or by streaming the reader) to the ctr command (containerd's CLI) running inside the node [3][4]. If you specify multiple nodes, the archive must be read and transferred to each node independently [4][5]. The kind implementation does not perform an "open once" operation across multiple nodes; rather, it handles the file reading and streaming process per node, as necessitated by the need to transmit the archive data into each individual container environment [3][4][5]. If you are concerned about performance or resource usage when loading to many nodes, using process substitution in your shell—such as kind load image-archive <(cat archive.tar)—or creating a single, combined tarball containing all desired images is generally recommended to optimize the transfer and loading process [1][2][5].
Citations:
- 1: image-archive: enable loading multiple image archives kubernetes-sigs/kind#2891
- 2: kind load image-archive xx1.tar xx2.tar , cannot find xx2.tar's image in container of kind-node kubernetes-sigs/kind#2881
- 3: https://github.com/kubernetes-sigs/kind/blob/main/pkg/cluster/nodeutils/util.go
- 4: https://github.com/kubernetes-sigs/kind/blob/main/pkg/cmd/kind/load/docker-image/docker-image.go
- 5:
kind load image-archivefrom stdin kubernetes-sigs/kind#927
🌐 Web query:
github.com kubernetes-sigs kind v0.32.0 pkg/cmd/kind/load/image-archive image-archive.go nodes archive loop
💡 Result:
In KIND (Kubernetes in Docker) version v0.32.0, the command kind load image-archive supports loading one or more image archives (tarballs) into cluster nodes [1][2]. The implementation of image-archive.go iterates over the provided arguments (the paths to the image archives) and performs the loading process for each archive [1]. When loading, the tool targets the cluster nodes—either all nodes by default or a specific set if defined by the user—and executes the loading logic for each archive on those nodes [1][3]. Historically, the command only supported loading a single archive [4], but support for multiple archives was added in pull request #2891 [1]. When multiple archives are provided, the code iterates through each file path and performs the load operation sequentially [1]. It does not involve a loop over nodes that creates redundant loading logic in the same way reported for the docker-image command (which, unlike image-archive, has faced issues where it would redundantly process nodes for each image) [5][3]. For technical details, the image-archive command ultimately uses nodeutils.LoadImageArchive to stream the archive file content to the node, where it is imported into the node's container runtime (typically containerd) [3][6]. The command also supports streaming from stdin by using - as the archive name [7].
Citations:
- 1: image-archive: enable loading multiple image archives kubernetes-sigs/kind#2891
- 2: https://kind.sigs.k8s.io/docs/user/quick-start/
- 3: https://github.com/kubernetes-sigs/kind/blob/main/pkg/cmd/kind/load/docker-image/docker-image.go
- 4: kind load image-archive xx1.tar xx2.tar , cannot find xx2.tar's image in container of kind-node kubernetes-sigs/kind#2881
- 5: Using "kind load docker-image img1 img2 ..." results in the image being loaded onto the node multiple times kubernetes-sigs/kind#3063
- 6: Accept .gz or stream in load image-archive kubernetes-sigs/kind#1636
- 7: allow to load container images from stdin kubernetes-sigs/kind#2041
🌐 Web query:
kubernetes-sigs/kind/pkg/cluster/nodeutils LoadImageArchive source
💡 Result:
The LoadImageArchive function in the kubernetes-sigs/kind repository, located in pkg/cluster/nodeutils/util.go, is responsible for loading a container image archive onto a specified KIND node [1][2]. It functions by executing the containerd command-line tool (ctr) inside the target node [1]. The function takes a node (nodes.Node) and an io.Reader (which provides the image archive data) as arguments [1]. It streams this archive data into the ctr command's standard input [1]. The specific command executed is: ctr --namespace=k8s.io images import --all-platforms --digests --snapshotter= - [1] Key aspects of the implementation include: - Snapshotter Detection: The function dynamically detects the appropriate containerd snapshotter to use by inspecting the node's containerd configuration [1]. It parses the configuration to determine the correct snapshotter based on the containerd version (supporting config versions 2, 3, and 4) [1]. - Input Handling: By using an io.Reader and piping it to the ctr command (indicated by the final "-" argument), the function supports efficient image loading from various sources, including file streams and pipes [1][3]. This utility is used by higher-level KIND commands, such as kind load docker-image, to facilitate the transfer of container images from the host environment into the KIND cluster nodes [4][5][6].
Citations:
- 1: https://github.com/kubernetes-sigs/kind/blob/main/pkg/cluster/nodeutils/util.go
- 2: https://github.com/kubernetes-sigs/kind/blob/v0.9.0/pkg/cluster/nodeutils/util.go
- 3: allow to load container images from stdin kubernetes-sigs/kind#2041
- 4: https://github.com/kubernetes-sigs/kind/blob/main/pkg/cmd/kind/load/docker-image/docker-image.go
- 5: kind load docker-image fails, but nodes believe image exists kubernetes-sigs/kind#3479
- 6: Unable kind load docker image into node kubernetes-sigs/kind#3578
🏁 Script executed:
#!/bin/sh
set -eu
printf '--- local archive loading scripts nearby ---\n'
cat -n local-build-and-test.sh | sed -n '1,45p'
printf '\n--- makefile nodes target count/default ---\n'
rg -n "control-plane|worker|node count|nodes" authbridge/sparc-service/deploy/Makefile .github README.md LOCAL_TESTING_GUIDE.md || trueRepository: rossoctl/cortex
Length of output: 244
🏁 Script executed:
#!/bin/sh
set -eu
printf '%s\n' '--- local archive loading scripts nearby ---'
sed -n '1,45p' local-build-and-test.sh
printf '%s\n' ''
printf '%s\n' '--- makefile nodes target count/default ---'
rg -n "control-plane|worker|node count|nodes" authbridge/sparc-service/deploy/Makefile .github README.md LOCAL_TESTING_GUIDE.md || trueRepository: rossoctl/cortex
Length of output: 2197
🌐 Web query:
github.com kubernetes-sigs/kind v0.32.0 pkg/cmd/kind/load/image-archive/image-archive.go func RunLoadImageArchive os.Args args load
💡 Result:
In kind v0.32.0, the function RunLoadImageArchive (located in pkg/cmd/kind/load/image-archive/image-archive.go) handles the loading of container image archives into cluster nodes. The command processes image archives passed as arguments in os.Args. Specifically: 1. Handling of Arguments: The command accepts one or more image archive file paths as arguments [1]. While earlier versions of kind (prior to PR #2891) only processed the first argument [1][2], current implementations iterate through all provided arguments (args) [1]. 2. Processing Logic: Within the command's execution flow, each provided file path (representing a tar archive) is opened by the host [3][4]. The kind tool then utilizes internal node utilities to stream the archive content into the target cluster nodes [3][4]. 3. Stdin Support: As of recent versions, users can pass "-" as an argument to indicate that the image archive should be read from standard input (stdin) [5]. This allows for workflows that pipe output directly into kind, such as docker save my-image:latest | kind load image-archive - [6][5]. The command uses the concurrent.UntilError package to efficiently distribute and load the specified archives across the selected cluster nodes in parallel [3]. If no specific nodes are selected via flags, the command defaults to loading the archives into all available cluster nodes [3].
Citations:
- 1: image-archive: enable loading multiple image archives kubernetes-sigs/kind#2891
- 2: kind load image-archive xx1.tar xx2.tar , cannot find xx2.tar's image in container of kind-node kubernetes-sigs/kind#2881
- 3: kubernetes-sigs/kind@1649222
- 4: https://github.com/kubernetes-sigs/kind/blob/main/pkg/cmd/kind/load/docker-image/docker-image.go
- 5: allow to load container images from stdin kubernetes-sigs/kind#2041
- 6:
kind load image-archivefrom stdin kubernetes-sigs/kind#927
Materialize the archive before loading it to kind.
This fallback streams one tarball into kind load image-archive. For multi-node kind clusters, kind can load the selected nodes concurrently, so the pipe can be split incorrectly. Save to a temporary archive first, chain the save with &&, then load that file.
Proposed fix
else \
- $(CONTAINER_RUNTIME) save $(IMAGE) | kind load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME); \
+ archive=$$(mktemp) || exit 1; \
+ trap 'rm -f "$$archive"' EXIT; \
+ $(CONTAINER_RUNTIME) save $(IMAGE) > "$$archive" && \
+ kind load image-archive "$$archive" --name $(KIND_CLUSTER_NAME); \
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @# kind load docker-image fails on Linux with rootful Podman — use image-archive instead. | |
| if kind load docker-image $(IMAGE) --name $(KIND_CLUSTER_NAME) 2>/dev/null; then \ | |
| true; \ | |
| else \ | |
| $(CONTAINER_RUNTIME) save $(IMAGE) | kind load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME); \ | |
| fi | |
| @# kind load docker-image fails on Linux with rootful Podman — use image-archive instead. | |
| if kind load docker-image $(IMAGE) --name $(KIND_CLUSTER_NAME) 2>/dev/null; then \ | |
| true; \ | |
| else \ | |
| archive=$$(mktemp) || exit 1; \ | |
| trap 'rm -f "$$archive"' EXIT; \ | |
| $(CONTAINER_RUNTIME) save $(IMAGE) > "$$archive" && \ | |
| kind load image-archive "$$archive" --name $(KIND_CLUSTER_NAME); \ | |
| fi |
🤖 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/deploy/Makefile` around lines 54 - 59, Update the
fallback in the kind image-loading command to save $(IMAGE) to a temporary
archive first, chaining the container runtime save with && before invoking kind
load image-archive on that file. Preserve the existing fallback behavior and
ensure the temporary archive is cleaned up after loading.
| # SPARC_LOG_REQUESTS=true — log the full incoming ReflectRequest JSON at DEBUG. | ||
| # Useful for diagnosing unexpected tool argument keys. Disabled by default — | ||
| # payloads can be large. Requires LOG_LEVEL=DEBUG to be visible. | ||
| _LOG_REQUESTS: bool = os.getenv("SPARC_LOG_REQUESTS", "").strip().lower() in {"1", "true", "yes"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'LOG_LEVEL|SPARC_LOG_REQUESTS|basicConfig|setLevel\(' authbridge/sparc-serviceRepository: rossoctl/cortex
Length of output: 3733
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant files:\n'
fd -a '__main__.py|api.py' authbridge/sparc-service | sed 's#^\./##'
printf '\n__main__.py:\n'
cat -n authbridge/sparc-service/sparc_service/__main__.py | sed -n '1,40p'
printf '\napi.py imports and request handler section:\n'
cat -n authbridge/sparc-service/sparc_service/api.py | sed -n '1,130p'
printf '\nCheck whether logger hierarchy enables sparc_service(api) DEBUG under root INFO:\n'
python3 - <<'PY'
import logging
logging.basicConfig(level=logging.INFO)
logging.getLogger("LiteLLM").setLevel(logging.WARNING)
log = logging.getLogger("sparc_service.api")
print("root level:", logging.root.level)
print("sparc_service.api effective level:", log.getEffectiveLevel())
print("debug enabled:", log.isEnabledFor(logging.DEBUG))
PYRepository: rossoctl/cortex
Length of output: 8201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant files:\n'
fd -a '__main__.py|api.py' authbridge/sparc-service | sed 's#^\./##'
printf '\n__main__.py:\n'
cat -n authbridge/sparc-service/sparc_service/__main__.py | sed -n '1,40p'
printf '\napi.py imports and request handler section:\n'
cat -n authbridge/sparc-service/sparc_service/api.py | sed -n '1,130p'
printf '\nCheck whether logger hierarchy enables sparc_service.api DEBUG under root INFO:\n'
python3 - <<'PY'
import logging
logging.basicConfig(level=logging.INFO)
logging.getLogger("LiteLLM").setLevel(logging.WARNING)
log = logging.getLogger("sparc_service.api")
print("root level:", logging.root.level)
print("sparc_service.api effective level:", log.getEffectiveLevel())
print("debug enabled:", log.isEnabledFor(logging.DEBUG))
PYRepository: rossoctl/cortex
Length of output: 8200
Make SPARC_LOG_REQUESTS debug output reachable.
authbridge/sparc-service/sparc_service/api.py calls log.debug(...) for request payloads, but authbridge/sparc-service/sparc_service/__main__.py configures the root logger at INFO and only enables sparc_service.llm_debug when SPARC_DEBUG_LLM is set. Support LOG_LEVEL, or enable sparc_service.api debug logs when SPARC_LOG_REQUESTS=true.
🤖 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 29 - 32, Update
the logging configuration in __main__.py so LOG_LEVEL controls the root logger
level, allowing DEBUG when configured, and ensure sparc_service.api debug
logging is enabled when SPARC_LOG_REQUESTS is true. Preserve the existing
SPARC_DEBUG_LLM-specific behavior and default INFO level.
| if _SKIP_TOOLS and request.tool_calls: | ||
| tool_name = request.tool_calls[0].get("function", {}).get("name", "") | ||
| if tool_name in _SKIP_TOOLS: | ||
| # DEBUG: per-call skip entry — visible only at DEBUG level | ||
| log.debug("reflect tool=%s skipped (SPARC_SKIP_TOOLS)", tool_name) | ||
| return ReflectResponse(decision="approve", issues=[], overall_avg_score=None, execution_time_ms=None) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not approve a mixed tool-call request from its first tool name.
Line 115 approves the whole tool_calls batch when only the first call matches _SKIP_TOOLS. A request can place an infrastructure tool first and an enforced tool later. Require every call to be an allowed infrastructure tool before the fast path. Reject or evaluate the batch otherwise. Validate _SKIP_TOOLS against a fixed infrastructure allowlist at startup.
As per coding guidelines, do not bypass authentication or policy for traffic that requires IBAC or token-exchange enforcement; listener.skip_hosts is reserved for identifiable infrastructure traffic because matched requests bypass plugins and session recording entirely.
🤖 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 113 - 118, The
skip fast path in the request handling logic must approve a batch only when
every tool call is in the fixed infrastructure allowlist and `_SKIP_TOOLS`;
otherwise continue normal evaluation or rejection. Add startup validation
ensuring `_SKIP_TOOLS` contains only allowlisted infrastructure tools,
preserving authentication, IBAC, token-exchange, plugin, and session-recording
enforcement for all other traffic.
Source: Coding guidelines
| # Extract tool name + args from the first tool call for correlation. | ||
| first_tc = request.tool_calls[0] if request.tool_calls else {} | ||
| fn = first_tc.get("function", {}) | ||
| if not fn: | ||
| log.warning("reflect: tool_calls[0] has no 'function' key; tool correlation unavailable. call=%s", first_tc) | ||
| tool_name = fn.get("name", "-") | ||
| raw_args = fn.get("arguments", "{}") | ||
| try: | ||
| tool_args = json.loads(raw_args) if isinstance(raw_args, str) else raw_args | ||
| except (json.JSONDecodeError, TypeError): | ||
| tool_args = raw_args | ||
| try: | ||
| args_str = json.dumps(tool_args, separators=(",", ":")) | ||
| except (TypeError, ValueError): | ||
| args_str = repr(tool_args) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Remove raw request data from service diagnostics.
The diagnostics log user-controlled tool arguments, complete request payloads, prompts, and provider results. These values can contain credentials or personal data. Log redacted, allowlisted metadata only.
authbridge/sparc-service/sparc_service/engine.py#L124-L138: do not logfirst_tcor serialized arguments without redaction.authbridge/sparc-service/sparc_service/engine.py#L151-L163: remove rawargsfrom INFO and DEBUG telemetry.authbridge/sparc-service/sparc_service/api.py#L102-L111: redact before the first request log event.authbridge/sparc-service/sparc_service/providers.py#L134-L151: redact prompt, schema, and result fields before diagnostic logging.
🧰 Tools
🪛 ast-grep (0.45.0)
[info] 135-135: use jsonify instead of json.dumps for JSON output
Context: json.dumps(tool_args, separators=(",", ":"))
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
📍 Affects 3 files
authbridge/sparc-service/sparc_service/engine.py#L124-L138(this comment)authbridge/sparc-service/sparc_service/engine.py#L151-L163authbridge/sparc-service/sparc_service/api.py#L102-L111authbridge/sparc-service/sparc_service/providers.py#L134-L151
🤖 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/engine.py` around lines 124 - 138,
Remove raw user-controlled data from diagnostics: in
authbridge/sparc-service/sparc_service/engine.py lines 124-138, stop logging
first_tc or unredacted serialized arguments; in lines 151-163, remove raw args
from INFO and DEBUG telemetry. In authbridge/sparc-service/sparc_service/api.py
lines 102-111, apply the approved redaction and allowlisted metadata before the
first request log event. In authbridge/sparc-service/sparc_service/providers.py
lines 134-151, redact prompt, schema, and result fields before diagnostic
logging, preserving only safe allowlisted metadata.
|
The empty-response investigation here is genuinely good work — the 1. Split the PRThree unrelated changes with very different risk profiles are bundled: the Dockerfile/Makefile fix is near-trivial, the WatsonX patches change LLM behavior for existing users, and 2.
|
Cherry-picked from vz-ibm/kagenti-extensions branch fix/sparc-watsonx-reasoning-patch (a real GitHub fork of this repo, just carrying its pre-rename display name), isolated to providers.py and the Makefile only — that branch as a whole is 400+ files behind current main (missing tlsbridge, contextguru, cpex, and other subsystems merged since it diverged), so checking it out directly would have regressed main rather than just adding the fix. Three patches to providers.py: - _patch_watsonx_for_reasoning_models: WatsonX + IBM LiteLLM proxy return reasoning_content, not content; without this all semantic SPARC calls fail with decision=error - _patch_empty_response_retry: retries on ValueError from empty responses (ISSUE-019), which the IBM LiteLLM proxy returns intermittently under load - _patch_debug_logging: optional SPARC_DEBUG_LLM=true logging Makefile: kind-load target falls back to `podman save | kind load image-archive` when `kind load docker-image` fails (always fails on Linux with rootful Podman). Signed-off-by: Vitaly Zabershinsky <VITALYZ@il.ibm.com>
…IP_TOOLS, logging) Continuing the cherry-pick from vz-ibm/kagenti-extensions fix/sparc-watsonx-reasoning-patch — the first commit only covered providers.py and the deploy Makefile; this one picks up the rest of what that branch touched under authbridge/sparc-service/: - Dockerfile: chown -R sparc:sparc /app before USER sparc — without this, the container crashes on startup with PermissionError since files copied as root are unreadable by the non-root user - api.py: adds SPARC_SKIP_TOOLS (auto-approve named tools without evaluation — required by Step 24 of the VPC guide, which sets this env var directly and would silently no-op without this patch), SPARC_LOG_REQUESTS, and SPARC_STRIP_TOOL_ARG_KEYS - engine.py: reflect log line now includes tool name/args/timestamp for correlation; splits INFO (clean) vs DEBUG (verbose) detail - settings.py, __main__.py: litellm.watsonx provider alias, INFO/DEBUG log level split, SPARC_DEBUG_LLM flag Deliberately NOT ported: the Go-side strip_tool_args patch to authbridge/authlib/plugins/sparc/ (collect.go, plugin.go) — that requires a separate authbridge-proxy image rebuild and isn't needed for the retail/airline benchmarks; the VPC guide's Step 19 already notes it's unusable without a custom AuthBridge build. Signed-off-by: Vitaly Zabershinsky <VITALYZ@il.ibm.com>
24b6405 to
3d82569
Compare
Reviewer OsherElhadad flagged three blocking issues (plus CodeQL alert rossoctl#179): 1. _patch_watsonx_for_reasoning_models was unconditional — it applied to every WatsonX deployment including the default mistral-large-2512, which supports response_format natively. That silently regressed structured output for existing users. Gate the patch behind a new SPARC_SCHEMA_IN_PROMPT=true setting (Settings.schema_in_prompt); the default path is now unchanged. 2. All three _patch_* helpers rebound methods on the shared ALTK class without a re-entry guard. ReflectionEngine caches components per track and lazily builds them, so a track switch would wrap the already-wrapped method — retry counts multiplied and debug lines duplicated on every subsequent call. Add _sparc_patched_reasoning / _sparc_patched_retry / _sparc_patched_debug sentinel attributes; each _patch_* no-ops if its sentinel is already set. 3. engine.reflect logged raw tool arguments at INFO. Args are caller-controlled — embedded newlines let a caller forge log lines (CodeQL alert rossoctl#179) — and can carry payloads (PII, payment ids, etc.). The same hunk had also dropped session_id/track from INFO. Restore session_id and track at INFO alongside tool/decision/score, and log args only at DEBUG when SPARC_LOG_REQUESTS=true. Also addresses smaller items from the same review: - tests/test_haiku_empty_response.py: replaced production PII in the conversation fixture (user id, names, DOBs, payment id, reservation ids) with synthetic values; gated both live-LLM probes behind pytest.skipif so they only run when RUN_HAIKU_TESTS=1 plus OAIKEY/OAIBASE are set (matches the reviewer's suggested opt-in path). - tests/test_providers.py: added three deterministic unit tests — retry wrapper recovers after two ValueError empty-response failures, retry wrapper re-raises unrelated ValueErrors, and all three patches are idempotent under repeat application. No credentials or network required; fills in the coverage gap the reviewer noted for the retry and schema-injection wrappers. Signed-off-by: Vitaly Zabershinsky <VITALYZ@il.ibm.com>
|
Thanks — appreciate the thorough read. Pushed 1. Split the PR — held off on splitting for now because the Dockerfile/chown answer is still open (see (7) below); happy to split into a Dockerfile-only PR once we agree on the fix there. 2. Unconditional watsonx patch (blocking) — fixed. Added 3. Global non-idempotent monkeypatching (blocking) — fixed with the sentinel approach ( 4. Drop 5. Raw args at INFO / CodeQL (blocking) — fixed. 6.
7. Smaller items — left the Dockerfile chown, Makefile Tests locally: |
4 participants
What this bundles
Three related sparc-service fixes ported from the old kagenti-sparc
codebase, needed to run SPARC on a rossoctl cluster:
1. WatsonX reasoning-model patches
_patch_watsonx_for_reasoning_models— injects the response schema viasystem prompt instead of
response_formatfor WatsonX reasoning modelsand the IBM LiteLLM proxy, which don't support
response_format._patch_empty_response_retry— retries onValueError: No content or tool calls found(an upstream ALTK bug not yet fixed there), withexponential backoff.
SPARC_DEBUG_LLM=true.2. Dockerfile chown fix
Without this the pod crashes on startup — found live while deploying to a
rossoctl cluster (non-root user can't access files owned by root in the
image layer).
3. SPARC_SKIP_TOOLS
SPARC_SKIP_TOOLS=<comma-separated tool names>— auto-approves the namedtools without SPARC evaluation. Needed for infrastructure tools
(
message,calculate,create_session, etc.) and READ-only domaintools that have no policy risk and would otherwise cause false-positive
rejects. Only WRITE tools should go through SPARC's actual reasoning.
Verification
All three were run live on a rossoctl Kind cluster: SPARC deployed and
serving
/reflectsuccessfully with a WatsonX-compatible LiteLLMbackend, pod stable across restarts, and
SPARC_SKIP_TOOLSconfirmed viastartup log (
the following tools will be auto-approved without evaluation: [...]) plus a full 50-task Tau2 airline benchmark runcompleting cleanly with the skip list active.
Summary by CodeRabbit
New Features
Bug Fixes
Tests