Uh oh!
There was an error while loading. Please reload this page.
feat(aca): resolve Copilot credential automatically via gh auth token - #338
Merged
Conversation
Add the solution design for an experimental `aca` provider that relocates an agent's whole runtime (agentic loop + tool execution) into an Azure Container Apps (ACA) dynamic session, Hyper-V isolated off the host. Covers the architecture, design decisions (DD1-DD7), proposed ProviderCapabilities, requirements, security considerations, risks, and open questions — including answers worked through with the reviewer for five of the six pre-MVP open questions and the Phase 0 transport spike (tracked separately, now resolved in #312). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 634ddaec-6378-44cd-8da4-448e81351243
Fold the Phase 0 transport spike's measured results (issue #312, closed) into the design: Branch S (single streaming request) is chosen over Branch P (submit+poll). A real ACA custom-container session pool measured a reproducible ~30-minute per-request cap on default (non- premium) ingress, confirmed via a disambiguation run at 2400s that cut off at the same ~1801s mark as the 1800s run (ruling out coincidence with the test's own duration). Streaming durability (>=10 min) and Last-Event-ID resume were both confirmed working cleanly; Branch P was also verified end-to-end with zero missing frames and remains available as a fallback mechanism, though not required for the MVP. Updates: - Decision Status table: DD3 and the streaming_events/interrupt capability values move from "Blocking unknown" to "Resolved". - DD3: replaced the "Phase 0 picks" framing with the actual measured results and the Branch S decision + caveat (turns exceeding ~30 min still hit the cap). - Capabilities table: streaming_events=True, interrupt=True (no longer conditional). - Data Flow: simplified to describe the single streaming request instead of a branch-dependent description. - Risks table: resolved the "sessions endpoint drops long/streamed requests" and "interrupt weaker than on-host" rows; added a new residual risk row for turns exceeding the measured cap. - Open Questions: moved the "Runner API branch" question to resolved with the full measured data, and added a new Pre-MVP question on whether to build reconnect-and-resume for v1 or defer it (spike showed resume is mechanically viable, but needs a bounded server-side event log for production, unlike the spike's unbounded in-memory one). - References: added the spike issue and spikes/aca-transport/ artifacts. - Header note: pointed at the closed spike issue. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 634ddaec-6378-44cd-8da4-448e81351243
Missed in the DD3 resolution pass — the Branch P fallback contract still said 'Phase 0 (DD3) determines which branch ships.' Branch S is chosen; Branch P stays documented as an optional, non-required fallback the runner could still expose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 634ddaec-6378-44cd-8da4-448e81351243
Add docs/aca-provider-example.yaml, a PREVIEW workflow illustrating what
a workflow YAML will look like once the aca provider ships. It visualizes:
- The structured runtime.provider: {name: aca, ...} extension (pool_endpoint,
api_version, inner_provider, identifier_scope, egress, lifecycle, auth).
- A coding-agent pattern (clone -> implement -> test -> loop back on
failure) that stays in the SAME ACA session across loop-back
re-executions via identifier_scope: agent (DD5).
- The per-agent sandbox: override block (working_dir, identifier_scope)
from the Open Questions answers.
- max_session_seconds kept under the Phase 0 spike's measured ~30-minute
per-request cap (issue #312).
- stdio MCP tools baked into the runner image (runner-image contract).
Deliberately placed under docs/, not examples/: the aca provider is not
implemented yet, so this file cannot pass `conductor validate` (confirmed
it fails only on the not-yet-recognized aca-specific fields, not on any
unrelated YAML/schema issue) and would break `make validate-examples`/CI
if added to examples/. Once the provider ships, an updated/verified
version becomes the "one runnable examples/ workflow" required by the
design's Goal #6, at that point living in examples/ instead.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 634ddaec-6378-44cd-8da4-448e81351243Per review feedback, rename two identifier_scope enum values for clarity: - run -> workflow: "run" was ambiguous against the `conductor run` CLI command and the internal `run_salt` implementation detail. `workflow` more directly names what's being shared (the whole workflow run) and matches existing terminology (workflow.input, workflow.dir). - step -> none: "step" didn't describe the behavior (no reuse at all, every execution including retries gets a fresh sandbox). `none` reads naturally as "no persistence scope" alongside workflow/agent/item. No functional change — same four semantics, clearer names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 634ddaec-6378-44cd-8da4-448e81351243
Relocate aca-provider.design.md and aca-provider-example.yaml from docs/ into a dedicated docs/projects/aca/ directory. Pure move, no content changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add the implementation plan for the Agent-in-Sandbox remote `aca` provider, generated from the solution design. Breaks the work into epics/tasks with target file paths, grounded in the conductor codebase. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Parse and validate runtime.provider: {name: aca, ...} and the per-agent
sandbox: block on AgentDef, mirroring existing ProviderSettings field-gating
guardrails used for copilot/claude/hermes.
- Add SandboxConfig model (identifier_scope override + container-relative
working_dir) and AgentDef.sandbox field; forbidden on non-provider-backed
step types.
- Fix review finding: default inner_provider->copilot, identifier_scope->agent,
and auth->azure_default when name=='aca' and left unset, applied via
object.__setattr__ in ProviderSettings._check_field_compatibility (model is
frozen=True).
- Mark E1 DONE in the aca-provider plan, including OQ#3 (working_dir) resolution.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>…ider Instantiate AcaRuntimeProvider through create_provider(), register its capabilities for resolution at validate time via _PROVIDER_CLASS_PATHS, and isolate azure-identity behind an 'aca' optional extra. Fixed two test reliability issues in tests/test_providers/test_aca.py found during review: - test_factory_raises_when_azure_identity_not_available and test_factory_error_includes_install_suggestion now explicitly patch conductor.providers.factory.AZURE_IDENTITY_AVAILABLE to False instead of relying on azure-identity being absent from the environment (which broke both tests when the 'aca' extra is installed). - test_factory_forwards_provider_settings_and_config now passes non-default mcp_servers and a non-default ToolOutputConfig through create_provider(), asserting they are forwarded verbatim to AcaRuntimeProvider.__init__. - Corrected a docstring that incorrectly assumed azure-identity was not installed in the test environment. Marks Epic E2 DONE in docs/projects/aca/aca-provider.plan.md, including the E3-T1 AcaRuntimeProvider skeleton pulled forward as a prerequisite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…scope-creep - Rework _acquire_wire_identifier/_release_wire_identifier in aca.py to track a set of reserved slot numbers per logical identifier (dict[str, set[int]]) instead of a plain in-flight count, so a release always frees the exact slot a call acquired and a subsequent caller reuses the smallest free slot rather than colliding with a still-active sibling under out-of-order completion. - Change _acquire_wire_identifier to return (wire_identifier, slot) and _release_wire_identifier to accept (logical_id, slot); update the execute() call site accordingly. - Revert AgentOutput.session_seconds field addition in base.py (out of E3 scope; belongs to E6). - Remove AcaResultData.session_seconds field and its docstring bullet from aca_protocol.py; the runner's raw result dict still reaches AgentOutput.raw_response untouched. - Remove session_seconds=result.session_seconds from _agent_output_from_result in aca.py. - Remove session_seconds fixture data and assertion from test_execute_relays_events_and_parses_result in test_aca.py. - Add test_out_of_order_release_does_not_collide_with_still_active_slot, a three-call regression test directly exercising the acquire/release slot logic. - Update docs/projects/aca/aca-provider.plan.md: mark E3 DONE, document both rounds of review fixes, update E3-T3/E3-T5/E3-T7 status notes and the OQ#1 resolution narrative, remove the stale 'AgentOutput gained session_seconds' note, and correct E6's prerequisite text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the FastAPI runner that wraps a real CopilotProvider behind the
POST /execute (streaming NDJSON) + GET /health contract consumed by
AcaRuntimeProvider:
- src/conductor/aca_runner/{__init__,server,__main__}.py: create_app()
factory, /health (readiness + conductor/runner version), /execute
(deserializes AcaExecuteRequest, constructs/reuses a CopilotProvider,
streams event frames via event_callback, terminates with a result
frame including session_seconds).
- Tools/MCP passthrough: full mcp_servers + per-agent tools forwarded to
the inner provider; a declared-but-absent stdio binary fails loudly
(400) before the stream opens, never silently dropped.
- OQ#6 credential stopgap: inner_provider_settings builds
ProviderSettings(name="copilot", ...) via the existing bearer_token
custom-routing path; the Phase 2 gateway seam is isolated to
_InnerProviderCache.get.
- OQ#5 dialog turns: AcaRuntimeProvider.execute_dialog_turn now raises a
clear ProviderError (disable-with-clear-error fallback) instead of the
generic base-class NotImplementedError; no runner dialog endpoint was
built.
- Tests: tests/test_aca_runner/test_server.py (mocked CopilotProvider —
streaming frames/terminal result, health version, missing-binary
error, tools/mcp forwarding, provider reuse/reconstruction) and two
new tests in tests/test_providers/test_aca.py for the dialog-turn
fallback.
- docs/projects/aca/aca-provider.plan.md: E4 marked DONE, OQ#4/#5/#6
answered.
Not built (no E4 task/acceptance criterion assigns them; flagged as
follow-up gaps in the server module docstring and plan): a /interrupt
endpoint and a runner-side max_session_seconds wall-clock guard.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>- Validate agent payload before opening StreamingResponse so malformed requests (e.g. bad context_tier) return a clean 400 instead of a broken mid-stream frame. - Make _InnerProviderCache concurrency-safe with an asyncio.Lock around get()/close() to prevent double-close/orphaned-provider races. - Forward per-agent retry and context_tier settings end-to-end (AcaAgentPayload -> AcaRuntimeProvider._build_request -> runner _build_agent), which were previously silently dropped. - Add tests for terminal error frames and cache concurrency safety. - Update aca-provider.plan.md with review-fix notes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- docker/aca-runner/Dockerfile: repin the default CONDUCTOR_VERSION from v0.1.25 (predates epic E4, causes ModuleNotFoundError on `conductor.aca_runner`) to the immutable commit that introduces the runner module. Verified via a git+file install (same command, local transport): the old default fails importing conductor.aca_runner, the new default installs cleanly, starts, and answers GET /health. - docker/aca-runner/.dockerignore: fix comment — Conductor is installed from GitHub (git+https), not PyPI. - scripts/aca/provision-pool.sh: - forward --build-arg TARGET_PORT to `az acr build` so the pushed image's listening port always matches the pool's --target-port. - stop passing both --cooldown-period and --max-alive-period (mutually exclusive per lifecycle type in the sessionpool create API); build the matching flag from lifecycle_type instead. - pre-create a dedicated user-assigned managed identity and grant it acrpull *before* creating the session pool, then pass its resource ID as --registry-identity. --registry-identity system referred to the pool's own identity while acrpull was being granted to the environment's identity instead, and after pool creation rather than before, which Azure requires. - remove `|| true` around the acrpull role assignment so authorization failures surface instead of being silently swallowed. - docs/projects/aca/aca-provider.plan.md: record the fixes and the verification method (Docker itself isn't available in this environment). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
docker/aca-runner/Dockerfile: - Bake in a pinned git-mcp-server npm binary (@cyanheads/git-mcp-server@2.15.1, exposes bin `git-mcp-server`) so aca-provider-example.yaml's mcp_servers.git.command: git-mcp-server works out of the box even with pool egress disabled — previously undocumented/uninstalled, so that example would fail at runtime. - Clarify the CONDUCTOR_VERSION pin comment: the commit is only a usable default once it is reachable from a real microsoft/conductor ref (this branch, pushed as part of this change), not merely present in a local clone. scripts/aca/provision-pool.sh: - Registry role grant now uses --assignee-object-id + --assignee-principal-type ServicePrincipal instead of --assignee <principalId>, avoiding an Entra ID replication race on the just-created identity (the graph-lookup path --assignee takes isn't guaranteed to resolve the principal immediately). - Detect the ACR's roleAssignmentMode and grant 'Container Registry Repository Reader' instead of 'AcrPull' on ABAC-enabled (reused) registries, where AcrPull is not honored. - IMAGE_TAG now defaults to a unique UTC-timestamp tag instead of the mutable 'latest', so reprovisioning can't silently keep serving a stale cached image; still overridable. - Add a preflight (az upgrade; az extension add --name containerapp --upgrade) before any session-pool command, mirroring the official minimum-tooling guidance so an out-of-date CLI/extension can't silently lack the lifecycle flags this script depends on. tests/test_integration/test_aca_provision_pool.py (new), _mock_az.py (new): - Automated coverage for provision-pool.sh's generated Azure CLI arguments, running the real script against a scripted mock 'az' that records every invocation. Covers TARGET_PORT propagation, lifecycle flag exclusivity, the assignee-object-id/principal-type role-assignment shape, ABAC-vs-legacy role selection, unique image tagging, and the CLI preflight. Each new test was confirmed to fail against the pre-fix script. docs/projects/aca/aca-provider.plan.md: record the above fixes and verification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
scripts/aca/provision-pool.sh: - Move EGRESS/LIFECYCLE validation above the az upgrade/extension-add preflight so an invalid value is rejected before any Azure CLI call is made, instead of after burning two az calls. - IMAGE_TAG's default now appends this script's PID and bash's $RANDOM to the UTC timestamp, so two runs landing in the same wall-clock second (e.g. concurrent CI jobs) get distinct tags. The prior timestamp-only default was reproducibly shown to collide across two sequential test runs within the same second. tests/test_integration/test_aca_provision_pool.py: - test_invalid_lifecycle_rejected / new test_invalid_egress_rejected now assert the mock az call log stays empty on invalid input (regression tests for the reordering above). - test_default_tag_is_unique_across_runs now asserts the two tags actually differ, rather than only checking neither is "latest". - Strengthened the preflight test to assert --allow-preview true and --yes on the extension-add call, and --yes on the upgrade call. - Added coverage for the previously-untested empty/unrecognized ACR roleAssignmentMode fallback to AcrPull. - Added a new TestSessionExecutorRoleAssignment class asserting the Session Executor grant's --assignee shape (distinct from the registry grant's --assignee-object-id/--assignee-principal-type shape). docs/projects/aca/aca-provider.plan.md: - Corrected the E5-T2/E5-T3 status text and Acceptance Criteria notes, which previously overstated that both role grants used the --assignee-object-id shape (only the registry grant does) and that every new test failed against the pre-fix script (only 3 do). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Session Executor role grant now determines the principal type without
Microsoft Graph when possible: the default (signed-in user) branch sets
ASSIGNEE_PRINCIPAL_TYPE=User and passes --assignee-principal-type,
since --assignee-object-id alone does not stop Azure CLI (2.88.0) from
separately querying Graph to infer the principal type. An explicit
ASSIGNEE override can set ASSIGNEE_PRINCIPAL_TYPE explicitly to get the
same Graph-free behavior; otherwise the fallback to the Graph lookup is
now documented instead of incorrectly claimed away.
- Updated/added tests: test_defaults_assignee_to_signed_in_user now
asserts --assignee-principal-type User; added
test_explicit_assignee_principal_type_is_honored for the override path.
- Nonce format test now asserts an exact ^[0-9a-f]{32}$ match instead of
just a length floor, catching a regression to non-hex/uppercase output.
- Updated docs/projects/aca/aca-provider.plan.md to correct the
inaccurate fourth-review-pass claim and describe the actual behavior.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>Add optional session_seconds field to AgentOutput and AcaResultData, populate it from the aca runner's terminal result frame, and record a token-free "<agent> (sandbox)" usage row (cost None) mirroring the existing "(validator)" row pattern. Wired at the main-loop, parallel-group, and for-each usage-recording sites in the engine. Resolves OQ#2 in the aca-provider plan (usage-surfacing mechanism was previously undecided). Non-aca providers are unaffected since session_seconds stays None. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
docs/providers/aca.md (interrupt capability row, Known Gaps bullet, and the Stopping the workflow troubleshooting entry) and AGENTS.md's aca.py parity notes claimed cleanup after a stopped/interrupted aca-backed agent may wait through the httpx client's 30-second connect/write/pool defaults. In fact src/conductor/providers/aca.py's _send_interrupt and _stop_session both pass an explicit timeout=10.0, overriding those defaults. Corrected all four passages to describe the actual 10-second per-call cleanup timeout, and recorded a third review-fix round in the plan doc. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…curacy - pyproject.toml/uv.lock: add azure-core[aio] (pulls in aiohttp) to the aca extra. azure.identity.aio.DefaultAzureCredential requires an async HTTP transport that azure-identity alone does not provide; constructing it raised ImportError: aiohttp package is not installed. Added TestAcaExtraCleanInstall (tests/test_providers/test_aca.py) exercising the real, unmocked credential construction — every other test in that file patches AZURE_IDENTITY_AVAILABLE and swaps in a fake credential, so this regression had no coverage at all. - src/conductor/config/schema.py: require pool_endpoint to be https:// — AAD bearer tokens and forwarded provider credentials (inner_provider_settings) are sent to it on every request. Added tests to tests/test_config/test_provider_settings_aca.py. - src/conductor/providers/aca.py: correct two capability declarations to match observed behavior instead of aspiration: - workflow_tools_passthrough: True -> False. The in-container CopilotProvider the runner wraps never applies the tools: allowlist to the SDK session (confirmed in copilot.py — the parameter is recorded for call-history/logging only). Same carve-out claude_agent_sdk.py and hermes.py already declare. - working_dir: True -> False. That capability field means "applies the generic, host-resolved agent.working_dir/runtime.working_dir" — aca never reads that field, only the separate, container-relative sandbox.working_dir. Declaring True let conductor validate silently accept a meaningless combination. Added TestAcaRealCapabilitiesCrossCheck (tests/test_config/ test_validator_capabilities.py) against the real capability descriptor, plus _build_request-level coverage in tests/test_providers/test_aca.py. - docs/providers/aca.md, docs/providers/experimental.md, AGENTS.md: updated capability tables/parity notes to match; fixed the identifier_scope ASCII diagram's stale 'run | agent | item' comment to 'workflow | agent | item | none'; added a pool_endpoint HTTPS troubleshooting entry. - docs/projects/aca/aca-provider.plan.md: recorded this as the fourth review-fix round. Full suite: 4344 passed, 33 skipped, 1 pre-existing known-flaky failure in test_event_log.py (unrelated, documented in every prior review round). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…_endpoint - src/conductor/config/validator.py: the shared _check_agent_tools cross-check treated explicit tools: [] as an unconditionally valid "no tools" opt-out against any workflow_tools_passthrough=False provider. That's only true when the provider has nothing to forward regardless of the list (mcp_tools=False, e.g. claude_agent_sdk/hermes). aca declares mcp_tools=True alongside workflow_tools_passthrough=False, so tools: [] was silently accepted while the runner still attached every configured MCP server. Added a branch rejecting any explicit tools: (empty or not) against a provider with mcp_tools=True and no passthrough. Updated the pre-existing generic (copilot-mocked) tests that relied on the old blanket assumption to set mcp_tools=False explicitly, and added new tests for the mcp_tools=True combination (generic + against real AcaRuntimeProvider.CAPABILITIES). - src/conductor/config/schema.py: pool_endpoint validation only checked the "https://" string prefix, so "https://" (no hostname) and "https://host?x=1"/"#frag" (a pre-existing query/fragment that _build_url then appends /execute + identifier/api-version onto) both passed and produced malformed request URLs. Replaced with urllib.parse.urlparse-based validation requiring an https scheme, a non-empty hostname, and no query/fragment. Added tests covering all three new rejection cases. - docs/providers/aca.md, AGENTS.md: updated the tools/pool_endpoint documentation and added matching Troubleshooting entries to reflect the corrected validator behavior. - docs/projects/aca/aca-provider.plan.md: recorded a fifth review-fix round under epic E7. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolves 5 round-3 review findings against the prior E8 implementation (host forwarding of a GitHub token for Copilot capacity): - server.py: _InnerProviderCache._key_for now hashes the canonical JSON (after unwrapping SecretStr) with hashlib.sha256(...).hexdigest() instead of storing the plaintext canonical JSON in the long-lived self._key attribute. Cache correctness (distinct credentials produce distinct keys) is preserved without retaining plaintext credentials. - server.py: replaced a stale "OQ#6 Phase 1 stopgap" comment with an accurate note describing the seam E9 will use to wire github_token into the inner CopilotProvider's actual auth. - aca_protocol.py: narrowed the "SecretStr wrapping on every construction path" claim to "every validated construction path", noting model_construct()/model_copy(update=...) bypass field validators and are unused in this codebase. - plan.md: corrected the E8 review-fix narrative to match the real hashing behavior and the narrowed validated-construction-path claim, and documented the aca-provider.design.md DD4 edits as in-scope for this epic's review fixes. - plan.md: added server.py and design.md to E8-T1's Files column, and test_server.py to E8-T3's Files column, so the task table matches the actual diff. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Corrects a stale comment in _InnerProviderCache.get() (server.py) that described the forwarded github_token flowing to CopilotClient(github_token=...) at construction time. It actually flows in memory via CopilotProvider._apply_github_token on each create_session/resume_session call, matching the SDK's gitHubToken RPC payload rather than the rejected constructor-based mechanism (which would leak via COPILOT_SDK_AUTH_TOKEN env var). Marks E9 (and tasks E9-T1/T2/T3) DONE in aca-provider.plan.md with both acceptance criteria checked off, reflecting completed implementation, tests, and this review fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rewrite docs/providers/aca.md's Quick Start, Inner Copilot Authentication, and Security sections so the default, recommended path is a fine-grained *Copilot Requests* PAT exported as COPILOT_GITHUB_TOKEN (no COPILOT_PROVIDER_* required), with BYOK custom routing documented as the fallback. States the trusted-use posture explicitly (the credential enters the sandbox; keep it narrowly scoped with a short expiry; off-sandbox isolation is future work). Flip DD4's status to Accepted in the design's Decision Status table now that E8/E9 have shipped the implementation. Update examples/aca-coding-agent.yaml's prerequisite comments and Run snippet to lead with COPILOT_GITHUB_TOKEN, with BYOK noted as the fallback; conductor validate still passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address code-review feedback on E10 (default Quick Start to Copilot capacity): several docs still implied a credential is unconditionally forwarded to the sandbox, contradicting the fact that BYOK's endpoint credential is optional. - docs/providers/aca.md: soften "a credential is always forwarded" phrasing at the Quick Start step-3 intro, the Inner Copilot Authentication resolution intro, and the in-memory-delivery sentence to describe the host resolving/forwarding "either a GitHub token or BYOK routing settings". - docs/projects/aca/aca-provider.design.md: same wording fix in the Decision Status & Review Ask section (DD4 shipped-status paragraph and the "What is the credential posture?" callout); note BYOK endpoint credentials are optional. - examples/aca-coding-agent.yaml: reword prerequisite #4 so the host "must forward either a GitHub token or BYOK routing settings," not an unconditional credential; re-wrap affected comment lines. - docs/projects/aca/aca-provider.plan.md: remove a duplicated, stale Acceptance Criteria block left over from an earlier edit pass on E10, and add a completion note describing this review-feedback pass. Verified `conductor validate examples/aca-coding-agent.yaml` still passes and reflects the Copilot-token default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Bump docker/aca-runner/Dockerfile's CONDUCTOR_VERSION default off the E4-era commit (d6db5c8) onto the current branch tip, which includes E9's _InnerProviderCache fix (popping github_token before constructing ProviderSettings). The E4-pinned runner rejected the E8/E10 default credential body ({"github_token": ...}) with a Pydantic extra="forbid" ValidationError, breaking the documented Quick Start's happy path. Rewrote the pin comment to state the actual requirement instead of a stale "verified against d6db5c8" claim, and to flag that whoever next bumps the pin must push the referenced commit to origin first. - Added tests/test_aca_runner/test_server.py:: TestHostRunnerCredentialContract, a cross-component contract test that builds AcaRuntimeProvider._resolve_inner_provider_settings()'s actual default output and feeds it directly into the runner's _InnerProviderCache.get(), asserting it's accepted. Verified this test fails against the E4 runner's cache logic and passes against current. - Aligned the SandboxConfig.working_dir / AgentDef.sandbox docstring examples in schema.py and the /execute wire-contract sample in aca.md to /workspace (matching the E7-round fix already applied to the runnable example and workflow-syntax.md) since /workspace/repo doesn't exist at session start. - Documented this fix round as epic E11 in aca-provider.plan.md, including two items explicitly left for the maintainer: reconciling this branch with origin/docs/284-aca-provider-design (which has diverged with a different E8+ design) before the new pin is externally resolvable, and a real docker build/run verification (blocked in this sandbox by the lack of a Docker daemon). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Switch the aca-coding-agent example workflow's runtime default_model from gpt-4.1 to gpt-5-mini. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Running an `aca` workflow required hand-creating a fine-grained "Copilot Requests" PAT and exporting COPILOT_GITHUB_TOKEN, even though the operator is almost always already signed in to GitHub locally. The sandbox's inner Copilot session can't do interactive OAuth, so the host must forward a token — but nothing tried the credential the operator already had. Add `gh auth token` as the final step of the DD4 precedence chain: COPILOT_PROVIDER_BASE_URL (BYOK) -> COPILOT_GITHUB_TOKEN / GH_TOKEN / GITHUB_TOKEN -> gh auth token -> ProviderError. This mirrors the Copilot CLI's own documented chain, so an explicit token still overrides the ambient `gh` identity, and a scoped PAT remains the recommendation for CI and service accounts. Every `gh` failure mode (absent, not signed in, wedged keyring, empty output) means "no token" and falls through to the existing actionable error rather than raising. Reading the Copilot editor plugins' ~/.config/github-copilot/auth.db is deliberately NOT implemented: that store belongs to the Copilot Language Server rather than the CLI/SDK the runner drives, has changed format twice (hosts.json -> apps.json -> auth.db), and has already shipped encryption-at-rest once before an incident-driven rollback — so a plaintext read is a temporary accident, not a contract. Also fixes three defects found while running examples/aca-coding-agent.yaml end-to-end against a real ACA pool: - provision-pool.sh failed on EVERY run with default settings. Its default IMAGE_TAG used `date -u +%Y%m%dT%H%M%SZ`, but the ACA session-pool API lowercases image references while OCI tags are case-sensitive, so the image pushed as ...T1816Z-... and pool creation then failed minutes later looking for ...t1816z-... (ImageManifestNotFound/MANIFEST_UNKNOWN). The default tag is now all-lowercase and an uppercase override is rejected before any az call. - A non-2xx response whose body carried no recognizable ACA `message` collapsed to the bare placeholder "aca runner reported an error", discarding the body and implying the runner had been reached when it may not have been. The raw body is now echoed (truncated) when parsing yields nothing better — this immediately surfaced a real "Error happened when allocating pod for identifier ... in pool ..." during verification. - examples/aca-coding-agent.yaml pinned model: gpt-4.1, which is not available on all Copilot accounts and failed inside the sandbox at session.create. It now inherits default_model (gpt-5-mini). Tests: TestAcaCredentialPrecedence._clear_credential_env now also stubs the `gh` subprocess — without it, every "no credential configured" assertion shells out and picks up the developer's real token (verified: the test fails without the stub). Verified end-to-end against a live ACA pool with ALL credential env vars unset — the gh fallback supplied the credential and the workflow cloned a repo, wrote CONTRIBUTING.md, and completed in 73s for $0.0031. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
8 tasks
7 tasks
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Running an
acaworkflow required hand-creating a fine-grained Copilot Requests PAT and exportingCOPILOT_GITHUB_TOKEN— even though the operator is almost always already signed in to GitHub locally. This addsgh auth tokenas the final step of the DD4 credential chain, so a workflow runs with zero ACA-specific credential setup.Found and fixed while running
examples/aca-coding-agent.yamlend-to-end against a real ACA dynamic-sessions pool.New precedence (mirrors the Copilot CLI's own documented chain):
An explicit token still overrides the ambient
ghidentity, and a scoped PAT remains the recommendation for CI/service accounts. Everyghfailure mode (absent, not signed in, wedged keyring, empty output) means "no token" and falls through to the existing actionable error rather than raising.Why not read
~/.config/github-copilot/auth.db?It's the obvious shortcut and it does work today — but it's a bad engineering bet, so it is deliberately not implemented:
@github/copilotbinary has zero references to it.hosts.json→apps.json→auth.db), breaking downstream consumers both times.gh auth tokenis the documented, supported equivalent (copilot login --helplists "OAuth tokens from the GitHub CLI (gh) app"), and needs no special OAuth scope — Copilot entitlement is per-user seat.Three defects fixed
provision-pool.shfailed on every run with default settings. Its defaultIMAGE_TAGuseddate -u +%Y%m%dT%H%M%SZ, but the ACA session-pool API lowercases image references while OCI tags are case-sensitive — so the image pushed as...T1816Z-...and pool creation failed minutes later looking for...t1816z-...(ImageManifestNotFound/MANIFEST_UNKNOWN). Default tag is now all-lowercase, and an uppercase override is rejected before anyazcall rather than after a multi-minute image build.Error bodies were discarded. A non-2xx whose body carried no recognizable ACA
messagecollapsed to the bare placeholderaca runner reported an error— actively misleading, since such a response may mean the runner was never reached. The raw body is now echoed (truncated). This immediately paid off during verification, surfacing the real cause:The example pinned an unavailable model.
implementhard-codedmodel: gpt-4.1, which isn't available on all Copilot accounts and failed in-sandbox atsession.create. It now inheritsdefault_model(gpt-5-mini). Verified working:gpt-5-mini,claude-sonnet-4.5; not available:gpt-4.1,gpt-4o.Test note worth reviewing
TestAcaCredentialPrecedence._clear_credential_envnow also stubs theghsubprocess. Without it, every "no credential configured" assertion shells out and picks up the developer's real token — passing on a machine withoutgh, failing on one with it. I verified this is load-bearing: removing the stub makestest_raises_provider_error_when_neither_credential_configuredfail withDID NOT RAISE.Verification
4408 passed, 33 skipped(full suite)make check(ruff + ty) exit 0ghfallback supplied the credential; the agent cloned the repo, wroteCONTRIBUTING.md, and completed in 73.14s for $0.0031:Docs
docs/providers/aca.mdstep 3 now leads with "nothing to do if you're signed in withgh", keeping the PAT as the documented escape hatch and preserving the trusted-use security posture (aghtoken is broader than a Copilot-Requests PAT, and it does enter the sandbox). Added troubleshooting for the lowercase-tag failure, unavailable models, and 429s, plus two real setup blockers hit during the functional test:az login --tenant(plainaz logincan land on a disabled guest tenant,AADSTS500571) anduv tool install azure-cli --with pip(elseaz extension addfails with "No module named pip"). Also updatedAGENTS.md,CHANGELOG.md, and DD4 in the design doc.Related issues
Closes#284
Closes#336
ClosesIdea: run agents inside Azure Container Apps sandboxes via a remote
acaprovider (Agent-in-Sandbox) #284 (acaprovider epic) — this PR merges the full branch (29commits, E1–E11) into
main, delivering the provider end-to-end: hosttransport, in-package runner, runner image, provisioning example, docs, and
the credential resolution that completes DD4.
Closesaca: make the Phase 2 credential gateway the default, configurable via workflow YAML (retire the plaintext stopgap as a silent default) #336 (make the credential gateway the default) — closed as
superseded, not implemented. Two reasons, recorded in
a comment on the issue:
AcaCredentialGateway("epic E8") as already shipped, but no such classexists on this branch or on
main. It also cites DD4 asProposed; DD4has since been settled as Accepted — "forward one narrowly-scoped
credential; no modes" — which is the design this PR implements.
real but is a known, documented carve-out of the trusted-use posture,
not a regression: see
docs/providers/aca.md#security. Keeping thecredential entirely off the sandbox (a host-side broker/relay) remains
tracked there as future work, and is worth a fresh issue written against
the design as it actually shipped.
Note for reviewers: this PR makes the default credential a
ghOAuthtoken, which is broader than a Copilot Requests-only PAT. That trade is
called out explicitly in the docs, which continue to recommend a scoped PAT
wherever blast radius matters.