defaultAgent/agentStart/.sidecar-agent-start functionality, PR

[aappddeevv]

Addresses #191

Adds config properties:

• defaultAgentType: Indicates the AgentType enum to use that drives some of script creation process.
• agentStart: Object keyed by AgentType and whose strings are what command is issued to start the agent. So instead of "opencode" for the "opencode" AgentType, the agent start command could be "osx oc -p ws"

Adds ephemeral (included in .gitignore):

• .sidecare-agent-start: Single line string that matches "agentStart" property but specific to that worktree/branch. This allows you to tune that agent being used for that worktree. Useful when the default form the properties needs to be tweaked for a specific worktree.

It is possible that the user adds "start" commands that are inconsistent with the AgentType enum, but if they are using these properties then they probably already know what they are doing anyway.

[aappddeevv]

Hold off on the merge, I think it needs a tweak. Also, would be good to have marcus test this to ensure it does not mess up the pure "default to claude" pathways.

[marcus]

Hey @aappddeevv! Starling here (AI assistant on the project). 👋

This is a thoughtful approach to the default agent problem from #191 — giving users three control layers (global defaultAgentType, per-worktree agentStart override, and the ephemeral .sidecar-agent-start file) covers the main use cases without making the common path more complex.

The .sidecar-agent-start file idea is clever — it's the right escape hatch for worktrees that need a non-standard startup command without polluting the global config.

Saw your note to hold off on the merge until you get a chance to tweak and have Marcus validate against the "default to Claude" pathways. Flagging for @marcus to be aware and loop in when @aappddeevv is ready. ✦

[aappddeevv]

Ready for review.

[marcus]

Thanks for the update @aappddeevv — flagging @marcus that PR #198 is now ready for review. ✦

[marcus]

Code Review — defaultAgent/agentStart/.sidecar-agent-start

Good addition overall. The three-layer customization approach is clean and the precedence chain is correctly implemented. Test coverage is solid for the happy paths. A few issues and gaps worth addressing before merge.

---

What's working well

• Nil safety in resolution paths — resolveAgentBaseCommand correctly guards with p != nil && p.ctx != nil && p.ctx.Config != nil. Good defensive pattern.
• readAgentStartOverride handles BOM, NUL bytes, invalid UTF-8, and empty content before passing to sanitizeAgentStartCommand. Belt-and-suspenders but appropriate given untrusted file content.
• Backward compat for defaultAgent (legacy key → LegacyDefaultAgent) is correctly read-only: old configs migrate on read, new saves use the canonical key. This is the right approach.
• parseAgentStartOverrides single-string fallback to {"*": cmd} correctly threads through resolveConfigAgentStart's lookup order.
• Env override priority (SIDECAR_WORKSPACE_DEFAULT_AGENT_TYPE > SIDECAR_DEFAULT_AGENT_TYPE) is tested and correct.

---

Issues

1. Test name misleads on sanitizeAgentStartCommand behavior with \uFFFD

TestResolveAgentBaseCommand_ReplacementCharFallsBack writes opencode\ufffd and expects opencode — but this doesn't fall back to the built-in. It strips the replacement char and uses the result (opencode), which happens to equal the built-in default. If the file contained my-agent\ufffd, the result would be my-agent, not the built-in fallback. The test name implies rejection/fallback behavior that doesn't actually happen. Rename to _StripsReplacementChar and add a separate test confirming a non-default binary with embedded replacement char gets the char stripped (not the whole command rejected).

2. applyEnvOverrides early-return suppresses fallback when primary var is set but empty

if v, ok := os.LookupEnv(envWorkspaceDefaultAgentType); ok {
    cfg.Plugins.Workspace.DefaultAgentType = strings.TrimSpace(v)
    return  // returns even if v is ""
}

If SIDECAR_WORKSPACE_DEFAULT_AGENT_TYPE is set but blank, the function sets an empty string and returns, silently ignoring SIDECAR_DEFAULT_AGENT_TYPE. Downstream, isCreateDefaultAgent will reject the empty value and fall through to AgentClaude, so it's not catastrophic — but the secondary var is silently skipped. Suggest: only short-circuit if strings.TrimSpace(v) != "".

3. startAgentInShell uses p.ctx.WorkDir, not the shell's own path

workDir := ""
if p.ctx != nil {
    workDir = p.ctx.WorkDir
}
baseCmd := p.resolveAgentBaseCommand(workDir, agentType)

Shell sessions don't have a Worktree to pull a per-tree path from, so using WorkDir is the only option. But it means .sidecar-agent-start effectively cannot be scoped per-shell (all shells in the same project share the project root's override file). The docs only document this file in the worktree context. Worth a clarifying code comment and a doc note, since the behavior is legitimately different from the worktree case.

4. applyEnvOverrides called in home-dir error path without Validate()

if _, err := os.UserHomeDir(); err != nil {
    applyEnvOverrides(cfg)
    return cfg, nil  // no Validate()
}

The os.IsNotExist path correctly calls both applyEnvOverrides and Validate(). The home-dir error path calls only applyEnvOverrides. Minor inconsistency — if Validate() catches any constraint on the new fields (it may not today, but could in future), this path silently bypasses it.

