WIP: fix: add ACPCommand/ACPArgs fields and protocolVersion to ACP init

[tesdal]

Summary

Two independent fixes for OpenCode ACP support in Gas City:

1. Include protocolVersion in ACP initialize handshake

The ACP spec requires protocolVersion in InitializeParams. Without it, strict servers (e.g. OpenCode) reject the handshake. Claude Code was lenient and masked the non-compliance.

2. Add ACPCommand/ACPArgs for transport-specific provider commands

Providers like OpenCode need different commands for ACP vs tmux transport (opencode acp for ACP, bare opencode for tmux). The existing Args field applies to both transports with no override. Adds ACPCommand and ACPArgs fields that fall back field-by-field to Command/Args.

Known limitation

Resume/reconciliation paths (session_runtime.go, cmd_session.go, worker_handle.go) pass transport="" to BuildProviderLaunchCommand. These paths use the stored command from session creation, which already contains the resolved ACP command — so they work correctly in practice. Threading transport through these paths would require adding a transport field to session.Info, which is a larger change.

Closes gastownhall#1008

[Copilot]

Pull request overview

This PR extends provider configuration to support ACP-specific launch overrides (acp_command, acp_args) and updates the ACP JSON-RPC initialize handshake to include an explicit protocolVersion, with corresponding resolver, docs, and template-launch updates.

Changes:

• Add ACPCommand / ACPArgs to provider specs and resolved provider, plus an ACPCommandString() helper.
• Merge/copy the new fields through provider resolution and packing, and document them in schema/reference docs.
• Include protocolVersion: 1 in ACP initialize request params.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
internal/worker/builtin/profiles.go	Adds ACP command/args fields to built-in provider specs and clones ACP args.
internal/runtime/acp/protocol.go	Adds protocolVersion to ACP initialize params and sets it to 1.
internal/config/provider.go	Adds new config fields and introduces ACPCommandString().
internal/config/resolve.go	Merges ACPCommand and ACPArgs through provider resolution and folding.
internal/config/pack.go	Deep-copies ACPArgs in provider spec packing.
cmd/gc/template_resolve.go	Uses ACP-specific command string when building the template command for ACP sessions.
internal/config/resolve_test.go	Extends merge field-sync test data to include the new fields.
internal/config/field_sync_test.go	Marks new ACP fields as intentionally not patchable.
docs/schema/city-schema.txt	Documents new acp_command / acp_args fields in schema.
docs/schema/city-schema.json	Documents new acp_command / acp_args fields in schema.
docs/reference/config.md	Documents new acp_command / acp_args provider fields.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

specToResolved only deep-copies ACPArgs when len(spec.ACPArgs) > 0, which collapses an explicitly configured empty slice (e.g., acp_args = []) back to nil. That breaks the documented semantics where nil means “fall back to Args” but an empty slice should mean “override to no args”. Copy ACPArgs when spec.ACPArgs != nil (even if empty) to preserve this distinction.

