📖 Contribute ADRs 0012 (client contract) and 0013 (platform-resolved params) - #106
Conversation
📝 WalkthroughWalkthroughAdds ADRs 0012 and 0013 and expands the Agent REST API specification. The documents define AgentRun contracts, transports, lifecycle semantics, SHIM APIs, platform-resolved parameters, credentials, and application-aware run creation. ChangesAgentRun Contracts and Platform Resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
edfa5b1 to
fdb582e
Compare
ibolton336
changed the title
fdb582e to
08c5fd9
Compare
08c5fd9 to
4f9ac3d
Compare
ibolton336
changed the title
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Both were written and verified in the hub-shim prototype and have been cited by URL from konveyor#22 and konveyor/enhancements#295 ever since. Those citations point into a personal playground repo, which is the wrong home for decisions the platform depends on. Move them here. ADR 0012 freezes the client contract that the live controller actually exhibits — verified against PR konveyor#4 on a real cluster, not inferred from the types: pod resolved by status.sandboxName (never by label, never by string-munging the run name), ACP key secret data-key order, headless Service so clients must dial the pod, spec immutability implying delete-and-recreate for every "edit"/"retry" affordance. It also records SHIM HTTP API v1, the route table the future Hub passthrough proxy is expected to expose, which is the concrete acceptance contract for konveyor#72. ADR 0013 covers platform-resolved params: source identifiers as free-form namespaced strings rather than a CRD enum (an enum bakes one consumer's vocabulary into a generic CRD whose controller ignores the field, and skew fails closed), the fail-open rule that keeps older clients working, the managed-agent label, and the honest open question — Hub must materialize a vault Identity into the sandbox, which the shim can only stub. Numbered 0012/0013 to continue this series past #108s 0009-0011; they were 0004/0005 in the prototype repo, where 0004 collided with "OpenShell as Execution Interface". The old paths remain there as pointers so posted links survive. prototype repo, where 0004 collided with "OpenShell as Execution Interface". The old paths remain there as pointers so posted links survive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
4f9ac3d to
f664a2e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/adr/0012-client-contract-and-transports.md`:
- Line 122: Update the WS contract row to distinguish upstream normal closures
from proxy failures: forward normal ACP close codes to the client, and reserve
client close code 1011 for upstream or tunnel proxy errors. Keep the existing
bidirectional piping and cleanup behavior unchanged.
- Around line 130-134: Update the AgentRun edit/retry contract near the
immutable-spec guidance so deleted runs are not described as preserving history
through later listing. Retain the existing run and create a new attempt, or
explicitly persist its history in a separate resource before deletion; ensure
the stated behavior matches the deletion semantics defined earlier.
- Around line 124-126: Update the unauthenticated shim’s CORS handling for
`/api/*` to remove `Access-Control-Allow-Origin: *`; restrict requests to
trusted origins and enforce Origin/CSRF validation or authentication for
mutating endpoints such as `POST /api/agentruns` and `DELETE
/api/agentruns/:name`, while preserving OPTIONS handling only for approved
origins.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e9b831f-81a4-4b9d-bcd3-0452e76b258f
📒 Files selected for processing (2)
docs/adr/0012-client-contract-and-transports.mddocs/adr/0013-platform-resolved-params.md
| | GET | `/api/agentworkflowruns/:name` | 200 `AgentWorkflowRun` \| 404 | | ||
| | POST | `/api/agentworkflowruns` (body `{workflowRef, params?, applicationRef?, targetBranch?, gateway?}`) | 201 `AgentWorkflowRun` (generateName `ui-`), labelled `konveyor.io/managed` plus `konveyor.io/application` when scoped. | | ||
| | DELETE | `/api/agentworkflowruns/: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. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve normal ACP close codes.
Line 122 maps both an upstream normal close and an upstream error to client code 1011. Code 1011 means an internal server error. A successful ACP session can therefore appear failed. Forward normal close codes and use 1011 only for proxy errors.
Proposed contract wording
- upstream close/error → close client 1011 with reason
+ upstream normal close → forward the close code and reason
+ upstream error → close client 1011 with reason📝 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.
| | 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. | | |
| | 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 normal close → forward the close code and reason; upstream error → close client 1011 with reason. | |
🤖 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/0012-client-contract-and-transports.md` at line 122, Update the WS
contract row to distinguish upstream normal closures from proxy failures:
forward normal ACP close codes to the client, and reserve client close code 1011
for upstream or tunnel proxy errors. Keep the existing bidirectional piping and
cleanup behavior unchanged.
| The shim itself is unauthenticated (localhost dev tool) and serves | ||
| `Access-Control-Allow-Origin: *` on `/api/*` (plus OPTIONS preflight). The | ||
| real Hub proxy adds its own authn/z in front of the same shape. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository ADR context:"
sed -n '1,200p' docs/adr/0012-client-contract-and-transports.md 2>/dev/null || true
echo
echo "Search for shim CORS /api implementation:"
rg -n --hidden --glob '!docs/adr/**' "Access-Control-Allow-Origin|/api/|agentruns|localhost dev tool|unauthenticated" . \
| head -n 250
echo
echo "Files likely implementing shim HTTP server:"
git ls-files | rg "shim|local|api|agent.*run|controller" | head -n 200Repository: konveyor/agentic-controller
Length of output: 20355
CSRF (CWE-942)
Reachability: External
Remove wildcard CORS from the unauthenticated shim.
The shim is unauthenticated and exposes mutating HTTP endpoints, including POST /api/agentruns and DELETE /api/agentruns/:name. Access-Control-Allow-Origin: * on /api/* lets any webpage preflight and then read or mutate the local shim state through the user’s browser. Restrict CORS to trusted origins, enforce Origin/CSRF checks, or require authentication.
🤖 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/0012-client-contract-and-transports.md` around lines 124 - 126,
Update the unauthenticated shim’s CORS handling for `/api/*` to remove
`Access-Control-Allow-Origin: *`; restrict requests to trusted origins and
enforce Origin/CSRF validation or authentication for mutating endpoints such as
`POST /api/agentruns` and `DELETE /api/agentruns/:name`, while preserving
OPTIONS handling only for approved origins.
djzager
left a comment
djzager
left a comment
There was a problem hiding this comment.
Thanks for contributing these from the prototype — the operational knowledge here is valuable. The core decisions (layered transports, source identifiers as free-form strings, managed-agent labels) are sound.
The main issues are alignment with the enhancement (konveyor/enhancements#295) and ADRs already in the codebase. Five comments inline.
| guarantee is deliberately scoped to recognized sources with an | ||
| application selected; outside that scope the param is caller-supplied and | ||
| ordinary required-ness rules apply. | ||
| - An annotation entry naming a param the Agent does not declare in |
There was a problem hiding this comment.
The create-time param resolution pattern here (Hub resolves values from the application record when applicationRef is set) contradicts the enhancement (konveyor/enhancements#295) which explicitly rejected the "Smart Hub endpoint" alternative, and CONTEXT.md which says "Hub does not resolve application data at create time — the harness resolves at runtime." This ADR needs to be updated to align with the established architecture: Hub injects connectivity info, the harness resolves at runtime.
There was a problem hiding this comment.
Fixed in cbd2d79. Sources are now a UI/form contract plus a standalone-host mechanism; on the Konveyor path resolution is the harness at runtime per ADR 0006 — no create-time resolution. The smart-endpoint language is gone.
| diff blocks to file-modifying permission asks; clients SHOULD render | ||
| them before the approve/reject choice. Verified end-to-end (mock | ||
| harness → hub-shim WS proxy → browser ChatPanel, 2026-07-07). | ||
|
|
There was a problem hiding this comment.
The injected env contract here lists KONVEYOR_PARAM_<NAME> as normative, but ADR 0009 (merged) replaces per-param env vars with /run/konveyor/params.json. This section should describe the contract in terms of what's on the CR (spec.params), not the delivery mechanism, since that's changing. The client-facing contract is unaffected — params ride the CR, not the pod env.
There was a problem hiding this comment.
Fixed in cbd2d79 — contract now stated in CR terms (spec.params); delivery explicitly not part of the client contract, with ADR 0009 referenced.
| | GET | `/api/agentruns[?application=<hub id>]` | 200 `AgentRun[]` (full CRs). `application` filters by the `konveyor.io/application` label (ADR 0013) — a `client.List()` label selector, never a client-side scan. Runs predating the label are not selected. 400 on a non-numeric id; 400 on any resource that cannot honour the filter, never a silent unfiltered list. | | ||
| | POST | `/api/agentruns` (body `{agentRef, params?: Record<string,string>, instructions?, applicationRef?, targetBranch?, gateway?}`) | 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 0013): resolved params merge under caller-supplied ones, credentials become `spec.envFrom`, and the run is stamped `konveyor.io/application: "<id>"`. 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 | |
There was a problem hiding this comment.
The enhancement says the UI cancels runs, never deletes them (line 820), and exposes Cancel on the Hub endpoint. The shim table here exposes DELETE, and section (c) says edit/retry is delete+recreate with history "preserved by listing past runs" — but deleted runs won't appear in lists. This should align with the enhancement: cancel for in-flight runs, TTL pruning for completed runs, retry by creating a new run (old one stays until pruned).
There was a problem hiding this comment.
Fixed in cbd2d79 — section (c) is now cancel-never-delete + TTL pruning with new-run semantics for edit/retry; the shim's DELETE marked prototype-only. You're right that the old text contradicted itself on history.
| HTTP + a plain WebSocket — no headers, no kube credentials; the proxy owns | ||
| endpoint resolution, tunneling, and secret injection server-side. | ||
|
|
||
| The local **hub-shim** implements the proxy side today, and its HTTP surface |
There was a problem hiding this comment.
The route table here should live in a separate API spec document coordinated with the Hub implementation (issue #72), not frozen in an ADR. ADRs are immutable once accepted, but these endpoint shapes will evolve as Hub takes over from the shim. The ADR should record the architectural decision (layered transports, shim as Hub proxy reference shape) and reference the spec, not embed it.
There was a problem hiding this comment.
|
|
||
| Credentials must not be an `envFrom` punt (that couples every caller to | ||
| per-agent Secret knowledge — same flaw the SigV4 feedback flagged). | ||
| An agent declares credential needs identically: |
There was a problem hiding this comment.
Section (c) surfaces a real problem but doesn't record a decision — it documents an open question. The harness already handles credential resolution at runtime via Hub's scoped token (enhancement lines 945-953). If the decision is to keep that pattern (harness resolves credentials at runtime, not at create time), this section should say so explicitly rather than leaving it open.
There was a problem hiding this comment.
Fixed in cbd2d79 — now records the decision: runtime credential fetch via the scoped token per enhancement §945-953; the shim's identity→Secret bridge reframed as the standalone-host stub of it.
Per djzager's review on konveyor#106: - 0013 no longer specifies create-time resolution on the Hub path: sources are a UI/form contract plus a standalone-host mechanism; on the Konveyor path the harness resolves at runtime per ADR 0006 and enhancement #295. - 0013 credentials section records the decision (runtime fetch via scoped token; the shim's identity->Secret bridge is a standalone stub), instead of documenting an open question. - 0012 describes the param contract in CR terms (spec.params); delivery mechanism explicitly not part of the client contract (ADR 0009). - 0012 run lifecycle: cancel-never-delete + TTL pruning; new-run semantics for edit/retry; the shim DELETE route marked prototype-only. - Route table extracted from the ADR into docs/agent-api-spec.md as a living spec coordinated with konveyor#72; the ADR freezes the layering decision, not endpoint shapes. WS row now forwards normal close codes and reserves 1011 for proxy errors (matches the shim's actual sendableCloseCode behavior). Signed-off-by: ibolton336 <ibolton@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/agent-api-spec.md (1)
44-50: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove wildcard CORS from the unauthenticated shim.
The shim exposes mutating endpoints, including
POST /api/agentruns, while allowingAccess-Control-Allow-Origin: *. A web page from another origin can call the local unauthenticated shim and read or mutate run state.Restrict CORS to trusted origins, enforce
Origin/CSRF checks, or require authentication. This is the same unresolved issue raised in the previous review after the route table moved to this file.🤖 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/agent-api-spec.md` around lines 44 - 50, Update the unauthenticated shim’s Auth documentation to remove the wildcard Access-Control-Allow-Origin behavior and specify a concrete protection for its mutating /api endpoints, such as trusted-origin CORS with Origin/CSRF validation or authentication; ensure the documented topology no longer permits arbitrary cross-origin access.
🤖 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 `@docs/adr/0012-client-contract-and-transports.md`:
- Line 105: Update the Markdown text containing `#72.` so the issue reference
appears within a sentence, such as `issue `#72`.`, rather than at the start of the
line where it is parsed as a heading. Preserve the reference and surrounding
documentation content.
- Around line 107-115: Update the AgentRun CRD/schema to permit the cancellation
exception while keeping all other spec fields immutable, then revise the
contract text in docs/adr/0012-client-contract-and-transports.md and
docs/agent-api-spec.md (anchor lines 107-115 and sibling lines 32-38) to state
that PATCHing spec.cancel is accepted at runtime. Ensure both documents
consistently describe cancellation as the sole mutable spec field and preserve
new-run semantics for other edits.
In `@docs/agent-api-spec.md`:
- Line 24: The POST /api/agentruns contract must define the source and
validation of targetBranch for application-scoped runs. Update the API behavior
and documentation so Hub supplies targetBranch through spec.env, or require and
reject requests missing it before creating the AgentRun; ensure the harness
receives TARGET_BRANCH and update the visible endpoint description accordingly.
---
Outside diff comments:
In `@docs/agent-api-spec.md`:
- Around line 44-50: Update the unauthenticated shim’s Auth documentation to
remove the wildcard Access-Control-Allow-Origin behavior and specify a concrete
protection for its mutating /api endpoints, such as trusted-origin CORS with
Origin/CSRF validation or authentication; ensure the documented topology no
longer permits arbitrary cross-origin access.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e01a275-b280-4f8b-808f-fe4d058782bf
📒 Files selected for processing (3)
docs/adr/0012-client-contract-and-transports.mddocs/adr/0013-platform-resolved-params.mddocs/agent-api-spec.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/adr/0013-platform-resolved-params.md
| implementation (#72). The decision this ADR freezes is the layering above | ||
| and that the spec is verified against the live controller before Hub | ||
| reimplements it — not the endpoint shapes themselves, which evolve with | ||
| #72. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Markdown token on Line 105.
Line 105 starts with #72.. markdownlint-cli2 reports MD018 because it parses this as an ATX heading without a space. Keep the issue reference in the sentence, for example change the line to issue #72..
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 105-105: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 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/0012-client-contract-and-transports.md` at line 105, Update the
Markdown text containing `#72.` so the issue reference appears within a
sentence, such as `issue `#72`.`, rather than at the start of the line where it is
parsed as a heading. Preserve the reference and surrounding documentation
content.
Source: Linters/SAST tools
| ### (c) Spec immutability ⇒ new-run semantics (cancel, never delete) | ||
|
|
||
| Because the AgentRun spec is immutable, every client "edit"/"retry" | ||
| affordance creates a NEW run — UIs MUST NOT offer in-place spec mutation; | ||
| run identity is per-attempt. The platform surface cancels in-flight runs | ||
| (token revocation + `spec.cancel`, ADR 0006) and never deletes them: | ||
| completed runs age out via per-condition TTL pruning, which is what keeps | ||
| "history is the run list" true. The prototype shim's DELETE route predates | ||
| this and is a dev convenience, not part of the platform contract. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
git ls-files | rg '(^docs/adr/0012-client-contract-and-transports\.md$|^docs/agent-api-spec\.md$|^.*crd.*|^.*controller.*|specs|schema)' || true
echo
echo "== relevant docs snippets =="
for f in docs/adr/0012-client-contract-and-transports.md docs/agent-api-spec.md; do
if [ -f "$f" ]; then
echo "--- $f lines 1-140 ---"
sed -n '1,140p' "$f" | nl -ba -v1
fi
done
echo
echo "== search for AgentRun/spec/cancel/crud references =="
rg -n "AgentRun|AgentRunSpec|spec\.cancel|cancel.*spec|run.*cancel|DELETE|immutable|TTL|CRD|C|controller|subresource" -S . \
-g '!node_modules' -g '!dist' -g '!build' -g '!coverage' -g '!vendor' \
| head -200Repository: konveyor/agentic-controller
Length of output: 1916
🌐 Web query:
Kubernetes immutability of spec fields cancellation mechanism subresources status patch custom resource documentation
💡 Result:
In Kubernetes, immutability of spec fields for Custom Resources (CRs) is primarily enforced through Common Expression Language (CEL) validation rules, which replaced the need for custom webhooks [1][2][3]. ### Immutability Mechanisms For Custom Resource Definitions (CRDs), immutability is implemented using the x-kubernetes-validations extension [2][3]. By applying transition rules that compare the self (new) value with the oldSelf (previous) value, you can prevent modifications [1][3]. - Example: self == oldSelf ensures a field cannot be changed after its initial creation [1]. - Kubernetes also supports the x-kubernetes-immutable OpenAPI vendor extension, which marks fields as immutable and prevents modifications after object creation [4]. - Declarative validation tags (e.g., +k8s:immutable) are an evolving framework in Kubernetes for defining such API constraints alongside type definitions [5][6]. ### Status Subresource and Patching The status subresource is a specialized API endpoint that allows updates specifically to the status block of a resource without impacting the spec [7]. - Patch vs. Update: When using client libraries (like controller-runtime), Status().Update() or Status().Patch() must be used to target the status subresource [8]. Using the standard Update() or Patch() on the main resource often fails to affect the status or is ignored by the API server if the intention is to modify status exclusively [8]. - Parent Resource Impact: While the status subresource allows isolated modification of the status block, it is still part of the same parent object. Consequently, any modification to the status will trigger a change event for the parent resource in watches, as the overall object state has changed [7]. - kubectl Usage: The --subresource flag is used with kubectl (e.g., kubectl patch --subresource=status) to explicitly target subresources [9][10]. Note that strategic merge patch is not supported for custom resources; JSON merge patch or JSON patch must be used instead [10]. There is no specific "cancellation mechanism" for immutability; once a rule (like a CEL transition rule) is enforced by the API server, it cannot be overridden by the user without modifying the CRD's validation schema itself [1][3].
Citations:
- 1: https://kubernetes.io/blog/2022/09/29/enforce-immutability-using-cel/
- 2: CRD validation resource immutable kubernetes/kubernetes#65973
- 3: https://stackoverflow.com/questions/56740758/enforcing-immutability-of-kubernetes-custom-resource-spec-fields
- 4: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1101-immutable-fields
- 5: https://kubernetes.io/docs/reference/using-api/declarative-validation/
- 6: https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/5073-declarative-validation-with-validation-gen
- 7: https://stackoverflow.com/questions/65228444/kubernetes-why-does-patching-a-custom-resources-status-subresource-update-the
- 8: Question: update status field kubernetes-sigs/controller-runtime#2850
- 9: https://kubernetes.io/docs/reference/kubectl/conventions/
- 10: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== doc snippets with cat =="
for f in docs/adr/0012-client-contract-and-transports.md docs/agent-api-spec.md; do
echo "--- $f lines 1-140 ---"
sed -n '1,140p' "$f" | awk '{printf "%6d %s\n", NR, $0}'
done
echo
echo "== AgentRun CRD outline/sections =="
for f in config/crd/bases/konveyor.io_agentruns.yaml config/crd/bases/konveyor.io_agents.yaml config/crd/bases/konveyor.io_agentworkflowruns.yaml; do
if [ -f "$f" ]; then
echo "--- $f line count ---"
wc -l "$f"
echo "--- immutability/cancel/termination/cancellation fields ---"
rg -n "AgentRun|agentRun|cancel|cancellation|delete|garbage|ttl|TTL|deletePropagation|terminationGracePeriod|status|subresource|x-kubernetes-immutable|x-kubernetes-validations|immutable" "$f" || true
echo "--- surrounding AgentRun schema context ---"
file="$(mktemp)"
rg -n -C 3 "AgentRun|cancel|cancellation|immutable|x-kubernetes-validations" "$f" "$file" || true
fi
done
echo
echo "== controller references to cancel/spec/status/delete =="
for f in internal/controller/agentrun_controller.go internal/controller/agent_controller.go internal/controller/agentworkflowrun_controller.go; do
if [ -f "$f" ]; then
echo "--- $f references ---"
rg -n "cancel|Cancel|DELETE|Delete|Finalize|status|Subresource|Patch|Update|spec" "$f" || true
echo "--- file relevant sections ---"
sed -n '1,320p' "$f" | awk '{printf "%6d %s\n", NR, $0}' | sed -n '80,240p'
fi
done
echo
echo "== search all tracked files for spec.cancel and cancellation api semantics =="
rg -n "spec\.cancel|status\.cancel|cancel|cancellation|AgentRunSpec|AgentRun|never deletes|delete" -S . \
-g '!node_modules' -g '!dist' -g '!build' -g '!vendor' \
| sed -n '1,260p'Repository: konveyor/agentic-controller
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect docs/adr/0012-client-contract-and-transports.md around lines 80-120 =="
python3 - <<'PY'
from pathlib import Path
p=Path("docs/adr/0012-client-contract-and-transports.md")
lines=p.read_text().splitlines()
for i,l in enumerate(lines,1):
if 80 <= i <= 125:
print(f"{i:4d} {l}")
PY
echo
echo "== inspect docs/agent-api-spec.md around lines 1-60 =="
python3 - <<'PY'
from pathlib import Path
p=Path("docs/agent-api-spec.md")
lines=p.read_text().splitlines()
for i,l in enumerate(lines,1):
if 1 <= i <= 70:
print(f"{i:4d} {l}")
PY
echo
echo "== exact occurrences of AgentRun.spec immutability and cancellation phrasing =="
python3 - <<'PY'
from pathlib import Path
for p in [Path("docs/adr/0012-client-contract-and-transports.md"), Path("docs/agent-api-spec.md"), Path("config/crd/bases/konveyor.io_agentruns.yaml")]:
text=p.read_text()
print(f"--- {p} ---")
for needle in ["AgentRun spec is immutable", "AgentRun.spec", "AgentRunSpec", "spec.cancel", "cancel", "cancellation", "TTL", "immutable"]:
hits=[i+1 for i,l in enumerate(text.splitlines()) if needle in l]
print(f"{needle}: {hits}")
PYRepository: konveyor/agentic-controller
Length of output: 8196
Fix the AgentRun.spec immutability contradiction in ADR 0012.
ADR 0012 says the AgentRun spec is whole-spec immutable, then says cancellation sets spec.cancel. Add the immutability exception to the CRD/schema and update this ADR and docs/agent-api-spec.md to match it; otherwise clients must be told that PATCHing spec.cancel is accepted at runtime despite the whole-spec validation.
📍 Affects 2 files
docs/adr/0012-client-contract-and-transports.md#L107-L115(this comment)docs/agent-api-spec.md#L32-L38
🤖 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/0012-client-contract-and-transports.md` around lines 107 - 115,
Update the AgentRun CRD/schema to permit the cancellation exception while
keeping all other spec fields immutable, then revise the contract text in
docs/adr/0012-client-contract-and-transports.md and docs/agent-api-spec.md
(anchor lines 107-115 and sibling lines 32-38) to state that PATCHing
spec.cancel is accepted at runtime. Ensure both documents consistently describe
cancellation as the sole mutable spec field and preserve new-run semantics for
other edits.
| | GET | `/api/skillcards[/:name]` | 200 `SkillCard[]` \| `SkillCard` \| 404 | | ||
| | GET | `/api/skillcollections[/:name]` | 200 `SkillCollection[]` \| `SkillCollection` \| 404 | | ||
| | GET | `/api/agentruns[?application=<hub id>]` | 200 `AgentRun[]` (full CRs). `application` filters by the `konveyor.io/application` label (ADR 0006) — a `client.List()` label selector, never a fetch-and-scan. Runs predating the label are not selected. 400 on a non-numeric id or on any resource that cannot honour the filter — never a silent unfiltered list. | | ||
| | POST | `/api/agentruns` (body `{agentRef, params?: Record<string,string>, instructions?, applicationRef?, targetBranch?, gateway?}`) | 201 `AgentRun` (generateName `ui-`, params mapped to `[{name,value}]`). When `applicationRef` is set the run carries the Hub coordinates (`HUB_BASE_URL`, `APP_ID`, target branch) and the `konveyor.io/application` label; the harness resolves application data from Hub at runtime (ADR 0006). 400 on unknown `applicationRef`. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'targetBranch|TARGET_BRANCH|applicationRef|HUB_BASE_URL|APP_ID' \
--glob '*.go' --glob '*.md' .Repository: konveyor/agentic-controller
Length of output: 45538
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- files matching agentruns/controller/api ---\n'
git ls-files | rg '(^|/)(controller|api|shim|internal|routes|handler|config).*\.(go)$|docs/agent-api-spec\.md$' | head -200
printf '\n--- api endpoint candidates with agentruns/applicationRef/targetBranch ---\n'
rg -n -C 6 \
'agentruns|AgentRun|applicationRef|targetBranch|applicationRef|TARGET_BRANCH|HubBaseURL|HUB_BASE_URL' \
--glob '*.go' .Repository: konveyor/agentic-controller
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- files matching agentruns/controller/api ---'
git ls-files | rg '(^|/)(controller|api|shim|internal|routes|handler|config).*\.go$|docs/agent-api-spec\.md$' | head -200
printf '%s\n' ''
printf '%s\n' '--- api endpoint candidates with agentruns/applicationRef/targetBranch ---'
rg -n -C 8 \
'agentruns|AgentRun|applicationRef|targetBranch|applicationRef|TARGET_BRANCH|HubBaseURL|HUB_BASE_URL' \
--glob '*.go' .Repository: konveyor/agentic-controller
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AgentRun spec and env population ---'
sed -n '1,220p' api/v1alpha1/agentrun_types.go
printf '%s\n' ''
awk 'BEGIN {p=0} /^func \(r \*AgentRunReconciler\) buildEnvVars/ {p=1} p' internal/controller/agentrun_controller.go | sed -n '1,220p'
printf '%s\n' ''
printf '%s\n' '--- workflow run controller relevant branch/env handling ---'
rg -n -C 12 'targetBranch|TARGET_BRANCH|Env|HUB_BASE_URL|APP_ID|Spec\.Env' internal/controller/agentworkflowrun_controller.go internal/controller/agentrun_controller.go
printf '%s\n' ''
printf '%s\n' '--- test cases mentioning omitted targetBranch / required env ---'
rg -n -C 4 'targetBranch|[Tt]arget[Bb]ranch|TARGET_BRANCH|TargetBranch|applicationRef|ApplicationRef' internal/controller -g '*_test.go'Repository: konveyor/agentic-controller
Length of output: 37694
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- AgentRunReconciler Reconcile and env building ---'
sed -n '83,175p' internal/controller/agentrun_controller.go
sed -n '528,533p' internal/controller/agentrun_controller.go
sed -n '168,176p' internal/controller/agentworkflowrun_controller.go
printf '%s\n' ''
printf '%s\n' '--- current docs line ---'
sed -n '20,26p' docs/agent-api-spec.mdRepository: konveyor/agentic-controller
Length of output: 5697
🌐 Web query:
k8s.io/client-go controller-runtime 2023 CRD CEL validation XValidate empty string optional string field required true
💡 Result:
In Kubernetes Custom Resource Definitions (CRDs), an optional string field that allows an empty string ("") is treated as a valid value by standard Kubernetes OpenAPI markers [1][2]. To enforce that such a field must contain actual content when provided—or to distinguish between an unset field and an empty string—you should combine different validation strategies [1][2]. Standard OpenAPI markers such as //+kubebuilder:validation:Required only verify the presence of the field in the request payload; they do not reject zero values like empty strings [1][2]. To ensure a string field is not empty when provided, use the following approaches: 1. Use MinLength Validation: Apply the marker //+kubebuilder:validation:MinLength=1 to the string field [1]. This forces the value to have at least one character, effectively prohibiting the empty string ("") [1]. 2. Use CEL Validation (x-kubernetes-validations): For more complex conditional requirements (e.g., "required if another field is set"), use the x-kubernetes-validations extension with Common Expression Language (CEL) [3][4]. When checking for empty strings, be aware of how CEL handles optional fields [5][6]. If you need to distinguish between an "unset" field and an "empty string" (for example, to allow an optional field to remain unset but forbid it from being set to ""), define the Go struct field as a pointer to a string (e.g., *string) rather than a direct string [1]. With a pointer type, the absence of the field becomes nil, while an empty string input remains a non-nil pointer to "" [1]. If using CEL to validate the field, you may need to check the field's presence if it is optional [7][6]. While newer Kubernetes versions have introduced enhanced CEL features for optional types, a common pattern for standard string fields is to validate against self: - To simply disallow empty strings in CEL: rule: "self.size > 0" (if the field is guaranteed present) [3]. - To handle optionality more robustly: If the field is a pointer in Go, it is often treated as present if non-null [1]. If you encounter difficulty distinguishing unset vs. empty, ensure your CRD schema and Go struct definitions are aligned regarding omitempty and pointer types [1]. Top Results: [5][3][4][1][2]
Citations:
- 1: https://ahmet.im/blog/crd-generation-pitfalls/
- 2: docs: Marker docs about required fields are not fully accurate kubernetes-sigs/kubebuilder#3675
- 3: https://opensource.googleblog.com/2023/11/kubernetes-crd-validation-using-cel.html
- 4: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/
- 5: https://kubernetes.io/docs/reference/using-api/cel/
- 6: CEL Validation: Add 'optionalSelf: true' config for consistency with 'optionalOldSelf: true' kubernetes/kubernetes#132510
- 7: https://stackoverflow.com/questions/79734436/how-to-check-if-an-optional-struct-field-is-set-using-x-kubernetes-validations
Define where targetBranch comes from for app-scoped runs.
POST /api/agentruns describes targetBranch as optional, but the harness requires TARGET_BRANCH. The AgentRun CR has no targetBranch field and buildEnvVars() does not populate TARGET_BRANCH from application metadata, so Hub must supply it in spec.env, or the API should require it / reject the run early.
🤖 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/agent-api-spec.md` at line 24, The POST /api/agentruns contract must
define the source and validation of targetBranch for application-scoped runs.
Update the API behavior and documentation so Hub supplies targetBranch through
spec.env, or require and reject requests missing it before creating the
AgentRun; ensure the harness receives TARGET_BRANCH and update the visible
endpoint description accordingly.
All five comments addressed in cbd2d79. Verified.
Closes #135, follow-up to the #70 spike. goose has its own skills implementation and never sees the ones we mount, so the harness concatenating every `SKILL.md` into the prompt is doing work the runtime would do better. This says the harness stops assembling skill content and links `~/.agents/skills` at `/opt/skills` instead, so goose discovers them and `load_skill` resolves supporting files. `skills/javaee-to-quarkus` ships 12 files under `modules/` and `references/` that the agent can't reliably reach today, which is the concrete thing it fixes. The one thing that can't be native is `type: rule`. Nothing in goose forces a skill into context, so rules stay harness-injected. They keep their mount at `/opt/skills` and the controller names them in `KONVEYOR_RULES`, so a rule is both injected and still discoverable for its own `references/`. Unset means inject everything, which is what the harness does today, so an old controller with a new harness doesn't silently drop rules. Verified against goose v1.45.0, the version agent-base pins, and measured in the image: with the link in place `goose skills list` picks up the mounted skill as uid 1001 with the mount read-only. Transcript's in the ADR. Not verified is the `goose serve` path, the probe drives the CLI. The harness half is already implemented in #136. What's left is the rules path, the container-layout globs in `skills/execute`, `skills/verify` and `skills/plan`, and retyping any existing SkillCard written as a constraint, since `type` defaults to `skill`. Numbered 0014 because #108 has 0009-0011 and #106 has 0012-0013 open. Happy to renumber if either lands differently. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added architectural documentation for native skill discovery and progressive skill loading. * Documented how ordinary skills and rule-based skills are handled, including an option to restore legacy behavior. * Added guidance on skill naming, shadowing, validation limitations, discovery mechanics, and rejected alternatives. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…params) (konveyor#106) > **Renumbered 0012/0013** — konveyor#108 claimed 0009–0011 for params.json / skill boundary / execution controls shortly after this opened; the maintainer series keeps those. Content otherwise unchanged, plus cross-references to konveyor#108's params.json ADR where these touch the KONVEYOR_PARAM_* carrier: that ADR covers how values reach the pod, these cover where values come from and the client-facing contract — complementary layers. Two ADRs that have been steering this work since July but live in a personal prototype repo. konveyor#22 and konveyor/enhancements#295 both cite them by URL — which means decisions the platform depends on are currently sourced from a playground. Moving them here. No content invented for this PR: both are as-written, adjusted only for their new home (numbering, and a provenance line on each). ## ADR 0012 — verified client contract and layered transports Freezes what the live controller **actually exhibits**, verified against PR konveyor#4 on a real cluster rather than inferred from the types. The load-bearing ones, because each has already bitten a client: - **Pod resolution is by `status.sandboxName`, exactly** — never by label (the pod carries only `agents.x-k8s.io/sandbox-name-hash`, there is no `konveyor.io/agentrun` label on it) and never by string-munging the run name. - **The auto-created Service is headless** — clients must dial the pod. - **ACP key secret** data-key order: `secret-key`, then `ACP_SECRET_KEY`, then sole-entry fallback. - **Spec is immutable**, so every "edit"/"retry" affordance in a UI is defined as delete + recreate. Run identity is per-attempt. It also records **SHIM HTTP API v1** — the route table the hub-shim serves today and the shape the Hub passthrough proxy is expected to expose. That table is a concrete acceptance contract for konveyor#72: browser UIs written against it should keep working when Hub replaces the shim. ## ADR 0013 — platform-resolved params Where a param's *value* comes from, so a create form can collapse to "application picker + instructions" without hard-coding per-agent knowledge. The decision worth arguing about: **source identifiers are free-form namespaced strings, not a CRD enum.** An enum bakes one consumer's (Hub's) domain vocabulary into a generic CRD whose own controller ignores the field, and every new value becomes a schema upgrade whose skew fails *closed* — an older CRD rejects newer Agent manifests at admission. Namespaced strings follow the `storageClassName`/`ingressClassName` precedent and fail *open*. Also here: the managed-agent label, and the open question this hits at the Hub boundary — repo URL and branch are plain fields, but a credential is an `Identity` in Hub's encrypted vault and the REST API exposes only its name. Materializing it into the sandbox is something only Hub can do; the shim can bridge known identity names to a pre-created Secret and no more. That bridge is the one honest stub left in the flow. ## Note on the label subsection ADR 0013's managed-label section also records `konveyor.io/application`, which konveyor#105 proposes amending ADR 0006 to adopt. If konveyor#105 is rejected, that subsection goes with it — the rest of both ADRs is independent of that outcome. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added architecture guidance for the AgentRun client, supported transports, and SHIM API. * Documented Kubernetes resource resolution, authentication, endpoint behavior, and edit/retry semantics. * Defined platform-resolved agent parameters and credentials, including validation and application-aware run creation. * Added REST and WebSocket API specifications for applications, agents, gateways, skills, runs, workflows, and ACP connections. * Documented request formats, response behavior, validation errors, authentication, CORS, and lifecycle rules. * Recorded compatibility considerations, known gaps, and open platform questions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: ibolton336 <ibolton@redhat.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Closes konveyor#135, follow-up to the konveyor#70 spike. goose has its own skills implementation and never sees the ones we mount, so the harness concatenating every `SKILL.md` into the prompt is doing work the runtime would do better. This says the harness stops assembling skill content and links `~/.agents/skills` at `/opt/skills` instead, so goose discovers them and `load_skill` resolves supporting files. `skills/javaee-to-quarkus` ships 12 files under `modules/` and `references/` that the agent can't reliably reach today, which is the concrete thing it fixes. The one thing that can't be native is `type: rule`. Nothing in goose forces a skill into context, so rules stay harness-injected. They keep their mount at `/opt/skills` and the controller names them in `KONVEYOR_RULES`, so a rule is both injected and still discoverable for its own `references/`. Unset means inject everything, which is what the harness does today, so an old controller with a new harness doesn't silently drop rules. Verified against goose v1.45.0, the version agent-base pins, and measured in the image: with the link in place `goose skills list` picks up the mounted skill as uid 1001 with the mount read-only. Transcript's in the ADR. Not verified is the `goose serve` path, the probe drives the CLI. The harness half is already implemented in konveyor#136. What's left is the rules path, the container-layout globs in `skills/execute`, `skills/verify` and `skills/plan`, and retyping any existing SkillCard written as a constraint, since `type` defaults to `skill`. Numbered 0014 because konveyor#108 has 0009-0011 and konveyor#106 has 0012-0013 open. Happy to renumber if either lands differently. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added architectural documentation for native skill discovery and progressive skill loading. * Documented how ordinary skills and rule-based skills are handled, including an option to restore legacy behavior. * Added guidance on skill naming, shadowing, validation limitations, discovery mechanics, and rejected alternatives. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
## Summary Adds user-facing documentation and sample manifests so new contributors and testers can deploy the controller and configure agent workloads without reverse-engineering the codebase. ### What's included **Getting-started guide** (`docs/getting-started.md`): - Prerequisites (K8s 1.33+, Agent Sandbox) - Deploying the controller (`make deploy`) - Creating Gateways with credentials for each provider - Creating an Agent and triggering an AgentRun - Workflow pointers, local dev, e2e testing, troubleshooting **Sample CRs** (`config/samples/`): - `gateway_vertex_ai.yaml` — GCP Vertex AI with Claude - `gateway_openai.yaml` — OpenAI GPT-4o - `gateway_anthropic.yaml` — Anthropic direct API - `gateway_aws_bedrock.yaml` — AWS Bedrock - `agent_example.yaml` — Java migration agent referencing a gateway and skill - `agentrun_example.yaml` — Triggers the migration agent Sample CRs are self-contained reference examples with inline comments explaining prerequisites (e.g. secret creation commands). They are not added to the samples kustomization since they require user-specific credentials. **README update**: Added a "Getting started" section linking to the new guide. ### Context From team discussion: multiple people were blocked on testing because there was no documentation on how to configure things after deploying the controller. The `LLMProvider` CRD was renamed to `Gateway` and some team members were still on the old version without realizing. ### ADR compatibility Reviewed all open ADR PRs (#108, #106, #138) before writing. The guide stays at the CRD-level user interface and does not describe internal delivery mechanisms (params.json, skill loading, ACP transport) that are in flux. Nothing here contradicts pending decisions. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added sample configurations for Agents, AgentRuns, and AI gateways across Anthropic, AWS Bedrock, OpenAI, and Vertex AI. * Added a Java EE-to-Quarkus migration Agent example with repository and branch parameters. * **Documentation** * Added a comprehensive getting-started guide covering setup, deployment, provider configuration, workflows, testing, cleanup, and troubleshooting. * Added a README link to the getting-started guide. * **Bug Fixes** * Updated the AWS Bedrock example to use the Claude Sonnet 4.5 model identifier. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: David Zager <david.j.zager@gmail.com> Signed-off-by: Dylan Murray <dymurray@redhat.com> Co-authored-by: Dylan Murray <dymurray@redhat.com>
2 participants
Two ADRs that have been steering this work since July but live in a personal
prototype repo. #22 and konveyor/enhancements#295 both cite them by URL —
which means decisions the platform depends on are currently sourced from a
playground. Moving them here.
No content invented for this PR: both are as-written, adjusted only for their
new home (numbering, and a provenance line on each).
ADR 0012 — verified client contract and layered transports
Freezes what the live controller actually exhibits, verified against PR #4
on a real cluster rather than inferred from the types. The load-bearing ones,
because each has already bitten a client:
status.sandboxName, exactly — never by label(the pod carries only
agents.x-k8s.io/sandbox-name-hash, there is nokonveyor.io/agentrunlabel on it) and never by string-munging the run name.secret-key, thenACP_SECRET_KEY, thensole-entry fallback.
as delete + recreate. Run identity is per-attempt.
It also records SHIM HTTP API v1 — the route table the hub-shim serves
today and the shape the Hub passthrough proxy is expected to expose. That
table is a concrete acceptance contract for #72: browser UIs written against
it should keep working when Hub replaces the shim.
ADR 0013 — platform-resolved params
Where a param's value comes from, so a create form can collapse to
"application picker + instructions" without hard-coding per-agent knowledge.
The decision worth arguing about: source identifiers are free-form
namespaced strings, not a CRD enum. An enum bakes one consumer's (Hub's)
domain vocabulary into a generic CRD whose own controller ignores the field,
and every new value becomes a schema upgrade whose skew fails closed — an
older CRD rejects newer Agent manifests at admission. Namespaced strings
follow the
storageClassName/ingressClassNameprecedent and fail open.Also here: the managed-agent label, and the open question this hits at the Hub
boundary — repo URL and branch are plain fields, but a credential is an
Identityin Hub's encrypted vault and the REST API exposes only its name.Materializing it into the sandbox is something only Hub can do; the shim can
bridge known identity names to a pre-created Secret and no more. That bridge
is the one honest stub left in the flow.
Note on the label subsection
ADR 0013's managed-label section also records
konveyor.io/application, which#105 proposes amending ADR 0006 to adopt. If #105 is rejected, that subsection
goes with it — the rest of both ADRs is independent of that outcome.
🤖 Generated with Claude Code
Summary by CodeRabbit