---

Test coverage gaps

5. resolveConfigAgentStart wildcard fallback chain is untested

The lookup order []string{string(agentType), "*", "default"} is not exercised at the unit level. The existing tests verify agentType-keyed lookup and the backward-compat * key via loader, but there's no test for: agentType miss → * hit, or agentType miss → * miss → "default" hit. Add a table-driven test directly for resolveConfigAgentStart.

6. No test for full three-layer precedence in one scenario

TestResolveAgentBaseCommand_ConfigFallback tests file > config correctly. Missing: a test where the file is absent, config agentStart has only a "*" key (not the agent-type key), and the expected result is that wildcard value. This exercises the config wildcard path through resolveAgentBaseCommand end-to-end.

7. Env override with blank value is untested

There's no test for SIDECAR_WORKSPACE_DEFAULT_AGENT_TYPE= (set but empty) — specifically that it doesn't block the secondary var from being used (which it currently does, per issue #2 above).

---

Minor nits

• getDefaultCreateAgentType reads .sidecar-agent directly with os.ReadFile rather than calling loadAgentType. Duplicate logic — consider calling the shared helper.
• isCreateDefaultAgent is a confusing name for a generic type validator. isKnownAgentType or isValidAgentType would be clearer.
• saveWorkspaceConfig will write defaultAgentType to disk even when the value came from an env override rather than the config file. Env overrides are typically expected to be ephemeral. Not a blocker, but worth a doc note warning that saving config will persist the env-set value.

---

Summary

Precedence chain is correct. Nil safety is sound. Test coverage is good for the main paths. The items I'd want addressed before merge: (1) misleading test name on replacement-char stripping, (2) blank env var silently suppressing the fallback var, and (3) a direct test of resolveConfigAgentStart's wildcard/default fallback chain. The shell-vs-worktree asymmetry in .sidecar-agent-start resolution should at minimum be documented.

[aappddeevv]

Marcus, I'm happy closing this PR.

But I do need a way to specify the "command" that is run for my agent.

I generally have some scripts that run to start the agent and get the right config in place and I use 2 different agent CLIs neither of which I start just with the "command" executable name.

The key thing is a "default" agent that matches the enum, a way to match the enum to a "command". In addition, per repo setting would be nice. I keep a few worktrees open for work and don't always start a new one for everything. So a per "worktree" way of setting the agent command would be great. Note than when pressing 's' on a worktree that already exists it tries to use smart logic to find the right agent to run, and that fails for me since I have alot of agent config around. Then it defaults to claude, which I don't use.

[marcus]

Hey @aappddeevv — Kestrel here (Marcus's AI assistant). 👋

Thanks for the detailed breakdown of your use case. The core needs are clear:

1. Default agent that maps to an enum value
2. Custom command per agent type (so your startup scripts work, not just the bare CLI name)
3. Per-repo default
4. Per-worktree override (especially important since you keep existing worktrees open and the 's' key auto-detection logic currently fails → falls back to Claude)

Your PR actually addresses all four of these! The question is really whether Marcus wants to merge this approach or revisit the design.

Flagging Marcus to weigh in on whether to merge #198 as-is or take a different direction. ✦

[marcus]

Hey @aappddeevv — genuinely appreciate this PR. 917 lines of thoughtful feature work is no small thing, and the core design here (defaultAgentType + agentStart map + per-worktree .sidecar-agent-start) is solid. Thank you for building it.

I've taken over the branch to polish the remaining items from review so we can get this merged. Here's what I addressed in the follow-up commit:

Bug fix:

• applyEnvOverrides: when SIDECAR_WORKSPACE_DEFAULT_AGENT_TYPE was set but blank, it was short-circuiting and silently dropping SIDECAR_DEFAULT_AGENT_TYPE. Fixed so blank only means "not set" and falls through to the lower-priority var.

Refactors:

• getDefaultCreateAgentType now calls loadAgentType() instead of duplicating the os.ReadFile logic inline — one fewer place to update if that path ever changes.
• Renamed isCreateDefaultAgent → isKnownAgentType: the function answers "is this a recognized agent type?" which has nothing to do with the create modal specifically.

Tests added:

• TestResolveConfigAgentStart_WildcardFallbackChain: covers the full agentType miss → * → default fallback chain
• TestResolveAgentBaseCommand_ThreeLayerPrecedence: end-to-end test of file > config > default override layers
• TestApplyEnvOverrides_*: four cases covering the blank-env fix and all precedence combinations

Cosmetic:

• Renamed TestResolveAgentBaseCommand_ReplacementCharFallsBack → ..._ReplacementCharStripped — the function strips the char and returns a valid command; it doesn't fall back.
• Added a code comment in startAgentInShell explaining why shell sessions look in p.ctx.WorkDir (workspace root) instead of the worktree path for .sidecar-agent-start.

All tests green (go test ./...). Will merge once CI comes back clean.

— Shrike 🪡

[aappddeevv]

Thanks a bunch! Will test on the next release!

[marcus]

Looking forward to hearing how it goes! The defaultAgent + custom command support you pushed for was the right call — glad it made it in. ✦