Suggested change

	if len(spec.ACPArgs) > 0 {
	if spec.ACPArgs != nil {

[tesdal]

Fixed — changed to spec.ACPArgs != nil in 58a7341.

[Copilot]

ACP command selection is based only on cfgAgent.Session == "acp". If the city-level session provider is "acp" (cfg.Session.Provider) and the agent leaves Session empty, this will incorrectly use CommandString() instead of ACPCommandString(), and the SupportsACP validation above will also be skipped. Derive an effective isACP value using both agent override and the city session provider (p.sessionProvider) and use it consistently.

[tesdal]

ACP transport is agent-level only (Agent.Session = "acp"). There is no city-level ACP override — cfg.Session is the SessionConfig struct (tmux backend config), not a transport selector. An agent that leaves session empty always uses tmux, regardless of city config. No change needed.

[Copilot]

ACPCommandString() is added, but it appears to be used only in cmd/gc/template_resolve.go. Other launch paths (e.g., internal/config/launch_command.go BuildProviderLaunchCommand) still hardcode resolved.CommandString(), so ACP sessions created via the API/worker may ignore acp_command/acp_args. Consider threading “is ACP transport” into those launch-command builders so ACP overrides are applied consistently.

[tesdal]

Fixed — BuildProviderLaunchCommand now takes a transport parameter and uses ACPCommandString() when transport == "acp". All callers updated in 58a7341.

[Copilot]

New ACPCommandString() behavior isn’t covered by tests yet (provider_test.go currently only asserts CommandString()). Add unit tests that cover: (1) ACPCommand set + ACPArgs nil fallback to Args, (2) ACPArgs explicitly empty overriding Args to none, and (3) ACPCommand empty falling back to Command.

[tesdal]

Fixed — added TestACPCommandString with 6 subtests (full override, fallback, partial command-only, partial args-only, empty args, fallback-matches-CommandString) and TestBuildProviderLaunchCommandUsesACPCommand with 2 subtests in 58a7341.

[Copilot]

ProtocolVersion is now part of the initialize params and is set in newInitializeRequest(), but protocol_test.go’s round-trip test doesn’t assert that protocolVersion is present/expected. Add an assertion to prevent regressions (and update any fake ACP server fixtures if they start validating initialize params).

[tesdal]

Fixed — added TestInitializeRequest_IncludesProtocolVersion that asserts both struct field (params.ProtocolVersion == 1) and raw JSON presence in 58a7341.

[Copilot]

Provider provenance tracking doesn’t appear to include the new acp_command/acp_args fields yet. buildProviderProvenance → recordScalarProvenance in internal/config/chain.go currently records title_model and print_args but not ACPCommand/ACPArgs, so gc config explain --provider ... (and any provenance-driven logic) won’t attribute these new fields correctly. Add both fields to recordScalarProvenance (and corresponding tests in provenance_test.go).

[tesdal]

Fixed — added acp_command and acp_args to recordScalarProvenance in 2d3ee8d.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

cmd/gc/worker_handle.go:336

• resolvedWorkerRuntimeWithConfig rebuilds the command using resolved.CommandString() and calls BuildProviderLaunchCommand with transport == "". For sessions whose transport is ACP (e.g., agent.Session == "acp"), this can overwrite the stored ACP runtime command during SessionByID resolution and end up resuming with the wrong base command/args. Derive the effective transport for the session (from the agent template in cfg or from session bead metadata) and use ACPCommandString() / transport == "acp" when appropriate.

	command := strings.TrimSpace(info.Command)
	if !shouldPreserveStoredRuntimeCommand(command, resolved.CommandString()) {
		launchCommand, err := config.BuildProviderLaunchCommand(cityPath, resolved, nil, "")
		command = resolved.CommandString()
		if err == nil {
			command = launchCommand.Command
		}
	}

cmd/gc/cmd_session.go:412

• resolvedSessionCommand always calls BuildProviderLaunchCommand with transport == "", even though gc session new can create ACP sessions when found.Session == "acp". That means acp_command/acp_args are ignored and the stored command may not match the actual transport. Thread the session transport into resolvedSessionCommand (or select ACPCommandString() when needed) and pass it through to BuildProviderLaunchCommand.

func resolvedSessionCommand(cityPath string, resolved *config.ResolvedProvider, optionOverrides map[string]string) (string, error) {
	if resolved == nil {
		return "", fmt.Errorf("resolved provider is nil")
	}
	launchCommand, err := config.BuildProviderLaunchCommand(cityPath, resolved, optionOverrides, "")
	if err != nil {
		return "", fmt.Errorf("resolving provider launch command: %w", err)
	}
	return launchCommand.Command, nil

internal/api/session_runtime.go:141

• resolvedSessionRuntimeCommand always compares against resolved.CommandString() and builds a launch command with transport == "". For ACP sessions, this can incorrectly decide not to preserve the stored runtime command and regenerate a tmux command, ignoring acp_command/acp_args, which can break resume. Plumb the session transport into this function (e.g., from resolveSessionTemplate or bead metadata) and use ACPCommandString() / pass transport through to BuildProviderLaunchCommand when transport is "acp".

func (s *Server) resolvedSessionRuntimeCommand(resolved *config.ResolvedProvider, storedCommand string) (string, error) {
	if command := strings.TrimSpace(storedCommand); shouldPreserveStoredRuntimeCommand(command, resolved.CommandString()) {
		return command, nil
	}
	launchCommand, err := config.BuildProviderLaunchCommand(s.state.CityPath(), resolved, nil, "")
	if err != nil {
		return "", fmt.Errorf("building provider launch command: %w", err)
	}
	return firstNonEmptyString(launchCommand.Command, resolved.CommandString(), resolved.Name), nil

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

deepCopyProviderSpec uses append([]string(nil), in.ACPArgs...) which collapses a non-nil empty slice (configured acp_args = []) back to nil. That breaks the documented semantics where nil means “fall back to Args” but an explicit empty slice means “override to no args”. Preserve the nil vs empty distinction by copying ACPArgs only when in.ACPArgs != nil using a make(len) + copy (or equivalent).

Suggested change

	out.ACPArgs = append([]string(nil), in.ACPArgs...)
	if in.ACPArgs != nil {
	out.ACPArgs = make([]string, len(in.ACPArgs))
	copy(out.ACPArgs, in.ACPArgs)
	}

[tesdal]

Fixed in 20f7edc. Changed to make+copy guarded by in.ACPArgs != nil.

[Copilot]

resolvedChainToSpec copies ACPArgs with append([]string(nil), r.ACPArgs...), which turns an explicitly configured empty-but-non-nil slice into nil. That loses the “empty slice overrides to no args” behavior for acp_args = []. Copy into a make([]string, len(r.ACPArgs)) buffer when r.ACPArgs != nil to preserve empty vs nil.

Suggested change

	out.ACPArgs = append([]string(nil), r.ACPArgs...)
	out.ACPArgs = make([]string, len(r.ACPArgs))
	copy(out.ACPArgs, r.ACPArgs)

[tesdal]

Fixed in 20f7edc. Changed to make+copy guarded by r.ACPArgs != nil.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

internal/api/session_runtime.go:141

• Resume/reconciliation logic preserves a stored runtime command only if it matches (or has a prefix of) resolved.CommandString(). For ACP sessions where ACPCommand differs from Command (the new supported use case), storedCommand will typically match resolved.ACPCommandString() instead, so this check will fail and the code will rebuild a tmux command (transport "") and overwrite the ACP command on resume. Preserve the stored command when it matches either CommandString() or ACPCommandString() (or fetch the persisted transport from session metadata and compare/build using that transport).

func (s *Server) resolvedSessionRuntimeCommand(resolved *config.ResolvedProvider, storedCommand string) (string, error) {
	if command := strings.TrimSpace(storedCommand); shouldPreserveStoredRuntimeCommand(command, resolved.CommandString()) {
		return command, nil
	}
	launchCommand, err := config.BuildProviderLaunchCommand(s.state.CityPath(), resolved, nil, "")
	if err != nil {
		return "", fmt.Errorf("building provider launch command: %w", err)
	}
	return firstNonEmptyString(launchCommand.Command, resolved.CommandString(), resolved.Name), nil

cmd/gc/worker_handle.go:335

• This resume path decides whether to keep info.Command by comparing it only against resolved.CommandString(). If the original session was ACP with ACPCommand != Command, info.Command will match resolved.ACPCommandString() and won’t be preserved, causing this code to rebuild a non-ACP launch command (transport "") and potentially break resume. Consider preserving when the stored command matches either CommandString() or ACPCommandString(), or thread/persist transport into session.Info so you can rebuild consistently.

	command := strings.TrimSpace(info.Command)
	if !shouldPreserveStoredRuntimeCommand(command, resolved.CommandString()) {
		launchCommand, err := config.BuildProviderLaunchCommand(cityPath, resolved, nil, "")
		command = resolved.CommandString()
		if err == nil {
			command = launchCommand.Command
		}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.