feat(sparc-service): one-command install; deploy SPARC via a shared i…#479
Conversation
…nstaller The SPARC service deploy artifacts don't belong in the kagenti Helm chart, so move them here next to the rest of SPARC. Add authbridge/sparc-service/deploy: a single `make install` that configures the provider (watsonx by default, or ollama/openai/azure/litellm) and deploys the service into a cluster — run once, before enabling the `sparc` plugin on any agent. Publish the sparc-service image from CI so the default install needs no local build (a local `make image` path remains for kind). Point the finance-sparc demo at the same installer instead of carrying its own copy of the manifest, so there is one source of truth, and make the demo's verdict summary scan every session so it reports reliably regardless of which session the agent's outbound calls land in. Signed-off-by: Osher-Elhadad <Osher.Elhadad@ibm.com>
📝 WalkthroughWalkthroughThis PR decouples SPARC reflection service deployment from the finance-sparc demo by introducing a shared, reusable Kubernetes deployment infrastructure. The service deployment is relocated to ChangesSPARC Service Shared Deployment
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@authbridge/demos/finance-sparc/scripts/show-verdicts.py`:
- Around line 13-18: Add Python 3.12 type annotations for the module-level state
and function signature: annotate BASE as str | None (or str if always a string),
add a return type and parameter types to get (e.g., def get(path: str) -> Any or
dict[str, Any] as appropriate), and annotate the top-level containers seen and
rows (e.g., set[str], list[dict[str, Any]]). Use modern union syntax (A | B)
where needed and import typing-only symbols (Any, dict) if required.
In `@authbridge/sparc-service/deploy/Makefile`:
- Around line 70-73: The Makefile currently builds a sec string from CRED_VARS
and always runs the kubectl create secret command even when sec is empty, which
causes failure for credential-less installs; change the install logic to check
if the computed sec variable is non-empty before invoking "kubectl -n
$(NAMESPACE) create secret generic sparc-creds $$sec --dry-run=client -o yaml |
kubectl apply -f -", e.g. after the loop test "[ -n \"$$sec\" ] && (kubectl ...
) || echo 'no credentials; skipping secret creation'", ensuring you reference
the same sec and CRED_VARS variables used in the snippet so the secret command
is only executed when at least one --from-literal was appended.
🪄 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: d9ba4f6c-fb73-484e-b79a-5248ba32a837
📒 Files selected for processing (10)
.github/workflows/build.yamlauthbridge/demos/finance-sparc/Makefileauthbridge/demos/finance-sparc/README.mdauthbridge/demos/finance-sparc/scripts/drive-demo.shauthbridge/demos/finance-sparc/scripts/show-verdicts.pyauthbridge/docs/sparc-plugin.mdauthbridge/sparc-service/README.mdauthbridge/sparc-service/deploy/Makefileauthbridge/sparc-service/deploy/README.mdauthbridge/sparc-service/deploy/sparc-service.yaml
| BASE = sys.argv[1] if len(sys.argv) > 1 else "http://localhost:19094" | ||
|
|
||
|
|
||
| def get(path): | ||
| with urllib.request.urlopen(BASE + path, timeout=10) as resp: | ||
| return json.load(resp) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add Python 3.12 type hints for script interfaces and state
authbridge/demos/finance-sparc/scripts/show-verdicts.py (under authbridge/**/*.py) has untyped top-level state and function/interface code (BASE, get, and likely seen/rows at 27-28), violating the repo’s Python 3.12 typing rule. Add explicit type hints using modern union syntax (e.g., str | None).
🧰 Tools
🪛 Ruff (0.15.15)
[error] 17-17: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🤖 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 13 -
18, Add Python 3.12 type annotations for the module-level state and function
signature: annotate BASE as str | None (or str if always a string), add a return
type and parameter types to get (e.g., def get(path: str) -> Any or dict[str,
Any] as appropriate), and annotate the top-level containers seen and rows (e.g.,
set[str], list[dict[str, Any]]). Use modern union syntax (A | B) where needed
and import typing-only symbols (Any, dict) if required.
| @sec=""; for v in $(CRED_VARS); do \ | ||
| val=$$(printenv $$v || true); [ -n "$$val" ] && sec="$$sec --from-literal=$$v=$$val"; \ | ||
| done; \ | ||
| kubectl -n $(NAMESPACE) create secret generic sparc-creds $$sec --dry-run=client -o yaml | kubectl apply -f - |
There was a problem hiding this comment.
Avoid creating an empty Secret in install.
When none of $(CRED_VARS) is set (for example PROVIDER=ollama), kubectl create secret generic sparc-creds $$sec ... runs with no --from-* flags and fails, which breaks the intended credential-less install path.
🔧 Suggested fix
`@sec`=""; for v in $(CRED_VARS); do \
val=$$(printenv $$v || true); [ -n "$$val" ] && sec="$$sec --from-literal=$$v=$$val"; \
done; \
- kubectl -n $(NAMESPACE) create secret generic sparc-creds $$sec --dry-run=client -o yaml | kubectl apply -f -
+ if [ -n "$$sec" ]; then \
+ kubectl -n $(NAMESPACE) create secret generic sparc-creds $$sec --dry-run=client -o yaml | kubectl apply -f -; \
+ else \
+ kubectl -n $(NAMESPACE) delete secret sparc-creds --ignore-not-found >/dev/null; \
+ 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.
| @sec=""; for v in $(CRED_VARS); do \ | |
| val=$$(printenv $$v || true); [ -n "$$val" ] && sec="$$sec --from-literal=$$v=$$val"; \ | |
| done; \ | |
| kubectl -n $(NAMESPACE) create secret generic sparc-creds $$sec --dry-run=client -o yaml | kubectl apply -f - | |
| `@sec`=""; for v in $(CRED_VARS); do \ | |
| val=$$(printenv $$v || true); [ -n "$$val" ] && sec="$$sec --from-literal=$$v=$$val"; \ | |
| done; \ | |
| if [ -n "$$sec" ]; then \ | |
| kubectl -n $(NAMESPACE) create secret generic sparc-creds $$sec --dry-run=client -o yaml | kubectl apply -f -; \ | |
| else \ | |
| kubectl -n $(NAMESPACE) delete secret sparc-creds --ignore-not-found >/dev/null; \ | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@authbridge/sparc-service/deploy/Makefile` around lines 70 - 73, The Makefile
currently builds a sec string from CRED_VARS and always runs the kubectl create
secret command even when sec is empty, which causes failure for credential-less
installs; change the install logic to check if the computed sec variable is
non-empty before invoking "kubectl -n $(NAMESPACE) create secret generic
sparc-creds $$sec --dry-run=client -o yaml | kubectl apply -f -", e.g. after the
loop test "[ -n \"$$sec\" ] && (kubectl ... ) || echo 'no credentials; skipping
secret creation'", ensuring you reference the same sec and CRED_VARS variables
used in the snippet so the secret command is only executed when at least one
--from-literal was appended.
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Review — clean structural refactor ✅
The headline of this PR is a directory-structure correction, and it's done right: the SPARC reflection-service manifest moves out of the demo's k8s/ folder into authbridge/sparc-service/deploy/, co-located with the service it deploys, and the demo now consumes that shared installer instead of carrying its own copy (single source of truth). Verified the old manifest is gone from the demo k8s/, the README's k8s/{agent,finance-mcp}.yaml reference matches reality, and the new CI build entry points at a real Dockerfile. Manifest hygiene is good (resource requests/limits, non-root, readiness/liveness probes, namespace de-baked). All 20 CI checks pass; single commit, signed-off.
A note on authbridge/sparc-service/ placement (non-blocking)
Worth separating two things that share the "sparc" name:
- The
sparcplugin lives atauthbridge/authlib/plugins/sparc/, next to its 6 peers — correctly placed. sparc-serviceis not the plugin; it's a standalone Python LLM-reflection backend the plugin calls over HTTP (own image, own lifecycle), so it can't live underauthlib/plugins/.
That directory is pre-existing on main — this PR only adds deploy/ inside it — so it's not in scope to gate here. The legitimate structural observation is one level up: SPARC is the only plugin that ships a backend service, and that service sits as a top-level authbridge/ sibling (alongside authlib, cmd, demos, docs, proxy-init). That's fine while it's the only one, but it won't scale — if another plugin later grows an out-of-process backend, a convention like authbridge/services/<name>/ would be cleaner than a top-level dir per service. Suggest tracking that as a follow-up issue rather than addressing it here.
Areas reviewed
Directory structure, Helm/K8s manifest, Makefiles, Python (show-verdicts.py), Docs, CI build matrix.
Verdict: APPROVE — no must-fix issues; comments below are 1 suggestion + 2 nits.
| # ConfigMap and the optional `sparc-watsonx` Secret, both created by the | ||
| # Makefile (provider-aware: watsonx or ollama). In production this ships via the | ||
| # kagenti Helm chart (components.sparcService.enabled=true). | ||
| # SPARC reflection service — the backend the AuthBridge `sparc` plugin calls. |
There was a problem hiding this comment.
suggestion (cross-repo, non-blocking): the previous version of this manifest noted the service "ships via the kagenti Helm chart (components.sparcService.enabled=true)". This PR moves to an installer-based model. If that Helm path still exists in kagenti/kagenti, it now diverges from this installer — the same two-sources-of-truth problem this PR fixes within-repo, just across repos. Worth confirming the chart side is reconciled or opening a tracking issue.
| @echo "[*] SPARC service ready at http://sparc-service.$(NAMESPACE).svc:$(PORT)/reflect" | ||
| @echo " Now enable the 'sparc' plugin in an agent's authbridge config (see docs/sparc-plugin.md)." | ||
|
|
||
| status: ## Show the SPARC service deployment + config |
There was a problem hiding this comment.
nit: kubectl get deploy,svc,cm/sparc-service-config -l app.kubernetes.io/name=sparc-service — the -l selector also filters the explicitly-named ConfigMap, which install creates without that label, so it won't show on this line. Harmless (the following kubectl ... cm sparc-service-config -o yaml covers it), but the label filter on a named resource is inconsistent.
| @printf "\nVars: PROVIDER(=%s) NAMESPACE(=%s) IMAGE MODEL TRACK OLLAMA_BASE_URL\n" "$(PROVIDER)" "$(NAMESPACE)" | ||
| @printf " LLM_KWARGS_JSON LLM_REGISTRY_ID KIND_CLUSTER_NAME\n\n" | ||
|
|
||
| image: ## Build the sparc-service image locally and load it into kind |
There was a problem hiding this comment.
nit: ctr ... tag localhost/$(IMAGE) docker.io/library/$(IMAGE) produces a doubled-registry ref (docker.io/library/ghcr.io/...) since IMAGE is a full ghcr path by default. It's || true defensive and mirrors the demo's existing pattern, so it won't break — just noting it likely no-ops for the ghcr-tagged default image.
github-project-automation
Bot
moved this from New /:ToDo
to Done
in Rossoctl Issue Prioritization
3 participants
…nstaller
The SPARC service deploy artifacts don't belong in the kagenti Helm chart, so move them here next to the rest of SPARC. Add authbridge/sparc-service/deploy: a single
make installthat configures the provider (watsonx by default, or ollama/openai/azure/litellm) and deploys the service into a cluster — run once, before enabling thesparcplugin on any agent. Publish the sparc-service image from CI so the default install needs no local build (a localmake imagepath remains for kind).Point the finance-sparc demo at the same installer instead of carrying its own copy of the manifest, so there is one source of truth, and make the demo's verdict summary scan every session so it reports reliably regardless of which session the agent's outbound calls land in.
Summary
Related issue(s)
(Optional) Testing Instructions
Fixes #
Summary by CodeRabbit
Release Notes
New Features
Documentation
Refactor