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 2 commits into
Conversation
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>
📝 WalkthroughWalkthroughThe SPARC service adds configurable request 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
Suggested reviewers: 🚥 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.
| client_cls.generate = patched_generate | ||
| client_cls.generate_async = patched_generate_async |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline authbridge/sparc-service/sparc_service/providers.py --items all
rg -n -C 4 '_patch_watsonx_for_reasoning_models|_patch_empty_response_retry|_patch_debug_logging|build_llm_client' \
authbridge/sparc-service/sparc_service/providers.py
rg -n -C 4 '_component_or_build|build_component|request\.track' \
authbridge/sparc-service/sparc_service/engine.pyRepository: rossoctl/cortex
Length of output: 6268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== providers.py relevant sections =="
sed -n '1,240p' authbridge/sparc-service/sparc_service/providers.py | cat -n
echo
echo "== engine.py relevant sections =="
sed -n '1,125p' authbridge/sparc-service/sparc_service/engine.py | cat -n
echo
echo "== focused behavioral probe for repeated patch chaining =="
python3 - <<'PY'
import asyncio
class FakeLLMClient:
calls = 0
async def generate_async(self, prompt):
FakeLLMClient.calls += 1
return "response"
def chain(fn, wrapper_name):
async def patched_generate_async(*a, **k):
print(f"wrapper:{wrapper_name}; calls={FakeLLMClient.calls}")
return await fn(*a, **k)
return patched_generate_async
client_cls = FakeLLMClient
FakeLLMClient.calls = 0
print("initial generate_async:", client_cls.generate_async)
client_cls.generate_async = chain(client_cls.generate_async, "schema")
print("after schema:", client_cls.generate_async)
FakeLLMClient.calls = 0
client_cls.generate_async = chain(client_cls.generate_async, "retry")
print("after retry:", client_cls.generate_async)
FakeLLMClient.calls = 0
client_cls.generate_async = chain(client_cls.generate_async, "retry")
print("after retry x2:", client_cls.generate_async)
client_cls.generate_async = chain(client_cls.generate_async, "retry")
print("after retry x3:", client_cls.generate_async)
# Simulate the patched generate_async body without actual retries/logging.
def simulate_wrapped_call(fn, max_retries, delay_factor):
FakeLLMClient.calls = 0
asyncio.get_event_loop().run_until_complete(fn({}))
return dict(calls=F_fakeLLMClient.calls)
print("chain_call_count_with_retry_count=", "not executable")
PYRepository: rossoctl/cortex
Length of output: 18738
Make client patch installation idempotent.
Each component build replaces methods on the shared client_cls. A second build for the same track wraps already wrapped methods again. If patch installation happens multiple times, the retry wrapper adds another outer retry layer and duplicates backoff plus debug logs. Store per-class patch markers, or use an instance-local wrapper or subclass.
authbridge/sparc-service/sparc_service/providers.py#L67-L68: install the schema wrapper once per client class.authbridge/sparc-service/sparc_service/providers.py#L112: install the retry wrapper once per client class.authbridge/sparc-service/sparc_service/providers.py#L154: install the debug wrapper once per client class.
📍 Affects 1 file
authbridge/sparc-service/sparc_service/providers.py#L67-L68(this comment)authbridge/sparc-service/sparc_service/providers.py#L112-L112authbridge/sparc-service/sparc_service/providers.py#L154-L154
🤖 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/providers.py` around lines 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.
| # Full production prompt extracted from the failing call in zeus logs (2026-07-28T13:14:37Z) | ||
| # This is the exact prompt that caused "No content or tool calls found in response" | ||
| # for tool=update_reservation_flights on the airline benchmark. | ||
| # The system message contains the full ALTK rubric + schema. | ||
| # The user message contains the full airline policy + multi-turn conversation history. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Remove production conversation data from the test fixture.
The fixture contains account identifiers, names, dates of birth, and payment identifiers from a production log. Replace it with a synthetic, minimized fixture before commit.
🤖 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/tests/test_haiku_empty_response.py` around lines 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.
| def test_response_format_intermittent_empty(n_calls: int = 10): | ||
| """Send the same call N times with response_format and count empty responses. | ||
|
|
||
| If the hypothesis is correct, at least some calls will return empty content, | ||
| proving the IBM proxy is intermittently broken in response_format mode. | ||
| """ | ||
| empty_count = 0 | ||
| results = [] | ||
|
|
||
| for i in range(n_calls): | ||
| result = _call_with_response_format() | ||
| results.append(result) | ||
| status = "EMPTY" if result["empty"] else "OK" | ||
| print(f" call {i+1:02d}: {status} content_len={len(result['content'] or '')}") | ||
| if result["empty"]: | ||
| empty_count += 1 | ||
|
|
||
| print(f"\nSummary: {empty_count}/{n_calls} calls returned empty content") | ||
|
|
||
| # The test proves the hypothesis if at least 1 call is empty. | ||
| # If 0 are empty, the IBM proxy may have been fixed or the prompt is too simple. | ||
| assert empty_count > 0, ( | ||
| f"All {n_calls} calls succeeded — IBM proxy may be stable now, " | ||
| f"or the prompt needs to be longer to trigger the failure." | ||
| ) | ||
|
|
||
|
|
||
| def test_system_prompt_mode_no_empty(n_calls: int = 10): | ||
| """Same N calls but with schema in system prompt instead of response_format. | ||
|
|
||
| If the fix works, zero calls should return empty content. | ||
| """ | ||
| empty_count = 0 | ||
|
|
||
| for i in range(n_calls): | ||
| result = _call_with_system_prompt() | ||
| status = "EMPTY" if result["empty"] else "OK" | ||
| print(f" call {i+1:02d}: {status} content_len={len(result['content'] or '')}") | ||
| if result["empty"]: | ||
| empty_count += 1 | ||
|
|
||
| print(f"\nSummary: {empty_count}/{n_calls} calls returned empty content") | ||
|
|
||
| assert empty_count == 0, ( | ||
| f"{empty_count}/{n_calls} calls returned empty content even with system-prompt mode" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'pytestmark|integration|OAIKEY|OAIBASE|test_haiku_empty_response|_patch_empty_response_retry' \
authbridge/sparc-serviceRepository: rossoctl/cortex
Length of output: 7183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test file outline/size =="
wc -l authbridge/sparc-service/tests/test_haiku_empty_response.py
sed -n '1,120p' authbridge/sparc-service/tests/test_haiku_empty_response.py
sed -n '120,290p' authbridge/sparc-service/tests/test_haiku_empty_response.py
echo "== conftest markers =="
fd -a 'conftest.py' authbridge | while read -r f; do
echo "--- $f"
sed -n '1,220p' "$f"
done
echo "== pytest defaults/run related =="
fd -a 'pytest.ini|setup.cfg|pyproject.toml|tox.ini|Makefile|uv.lock|poetry.lock|requirements*.txt' authbridge/sparc-service | while read -r f; do
echo "--- $f"
sed -n '1,180p' "$f"
done
echo "== static model of test markers and assertion patterns =="
python3 - <<'PY'
from pathlib import Path
import re
p = Path("authbridge/sparc-service/tests/test_haiku_empty_response.py")
text = p.read_text()
print("pytestmark:", bool(re.search(r'^pytestmark\s*=', text, re.M)))
print("contains OAIKEY:", "OAIKEY" in text)
print("contains OAIBASE:", "OAIBASE" in text)
print("contains assert empty_count > 0:", "assert empty_count > 0" in text)
print("contains assert empty_count == 0:", "assert empty_count == 0" in text)
print("contains skipif:", bool(re.search(r'skipif|skipif\(', text, re.M)))
PYRepository: rossoctl/cortex
Length of output: 28283
Move the Haiku empty-response probe out of the default pytest run.
test_response_format_intermittent_empty asserts at least one empty response, so it blocks the default test suite when the external proxy stabilizes. Both tests make ten live calls and require OAIKEY/OAIBASE. Add an opt-in marker like RUN_HAIKU_TESTS=1, skip without credentials or the marker, and keep covered behavior with deterministic unit tests that mock empty responses and the retry/schema-prompt wrappers.
🤖 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/tests/test_haiku_empty_response.py` around lines 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.
|
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.
|
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