Skip to content

feat(acp): dispatch external harness v2 runtimes - #487

Merged
Sertaç Özercan (sozercan) merged 88 commits into
mainfrom
feat/agentkit-harness-v2
Sep 8, 2026
Merged

Sertaç Özercan (sozercan) merged 88 commits into
mainfrom
feat/agentkit-harness-v2

Conversation

@sozercan

@sozercan Sertaç Özercan (sozercan) commented Sep 3, 2026 •

Copy link
Copy Markdown
Member

External AgentRuntime references can now use Orka's authenticated orka.harness.v2 session path, including AgentKit and Foundry runtimes. Dispatch freezes the registered profile and original runtime identity, checks operation capabilities and fences, and preserves streaming, cancellation, recovery and fail-closed admission.

Add the Foundry supervisor composition and broker integration, preserve permission callbacks for existing sessions during conformance, and retain exact cleanup authority across controller restarts. Fix deletion of denied repository Tasks when the workspace publisher is disabled.

After a broker cleanup failure, keep admission closed and return the resident session to a state that permits another authenticated cleanup attempt. On the first pool failure, schedule eligible idle peers for cleanup once; subsequent failures leave their residents available for authenticated retries without starting another automatic cleanup wave. AgentRuntime deletion retries one exact poisoned session using its retained authority and full fence, then requires a later authenticated quiescent status before releasing the finalizer. Retirement preserves the original prompt outcome and deletion replay history.

Regression tests cover concurrent registered policies, late Tool destination/schema/effect/endpoint drift, transient and persistent cleanup failure, idle peer retirement after task DELETE and automatic cleanup failure, concurrent and stale deletion requests, unchanged broker retirement context, successful deletion replay, and cleanup authority changes. The original ambiguous effect remains OutcomeUnknown with one attempt; metadata-only changes preserve normal execution and durable replay.

Validation:

  • make lint-fix and make test.
  • Focused controller cleanup tests with -race.
  • Linux/amd64 supervisor and Foundry cleanup regressions with -race on the remote builder.
  • Existing regressions cover retained authentication Secrets, controller epochs, cleanup ownership, permission delivery, conformance and prompt renewal.

Companion PRs: AgentKit and Foundry runtime.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Recovery can panic on an unobserved replacement runtime and can adopt authority that was not frozen into the Task binding.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 High severity · 1 Medium severity · 1 Low severity

New issues introduced by this change (3)
Severity Finding
High severity internal/​controller/​acp_recovery.go — When an AgentRuntime is replaced and its new object has no observed capabilities yet, the UID…
Medium severity internal/​controller/​agent_execution_plan.go — Task.spec.agentRuntime.maxTurns is still accepted here, but external dispatch sends…
Low severity website/​docs/​guides/​bring-your-own-agent-runtime.md — This newly enabled behavior leaves two user-facing pages contradictory:…
What changed in this PR

Adds authenticated harness v2 dispatch for external AgentRuntime references and AgentKit ACP runtime support.

Changes:

  • Dispatches external runtimes with frozen authority and operation fencing.
  • Adds AgentKit supervisor integration and container targets.
  • Expands tests, samples, and BYO runtime documentation.
File Description
workers/​acp/​supervisor/​server.go Allows sessions without per-task agent configuration.
workers/​acp/​supervisor/​provider_proxy.go Adds AgentKit proxy routing and limits.
workers/​acp/​supervisor/​env.go Configures AgentKit capabilities and profiles.
workers/​acp/​supervisor/​env_test.go Tests AgentKit environment configuration.
workers/​acp/​supervisor/​agentkit_provider.go Implements the AgentKit provider adapter.
workers/​acp/​supervisor/​agentkit_provider_test.go Tests AgentKit sessions and capabilities.
workers/​acp/​supervisor/​agentkit_provider_proxy_test.go Tests AgentKit proxy routes.
workers/​acp/​images/​dockerfile_test.go Verifies AgentKit image hardening.
workers/​acp/​images/​agentkit/​Dockerfile Builds the composed AgentKit runtime image.
website/​docs/​guides/​bring-your-own-agent-runtime.md Documents external runtime dispatch.
README.md Advertises external ACP dispatch.
Makefile Adds AgentKit image build and push targets.
internal/​harness/​v2/​client.go Adds pre-mutation validation hooks.
internal/​harness/​v2/​client_stream.go Applies validation before prompt mutations.
internal/​controller/​task_controller.go Queues external runtime tasks.
internal/​controller/​task_controller_unit_test.go Updates pending-task coverage.
internal/​controller/​agent_execution_plan.go Enables external execution planning.
internal/​controller/​agent_execution_plan_test.go Tests external plan selection.
internal/​controller/​agent_execution_binding.go Freezes and verifies external authority.
internal/​controller/​acp_task_queue.go Queues external targets without pools.
internal/​controller/​acp_task_queue_test.go Removes obsolete rejection tests.
internal/​controller/​acp_recovery.go Adds external-session recovery handling.
internal/​controller/​acp_external_runtime_dispatch_test.go Covers dispatch, drift, and continuation.
internal/​controller/​acp_dispatcher.go Implements fenced external dispatch.
internal/​controller/​acp_dispatcher_test.go Updates dispatcher expectations.
config/​samples/​core_v1alpha1_agentruntime.yaml Updates sample dispatch guidance.
AGENTS.md Records the external dispatch invariant.
Suppressed comments (1)

