-
Notifications
You must be signed in to change notification settings - Fork 40
fix(sparc-service): WatsonX reasoning-model support + Dockerfile fix + SPARC_SKIP_TOOLS #739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,11 +4,18 @@ | |
| POST /reflect — run SPARC on a proposed tool call, return the verdict. | ||
| GET /healthz — liveness (always ok if the process is up). | ||
| GET /readyz — readiness (config valid and component buildable). | ||
|
|
||
| Log levels: | ||
| INFO — clean operational log: startup skip list + evaluated verdicts only. | ||
| DEBUG — adds per-call skip entries and full request payloads | ||
| (payloads only when SPARC_LOG_REQUESTS=true). | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import logging | ||
| import os | ||
|
|
||
| from fastapi import FastAPI, HTTPException | ||
| from fastapi.concurrency import run_in_threadpool | ||
|
|
@@ -19,6 +26,43 @@ | |
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| # 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"} | ||
|
Comment on lines
+29
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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
🤖 Prompt for AI Agents |
||
|
|
||
| # SPARC_STRIP_TOOL_ARG_KEYS — comma-separated keys to remove from every | ||
| # tool_calls[].function.arguments before SPARC evaluates the call. | ||
| # Example: SPARC_STRIP_TOOL_ARG_KEYS=session_id,request_id | ||
| _STRIP_KEYS: frozenset[str] = frozenset( | ||
| k.strip() for k in os.getenv("SPARC_STRIP_TOOL_ARG_KEYS", "").split(",") if k.strip() | ||
| ) | ||
|
|
||
| # SPARC_SKIP_TOOLS — comma-separated tool names to auto-approve without SPARC. | ||
| # Use for infrastructure tools (e.g. message, calculate) that have no policy | ||
| # risk and would cause false-positive rejects. | ||
| # Example: SPARC_SKIP_TOOLS=message,calculate | ||
| _SKIP_TOOLS: frozenset[str] = frozenset( | ||
| t.strip() for t in os.getenv("SPARC_SKIP_TOOLS", "").split(",") if t.strip() | ||
| ) | ||
|
|
||
|
|
||
| def _strip_tool_arg_keys(tool_calls: list[dict], keys: frozenset[str]) -> list[dict]: | ||
| """Return a copy of tool_calls with the named argument keys removed.""" | ||
| result = [] | ||
| for tc in tool_calls: | ||
| fn = tc.get("function", {}) | ||
| raw_args = fn.get("arguments", "") | ||
| try: | ||
| args = json.loads(raw_args) if isinstance(raw_args, str) else raw_args | ||
| if isinstance(args, dict): | ||
| args = {k: v for k, v in args.items() if k not in keys} | ||
| new_args = json.dumps(args) if isinstance(args, dict) else raw_args | ||
| except (json.JSONDecodeError, TypeError): | ||
| new_args = raw_args | ||
| result.append({**tc, "function": {**fn, "arguments": new_args}}) | ||
| return result | ||
|
|
||
|
|
||
| def create_app(engine: ReflectionEngine | None = None) -> FastAPI: | ||
| """Build the FastAPI app. Inject ``engine`` in tests; defaults to env config.""" | ||
|
|
@@ -33,6 +77,10 @@ | |
| app.state.engine = engine | ||
| app.state.settings = settings | ||
|
|
||
| # INFO: announce skip list once at startup so operators know what is bypassed | ||
| if _SKIP_TOOLS: | ||
| log.info("SPARC_SKIP_TOOLS: the following tools will be auto-approved without evaluation: %s", sorted(_SKIP_TOOLS)) | ||
|
|
||
| @app.get("/healthz") | ||
| def healthz() -> dict[str, object]: | ||
| return { | ||
|
|
@@ -51,6 +99,24 @@ | |
|
|
||
| @app.post("/reflect", response_model=ReflectResponse) | ||
| async def reflect(request: ReflectRequest) -> ReflectResponse: | ||
| # DEBUG: full request payload — only when SPARC_LOG_REQUESTS=true | ||
| if _LOG_REQUESTS: | ||
| log.debug("incoming reflect request: %s", request.model_dump_json()) | ||
|
|
||
| if _STRIP_KEYS and request.tool_calls: | ||
| request = request.model_copy( | ||
| update={"tool_calls": _strip_tool_arg_keys(request.tool_calls, _STRIP_KEYS)} | ||
| ) | ||
| if _LOG_REQUESTS: | ||
| log.debug("after strip (%s): tool_calls=%s", sorted(_STRIP_KEYS), request.tool_calls) | ||
|
|
||
| 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) | ||
|
Comment on lines
+113
to
+118
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win Do not approve a mixed tool-call request from its first tool name. Line 115 approves the whole As per coding guidelines, do not bypass authentication or policy for traffic that requires IBAC or token-exchange enforcement; 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| # SPARCReflectionComponent.process is synchronous (and CPU/IO bound on the | ||
| # LLM call); run it off the event loop so the service stays responsive. | ||
| try: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,9 +8,11 @@ | |
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import logging | ||
| import threading | ||
| from dataclasses import replace | ||
| from datetime import datetime, timezone | ||
| from typing import Any, Callable | ||
|
|
||
| from .models import ReflectionIssue, ReflectRequest, ReflectResponse | ||
|
|
@@ -118,13 +120,47 @@ def reflect(self, request: ReflectRequest) -> ReflectResponse: | |
| decision = _decision_str(reflection.decision) | ||
| score = _extract_overall_score(raw_pipeline) | ||
| execution_ms = getattr(output, "execution_time_ms", None) | ||
|
|
||
| # 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) | ||
|
Comment on lines
+124
to
+138
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 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.
🧰 Tools🪛 ast-grep (0.45.0)[info] 135-135: use jsonify instead of json.dumps for JSON output (use-jsonify) 📍 Affects 3 files
🤖 Prompt for AI Agents |
||
|
|
||
| ts = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") | ||
| score_str = f"{score:.2f}" if score is not None else "-" | ||
| ms_str = f"{execution_ms:.1f}" if isinstance(execution_ms, (int, float)) else "-" | ||
|
|
||
| def _tok(*keys: str) -> str: | ||
| for k in keys: | ||
| v = raw_pipeline.get(k) | ||
| if v is not None: | ||
| return str(v) | ||
| return "-" | ||
|
|
||
| log.info( | ||
| "reflect session=%s track=%s decision=%s score=%s ms=%s", | ||
| request.session_id or "-", | ||
| "reflect ts=%s tool=%s args=%s decision=%s score=%s ms=%s", | ||
| ts, tool_name, args_str, decision, score_str, ms_str, | ||
| ) | ||
| log.debug( | ||
| "reflect ts=%s tool=%s args=%s decision=%s score=%s ms=%s" | ||
| " track=%s session=%s tokens_in=%s tokens_out=%s messages=%s", | ||
| ts, tool_name, args_str, decision, score_str, ms_str, | ||
| track, | ||
| decision, | ||
| f"{score:.2f}" if score is not None else "-", | ||
| f"{execution_ms:.1f}" if isinstance(execution_ms, (int, float)) else "-", | ||
| request.session_id or "-", | ||
| _tok("tokens_in", "input_tokens"), | ||
| _tok("tokens_out", "output_tokens"), | ||
| len(request.messages), | ||
| ) | ||
|
|
||
| return ReflectResponse( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: rossoctl/cortex
Length of output: 195
🏁 Script executed:
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
kindv0.32.0, thekind load image-archivecommand processes each specified archive file individually [1][2]. When multiple archives are provided,kindopens 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 usingos.Openand streams it into the node via a pipe (or by streaming the reader) to thectrcommand (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]. Thekindimplementation 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 askind 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:
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:
🌐 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:
🏁 Script executed:
Repository: rossoctl/cortex
Length of output: 244
🏁 Script executed:
Repository: 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:
kind load image-archivefrom stdin kubernetes-sigs/kind#927Materialize 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
📝 Committable suggestion
🤖 Prompt for AI Agents