epic: add SPARC pre-tool reflection plugin, service, and demo#478
Merged
huang195 merged 1 commit intoJun 3, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds SPARC pre-tool reflection: a Go plugin enforcing reflector verdicts, an HTTP SPARC reflector service (Python), service models/providers/settings, comprehensive tests, and a finance demo with agent, MCP server, k8s manifests, Makefile, and orchestration scripts. ChangesSPARC Pre-Tool Reflection Feature
Sequence Diagram(s)(Skipped — the change set is broad and contains many components; a focused sequence diagram would require selecting one interaction, omitted here.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
authbridge/docs/sparc-plugin.md (1)
1-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required “Gotchas and Known Issues” section to this doc.
This file is missing the required markdown section documenting common pitfalls (credential file race conditions,
ISSUERvsTOKEN_URL, and Keycloak port exclusion requirements). Please add it before merge.As per coding guidelines:
authbridge/**/*.mdfiles must include a gotchas/known-issues section explaining those exact pitfalls.🤖 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/docs/sparc-plugin.md` around lines 1 - 145, Add a new "Gotchas and Known Issues" section to the end of sparc-plugin.md (before merge) that documents the three required pitfalls: (1) credential file race conditions (explain race when multiple processes write/read credential files and recommend atomic writes/lock or unique temp files), (2) ISSUER vs TOKEN_URL confusion (clarify which config fields AuthBridge expects, note that ISSUER must not be used where TOKEN_URL is required and give guidance on resolving mismatches), and (3) Keycloak port exclusion requirements (state that Keycloak's advertised issuer URL must exclude the admin port or use proper host/port mapping and that mismatched ports break token validation). Ensure the section header is exactly "Gotchas and Known Issues" and that the three bullets are present and concise to satisfy authbridge/**/*.md guidelines.authbridge/demos/finance-sparc/scripts/show-verdicts.py (1)
1-27: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd type hints to comply with coding guidelines.
The file lacks type hints entirely. As per coding guidelines, Python code in
authbridge/**/*.pymust use Python 3.12+ syntax with type hints using modern union syntax (e.g.,str | None).📝 Proposed refactor to add type hints
#!/usr/bin/env python3 """Print the SPARC plugin verdicts from a session snapshot (read on stdin).""" +from __future__ import annotations import json import sys +from typing import Any try: data = json.load(sys.stdin) -except Exception: +except json.JSONDecodeError: print(" (no session events yet)") sys.exit(0) seen = False -for event in data.get("events", []): - for inv in (event.get("invocations") or {}).get("outbound", []): - if inv.get("plugin") == "sparc" and inv.get("action") in ("allow", "modify", "deny", "observe"): - det = inv.get("details", {}) +events: list[dict[str, Any]] = data.get("events", []) +for event in events: + invocations: dict[str, Any] = event.get("invocations") or {} + outbound: list[dict[str, Any]] = invocations.get("outbound", []) + for inv in outbound: + action: str | None = inv.get("action") + plugin: str | None = inv.get("plugin") + if plugin == "sparc" and action in ("allow", "modify", "deny", "observe"): + det: dict[str, Any] = inv.get("details", {}) seen = True - score = det.get("score") + score: float | str | None = det.get("score") score_s = f" score={score}" if score not in (None, "") else "" print( - " SPARC {}/{} tool={}{}".format( - inv.get("action"), inv.get("reason"), det.get("tool"), score_s - ) + f" SPARC {action}/{inv.get('reason')} tool={det.get('tool')}{score_s}" ) if not seen: print(" (no sparc verdicts found in session; check `make logs-sparc`)")As per coding guidelines: Python code must use Python 3.12+ syntax with type hints using modern union syntax.
🤖 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/demos/finance-sparc/scripts/show-verdicts.py` around lines 1 - 27, Wrap the script logic in a typed main() and add PEP 695-style annotations for all top-level variables and loop targets: create def main(stdin: typing.TextIO | None = None) -> None; call main(sys.stdin) from if __name__ == "__main__". Annotate parsed JSON as data: dict[str, typing.Any] = json.load(stdin or sys.stdin), seen: bool = False, and annotate loop targets as event: dict[str, typing.Any], inv: dict[str, typing.Any], det: dict[str, typing.Any]; annotate score as int | str | None and score_s as str. Import typing (or specific names) and use modern union syntax (e.g., int | None) everywhere to satisfy the coding guideline.
🟡 Minor comments (8)
authbridge/authlib/plugins/sparc/reflector.go-84-90 (1)
84-90:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
reflector_endpointbefore appending/reflect.
endpoint + "/reflect"turns a valid config likehttp://sparc:8000/intohttp://sparc:8000//reflect. Some routers normalize that, but others do not, so this becomes a fragile operator-facing config footgun.Suggested fix
func newHTTPReflector(endpoint, bearer string, timeout time.Duration) *httpReflector { + endpoint = strings.TrimRight(endpoint, "/") return &httpReflector{ url: endpoint + "/reflect", bearer: bearer, client: &http.Client{Timeout: timeout}, timeout: timeout, } }import "strings"🤖 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/authlib/plugins/sparc/reflector.go` around lines 84 - 90, newHTTPReflector builds the reflector URL by naively concatenating endpoint + "/reflect", which yields double slashes for endpoints that end with '/' (e.g., "http://.../"). Update newHTTPReflector to normalize the endpoint before appending by trimming any trailing '/' (use strings.TrimRight or strings.TrimSuffix on the endpoint) and then join with "/reflect" so the resulting httpReflector.url is always a single-canonical path; keep references to the httpReflector struct and its url field when making the change.authbridge/authlib/plugins/sparc/plugin.go-140-185 (1)
140-185:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
trackinConfigure().
ConfigSchema()advertises a fixed enum fortrack, butvalidate()never enforces it. A typo like"track":"fast-trak"will boot cleanly and only fail once the first reflection request hits the service.Suggested fix
switch c.Enforcement { case EnforcementMCP, EnforcementInference: default: return fmt.Errorf("enforcement must be mcp or inference, got %q", c.Enforcement) } + switch c.Track { + case "fast_track", "slow_track", "syntax", "spec_free", "transformations_only": + default: + return fmt.Errorf( + "track must be fast_track|slow_track|syntax|spec_free|transformations_only, got %q", + c.Track, + ) + } switch c.OnRejectAction { case OnRejectObserve, OnRejectReflect, OnRejectDeny: default: return fmt.Errorf("on_reject_action must be observe|reflect|deny, got %q", c.OnRejectAction) }🤖 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/authlib/plugins/sparc/plugin.go` around lines 140 - 185, The validate() method on sparcConfig is missing enforcement for the "track" enum advertised by ConfigSchema(), so typos like "fast-trak" silently pass; update sparcConfig.validate() to verify c.Track is one of the allowed track values (the same constants/enum names advertised by ConfigSchema or the Track* constants used elsewhere), returning a clear fmt.Errorf if it is not (e.g., "track must be one of [A B C], got %q"), so Configure()/validate() rejects invalid track values early; locate the check in the validate() function and mirror the exact allowed values used by ConfigSchema()/track-related constants.authbridge/sparc-service/README.md-98-123 (1)
98-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the required
Gotchas and known issuessection.This new AuthBridge README doesn't include the repo-mandated gotchas/known-issues section. Even if some standard pitfalls don't apply directly to this service, the section still needs to exist and call out the service-specific ones.
As per coding guidelines,
authbridge/**/*.md: Documentation files must include gotchas and known issues section explaining common pitfalls like credential file race conditions, ISSUER vs TOKEN_URL differences, and Keycloak port exclusion requirements.🤖 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/README.md` around lines 98 - 123, Add a "Gotchas and known issues" section to authbridge/sparc-service/README.md (header: "Gotchas and known issues") and list service-specific pitfalls: note credential file race conditions during startup, clarify ISSUER vs TOKEN_URL differences for OAuth flows, and call out Keycloak port exclusion requirements (e.g., omit port in issuer URL) plus any SPARC-specific quirks (local Ollama vs watsonx env vars like SPARC_LLM_PROVIDER, WX_API_KEY, WX_PROJECT_ID). Keep it brief bullet-style, placed near the end of the README after the Run/Build sections so reviewers can find and update it easily.authbridge/sparc-service/sparc_service/settings.py-53-60 (1)
53-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't silently ignore malformed numeric env vars.
If
SPARC_LLM_TIMEOUT,SPARC_RETRIES,SPARC_MAX_PARALLEL, orPORTis set to a non-integer value, the service quietly falls back to the default. That makes bad deployments look healthy while running with the wrong settings. SinceSettingsalready carrieserrors, these parse failures should be surfaced there instead of being masked.Also applies to: 155-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@authbridge/sparc-service/sparc_service/settings.py` around lines 53 - 60, The helper _int_env currently swallows malformed integers; change it to report parse failures to Settings.errors instead of silently returning defaults — update _int_env to accept an errors collection (or a callback) and the setting name, and on ValueError append a clear error message including the env var name and offending value to Settings.errors before returning the default; apply the same pattern to the similar parsing logic referenced around lines 155-174 so all uses (e.g., SPARC_LLM_TIMEOUT, SPARC_RETRIES, SPARC_MAX_PARALLEL, PORT) surface parse errors into Settings.errors rather than being masked.authbridge/sparc-service/pyproject.toml-5-18 (1)
5-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRaise the published Python floor to 3.12.
Line 10 currently advertises support for Python 3.10+, but this service is in
authbridge/**and the image already targets Python 3.12. Leaving the lower bound here allows unsupported runtimes to install the package.Suggested fix
-requires-python = ">=3.10" +requires-python = ">=3.12"As per coding guidelines,
authbridge/**/*.py: Python code must use Python 3.12+ syntax with type hints using modern union syntax.🤖 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/pyproject.toml` around lines 5 - 18, The pyproject metadata for the project "kagenti-sparc-service" currently allows Python >=3.10; update the requires-python field in the [project] table from ">=3.10" to ">=3.12" to match the image/runtime policy and ensure package installers won't install on unsupported runtimes, and scan any authbridge/**/*.py type hints to use Python 3.12+ syntax (e.g., modern union type hints) if needed.authbridge/sparc-service/tests/test_engine.py-92-97 (1)
92-97:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis assertion is a no-op and won't catch request-mapping regressions.
assert component is None or Truealways passes, so the test never verifies thatmessages,tool_specs, ortool_callsactually reached the fake component. Assert againstcomponent.calls[0]instead.Make the test prove the mapping
def test_run_input_receives_request_fields(): engine = make_engine(make_reflection_output("approve")) - engine.reflect(_request({"transaction_id": "TX4827"})) - # The fake records the run_input it was handed. - component = engine._component # noqa: SLF001 - test introspection - assert component is None or True # component built lazily inside reflect + request = _request({"transaction_id": "TX4827"}) + engine.reflect(request) + + component = engine._component # noqa: SLF001 - test introspection + assert component is not None + run_input = component.calls[0] + assert run_input.messages == request.messages + assert run_input.tool_specs == request.tool_specs + assert run_input.tool_calls == request.tool_calls🤖 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_engine.py` around lines 92 - 97, The test test_run_input_receives_request_fields currently contains a no-op assertion; replace the tautological assert with real checks that the fake component received the mapped run input after calling engine.reflect(_request(...)). Inspect engine._component (built lazily) and assert against component.calls[0] to verify the expected fields — e.g., that the recorded call includes messages, tool_specs, and tool_calls (and that the transaction_id from _request appears where expected). Keep using engine.reflect and component.calls[0] to locate and validate the run input rather than the current component is None or True check.authbridge/sparc-service/tests/test_watsonx_integration.py-21-24 (1)
21-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip this opt-in suite unless the watsonx project id is present too.
The gate only checks for an API key today, but
Settings.from_env()also requiresWX_PROJECT_ID/WATSONX_PROJECT_IDfor the watsonx provider. With just the key set, these tests run and fail during setup instead of skipping cleanly.Suggested skip gate
pytestmark = pytest.mark.skipif( - os.getenv("RUN_WATSONX_TESTS") != "1" or not (os.getenv("WX_API_KEY") or os.getenv("WATSONX_API_KEY")), + os.getenv("RUN_WATSONX_TESTS") != "1" + or not (os.getenv("WX_API_KEY") or os.getenv("WATSONX_API_KEY")) + or not (os.getenv("WX_PROJECT_ID") or os.getenv("WATSONX_PROJECT_ID")), reason="set RUN_WATSONX_TESTS=1 and WX_* creds to run watsonx integration tests", )🤖 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_watsonx_integration.py` around lines 21 - 24, The test skip gate (pytestmark) currently checks only for RUN_WATSONX_TESTS and an API key; update it to also require a watsonx project id so tests skip cleanly if the project id is missing. Change the condition used to set pytestmark (the variable at top of test_watsonx_integration.py) to require that either WX_PROJECT_ID or WATSONX_PROJECT_ID is present in the environment in addition to WX_API_KEY/WATSONX_API_KEY and RUN_WATSONX_TESTS, matching the requirements of Settings.from_env(); ensure the skip reason message is updated accordingly.authbridge/demos/finance-sparc/Makefile-76-82 (1)
76-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreflight should verify the selected container runtime.
CONTAINER_RUNTIMEfalls back todockereven when neitherdockernorpodmanis installed, andpreflightnever checks it. That letsmake demopass the prerequisite phase and fail much later in image build/load.Suggested fix
- `@for` t in kubectl kind python3 uv; do command -v $$t >/dev/null 2>&1 || { echo "ERROR: $$t not installed"; exit 1; }; done + `@for` t in kubectl kind python3 uv $(CONTAINER_RUNTIME); do command -v $$t >/dev/null 2>&1 || { echo "ERROR: $$t not installed"; exit 1; }; done🤖 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/demos/finance-sparc/Makefile` around lines 76 - 82, The preflight target in the Makefile does not validate the chosen container runtime (CONTAINER_RUNTIME) and allows a default "docker" even when it's not installed; update preflight to detect and validate the runtime by (1) resolving CONTAINER_RUNTIME to an available binary (prefer podman then docker) instead of unconditionally defaulting to docker, (2) adding a check in the preflight recipe that runs command -v $(CONTAINER_RUNTIME) >/dev/null 2>&1 and fails with a clear error if missing, and (3) ensure related targets that call CONTAINER_RUNTIME (image build/load logic) rely on this validated variable so make demo fails early if neither podman nor docker is present.
🧹 Nitpick comments (4)
authbridge/demos/finance-sparc/scripts/setup_keycloak_finance.py (1)
49-62: ⚡ Quick winConsider catching more specific exceptions for idempotency checks.
The blind
Exceptioncatches (lines 56, 61, 76, 88) work for idempotency but may mask real errors beyond "already exists" conditions. Consider catchingpython-keycloak-specific exceptions or checking error messages to distinguish between expected idempotency cases and genuine failures.♻️ Example improvement for mapper creation
+ from keycloak.exceptions import KeycloakPostError + try: kc.add_mapper_to_client_scope(scope_id, { "name": scope_name + "-aud-mapper", "protocol": "openid-connect", "protocolMapper": "oidc-audience-mapper", "config": {"included.custom.audience": agent_spiffe, "id.token.claim": "false", "access.token.claim": "true"}, }) - except Exception as e: - print(f" (mapper exists or: {e})") + except KeycloakPostError as e: + if "already exists" in str(e).lower(): + print(f" (mapper already exists)") + else: + print(f" warning: mapper creation failed: {e}")Similar improvements can be applied to other try/except blocks for more precise error handling.
🤖 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/demos/finance-sparc/scripts/setup_keycloak_finance.py` around lines 49 - 62, Replace broad except Exception blocks around kc.add_mapper_to_client_scope and kc.add_default_default_client_scope with targeted handling: import and catch python-keycloak errors (e.g., KeycloakPostError / KeycloakGetError or the library's KeycloakError) and check the error status/code or message for the idempotent "already exists"/409 condition, log the friendly "mapper exists" or "realm default exists" message only for that case, and re-raise or log as an actual error for any other codes/messages so real failures are not swallowed.authbridge/demos/finance-sparc/finance-agent/main.go (1)
20-34: ⚡ Quick winUse
log/slogin this demo binary.This file still uses
log.Printf/log.Fatalfthroughout, which diverges from the AuthBridge logging convention used for Go code underauthbridge/**. Please switch to a scopedslog.Loggerhere so the demo stays consistent with the rest of the module. As per coding guidelines,authbridge/**/*.go: Use Go 1.24 and log withlog/slog; binaries should log under their own name (authbridge-proxy, authbridge-envoy, authbridge-lite).🤖 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/demos/finance-sparc/finance-agent/main.go` around lines 20 - 34, Replace uses of the std log package (log.Printf, log.Fatalf, etc.) in this demo binary with a scoped slog.Logger for the binary (e.g., create logger := slog.New(...) at program start in main) and use that logger throughout; remove the "log" import and add "log/slog" to the imports list, initialize a logger with a clear program name (authbridge-finance-agent or similar) in main, then replace log.Printf/log.Fatalf calls with logger.Warn/Info/Error/Crit (or logger.Error with logger.Log + Level when needed) in all handlers and helper functions referenced from main so logging is consistent with authbridge conventions. Ensure every call site that used log.* now uses the scoped slog logger instance.authbridge/demos/finance-sparc/README.md (1)
26-32: ⚡ Quick winSpecify language for fenced code block.
The fenced code block should include a language identifier for proper syntax highlighting and better readability.
📝 Proposed fix
Example run (watsonx reflection): -``` +```console Turn 1 agent> This tool call was not executed because it could not be verified... ask for the exact transaction_id Turn 2 agent> Your transaction with ID TX4827 has been successfully refunded...🤖 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/demos/finance-sparc/README.md` around lines 26 - 32, The fenced code block in README.md lacks a language identifier; update the triple-backtick fence surrounding the transaction dialogue so it includes a language tag (e.g., use ```console) to enable proper syntax highlighting and readability for the block that contains the lines starting with "Turn 1 agent>" and "Turn 2 agent>" (the example block shown under the SPARC lines).authbridge/demos/finance-sparc/scripts/show-verdicts.py (1)
6-10: ⚡ Quick winNarrow the exception handling.
The bare
except Exception:is overly broad. Catchingjson.JSONDecodeErrorspecifically would be more precise and maintainable.♻️ Proposed refinement
try: data = json.load(sys.stdin) -except Exception: +except json.JSONDecodeError: print(" (no session events yet)") sys.exit(0)🤖 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/demos/finance-sparc/scripts/show-verdicts.py` around lines 6 - 10, The broad except in the try/except around json.load(sys.stdin) should be narrowed: replace the bare except Exception in the block that wraps the call to json.load with an except json.JSONDecodeError to only catch JSON parsing failures (keep the existing print(" (no session events yet)") and sys.exit(0) behavior); ensure you import json.JSONDecodeError or reference it as json.JSONDecodeError so the handler is specific to parsing errors.
🤖 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/authlib/plugins/sparc/plugin.go`:
- Around line 287-292: correlateInferenceContext returning ok==false should not
unconditionally call cont() and bypass SPARC; update the branch that handles !ok
to check the deployment/mode and fail policy (e.g., MCP mode / fail_policy ==
"closed") and, in those cases, reject or fail the request (use
pctx.Fail("missing_inference_context") or equivalent) instead of returning
cont(); only call pctx.Skip("no_inference_context") and return cont() when
running in a mode that explicitly allows open/fail-open behavior. Ensure this
logic is applied in the block around correlateInferenceContext(pctx),
pctx.Skip(...) and cont() so misconfigured MCP deployments stay protected.
In `@authbridge/demos/finance-sparc/finance-agent/main.go`:
- Around line 137-158: The current discoverTools uses toolsOnce.Do so a
transient mcpCall("tools/list", ...) failure makes discovery permanently fail;
change the logic so discovery is retried until it succeeds (or until a bounded
retry/backoff) and only mark completion after a successful response. Concretely,
remove or avoid using toolsOnce.Do for the initial mcpCall, perform the mcpCall
with a short retry/backoff loop (or check and retry when toolsCache is empty),
populate toolsCache and only then set the “done” state (e.g., set
toolsOnce/flag) so that discoverTools, mcpCall, toolsCache and toolsOnce behave
resiliently to transient MCP errors.
- Around line 205-209: The handler currently calls http.Post which bypasses the
custom 240s timeout client created by buildProxiedClient; replace the http.Post
call with the configured client (the variable returned by buildProxiedClient,
e.g., client or proxiedClient) and perform the request via client.Post or
client.Do with a new http.Request built from reqBody so the request uses the
proxied client's timeout and settings (refer to symbols buildProxiedClient,
ollamaURL, chatReq).
In `@authbridge/demos/finance-sparc/finance-mcp/main.go`:
- Line 25: Replace the import "log" with "log/slog" and update every use of the
std log package to slog: change log.Print/log.Println/log.Printf to slog.Info
(use fmt.Sprintf(...) for formatted messages) and change
log.Fatal/log.Fatalf/log.Fatalln to slog.Error followed by os.Exit(1) (or use
slog.Handler that exits if preferred); ensure errors are logged with slog.Error
and include key/value pairs or formatted message via fmt.Sprintf where
appropriate; update all occurrences noted (the current log usage points) and add
import for "fmt" and "os" if you use fmt.Sprintf or os.Exit.
In `@authbridge/demos/finance-sparc/k8s/ollama.yaml`:
- Around line 24-57: The pod spec for the container named "ollama" is missing
hardened securityContext settings; update the Pod/Container securityContext for
the ollama container (spec.containers -> name: ollama) to enforce non-root
execution and drop privileges: set runAsNonRoot: true and a non-root runAsUser
(e.g., 1000) at the pod or container level, add allowPrivilegeEscalation: false
under the container.securityContext, add securityContext.capabilities.drop:
["ALL"] for the container, and consider setting readOnlyRootFilesystem: true and
restrictive fsGroup/supplementalGroups as used by other demo workloads so the
manifest conforms to the project's hardened template.
In `@authbridge/demos/finance-sparc/k8s/sparc-service.yaml`:
- Around line 25-57: Add the same non-root pod/container security hardening used
by finance-agent/finance-mcp to this Deployment: set pod-level securityContext
with runAsNonRoot: true and fsGroup (match other demos), and add container-level
securityContext on the sparc-service container (name: sparc-service) with
runAsUser, allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, and
capabilities.drop: ["ALL"] plus an appropriate seccompProfile type; ensure these
keys sit alongside the existing envFrom/readinessProbe/livenessProbe/resources
so the manifest mirrors the demo templates in authbridge/demos/*/k8s.
In `@authbridge/demos/finance-sparc/Makefile`:
- Around line 23-31: The Makefile exposes NS, AGENT, and PLATFORM_NS but the
manifests hardcode the resource names (team1, finance-agent, kagenti-system), so
overrides in the Makefile never affect created resources; update the manifests
to read these values from the Makefile/environment (e.g., replace hardcoded
namespace and agent name with templated vars and apply with envsubst or
kustomize vars) or alternatively remove/lock the override knobs in the Makefile
so defaults match the hardcoded values; ensure the symbols NS, AGENT, and
PLATFORM_NS are the single source of truth and used by the deployment manifests
that create the namespace, agent, and platform resources.
In `@authbridge/sparc-service/sparc_service/api.py`:
- Line 11: Replace all uses of Optional[...] in this module with the modern
union syntax using ReflectionEngine | None; remove the now-unnecessary "from
typing import Optional" import and update any function signatures or variable
annotations that reference Optional[ReflectionEngine] (and the occurrence
referenced at 21-21) to use ReflectionEngine | None instead, and ensure
ReflectionEngine is imported or visible in the module.
- Around line 54-57: Replace the direct exposure of backend exception text in
the reflect error handler: inside the try/except around
run_in_threadpool(engine.reflect, request) catch the Exception and log the full
exception (including stacktrace) to the server log (use the existing logger or
raise to a logging facility) but raise HTTPException(status_code=502) with a
stable, non-sensitive detail like {"error": "upstream reflection failed"} (or
similar) instead of str(exc); keep the original exception as the exception cause
(from exc) if desired for traceback chaining.
In `@authbridge/sparc-service/sparc_service/engine.py`:
- Line 11: Replace all uses of Optional[...] in this module with modern union
syntax (e.g., Type | None) and remove Optional from the typing import; update
the top import line (from typing import Any, Callable, Optional) to no longer
import Optional (keeping Any, Callable or adding other needed names).
Specifically change every annotation that currently uses Optional[...] in
sparc_service/engine.py to use X | None (covering the occurrences referenced in
the review), and run a type-check to ensure signatures (functions/methods and
any variables) still type-check with the new syntax.
- Around line 82-86: The code is silently dropping request.track and
request.session_id when constructing SPARCReflectionRunInput; either wire
request.track into the component configuration (set on SPARCReflectionComponent
before calling reflector.process(...)) and propagate request.session_id through
the tracing/logging context around the reflection call, or remove these fields
from the public request model/docs so callers aren’t misled. Specifically,
update the code paths that build SPARCReflectionRunInput and invoke
reflector.process to: 1) set the SPARCReflectionComponent.track (or equivalent
component config) from request.track prior to reflector.process, and 2) push
request.session_id into the current tracing/logging context (correlation id) so
reflector.process and downstream logs include it; if you choose removal, delete
track/session_id from the public ReflectRequest model and docs instead of
ignoring them.
In `@authbridge/sparc-service/sparc_service/models.py`:
- Line 10: Replace all uses of Optional[...] with the Python 3.12 union syntax
(T | None) in this module: remove Optional from the import line (keep Any) and
update every annotation that currently uses Optional (e.g., fields or function
args/returns referenced in this file that use Optional) to use the pipe union
form (for example change Optional[str] to str | None); ensure imports reflect
the change (drop Optional) and run a quick type-check to confirm no remaining
Optional usages remain.
In `@authbridge/sparc-service/sparc_service/providers.py`:
- Around line 44-78: The registry override isn't honored because
get_llm(settings.registry_id) may return any client class but the code still
picks constructor args from settings.provider; fix by short-circuiting when an
explicit registry id is provided: if settings.registry_id is set, build generic
lite_kwargs from settings.llm_kwargs (set default timeout and apply
openai_base_url logic) and return client_cls(model_name=settings.model,
**lite_kwargs) immediately, skipping the provider-specific watsonx/ollama
branches (alternatively, instead of bypassing, validate compatibility between
settings.registry_id and settings.provider and raise a clear error); update code
references: get_llm, client_cls, settings.registry_id, settings.provider,
lite_kwargs.
In `@authbridge/sparc-service/tests/conftest.py`:
- Line 11: The test helpers in conftest.py import and use typing.Optional which
must be updated to Python 3.12 union syntax; remove Optional from the import and
update all type hints that use Optional[...] to the modern form (e.g., X | None)
— specifically update the import line that currently imports Any, Optional and
every fixture/type hint around the earlier helpers (lines referenced near 20-21
and 57) to use Any and use | None in function signatures/variable annotations
instead of Optional; ensure no Optional[...] usages remain in conftest.py.
---
Outside diff comments:
In `@authbridge/demos/finance-sparc/scripts/show-verdicts.py`:
- Around line 1-27: Wrap the script logic in a typed main() and add PEP
695-style annotations for all top-level variables and loop targets: create def
main(stdin: typing.TextIO | None = None) -> None; call main(sys.stdin) from if
__name__ == "__main__". Annotate parsed JSON as data: dict[str, typing.Any] =
json.load(stdin or sys.stdin), seen: bool = False, and annotate loop targets as
event: dict[str, typing.Any], inv: dict[str, typing.Any], det: dict[str,
typing.Any]; annotate score as int | str | None and score_s as str. Import
typing (or specific names) and use modern union syntax (e.g., int | None)
everywhere to satisfy the coding guideline.
In `@authbridge/docs/sparc-plugin.md`:
- Around line 1-145: Add a new "Gotchas and Known Issues" section to the end of
sparc-plugin.md (before merge) that documents the three required pitfalls: (1)
credential file race conditions (explain race when multiple processes write/read
credential files and recommend atomic writes/lock or unique temp files), (2)
ISSUER vs TOKEN_URL confusion (clarify which config fields AuthBridge expects,
note that ISSUER must not be used where TOKEN_URL is required and give guidance
on resolving mismatches), and (3) Keycloak port exclusion requirements (state
that Keycloak's advertised issuer URL must exclude the admin port or use proper
host/port mapping and that mismatched ports break token validation). Ensure the
section header is exactly "Gotchas and Known Issues" and that the three bullets
are present and concise to satisfy authbridge/**/*.md guidelines.
---
Minor comments:
In `@authbridge/authlib/plugins/sparc/plugin.go`:
- Around line 140-185: The validate() method on sparcConfig is missing
enforcement for the "track" enum advertised by ConfigSchema(), so typos like
"fast-trak" silently pass; update sparcConfig.validate() to verify c.Track is
one of the allowed track values (the same constants/enum names advertised by
ConfigSchema or the Track* constants used elsewhere), returning a clear
fmt.Errorf if it is not (e.g., "track must be one of [A B C], got %q"), so
Configure()/validate() rejects invalid track values early; locate the check in
the validate() function and mirror the exact allowed values used by
ConfigSchema()/track-related constants.
In `@authbridge/authlib/plugins/sparc/reflector.go`:
- Around line 84-90: newHTTPReflector builds the reflector URL by naively
concatenating endpoint + "/reflect", which yields double slashes for endpoints
that end with '/' (e.g., "http://.../"). Update newHTTPReflector to normalize
the endpoint before appending by trimming any trailing '/' (use
strings.TrimRight or strings.TrimSuffix on the endpoint) and then join with
"/reflect" so the resulting httpReflector.url is always a single-canonical path;
keep references to the httpReflector struct and its url field when making the
change.
In `@authbridge/demos/finance-sparc/Makefile`:
- Around line 76-82: The preflight target in the Makefile does not validate the
chosen container runtime (CONTAINER_RUNTIME) and allows a default "docker" even
when it's not installed; update preflight to detect and validate the runtime by
(1) resolving CONTAINER_RUNTIME to an available binary (prefer podman then
docker) instead of unconditionally defaulting to docker, (2) adding a check in
the preflight recipe that runs command -v $(CONTAINER_RUNTIME) >/dev/null 2>&1
and fails with a clear error if missing, and (3) ensure related targets that
call CONTAINER_RUNTIME (image build/load logic) rely on this validated variable
so make demo fails early if neither podman nor docker is present.
In `@authbridge/sparc-service/pyproject.toml`:
- Around line 5-18: The pyproject metadata for the project
"kagenti-sparc-service" currently allows Python >=3.10; update the
requires-python field in the [project] table from ">=3.10" to ">=3.12" to match
the image/runtime policy and ensure package installers won't install on
unsupported runtimes, and scan any authbridge/**/*.py type hints to use Python
3.12+ syntax (e.g., modern union type hints) if needed.
In `@authbridge/sparc-service/README.md`:
- Around line 98-123: Add a "Gotchas and known issues" section to
authbridge/sparc-service/README.md (header: "Gotchas and known issues") and list
service-specific pitfalls: note credential file race conditions during startup,
clarify ISSUER vs TOKEN_URL differences for OAuth flows, and call out Keycloak
port exclusion requirements (e.g., omit port in issuer URL) plus any
SPARC-specific quirks (local Ollama vs watsonx env vars like SPARC_LLM_PROVIDER,
WX_API_KEY, WX_PROJECT_ID). Keep it brief bullet-style, placed near the end of
the README after the Run/Build sections so reviewers can find and update it
easily.
In `@authbridge/sparc-service/sparc_service/settings.py`:
- Around line 53-60: The helper _int_env currently swallows malformed integers;
change it to report parse failures to Settings.errors instead of silently
returning defaults — update _int_env to accept an errors collection (or a
callback) and the setting name, and on ValueError append a clear error message
including the env var name and offending value to Settings.errors before
returning the default; apply the same pattern to the similar parsing logic
referenced around lines 155-174 so all uses (e.g., SPARC_LLM_TIMEOUT,
SPARC_RETRIES, SPARC_MAX_PARALLEL, PORT) surface parse errors into
Settings.errors rather than being masked.
In `@authbridge/sparc-service/tests/test_engine.py`:
- Around line 92-97: The test test_run_input_receives_request_fields currently
contains a no-op assertion; replace the tautological assert with real checks
that the fake component received the mapped run input after calling
engine.reflect(_request(...)). Inspect engine._component (built lazily) and
assert against component.calls[0] to verify the expected fields — e.g., that the
recorded call includes messages, tool_specs, and tool_calls (and that the
transaction_id from _request appears where expected). Keep using engine.reflect
and component.calls[0] to locate and validate the run input rather than the
current component is None or True check.
In `@authbridge/sparc-service/tests/test_watsonx_integration.py`:
- Around line 21-24: The test skip gate (pytestmark) currently checks only for
RUN_WATSONX_TESTS and an API key; update it to also require a watsonx project id
so tests skip cleanly if the project id is missing. Change the condition used to
set pytestmark (the variable at top of test_watsonx_integration.py) to require
that either WX_PROJECT_ID or WATSONX_PROJECT_ID is present in the environment in
addition to WX_API_KEY/WATSONX_API_KEY and RUN_WATSONX_TESTS, matching the
requirements of Settings.from_env(); ensure the skip reason message is updated
accordingly.
---
Nitpick comments:
In `@authbridge/demos/finance-sparc/finance-agent/main.go`:
- Around line 20-34: Replace uses of the std log package (log.Printf,
log.Fatalf, etc.) in this demo binary with a scoped slog.Logger for the binary
(e.g., create logger := slog.New(...) at program start in main) and use that
logger throughout; remove the "log" import and add "log/slog" to the imports
list, initialize a logger with a clear program name (authbridge-finance-agent or
similar) in main, then replace log.Printf/log.Fatalf calls with
logger.Warn/Info/Error/Crit (or logger.Error with logger.Log + Level when
needed) in all handlers and helper functions referenced from main so logging is
consistent with authbridge conventions. Ensure every call site that used log.*
now uses the scoped slog logger instance.
In `@authbridge/demos/finance-sparc/README.md`:
- Around line 26-32: The fenced code block in README.md lacks a language
identifier; update the triple-backtick fence surrounding the transaction
dialogue so it includes a language tag (e.g., use ```console) to enable proper
syntax highlighting and readability for the block that contains the lines
starting with "Turn 1 agent>" and "Turn 2 agent>" (the example block shown
under the SPARC lines).
In `@authbridge/demos/finance-sparc/scripts/setup_keycloak_finance.py`:
- Around line 49-62: Replace broad except Exception blocks around
kc.add_mapper_to_client_scope and kc.add_default_default_client_scope with
targeted handling: import and catch python-keycloak errors (e.g.,
KeycloakPostError / KeycloakGetError or the library's KeycloakError) and check
the error status/code or message for the idempotent "already exists"/409
condition, log the friendly "mapper exists" or "realm default exists" message
only for that case, and re-raise or log as an actual error for any other
codes/messages so real failures are not swallowed.
In `@authbridge/demos/finance-sparc/scripts/show-verdicts.py`:
- Around line 6-10: The broad except in the try/except around
json.load(sys.stdin) should be narrowed: replace the bare except Exception in
the block that wraps the call to json.load with an except json.JSONDecodeError
to only catch JSON parsing failures (keep the existing print(" (no session
events yet)") and sys.exit(0) behavior); ensure you import json.JSONDecodeError
or reference it as json.JSONDecodeError so the handler is specific to parsing
errors.
🪄 Autofix (Beta)
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: eb4f2048-b53b-4059-8cab-94e490785466
📒 Files selected for processing (48)
authbridge/CLAUDE.mdauthbridge/authlib/plugins/sparc/collect.goauthbridge/authlib/plugins/sparc/plugin.goauthbridge/authlib/plugins/sparc/plugin_test.goauthbridge/authlib/plugins/sparc/reflector.goauthbridge/authlib/plugins/sparc/reflector_test.goauthbridge/authlib/plugins/sparc/respond.goauthbridge/cmd/authbridge-envoy/main.goauthbridge/cmd/authbridge-proxy/main.goauthbridge/demos/README.mdauthbridge/demos/finance-sparc/.gitignoreauthbridge/demos/finance-sparc/Makefileauthbridge/demos/finance-sparc/README.mdauthbridge/demos/finance-sparc/finance-agent/Dockerfileauthbridge/demos/finance-sparc/finance-agent/main.goauthbridge/demos/finance-sparc/finance-mcp/Dockerfileauthbridge/demos/finance-sparc/finance-mcp/main.goauthbridge/demos/finance-sparc/go.modauthbridge/demos/finance-sparc/k8s/agent.yamlauthbridge/demos/finance-sparc/k8s/finance-mcp.yamlauthbridge/demos/finance-sparc/k8s/ollama.yamlauthbridge/demos/finance-sparc/k8s/sparc-patch.yamlauthbridge/demos/finance-sparc/k8s/sparc-service.yamlauthbridge/demos/finance-sparc/scripts/drive-demo.shauthbridge/demos/finance-sparc/scripts/host-setup.shauthbridge/demos/finance-sparc/scripts/launch-abctl.shauthbridge/demos/finance-sparc/scripts/patch-sparc-config.shauthbridge/demos/finance-sparc/scripts/pipeline-merge.pyauthbridge/demos/finance-sparc/scripts/setup_keycloak_finance.pyauthbridge/demos/finance-sparc/scripts/show-verdicts.pyauthbridge/demos/finance-sparc/scripts/stage-ollama-model.shauthbridge/docs/sparc-plugin.mdauthbridge/sparc-service/.dockerignoreauthbridge/sparc-service/Dockerfileauthbridge/sparc-service/README.mdauthbridge/sparc-service/pyproject.tomlauthbridge/sparc-service/sparc_service/__init__.pyauthbridge/sparc-service/sparc_service/__main__.pyauthbridge/sparc-service/sparc_service/api.pyauthbridge/sparc-service/sparc_service/engine.pyauthbridge/sparc-service/sparc_service/models.pyauthbridge/sparc-service/sparc_service/providers.pyauthbridge/sparc-service/sparc_service/settings.pyauthbridge/sparc-service/tests/conftest.pyauthbridge/sparc-service/tests/test_api.pyauthbridge/sparc-service/tests/test_engine.pyauthbridge/sparc-service/tests/test_settings.pyauthbridge/sparc-service/tests/test_watsonx_integration.py
huang195
requested changes
Jun 3, 2026
huang195
left a comment
huang195
left a comment
Member
There was a problem hiding this comment.
Summary
Large, well-engineered feature (48 files, ~4.9k LOC). The Go plugin closely follows the existing ibac patterns (config decode/validate, reentrancy sentinel header, bypass matcher, Invocation recording, secret-free logging), Dockerfiles are exemplary (distroless/nonroot, pinned), k8s manifests carry securityContexts + resource limits, and shell scripts use set -euo pipefail with proper quoting. Tests are genuinely assertive — both fail-open and fail-closed paths are exercised in the Go plugin, and the watsonx integration test uses an honest skipif gate rather than a silent skip.
Requesting changes on a single blocking item: a tautological test assertion (test_engine.py) that always passes and asserts nothing in a security-critical component. Everything else is suggestion/discussion.
Two items worth attention beyond the inline notes:
sparc-servicenetwork posture (please confirm): the Deployment setskagenti.io/inject: disabled(pod is outside the Istio ambient mesh — no mTLS / AuthorizationPolicy) and/reflectis unauthenticated. Any pod in the cluster could POST conversation content and trigger LLM calls billed to the configured credentials. Confirm this relies on a NetworkPolicy, or add authn / keep it in the mesh — and document the isolation story.- Non-blocking nits: unpinned
python:3.12-slimbase + floor-pinned Python deps (no lockfile);ollama/ollama:latestpulled unpinned; score-scale docs mix0..1(README) and1..5(sparc-plugin.md) for two different score fields — one reconciling sentence would help.
Areas reviewed: Go plugin, Python service, k8s, Shell, Dockerfiles, Docs, Tests
Commits: 1, signed-off ✅ (DCO passing)
CI: add-to-project ✅ · DCO ✅ · CodeRabbit pending
Assisted-By: Claude Code
| engine = make_engine(make_reflection_output("approve")) | ||
| engine.reflect(_request({"transaction_id": "TX4827"})) | ||
| # The fake records the run_input it was handed. | ||
| component = engine._component # noqa: SLF001 - test introspection |
Member
There was a problem hiding this comment.
must-fix — assert component is None or True is tautological; it always passes. The test name promises it verifies the request fields reach the component, but it asserts nothing. Either assert on FakeComponent.calls[0] (messages / tool_specs / tool_calls) or delete the test — as written it's dead coverage in a security-critical path.
|
|
||
| DenyScoreThreshold float64 `json:"deny_score_threshold" description:"When SPARC's score (1=worst..5=best) is <= this, escalate any reject to a hard deny regardless of on_reject_action. 0 disables escalation." default:"0"` | ||
|
|
||
| FailPolicy string `json:"fail_policy" description:"On SPARC unavailable or decision=error: open=allow and record; closed=block." default:"open" enum:"open,closed"` |
Member
There was a problem hiding this comment.
suggestion — fail_policy defaults to open: when the reflector is unreachable, times out, or returns decision=error, the proposed tool call is allowed through. The sibling ibac plugin fails closed. The comment frames SPARC as "a quality gate, not auth," so this may be deliberate — please confirm the default-open posture matches the intended threat model, and call it out in the PR description / docs since it diverges from ibac.
| try: | ||
| return await run_in_threadpool(engine.reflect, request) | ||
| except Exception as exc: # reflection failure → 502 so the plugin can fail-policy | ||
| raise HTTPException(status_code=502, detail={"error": str(exc)}) from exc |
Member
There was a problem hiding this comment.
suggestion — returning detail={"error": str(exc)} (and str(exc) in /readyz reason) echoes raw provider exception text to the caller. LiteLLM/provider errors have historically embedded the request URL with an api_key. Sanitize or truncate the exception text before returning it over the wire.
| description="Proposed tool call(s); SPARC evaluates the first.", | ||
| ) | ||
| session_id: Optional[str] = Field(default=None, description="Opaque correlation id for logs/observability.") | ||
| track: Optional[str] = Field( |
Member
There was a problem hiding this comment.
suggestion — track is accepted and documented as a per-request override, but engine.reflect() only ever uses settings.track; the field has no effect. Wire it through (and validate against the supported tracks) or drop it to avoid a misleading API contract. session_id is similarly accepted but never used/logged.
| # In-cluster Ollama for the demo — provides the agent's reasoning LLM (and the | ||
| # SPARC reflection LLM when PROVIDER=ollama) WITHOUT depending on host | ||
| # networking. The model is staged onto the kind node at /opt/ollama-models | ||
| # (by scripts/ollama-incluster.sh) and mounted here, so no large pull is needed. |
Member
There was a problem hiding this comment.
suggestion — this comment references scripts/ollama-incluster.sh, which doesn't exist in the PR. The actual staging script is scripts/stage-ollama-model.sh.
| exit 1 | ||
| fi | ||
| if ! kubectl -n "$NAMESPACE" get deploy "$AGENT_NAME" >/dev/null 2>&1; then | ||
| echo "ERROR: deployment $NAMESPACE/$AGENT_NAME not found. Run 'make demo-sparc' first." >&2 |
Member
There was a problem hiding this comment.
nit — error message says Run 'make demo-sparc' first, but the Makefile target is make demo (there's no demo-sparc target).
…demo SPARC checks that an agent's proposed tool call is grounded in the conversation and the available tool specs before it runs, catching hallucinated or ungrounded arguments such as an invented transaction id. - authlib/plugins/sparc: the Go plugin. It collects SPARC's three inputs from the inference and mcp parsers, enforces per format (an MCP tool result in mcp mode, a rewritten completion in inference mode), and records verdicts as normal pipeline invocations plus a structured session event. - sparc-service: a small FastAPI wrapper over ALTK's SPARCReflectionComponent. The provider is set via environment: watsonx by default, with ollama, openai, azure, and a generic litellm option for anything else LiteLLM supports. - demos/finance-sparc: a regular MCP finance agent where SPARC catches a hallucinated transaction id and the agent asks the user to confirm it before issuing the refund. - docs/sparc-plugin.md and the demo README. Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
OsherElhadad
force-pushed
the
feat/sparc-pretool-reflection
branch
from
June 3, 2026 21:33
23c9e9d to
e9d0a40
Compare
Author
@huang195Summary / requesting changes
must-fix — tautological assertion in
|
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/demos/finance-sparc/Makefile`:
- Around line 80-86: The preflight target currently only checks for generic
tools and doesn't verify the chosen container runtime, causing later failures if
CONTAINER_RUNTIME fell back to an unavailable option; update the preflight
recipe (target "preflight" in the Makefile) to check the effective
CONTAINER_RUNTIME value and ensure that the corresponding binary is present
(e.g., if CONTAINER_RUNTIME is "docker" verify command -v docker, if "podman"
verify command -v podman), and fail fast with a clear error message if the
selected runtime binary is not installed so build-images/load-images won't
proceed with an invalid runtime.
In `@authbridge/demos/finance-sparc/README.md`:
- Around line 24-32: The fenced example block in the README (the "Example run
(watsonx reflection):" snippet showing Turn 1/Turn 2 and SPARC lines) lacks a
language tag and triggers markdownlint; update that fenced code block by adding
a language identifier such as text or console (e.g., change ``` to ```text) so
the linter recognizes it and the block remains unchanged semantically while
satisfying markdownlint.
In `@authbridge/demos/finance-sparc/scripts/setup_keycloak_finance.py`:
- Around line 27-30: The script currently hardcodes USER_NAME, USER_PASS and
USER which creates a predictable privileged account; update it to derive the
admin username/password from a secure source (prefer an environment variable
like KEYCLOAK_ADMIN and KEYCLOAK_ADMIN_PASS or generate a
cryptographically-random password via secrets.token_urlsafe()) and set USER to
use those values, and remove or gate any automatic assignment of the realm-admin
role (do not grant realm-admin by default—require an explicit flag or only grant
in a local/test-only path). Ensure you update references to USER_NAME, USER_PASS
and USER and the role-assignment logic to use the new secure source and gating.
- Around line 49-62: The try/except blocks around kc.add_mapper_to_client_scope
and kc.add_default_default_client_scope are masking real failures by catching
all Exceptions; implement idempotent helpers (e.g., get_or_create_mapper and
get_or_create_default_scope) that explicitly detect the "already
exists/assigned" condition for kc.add_mapper_to_client_scope (using scope_id,
scope_name, agent_spiffe) and kc.add_default_default_client_scope (using
scope_id) and only swallow that specific case, while letting any other exception
propagate (or re-raise) so the script fails fast; replace the broad excepts with
calls to these helpers and log success after they return.
In `@authbridge/demos/finance-sparc/scripts/show-verdicts.py`:
- Around line 6-10: Read stdin into a string first (use sys.stdin.read()), check
if the content is empty or only whitespace and in that case print "(no session
events yet)" and exit 0; otherwise call json.loads on the content and catch
json.JSONDecodeError to handle malformed JSON (log/print an error and exit
non‑zero). Replace the current try: data = json.load(sys.stdin) except
Exception: ... with this read-then-parse flow, using json.loads and catching
json.JSONDecodeError so empty input and invalid JSON are differentiated.
In `@authbridge/sparc-service/sparc_service/settings.py`:
- Line 75: The include_raw_response setting currently defaults to True; change
its default to False and make it driven by the SPARC_INCLUDE_RAW_RESPONSE
environment/config value so raw pipeline payloads are opt-in. Update the
include_raw_response declaration (and the other occurrence of
include_raw_response at the second instance) to default to False and read/parse
SPARC_INCLUDE_RAW_RESPONSE (e.g., truthy check) so only deployments that set
that env var will enable ReflectResponse.raw_pipeline_result.
- Line 130: The current model selection line lets WX_MODEL_ID override every
provider; change the assignment in settings.py (the `model` variable) so
WX_MODEL_ID is used only when the configured provider is Watsonx (e.g.,
`provider == "watsonx"`). Concretely, replace the expression that directly ORs
os.getenv("WX_MODEL_ID") with a conditional that reads WX_MODEL_ID only when
`provider` equals the Watsonx identifier, falling back to SPARC_MODEL and then
PROVIDER_DEFAULT_MODELS.get(provider, "").
In `@authbridge/sparc-service/tests/test_api.py`:
- Around line 44-56: The test uses an impossible overall_avg_score of 2.0;
update the fixture created by make_reflection_output (used in test_api.py) to
pass a normalized score in 0..1 (e.g., 0.5) and change the assertion assert
body["overall_avg_score"] == 2.0 to match that normalized value, or
alternatively adjust the test to call the endpoint with a raw score and assert
the service normalizes it (i.e., post a raw score and assert the response
contains the normalized score). Ensure you update the make_reflection_output
call and the corresponding assertion so the test reflects the documented 0..1
contract.
In `@authbridge/sparc-service/tests/test_engine.py`:
- Around line 42-89: The tests use out-of-range score fixtures (4.5, 2.0, 5.0)
but ReflectResponse.overall_avg_score is documented as normalized 0..1; update
the fixtures created by make_reflection_output in test_approve_maps_cleanly,
test_reject_carries_issue_and_correction, and
test_raw_pipeline_suppressed_when_disabled to use normalized values (e.g., 0.9,
0.4, 1.0) and change the corresponding assertions (resp.overall_avg_score) to
match those normalized numbers so the tests enforce the documented 0..1
contract; locate usages via make_reflection_output and assertions of
resp.overall_avg_score in the listed tests.
🪄 Autofix (Beta)
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: 19b0e383-d351-4617-912a-e92c10a07ea0
📒 Files selected for processing (49)
authbridge/CLAUDE.mdauthbridge/authlib/plugins/sparc/collect.goauthbridge/authlib/plugins/sparc/plugin.goauthbridge/authlib/plugins/sparc/plugin_test.goauthbridge/authlib/plugins/sparc/reflector.goauthbridge/authlib/plugins/sparc/reflector_test.goauthbridge/authlib/plugins/sparc/respond.goauthbridge/cmd/authbridge-envoy/main.goauthbridge/cmd/authbridge-proxy/main.goauthbridge/demos/README.mdauthbridge/demos/finance-sparc/.gitignoreauthbridge/demos/finance-sparc/Makefileauthbridge/demos/finance-sparc/README.mdauthbridge/demos/finance-sparc/finance-agent/Dockerfileauthbridge/demos/finance-sparc/finance-agent/main.goauthbridge/demos/finance-sparc/finance-mcp/Dockerfileauthbridge/demos/finance-sparc/finance-mcp/main.goauthbridge/demos/finance-sparc/go.modauthbridge/demos/finance-sparc/k8s/agent.yamlauthbridge/demos/finance-sparc/k8s/finance-mcp.yamlauthbridge/demos/finance-sparc/k8s/ollama.yamlauthbridge/demos/finance-sparc/k8s/sparc-patch.yamlauthbridge/demos/finance-sparc/k8s/sparc-service.yamlauthbridge/demos/finance-sparc/scripts/drive-demo.shauthbridge/demos/finance-sparc/scripts/host-setup.shauthbridge/demos/finance-sparc/scripts/launch-abctl.shauthbridge/demos/finance-sparc/scripts/patch-sparc-config.shauthbridge/demos/finance-sparc/scripts/pipeline-merge.pyauthbridge/demos/finance-sparc/scripts/setup_keycloak_finance.pyauthbridge/demos/finance-sparc/scripts/show-verdicts.pyauthbridge/demos/finance-sparc/scripts/stage-ollama-model.shauthbridge/docs/sparc-plugin.mdauthbridge/sparc-service/.dockerignoreauthbridge/sparc-service/Dockerfileauthbridge/sparc-service/README.mdauthbridge/sparc-service/pyproject.tomlauthbridge/sparc-service/sparc_service/__init__.pyauthbridge/sparc-service/sparc_service/__main__.pyauthbridge/sparc-service/sparc_service/api.pyauthbridge/sparc-service/sparc_service/engine.pyauthbridge/sparc-service/sparc_service/models.pyauthbridge/sparc-service/sparc_service/providers.pyauthbridge/sparc-service/sparc_service/settings.pyauthbridge/sparc-service/tests/conftest.pyauthbridge/sparc-service/tests/test_api.pyauthbridge/sparc-service/tests/test_engine.pyauthbridge/sparc-service/tests/test_providers.pyauthbridge/sparc-service/tests/test_settings.pyauthbridge/sparc-service/tests/test_watsonx_integration.py
✅ Files skipped from review due to trivial changes (8)
- authbridge/demos/finance-sparc/.gitignore
- authbridge/demos/finance-sparc/finance-agent/Dockerfile
- authbridge/CLAUDE.md
- authbridge/demos/finance-sparc/finance-mcp/Dockerfile
- authbridge/sparc-service/.dockerignore
- authbridge/sparc-service/sparc_service/init.py
- authbridge/sparc-service/README.md
- authbridge/docs/sparc-plugin.md
🚧 Files skipped from review as they are similar to previous changes (28)
- authbridge/demos/README.md
- authbridge/sparc-service/Dockerfile
- authbridge/demos/finance-sparc/scripts/launch-abctl.sh
- authbridge/cmd/authbridge-envoy/main.go
- authbridge/demos/finance-sparc/scripts/stage-ollama-model.sh
- authbridge/cmd/authbridge-proxy/main.go
- authbridge/sparc-service/pyproject.toml
- authbridge/demos/finance-sparc/k8s/sparc-patch.yaml
- authbridge/demos/finance-sparc/scripts/pipeline-merge.py
- authbridge/demos/finance-sparc/k8s/finance-mcp.yaml
- authbridge/authlib/plugins/sparc/reflector_test.go
- authbridge/demos/finance-sparc/scripts/patch-sparc-config.sh
- authbridge/demos/finance-sparc/k8s/agent.yaml
- authbridge/demos/finance-sparc/k8s/sparc-service.yaml
- authbridge/sparc-service/tests/conftest.py
- authbridge/sparc-service/sparc_service/main.py
- authbridge/sparc-service/sparc_service/models.py
- authbridge/authlib/plugins/sparc/reflector.go
- authbridge/demos/finance-sparc/scripts/drive-demo.sh
- authbridge/authlib/plugins/sparc/collect.go
- authbridge/sparc-service/sparc_service/providers.py
- authbridge/demos/finance-sparc/scripts/host-setup.sh
- authbridge/sparc-service/tests/test_watsonx_integration.py
- authbridge/sparc-service/tests/test_settings.py
- authbridge/sparc-service/sparc_service/engine.py
- authbridge/demos/finance-sparc/finance-agent/main.go
- authbridge/authlib/plugins/sparc/plugin_test.go
- authbridge/authlib/plugins/sparc/respond.go
Comment on lines
+80
to
+86
| preflight: ## Verify kagenti + tools (+ watsonx creds when PROVIDER=watsonx) | ||
| @kubectl get ns $(PLATFORM_NS) >/dev/null 2>&1 || { echo "ERROR: '$(PLATFORM_NS)' not found — install kagenti first."; exit 1; } | ||
| @kubectl get ns $(NS) >/dev/null 2>&1 || { echo "ERROR: agent namespace '$(NS)' not found."; exit 1; } | ||
| @for t in kubectl kind python3 uv; do command -v $$t >/dev/null 2>&1 || { echo "ERROR: $$t not installed"; exit 1; }; done | ||
| @if [ "$(PROVIDER)" = "watsonx" ]; then \ | ||
| [ -n "$${WX_API_KEY:-}" ] && [ -n "$${WX_PROJECT_ID:-}" ] || { echo "ERROR: PROVIDER=watsonx needs WX_API_KEY/WX_PROJECT_ID (or run PROVIDER=ollama)"; exit 1; }; \ | ||
| fi |
There was a problem hiding this comment.
Fail fast when no container runtime is available.
preflight currently succeeds even when CONTAINER_RUNTIME fell back to docker only because neither Docker nor Podman is installed, so the demo dies later in build-images/load-images. Check the selected runtime here as part of the prerequisite gate.
Suggested fix
preflight: ## Verify kagenti + tools (+ watsonx creds when PROVIDER=watsonx)
`@kubectl` get ns $(PLATFORM_NS) >/dev/null 2>&1 || { echo "ERROR: '$(PLATFORM_NS)' not found — install kagenti first."; exit 1; }
`@kubectl` get ns $(NS) >/dev/null 2>&1 || { echo "ERROR: agent namespace '$(NS)' not found."; exit 1; }
- `@for` t in kubectl kind python3 uv; do command -v $$t >/dev/null 2>&1 || { echo "ERROR: $$t not installed"; exit 1; }; done
+ `@for` t in kubectl kind python3 uv $(CONTAINER_RUNTIME); do command -v $$t >/dev/null 2>&1 || { echo "ERROR: $$t not installed"; exit 1; }; done
`@if` [ "$(PROVIDER)" = "watsonx" ]; then \
[ -n "$${WX_API_KEY:-}" ] && [ -n "$${WX_PROJECT_ID:-}" ] || { echo "ERROR: PROVIDER=watsonx needs WX_API_KEY/WX_PROJECT_ID (or run PROVIDER=ollama)"; exit 1; }; \
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.
Suggested change
| preflight: ## Verify kagenti + tools (+ watsonx creds when PROVIDER=watsonx) | |
| @kubectl get ns $(PLATFORM_NS) >/dev/null 2>&1 || { echo "ERROR: '$(PLATFORM_NS)' not found — install kagenti first."; exit 1; } | |
| @kubectl get ns $(NS) >/dev/null 2>&1 || { echo "ERROR: agent namespace '$(NS)' not found."; exit 1; } | |
| @for t in kubectl kind python3 uv; do command -v $$t >/dev/null 2>&1 || { echo "ERROR: $$t not installed"; exit 1; }; done | |
| @if [ "$(PROVIDER)" = "watsonx" ]; then \ | |
| [ -n "$${WX_API_KEY:-}" ] && [ -n "$${WX_PROJECT_ID:-}" ] || { echo "ERROR: PROVIDER=watsonx needs WX_API_KEY/WX_PROJECT_ID (or run PROVIDER=ollama)"; exit 1; }; \ | |
| fi | |
| preflight: ## Verify kagenti + tools (+ watsonx creds when PROVIDER=watsonx) | |
| `@kubectl` get ns $(PLATFORM_NS) >/dev/null 2>&1 || { echo "ERROR: '$(PLATFORM_NS)' not found — install kagenti first."; exit 1; } | |
| `@kubectl` get ns $(NS) >/dev/null 2>&1 || { echo "ERROR: agent namespace '$(NS)' not found."; exit 1; } | |
| `@for` t in kubectl kind python3 uv $(CONTAINER_RUNTIME); do command -v $$t >/dev/null 2>&1 || { echo "ERROR: $$t not installed"; exit 1; }; done | |
| `@if` [ "$(PROVIDER)" = "watsonx" ]; then \ | |
| [ -n "$${WX_API_KEY:-}" ] && [ -n "$${WX_PROJECT_ID:-}" ] || { echo "ERROR: PROVIDER=watsonx needs WX_API_KEY/WX_PROJECT_ID (or run PROVIDER=ollama)"; exit 1; }; \ | |
| fi |
🧰 Tools
🪛 checkmake (0.3.2)
[warning] 80-80: Target body for "preflight" exceeds allowed length of 5 lines (6).
(maxbodylength)
🤖 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/demos/finance-sparc/Makefile` around lines 80 - 86, The preflight
target currently only checks for generic tools and doesn't verify the chosen
container runtime, causing later failures if CONTAINER_RUNTIME fell back to an
unavailable option; update the preflight recipe (target "preflight" in the
Makefile) to check the effective CONTAINER_RUNTIME value and ensure that the
corresponding binary is present (e.g., if CONTAINER_RUNTIME is "docker" verify
command -v docker, if "podman" verify command -v podman), and fail fast with a
clear error message if the selected runtime binary is not installed so
build-images/load-images won't proceed with an invalid runtime.
Comment on lines
+24
to
+32
| Example run (watsonx reflection): | ||
|
|
||
| ``` | ||
| Turn 1 agent> This tool call was not executed because it could not be verified... ask for the exact transaction_id | ||
| Turn 2 agent> Your transaction with ID TX4827 has been successfully refunded... | ||
|
|
||
| SPARC modify/reflected tool=issue_refund score=0.00 # Turn 1: ungrounded "$450" → reject + clarify | ||
| SPARC allow/grounded tool=issue_refund score=1.00 # Turn 2: grounded TX4827 → approve → refund | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced block.
markdownlint is already flagging this example block. Mark it as text or console so docs lint stays clean.
Suggested fix
-```
+```text
Turn 1 agent> This tool call was not executed because it could not be verified... ask for the exact transaction_id
Turn 2 agent> Your transaction with ID TX4827 has been successfully refunded...
SPARC modify/reflected tool=issue_refund score=0.00 # Turn 1: ungrounded "$450" → reject + clarify
SPARC allow/grounded tool=issue_refund score=1.00 # Turn 2: grounded TX4827 → approve → refund</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
Example run (watsonx reflection):
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/demos/finance-sparc/README.md` around lines 24 - 32, The fenced
example block in the README (the "Example run (watsonx reflection):" snippet
showing Turn 1/Turn 2 and SPARC lines) lacks a language tag and triggers
markdownlint; update that fenced code block by adding a language identifier such
as text or console (e.g., change ``` to ```text) so the linter recognizes it and
the block remains unchanged semantically while satisfying markdownlint.
Comment on lines
+27
to
+30
| USER_NAME = "alice" | ||
| USER_PASS = "alice123" | ||
| USER = {"username": USER_NAME, "email": "alice@example.com", | ||
| "enabled": True, "emailVerified": True, "firstName": "Alice", "lastName": "Demo"} |
There was a problem hiding this comment.
Avoid shipping a fixed realm-admin credential.
This script creates the same alice password every time and then grants that user the realm admin role. Point this at any non-local Keycloak by mistake and you've provisioned a predictable privileged account.
Safer minimal change
-USER_PASS = "alice123"
+USER_PASS = os.environ.get("DEMO_USER_PASSWORD", "")
...
- kc.set_user_password(uid, USER_PASS, temporary=False)
+ if not USER_PASS:
+ raise RuntimeError("DEMO_USER_PASSWORD must be set")
+ kc.set_user_password(uid, USER_PASS, temporary=False)Also applies to: 85-89
🧰 Tools
🪛 Ruff (0.15.15)
[error] 28-28: Possible hardcoded password assigned to: "USER_PASS"
(S105)
🤖 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/demos/finance-sparc/scripts/setup_keycloak_finance.py` around
lines 27 - 30, The script currently hardcodes USER_NAME, USER_PASS and USER
which creates a predictable privileged account; update it to derive the admin
username/password from a secure source (prefer an environment variable like
KEYCLOAK_ADMIN and KEYCLOAK_ADMIN_PASS or generate a cryptographically-random
password via secrets.token_urlsafe()) and set USER to use those values, and
remove or gate any automatic assignment of the realm-admin role (do not grant
realm-admin by default—require an explicit flag or only grant in a
local/test-only path). Ensure you update references to USER_NAME, USER_PASS and
USER and the role-assignment logic to use the new secure source and gating.
Comment on lines
+49
to
+62
| try: | ||
| kc.add_mapper_to_client_scope(scope_id, { | ||
| "name": scope_name + "-aud-mapper", "protocol": "openid-connect", | ||
| "protocolMapper": "oidc-audience-mapper", | ||
| "config": {"included.custom.audience": agent_spiffe, | ||
| "id.token.claim": "false", "access.token.claim": "true"}, | ||
| }) | ||
| except Exception as e: | ||
| print(f" (mapper exists or: {e})") | ||
| try: | ||
| kc.add_default_default_client_scope(scope_id) | ||
| print(f" '{scope_name}' is now a realm default scope") | ||
| except Exception as e: | ||
| print(f" (realm default: {e})") |
There was a problem hiding this comment.
Don't mask required Keycloak mutations as harmless idempotency.
These except Exception branches let the script print Keycloak setup complete even when the audience mapper, default scopes, or admin role were never applied, which leaves the demo broken in a hard-to-diagnose way. Only swallow the specific "already exists / already assigned" case inside dedicated get_or_create_* helpers; fail fast on everything else.
Suggested direction
+def get_or_create_client_scope(kc: KeycloakAdmin, name: str) -> str:
+ existing = next((s["id"] for s in kc.get_client_scopes() if s["name"] == name), None)
+ if existing:
+ return existing
+ return kc.create_client_scope({...}, skip_exists=True)
+
+def ensure_default_scope(kc: KeycloakAdmin, client_id: str, scope_id: str, label: str) -> None:
+ try:
+ kc.add_client_default_client_scope(client_id, scope_id, {})
+ except SomeAlreadyAssignedError:
+ pass
+ else:
+ print(f" added aud scope as default on {label}")As per coding guidelines, authbridge/**/setup_keycloak*.py: Implement idempotent get_or_create_* helper functions in Python Keycloak setup scripts.
Also applies to: 73-89
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 56-56: Do not catch blind exception: Exception
(BLE001)
[warning] 61-61: Do not catch blind exception: Exception
(BLE001)
🤖 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/demos/finance-sparc/scripts/setup_keycloak_finance.py` around
lines 49 - 62, The try/except blocks around kc.add_mapper_to_client_scope and
kc.add_default_default_client_scope are masking real failures by catching all
Exceptions; implement idempotent helpers (e.g., get_or_create_mapper and
get_or_create_default_scope) that explicitly detect the "already
exists/assigned" condition for kc.add_mapper_to_client_scope (using scope_id,
scope_name, agent_spiffe) and kc.add_default_default_client_scope (using
scope_id) and only swallow that specific case, while letting any other exception
propagate (or re-raise) so the script fails fast; replace the broad excepts with
calls to these helpers and log success after they return.
Comment on lines
+6
to
+10
| try: | ||
| data = json.load(sys.stdin) | ||
| except Exception: | ||
| print(" (no session events yet)") | ||
| sys.exit(0) |
There was a problem hiding this comment.
Differentiate empty input from invalid session JSON.
Catching every exception here turns malformed snapshots into the same message as "nothing has happened yet", which hides real demo/debug failures. Limit this to parse/read errors you actually expect.
Suggested change
try:
data = json.load(sys.stdin)
-except Exception:
+except (json.JSONDecodeError, OSError):
print(" (no session events yet)")
sys.exit(0)🧰 Tools
🪛 Ruff (0.15.15)
[warning] 8-8: Do not catch blind exception: Exception
(BLE001)
🤖 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/demos/finance-sparc/scripts/show-verdicts.py` around lines 6 - 10,
Read stdin into a string first (use sys.stdin.read()), check if the content is
empty or only whitespace and in that case print "(no session events yet)" and
exit 0; otherwise call json.loads on the content and catch json.JSONDecodeError
to handle malformed JSON (log/print an error and exit non‑zero). Replace the
current try: data = json.load(sys.stdin) except Exception: ... with this
read-then-parse flow, using json.loads and catching json.JSONDecodeError so
empty input and invalid JSON are differentiated.
| llm_timeout_seconds: int = 120 | ||
| retries: int = 3 | ||
| max_parallel: int = 2 | ||
| include_raw_response: bool = True |
There was a problem hiding this comment.
Make raw pipeline payloads opt-in, not default-on.
ReflectResponse.raw_pipeline_result is documented as included only when SPARC_INCLUDE_RAW_RESPONSE is set, but this settings default turns it on for every deployment unless operators explicitly disable it. That expands response size and exposes full backend payloads on the default path.
Suggested fix
- include_raw_response: bool = True
+ include_raw_response: bool = False
...
- include_raw_response=_truthy(os.getenv("SPARC_INCLUDE_RAW_RESPONSE", "true")),
+ include_raw_response=_truthy(os.getenv("SPARC_INCLUDE_RAW_RESPONSE", "false")),Also applies to: 162-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@authbridge/sparc-service/sparc_service/settings.py` at line 75, The
include_raw_response setting currently defaults to True; change its default to
False and make it driven by the SPARC_INCLUDE_RAW_RESPONSE environment/config
value so raw pipeline payloads are opt-in. Update the include_raw_response
declaration (and the other occurrence of include_raw_response at the second
instance) to default to False and read/parse SPARC_INCLUDE_RAW_RESPONSE (e.g.,
truthy check) so only deployments that set that env var will enable
ReflectResponse.raw_pipeline_result.
| wx_project_id = os.getenv("WX_PROJECT_ID") or os.getenv("WATSONX_PROJECT_ID") or "" | ||
| wx_url = os.getenv("WX_URL") or os.getenv("WATSONX_URL") or "https://us-south.ml.cloud.ibm.com" | ||
|
|
||
| model = os.getenv("SPARC_MODEL") or os.getenv("WX_MODEL_ID") or PROVIDER_DEFAULT_MODELS.get(provider, "") |
There was a problem hiding this comment.
Scope WX_MODEL_ID to Watsonx only.
WX_MODEL_ID currently overrides the model for every provider. In an environment that already exports Watsonx vars, switching SPARC_LLM_PROVIDER to openai, azure, or ollama can silently build requests with the wrong model id.
Suggested fix
- model = os.getenv("SPARC_MODEL") or os.getenv("WX_MODEL_ID") or PROVIDER_DEFAULT_MODELS.get(provider, "")
+ model = (os.getenv("SPARC_MODEL") or "").strip()
+ if not model:
+ if provider == "watsonx":
+ model = os.getenv("WX_MODEL_ID") or PROVIDER_DEFAULT_MODELS[provider]
+ else:
+ model = PROVIDER_DEFAULT_MODELS.get(provider, "")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@authbridge/sparc-service/sparc_service/settings.py` at line 130, The current
model selection line lets WX_MODEL_ID override every provider; change the
assignment in settings.py (the `model` variable) so WX_MODEL_ID is used only
when the configured provider is Watsonx (e.g., `provider == "watsonx"`).
Concretely, replace the expression that directly ORs os.getenv("WX_MODEL_ID")
with a conditional that reads WX_MODEL_ID only when `provider` equals the
Watsonx identifier, falling back to SPARC_MODEL and then
PROVIDER_DEFAULT_MODELS.get(provider, "").
Comment on lines
+44
to
+56
| output = make_reflection_output( | ||
| "reject", | ||
| issues=[ | ||
| {"issue_type": "semantic_function", "metric_name": "m", "explanation": "no refund tool", "correction": None} | ||
| ], | ||
| overall_avg_score=2.0, | ||
| ) | ||
| resp = _client(output).post("/reflect", json=REQUEST_BODY) | ||
| assert resp.status_code == 200 | ||
| body = resp.json() | ||
| assert body["decision"] == "reject" | ||
| assert body["overall_avg_score"] == 2.0 | ||
| assert body["issues"][0]["explanation"] == "no refund tool" |
There was a problem hiding this comment.
Use an in-range score fixture in this API contract test.
overall_avg_score is documented as normalized 0..1, so asserting 2.0 here bakes an impossible value into the HTTP contract. Use a normalized fixture instead, or explicitly assert that the engine normalizes raw provider output before serialization.
🤖 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_api.py` around lines 44 - 56, The test
uses an impossible overall_avg_score of 2.0; update the fixture created by
make_reflection_output (used in test_api.py) to pass a normalized score in 0..1
(e.g., 0.5) and change the assertion assert body["overall_avg_score"] == 2.0 to
match that normalized value, or alternatively adjust the test to call the
endpoint with a raw score and assert the service normalizes it (i.e., post a raw
score and assert the response contains the normalized score). Ensure you update
the make_reflection_output call and the corresponding assertion so the test
reflects the documented 0..1 contract.
Comment on lines
+42
to
+89
| def test_approve_maps_cleanly(): | ||
| engine = make_engine(make_reflection_output("approve", overall_avg_score=4.5)) | ||
| resp = engine.reflect(_request({"transaction_id": "TX4827"})) | ||
| assert resp.decision == "approve" | ||
| assert resp.issues == [] | ||
| assert resp.overall_avg_score == 4.5 | ||
| assert resp.execution_time_ms == 12.3 | ||
|
|
||
|
|
||
| def test_reject_carries_issue_and_correction(): | ||
| output = make_reflection_output( | ||
| "reject", | ||
| issues=[ | ||
| { | ||
| "issue_type": "semantic_function", | ||
| "metric_name": "function_selection_appropriateness", | ||
| "explanation": "get_transaction does not process refunds.", | ||
| "correction": {"corrected_function_name": "no_function"}, | ||
| } | ||
| ], | ||
| overall_avg_score=2.0, | ||
| ) | ||
| engine = make_engine(output) | ||
| resp = engine.reflect(_request({"transaction_id": "TX4821"})) | ||
| assert resp.decision == "reject" | ||
| assert len(resp.issues) == 1 | ||
| issue = resp.issues[0] | ||
| assert issue.metric_name == "function_selection_appropriateness" | ||
| assert "refund" in issue.explanation | ||
| assert issue.correction["corrected_function_name"] == "no_function" | ||
| assert resp.overall_avg_score == 2.0 | ||
|
|
||
|
|
||
| def test_error_decision_passthrough(): | ||
| engine = make_engine(make_reflection_output("error")) | ||
| resp = engine.reflect(_request({"transaction_id": "TX4821"})) | ||
| assert resp.decision == "error" | ||
|
|
||
|
|
||
| def test_raw_pipeline_suppressed_when_disabled(): | ||
| from sparc_service.settings import Settings | ||
|
|
||
| settings = Settings(provider="watsonx", wx_api_key="k", wx_project_id="p", include_raw_response=False) | ||
| engine = make_engine(make_reflection_output("approve", overall_avg_score=5.0), settings) | ||
| resp = engine.reflect(_request({"transaction_id": "TX4827"})) | ||
| # Score is still surfaced, but the verbose raw pipeline is withheld. | ||
| assert resp.overall_avg_score == 5.0 | ||
| assert resp.raw_pipeline_result is None |
There was a problem hiding this comment.
Keep the score fixtures inside the documented 0..1 range.
These tests use 4.5, 2.0, and 5.0, but ReflectResponse.overall_avg_score is documented as normalized 0..1. As written, they allow an out-of-contract projection to pass unnoticed instead of catching it.
🤖 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_engine.py` around lines 42 - 89, The
tests use out-of-range score fixtures (4.5, 2.0, 5.0) but
ReflectResponse.overall_avg_score is documented as normalized 0..1; update the
fixtures created by make_reflection_output in test_approve_maps_cleanly,
test_reject_carries_issue_and_correction, and
test_raw_pipeline_suppressed_when_disabled to use normalized values (e.g., 0.9,
0.4, 1.0) and change the corresponding assertions (resp.overall_avg_score) to
match those normalized numbers so the tests enforce the documented 0..1
contract; locate usages via make_reflection_output and assertions of
resp.overall_avg_score in the listed tests.
huang195
approved these changes
Jun 3, 2026
huang195
left a comment
huang195
left a comment
Member
There was a problem hiding this comment.
Re-review (commit e9d0a40) — all previous findings addressed ✅
Thanks for the quick turnaround. The full CI suite is now green (Go CI, Python Tests, Bandit, Hadolint, shellcheck, yamllint, Trivy, action-pinning).
Resolution of prior review:
| Finding | Status |
|---|---|
test_engine.py tautological assertion (must-fix) |
✅ Fixed — now asserts on component.calls[0] (messages / tool_specs / tool_calls) |
str(exc) secret-leak in 502 / readyz |
✅ Fixed — 502 returns generic "reflection failed", full detail logged server-side; ready() reason sanitized with an explanatory comment |
track accepted but ignored |
✅ Fixed — wired through (request.track or settings.track), validated → 400, with new test_unsupported_track_rejected / test_reflect_400_on_unsupported_track |
ollama.yaml stale ollama-incluster.sh reference |
✅ Fixed — now stage-ollama-model.sh |
launch-abctl.sh wrong make demo-sparc reference |
✅ Fixed |
fail_policy default-open (confirm-by-design) |
✅ Resolved — kept by design; docs now clarify SPARC is a quality gate, not an auth control, and handleNoContext applies the policy on missing context too |
| sparc-service unauthenticated / out-of-mesh | ✅ Addressed — documented NetworkPolicy guidance in README + sparc-plugin.md, plus added an optional reflector_bearer token |
Also noticed a new test_providers.py for provider-selection coverage — nice.
No blocking items remain. Approving.
Assisted-By: Claude Code
github-project-automation
Bot
moved this from New /:ToDo
to Done
in Rossoctl Issue Prioritization
Ronen-Levy
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SPARC checks that an agent's proposed tool call is grounded in the conversation and the available tool specs before it runs, catching hallucinated or ungrounded arguments such as an invented transaction id.
Summary
Related issue(s)
#479 feat(sparc-service): one-command install; deploy SPARC via a shared i…
(Optional) Testing Instructions
Fixes #
Summary by CodeRabbit
New Features
Documentation
Tests