internal/controller/acp_recovery.go:1214

  • Recovery compares only the registration UID and runtime instance ID. If the same AgentRuntime is edited and reconformed with a changed generation, endpoint, or authentication Secret authority while retaining its instance ID, this path later constructs a client from the new registration and sends cleanup mutations for the old Task. Revalidate the current registration against the Task's frozen external-runtime snapshot before any recovery mutation; otherwise a bound Task silently adopts changed authority.
		if string(runtime.UID) != execution.AgentRuntimeUID || observed.RuntimeInstanceID != execution.RuntimeInstanceID {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/controller/acp_recovery.go Outdated
Comment thread internal/controller/agent_execution_plan.go Outdated
Comment thread website/docs/guides/bring-your-own-agent-runtime.md

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

if target.pool != nil {
agentConfigurationRef = &agentConfiguration
}

P1 Badge Reject external runtimes that require session configuration

When an external runtime advertises supportsAgentSessionConfiguration: true, this branch still sends AgentConfiguration: nil; the repository's non-AgentKit supervisor rejects that request with a retryable 429, so exposing a conformant Codex, Claude, Copilot, or OpenCode supervisor through runtimeRef leaves every Task requeueing until its deadline. Conformance and validateExternalRuntimeCapabilities currently accept this capability value, so admission must require an external runtime that accepts nil configuration (or the dispatcher must supply one).

AGENTS.md reference: AGENTS.md:L92-L92

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".

Comment thread internal/controller/acp_recovery.go Outdated
Comment thread internal/controller/acp_dispatcher.go Outdated
Comment thread internal/controller/acp_dispatcher.go
Comment thread internal/controller/acp_dispatcher.go Outdated
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Copilot AI review requested due to automatic review settings September 3, 2026 21:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

External dispatch can silently ignore Agent configuration and admit runtimes incompatible with nil session configuration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2 High severity · 2 Medium severity · 2 Low severity

New issues introduced by this change (3)
Severity Finding
High severity internal/​controller/​acp_dispatcher.go — External session creation always sends AgentConfiguration: nil, but this compatibility check…
Medium severity internal/​controller/​task_controller.go — Enabling external dispatch still accepts Agent-owned configuration that this path never applies.…
Low severity internal/​controller/​acp_external_runtime_dispatch_test.go — These drift cases all mutate the registration before reserveTask, so they are rejected by binding…
Pre-existing issues (3)
Severity Finding
High severity internal/​controller/​acp_recovery.go — When an AgentRuntime is replaced and its new object has no observed capabilities yet, the UID… View comment
Medium severity internal/​controller/​agent_execution_plan.go — Task.spec.agentRuntime.maxTurns is still accepted here, but external dispatch sends… View comment
Low severity website/​docs/​guides/​bring-your-own-agent-runtime.md — This newly enabled behavior leaves two user-facing pages contradictory:… View comment

Comment thread internal/controller/acp_dispatcher.go Outdated
Comment thread internal/controller/task_controller.go
Comment thread internal/controller/acp_external_runtime_dispatch_test.go

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7db0e78715

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".

Comment thread internal/controller/agent_execution_plan.go
Comment thread internal/controller/agent_execution_binding.go
Comment thread internal/harness/v2/client.go Outdated
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Copilot AI review requested due to automatic review settings September 3, 2026 21:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The cross-cutting authentication, fencing, recovery, and external-runtime lifecycle changes warrant final human security and architecture review.

Review tier: Balanced
Findings: 1 High severity · 1 Medium severity

Pre-existing issues (2)
Severity Finding
High severity internal/​controller/​acp_dispatcher.go — External session creation always sends AgentConfiguration: nil, but this compatibility check… View comment
Medium severity internal/​controller/​task_controller.go — Enabling external dispatch still accepts Agent-owned configuration that this path never applies.… View comment
Issues resolved since last review (4)
Severity Finding
Low severity internal/​controller/​acp_external_runtime_dispatch_test.go — These drift cases all mutate the registration before reserveTask, so they are rejected by binding… View resolved comment
Low severity website/​docs/​guides/​bring-your-own-agent-runtime.md — This newly enabled behavior leaves two user-facing pages contradictory:… View resolved comment
Medium severity internal/​controller/​agent_execution_plan.go — Task.spec.agentRuntime.maxTurns is still accepted here, but external dispatch sends… View resolved comment
High severity internal/​controller/​acp_recovery.go — When an AgentRuntime is replaced and its new object has no observed capabilities yet, the UID… View resolved comment

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02d107b310

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".

Comment thread internal/controller/acp_dispatcher.go Outdated
Comment thread internal/controller/task_controller.go Outdated
Comment thread internal/controller/agent_execution_binding.go
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Copilot AI review requested due to automatic review settings September 3, 2026 22:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Live dispatch fails to reject drift in supportsAgentSessionConfiguration before initializing its frozen capability envelope.

Review tier: Balanced
Findings: 1 High severity

Pre-existing issues (1)
Severity Finding
High severity internal/​controller/​acp_dispatcher.go — External session creation always sends AgentConfiguration: nil, but this compatibility check… View comment
Issues resolved since last review (1)
Severity Finding
Medium severity internal/​controller/​task_controller.go — Enabling external dispatch still accepts Agent-owned configuration that this path never applies.… View resolved comment
Suppressed comments (1)

internal/controller/acp_dispatcher.go:4221

  • The initial live capability check does not enforce the external-runtime contract that supportsAgentSessionConfiguration remains false. A runtime can pass conformance with it false, flip it to true before the first dispatch, and have that drift accepted because expectedCapabilities is initialized from this already-drifted response; subsequent pre-mutation checks then preserve the wrong envelope. Reject this flag here just as conformance does before sending a nil AgentConfiguration.
		!capabilities.Provider.SupportsCancel || !capabilities.Provider.SupportsPermissions || !capabilities.Provider.SupportsTools {

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Copilot AI review requested due to automatic review settings September 3, 2026 22:25

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 418d617ae1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".

Comment thread internal/controller/acp_task_queue.go Outdated
Comment thread internal/controller/acp_dispatcher.go
Comment thread internal/controller/acp_dispatcher.go

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

External capability supersets, capability drift, and preserved legacy workspace fields are not handled safely.

Review tier: Balanced
Findings: None

Issues resolved since last review (1)
Severity Finding
High severity internal/​controller/​acp_dispatcher.go — External session creation always sends AgentConfiguration: nil, but this compatibility check… View resolved comment
Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

internal/controller/acp_dispatcher.go:4223

  • The conformance gate now requires external runtimes to advertise supportsAgentSessionConfiguration: false, but this dispatch-time validation does not enforce that field. A previously-ready registration can therefore drift to true and still be accepted and frozen before mutation, even though Orka sends a nil configuration. Mirror the conformance check here so dispatch remains fail-closed during upgrade/reconcile races.
    internal/controller/agent_execution_binding.go:608
  • A conformant runtime may advertise provider/model supersets, but observedCapabilitiesFromConformance leaves these scalar status fields empty unless each list has exactly one entry. This makes a real ready superset registration fail binding here, despite the new live-capability test explicitly accepting it. Rely on the already-verified profile digest for provider/model identity (or change the status projection to preserve the matching values).

internal/controller/task_controller.go:3553

  • This restriction block does not reject the preserved agentRuntime.workspace field. Although new objects cannot introduce it, upgraded stored Tasks may still carry it; enabling external dispatch now runs those Tasks while silently ignoring that legacy workspace authority. Reject it here as the harness-v1 binding path does, preserving Task.spec.workspace as the only repository surface.
	if task != nil && task.Spec.AgentRuntime != nil {
		if task.Spec.AgentRuntime.MaxTurns != nil {
			return fmt.Errorf("runtimeRef custom runtimes do not support maxTurns; iteration limits are fixed by the registered runtime profile")
		}

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Copilot AI review requested due to automatic review settings September 3, 2026 22:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Multi-provider runtimes cannot bind after passing conformance, and the AgentKit image drops its required adapter digest.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2 High severity

New issues introduced by this change (2)
Severity Finding
High severity internal/​controller/​agent_execution_binding.go — A runtime that advertises multiple provider kinds or models passes conformance and the live…
High severity workers/​acp/​images/​agentkit/​Dockerfile — The Make target requires and passes AGENTKIT_ADAPTER_DIGEST, and supervisor startup rejects a…

Comment thread internal/controller/agent_execution_binding.go
Comment thread workers/acp/images/agentkit/Dockerfile Outdated
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Runtime cleanup violates the epoch-interlock contract, and generated harness-v1 policy plus MCP metadata validation remain incorrect.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 1 High severity · 1 Medium severity

New issues introduced by this change (1)
Severity Finding
High severity internal/​store/​kube/​session_cleanup.go — ReclaimSession invokes this callback while holding the controller-epoch mutation interlock, but…
Pre-existing issues (1)
Severity Finding
Medium severity workers/​acp/​images/​foundry/​Dockerfile — The new Foundry composition is not covered by either the built-in Dockerfile table or the new… View comment
Issues resolved since last review (4)
Severity Finding
Medium severity cmd/​orka-harness-v2-e2e-fixture/​main.go — This fixture reuses the same supervisor boot ID after a container restart. Because harness-v2… View resolved comment
Low severity workers/​acp/​images/​README.md — The image overview now documents the AgentKit composition as the only opt-in composition, but this… View resolved comment
Medium severity workers/​acp/​images/​dockerfile_test.go — This adds policy assertions for the new AgentKit composition, but the simultaneously added Foundry… View resolved comment
High severity internal/​publisher/​pr.go — Including SessionUID in the existing orka.publisher.pr-intent.v1 key changes the canonical key… View resolved comment
Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/agentruntimepolicy/policy.go:66

  • Returning no policy for harness v1 leaves generated runtimeRef Tasks unchanged, but the existing harness-v1 admission path requires spec.agentRuntime.allowedTools to be an explicit empty list and allowBash=false (internal/controller/harness_v1_brokered.go:203-218). Consequently, producers newly using this shared helper—such as create_agent_task, delegate_tasks, and repository scan Tasks—create harness-v1 Tasks that are permanently rejected. Materialize the required v1 deny-all override (or reject the request before creating the Task) instead of treating v1 like a built-in runtime.

Comment thread internal/store/kube/session_cleanup.go Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0fa90a235

ℹ️ 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".

Comment thread internal/controller/acp_dispatcher.go
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Session runtime cleanup can outlive its fixed mutation lease, permitting overlapping controller mutations.

Review tier: Balanced
Findings: 1 High severity

Pre-existing issues (1)
Severity Finding
High severity internal/​store/​kube/​session_cleanup.go — ReclaimSession invokes this callback while holding the controller-epoch mutation interlock, but… View comment
Issues resolved since last review (4)
Severity Finding
Medium severity workers/​acp/​images/​foundry/​Dockerfile — The new Foundry composition is not covered by either the built-in Dockerfile table or the new… View resolved comment
Medium severity cmd/​orka-harness-v2-e2e-fixture/​main.go — This fixture reuses the same supervisor boot ID after a container restart. Because harness-v2… View resolved comment
Low severity workers/​acp/​images/​README.md — The image overview now documents the AgentKit composition as the only opt-in composition, but this… View resolved comment
Medium severity workers/​acp/​images/​dockerfile_test.go — This adds policy assertions for the new AgentKit composition, but the simultaneously added Foundry… View resolved comment

Retry only definitely unsent cancellations within their sealed deadlines.
Keep Session runtime I/O outside the shared epoch interlock and preserve
exact cleanup intent and original authority before reclamation.

Retain each original Pod before committing admission evidence, and recover
late preparation writes and interrupted legacy retention safely. Document
receipt retention and use the Kubernetes minimum cleanup token duration.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Write-intent UI serialization is incorrect, and archived cleanup receipts currently grow without a reclamation policy.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2 Medium severity

New issues introduced by this change (2)
Severity Finding
Medium severity internal/​store/​sqlite/​session_turn_cleanup.go — This archives a full receipt (including the terminal projection payload) for every turn of every…
Medium severity ui/​src/​components/​tasks/​task-create-form.tsx — When a user selects write but leaves the other workspace fields empty, the earlier serialization…
Issues resolved since last review (1)
Severity Finding
High severity internal/​store/​kube/​session_cleanup.go — ReclaimSession invokes this callback while holding the controller-epoch mutation interlock, but… View resolved comment

Comment thread internal/store/sqlite/session_turn_cleanup.go
Comment thread ui/src/components/tasks/task-create-form.tsx
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The cross-cutting runtime and cleanup changes require human review, and unresolved metadata validation and unbounded SQLite retention concerns remain.

Review tier: Balanced
Findings: 2 Medium severity

New issues introduced by this change (2)
Severity Finding
Medium severity internal/​store/​sqlite/​sqlite.go — This table is append-only and stores a full terminal-projection BLOB for every finalized turn after…
Medium severity workers/​acp/​supervisor/​mcp_proxy.go — encoding/json accepts _meta: null for this map, so the new field does not enforce the “metadata…
Issues resolved since last review (2)
Severity Finding
Medium severity ui/​src/​components/​tasks/​task-create-form.tsx — When a user selects write but leaves the other workspace fields empty, the earlier serialization… View resolved comment
Medium severity internal/​store/​sqlite/​session_turn_cleanup.go — This archives a full receipt (including the terminal projection payload) for every turn of every… View resolved comment

Comment thread internal/store/sqlite/sqlite.go
Comment thread workers/acp/supervisor/mcp_proxy.go Outdated
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

It spans security-sensitive dispatch, authorization, durable recovery, and runtime retirement paths that warrant final human review.

Review tier: Balanced
Findings: None

Issues resolved since last review (2)
Severity Finding
Medium severity workers/​acp/​supervisor/​mcp_proxy.go — encoding/json accepts _meta: null for this map, so the new field does not enforce the “metadata… View resolved comment
Medium severity internal/​store/​sqlite/​sqlite.go — This table is append-only and stores a full terminal-projection BLOB for every finalized turn after… View resolved comment

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The 270-file security-sensitive dispatch and cleanup change, together with still-open companion runtimes, requires final human integration review.

Review tier: Balanced
Findings: None

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a440579d5

ℹ️ 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".

Comment thread workers/acp/supervisor/server.go
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The broad security-sensitive dispatch, recovery, cleanup, and concurrency changes warrant final human validation.

Review tier: Balanced
Findings: None

@sozercan

Copy link
Copy Markdown
Member Author

Addressed the idle-peer cleanup finding in 7cdbc981.

The first pool-poison transition now schedules eligible peers through the existing drain selector and starts cleanup after releasing the lock. Later cleanup failures leave residents poisoned and unscheduled for authenticated retries. Added regressions for task DELETE and automatic cleanup, including peer attempt counts and closed admission.

Validation passed: make lint-fix, make test, and 49 Linux/amd64 tests and subtests with -race.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7cdbc98196

ℹ️ 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".

Comment thread internal/controller/agent_runtime_controller.go Outdated
Comment thread internal/controller/agent_runtime_recovery_topology.go
Comment thread internal/controller/agent_runtime_recovery_witness.go Outdated
Resolve Service target ports per Pod before accepting EndpointSlice pins. Reject supervisor service account token Secret references during recovery validation and retain secondary Pod IP support. Update conformance fixtures to advertise consistent target ports.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@sozercan

Copy link
Copy Markdown
Member Author

Fixed the three findings in 96eba10b.

  • EndpointSlice pins must match the Service TCP targetPort. Named targets resolve against each Pod, including restartable init sidecars. Port mapping thread.
  • Recovery resolves supervisor Secret references through the uncached reader and rejects service-account-token Secrets in mounted volumes, projections, and explicit environment references. Missing or failed reads fail closed. Broker-only identity remains supported. These checks also apply before accepting container retirement and before retained-boot mutations. Secret validation thread.
  • Recovery can select a secondary Pod IP while retaining the single-address and ambiguous-backend guards. Secondary address thread.

Validation passed: make lint-fix, make test, and focused regressions for target ports, Secret types and read failures, and secondary IPv4/IPv6 addresses. Existing conformance fixtures now declare the actual ports used by their HTTP servers.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The new UI and CLI harness-v1 paths omit mandatory allowBash=false, causing their submitted Tasks to be rejected.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Balanced
Findings: 2 High severity

New issues introduced by this change (2)
Severity Finding
High severity cmd/​cli/​task.go — The v1 policy returned below is serialized with only allowedTools, but the controller requires…
High severity ui/​src/​components/​tasks/​task-create-form.tsx — This newly exposed harness-v1 path creates an invalid Task: observed external v1 bindings require…

Comment thread cmd/cli/task.go
Comment thread ui/src/components/tasks/task-create-form.tsx
@sozercan

Copy link
Copy Markdown
Member Author

The existing payload is correct for the external runtimeRef branches identified in these two comments: CLI thread and UI thread.

At 96eba10b, resolveHarnessV1ToolGovernance, an external runtimeRef enters brokered execution when supported. External observed-only registrations are rejected before the cited observed-policy validator is called. The brokered validator requires an explicit allowedTools list and rejects any non-nil AllowBash, including false. Adding allowBash: false here would reject valid external brokered Tasks.

Verification passed on the current head: the CLI policy serialization test, controller target-mode and brokered binding tests, and the UI harness-v1 runtimeRef submission test. The focused Go run passed 12 tests/subtests; the selected UI test passed. No code change is needed for these findings.

Exercise Task AlreadyExists through the fake client Create boundary using the actual generated Task name. Keep the ScanRun and status assertions independent of second-boundary timing.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@sozercan

Copy link
Copy Markdown
Member Author

Fixed the Go CI failure in 5e13504d.

TestCreateScanRunIsIdempotentWhenTaskAlreadyExists predicted a Task name separately from the controller. Both names include the current Unix second, so crossing a second boundary made the test query a different ScanRun ID. A forced 1.1-second gap reproduced the exact GetScanRun() error = not found failure from the failed job.

The fixture now seeds the existing Task at the fake client's Create boundary using the actual requested name, verifies that the controller receives AlreadyExists, and preserves the ScanRun and status assertions. The same forced timing gap passes after the fix. make lint-fix and the full make test suite passed. This commit changes only the test.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Forks switching to an external harness-v1 Agent retain incompatible or stale runtime policy.

Review tier: Balanced
Findings: None

Issues resolved since last review (2)
Severity Finding
High severity ui/​src/​components/​tasks/​task-create-form.tsx — This newly exposed harness-v1 path creates an invalid Task: observed external v1 bindings require… View resolved comment
High severity cmd/​cli/​task.go — The v1 policy returned below is serialized with only allowedTools, but the controller requires… View resolved comment
Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

internal/api/fork_handlers.go:155

  • When a fork overrides agentRef to an external harness-v1 Agent, this helper returns a nil policy without modifying the inherited spec.agentRuntime. A built-in source therefore carries allowBash/disallowedTools into the brokered v1 path (or has no explicit allowedTools at all), which resolveHarnessV1BrokeredTools rejects; a v2 source can also leak its stale allowed-tool list into the new runtime. Materialize the harness-v1 task policy for the selected Agent, or clear incompatible inherited fields whenever the Agent changes.

Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
@sozercan

Copy link
Copy Markdown
Member Author

Fixed the Live Copilot Proxy E2E failure in 7d3f97a.

OpenCode 1.18.9 starts optional title inference in the background and can return ACP end_turn while that request is still active. Both attempts in the failed job then failed the supervisor's inference drain check.

A native 1.18.9 fixture reproduced this with a held title response: the foreground answer completed while the title request remained active. With agent.title.disable=true, the same fixture produced the expected answer with one foreground request, zero title requests, and no request left in flight. Orka now projects that setting into its OpenCode profile. The inference accounting, settlement checks, timeouts, and live assertions are unchanged.

Validation passed: focused OpenCode/proxy/drain tests, make lint-fix, and the full make test suite. The native fixture ran on Darwin arm64; the fresh live CI run will verify the Linux path.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The 275-file security- and recovery-sensitive change requires final human validation despite extensive automated coverage.

Review tier: Balanced
Findings: None

@sozercan

Copy link
Copy Markdown
Member Author

The OpenCode fix in 7d3f97a passed both fresh Live Copilot Proxy E2E jobs: pull-request run and push run.

The rebuilt Linux/amd64 image also passed a live cluster run against the pinned repository workspace. The Task completed on attempt 1 with Succeeded / ReadValidated, and the result API returned the expected repository sentinel. The test requested shell and mutation tools under read intent, using the existing live-test assertions. The Task, Agent, RuntimePool, and runtime Pod were then removed through normal cleanup with no manual finalizer removal.

The first cluster attempt timed out before scheduling because all nodes lacked free CPU requests. After temporarily pausing an inactive validation controller, the fresh Task passed in 38 seconds. That controller has been restored and is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants