feat(acp): harness v2 ACP runtime platform with isolated v1/v2 controller modes - #306
feat(acp): harness v2 ACP runtime platform with isolated v1/v2 controller modes#306Sertaç Özercan (sozercan) wants to merge 39 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Codex (@codex) review Please review the current head |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 305e0149ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review Please review the current head |
|
Codex (@codex) review Please review the current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b5b53f33c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Codex (@codex) review Please review the current head |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback". |
|
Codex (@codex) review Please review the current head |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e41031318
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
…ipts, and CI Go cleanup (no functionality change, verified by deadcode -test + whole-repo reference checks): - delete the pre-cutover runtime-secret discovery API and orphaned secret-name constants, unused exported v2 client/protocol/lifecycle helpers, unused functional options in cliwrapper and the event recorder, and test-only wrappers (retargeting their tests to the real entry points) - collapse the impossible integer type-switch in chat agent-model parsing (plain json.Unmarshal only ever yields float64), the duplicate built-in runtime model-limit validators, and a subsumed OpenCode systemPrompt guard - extract one shared OpenCode model-spec validator behind both the create and chat tool surfaces with byte-identical error messages - de-abstract artifactcap ReplayLedger to the sole *FileLedger implementation - move the test-only PlanACPRuntime wrapper into a _test.go helper ACP runtime consolidation: - share one BuiltInRuntimeAdapterDigests table in internal/acp between the controller runtime profile and the supervisor (removes a silent-drift hazard) - replace per-image Dockerfile grep pin assertions and the two divergent dockerfile tests with one table-driven suite covering all four runtimes, asserting digest-pinned bases, a unified forbidden-term list, and that every pin constant appears in its Dockerfile - extract the repeated admin-handler guard, operation-replay writer, and adapter workdir resolution in cliwrapper/supervisor; generate the per-runtime docker build/push targets in the Makefile Scripts and CI: - add scripts/lib/e2e-common.sh and scripts/lib/redact.sh; the redact union covers strictly more than each of the six divergent per-script redactors - add .github/actions/free-disk-space and setup-kind composite actions; kind downloads are now checksum-verified in all 12 workflows (previously 4), and live-copilot-proxy-e2e no longer fetches an unpinned latest kind - run every scripts/tests/*-test.sh suite and bash -n every shell script in coexistence-smoke (previously 13 of ~40 files, 0 of 7 suites executed); fix two stale suite assertions that targeted the promoted chart instead of manifest_staging - wire the helm-chart package-comparison pytest into helm-chart.yml, align agent-substrate-e2e to go-version-file, and register the missing outboundaccesspolicy sample in config/samples
Signed-off-by: Sertac Ozercan <sozercan@gmail.com> # Conflicts: # go.mod # go.sum
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ff8122a61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
…, and CodeQL findings Controller settlement (return durable-transition errors before terminal Task projection, so store conflicts/outages leave Tasks retryable instead of terminal above nonterminal attempts): - pre-prompt client errors, workspace-unsupported settlement, prompt stream rejection, cancelled-before-acceptance, proven CancelPrompt settlement, and the workspace-validation delivery conflict in the ACP dispatcher - classify outbox delivery failures: only payload-intrinsic errors may dead-letter; infrastructure failures stay Pending with capped backoff so terminal projections remain deliverable after control-plane recovery - wire the production outbound-access resolver and transaction-exchange config through the ACP MCP broker so brokered custom Tools with outboundAccessPolicyRef resolve governed egress Helm chart sources (restore documented keys the v2 cutover dropped while the controller flags and docs kept them): - controller.contextToken.tts.endpoint (was renamed to tts.url), the contextToken.scopes.secretCredentialRead rendering, the transaction-token profile comments, and the controller.outboundAccess trust allowlists Other: - publisher client honors caller deadlines instead of clamping every request to 3 minutes; the former value remains a fallback for deadline-less calls - re-include the security-scan fake-ACP fixture subtree in .dockerignore - bound integer conversions flagged by CodeQL: forge PR numbers parse as int32, child UID/GID parse with bitSize 31, chat maxTurns validates [1,1000] in both schema and enforcement Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d1cfe7668
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
- rename the fake kubectl's awk namespace variable: gawk 5 reserves "namespace" as a builtin, so the wrapper-cleanup delete path died on GitHub runners while passing under mawk and BSD awk - pin agent-substrate-e2e setup-go back to 1.26.3: the Substrate clone's go.mod floor is newer than this repository's go directive, and the E2E runs with GOTOOLCHAIN=local, so go-version-file cannot be used here Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
…put validation - bind per-request Task transaction authority in the ACP MCP broker's custom-Tool executor: under context-token enforcement the executor now carries the authenticated Task's credential-read scopes, Secret constraint, and owner-referenced transaction token (exact owner-reference check), failing closed when any cannot be resolved; enforcement-off behavior is unchanged. Zero-valued authority previously skipped the scope/constraint checks entirely because ValidateCredentialAuthority returns nil when not enforced. - classify outbox identity mismatches (recreated Task UID, incompatible v1 binding) as permanent so MaxAttempts bounds their retries before dead-lettering, while the bounded retries still cover in-progress restore settlement - reject non-integral and non-finite float64 values in chat integer tool arguments instead of silently truncating (maxTurns 1.9 no longer runs as 1) Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97755a78df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
cmd.Wait closes StdoutPipe/StderrPipe as soon as the process exits, racing the io.Copy goroutines and silently truncating or dropping child output on slow machines (observed as an empty ExactStdout in CI). Use parent-owned os.Pipe ends instead: the child inherits duplicated write ends, the parent closes its copies after Start, and the read ends stay open until the copies drain to EOF or waitForPipeCopies times out — preserving the existing bounded-wait and process-group termination semantics. Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db39451643
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
…ight, and pipe lifecycle - bound external-effect calls to the lease-accountable duration: every brokered/publisher effect call is clamped to lease minus one minute, and consequential custom Tools whose spec.http.timeout exceeds that bound are rejected at ACP exposure time, so an effect can no longer outlive its ledger lease and strand a late success as permanently outcome-unknown; the constraint is documented on the Tool CRD timeout field - mirror controller workspace preflight rules in create_agent_task (write intent requires gitRepo, createPR requires prBaseBranch and forgeCredentialRef, publication/read credential dependencies, branch-ref validity) and reject custom-image publication in create_container_task, so LLM tools no longer create Tasks that dispatch later rejects; controller-side validation stays authoritative - close the parent-owned pipe readers on every CommandRunner path, not just the bounded-wait timeout path, so successful commands do not leak two file descriptors per turn Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4143cd7bc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
The script jobs now syntax-check every shell script and execute every scripts/tests/*-test.sh suite, but the paths filters still enumerated a subset — a PR touching only scripts/upgrade-orka-crds.sh ran none of its tests. Replace the enumerated script entries with scripts/** in both the push and pull_request filters. Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bb94863e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| executor := worker.NewToolExecutorForNamespace(namespace, kubeClient, nil, outboundAccessResolver) | ||
| executor.SetTransactionExchangeConfig(brokeredTransactionExchange) | ||
| execCtx := worker.WithToolCallID(ctx, request.ToolCallID) |
There was a problem hiding this comment.
In harness-v1 mode with context-token authorization enforced, a brokered custom Tool using authSecretRef or transaction-token outbound policy gets this fresh executor with only exchange configuration and no Task-scoped credential constraint, scopes, or owner-referenced token. Separate from the fixed harness-v2 broker path, ValidateCredentialAuthority therefore sees enforcement as false and permits the controller client to read a Tool Secret outside the Task's frozen authority, while token exchange cannot use the Task's delegated authority; propagate the verified Task through the broker executor and call both transaction-authority setters before execution.
AGENTS.md reference: AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — and this exact gap is already fixed in the follow-up PR #368 (targeting this branch): the closure here is replaced by a concrete per-request executor that binds the dispatcher-verified Task's credential-read scopes, Secret constraint, and owner-referenced transaction token via helpers shared with the fixed v2 ACP MCP broker, failing closed under enforcement (KubernetesHarnessV1BrokeredToolExecutor + bindVerifiedTaskTransactionAuthority, covered by a 9-case fail-closed test matrix). Leaving this thread open until #368 merges into this branch, at which point the fix lands here.
…l preflight, and effect windows - build every standalone terminal Task projection (prompt failure and cancellation, delivery failure, post-execution cancellation, and the success path) through a shared helper that deep-copies the frozen execution status and overlays only the terminal classification: the sparse hand-built payloads omitted the RuntimeSession supervisor boot ID, profile/MCP/workspace digests, and in two cases the request digest, which ValidateRestoredProjection surfaces as a permanent ErrConflict during PromptAttempt reclamation, leaving terminal Tasks undeletable; a round-trip regression test validates each classification and proves the old sparse payloads fail closed - mirror the Job builder's full container-publication rule set against the effective post-inheritance workspace in create_container_task, rejecting inherited createPR, expectedRemoteSHA, and clean-room policy fields with an error naming the inherited field - canonicalize GitHub-style SSH scan repository URLs to credential-free HTTPS clone URLs at scan Task construction (reusing the repository-monitor parsing machinery) and compare patch-publication verification in the same canonical form, so SSH-specified RepositoryScans execute instead of settling WorkspaceUnsupported - size external-effect leases from each effect's actual bounded call duration: publisher-backed kinds honor a controller-visible ORKA_PUBLISHER_PUBLISH_TIMEOUT while brokered tool kinds keep the fixed clamp their descriptors were admitted under, the publication settlement and delivery windows grow with the configured timeout, and the no-effect-outlives-its-lease invariant holds by construction with byte-identical defaults Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5632fe018
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
…ffect budgets - canonicalize GitHub SSH roots and reject non-HTTPS or credential-embedding repository URLs in create_agent_task (gitRepo and publicationGitRepo), mirroring the controller preflight's acceptance rule so accepted URLs can no longer fail deterministically after Task creation - gate delegate_task read-credential resolution on a non-empty gitRepo and reject an explicit readCredentialRef without one, mirroring the create_agent_task behavior - budget the publication settlement and delivery windows for the full three-stage sequential publisher sequence (publish, verify, PR reconcile) instead of a single call, with the arithmetic documented and pinned by test - size harness-v1 brokered read-tool effect deadlines and leases from the Tool's own spec.http.timeout (read-class Tools legitimately bypass the consequential 4m admission cap), and stop committing deadline/authority cancellations to the effect ledger as terminal tool failures: the entry stays in-flight so a post-expiry retry re-executes the read call or reconciliation classifies it outcome-unknown, while deterministic executor failures still commit and replay Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e568795022
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| publication, err = d.transitionPublication(ctx, publication, fence, store.PublicationVerifying, | ||
| publishReceipt.OperationID, publishReceipt.RequestDigest, nil, publishReceipt, nil, "") |
There was a problem hiding this comment.
When pre-publish work consumes most of deliveryCtx, the fresh settlementCtx can still let the push succeed, but this transition uses the old, expired ctx. The resulting generic deadline error makes the caller terminalize the PromptAttempt as DeliveryConflict while the Publication remains Publishing; same-epoch recovery then sees an already-terminal delivery and skips reconcilePersistedPublication, leaving the successful push unverified and cleanup blocked. Use settlementCtx for all post-publishing-CAS transitions and reclamation.
AGENTS.md reference: AGENTS.md:L98-L98
Useful? React with 👍 / 👎.
| } | ||
| workspace := &corev1alpha1.WorkspaceConfig{ | ||
| Intent: intent, | ||
| GitRepo: dc.args.Workspace.GitRepo, |
There was a problem hiding this comment.
When delegate_task targets an agent runtime and receives an accepted-looking repository such as git@github.com:owner/repo.git or an HTTP URL, this copies it verbatim and creates the child Task. validateACPWorkspacePreflight later accepts only credential-free HTTPS URLs, so the child deterministically fails before prompt execution; apply the same canonicalization and runtime validation used by create_agent_task before creation.
AGENTS.md reference: AGENTS.md:L75-L75
Useful? React with 👍 / 👎.
| if workspace.Intent == corev1alpha1.WorkspaceIntentWrite && publicationCredential == "" { | ||
| return fmt.Errorf("workspace publicationCredentialRef is required for write intent") |
There was a problem hiding this comment.
When a caller supplies intent: "write" plus publicationCredentialRef but omits gitRepo, or requests createPR without prBaseBranch or forgeCredentialRef, this is the only write validation and the child Task is created successfully. The controller preflight then rejects those configurations before execution, so validate the full write/PR prerequisite set in the tool's schema and Execute path before creating the Task.
AGENTS.md reference: AGENTS.md:L75-L75
Useful? React with 👍 / 👎.
| return | ||
| } | ||
|
|
||
| const workspace: Record<string, unknown> = { intent: workspaceIntent } |
There was a problem hiding this comment.
When the agent form is submitted without any workspace inputs, this still attaches {"intent":"read"} to the Task; taskWorkspaceCreateOptions.build does the same for the CLI. In harness-v1 mode, any non-nil workspace is treated as a request for the compatibility wrapper's public repository lane and therefore requires gitRepo, so otherwise valid prompt-only Tasks created through either client fail before execution. Only serialize workspace when the user actually configures a workspace field.
Useful? React with 👍 / 👎.
Summary
orka.harness.v1wrapper with the session-centricorka.harness.v2supervisor contract for built-in agent runtimesharness-v1/harness-v2controller modes so both harnesses can operate on one cluster as independent installations with disjoint namespaces, state, and identityRuntimePoolresources, exact-Pod routing, admission, drain, replacement, and scale-to-zero lifecycle managementAgentRuntimev2 conformanceWhy
The v1 harness was organized around individual turns and a generic wrapper process. It did not provide the exact instance/session fencing, durable replay semantics, workspace-governance boundary, clean-room publication ownership, or recovery model needed for long-lived shared runtimes and safe write workloads.
V2 makes Orka the governance and publication control plane. Runtime Pods own only short-lived provider processes and isolated session state; Orka owns durable attempts, authorization, workspace effects, external publication, recovery, and final outcome projection.
Security and governance
publication_preparedto retirement-onlyfinalizing; they never return to prompt-admittingidleCoexistence
This is not a hard cutover. Harness v1 and harness v2 run side by side on one cluster as two independent installations:
--controller.mode=harness-v1orharness-v2) plus a non-empty watched namespace labeled with the same mode; a missing or mismatched label fails startupdual,auto, or drain mode, and a release never changes mode in placewebsite/docs/operations/harness-modes.md)harness-v2installation, built-in agent runtimes (Codex, Claude, Copilot, OpenCode) use only the ACP RuntimePool path — there is no per-Task Job or v1 execution fallback in that modeExternal
runtimeRefregistrations are v2-only: registration and conformance exist, and Task dispatch fails closed until the external v2 dispatcher is wired.Validation
make manifests generatemake verify-helm-crds test-helm-crd-syncmake lint-fix— 0 issuesmake test— all non-E2E Go packages passedcd ui && bun run lint && bun run testbash -n scripts/*.sh scripts/tests/*.shLive ACP Runtime E2Eworkflow builds the controller and all four built-in runtime images and executes live Codex, OpenCode, Claude, and Copilot RuntimePools through the canonical smoke validator (trusted default-branch changes, nightly, or manual dispatch)Coexistence Smokeworkflow and per-push E2E suites are green on the current branch headDeployed validation images (July 27 gate, historical)
sha256:20cf85b9378d8facc6504a921487d603ea953435591cd3288e51448142b50dfcsha256:87a624d1d2148f3e1172626d55159eaf0b9e53a457673c9c8b587241a46ffbb9sha256:94aef3d27e3d9c35987c1eb197a039fef601b09db9af6c7214d08533b6a6b1f8sha256:d6456cdad738e38c7ef840a10289e2919903de60f69229bbca4c2edefe196500Explicitly deferred
Task.spec.execution.workspacecurrently fails closed in both modes)