✨ Import the reference client stack (client core, hub-shim, UI, harnesses, demo) - #35
Closed
ibolton336 wants to merge 1 commit into
Closed
ibolton336 wants to merge 1 commit into
ibolton336 wants to merge 1 commit into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR imports a reference client stack with shared AgentRun and ACP clients, a hub-shim service, mock and Goose harnesses, a PatternFly UI, Kubernetes deployment resources, demo workflows, and ADR documentation. ChangesReference client contracts and transports
Hub-shim and harness runtime
Browser UI and cluster deployment
Demo workflows and documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ibolton336
force-pushed
the
clients-reference-stack
branch
from
July 20, 2026 18:34
9b4a171 to
310f00c
Compare
Member
Author
|
Rebased onto main (post #33/#34/#37) — no conflicts — plus a follow-up commit aligning the stack with what merged while this sat:
All packages typecheck/build green (tsc + vite). @djzager ready for review when you get a chance. 🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
clients/hack/demo-up.sh (1)
83-84: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse a project-local directory for state files instead of
/tmp. Hardcoding predictable paths in the world-writable/tmpdirectory exposes the scripts to local symlink attacks (CWE-377) and pollutes global state. Consider creating a project-local directory likeSTATE_DIR="$ROOT/.demo"and writing PID and log files there.
clients/hack/demo-up.sh#L83-L84: redirect the port-forward log and pid to$STATE_DIR/demo-hub-pf....clients/hack/demo-up.sh#L110-L111: redirect the hub-shim log and pid to$STATE_DIR/demo-hub-shim....clients/hack/demo-up.sh#L128-L129: redirect the ui log and pid to$STATE_DIR/demo-ui....clients/hack/demo-down.sh#L8-L8: read the pidfiles from$ROOT/.demo/demo-$name.pidinstead of/tmp.🤖 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 `@clients/hack/demo-up.sh` around lines 83 - 84, Replace world-writable /tmp state paths with a project-local STATE_DIR under $ROOT/.demo, creating it before use. In clients/hack/demo-up.sh lines 83-84, 110-111, and 128-129, write each demo log and PID file beneath STATE_DIR; in clients/hack/demo-down.sh line 8, read the demo-$name.pid files from $ROOT/.demo instead of /tmp.Source: Linters/SAST tools
clients/deploy/ui/nginx.conf (1)
12-13: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueStartup-time DNS resolution of the gateway upstream can crashloop the UI pod.
proxy_passto the bareagentic-gatewayhostname is resolved once at nginx startup. If theagentic-gatewayService object isn't present when this pod starts, nginx fails with "host not found in upstream" and crashloops. Applyinggateway.yamlandui.yamltogether via the kustomization usually avoids this, but ordering isn't guaranteed. For resilience, resolve lazily via aresolver+ variable upstream (appending$request_urito preserve the path).🤖 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 `@clients/deploy/ui/nginx.conf` around lines 12 - 13, Update the nginx location /api/ configuration to resolve agentic-gateway lazily using a resolver directive and a variable-based proxy_pass, appending $request_uri so the original request path and query are preserved; avoid the bare static upstream form that performs DNS resolution during nginx startup.clients/deploy/manifests/gateway.yaml (1)
59-84: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winHarden both Deployments with an explicit container
securityContext. Both containers run with the default security context (root-capable, privilege escalation allowed), flagged by Trivy (KSV-0014/KSV-0118) and Checkov (CKV_K8S_20/CKV_K8S_23). The gateway holdssecrets: getRBAC, so this is worth tightening even for an interim deployment. Apply the always-safe subset below;readOnlyRootFilesystem: trueadditionally needs writableemptyDirmounts (e.g./tmp,/var/cache/nginxfor the UI) so add it separately once volumes are wired.
clients/deploy/manifests/gateway.yaml#L59-L84: add asecurityContextto thegatewaycontainer.clients/deploy/manifests/ui.yaml#L22-L40: add the samesecurityContextto theuicontainer (thenginx-unprivilegedbase already runs as uid 101, sorunAsNonRootis consistent).securityContext: runAsNonRoot: true allowPrivilegeEscalation: false capabilities: drop: ["ALL"] seccompProfile: type: RuntimeDefault🤖 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 `@clients/deploy/manifests/gateway.yaml` around lines 59 - 84, The gateway container in clients/deploy/manifests/gateway.yaml lines 59-84 and the UI container in clients/deploy/manifests/ui.yaml lines 22-40 both require the same explicit container securityContext: set runAsNonRoot, disable privilege escalation, drop all capabilities, and use the RuntimeDefault seccomp profile. Do not enable readOnlyRootFilesystem until the required writable emptyDir mounts are added.Source: Linters/SAST tools
clients/ui/src/app.css (1)
59-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace deprecated
word-break: break-word.The
word-break: break-wordproperty is deprecated in modern CSS. Useoverflow-wrap: break-wordinstead to achieve the same result in a standard-compliant way.♻️ Proposed refactor
white-space: pre-wrap; - word-break: break-word; + overflow-wrap: break-word; }🤖 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 `@clients/ui/src/app.css` around lines 59 - 61, In the affected CSS rule in app.css, replace the deprecated word-break: break-word declaration with overflow-wrap: break-word, preserving the existing white-space behavior and other declarations.Source: Linters/SAST tools
clients/deploy/README.md (1)
8-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language identifier to the fenced code block.
As suggested by static analysis, fenced code blocks should have a language specified. Since this is an ASCII architecture diagram, using
textis appropriate.♻️ Proposed refactor
-``` +```text browser ── ingress/route (TLS + SSO) ── agentic-ui (nginx, static SPA) │ /api + WS, same-origin agentic-gateway (SA + RBAC) │ CRs via k8s API · pod :4000 via service DNS agentic-controller / Agent Sandbox / sandbox pods</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@clients/deploy/README.mdaround lines 8 - 14, Update the fenced architecture
diagram code block in the README to include the text language identifier,
changing the opening fence to ```text while preserving the diagram content
unchanged.</details> <!-- cr-comment:v1:b79bc3a37843f16e04d4d1ac --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>clients/harness-mock/Dockerfile (1)</summary><blockquote> `1-7`: _🔒 Security & Privacy_ | _🔵 Trivial_ | _💤 Low value_ **Both harness images run as `root` (Trivy DS-0002).** Shared root cause: no `USER` directive. Since the goose harness clones arbitrary repos and executes agent tooling, dropping root is especially worthwhile there. - `clients/harness-mock/Dockerfile#L1-L7`: add `USER node` before `CMD` (ensure `/app` is readable by it). - `clients/harness-goose/Dockerfile#L28-L33`: create/own a non-root user with write access to `/workspace` and switch to it before `CMD`. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/harness-mock/Dockerfile` around lines 1 - 7, Both harness Dockerfiles currently run as root because they lack a USER directive. In clients/harness-mock/Dockerfile, ensure /app is readable by the existing node user and add USER node before CMD. In clients/harness-goose/Dockerfile, create and own a non-root user with write access to /workspace, then switch to that user before CMD. ``` </details> <!-- cr-comment:v1:1f137d115f67f507714e07ba --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>clients/manifests/goose-bedrock.yaml (1)</summary><blockquote> `15-17`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Stale references to the removed simulator in both sample manifests.** This PR excludes the prototype simulator scaffolding, yet both files still cite it. - `clients/manifests/goose-bedrock.yaml#L15-L17`: drop/replace the "See simulate-controller.ts" comment. - `clients/manifests/samples.yaml#L66-L68`: reword "The controller simulator substitutes the mock ACP harness image" to describe the real-controller behavior. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@clients/manifests/goose-bedrock.yaml` around lines 15 - 17, Remove or replace the stale “See simulate-controller.ts” reference in clients/manifests/goose-bedrock.yaml lines 15-17 while preserving the goose-provider contract comment. In clients/manifests/samples.yaml lines 66-68, reword the note about the controller simulator substituting the mock ACP harness image to accurately describe real-controller behavior. ``` </details> <!-- cr-comment:v1:581301e5b9d12d02b6377a9d --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@clients/hack/demo-up.sh:
- Line 104: Update both npm install commands in clients/hack/demo-up.sh at lines
104-104 and 125-125 to handle failures explicitly: retain or replace the
redirection with visible/logged output and append failure handling that calls
die with a context-specific npm install error for hub-shim and the UI. Ensure
set -e failures no longer terminate the script silently.In
@clients/harness-goose/Dockerfile:
- Around line 6-19: Update the GOOSE_ARCH default used by the Dockerfile
download flow to match the target platform, using Docker’s TARGETARCH mapping
where appropriate so x86_64 builds select the goose-x86_64-unknown-linux-gnu
archive while preserving the correct ARM mapping. Keep the existing download,
extraction, and version-check steps unchanged.In
@clients/packages/agentic-client/src/transport-shim/index.ts:
- Around line 98-119: Update the transport shim’s send method to pass an
AbortSignal.timeout(30_000) signal in the fetch options, ensuring stalled
requests abort after 30 seconds while preserving the existing error handling and
response validation.In
@clients/packages/agentrun-client/src/kube.ts:
- Around line 183-197: Update the sandbox pod lookup around readNamespacedPod so
only a 404 result is treated as a missing pod and falls through to the
label-selector fallback. Preserve the fallback for not-found responses, but
rethrow non-404 errors such as RBAC, network, or server failures instead of
converting them into the generic “No sandbox pod found” error; use the existing
k8sStatusCode helper or equivalent status check.
Nitpick comments:
In@clients/deploy/manifests/gateway.yaml:
- Around line 59-84: The gateway container in
clients/deploy/manifests/gateway.yaml lines 59-84 and the UI container in
clients/deploy/manifests/ui.yaml lines 22-40 both require the same explicit
container securityContext: set runAsNonRoot, disable privilege escalation, drop
all capabilities, and use the RuntimeDefault seccomp profile. Do not enable
readOnlyRootFilesystem until the required writable emptyDir mounts are added.In
@clients/deploy/README.md:
- Around line 8-14: Update the fenced architecture diagram code block in the
README to include the text language identifier, changing the opening fence toIn `@clients/deploy/ui/nginx.conf`: - Around line 12-13: Update the nginx location /api/ configuration to resolve agentic-gateway lazily using a resolver directive and a variable-based proxy_pass, appending $request_uri so the original request path and query are preserved; avoid the bare static upstream form that performs DNS resolution during nginx startup. In `@clients/hack/demo-up.sh`: - Around line 83-84: Replace world-writable /tmp state paths with a project-local STATE_DIR under $ROOT/.demo, creating it before use. In clients/hack/demo-up.sh lines 83-84, 110-111, and 128-129, write each demo log and PID file beneath STATE_DIR; in clients/hack/demo-down.sh line 8, read the demo-$name.pid files from $ROOT/.demo instead of /tmp. In `@clients/harness-mock/Dockerfile`: - Around line 1-7: Both harness Dockerfiles currently run as root because they lack a USER directive. In clients/harness-mock/Dockerfile, ensure /app is readable by the existing node user and add USER node before CMD. In clients/harness-goose/Dockerfile, create and own a non-root user with write access to /workspace, then switch to that user before CMD. In `@clients/manifests/goose-bedrock.yaml`: - Around line 15-17: Remove or replace the stale “See simulate-controller.ts” reference in clients/manifests/goose-bedrock.yaml lines 15-17 while preserving the goose-provider contract comment. In clients/manifests/samples.yaml lines 66-68, reword the note about the controller simulator substituting the mock ACP harness image to accurately describe real-controller behavior. In `@clients/ui/src/app.css`: - Around line 59-61: In the affected CSS rule in app.css, replace the deprecated word-break: break-word declaration with overflow-wrap: break-word, preserving the existing white-space behavior and other declarations.🪄 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
Run ID:
bb2b57cb-0172-4b89-b1ca-b2de1851834d⛔ Files ignored due to path filters (5)
clients/harness-mock/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentic-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentrun-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/hub-shim/package-lock.jsonis excluded by!**/package-lock.jsonclients/ui/package-lock.jsonis excluded by!**/package-lock.json📒 Files selected for processing (68)
README.mdchanges/unreleased/clients-reference-stack.yamlclients/.gitignoreclients/README.mdclients/deploy/README.mdclients/deploy/gateway/Dockerfileclients/deploy/manifests/gateway.yamlclients/deploy/manifests/ingress.example.yamlclients/deploy/manifests/kustomization.yamlclients/deploy/manifests/ui.yamlclients/deploy/ui/Dockerfileclients/deploy/ui/nginx.confclients/docs/DEMO.mdclients/docs/demo/real-run.yamlclients/docs/demo/skill-probe.yamlclients/hack/demo-check.shclients/hack/demo-down.shclients/hack/demo-up.shclients/harness-goose/Dockerfileclients/harness-goose/entrypoint.shclients/harness-mock/Dockerfileclients/harness-mock/package.jsonclients/harness-mock/server.mjsclients/manifests/goose-bedrock.yamlclients/manifests/samples.yamlclients/packages/agentic-client/dev/selfcheck.tsclients/packages/agentic-client/package.jsonclients/packages/agentic-client/src/acp/index.tsclients/packages/agentic-client/src/contract/index.tsclients/packages/agentic-client/src/index.tsclients/packages/agentic-client/src/transport-shim/index.tsclients/packages/agentic-client/tsconfig.jsonclients/packages/agentrun-client/dev/demo.tsclients/packages/agentrun-client/dev/local-smoke.tsclients/packages/agentrun-client/package.jsonclients/packages/agentrun-client/src/acp.tsclients/packages/agentrun-client/src/index.tsclients/packages/agentrun-client/src/kube.tsclients/packages/agentrun-client/src/portforward.tsclients/packages/agentrun-client/src/types.tsclients/packages/agentrun-client/tsconfig.jsonclients/packages/hub-shim/dev/browser-smoke.tsclients/packages/hub-shim/dev/dial-check.tsclients/packages/hub-shim/dev/drop-check.tsclients/packages/hub-shim/package.jsonclients/packages/hub-shim/src/acp-dial.tsclients/packages/hub-shim/src/server.tsclients/packages/hub-shim/tsconfig.jsonclients/ui/.gitignoreclients/ui/README.mdclients/ui/index.htmlclients/ui/package.jsonclients/ui/src/App.tsxclients/ui/src/app.cssclients/ui/src/components/ChatPanel.tsxclients/ui/src/components/CreateRunModal.tsxclients/ui/src/components/PhaseLabel.tsxclients/ui/src/components/RunDetailPage.tsxclients/ui/src/components/RunsPage.tsxclients/ui/src/format.tsclients/ui/src/main.tsxclients/ui/src/vite-env.d.tsclients/ui/tsconfig.app.jsonclients/ui/tsconfig.jsonclients/ui/tsconfig.node.jsonclients/ui/vite.config.tsdocs/adr/0004-client-contract-and-transports.mddocs/adr/0005-platform-resolved-params.md
ibolton336
force-pushed
the
clients-reference-stack
branch
from
July 20, 2026 19:10
310f00c to
7e54526
Compare
This was referenced Jul 20, 2026
djzager
pushed a commit
that referenced
this pull request
Jul 21, 2026
Fixes #48. The `changelog` job runs under `pull_request_target` and checked out `github.event.pull_request.head.sha`, which actions/checkout now refuses for fork PRs ("Refusing to check out fork pull request code from a 'pull_request_target' workflow"). Every fork PR of type feature/bugfix/breaking failed the check — e.g. [this run](https://github.com/konveyor/agentic-controller/actions/runs/29768454525/job/88440535802) on PR #35. Since the job only diffs file names and never executes PR code, this checks out the **base** repo and fetches the PR head as a plain ref (`git fetch origin pull/<n>/head:pr-head`), then diffs `origin/<base>...pr-head`. Fork code is never checked out into the workspace, so no `allow-unsafe-pr-checkout: true` escape hatch is needed and the workflow keeps its safety guarantees. Verification: YAML validated; the diff expression is unchanged apart from `HEAD` → `pr-head`. The fix itself can only be fully exercised by a fork PR against a repo where this workflow already runs on main (pull_request_target uses the base branch's workflow definition), so the real proof will be the next fork PR after this merges. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Improved pull request checks for changelog fragments. * Changelog verification now more reliably compares proposed changes with the target branch. * Updated validation behavior helps ensure checks run consistently without affecting the application experience. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: ibolton336 <ibolton@redhat.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ibolton336
force-pushed
the
clients-reference-stack
branch
from
July 21, 2026 01:42
b5215e3 to
130a052
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
clients/packages/agentrun-client/dev/local-smoke.ts (1)
11-15: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider using
||instead of??for environment variable fallbacks.If
process.env.PORTorprocess.env.GOOSE_SERVER__SECRET_KEYare set to empty strings, the nullish coalescing operator (??) will not trigger the fallback. ForPORT,Number("")evaluates to0, which is likely unintended. Using||ensures a proper fallback for empty strings.💡 Proposed fix
const target = { host: "127.0.0.1", - port: Number(process.env.PORT ?? 4100), - secretKey: process.env.GOOSE_SERVER__SECRET_KEY ?? "localtest", + port: Number(process.env.PORT || 4100), + secretKey: process.env.GOOSE_SERVER__SECRET_KEY || "localtest", };🤖 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 `@clients/packages/agentrun-client/dev/local-smoke.ts` around lines 11 - 15, Update the target configuration’s environment fallbacks for PORT and GOOSE_SERVER__SECRET_KEY to use || instead of ??, so empty-string values select the existing defaults before port conversion or secret assignment.clients/deploy/manifests/ui.yaml (1)
21-40: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd explicit securityContext hardening to the UI container.
Lower risk than the gateway (no RBAC/secrets access), but still missing
securityContextat pod/container level. Note: if you setreadOnlyRootFilesystem: true, nginx will need writableemptyDirmounts for its cache/pid dirs (e.g./var/cache/nginx,/tmp,/var/run).🔒 Proposed fix
spec: + securityContext: + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault containers: - name: ui image: agentic-ui:dev imagePullPolicy: IfNotPresent + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: ["ALL"] ports: - containerPort: 8080 name: httpAs per static analysis hints (Trivy KSV-0014/KSV-0118, Checkov CKV_K8S_20/CKV_K8S_23).
🤖 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 `@clients/deploy/manifests/ui.yaml` around lines 21 - 40, Add explicit securityContext hardening for the UI container identified by the `name: ui` entry: run it as non-root, disallow privilege escalation, drop unnecessary Linux capabilities, and set the root filesystem read-only. Add writable `emptyDir` mounts for nginx runtime/cache paths such as `/var/cache/nginx`, `/tmp`, and `/var/run` so the existing nginx process continues to function.Source: Linters/SAST tools
clients/packages/hub-shim/src/server.ts (1)
374-387: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winApplication-identity
envFromisn't markedoptional, unlike the model-provider path.
resolveModels()(Line 446) deliberately setssecretRef.optional: trueso a missing provider Secret doesn't wedge pod creation. The identity-credential injection here (Line 381) uses a baresecretRef: { name: app.identitySecret }. If anIDENTITY_SECRET_BRIDGEentry ever points at a Secret that hasn't been created yet, the run's pod will fail withCreateContainerConfigErrorinstead of degrading gracefully the way the model path does.🔧 Proposed fix
if (app.identitySecret) { - resolved.envFrom.push({ secretRef: { name: app.identitySecret } }); + resolved.envFrom.push({ secretRef: { name: app.identitySecret, optional: true } }); } else {🤖 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 `@clients/packages/hub-shim/src/server.ts` around lines 374 - 387, Update the application-identity credential injection in the credentialSources loop to mark the generated secretRef as optional, matching the optional secret reference behavior in resolveModels(). Preserve the existing identitySecret check and skip/log behavior.clients/harness-mock/Dockerfile (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning installs with a committed lockfile +
npm ci.Only
package.jsonis copied andnpm installis used, so the resolved dependency tree isn't reproducible across builds/environments. Ifclients/harness-mockhas (or gets) apackage-lock.json, copy it too and switch tonpm ci --omit=devfor deterministic, cache-friendly builds.🤖 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 `@clients/harness-mock/Dockerfile` around lines 3 - 4, Update the clients/harness-mock Dockerfile dependency installation to copy the committed package-lock.json alongside package.json and replace npm install with npm ci --omit=dev, preserving the existing production-only installation behavior.clients/harness-goose/Dockerfile (1)
1-45: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueBoth harness images run as root. Neither Dockerfile drops privileges before
CMD; the shared fix is adding a non-rootUSERto each.
clients/harness-goose/Dockerfile#L1-L45: add a dedicated user (e.g.useradd -m agent),chown/workspaceand/usr/local/bin/agent-entrypoint.sh, thenUSER agentbeforeCMD.clients/harness-mock/Dockerfile#L1-L8: switch to the built-innodeuser (node:24-alpineships one) viaUSER nodebeforeCMD, after ensuring/appis writable by 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 `@clients/harness-goose/Dockerfile` around lines 1 - 45, Both harness images currently run as root; update clients/harness-goose/Dockerfile lines 1-45 to create a dedicated agent user, grant it ownership of /workspace and /usr/local/bin/agent-entrypoint.sh, and set USER agent before CMD. Update clients/harness-mock/Dockerfile lines 1-8 to ensure /app is writable by the built-in node user and set USER node before CMD.Source: Linters/SAST tools
🤖 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 `@clients/deploy/manifests/gateway.yaml`:
- Around line 59-84: Harden both Deployments: in
clients/deploy/manifests/gateway.yaml lines 59-84, add pod-level runAsNonRoot
and RuntimeDefault seccompProfile, plus container-level allowPrivilegeEscalation
false, readOnlyRootFilesystem true, and drop ALL capabilities for gateway; in
clients/deploy/manifests/ui.yaml lines 21-40, add the same pod-level settings
and container-level privilege-escalation and capability restrictions for ui,
adding emptyDir mounts for nginx writable directories if enabling
readOnlyRootFilesystem.
In `@clients/hack/demo-up.sh`:
- Around line 45-54: Update the minikube image build commands in the
acp-mock-harness and goose-harness blocks to use the same explicit failure
handling as the existing npm install commands, invoking die with a clear,
image-specific message when either build fails. Preserve the current directory
changes, build arguments, and success flow.
---
Nitpick comments:
In `@clients/deploy/manifests/ui.yaml`:
- Around line 21-40: Add explicit securityContext hardening for the UI container
identified by the `name: ui` entry: run it as non-root, disallow privilege
escalation, drop unnecessary Linux capabilities, and set the root filesystem
read-only. Add writable `emptyDir` mounts for nginx runtime/cache paths such as
`/var/cache/nginx`, `/tmp`, and `/var/run` so the existing nginx process
continues to function.
In `@clients/harness-goose/Dockerfile`:
- Around line 1-45: Both harness images currently run as root; update
clients/harness-goose/Dockerfile lines 1-45 to create a dedicated agent user,
grant it ownership of /workspace and /usr/local/bin/agent-entrypoint.sh, and set
USER agent before CMD. Update clients/harness-mock/Dockerfile lines 1-8 to
ensure /app is writable by the built-in node user and set USER node before CMD.
In `@clients/harness-mock/Dockerfile`:
- Around line 3-4: Update the clients/harness-mock Dockerfile dependency
installation to copy the committed package-lock.json alongside package.json and
replace npm install with npm ci --omit=dev, preserving the existing
production-only installation behavior.
In `@clients/packages/agentrun-client/dev/local-smoke.ts`:
- Around line 11-15: Update the target configuration’s environment fallbacks for
PORT and GOOSE_SERVER__SECRET_KEY to use || instead of ??, so empty-string
values select the existing defaults before port conversion or secret assignment.
In `@clients/packages/hub-shim/src/server.ts`:
- Around line 374-387: Update the application-identity credential injection in
the credentialSources loop to mark the generated secretRef as optional, matching
the optional secret reference behavior in resolveModels(). Preserve the existing
identitySecret check and skip/log behavior.
🪄 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
Run ID: 12d8f2e5-617d-4e8b-a085-a0c0ad36f38d
⛔ Files ignored due to path filters (5)
clients/harness-mock/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentic-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentrun-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/hub-shim/package-lock.jsonis excluded by!**/package-lock.jsonclients/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (68)
README.mdchanges/unreleased/clients-reference-stack.yamlclients/.gitignoreclients/README.mdclients/deploy/README.mdclients/deploy/gateway/Dockerfileclients/deploy/manifests/gateway.yamlclients/deploy/manifests/ingress.example.yamlclients/deploy/manifests/kustomization.yamlclients/deploy/manifests/ui.yamlclients/deploy/ui/Dockerfileclients/deploy/ui/nginx.confclients/docs/DEMO.mdclients/docs/demo/real-run.yamlclients/docs/demo/skill-probe.yamlclients/hack/demo-check.shclients/hack/demo-down.shclients/hack/demo-up.shclients/harness-goose/Dockerfileclients/harness-goose/entrypoint.shclients/harness-mock/Dockerfileclients/harness-mock/package.jsonclients/harness-mock/server.mjsclients/manifests/goose-bedrock.yamlclients/manifests/samples.yamlclients/packages/agentic-client/dev/selfcheck.tsclients/packages/agentic-client/package.jsonclients/packages/agentic-client/src/acp/index.tsclients/packages/agentic-client/src/contract/index.tsclients/packages/agentic-client/src/index.tsclients/packages/agentic-client/src/transport-shim/index.tsclients/packages/agentic-client/tsconfig.jsonclients/packages/agentrun-client/dev/demo.tsclients/packages/agentrun-client/dev/local-smoke.tsclients/packages/agentrun-client/package.jsonclients/packages/agentrun-client/src/acp.tsclients/packages/agentrun-client/src/index.tsclients/packages/agentrun-client/src/kube.tsclients/packages/agentrun-client/src/portforward.tsclients/packages/agentrun-client/src/types.tsclients/packages/agentrun-client/tsconfig.jsonclients/packages/hub-shim/dev/browser-smoke.tsclients/packages/hub-shim/dev/dial-check.tsclients/packages/hub-shim/dev/drop-check.tsclients/packages/hub-shim/package.jsonclients/packages/hub-shim/src/acp-dial.tsclients/packages/hub-shim/src/server.tsclients/packages/hub-shim/tsconfig.jsonclients/ui/.gitignoreclients/ui/README.mdclients/ui/index.htmlclients/ui/package.jsonclients/ui/src/App.tsxclients/ui/src/app.cssclients/ui/src/components/ChatPanel.tsxclients/ui/src/components/CreateRunModal.tsxclients/ui/src/components/PhaseLabel.tsxclients/ui/src/components/RunDetailPage.tsxclients/ui/src/components/RunsPage.tsxclients/ui/src/format.tsclients/ui/src/main.tsxclients/ui/src/vite-env.d.tsclients/ui/tsconfig.app.jsonclients/ui/tsconfig.jsonclients/ui/tsconfig.node.jsonclients/ui/vite.config.tsdocs/adr/0004-client-contract-and-transports.mddocs/adr/0005-platform-resolved-params.md
🚧 Files skipped from review as they are similar to previous changes (51)
- clients/ui/vite.config.ts
- clients/packages/agentic-client/src/index.ts
- clients/harness-mock/package.json
- clients/docs/demo/real-run.yaml
- clients/ui/src/vite-env.d.ts
- clients/deploy/ui/nginx.conf
- clients/ui/package.json
- clients/.gitignore
- clients/packages/agentrun-client/package.json
- clients/ui/index.html
- clients/packages/agentrun-client/src/index.ts
- clients/ui/src/components/PhaseLabel.tsx
- clients/README.md
- clients/ui/tsconfig.node.json
- clients/ui/tsconfig.app.json
- clients/ui/src/format.ts
- clients/ui/.gitignore
- clients/docs/demo/skill-probe.yaml
- clients/packages/agentic-client/package.json
- changes/unreleased/clients-reference-stack.yaml
- README.md
- clients/ui/src/App.tsx
- clients/ui/src/main.tsx
- clients/packages/agentrun-client/src/portforward.ts
- clients/packages/hub-shim/dev/dial-check.ts
- clients/deploy/manifests/ingress.example.yaml
- clients/packages/agentrun-client/tsconfig.json
- clients/hack/demo-check.sh
- clients/packages/hub-shim/tsconfig.json
- clients/packages/agentrun-client/src/acp.ts
- clients/packages/agentic-client/tsconfig.json
- clients/ui/tsconfig.json
- clients/ui/README.md
- clients/packages/agentic-client/src/transport-shim/index.ts
- clients/packages/agentic-client/dev/selfcheck.ts
- clients/packages/hub-shim/src/acp-dial.ts
- clients/manifests/goose-bedrock.yaml
- clients/deploy/gateway/Dockerfile
- clients/manifests/samples.yaml
- clients/ui/src/components/RunsPage.tsx
- clients/hack/demo-down.sh
- clients/ui/src/components/RunDetailPage.tsx
- clients/packages/agentic-client/src/acp/index.ts
- clients/packages/agentrun-client/dev/demo.ts
- clients/ui/src/components/ChatPanel.tsx
- clients/ui/src/components/CreateRunModal.tsx
- clients/harness-mock/server.mjs
- clients/packages/hub-shim/dev/drop-check.ts
- clients/packages/agentrun-client/src/types.ts
- clients/packages/agentrun-client/src/kube.ts
- clients/packages/agentic-client/src/contract/index.ts
Comment on lines
+59
to
+84
| spec: | ||
| serviceAccountName: agentic-gateway | ||
| containers: | ||
| - name: gateway | ||
| image: agentic-gateway:dev | ||
| imagePullPolicy: IfNotPresent | ||
| ports: | ||
| - containerPort: 7080 | ||
| name: http | ||
| env: | ||
| - name: NAMESPACE | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.namespace | ||
| readinessProbe: | ||
| httpGet: | ||
| path: /healthz | ||
| port: http | ||
| initialDelaySeconds: 2 | ||
| periodSeconds: 5 | ||
| resources: | ||
| requests: | ||
| cpu: 50m | ||
| memory: 128Mi | ||
| limits: | ||
| memory: 512Mi |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing securityContext hardening across both deploy Deployments. Neither agentic-gateway nor agentic-ui sets a pod/container securityContext, so both run with the default (root-capable, writable-rootfs, privilege-escalation-allowed) context. Same fix pattern applies to both; the gateway is higher priority since it also holds RBAC to read secrets and pods.
clients/deploy/manifests/gateway.yaml#L59-L84: addsecurityContext.runAsNonRoot: true+seccompProfile.type: RuntimeDefaultat pod level, andallowPrivilegeEscalation: false,readOnlyRootFilesystem: true,capabilities.drop: ["ALL"]on thegatewaycontainer.clients/deploy/manifests/ui.yaml#L21-L40: add the same pod-levelsecurityContext, andallowPrivilegeEscalation: false+capabilities.drop: ["ALL"]on theuicontainer (if addingreadOnlyRootFilesystem: true, mountemptyDirvolumes for nginx's writable dirs, e.g./var/cache/nginx,/tmp,/var/run).
📍 Affects 2 files
clients/deploy/manifests/gateway.yaml#L59-L84(this comment)clients/deploy/manifests/ui.yaml#L21-L40
🤖 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 `@clients/deploy/manifests/gateway.yaml` around lines 59 - 84, Harden both
Deployments: in clients/deploy/manifests/gateway.yaml lines 59-84, add pod-level
runAsNonRoot and RuntimeDefault seccompProfile, plus container-level
allowPrivilegeEscalation false, readOnlyRootFilesystem true, and drop ALL
capabilities for gateway; in clients/deploy/manifests/ui.yaml lines 21-40, add
the same pod-level settings and container-level privilege-escalation and
capability restrictions for ui, adding emptyDir mounts for nginx writable
directories if enabling readOnlyRootFilesystem.
Source: Linters/SAST tools
Comment on lines
+45
to
+54
| if ! minikube image ls 2>/dev/null | grep -q 'acp-mock-harness:dev'; then | ||
| warn "building acp-mock-harness:dev" | ||
| (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null) | ||
| fi | ||
| ok "acp-mock-harness:dev" | ||
| if ! minikube image ls 2>/dev/null | grep -q 'goose-harness:dev'; then | ||
| warn "building goose-harness:dev" | ||
| (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null) | ||
| fi | ||
| ok "goose-harness:dev" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Make minikube image build failures explicit, consistent with the npm install fix.
If either build fails, set -e will still abort the script, but with minikube's raw output instead of a clear die message — inconsistent with the || die pattern now used for npm install (lines 105-106, 128-129).
🩹 Proposed fix
if ! minikube image ls 2>/dev/null | grep -q 'acp-mock-harness:dev'; then
warn "building acp-mock-harness:dev"
- (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null)
+ (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null) \
+ || die "minikube image build failed for acp-mock-harness:dev"
fi
ok "acp-mock-harness:dev"
if ! minikube image ls 2>/dev/null | grep -q 'goose-harness:dev'; then
warn "building goose-harness:dev"
- (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null)
+ (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null) \
+ || die "minikube image build failed for goose-harness:dev"
fi
ok "goose-harness:dev"📝 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
| if ! minikube image ls 2>/dev/null | grep -q 'acp-mock-harness:dev'; then | |
| warn "building acp-mock-harness:dev" | |
| (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null) | |
| fi | |
| ok "acp-mock-harness:dev" | |
| if ! minikube image ls 2>/dev/null | grep -q 'goose-harness:dev'; then | |
| warn "building goose-harness:dev" | |
| (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null) | |
| fi | |
| ok "goose-harness:dev" | |
| if ! minikube image ls 2>/dev/null | grep -q 'acp-mock-harness:dev'; then | |
| warn "building acp-mock-harness:dev" | |
| (cd "$ROOT/harness-mock" && minikube image build -t acp-mock-harness:dev -f Dockerfile . >/dev/null) \ | |
| || die "minikube image build failed for acp-mock-harness:dev" | |
| fi | |
| ok "acp-mock-harness:dev" | |
| if ! minikube image ls 2>/dev/null | grep -q 'goose-harness:dev'; then | |
| warn "building goose-harness:dev" | |
| (cd "$ROOT/harness-goose" && minikube image build -t goose-harness:dev -f Dockerfile . >/dev/null) \ | |
| || die "minikube image build failed for goose-harness:dev" | |
| fi | |
| ok "goose-harness:dev" |
🤖 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 `@clients/hack/demo-up.sh` around lines 45 - 54, Update the minikube image
build commands in the acp-mock-harness and goose-harness blocks to use the same
explicit failure handling as the existing npm install commands, invoking die
with a clear, image-specific message when either build fails. Preserve the
current directory changes, build arguments, and success flow.
7 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
clients/ui/src/components/PlaybookRunDetailPage.tsx (2)
1-1: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicate, inconsistent duration formatting between the playbook list and detail pages. Both render the same
playbookDuration()seconds value, but with different output for sub-minute durations (45svs0m45s), because the detail page reimplements the formatting inline instead of reusing the list page'sformatSeconds.
clients/ui/src/components/PlaybookRunDetailPage.tsx#L131-136: replace the inline${Math.floor(duration / 60)}m${...}expression with a call to the sharedformatSeconds(duration).clients/ui/src/components/PlaybookRunsPage.tsx#L30-35: exportformatSeconds(it currently isn't exported) soPlaybookRunDetailPage.tsxcan import it alongside the already-importedplaybookDuration.Proposed fix
--- a/clients/ui/src/components/PlaybookRunsPage.tsx +++ b/clients/ui/src/components/PlaybookRunsPage.tsx @@ -function formatSeconds(seconds?: number): string { +export function formatSeconds(seconds?: number): string {--- a/clients/ui/src/components/PlaybookRunDetailPage.tsx +++ b/clients/ui/src/components/PlaybookRunDetailPage.tsx @@ -import { playbookDuration } from "./PlaybookRunsPage"; +import { formatSeconds, playbookDuration } from "./PlaybookRunsPage"; @@ - <DescriptionListDescription> - {duration !== undefined ? `${Math.floor(duration / 60)}m${String(duration % 60).padStart(2, "0")}s` : "—"} - </DescriptionListDescription> + <DescriptionListDescription>{formatSeconds(duration)}</DescriptionListDescription>🤖 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 `@clients/ui/src/components/PlaybookRunDetailPage.tsx` at line 1, Export the existing formatSeconds helper from PlaybookRunsPage, then import and reuse it in PlaybookRunDetailPage wherever playbookDuration is displayed. Replace the detail page’s inline minute/second formatting with formatSeconds(duration) so sub-minute durations match the list page.
60-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePolling continues (as a no-op) indefinitely after
goneis set.Once
gonebecomestrue,refreshis recreated (dependency change) and the effect restartssetInterval, which now just no-ops every 2s forever instead of being torn down. Harmless given the guard, but the interval could simply not be (re)started oncegone.🤖 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 `@clients/ui/src/components/PlaybookRunDetailPage.tsx` around lines 60 - 75, Update the polling effect that invokes refresh so it does not create or restart its interval when gone is true. Ensure the effect depends on gone and cleans up any existing interval when the run becomes gone, while preserving normal polling behavior before that state.clients/ui/src/components/PlaybookRunsPage.tsx (1)
22-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
playbookDuration/formatSecondslook correct;formatSecondsshould be exported for reuse.See consolidated comment —
PlaybookRunDetailPage.tsxreimplements the same seconds-to-Xm YYsformatting inline with slightly different (inconsistent) behavior for sub-minute durations, instead of importing this function the way it already importsplaybookDuration.🤖 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 `@clients/ui/src/components/PlaybookRunsPage.tsx` around lines 22 - 35, Export the existing formatSeconds function alongside playbookDuration, then update PlaybookRunDetailPage to import and reuse it instead of maintaining its inline seconds formatting. Preserve the current formatSeconds behavior for undefined, sub-minute, and minute-plus durations.clients/ui/src/App.tsx (1)
73-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a switch/lookup instead of the 4-way nested ternary for readability.
The
view.kind === "list" ? ... : view.kind === "playbooks" ? ... : view.kind === "playbookDetail" ? ... : (...)chain is functionally correct but getting hard to scan. Aswitch (view.kind)(or a small per-kind render map) would read more clearly as more view kinds are added.🤖 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 `@clients/ui/src/App.tsx` around lines 73 - 125, Refactor the nested view-kind ternary in the App component into a clearer switch or per-kind render lookup keyed by view.kind. Preserve the existing RunsPage, PlaybookRunsPage, PlaybookRunDetailPage, and RunDetailPage props and navigation callbacks, including the detail back-navigation behavior.
🤖 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 `@clients/ui/src/components/PlaybookRunDetailPage.tsx`:
- Around line 131-136: Update the Duration rendering in PlaybookRunDetailPage to
reuse the existing formatSeconds helper used by PlaybookRunsPage instead of
constructing an inline minutes-and-seconds string. Preserve the undefined
fallback of "—" while ensuring identical playbookDuration() values use the same
formatting in list and detail views.
- Around line 66-71: Update the 404 detection in the getPlaybookRun error
handling catch block to match the specific HTTP 404 status, using the same
stricter “HTTP 404” check as RunDetailPage; retain the existing not-found
message check and setFetchError behavior.
---
Nitpick comments:
In `@clients/ui/src/App.tsx`:
- Around line 73-125: Refactor the nested view-kind ternary in the App component
into a clearer switch or per-kind render lookup keyed by view.kind. Preserve the
existing RunsPage, PlaybookRunsPage, PlaybookRunDetailPage, and RunDetailPage
props and navigation callbacks, including the detail back-navigation behavior.
In `@clients/ui/src/components/PlaybookRunDetailPage.tsx`:
- Line 1: Export the existing formatSeconds helper from PlaybookRunsPage, then
import and reuse it in PlaybookRunDetailPage wherever playbookDuration is
displayed. Replace the detail page’s inline minute/second formatting with
formatSeconds(duration) so sub-minute durations match the list page.
- Around line 60-75: Update the polling effect that invokes refresh so it does
not create or restart its interval when gone is true. Ensure the effect depends
on gone and cleans up any existing interval when the run becomes gone, while
preserving normal polling behavior before that state.
In `@clients/ui/src/components/PlaybookRunsPage.tsx`:
- Around line 22-35: Export the existing formatSeconds function alongside
playbookDuration, then update PlaybookRunDetailPage to import and reuse it
instead of maintaining its inline seconds formatting. Preserve the current
formatSeconds behavior for undefined, sub-minute, and minute-plus durations.
🪄 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
Run ID: 6f589f55-ec30-4b39-b310-4980030df577
📒 Files selected for processing (8)
clients/packages/agentic-client/src/contract/index.tsclients/packages/agentic-client/src/transport-shim/index.tsclients/packages/agentrun-client/src/types.tsclients/packages/hub-shim/src/server.tsclients/ui/src/App.tsxclients/ui/src/components/PlaybookRunDetailPage.tsxclients/ui/src/components/PlaybookRunsPage.tsxclients/ui/src/components/RunsPage.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- clients/packages/agentic-client/src/transport-shim/index.ts
- clients/packages/agentrun-client/src/types.ts
- clients/ui/src/components/RunsPage.tsx
- clients/packages/hub-shim/src/server.ts
Comment on lines
+131
to
+136
| <DescriptionListGroup> | ||
| <DescriptionListTerm>Duration</DescriptionListTerm> | ||
| <DescriptionListDescription> | ||
| {duration !== undefined ? `${Math.floor(duration / 60)}m${String(duration % 60).padStart(2, "0")}s` : "—"} | ||
| </DescriptionListDescription> | ||
| </DescriptionListGroup> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Duration formatting diverges from PlaybookRunsPage's formatSeconds for the same data.
For durations under 60s, this inlines 0m45s while formatSeconds (used on the playbook list page for the identical playbookDuration() value) renders 45s. Same underlying data, inconsistent presentation between the list and detail views. See consolidated comment.
🤖 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 `@clients/ui/src/components/PlaybookRunDetailPage.tsx` around lines 131 - 136,
Update the Duration rendering in PlaybookRunDetailPage to reuse the existing
formatSeconds helper used by PlaybookRunsPage instead of constructing an inline
minutes-and-seconds string. Preserve the undefined fallback of "—" while
ensuring identical playbookDuration() values use the same formatting in list and
detail views.
ibolton336
added a commit
to ibolton336/agentic-controller
that referenced
this pull request
Jul 29, 2026
goose-bedrock.yaml becomes a konveyor#53 sample (provider named aws-bedrock for the verbatim goose-id mapping, agent-java image, skillCards; no more param-source annotations), samples.yaml drops the identity-Secret bridge and ADR 0005 annotations (mock is a fixture), image-catalog.yaml seeds the agent-image ConfigMap. The deployed gateway Role predated playbooks/skills — deployed UIs 403'd on agentplaybooks, agentplaybookruns, skillcards, skillcollections; now covered, plus secrets/configmaps writes for the token Secret and catalog. harness-goose is deprecated (superseded by the konveyor#53 image hierarchy; delete after konveyor#35 rebases onto merged konveyor#53); DEMO.md records the contract update and the API-first seeding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
clients/packages/hub-shim/src/server.ts (1)
1440-1453: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winWildcard CORS now fronts write routes. With
Access-Control-Allow-Origin: *and no authentication, this release addsPOST/PUT/DELETEfor agents, skillcards, skillcollections, playbooks and both run kinds — any page a developer visits can script cluster mutations against a locally running shim. Consider gating writes behind an origin allowlist (e.g.SHIM_ALLOWED_ORIGINS, defaulting to the UI origin) and keeping*for reads only, or document this as strictly loopback-only tooling in the deploy README.🤖 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 `@clients/packages/hub-shim/src/server.ts` around lines 1440 - 1453, The CORS configuration in the request handler exposes unauthenticated write routes to any browser origin. Update the handler around the Access-Control-Allow-Origin setup to restrict POST, PUT, and DELETE requests using an origin allowlist such as SHIM_ALLOWED_ORIGINS with the UI origin as the default, while preserving wildcard access for read requests; alternatively, explicitly document and enforce that the shim is loopback-only in the deploy README.
🧹 Nitpick comments (7)
clients/ui/src/components/SkillsPage.tsx (1)
127-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
MANAGED_LABELconstant instead of the hardcoded label string.
AgentsPage.tsxrenders this same notice fromMANAGED_LABEL(imported from@konveyor/agentic-client/contract); hardcodingkonveyor.io/managed=truehere drifts if the contract constant ever changes.🤖 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 `@clients/ui/src/components/SkillsPage.tsx` around lines 127 - 133, Update the Alert title in SkillsPage to derive the managed-resource label from the MANAGED_LABEL constant imported from `@konveyor/agentic-client/contract`, matching the existing AgentsPage pattern, and remove the hardcoded konveyor.io/managed=true value.clients/ui/src/components/PlaybookComposerModal.tsx (1)
60-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStage-name validation checks the untrimmed value while the duplicate check uses the trimmed one.
invalidNameReason(line 51) normalizes withtrim(); herename.lengthandSTAGE_NAME_PATTERN.test(name)run on the raw input, so"plan "fails the pattern whilesubmit()(line 207) would have sent the trimmed value. Validate the trimmed name for consistency.♻️ Proposed change
function invalidStageNameReason(name: string, duplicate: boolean): string | undefined { - if (!name.trim()) return "a stage name is required"; - if (name.length > 63) return "at most 63 characters"; - if (!STAGE_NAME_PATTERN.test(name)) { + const n = name.trim(); + if (!n) return "a stage name is required"; + if (n.length > 63) return "at most 63 characters"; + if (!STAGE_NAME_PATTERN.test(n)) {🤖 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 `@clients/ui/src/components/PlaybookComposerModal.tsx` around lines 60 - 68, Update invalidStageNameReason to trim the stage name once and use the normalized value for the length and STAGE_NAME_PATTERN checks, matching the trimmed value used by submit() and duplicate validation. Preserve the existing required-name, duplicate, and validation messages.clients/ui/src/components/PlaybookRunDetailPage.tsx (1)
104-128: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueInterval keeps firing after
gone.Once
goneis set,refresh()returns immediately but the 2s interval keeps running for the lifetime of the page. Clearing it ongoneis cheap.♻️ Proposed change
useEffect(() => { + if (gone) return; void refresh(); const timer = setInterval(() => void refresh(), POLL_MS); return () => clearInterval(timer); - }, [refresh]); + }, [refresh, gone]);🤖 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 `@clients/ui/src/components/PlaybookRunDetailPage.tsx` around lines 104 - 128, Update the polling useEffect around refresh and POLL_MS so the interval is cleared when gone becomes true, rather than continuing to invoke refresh for the page lifetime. Preserve the initial refresh and existing cleanup behavior while ensuring the effect reacts to the gone state.clients/ui/src/components/BranchPanel.tsx (1)
69-160: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
filePresencesurvives repo/branch changes and can show stale "present" labels.The effect re-runs when
owner/repo/targetBranchchange, butfilePresence(andcommits/feed) keep the previous branch's values until the new requests land, soPLAN.md presentcan be attributed to the wrong branch. Reset the derived state at the top of the effect.♻️ Proposed reset
useEffect(() => { if (!owner || !repo) return; + setFilePresence({}); + setCommits(null); + setFeed("loading"); let disposed = false;🤖 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 `@clients/ui/src/components/BranchPanel.tsx` around lines 69 - 160, Reset the branch-specific derived state at the start of the useEffect that polls commits: clear filePresence, set commits back to null, and set feed to "loading" before issuing requests. Keep this reset tied to owner, repo, and targetBranch changes so stale values from the previous branch cannot be displayed while the new fetch is pending.clients/ui/src/components/RunDetailPage.tsx (1)
123-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicated rerun branch gating and modal across both detail pages.
openRerun,rerunBranchReason,canRerun, and the entire "Re-run (create a new run)" modal (target-branchTextInput, helper text, "Generate new" button, footer) are identical in both files apart from field ids and which create call runs; the same is true ofconfirmDelete. Divergence here means one page silently drifts from the shim's validation rules.
clients/ui/src/components/RunDetailPage.tsx#L123-L139: extract the branch state/validation into a shared hook (e.g.useRerunBranch(coordinates, application)) and aRerunModalcomponent takingidPrefixplus anonSubmitcallback, then consume them here.clients/ui/src/components/PlaybookRunDetailPage.tsx#L149-L165: consume the same hook/component, passing thecreatePlaybookRunsubmit handler.🤖 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 `@clients/ui/src/components/RunDetailPage.tsx` around lines 123 - 139, Extract the duplicated rerun branch state and validation from RunDetailPage.tsx lines 123-139 and PlaybookRunDetailPage.tsx lines 149-165 into a shared useRerunBranch hook, and extract the identical rerun modal into a RerunModal component accepting idPrefix and onSubmit. Update both pages to consume the shared hook and modal, preserving each page’s existing create-run submit handler and field IDs; also consolidate the duplicated confirmDelete flow if it is identical.clients/packages/hub-shim/src/defaults.ts (1)
111-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeeded provider declares no
primary-tier model.resolveProviderModelpreferstier === "primary"and only falls back to the first entry, so the sole"premium"model works by accident. Tagging itprimarymakes the default policy explicit.🏷️ Proposed tweak
- models: [{ name: "claude-sonnet-4-5", contextWindow: 200000, tier: "premium" }], + models: [{ name: "claude-sonnet-4-5", contextWindow: 200000, tier: "primary" }],🤖 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 `@clients/packages/hub-shim/src/defaults.ts` around lines 111 - 148, Update the sole model definition in defaultResources for SEED_PROVIDER so its tier is “primary” instead of “premium”. Leave the provider endpoint, credentials, model name, and context window unchanged.clients/ui/src/format.ts (1)
52-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate host-detection/normalization logic between
repoBranchUrlandrepoFileUrl.Both functions repeat the same trim/strip-trailing-slash/strip-
.gitnormalization and the same GitHub/GitLab host detection. Extracting a shared helper would make adding a third host (e.g., Bitbucket) a one-place change.♻️ Proposed refactor
+function gitHostSegment(repoUrl: string, kind: "tree" | "blob"): { base: string; segment: string } | undefined { + const base = repoUrl.trim().replace(/\/+$/, "").replace(/\.git$/, ""); + if (parseGitHubRepo(repoUrl)) return { base, segment: kind }; + if (/^https?:\/\/(?:www\.)?gitlab\.com\//.test(base)) return { base, segment: `-/${kind}` }; + return undefined; +} + export function repoBranchUrl(repoUrl: string | undefined, branch: string): string | undefined { if (!repoUrl) return undefined; - const base = repoUrl.trim().replace(/\/+$/, "").replace(/\.git$/, ""); - const gh = parseGitHubRepo(repoUrl); - if (gh) return `${base}/tree/${encodeURIComponent(branch)}`; - if (/^https?:\/\/(?:www\.)?gitlab\.com\//.test(base)) { - return `${base}/-/tree/${encodeURIComponent(branch)}`; - } - return undefined; + const h = gitHostSegment(repoUrl, "tree"); + return h ? `${h.base}/${h.segment}/${encodeURIComponent(branch)}` : undefined; }🤖 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 `@clients/ui/src/format.ts` around lines 52 - 80, Extract the shared repository URL normalization and GitHub/GitLab host detection used by repoBranchUrl and repoFileUrl into a private helper. Update both functions to consume that helper while preserving their existing branch and file URL formats, so future host support is added in one place.
🤖 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 `@clients/deploy/manifests/gateway.yaml`:
- Around line 30-41: The gateway Role currently grants broad Secret
get/create/update access, allowing it to read provider credentials; restrict the
secrets permissions in the Role around the existing ACP key and hub-token
handling. Use name-restricted rules for fixed Secret names where possible, avoid
granting read access to unrelated provider credential Secrets, and preserve only
the minimum permissions required for dynamic per-run ACP Secrets and hub-token
upserts.
In `@clients/docs/DEMO.md`:
- Around line 22-23: Update the adjacent blockquotes in the Phase 2 note in
DEMO.md by adding a blank separator, using a bare “>” line or another
non-blockquote separator so markdownlint MD028 passes.
In `@clients/packages/hub-shim/src/server.ts`:
- Around line 864-897: Trim validated identifiers before returning them from
parseCreateRunBody: return normalized agentRef and applicationRef values while
preserving the existing non-empty validation. Apply the same normalization to
playbookRef and applicationRef in parseCreatePlaybookRunBody so downstream CR
and inventory lookups receive trimmed identifiers.
- Around line 269-278: Update hubGet to enforce a request timeout by creating an
AbortController, scheduling its abort after the same timeout budget used by
ShimClient.send, and passing the controller’s signal to fetch. Ensure the timer
is cleaned up after completion so stalled Hub requests reject and existing
fallback handling can run.
In `@clients/ui/src/components/CreateRunModal.tsx`:
- Around line 94-133: Make the fatal skill/application mismatch checks block
creation: update canCreate to require both !skillless and !skilledNoApp
alongside the existing validation guards. Keep the existing skillless and
skilledNoApp alert conditions and messaging unchanged.
In `@clients/ui/src/components/PlaybookComposerModal.tsx`:
- Around line 137-147: Update the provider-overlap advisory in
PlaybookComposerModal to compare the full provider reference lists for all
referenced stage agents and flag only when their intersection is empty, matching
the run-creation logic. Replace the single first-provider values in the advisory
data with each stage’s full provider list, and update the alert rendering to
display those lists.
In `@clients/ui/src/components/SkillCollectionModal.tsx`:
- Around line 89-100: Update the nextKey initialization alongside members and
hadSourceMembers to access collection.spec.skills defensively, using the same
empty-array fallback before reading length. Preserve the existing initial key
value when skills are present and use zero when skills is absent.
---
Outside diff comments:
In `@clients/packages/hub-shim/src/server.ts`:
- Around line 1440-1453: The CORS configuration in the request handler exposes
unauthenticated write routes to any browser origin. Update the handler around
the Access-Control-Allow-Origin setup to restrict POST, PUT, and DELETE requests
using an origin allowlist such as SHIM_ALLOWED_ORIGINS with the UI origin as the
default, while preserving wildcard access for read requests; alternatively,
explicitly document and enforce that the shim is loopback-only in the deploy
README.
---
Nitpick comments:
In `@clients/packages/hub-shim/src/defaults.ts`:
- Around line 111-148: Update the sole model definition in defaultResources for
SEED_PROVIDER so its tier is “primary” instead of “premium”. Leave the provider
endpoint, credentials, model name, and context window unchanged.
In `@clients/ui/src/components/BranchPanel.tsx`:
- Around line 69-160: Reset the branch-specific derived state at the start of
the useEffect that polls commits: clear filePresence, set commits back to null,
and set feed to "loading" before issuing requests. Keep this reset tied to
owner, repo, and targetBranch changes so stale values from the previous branch
cannot be displayed while the new fetch is pending.
In `@clients/ui/src/components/PlaybookComposerModal.tsx`:
- Around line 60-68: Update invalidStageNameReason to trim the stage name once
and use the normalized value for the length and STAGE_NAME_PATTERN checks,
matching the trimmed value used by submit() and duplicate validation. Preserve
the existing required-name, duplicate, and validation messages.
In `@clients/ui/src/components/PlaybookRunDetailPage.tsx`:
- Around line 104-128: Update the polling useEffect around refresh and POLL_MS
so the interval is cleared when gone becomes true, rather than continuing to
invoke refresh for the page lifetime. Preserve the initial refresh and existing
cleanup behavior while ensuring the effect reacts to the gone state.
In `@clients/ui/src/components/RunDetailPage.tsx`:
- Around line 123-139: Extract the duplicated rerun branch state and validation
from RunDetailPage.tsx lines 123-139 and PlaybookRunDetailPage.tsx lines 149-165
into a shared useRerunBranch hook, and extract the identical rerun modal into a
RerunModal component accepting idPrefix and onSubmit. Update both pages to
consume the shared hook and modal, preserving each page’s existing create-run
submit handler and field IDs; also consolidate the duplicated confirmDelete flow
if it is identical.
In `@clients/ui/src/components/SkillsPage.tsx`:
- Around line 127-133: Update the Alert title in SkillsPage to derive the
managed-resource label from the MANAGED_LABEL constant imported from
`@konveyor/agentic-client/contract`, matching the existing AgentsPage pattern, and
remove the hardcoded konveyor.io/managed=true value.
In `@clients/ui/src/format.ts`:
- Around line 52-80: Extract the shared repository URL normalization and
GitHub/GitLab host detection used by repoBranchUrl and repoFileUrl into a
private helper. Update both functions to consume that helper while preserving
their existing branch and file URL formats, so future host support is added in
one place.
🪄 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: f9305e50-bcaf-4d48-9578-dbae8a5deadc
📒 Files selected for processing (32)
clients/deploy/manifests/gateway.yamlclients/docs/DEMO.mdclients/hack/demo-up.shclients/harness-goose/Dockerfileclients/manifests/goose-bedrock.yamlclients/manifests/image-catalog.yamlclients/manifests/samples.yamlclients/packages/agentic-client/dev/selfcheck.tsclients/packages/agentic-client/src/acp/index.tsclients/packages/agentic-client/src/contract/index.tsclients/packages/agentic-client/src/transport-shim/index.tsclients/packages/agentrun-client/src/types.tsclients/packages/hub-shim/src/defaults.tsclients/packages/hub-shim/src/server.tsclients/ui/src/App.tsxclients/ui/src/components/AgentDesignerModal.tsxclients/ui/src/components/AgentsPage.tsxclients/ui/src/components/BranchPanel.tsxclients/ui/src/components/CreatePlaybookRunModal.tsxclients/ui/src/components/CreateRunModal.tsxclients/ui/src/components/LoadDefaultsButton.tsxclients/ui/src/components/PlaybookComposerModal.tsxclients/ui/src/components/PlaybookRunDetailPage.tsxclients/ui/src/components/PlaybookRunsPage.tsxclients/ui/src/components/PlaybooksPage.tsxclients/ui/src/components/RunDetailPage.tsxclients/ui/src/components/RunsPage.tsxclients/ui/src/components/SkillCardModal.tsxclients/ui/src/components/SkillCollectionModal.tsxclients/ui/src/components/SkillsPage.tsxclients/ui/src/components/sources.tsxclients/ui/src/format.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- clients/packages/agentic-client/dev/selfcheck.ts
- clients/harness-goose/Dockerfile
- clients/packages/agentrun-client/src/types.ts
- clients/ui/src/components/RunsPage.tsx
- clients/hack/demo-up.sh
- clients/packages/agentic-client/src/acp/index.ts
Comment on lines
+30
to
+41
| # get: ACP key Secrets. create/update: the hub-token Secret upsert the | ||
| # defaults seeding performs (ensureHubTokenSecret). | ||
| - apiGroups: [""] | ||
| resources: ["secrets"] | ||
| verbs: ["get", "create", "update"] | ||
| - apiGroups: [""] | ||
| resources: ["pods"] | ||
| verbs: ["get", "list"] | ||
| # Agent-image catalog ConfigMap (GET /api/images, seeded by /api/defaults). | ||
| - apiGroups: [""] | ||
| resources: ["configmaps"] | ||
| verbs: ["get", "create"] |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚖️ Poor tradeoff
Unrestricted secrets get/update in the namespace lets the gateway read provider credentials.
The comment scopes this rule to per-run ACP key Secrets plus the hub-token upsert, but the rule grants get/update on every Secret in konveyor-agents — including aws-bedrock-creds from clients/manifests/goose-bedrock.yaml. Since per-run ACP Secret names are dynamic, resourceNames only helps for the fixed hub-token Secret; consider splitting this into a name-restricted rule for update (and the known fixed names) and keeping credential Secrets out of this namespace, or accept and document the trust boundary explicitly.
🤖 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 `@clients/deploy/manifests/gateway.yaml` around lines 30 - 41, The gateway Role
currently grants broad Secret get/create/update access, allowing it to read
provider credentials; restrict the secrets permissions in the Role around the
existing ACP key and hub-token handling. Use name-restricted rules for fixed
Secret names where possible, avoid granting read access to unrelated provider
credential Secrets, and preserve only the minimum permissions required for
dynamic per-run ACP Secrets and hub-token upserts.
Comment on lines
+269
to
+278
| async function hubGet<T>(path: string): Promise<T> { | ||
| const res = await fetch(`${HUB_URL}/${path}`, { | ||
| headers: { | ||
| accept: "application/json", | ||
| ...(HUB_TOKEN ? { authorization: `Bearer ${HUB_TOKEN}` } : {}), | ||
| }, | ||
| }); | ||
| if (!res.ok) throw new Error(`Hub GET /${path} -> HTTP ${res.status}`); | ||
| return (await res.json()) as T; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the Hub fetch. hubGet has no abort signal, so a stalled/blackholed Hub keeps GET /api/applications and every application-scoped run create (hubEnvForRun → getApplications) hanging indefinitely — the stub fallback only triggers on an actual rejection. Mirrors the budget already added in ShimClient.send.
⏱️ Proposed fix
+/** Abort Hub reads that haven't answered within this budget. */
+const HUB_TIMEOUT_MS = 10_000;
+
async function hubGet<T>(path: string): Promise<T> {
const res = await fetch(`${HUB_URL}/${path}`, {
headers: {
accept: "application/json",
...(HUB_TOKEN ? { authorization: `Bearer ${HUB_TOKEN}` } : {}),
},
+ signal: AbortSignal.timeout(HUB_TIMEOUT_MS),
});📝 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
| async function hubGet<T>(path: string): Promise<T> { | |
| const res = await fetch(`${HUB_URL}/${path}`, { | |
| headers: { | |
| accept: "application/json", | |
| ...(HUB_TOKEN ? { authorization: `Bearer ${HUB_TOKEN}` } : {}), | |
| }, | |
| }); | |
| if (!res.ok) throw new Error(`Hub GET /${path} -> HTTP ${res.status}`); | |
| return (await res.json()) as T; | |
| } | |
| /** Abort Hub reads that haven't answered within this budget. */ | |
| const HUB_TIMEOUT_MS = 10_000; | |
| async function hubGet<T>(path: string): Promise<T> { | |
| const res = await fetch(`${HUB_URL}/${path}`, { | |
| headers: { | |
| accept: "application/json", | |
| ...(HUB_TOKEN ? { authorization: `Bearer ${HUB_TOKEN}` } : {}), | |
| }, | |
| signal: AbortSignal.timeout(HUB_TIMEOUT_MS), | |
| }); | |
| if (!res.ok) throw new Error(`Hub GET /${path} -> HTTP ${res.status}`); | |
| return (await res.json()) as T; | |
| } |
🤖 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 `@clients/packages/hub-shim/src/server.ts` around lines 269 - 278, Update
hubGet to enforce a request timeout by creating an AbortController, scheduling
its abort after the same timeout budget used by ShimClient.send, and passing the
controller’s signal to fetch. Ensure the timer is cleaned up after completion so
stalled Hub requests reject and existing fallback handling can run.
Comment on lines
+864
to
+897
| /** Validates the POST /api/agentruns body; throws with a client-facing message. */ | ||
| function parseCreateRunBody(raw: unknown): CreateRunBody { | ||
| if (!raw || typeof raw !== "object" || Array.isArray(raw)) { | ||
| badRequest( | ||
| "body must be a JSON object: {agentRef, params?, instructions?, applicationRef?, targetBranch?, model?}", | ||
| ); | ||
| } | ||
| const body = raw as Record<string, unknown>; | ||
| if (typeof body.agentRef !== "string" || body.agentRef.trim() === "") { | ||
| badRequest("agentRef is required and must be a non-empty string"); | ||
| } | ||
| const params = parseParamsField(body); | ||
| if (body.instructions !== undefined && typeof body.instructions !== "string") { | ||
| badRequest("instructions must be a string"); | ||
| } | ||
| if ( | ||
| body.applicationRef !== undefined && | ||
| (typeof body.applicationRef !== "string" || body.applicationRef.trim() === "") | ||
| ) { | ||
| badRequest("applicationRef must be a non-empty string"); | ||
| } | ||
| const targetBranch = parseTargetBranchField(body); | ||
| if (targetBranch !== undefined && body.applicationRef === undefined) { | ||
| badRequest("targetBranch is only meaningful with applicationRef"); | ||
| } | ||
| return { | ||
| agentRef: body.agentRef, | ||
| params, | ||
| instructions: body.instructions as string | undefined, | ||
| applicationRef: body.applicationRef as string | undefined, | ||
| targetBranch, | ||
| model: parseModelField(body), | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim agentRef/applicationRef like the other fields. Emptiness is checked after trim() but the untrimmed value is returned, so " my-agent " reaches the CR as an invalid agentRef, and a padded applicationRef fails the inventory find with a misleading "unknown applicationRef".
✂️ Proposed fix
return {
- agentRef: body.agentRef,
+ agentRef: body.agentRef.trim(),
params,
instructions: body.instructions as string | undefined,
- applicationRef: body.applicationRef as string | undefined,
+ applicationRef: (body.applicationRef as string | undefined)?.trim(),
targetBranch,
model: parseModelField(body),
};The same applies to playbookRef/applicationRef in parseCreatePlaybookRunBody (Lines 934-940).
📝 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
| /** Validates the POST /api/agentruns body; throws with a client-facing message. */ | |
| function parseCreateRunBody(raw: unknown): CreateRunBody { | |
| if (!raw || typeof raw !== "object" || Array.isArray(raw)) { | |
| badRequest( | |
| "body must be a JSON object: {agentRef, params?, instructions?, applicationRef?, targetBranch?, model?}", | |
| ); | |
| } | |
| const body = raw as Record<string, unknown>; | |
| if (typeof body.agentRef !== "string" || body.agentRef.trim() === "") { | |
| badRequest("agentRef is required and must be a non-empty string"); | |
| } | |
| const params = parseParamsField(body); | |
| if (body.instructions !== undefined && typeof body.instructions !== "string") { | |
| badRequest("instructions must be a string"); | |
| } | |
| if ( | |
| body.applicationRef !== undefined && | |
| (typeof body.applicationRef !== "string" || body.applicationRef.trim() === "") | |
| ) { | |
| badRequest("applicationRef must be a non-empty string"); | |
| } | |
| const targetBranch = parseTargetBranchField(body); | |
| if (targetBranch !== undefined && body.applicationRef === undefined) { | |
| badRequest("targetBranch is only meaningful with applicationRef"); | |
| } | |
| return { | |
| agentRef: body.agentRef, | |
| params, | |
| instructions: body.instructions as string | undefined, | |
| applicationRef: body.applicationRef as string | undefined, | |
| targetBranch, | |
| model: parseModelField(body), | |
| }; | |
| } | |
| /** Validates the POST /api/agentruns body; throws with a client-facing message. */ | |
| function parseCreateRunBody(raw: unknown): CreateRunBody { | |
| if (!raw || typeof raw !== "object" || Array.isArray(raw)) { | |
| badRequest( | |
| "body must be a JSON object: {agentRef, params?, instructions?, applicationRef?, targetBranch?, model?}", | |
| ); | |
| } | |
| const body = raw as Record<string, unknown>; | |
| if (typeof body.agentRef !== "string" || body.agentRef.trim() === "") { | |
| badRequest("agentRef is required and must be a non-empty string"); | |
| } | |
| const params = parseParamsField(body); | |
| if (body.instructions !== undefined && typeof body.instructions !== "string") { | |
| badRequest("instructions must be a string"); | |
| } | |
| if ( | |
| body.applicationRef !== undefined && | |
| (typeof body.applicationRef !== "string" || body.applicationRef.trim() === "") | |
| ) { | |
| badRequest("applicationRef must be a non-empty string"); | |
| } | |
| const targetBranch = parseTargetBranchField(body); | |
| if (targetBranch !== undefined && body.applicationRef === undefined) { | |
| badRequest("targetBranch is only meaningful with applicationRef"); | |
| } | |
| return { | |
| agentRef: body.agentRef.trim(), | |
| params, | |
| instructions: body.instructions as string | undefined, | |
| applicationRef: (body.applicationRef as string | undefined)?.trim(), | |
| targetBranch, | |
| model: parseModelField(body), | |
| }; | |
| } |
🤖 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 `@clients/packages/hub-shim/src/server.ts` around lines 864 - 897, Trim
validated identifiers before returning them from parseCreateRunBody: return
normalized agentRef and applicationRef values while preserving the existing
non-empty validation. Apply the same normalization to playbookRef and
applicationRef in parseCreatePlaybookRunBody so downstream CR and inventory
lookups receive trimmed identifiers.
Comment on lines
+94
to
+133
| const selected = agents?.find((a) => a.metadata.name === agentName); | ||
|
|
||
| const selectAgent = (name: string) => { | ||
| setAgentName(name); | ||
| setParamValues(defaultsFor(agents?.find((a) => a.metadata.name === name))); | ||
| setModel(null); | ||
| }; | ||
|
|
||
| const userParams = selected?.spec.params ?? []; | ||
| // The shim refuses application-scoped creates when the inventory is the | ||
| // offline stub — stub applications have no Hub behind them to pull from. | ||
| const stubInventory = inventory.source === "stub"; | ||
| const application = applications.find((a) => a.id === applicationId); | ||
| const skillless = !!application && skillCount(selected) === 0; | ||
| // The inverse mismatch: an agent that mounts skills runs the migration | ||
| // harness, which hard-requires the Hub coordinates an application brings. | ||
| const skilledNoApp = !application && skillCount(selected) > 0; | ||
| const repoMissing = !!application && !application.repository?.url; | ||
|
|
||
| const allowedProviderRefs = (selected?.spec.providers ?? []).map((p) => p.ref); | ||
|
|
||
| const missingRequired = userParams.filter((p) => p.required && !(paramValues[p.name] ?? "").trim()); | ||
| const paramsInvalid = userParams.some( | ||
| (p) => paramValueInvalidReason(p, paramValues[p.name] ?? "") !== undefined, | ||
| ); | ||
| // Mirror the shim's validation so Create is disabled with a reason instead | ||
| // of failing on submit: with an application, the target branch must be a | ||
| // valid git refname (shared rules) and differ from the source branch. | ||
| const branchInvalid = | ||
| !!application && | ||
| (invalidTargetBranchReason(targetBranch) !== undefined || | ||
| (application.repository?.branch !== undefined && | ||
| targetBranch.trim() === application.repository.branch)); | ||
| const canCreate = | ||
| !!selected && | ||
| missingRequired.length === 0 && | ||
| !paramsInvalid && | ||
| !branchInvalid && | ||
| !repoMissing && | ||
| !submitting; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
skillless/skilledNoApp are described as fatal but don't gate canCreate.
The alerts rendered for these (lines 273-287) state the harness will fail deterministically ("fails fatally with zero skills", "fails at startup without the Hub coordinates"), yet unlike repoMissing (which is danger-styled and does block via !repoMissing in canCreate), these are only warning-styled and don't block submission. Users can click Create straight into a guaranteed failed run.
const canCreate =
!!selected &&
missingRequired.length === 0 &&
!paramsInvalid &&
!branchInvalid &&
!repoMissing &&
+ !skillless &&
+ !skilledNoApp &&
!submitting;📝 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
| const selected = agents?.find((a) => a.metadata.name === agentName); | |
| const selectAgent = (name: string) => { | |
| setAgentName(name); | |
| setParamValues(defaultsFor(agents?.find((a) => a.metadata.name === name))); | |
| setModel(null); | |
| }; | |
| const userParams = selected?.spec.params ?? []; | |
| // The shim refuses application-scoped creates when the inventory is the | |
| // offline stub — stub applications have no Hub behind them to pull from. | |
| const stubInventory = inventory.source === "stub"; | |
| const application = applications.find((a) => a.id === applicationId); | |
| const skillless = !!application && skillCount(selected) === 0; | |
| // The inverse mismatch: an agent that mounts skills runs the migration | |
| // harness, which hard-requires the Hub coordinates an application brings. | |
| const skilledNoApp = !application && skillCount(selected) > 0; | |
| const repoMissing = !!application && !application.repository?.url; | |
| const allowedProviderRefs = (selected?.spec.providers ?? []).map((p) => p.ref); | |
| const missingRequired = userParams.filter((p) => p.required && !(paramValues[p.name] ?? "").trim()); | |
| const paramsInvalid = userParams.some( | |
| (p) => paramValueInvalidReason(p, paramValues[p.name] ?? "") !== undefined, | |
| ); | |
| // Mirror the shim's validation so Create is disabled with a reason instead | |
| // of failing on submit: with an application, the target branch must be a | |
| // valid git refname (shared rules) and differ from the source branch. | |
| const branchInvalid = | |
| !!application && | |
| (invalidTargetBranchReason(targetBranch) !== undefined || | |
| (application.repository?.branch !== undefined && | |
| targetBranch.trim() === application.repository.branch)); | |
| const canCreate = | |
| !!selected && | |
| missingRequired.length === 0 && | |
| !paramsInvalid && | |
| !branchInvalid && | |
| !repoMissing && | |
| !submitting; | |
| const selected = agents?.find((a) => a.metadata.name === agentName); | |
| const selectAgent = (name: string) => { | |
| setAgentName(name); | |
| setParamValues(defaultsFor(agents?.find((a) => a.metadata.name === name))); | |
| setModel(null); | |
| }; | |
| const userParams = selected?.spec.params ?? []; | |
| // The shim refuses application-scoped creates when the inventory is the | |
| // offline stub — stub applications have no Hub behind them to pull from. | |
| const stubInventory = inventory.source === "stub"; | |
| const application = applications.find((a) => a.id === applicationId); | |
| const skillless = !!application && skillCount(selected) === 0; | |
| // The inverse mismatch: an agent that mounts skills runs the migration | |
| // harness, which hard-requires the Hub coordinates an application brings. | |
| const skilledNoApp = !application && skillCount(selected) > 0; | |
| const repoMissing = !!application && !application.repository?.url; | |
| const allowedProviderRefs = (selected?.spec.providers ?? []).map((p) => p.ref); | |
| const missingRequired = userParams.filter((p) => p.required && !(paramValues[p.name] ?? "").trim()); | |
| const paramsInvalid = userParams.some( | |
| (p) => paramValueInvalidReason(p, paramValues[p.name] ?? "") !== undefined, | |
| ); | |
| // Mirror the shim's validation so Create is disabled with a reason instead | |
| // of failing on submit: with an application, the target branch must be a | |
| // valid git refname (shared rules) and differ from the source branch. | |
| const branchInvalid = | |
| !!application && | |
| (invalidTargetBranchReason(targetBranch) !== undefined || | |
| (application.repository?.branch !== undefined && | |
| targetBranch.trim() === application.repository.branch)); | |
| const canCreate = | |
| !!selected && | |
| missingRequired.length === 0 && | |
| !paramsInvalid && | |
| !branchInvalid && | |
| !repoMissing && | |
| !skillless && | |
| !skilledNoApp && | |
| !submitting; |
🤖 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 `@clients/ui/src/components/CreateRunModal.tsx` around lines 94 - 133, Make the
fatal skill/application mismatch checks block creation: update canCreate to
require both !skillless and !skilledNoApp alongside the existing validation
guards. Keep the existing skillless and skilledNoApp alert conditions and
messaging unchanged.
Comment on lines
+89
to
+100
| const [members, setMembers] = useState<MemberRow[]>(() => | ||
| rowsFrom(collection?.spec.skills ?? []), | ||
| ); | ||
| const nextKey = useRef(collection?.spec.skills.length ?? 0); | ||
| const [cards, setCards] = useState<SkillCard[]>([]); | ||
| const [cardsError, setCardsError] = useState<string | null>(null); | ||
| const [submitting, setSubmitting] = useState(false); | ||
| const [submitError, setSubmitError] = useState<string | null>(null); | ||
|
|
||
| const hadSourceMembers = (collection?.spec.skills ?? []).some( | ||
| (s) => !s.skillCardRef && !s.image && s.source, | ||
| ); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Inconsistent optional access on spec.skills — line 92 can throw where lines 90/98 don't.
Lines 90 and 98 defensively treat spec.skills as possibly absent, but line 92 dereferences .length directly. If a collection ever arrives without skills, the modal crashes during initial render.
🛡️ Proposed fix
- const nextKey = useRef(collection?.spec.skills.length ?? 0);
+ const nextKey = useRef(collection?.spec.skills?.length ?? 0);📝 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
| const [members, setMembers] = useState<MemberRow[]>(() => | |
| rowsFrom(collection?.spec.skills ?? []), | |
| ); | |
| const nextKey = useRef(collection?.spec.skills.length ?? 0); | |
| const [cards, setCards] = useState<SkillCard[]>([]); | |
| const [cardsError, setCardsError] = useState<string | null>(null); | |
| const [submitting, setSubmitting] = useState(false); | |
| const [submitError, setSubmitError] = useState<string | null>(null); | |
| const hadSourceMembers = (collection?.spec.skills ?? []).some( | |
| (s) => !s.skillCardRef && !s.image && s.source, | |
| ); | |
| const [members, setMembers] = useState<MemberRow[]>(() => | |
| rowsFrom(collection?.spec.skills ?? []), | |
| ); | |
| const nextKey = useRef(collection?.spec.skills?.length ?? 0); | |
| const [cards, setCards] = useState<SkillCard[]>([]); | |
| const [cardsError, setCardsError] = useState<string | null>(null); | |
| const [submitting, setSubmitting] = useState(false); | |
| const [submitError, setSubmitError] = useState<string | null>(null); | |
| const hadSourceMembers = (collection?.spec.skills ?? []).some( | |
| (s) => !s.skillCardRef && !s.image && s.source, | |
| ); |
🤖 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 `@clients/ui/src/components/SkillCollectionModal.tsx` around lines 89 - 100,
Update the nextKey initialization alongside members and hadSourceMembers to
access collection.spec.skills defensively, using the same empty-array fallback
before reading length. Preserve the existing initial key value when skills are
present and use zero when skills is absent.
…sses, demo) Imports the working client stack prototyped in ibolton336/agentcontroller-client — the running system behind the contract proposal on konveyor#22 — cleaned of POC scaffolding, as clients/, plus ADRs 0004 (verified client contract and transports) and 0005 (platform-resolved params). Each piece is the reference implementation for a planned stream: clients/packages/agentic-client isomorphic core: contract types, AcpSession, HTTP transport (streams 3, konveyor#22-konveyor#24) clients/packages/hub-shim proposed Hub passthrough surface: run CRUD, ACP proxy, catalog read/write routes, image catalog and defaults (stream 2, konveyor#21) clients/packages/agentrun-client node-side client shaped for konveyor/editor-extensions clients/ui PatternFly SPA: streaming chat with HITL permission round-trips, run launcher, playbook stage ladder, management console clients/harness-goose working reference for the KONVEYOR_* env contract (stream 4, konveyor#25/konveyor#26) clients/harness-mock deterministic ACP agent for e2e fixtures clients/deploy interim in-cluster gateway + UI manifests Verified end to end against the live controller on minikube (Agent Sandbox v0.5.0) with both the deterministic mock agent and a real goose+Bedrock agent, across both run kinds: AgentRun and AgentPlaybookRun (assess -> remediate -> validate sharing one target branch). The client contract tracks the konveyor#53 migration-harness env/param shape; model selection is explicit and validated against each Agent's declared providers. Squashed from the branch's incremental history into the single import commit this PR was always meant to land as. Signed-off-by: ibolton336 <ibolton@redhat.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ibolton336
force-pushed
the
clients-reference-stack
branch
from
July 29, 2026 12:22
9147b7f to
6ab3835
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
clients/ui/src/components/ChatPanel.tsx (1)
434-465: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLCS diff is O(n·m); unbounded diff size could freeze the UI on the permission-approval path.
Nothing caps
oldText/newTextlength before running the quadratic LCS. Consider a size guard that falls back to a simple "N lines changed" summary (or truncates) above a threshold, so a large agent-proposed edit can't stall the main thread right when the user is deciding whether to approve 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 `@clients/ui/src/components/ChatPanel.tsx` around lines 434 - 465, The diffLines function runs an unbounded quadratic LCS computation that can freeze the UI for large permission previews. Add a size threshold before constructing the LCS table, and above it return a bounded fallback such as a summary of the number of changed lines or a truncated diff; preserve the existing detailed LCS behavior for inputs below the threshold.clients/harness-mock/Dockerfile (1)
1-7: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRun the mock harness as non-root. The
nodeimage ships an unprivilegednodeuser; addingUSER nodeclears the Trivy DS-0002 finding and keeps the fixture aligned with the pod security posture used elsewhere.🔒 Proposed fix
COPY server.mjs ./ +USER node EXPOSE 4000 CMD ["node", "server.mjs"]🤖 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 `@clients/harness-mock/Dockerfile` around lines 1 - 7, Add the Dockerfile USER directive to run the mock harness container as the image’s unprivileged node user, placing it after setup commands such as COPY and before CMD. Keep the existing server.mjs startup behavior unchanged.Source: Linters/SAST tools
clients/ui/src/app.css (1)
55-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the deprecated
word-break: break-word. Useoverflow-wrapinstead, which is the standardized property for this behavior.🎨 Proposed fix
white-space: pre-wrap; - word-break: break-word; + overflow-wrap: break-word;🤖 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 `@clients/ui/src/app.css` around lines 55 - 61, Update the .chat-bubble style by replacing the deprecated word-break: break-word declaration with the standardized overflow-wrap property, preserving the current long-word wrapping behavior.Source: Linters/SAST tools
🤖 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 `@clients/deploy/manifests/ui.yaml`:
- Around line 22-40: Add a securityContext to the ui container definition,
configuring it to run as non-root with privilege escalation disabled and
capabilities dropped. Keep the existing nginx-unprivileged port configuration
and other container settings unchanged; do not add readOnlyRootFilesystem unless
the required emptyDir mounts are also defined.
In `@clients/harness-goose/entrypoint.sh`:
- Around line 24-31: Update the clone log in the REPO handling block of
entrypoint.sh to redact any URL userinfo, including usernames and tokens, before
logging the repository address. Keep the original REPO value unchanged for git
clone, and use the sanitized value only in the “cloning … into /workspace”
message.
In `@clients/packages/agentic-client/src/transport-shim/index.ts`:
- Around line 66-72: Update the Transport Shim constructor’s baseUrl validation
to reject URLs with nonempty parsed.search or parsed.hash, then normalize and
store the URL from the parsed URL rather than the original string so query
strings and fragments cannot be retained before appended API routes. Preserve
the existing HTTP/HTTPS protocol validation and trailing-slash removal.
In `@clients/packages/hub-shim/src/server.ts`:
- Line 66: Restrict the shim’s unauthenticated behavior and wildcard CORS to a
verified loopback/dev-only HOST configuration; deployed or non-loopback
configurations must require authentication and avoid
Access-Control-Allow-Origin: *. Apply this consistently to the /api/* routes and
ACP session bridge, preserving the current localhost development behavior.
In `@clients/ui/src/components/ChatPanel.tsx`:
- Around line 267-272: Reset session-scoped state whenever the connection closes
or a reconnect creates a new session: update the localSession.onClosed handler
to clear turnActive and sessionRef.current alongside conn and session, and
ensure the reconnect path around the prior/loadSessionSupported branching clears
items before calling newSession when loadSession is unsupported. Preserve
transcript restoration when loadSession is supported, while preventing stale
permission controls and old content from carrying into the new session.
In `@docs/adr/0004-client-contract-and-transports.md`:
- Around line 111-116: Update the POST /api/agentruns contract row to match the
current hub-shim handler: include optional targetBranch and model in the request
body, document that targetBranch requires applicationRef, and remove the retired
ADR 0005 parameter/credential resolution behavior. Describe applicationRef as
injecting Hub coordinates and TARGET_BRANCH into spec.env, while preserving the
existing response and validation details.
In `@docs/adr/0005-platform-resolved-params.md`:
- Around line 99-126: Add a status note to ADR 0005 clarifying that its
application inventory and identity-to-Secret bridge are historical or retired
for the shipped platform path. State that the current hub-shim application
mapping omits identitySecret and IDENTITY_SECRET_BRIDGE, and that Hub
coordinates are injected through RUN_ENV instead; preserve the ADR’s original
design context while clearly distinguishing it from current behavior.
---
Nitpick comments:
In `@clients/harness-mock/Dockerfile`:
- Around line 1-7: Add the Dockerfile USER directive to run the mock harness
container as the image’s unprivileged node user, placing it after setup commands
such as COPY and before CMD. Keep the existing server.mjs startup behavior
unchanged.
In `@clients/ui/src/app.css`:
- Around line 55-61: Update the .chat-bubble style by replacing the deprecated
word-break: break-word declaration with the standardized overflow-wrap property,
preserving the current long-word wrapping behavior.
In `@clients/ui/src/components/ChatPanel.tsx`:
- Around line 434-465: The diffLines function runs an unbounded quadratic LCS
computation that can freeze the UI for large permission previews. Add a size
threshold before constructing the LCS table, and above it return a bounded
fallback such as a summary of the number of changed lines or a truncated diff;
preserve the existing detailed LCS behavior for inputs below the threshold.
🪄 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: eea2ace6-080b-461d-af01-769d0bef10ed
⛔ Files ignored due to path filters (5)
clients/harness-mock/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentic-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/agentrun-client/package-lock.jsonis excluded by!**/package-lock.jsonclients/packages/hub-shim/package-lock.jsonis excluded by!**/package-lock.jsonclients/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (83)
README.mdchanges/unreleased/clients-reference-stack.yamlclients/.gitignoreclients/README.mdclients/deploy/README.mdclients/deploy/gateway/Dockerfileclients/deploy/manifests/gateway.yamlclients/deploy/manifests/ingress.example.yamlclients/deploy/manifests/kustomization.yamlclients/deploy/manifests/ui.yamlclients/deploy/ui/Dockerfileclients/deploy/ui/nginx.confclients/docs/DEMO.mdclients/docs/demo/real-run.yamlclients/docs/demo/skill-probe.yamlclients/hack/demo-check.shclients/hack/demo-down.shclients/hack/demo-up.shclients/harness-goose/Dockerfileclients/harness-goose/entrypoint.shclients/harness-mock/Dockerfileclients/harness-mock/package.jsonclients/harness-mock/server.mjsclients/manifests/goose-bedrock.yamlclients/manifests/image-catalog.yamlclients/manifests/samples.yamlclients/packages/agentic-client/dev/selfcheck.tsclients/packages/agentic-client/package.jsonclients/packages/agentic-client/src/acp/index.tsclients/packages/agentic-client/src/contract/index.tsclients/packages/agentic-client/src/index.tsclients/packages/agentic-client/src/transport-shim/index.tsclients/packages/agentic-client/tsconfig.jsonclients/packages/agentrun-client/dev/demo.tsclients/packages/agentrun-client/dev/local-smoke.tsclients/packages/agentrun-client/package.jsonclients/packages/agentrun-client/src/acp.tsclients/packages/agentrun-client/src/index.tsclients/packages/agentrun-client/src/kube.tsclients/packages/agentrun-client/src/portforward.tsclients/packages/agentrun-client/src/types.tsclients/packages/agentrun-client/tsconfig.jsonclients/packages/hub-shim/dev/browser-smoke.tsclients/packages/hub-shim/dev/dial-check.tsclients/packages/hub-shim/dev/drop-check.tsclients/packages/hub-shim/package.jsonclients/packages/hub-shim/src/acp-dial.tsclients/packages/hub-shim/src/defaults.tsclients/packages/hub-shim/src/server.tsclients/packages/hub-shim/tsconfig.jsonclients/ui/.gitignoreclients/ui/README.mdclients/ui/index.htmlclients/ui/package.jsonclients/ui/src/App.tsxclients/ui/src/app.cssclients/ui/src/components/AgentDesignerModal.tsxclients/ui/src/components/AgentsPage.tsxclients/ui/src/components/BranchPanel.tsxclients/ui/src/components/ChatPanel.tsxclients/ui/src/components/CreatePlaybookRunModal.tsxclients/ui/src/components/CreateRunModal.tsxclients/ui/src/components/LoadDefaultsButton.tsxclients/ui/src/components/PhaseLabel.tsxclients/ui/src/components/PlaybookComposerModal.tsxclients/ui/src/components/PlaybookRunDetailPage.tsxclients/ui/src/components/PlaybookRunsPage.tsxclients/ui/src/components/PlaybooksPage.tsxclients/ui/src/components/RunDetailPage.tsxclients/ui/src/components/RunsPage.tsxclients/ui/src/components/SkillCardModal.tsxclients/ui/src/components/SkillCollectionModal.tsxclients/ui/src/components/SkillsPage.tsxclients/ui/src/components/sources.tsxclients/ui/src/format.tsclients/ui/src/main.tsxclients/ui/src/vite-env.d.tsclients/ui/tsconfig.app.jsonclients/ui/tsconfig.jsonclients/ui/tsconfig.node.jsonclients/ui/vite.config.tsdocs/adr/0004-client-contract-and-transports.mddocs/adr/0005-platform-resolved-params.md
🚧 Files skipped from review as they are similar to previous changes (64)
- changes/unreleased/clients-reference-stack.yaml
- clients/harness-mock/package.json
- clients/manifests/image-catalog.yaml
- clients/ui/src/main.tsx
- clients/packages/agentrun-client/package.json
- clients/.gitignore
- clients/docs/demo/real-run.yaml
- clients/packages/agentrun-client/dev/local-smoke.ts
- clients/deploy/manifests/kustomization.yaml
- clients/packages/agentic-client/tsconfig.json
- clients/docs/demo/skill-probe.yaml
- clients/deploy/ui/nginx.conf
- clients/packages/agentrun-client/tsconfig.json
- clients/packages/hub-shim/tsconfig.json
- clients/packages/agentrun-client/src/index.ts
- clients/deploy/manifests/ingress.example.yaml
- clients/hack/demo-down.sh
- clients/packages/agentic-client/dev/selfcheck.ts
- clients/packages/hub-shim/package.json
- clients/ui/src/components/PhaseLabel.tsx
- clients/packages/agentic-client/src/index.ts
- clients/README.md
- clients/ui/src/vite-env.d.ts
- clients/ui/vite.config.ts
- clients/ui/README.md
- clients/ui/.gitignore
- clients/ui/tsconfig.app.json
- clients/deploy/gateway/Dockerfile
- clients/packages/agentrun-client/src/portforward.ts
- clients/ui/tsconfig.json
- clients/packages/hub-shim/src/acp-dial.ts
- clients/packages/agentic-client/package.json
- clients/manifests/samples.yaml
- clients/packages/hub-shim/dev/dial-check.ts
- clients/ui/tsconfig.node.json
- clients/ui/src/components/CreatePlaybookRunModal.tsx
- clients/ui/src/components/PlaybookRunsPage.tsx
- clients/packages/hub-shim/dev/drop-check.ts
- clients/manifests/goose-bedrock.yaml
- clients/ui/src/components/LoadDefaultsButton.tsx
- clients/packages/agentrun-client/src/types.ts
- clients/ui/src/components/AgentsPage.tsx
- clients/ui/src/components/SkillCollectionModal.tsx
- clients/packages/agentrun-client/src/acp.ts
- clients/hack/demo-check.sh
- clients/ui/src/components/SkillsPage.tsx
- clients/ui/src/components/AgentDesignerModal.tsx
- clients/ui/src/components/SkillCardModal.tsx
- clients/ui/src/App.tsx
- clients/ui/src/components/PlaybooksPage.tsx
- README.md
- clients/packages/agentrun-client/dev/demo.ts
- clients/ui/src/components/BranchPanel.tsx
- clients/harness-mock/server.mjs
- clients/packages/agentrun-client/src/kube.ts
- clients/ui/src/components/RunDetailPage.tsx
- clients/packages/hub-shim/src/defaults.ts
- clients/packages/agentic-client/src/contract/index.ts
- clients/ui/src/components/PlaybookRunDetailPage.tsx
- clients/ui/src/components/CreateRunModal.tsx
- clients/packages/agentic-client/src/acp/index.ts
- clients/ui/src/components/RunsPage.tsx
- clients/ui/src/components/PlaybookComposerModal.tsx
- clients/ui/src/components/sources.tsx
Comment on lines
+22
to
+40
| containers: | ||
| - name: ui | ||
| image: agentic-ui:dev | ||
| imagePullPolicy: IfNotPresent | ||
| ports: | ||
| - containerPort: 8080 | ||
| name: http | ||
| readinessProbe: | ||
| httpGet: | ||
| path: / | ||
| port: http | ||
| initialDelaySeconds: 2 | ||
| periodSeconds: 5 | ||
| resources: | ||
| requests: | ||
| cpu: 10m | ||
| memory: 32Mi | ||
| limits: | ||
| memory: 128Mi |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add a securityContext to the UI container. The pod runs with the default context (root-capable, privilege escalation allowed), which Trivy/Checkov flag. The nginx-unprivileged base already listens on 8080 as non-root, so this is a manifest-only change.
🔒 Proposed hardening
spec:
+ securityContext:
+ runAsNonRoot: true
+ seccompProfile:
+ type: RuntimeDefault
containers:
- name: ui
image: agentic-ui:dev
imagePullPolicy: IfNotPresent
+ securityContext:
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop: ["ALL"]readOnlyRootFilesystem: true is also achievable if you add emptyDir mounts for nginx's temp/cache paths.
📝 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
| containers: | |
| - name: ui | |
| image: agentic-ui:dev | |
| imagePullPolicy: IfNotPresent | |
| ports: | |
| - containerPort: 8080 | |
| name: http | |
| readinessProbe: | |
| httpGet: | |
| path: / | |
| port: http | |
| initialDelaySeconds: 2 | |
| periodSeconds: 5 | |
| resources: | |
| requests: | |
| cpu: 10m | |
| memory: 32Mi | |
| limits: | |
| memory: 128Mi | |
| securityContext: | |
| runAsNonRoot: true | |
| seccompProfile: | |
| type: RuntimeDefault | |
| containers: | |
| - name: ui | |
| image: agentic-ui:dev | |
| imagePullPolicy: IfNotPresent | |
| securityContext: | |
| allowPrivilegeEscalation: false | |
| capabilities: | |
| drop: ["ALL"] | |
| ports: | |
| - containerPort: 8080 | |
| name: http | |
| readinessProbe: | |
| httpGet: | |
| path: / | |
| port: http | |
| initialDelaySeconds: 2 | |
| periodSeconds: 5 | |
| resources: | |
| requests: | |
| cpu: 10m | |
| memory: 32Mi | |
| limits: | |
| memory: 128Mi |
🧰 Tools
🪛 Trivy (0.72.0)
[error] 22-39: Root file system is not read-only
Container 'ui' of Deployment 'agentic-ui' should set 'securityContext.readOnlyRootFilesystem' to true
Rule: KSV-0014
(IaC/Kubernetes)
[error] 22-39: Default security context configured
container agentic-ui in konveyor-agents namespace is using the default security context
Rule: KSV-0118
(IaC/Kubernetes)
🤖 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 `@clients/deploy/manifests/ui.yaml` around lines 22 - 40, Add a securityContext
to the ui container definition, configuring it to run as non-root with privilege
escalation disabled and capabilities dropped. Keep the existing
nginx-unprivileged port configuration and other container settings unchanged; do
not add readOnlyRootFilesystem unless the required emptyDir mounts are also
defined.
Source: Linters/SAST tools
Comment on lines
+24
to
+31
| if [ -n "$REPO" ]; then | ||
| if [ -z "$(ls -A /workspace 2>/dev/null)" ]; then | ||
| log "cloning $REPO@$BRANCH into /workspace" | ||
| if git clone --depth 1 --branch "$BRANCH" "$REPO" /workspace 2>&1; then | ||
| log "clone OK: $(ls /workspace | head -6 | tr '\n' ' ')" | ||
| else | ||
| log "WARNING: clone failed — agent starts with an empty workspace" | ||
| fi |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact credentials before logging the repo URL. KONVEYOR_PARAM_REPOSITORY may carry userinfo (https://user:token@host/org/repo.git), and Line 26 writes it verbatim to pod logs. Strip the userinfo for the log line.
🔒 Proposed fix
if [ -n "$REPO" ]; then
if [ -z "$(ls -A /workspace 2>/dev/null)" ]; then
- log "cloning $REPO@$BRANCH into /workspace"
+ SAFE_REPO="$(printf '%s' "$REPO" | sed -E 's#(://)[^/@]*@#\1***@#')"
+ log "cloning $SAFE_REPO@$BRANCH into /workspace"📝 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
| if [ -n "$REPO" ]; then | |
| if [ -z "$(ls -A /workspace 2>/dev/null)" ]; then | |
| log "cloning $REPO@$BRANCH into /workspace" | |
| if git clone --depth 1 --branch "$BRANCH" "$REPO" /workspace 2>&1; then | |
| log "clone OK: $(ls /workspace | head -6 | tr '\n' ' ')" | |
| else | |
| log "WARNING: clone failed — agent starts with an empty workspace" | |
| fi | |
| if [ -n "$REPO" ]; then | |
| if [ -z "$(ls -A /workspace 2>/dev/null)" ]; then | |
| SAFE_REPO="$(printf '%s' "$REPO" | sed -E 's#(://)[^/@]*@#\1***@#')" | |
| log "cloning $SAFE_REPO@$BRANCH into /workspace" | |
| if git clone --depth 1 --branch "$BRANCH" "$REPO" /workspace 2>&1; then | |
| log "clone OK: $(ls /workspace | head -6 | tr '\n' ' ')" | |
| else | |
| log "WARNING: clone failed — agent starts with an empty workspace" | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 28-28: Use find instead of ls to better handle non-alphanumeric filenames.
(SC2012)
🤖 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 `@clients/harness-goose/entrypoint.sh` around lines 24 - 31, Update the clone
log in the REPO handling block of entrypoint.sh to redact any URL userinfo,
including usernames and tokens, before logging the repository address. Keep the
original REPO value unchanged for git clone, and use the sanitized value only in
the “cloning … into /workspace” message.
Comment on lines
+66
to
+72
| constructor(baseUrl: string) { | ||
| // Validate eagerly so a bad base fails at construction, not first call. | ||
| const parsed = new URL(baseUrl); | ||
| if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { | ||
| throw new Error(`ShimClient: baseUrl must be http(s), got ${parsed.protocol}//`); | ||
| } | ||
| this.baseUrl = baseUrl.replace(/\/+$/, ""); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject query strings and fragments in baseUrl.
A valid value such as https://shim.example/prefix?debug=1 is retained verbatim, so send() requests .../prefix?debug=1/api/agents rather than the API route. A fragment similarly prevents the appended path from reaching the server. Normalize from parsed and reject search/hash.
Proposed fix
if (parsed.protocol !== "http:" && parsed.protocol !== "https:") {
throw new Error(`ShimClient: baseUrl must be http(s), got ${parsed.protocol}//`);
}
- this.baseUrl = baseUrl.replace(/\/+$/, "");
+ if (parsed.search || parsed.hash) {
+ throw new Error("ShimClient: baseUrl must not include a query string or fragment");
+ }
+ this.baseUrl = parsed.toString().replace(/\/+$/, "");📝 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
| constructor(baseUrl: string) { | |
| // Validate eagerly so a bad base fails at construction, not first call. | |
| const parsed = new URL(baseUrl); | |
| if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { | |
| throw new Error(`ShimClient: baseUrl must be http(s), got ${parsed.protocol}//`); | |
| } | |
| this.baseUrl = baseUrl.replace(/\/+$/, ""); | |
| constructor(baseUrl: string) { | |
| // Validate eagerly so a bad base fails at construction, not first call. | |
| const parsed = new URL(baseUrl); | |
| if (parsed.protocol !== "http:" && parsed.protocol !== "https:") { | |
| throw new Error(`ShimClient: baseUrl must be http(s), got ${parsed.protocol}//`); | |
| } | |
| if (parsed.search || parsed.hash) { | |
| throw new Error("ShimClient: baseUrl must not include a query string or fragment"); | |
| } | |
| this.baseUrl = parsed.toString().replace(/\/+$/, ""); |
🤖 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 `@clients/packages/agentic-client/src/transport-shim/index.ts` around lines 66
- 72, Update the Transport Shim constructor’s baseUrl validation to reject URLs
with nonempty parsed.search or parsed.hash, then normalize and store the URL
from the parsed URL rather than the original string so query strings and
fragments cannot be retained before appended API routes. Preserve the existing
HTTP/HTTPS protocol validation and trailing-slash removal.
| * the ADR 0005 param/credential-source resolution formerly performed here is | ||
| * RETIRED for the platform path. | ||
| * | ||
| * No auth on the shim itself — localhost dev tool only. CORS `*` on /api/*. |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unauthenticated + Access-Control-Allow-Origin: * is no longer "localhost only". The deploy manifests run this shim in-cluster behind the gateway/UI, where these routes create/delete CRs and bridge ACP sessions with full namespace credentials. Any page a user visits can drive it once the port is reachable. Please either bind the wildcard CORS and the missing auth check to a dev-only flag (e.g. allow * only when HOST is loopback) or document the gateway as the enforced trust boundary in clients/deploy/README.md.
Also applies to: 1440-1444
🤖 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 `@clients/packages/hub-shim/src/server.ts` at line 66, Restrict the shim’s
unauthenticated behavior and wildcard CORS to a verified loopback/dev-only HOST
configuration; deployed or non-loopback configurations must require
authentication and avoid Access-Control-Allow-Origin: *. Apply this consistently
to the /api/* routes and ACP session bridge, preserving the current localhost
development behavior.
Comment on lines
+267
to
+272
| localSession.onClosed(() => { | ||
| if (!disposed) { | ||
| setConn({ kind: "disconnected" }); | ||
| setSession(null); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Stale transcript persists when reconnecting without loadSession support.
setItems([]) only runs inside the prior && loadSessionSupported branch (Line 278). When prior exists but the agent doesn't support loadSession (a real, documented possibility per the ACP contract), execution falls into the else branch and calls newSession() for a brand-new session while leaving the old session's transcript — including any permission item still shown with live option buttons — in items. Users then see old and new session content mixed together, and clicking a stale permission button silently no-ops against the closed session.
Separately, onClosed (Lines 267-272) doesn't reset turnActive/sessionRef.current, so after an unexpected drop mid-turn, "Cancel turn" stays visible pointing at a dead session until the user manually reconnects.
🐛 Proposed fix
localSession.onClosed(() => {
if (!disposed) {
setConn({ kind: "disconnected" });
setSession(null);
+ setTurnActive(false);
+ sessionRef.current = null;
}
});
// Prefer resuming the previous session after a drop — the agent
// replays its history as session/update notifications.
let sessionId: string;
const prior = lastSessionIdRef.current;
if (prior && localSession.loadSessionSupported) {
setItems([]); // the replay repopulates the transcript
try {
await localSession.loadSession(prior);
sessionId = prior;
} catch {
+ setItems([]);
sessionId = await localSession.newSession();
}
} else {
+ if (prior) setItems([]);
sessionId = await localSession.newSession();
}Also applies to: 275-288
🤖 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 `@clients/ui/src/components/ChatPanel.tsx` around lines 267 - 272, Reset
session-scoped state whenever the connection closes or a reconnect creates a new
session: update the localSession.onClosed handler to clear turnActive and
sessionRef.current alongside conn and session, and ensure the reconnect path
around the prior/loadSessionSupported branching clears items before calling
newSession when loadSession is unsupported. Preserve transcript restoration when
loadSession is supported, while preventing stale permission controls and old
content from carrying into the new session.
Comment on lines
+111
to
+116
| | GET | `/api/agentruns` | 200 `AgentRun[]` (full CRs) | | ||
| | POST | `/api/agentruns` (body `{agentRef, params?: Record<string,string>, instructions?, applicationRef?}`) | 201 `AgentRun` (generateName `ui-`, params mapped to `[{name,value}]`). When `applicationRef` is set, the platform resolves the Agent's declared param/credential sources from that application (ADR 0005): resolved params merge under caller-supplied ones, credentials become `spec.envFrom`. 400 on unknown `applicationRef`, or a required param with a recognized source the application cannot supply. | | ||
| | GET | `/api/agentruns/:name` | 200 `AgentRun` \| 404 | | ||
| | DELETE | `/api/agentruns/:name` | 204 | | ||
| | WS | `/api/agentruns/:name/acp` | Resolves the run's ACP endpoint (waitForAcpEndpoint semantics, 60s), opens a port-forward tunnel to the pod, dials `ws://127.0.0.1:<tunnel>/acp` upstream WITH `X-Secret-Key` (key read from the run's Secret), then pipes frames bidirectionally. Client close → close upstream + tunnel; upstream close/error → close client 1011 with reason. | | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stale POST /api/agentruns contract: describes the retired ADR-0005 resolution and omits targetBranch/model.
Line 112 states applicationRef triggers "the platform resolves the Agent's declared param/credential sources from that application (ADR 0005): resolved params merge under caller-supplied ones, credentials become spec.envFrom." The hub-shim's own route docstring says the opposite for this path — the ADR 0005 resolution is "RETIRED for the platform path," and applicationRef instead just injects "Hub coordinates + TARGET_BRANCH ... as spec.env."
The documented body {agentRef, params?, instructions?, applicationRef?} also omits targetBranch and model, both of which the real handler validates (targetBranch requires applicationRef) and which other UI code (e.g. the rerun flow) actually sends.
Since this ADR is meant to be "the reference shape the future Konveyor Hub passthrough proxy is expected to expose," this row should be updated to match the current hub-shim implementation before it misleads a future Hub-proxy implementation.
🤖 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 `@docs/adr/0004-client-contract-and-transports.md` around lines 111 - 116,
Update the POST /api/agentruns contract row to match the current hub-shim
handler: include optional targetBranch and model in the request body, document
that targetBranch requires applicationRef, and remove the retired ADR 0005
parameter/credential resolution behavior. Describe applicationRef as injecting
Hub coordinates and TARGET_BRANCH into spec.env, while preserving the existing
response and validation details.
Comment on lines
+99
to
+126
| **Open question surfaced by wiring this to real Hub.** Repo URL and branch | ||
| are plain fields on a Hub `Application` — read them and you're done. A | ||
| credential is *not*: Hub stores it as an `Identity` in its own encrypted | ||
| vault, and the REST API exposes the identity's *name*, never the secret. | ||
| So `application-identity` resolves cleanly to "this app uses Hub identity | ||
| `coolstore-git`", but turning that into something the sandbox can use | ||
| requires the platform to **decrypt the vault identity and materialize it | ||
| into the pod** (as a mounted Secret or injected env). Production Hub, which | ||
| owns the vault, does this itself. The shim can't — it only sees the name — | ||
| so it *bridges* known identity names to a pre-created k8s Secret | ||
| (`IDENTITY_SECRET_BRIDGE`) and the UI shows both: `Hub identity: | ||
| coolstore-git → git-credentials-coolstore`. That bridge is the one honest | ||
| stub left in the flow, and materialization is the concrete thing Hub must | ||
| own. | ||
|
|
||
| ### (d) API surface | ||
|
|
||
| SHIM API v1 (and therefore the future Hub proxy) gains: | ||
|
|
||
| - `GET /api/applications` → the platform's application inventory. The shim | ||
| reads **real Konveyor Hub** over `HUB_URL` (`/applications` + `/identities`, | ||
| mapped to `{id, name, repository, identity, identitySecret}`) and falls | ||
| back to a built-in stub only when Hub is unreachable. Repo URL/branch and | ||
| the identity name are genuine Hub data; only the identity→Secret bridge is | ||
| stubbed (see (c)). Production is Hub reading its own Application table. | ||
| - `POST /api/agentruns` accepts `applicationRef`; the platform resolves | ||
| sourced params/credentials from that application per the semantics | ||
| above. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
ADR describes an inventory shape and identity bridge the shipped shim no longer implements. clients/packages/hub-shim/src/server.ts maps applications to {id, name, repository, identity} only — there is no identitySecret field and no IDENTITY_SECRET_BRIDGE; its header explicitly states the ADR 0005 param/credential-source resolution is retired for the platform path (Hub coordinates are injected via RUN_ENV instead). Please add a status note here so the ADR doesn't read as current behavior.
🤖 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 `@docs/adr/0005-platform-resolved-params.md` around lines 99 - 126, Add a
status note to ADR 0005 clarifying that its application inventory and
identity-to-Secret bridge are historical or retired for the shipped platform
path. State that the current hub-shim application mapping omits identitySecret
and IDENTITY_SECRET_BRIDGE, and that Hub coordinates are injected through
RUN_ENV instead; preserve the ADR’s original design context while clearly
distinguishing it from current behavior.
Member
Author
|
Closing as superseded by the contract-only strategy: #106 moves the ADRs (client contract, param sources) upstream, the tackle2-ui port carries the UI work (konveyor/tackle2-ui#3504), and the prototype repo stays a reference implementation rather than being imported wholesale. Branch preserved if any piece turns out to be wanted directly. |
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
1 participant
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.
Imports the working client stack prototyped in ibolton336/agentcontroller-client (the running system behind the contract proposal on #22), cleaned of POC scaffolding, as
clients/— plus ADRs 0004/0005 continuing this repo's ADR sequence.What this is
A verified, end-to-end client-side implementation of this repo's AgentRun/ACP contract, developed against the live controller on minikube (Agent Sandbox v0.5.0) with both a deterministic mock agent and a real goose+Bedrock agent. Each piece is the reference for a planned stream:
clients/packages/hub-shimclients/packages/agentic-client+clients/uiAcpSession, HTTP transport) + PatternFly SPA with streaming chat and HITL permission round-tripsclients/packages/agentrun-clientclients/harness-gooseKONVEYOR_*env contract (clone, model env mapping, prompt hints)clients/harness-mockTEST_PERMISSION,TEST_CANCEL,TEST_DROP) — useful as an e2e fixtureclients/deployclients/docs/DEMO.mdis the narrated end-to-end runbook (browser create → real goose+Bedrock agent → IDE attach/handoff → connection-drop resilience);clients/hack/demo-check.shsmokes the full ACP round-trip against a live cluster.What was cleared out
Relative to the prototype repo: the controller simulator and vendored CRD copies (this repo's reconcilers are the real thing now), the rendered controller install snapshot, prepared upstream patch files (delivered separately as PRs), simulator-era docs, and posted issue-comment drafts.
Verification
npm install+ typecheck/build clean in the new location; the client core's protocol selfcheck passes 17/17.Notes for reviewers
status.sandboxName, ACP Secret data keysecret-key, headless portless Service, whole-spec immutability) — worth a close read as the stream-2 handover spec.What changed since the first push of this branch
The branch was squashed into the single import commit this PR was always meant
to land as (and picked up the
Signed-off-byline DCO wanted). Beyond theoriginal import, the stack now also covers:
AgentPlaybookRunis a first-class surface alongsideAgentRun: list view, stage ladder with per-stage run links, and a launcherthat resolves params/credentials against the union of the stage Agents.
SkillCollections and AgentPlaybooks are creatable and editable from the UI,
backed by new shim write routes (
POST/PUT/DELETE). Created and editedresources are stamped
konveyor.io/managed=true, which is also the listfilter — editing an unlabeled resource adopts it.
/api/imagesand/api/defaultslet thelauncher offer known agent images and prefill a working run.
model: {provider, model}on both createinputs, validated against each Agent's declared providers and the provider
CR's declared models; the playbook default policy is the intersection of the
stage Agents' provider lists.
harness's env/param shape (Hub self-pull via
HUB_BASE_URL/APP_ID, sharedTARGET_BRANCH), so runs created from this UI are the shape the harness reads.Verified end to end on minikube against the live controller, including a full
three-stage
AgentPlaybookRun(assess → remediate → validate) sharing onetarget branch, with a real goose+Bedrock agent and a Konveyor Hub supplying
analysis and git credentials.
Summary by CodeRabbit