📖 ADRs 0009-0011 — params.json, skill boundary, execution controls - #108
Conversation
|
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 adds ADRs for structured parameter delivery, skill-content boundaries, and execution controls. It updates ChangesExecution contracts
Estimated code review effort: 2 (Simple) | ~15 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 |
bcbc336 to
73478a6
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: 10
🤖 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/0009-parameter-delivery-via-params-json.md`:
- Line 116: Add the text language identifier to the fenced code blocks at
docs/adr/0009-parameter-delivery-via-params-json.md lines 116-116 and
docs/adr/0011-execution-controls-mode-and-session-type.md lines 95-95 and
101-101. No other content changes are needed.
- Around line 70-79: Expand the parameter semantics in the ADR around
AgentParamType to define accepted input forms and conversion failures for
string, number, and boolean values. Specify that invalid values are rejected
during reconciliation, and document whether omitted or defaulted parameters are
absent or represented as null in params.json so the harness receives a
consistent schema.
- Around line 81-100: Update the “Prompt templating” section to define
deterministic handling when workflow and agent parameters share a name: either
require duplicate names to be rejected during parameter resolution or specify
separate `.workflow` and `.agent` template namespaces. Ensure the documented
template context and params.json structure use the same behavior.
In `@docs/adr/0010-skill-content-boundary.md`:
- Around line 50-51: Resolve the ownership contradiction in the ADR by selecting
a single owner for the `.konveyor/handoff.md` instruction. Update the guidance
around “Judgment calls” and the harness base prompt section to state explicitly
whether individual skills may include this instruction, ensuring both sections
give consistent rules.
- Around line 58-59: Update the ADR’s gate-removal guidance around the
infrastructure-tool prohibition and the future hook/harness statements to
require an implemented, ordered build-verification replacement before removing
the javaee-to-quarkus build gates. Preserve compile verification until that
replacement is available; do not rely on an undefined hook or harness behavior.
- Around line 90-92: Update the verify skill guidance in ADR 0010 to explicitly
define how fix iterations are controlled: either document a harness or
hook-level iteration limit that replaces KONVEYOR_PARAM_MAX_FIX_ITERATIONS, or
clearly state that verify intentionally no longer enforces a fix-iteration cap;
do not imply that GOOSE_MAX_TURNS provides this control.
- Around line 29-30: Update ADR 0010 to define the required reference-loading
behavior: either document harness support for recursively loading each skill’s
`/opt/skills/*/references/*.md` files, or explicitly require embedding that
reference content in `SKILL.md` before removing skill-side discovery. Ensure the
ADR’s migration guidance preserves reference context.
- Around line 22-25: Update ADR 0010 to explicitly define commit ownership and
guarantee that successful stages produce a committed worktree before pushing.
Either retain the `git add -A && git commit` instructions in the plan, execute,
and verify skills, or document and implement deterministic committing in the
harness, aligning the decision with ADR 0007 and ADR 0011.
In `@docs/adr/0011-execution-controls-mode-and-session-type.md`:
- Around line 168-178: Update the generic mapping for sessionType: plan in the
execution-controls documentation so it does not rely on the prompt prefix
"Create a plan. Do not make changes." Require native plan-mode support, or
reject the run while enforcing a read-only policy; preserve the existing default
behavior for sessionType: execute.
- Around line 49-53: Update the maxCost entry in the execution-controls ADR to
define one canonical representation and processing contract: specify decimal
precision, rounding mode, supported currency and validation requirements,
JSON/ACP parsing and serialization behavior, and how invalid values are handled.
Ensure controller, harness, runtime thresholds, and status reporting all use
this same format.
🪄 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: 9b4f6620-913b-4eb8-b74e-a6f4396de4d6
📒 Files selected for processing (4)
CONTEXT.mddocs/adr/0009-parameter-delivery-via-params-json.mddocs/adr/0010-skill-content-boundary.mddocs/adr/0011-execution-controls-mode-and-session-type.md
| - **`workflow`** — workflow-level params declared on AgentWorkflow, | ||
| supplied on AgentWorkflowRun. Absent for standalone AgentRuns. | ||
| - **`agent`** — agent-level params declared on Agent, supplied on | ||
| AgentRun. Values are type-coerced: numbers as JSON numbers, booleans | ||
| as JSON booleans, strings as JSON strings, matching the | ||
| `AgentParamType` declaration on the CRD. | ||
| - **`execution`** — resolved execution controls (see ADR 0011). These | ||
| are first-class CRD fields with defined semantics, not arbitrary | ||
| params. Separated so the harness knows where to find them without | ||
| scanning agent params for magic names. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Specify parameter conversion failures.
AgentParamType supports string, number, and boolean, but the ADR does not define accepted input forms or invalid-value behavior. Define how the controller handles invalid values and whether omitted or defaulted values are absent or null in params.json. Reject invalid values during reconciliation so harnesses receive a consistent schema.
🤖 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/0009-parameter-delivery-via-params-json.md` around lines 70 - 79,
Expand the parameter semantics in the ADR around AgentParamType to define
accepted input forms and conversion failures for string, number, and boolean
values. Specify that invalid values are rejected during reconciliation, and
document whether omitted or defaulted parameters are absent or represented as
null in params.json so the harness receives a consistent schema.
| - Running infrastructure tools (`graphify update`, `mvn compile` as a | ||
| gate). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Provide a build-verification replacement before removing the gate.
Lines 58-59 prohibit mvn compile as a gate. Lines 109-111 say that a future hook or the harness will handle verification. The supplied harness contract does not define that hook. Do not remove the current javaee-to-quarkus build gates until a replacement is implemented and ordered before this change. Otherwise, compile verification disappears.
Proposed wording
- The `javaee-to-quarkus` domain skill's build gates should be
- stripped — the harness or a future pre/post-hook mechanism handles
- build verification.
+ The `javaee-to-quarkus` domain skill's build gates should move to a
+ defined harness or pre/post-stage hook before they are removed.Also applies to: 109-111
🤖 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/0010-skill-content-boundary.md` around lines 58 - 59, Update the
ADR’s gate-removal guidance around the infrastructure-tool prohibition and the
future hook/harness statements to require an implemented, ordered
build-verification replacement before removing the javaee-to-quarkus build
gates. Preserve compile verification until that replacement is available; do not
rely on an undefined hook or harness behavior.
|
Numbering heads-up, no action needed: #106 had claimed 0009/0010 about an hour before this opened — I've moved mine to 0012/0013 so this PR keeps 0009–0011 as filed. One substantive intersection worth knowing: my 0013 (platform-resolved params) covers where param values come from at create time, and cites |
djzager
changed the title
73478a6 to
28864d4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/0009-parameter-delivery-via-params-json.md`:
- Around line 87-88: Clarify the authoritative template context in the parameter
delivery ADR: explicitly document whether each workflow-level field, including
the workflow guide and stage instructions, resolves from workflow-only
parameters or the merged workflow-plus-agent parameter map. Update both the
prompt-field description and the workflow-level params section so they use one
consistent, unambiguous scope.
- Around line 103-105: Update the ADR’s prompt-rendering behavior section to
specify that reconciliation must fail when template parsing or execution errors
occur, including required missing parameters, and must not create or start a
Sandbox in those cases. Clarify that valid plain text remains unchanged and that
rendering is performed by the controller before the harness receives the prompt.
In `@docs/adr/0011-execution-controls-mode-and-session-type.md`:
- Around line 168-176: Do not modify the accepted ADR content; add only a
supersedence notice to ADR 0011 if that is the intended scope. If the ADR must
be superseded, create a new ADR documenting the replacement and mark ADR 0011 as
superseded through the repository’s established ADR convention.
- Around line 95-103: Update both hierarchy code fences in the ADR to specify
the text language identifier, using ```text for the AgentRun and
AgentWorkflowRun examples while preserving their contents.
🪄 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: a454dd6c-f4f7-4cad-ad8d-d9723629e509
📒 Files selected for processing (5)
CONTEXT.mdchanges/unreleased/108-execution-controls-params-skills.yamldocs/adr/0009-parameter-delivery-via-params-json.mddocs/adr/0010-skill-content-boundary.mddocs/adr/0011-execution-controls-mode-and-session-type.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/adr/0010-skill-content-boundary.md
- CONTEXT.md
28864d4 to
e2c5759
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0011-execution-controls-mode-and-session-type.md`:
- Around line 205-213: Update the execution-limit behavior described in the ADR
so commit, push, and wind-down write instructions occur only when sessionType is
execute. Preserve cancellation at 100% for sessionType plan, but ensure plan
runs perform no file, commit, or Git push operations.
- Around line 220-233: Update the AgentRun usage section around the ACP
usage_update description to explicitly define how used and size map to
inputTokens, outputTokens, and totalTokens, including handling of optional cost.
If ACP does not provide separate input/output dimensions, document the
deterministic convention or add corresponding ACP fields so all harnesses
produce compatible status values.
🪄 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: d03d1cd2-b90f-4382-a801-663593133b97
📒 Files selected for processing (5)
CONTEXT.mdchanges/unreleased/108-execution-controls-params-skills.yamldocs/adr/0009-parameter-delivery-via-params-json.mddocs/adr/0010-skill-content-boundary.mddocs/adr/0011-execution-controls-mode-and-session-type.md
🚧 Files skipped from review as they are similar to previous changes (3)
- changes/unreleased/108-execution-controls-params-skills.yaml
- docs/adr/0010-skill-content-boundary.md
- CONTEXT.md
| At **~85-90%** of any active limit (whichever is hit first), the | ||
| harness sends a wind-down prompt: | ||
|
|
||
| > You are approaching your execution limit. Wrap up your current work, | ||
| > commit what you have, and write a handoff to `.konveyor/handoff.md` | ||
| > documenting what remains. | ||
|
|
||
| At **100%**, the harness sends `session/cancel`, commits, pushes, and | ||
| exits. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve read-only behavior for sessionType: plan.
sessionType: plan promises no changes, but the wind-down prompt says “commit what you have” and the 100% path commits and pushes. Make these actions conditional on sessionType: execute. Plan runs must cancel without write or Git operations.
🤖 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/0011-execution-controls-mode-and-session-type.md` around lines 205 -
213, Update the execution-limit behavior described in the ADR so commit, push,
and wind-down write instructions occur only when sessionType is execute.
Preserve cancellation at 100% for sessionType plan, but ensure plan runs perform
no file, commit, or Git push operations.
| The harness records cumulative usage on the AgentRun status: | ||
|
|
||
| ```yaml | ||
| status: | ||
| usage: | ||
| turns: 47 | ||
| inputTokens: 35000 | ||
| outputTokens: 12000 | ||
| totalTokens: 47000 | ||
| cost: "0.045" | ||
| ``` | ||
|
|
||
| This is populated from ACP `usage_update` notifications and is | ||
| runtime-agnostic — any ACP-compliant agent sends these. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TARGET="docs/adr/0011-execution-controls-mode-and-session-type.md"
echo "== file exists =="
ls -l "$TARGET" || true
echo "== target lines =="
sed -n '200,245p' "$TARGET" | cat -n -v
echo "== nearby enforcement references used/size/usage_update/inputTokens/outputTokens/totalTokens =="
rg -n "used|size|usage_update|inputTokens|outputTokens|totalTokens|AgentRun status|cumulative usage|enforcement" "$TARGET" || trueRepository: konveyor/agentic-controller
Length of output: 3247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TARGET="docs/adr/0011-execution-controls-mode-and-session-type.md"
ACPA="docs/adr/0008-acp-notation.md"
echo "== surrounding ACP notation section =="
sed -n '1,80p' "$ACPA" | cat -n -v
echo "== usage_update definitions/references in ADRs =="
rg -n -C 3 "usage_update|used|size|input.*token|output.*token|total.*token|inputTokens|outputTokens|totalTokens|usage" docs/adr -g '*.md'Repository: konveyor/agentic-controller
Length of output: 277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ADR files with notation/acp in name =="
git ls-files 'docs/adr/*.md' | rg -n "notation|acp|execution-controls" || true
echo "== usage_update definitions/references in docs/adr =="
rg -n -C 3 "usage_update|used|size|input.*token|output.*token|total.*token|inputTokens|outputTokens|totalTokens|usage" docs/adr -g '*.md' || true
echo "== target file status/usage context =="
sed -n '210,235p' "docs/adr/0011-execution-controls-mode-and-session-type.md" | cat -nRepository: konveyor/agentic-controller
Length of output: 17970
Define inputTokens and outputTokens sources.
usage_update uses used, size, and optional cost, but AgentRun status records inputTokens, outputTokens, and totalTokens. Either add ACP tokens for these dimensions or document the deterministic mapping for used and size. Otherwise harnesses may produce incompatible usage status.
🤖 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/0011-execution-controls-mode-and-session-type.md` around lines 220 -
233, Update the AgentRun usage section around the ACP usage_update description
to explicitly define how used and size map to inputTokens, outputTokens, and
totalTokens, including handling of optional cost. If ACP does not provide
separate input/output dimensions, document the deterministic convention or add
corresponding ACP fields so all harnesses produce compatible status values.
…#105) **Problem.** The UI needs "show me this application's runs" (the application detail drawer, konveyor/tackle2-ui#3521). Runs live only in etcd — this ADR keeps no database copies — so when Hub filters a run list, it has exactly two options: pass a label selector to the apiserver, or fetch every run and scan it in the handler. The application id currently lives only in `spec.env`, so there is nothing to select on: the scan is the only option, on every request (reads are request-driven, no informer cache), and it couples Hub's query path to the pod's input format — an env surface #108 is actively reshaping. **Fix** (14 lines): - Hub writes the id as a label at create time: `konveyor.io/application: "42"` - Run list endpoints take `?application=42`, answered by a label selector `APP_ID` doesn't change — the harness still reads it inside the pod. The label is the same value, put where list queries can use it. It also works outside Hub: `kubectl get agentruns -l konveyor.io/application=42` answers the same question when debugging a cluster. Edge cases recorded in the diff: runs created before the label aren't matched; an invalid filter is an error instead of an unfiltered list; workflow stage runs don't get the label yet (#107). Already working in the shim prototype: ibolton336/agentcontroller-client#3. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: ibolton336 <ibolton@redhat.com>
ibolton336
left a comment
ibolton336
left a comment
There was a problem hiding this comment.
Read all three closely — the decomposition is clean, and nearly every claim I checked against main and the ACP v2 schema holds up (the no-cancel-on-limit behavior in session.go, KONVEYOR_PARAM_MAX_TURNS being the sole param read, tee fail-closed deny on no-viewers/timeout, the usage_update/Cost shapes). Four things worth resolving while these are still proposed:
1. maxTokens is specced against a context-occupancy metric. ACP v2 defines UsageUpdate.used as "Tokens currently in context" and size as the window size — occupancy, not cumulative consumption. So maxTokens: 500000 on a 200k-window model can never trigger, and used drops on compaction exactly when a budget matters most. cost is cumulative per the spec ("Total cumulative cost for session"), so maxCost is sound. Related: status.usage.inputTokens/outputTokens/totalTokens can't be populated from usage_update — it carries no input/output split and no cumulative total. Suggest enforcing budgets via maxCost + maxTurns and re-speccing status to what's observable (a used/size snapshot plus cumulative cost) — or explicitly redefining maxTokens as a context-occupancy threshold if that's the intent.
2. The 85–90% wind-down prompt needs mid-turn injection. The harness sends one prompt and blocks for the whole turn (ADR 0007), so at 85% the turn is in flight — delivery rides the steer path (_goose/unstable/session/steer, gated on customNotifications, goose ≥ 1.39). Worth naming that dependency: on a runtime without mid-turn injection, graceful wind-down doesn't exist and only cancel-at-100% is enforceable.
3. Who writes status.usage? The harness has no kube client or AgentRun RBAC today; the controller owns status. Options with very different consequences: pod termination message (controller reads it on completion), the controller attaching to the tee as a viewer, or granting sandbox pods status-patch RBAC (a real security decision — the sandbox runs untrusted agent output). The ADR should pick one.
4. A limit-hit run reads as a clean success. Wind-down commits, pushes, exits — nothing in the specced status says why the run ended. We already hit this class with the stage-gate experiment (a refusal reported Succeeded). Suggest an outcome marker alongside usage (limitReached: maxTurns, or a condition) so Hub/UI/stage sequencing can tell truncated from complete.
Smaller notes, take or leave: goose on current main still emits usage on _goose/unstable/session/update with contextLimit rather than the stable size (see the tee_test.go fixture) — worth a consequence bullet for that migration, plus defining behavior when a limit is set but the runtime never sends the metric (silent non-enforcement is the worst outcome); prompts containing literal {{ (Helm content is plausible in a migration product) fail template rendering with no escape hatch, and Go's default renders missing keys as <no value> rather than erroring — specify missingkey=error; maxCost as Quantity serializes as a string on the CRD while the params.json example shows a JSON number — pin the contract type; Claude Code's plan flag is --permission-mode plan, not --plan.
|
Addendum after a second pass — and after reading the full inline thread: a couple of my earlier "smaller notes" overlap CodeRabbit's inline comments (rendering-failure behavior, 5. The run-level 6. There's no plan-gate. 7. 8. Also +1 CodeRabbit's catch that the wind-down prompt ("commit what you have") and the 100% commit-and-push path contradict |
e2c5759 to
5c0c44f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/0009-parameter-delivery-via-params-json.md`:
- Around line 41-43: Update the ADR’s migration section to define the boundary
for the params.json delivery contract: require the controller changes in
agentrun_controller.go and agentworkflowrun_controller.go and corresponding
harness updates to land together, or explicitly state that the ADR remains
inactive until they are complete. Clarify that existing Sandbox behavior and new
harness implementations must not use different parameter contracts.
- Around line 95-100: Update the ADR’s template-reference rules to define the
exact grammar for recognized $(agent.<name>) and $(workflow.<name>) references,
including valid parameter-name constraints, and specify an escape sequence for
intentional literal $( text. Make non-escaped malformed or undeclared references
fail validation rather than pass through unchanged.
- Around line 60-64: Update the execution example to represent maxCost as the
string "10.00" rather than a JSON number, and add the required reference to ADR
0011 alongside the execution-control description. Preserve the other execution
fields and example values unchanged.
In `@docs/adr/0011-execution-controls-mode-and-session-type.md`:
- Around line 150-157: Update the mode precedence behavior described in the
execution-controls example so a run-level mode override cannot weaken a stage’s
supervision requirement: preserve mode: approve when the stage requires human
approval unless the override is explicitly authorized, while retaining allowed
strengthening overrides such as auto. Update the surrounding explanation to
document this monotonic precedence rule.
- Around line 21-24: Update the ADR’s maxCost policy to define fail-closed
handling when usage_update omits cost data or reports a non-USD currency: reject
maxCost as unsupported before execution begins, rather than proceeding without
enforceable USD cost data. Document that maxCost is accepted only when a valid
USD cost.amount is available.
- Around line 132-142: Update the ADR section describing mode: approve and the
plan-to-execute workflow to explicitly document where plan approval occurs:
state whether the harness/tee exposes a gate between stages, or clarify that
approval is outside the session controls covered by this ADR. Keep the workflow
example and session-control definitions consistent with that stated behavior.
- Around line 177-181: Update the mode controls table in the ADR to document ACP
version-specific translation: ACP v1 uses session/set_mode, while ACP v2 uses
session/set_config_option. Add a separate row or note and clarify that headless
approve mode depends on the ACP method supported by the runtime.
- Around line 51-52: Standardize enforcement limits on notification turns:
update the ADR’s maxTurns definition, the CRD/client counter, and runtime
mapping so each tool_call session/update notification counts as one unit. Ensure
SendPrompt, cfg.MaxTurns, and native limits such as GOOSE_MAX_TURNS use the same
unit, including for parallel tool requests.
🪄 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: 0f651dab-ce9c-4453-9975-1b6aea053cac
📒 Files selected for processing (5)
CONTEXT.mdchanges/unreleased/108-execution-controls-params-skills.yamldocs/adr/0009-parameter-delivery-via-params-json.mddocs/adr/0010-skill-content-boundary.mddocs/adr/0011-execution-controls-mode-and-session-type.md
🚧 Files skipped from review as they are similar to previous changes (3)
- CONTEXT.md
- docs/adr/0010-skill-content-boundary.md
- changes/unreleased/108-execution-controls-params-skills.yaml
| The controller writes all resolved parameter values to a single JSON | ||
| file mounted at `/run/konveyor/params.json` in the Sandbox container. | ||
| Individual `KONVEYOR_PARAM_*` environment variables are removed. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define the migration boundary for the new delivery contract.
The supplied controller code still emits KONVEYOR_PARAM_* variables in internal/controller/agentrun_controller.go lines [424-533]. It also delivers workflow data through controller-owned environment variables in internal/controller/agentworkflowrun_controller.go lines [360-380].
Add a migration requirement that updates the controller and harness together, or state that this ADR is not active until those changes land. Otherwise, existing Sandboxes and new harness implementations can use different parameter contracts.
Also applies to: 163-168
🤖 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/0009-parameter-delivery-via-params-json.md` around lines 41 - 43,
Update the ADR’s migration section to define the boundary for the params.json
delivery contract: require the controller changes in agentrun_controller.go and
agentworkflowrun_controller.go and corresponding harness updates to land
together, or explicitly state that the ADR remains inactive until they are
complete. Clarify that existing Sandbox behavior and new harness implementations
must not use different parameter contracts.
| If both scopes declare a param with the same name, the namespaced | ||
| references are unambiguous. The controller rejects unresolved | ||
| references during reconciliation — a reference to an undeclared | ||
| parameter is a configuration error and does not produce a Sandbox. | ||
| Literal `$(` text that does not match a declared parameter passes | ||
| through unchanged. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Define the exact grammar for template references.
The ADR rejects undeclared references but also allows $( text that does not match a declared parameter. A typo such as $(agent.source_urll) can therefore pass into the prompt as literal text.
Define recognized $(agent.<name>) and $(workflow.<name>) references, and define an escape sequence for intentional literal $( text. Otherwise prompt errors can pass silently.
🤖 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/0009-parameter-delivery-via-params-json.md` around lines 95 - 100,
Update the ADR’s template-reference rules to define the exact grammar for
recognized $(agent.<name>) and $(workflow.<name>) references, including valid
parameter-name constraints, and specify an escape sequence for intentional
literal $( text. Make non-escaped malformed or undeclared references fail
validation rather than pass through unchanged.
| - **Cost and token budgets** do not exist. The ACP protocol's | ||
| `usage_update` notification (stabilised June 2026) provides | ||
| real-time `used`, `size`, and optional `cost` data, but the harness | ||
| ignores it. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "usage_update|cost|currency|maxCost|session/cancel" .Repository: konveyor/agentic-controller
Length of output: 7524
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- ADR relevant sections ---\n'
sed -n '19,26p;48,62p;148,178p;192,216p;224,238p' docs/adr/0011-execution-controls-mode-and-session-type.md
printf '\n--- Harness implementation candidates around usage_update/cost/session/cancel ---\n'
for f in harness/internal/acp/session.go harness/internal/acp/acptest/steer_integration_test.go; do
echo "### $f"
wc -l "$f"
rg -n "usage_update|usageUpdate|cost|amount|currency|maxCost|session/cancel|Notify\\(\"session/cancel\"|cancel" "$f" -C 4
doneRepository: konveyor/agentic-controller
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ADR relevant sections ---'
sed -n '19,26p;48,62p;148,178p;192,216p;224,238p' docs/adr/0011-execution-controls-mode-and-session-type.md
printf '%s\n' ''
printf '%s\n' '--- Harness implementation candidates around usage_update/cost/session/cancel ---'
for f in harness/internal/acp/session.go harness/internal/acp/acptest/steer_integration_test.go; do
echo "### $f"
wc -l "$f"
rg -n "usage_update|usageUpdate|cost|amount|currency|maxCost|session/cancel|Notify\\(\"session/cancel\"|cancel" "$f" -C 4
doneRepository: konveyor/agentic-controller
Length of output: 7535
Reject maxCost when cost data cannot be enforced.
The ADR states maxCost is USD and uses cost.amount from optional usage_update cost objects. If a runtime omits cost or reports a non-USD currency, define and document the fail-closed behavior: reject before starting, cancel before work, or refuse maxCost as unsupported.
🤖 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/0011-execution-controls-mode-and-session-type.md` around lines 21 -
24, Update the ADR’s maxCost policy to define fail-closed handling when
usage_update omits cost data or reports a non-USD currency: reject maxCost as
unsupported before execution begins, rather than proceeding without enforceable
USD cost data. Document that maxCost is accepted only when a valid USD
cost.amount is available.
| | `maxTurns` | `int` | Maximum tool-call turns before wind-down | | ||
| | `maxCost` | `string` | Maximum cumulative cost (USD) before wind-down | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "maxTurns|GOOSE_MAX_TURNS|tool_call|turn|parallel" .Repository: konveyor/agentic-controller
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Target ADR relevant sections =="
sed -n '1,90p;110,190p;200,235p;275,350p' docs/adr/0011-execution-controls-mode-and-session-type.md | cat -n
echo
echo "== Harness maxTurns related code candidates =="
rg -n "MaxTurns|maxTurns|MAX_TURNS|tool_call|toolCall|turns|GOOSE_MAX_TURNS" harness config internal -g '*.go' -g '*.yaml' -g 'go.mod' | sed -n '1,220p'
echo
echo "== API/CRD schema snippets =="
for f in api/v1alpha1/*types.go config/crd/bases/*.yaml; do
if rg -q "MaxTurns|maxTurns|executioncontrols|ExecutionControls" "$f"; then
echo "--- $f"
rg -n "MaxTurns|maxTurns|executioncontrols|ExecutionControls" "$f" -C 4
fi
doneRepository: konveyor/agentic-controller
Length of output: 21963
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== harness/internal/acp/session.go relevant methods =="
sed -n '180,290p;300,435p' harness/internal/acp/session.go | cat -n
echo
echo "== harness internal config =="
sed -n '1,130p' harness/internal/config/config.go | cat -n
echo
echo "== migration harness main relevant flow =="
sed -n '280,320p' harness/cmd/migration-harness/main.go | cat -n
echo
echo "== CRD field occurrences only =="
rg -n "maxTurns|MaxTurns|max_turns|KONVEYOR_PARAM_MAX_TURNS|GOOSE_MAX_TURNS" -g '*.go' -g '*.yaml' .
echo
echo "== Read-only behavioral probe: notification-triggered increment semantics =="
python3 - <<'PY'
from dataclasses import dataclass
turn_limit = 10
call_start_count = 0
def is_tool_call_update(f):
return f.get("params", {}).get("update", {}).get("sessionUpdate") == "tool_call_update"
def count_tool_change(f):
global call_start_count
if not is_tool_call_update(f):
return
call_start_count += 1
def simulate_parallel_runtime_sequence():
notifications = []
for i in range(3):
call_id = f"parallel-call-{i}"
notifications.append({"params": {"update": {"sessionUpdate": "tool_call", "toolCallId": call_id}}})
notifications.append({"id": 0, "result": {"output": "tool result"}})
return count_tool_change, call_start_count, notifications
counter, reached, nf = simulate_parallel_runtime_sequence()
for f in nf:
if is_tool_call_update(f):
counter(f)
print("tool_call_update notifications in one runtime turn =", reached)
print("limit reached if counter equals maxTurns =", reached >= turn_limit)
PYRepository: konveyor/agentic-controller
Length of output: 18787
Use one notification-turn unit for enforcement limits.
maxTurns is defined as tool-call turns, but SendPrompt increments the counter on every session/update notification with sessionUpdate: "tool_call". Parallel tool requests can emit multiple such notifications in one prompt turn, so cfg.MaxTurns can stop a run before the intended turn count and disagree with native limits such as GOOSE_MAX_TURNS. Define one unit and keep it consistent across the ADR, CRD/client counter, and runtime mapping.
🤖 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/0011-execution-controls-mode-and-session-type.md` around lines 51 -
52, Standardize enforcement limits on notification turns: update the ADR’s
maxTurns definition, the CRD/client counter, and runtime mapping so each
tool_call session/update notification counts as one unit. Ensure SendPrompt,
cfg.MaxTurns, and native limits such as GOOSE_MAX_TURNS use the same unit,
including for parallel tool requests.
| mode: auto # override: CI batch run, no human | ||
| # maxTurns not set — stage/agent defaults apply | ||
| ``` | ||
|
|
||
| In this example the plan stage's `mode: approve` is overridden by the | ||
| workflow run's `mode: auto` — the CI run doesn't have a human | ||
| attached. The plan stage's `maxTurns: 100` still applies because the | ||
| workflow run doesn't override it. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Prevent a run-level override from weakening supervision.
AgentWorkflowRun.Spec.mode: auto overrides the stage's mode: approve. A run creator can therefore remove the human approval requirement defined by the workflow author. Make mode precedence monotonic so a specific value cannot weaken supervision, or require explicit authorization for this override.
🤖 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/0011-execution-controls-mode-and-session-type.md` around lines 150 -
157, Update the mode precedence behavior described in the execution-controls
example so a run-level mode override cannot weaken a stage’s supervision
requirement: preserve mode: approve when the stage requires human approval
unless the override is explicitly authorized, while retaining allowed
strengthening overrides such as auto. Update the surrounding explanation to
document this monotonic precedence rule.
|
Round 2 on 9. CONTEXT.md and the changelog now lag the ADRs. The AgentRun entry still lists 10. The turn unit is now load-bearing. Setting 11. The plan-mode contradiction survived the rewording. The new handoff prompt still opens "Commit your current work" — impossible under 12. Termination message size. It's now part of the harness contract — worth stating kubelet's 4,096-byte cap so third-party harness authors keep the blob small. Still open from the addendum, in untouched sections: 5 (run-level |
5c0c44f to
5af6e99
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Round 3 on 13. "Stages may further restrict allowed modes" has no field behind it. Stages gain 14. The drift survived a second push. CONTEXT.md's AgentRun entry still lists 15. Two one-liners. The limits table still defines |
ibolton336
left a comment
ibolton336
left a comment
There was a problem hiding this comment.
Architecture is settled for me as of 5af6e99f — declare/select/validate for mode, Agent-authoritative limits, the runtime as the sole turn counter, and the honest sessionType deferral are the right calls, and the exit-code contract (0/1/2 + LimitReached) gives the downstream outcome-reporting work a shape to extend instead of invent.
Blocking only on the mechanical residue already flagged as straggler 14: CONTEXT.md's AgentRun/Harness entries and the changelog fragment still describe the superseded draft (maxTokens, sessionType, Go-template rendering, 85–90% wind-down). Merging as-is ships a summary that contradicts its own ADRs, and implementation issues are about to derive from these documents.
On 13: either give stages allowedModes or drop the "stages may further restrict allowed modes" sentence — no strong preference, but the text and the field list need to agree before the CRD issue is cut. 15's two one-liners (turn-unit wording in the limits table; a sentence marking the 15–20% handoff reserve as advisory) are take-or-leave.
5af6e99 to
1c1b7f8
Compare
| fix it") and belongs in a skill. The distinction is between a skill | ||
| that says "make sure this compiles" (judgment) and one that says | ||
| "run `mvn compile`, check exit code, stop if non-zero" (gate). | ||
| - Filesystem discovery that depends on container layout conventions |
There was a problem hiding this comment.
goose already does this, and does it lazily. It implements Agent Skills
natively including progressive disclosure: name and description at session
start, full SKILL.md on activation, references/ and scripts/ only when the
agent reaches for them. It resolves relative paths from the skill directory.
Resolving references in the harness at startup would pull every reference file
into context on every run, which is the thing progressive disclosure exists to
avoid. skills/javaee-to-quarkus alone has 6 reference files and 6 modules.
The reason skills are doing ls today is that goose cannot see them. It
searches .agents/skills, .goose/skills and .claude/skills relative to the
working directory plus the home equivalents (all_skill_dirs() in
crates/goose/src/skills/mod.rs). We mount at /opt/skills, which is not in
that list, so the harness reads the files itself and concatenates every
SKILL.md into one prompt.
Verified with our exact serve flags, a skill at .agents/skills shows up in a
real session and one at /opt/skills does not:
$ goose run --with-builtin developer -t "List the names of every skill available to you."
- goose-doc-guide
- zebra-probe
Symlinking .agents/skills to /opt/skills is enough to fix it.
Would it be worth adding a third branch to the rule? Right now it reads skill
or harness. There is a case where the runtime already implements the thing, and
the answer is to let it rather than reimplement it. The harness is already
runtime specific everywhere else (goose serve, --with-builtin,
GOOSE_SERVER__SECRET_KEY, the GOOSE_* provider mapping), so knowing where
goose looks for skills seems like the same category of knowledge.
That gets you what the ADR wants, skills not knowing container layout, by
removing the need rather than by banning it and rebuilding it in the harness.
| build and fix what's broken is domain judgment ("does this compile? | ||
| fix it") and belongs in a skill. The distinction is between a skill | ||
| that says "make sure this compiles" (judgment) and one that says | ||
| "run `mvn compile`, check exit code, stop if non-zero" (gate). |
There was a problem hiding this comment.
does that mean we run deterministic tools as graphify update as a pre-hook step (based on the rule below) and then in the skills, can we still specify the following?
"Query the dependency graph with graphify query -m <module> --format json to find upstream dependencies before migrating a module."
"Run graphify query -m X --format json | jq '.deps[]', check if the list is empty, if so skip this module."
What the skill shouldn't say:
"Run graphify update, check exit code, stop if non-zero."
There was a problem hiding this comment.
note
"Query the dependency graph with graphify query -m <module> --format json to find upstream dependencies before migrating a module."
"Run graphify query -m X --format json | jq '.deps[]', check if the list is empty, if so skip this module."
cannot be run as hooks, as the agents determine what to do with these and the skill would be invoking cli.
There was a problem hiding this comment.
the rule lists "CLI invocation" as something that belongs in the harness, but graphify query is a CLI invocation that requires agent judgment.If it makes sense, I'd suggest changing "CLI invocation" to "deterministic CLI gates" so the rule matches the intent.
ADR 0009: Parameter delivery via /run/konveyor/params.json - Replace KONVEYOR_PARAM_* env vars with a single JSON file - Three-section shape: workflow, agent, execution - Controller renders Go templates in prompt text fields - Harness appends Parameters section to prompt ADR 0010: Skill content boundary — knowledge vs execution control - Skills contain knowledge and judgment, never execution control - Remove MAX_FIX_ITERATIONS from verify skill - Handoff on stuck is agent judgment, baked into base prompt - Rule: if a program could do it deterministically, it belongs in the harness ADR 0011: Execution controls, mode, and session type on CRDs - maxTurns, maxCost, maxTokens as first-class CRD fields - Mode: auto | approve (skip smart_approve) - Session type: plan | execute - CRD hierarchy: Agent > Stage > Run overrides - Harness monitors ACP usage_update, enforces limits, records usage - Wind-down at ~85-90% of any limit, cancel at 100% CONTEXT.md updated with new terms: Mode, Session Type, Execution Limits, Skill Content Boundary. AgentRun and Harness entries revised. Signed-off-by: David Zager <david.j.zager@gmail.com>
1c1b7f8 to
c607a65
Compare
Define the KONVEYOR_RULES wire format and the ordering constraint it implies, record the konveyor#136 workflow run as evidence for making stage drivers rules, state the cloned repo as untrusted and require the harness to log skill-name collisions, supersede ADR 0001's runtime-discovery claim in the Status line, and drop the stale instruction to ADR 0010 now that konveyor#108 carries it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…params) (#106) > **Renumbered 0012/0013** — #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 #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. #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: - **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 #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 #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](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>
Define the KONVEYOR_RULES wire format and the ordering constraint it implies, record the konveyor#136 workflow run as evidence for making stage drivers rules, state the cloned repo as untrusted and require the harness to log skill-name collisions, supersede ADR 0001's runtime-discovery claim in the Status line, and drop the stale instruction to ADR 0010 now that konveyor#108 carries it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Fabian von Feilitzsch <fabian@fabianism.us>
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>
…onveyor#108) ## Summary Three proposed ADRs and CONTEXT.md updates from a design grilling session covering parameter delivery, skill authoring boundaries, and execution controls. ### ADR 0009: Parameter Delivery via params.json Replaces per-parameter `KONVEYOR_PARAM_*` env vars with a single JSON file at `/run/konveyor/params.json`. Three-section shape: - `workflow` — workflow-level params (declared on AgentWorkflow, supplied on AgentWorkflowRun) - `agent` — agent-level params (declared on Agent, supplied on AgentRun), type-coerced - `execution` — resolved execution controls (maxTurns, maxCost, maxTokens, mode, sessionType) All prompt-composing text fields (Agent prompt, AgentRun instructions, AgentWorkflow guide and stage instructions) are rendered as Go templates by the controller with the resolved parameter map as the data context. Prior art: Ansible Playbook Bundles (`_apb_plan_parameters`), Ansible Runner (`extravars`), Tekton (`$(params.name)`). ### ADR 0010: Skill Content Boundary Refines ADR 0007's stage/domain skill taxonomy with a clear rule: > A skill contains knowledge and judgment criteria. It never contains execution control. **Allowed:** domain knowledge, approach guidance, quality criteria, output format, judgment calls. **Not allowed:** reading env vars, counting iterations, branching on exit codes, running infrastructure tools, git ops, filesystem discovery. Immediate changes: remove `MAX_FIX_ITERATIONS` from verify skill, remove env var reads, remove `git add && git commit` instructions. ### ADR 0011: Execution Controls, Mode, and Session Type on CRDs First-class CRD fields for execution concerns: | Category | Values | Purpose | |----------|--------|---------| | Execution limits | `maxTurns`, `maxCost`, `maxTokens` | Budget constraints — whichever limit is hit first triggers wind-down | | Mode | `auto` \| `approve` | Supervision policy — auto is headless, approve requires human via ACP tee | | Session type | `plan` \| `execute` | Runtime work pattern — plan uses native planning, execute does the work | CRD hierarchy (most specific wins): - Standalone: `AgentRun > Agent > system default` - Workflow: `AgentWorkflowRun > Stage > Agent > system default` Harness enforces limits by monitoring ACP `usage_update` notifications. Wind-down prompt at ~85-90% of any limit. Usage recorded on AgentRun status. ### CONTEXT.md Updated with new terms: Mode, Session Type, Execution Limits, Skill Content Boundary. Revised AgentRun and Harness entries to reflect params.json delivery and execution control semantics. ## Related - PR konveyor#99 (parameter injection to harness prompt — the starting point for this discussion) - ADR 0007 (harness as thin runner — refined by ADR 0010) - ADR 0008 (tee topology — mode and usage_update leverage) - Ian Bolton's HITL design on issues konveyor#55, konveyor#56 (mode and session type prior art) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded guidance on execution controls, parameter resolution, precedence, and runtime configuration. * Documented standardized delivery of workflow, agent, and execution parameters. * Clarified the boundary between skill content and execution control. * Defined turn and cost limits, supervision modes, and session types. * Documented usage monitoring, cancellation, wind-down behavior, and execution reporting. * Added an unreleased feature entry covering these agent execution improvements. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: David Zager <david.j.zager@gmail.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>
4 participants
Summary
Three proposed ADRs and CONTEXT.md updates from a design grilling session covering parameter delivery, skill authoring boundaries, and execution controls.
ADR 0009: Parameter Delivery via params.json
Replaces per-parameter
KONVEYOR_PARAM_*env vars with a single JSON file at/run/konveyor/params.json. Three-section shape:workflow— workflow-level params (declared on AgentWorkflow, supplied on AgentWorkflowRun)agent— agent-level params (declared on Agent, supplied on AgentRun), type-coercedexecution— resolved execution controls (maxTurns, maxCost, maxTokens, mode, sessionType)All prompt-composing text fields (Agent prompt, AgentRun instructions, AgentWorkflow guide and stage instructions) are rendered as Go templates by the controller with the resolved parameter map as the data context.
Prior art: Ansible Playbook Bundles (
_apb_plan_parameters), Ansible Runner (extravars), Tekton ($(params.name)).ADR 0010: Skill Content Boundary
Refines ADR 0007's stage/domain skill taxonomy with a clear rule:
Allowed: domain knowledge, approach guidance, quality criteria, output format, judgment calls.
Not allowed: reading env vars, counting iterations, branching on exit codes, running infrastructure tools, git ops, filesystem discovery.
Immediate changes: remove
MAX_FIX_ITERATIONSfrom verify skill, remove env var reads, removegit add && git commitinstructions.ADR 0011: Execution Controls, Mode, and Session Type on CRDs
First-class CRD fields for execution concerns:
maxTurns,maxCost,maxTokensauto|approveplan|executeCRD hierarchy (most specific wins):
AgentRun > Agent > system defaultAgentWorkflowRun > Stage > Agent > system defaultHarness enforces limits by monitoring ACP
usage_updatenotifications. Wind-down prompt at ~85-90% of any limit. Usage recorded on AgentRun status.CONTEXT.md
Updated with new terms: Mode, Session Type, Execution Limits, Skill Content Boundary. Revised AgentRun and Harness entries to reflect params.json delivery and execution control semantics.
Related
Summary by CodeRabbit