fix(opencode): detect agent-id collisions instead of silently dropping profiles

[tedswinyar]

Surfaced during an external code review of the OpenCode config layer by @jhp612.

to_opencode_agent_id() maps a profile name to its OpenCode agent id by replacing / with __. That mapping is not injective, and the id is used as a key on disk in three places — the <id>.md agent file, the agent.<id> section of opencode.json, and the --agent <id> runtime value. When two distinct profiles resolve to the same id, installing the second silently overwrites the first's files and config with no warning. Two ways this happens:

• the / → __ rewrite: a profile named a/b and a literal profile named a__b both produce a__b;
• two different profile files carrying the identical frontmatter name:.

(Note: names differing only by spaces or punctuation — foo bar vs foo-bar — do not collide, since only / is rewritten.)

Fix. A guard on the opencode install path (_guard_opencode_agent_id_collision) that fails loud instead of overwriting. Install runs one profile at a time, so the guard reconstructs the id-space the way installs actually populate it: profiles are discovered by file stem (the install handle, never contains /), but the installed id derives from the resolved frontmatter name (which may contain /). It resolves every other installable profile's name and, excluding the profile being installed by stem, raises OpenCodeAgentIdCollisionError (a ValueError, so the existing handler turns it into a clean CLI Error: rather than a traceback) naming both offending profiles when a different file resolves to the same id — including the case where the two resolved names are byte-for-byte identical. Discovery/parse failures on sibling profiles are non-fatal (skipped, not raised), so one unreadable profile can't block an unrelated install.

Tests. Against the real cao install CLI: a/b vs a__b fails naming both; two different files with an identical resolved name fails and leaves the first profile's file intact; reinstalling the same profile stays idempotent; foo bar vs foo-bar both install; single-profile install unaffected.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 87.44395% with 28 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@0ac5cc9). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...cli_agent_orchestrator/services/install_service.py	87.38%	28 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #493   +/-   ##
=======================================
  Coverage        ?   90.62%           
=======================================
  Files           ?      177           
  Lines           ?    22628           
  Branches        ?        0           
=======================================
  Hits            ?    20507           
  Misses          ?     2121           
  Partials        ?        0           

Flag	Coverage Δ
unittests	90.62% <87.44%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

This PR prevents silent overwrites in the OpenCode install path by detecting when two distinct agent profiles would resolve to the same OpenCode agent id (due to the non-injective / → __ mapping or duplicate frontmatter name: values), and failing with a clear CLI error instead.

Changes:

• Introduces an explicit OpenCodeAgentIdCollisionError (a ValueError) and an install-time guard that scans other installable profiles and raises on id collisions.
• Adds unit tests documenting the to_opencode_agent_id() behavior and the collision error’s type contract.
• Adds end-to-end CLI tests covering real collision cases, idempotent reinstall behavior, and non-colliding names.

Reviewed changes

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

File	Description
src/cli_agent_orchestrator/services/install_service.py	Adds _guard_opencode_agent_id_collision() and invokes it before writing OpenCode agent files/config to prevent silent overwrites.
src/cli_agent_orchestrator/utils/opencode_config.py	Documents the non-injective id mapping and introduces OpenCodeAgentIdCollisionError.
test/cli/commands/test_install_opencode.py	Adds CLI-level tests to validate collision failures, preservation of existing installs, idempotent reinstall, and non-colliding cases.
test/utils/test_opencode_config.py	Adds focused tests for to_opencode_agent_id() behavior and the collision error type contract.

---

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

[haofeif]

@tedswinyar Thanks for fixing the OpenCode agent-id collision case. The guard catches the main overwrite path, but I think there is still one blocking ordering issue before this can merge.

The collision guard currently runs after the shared context file is written:

context_file = _write_context_file(profile.name, raw_content)
...
_guard_opencode_agent_id_collision(agent_name, profile.name)

I reproduced this with two different profile files that both resolve to name: shared-alias. The second install correctly fails and preserves opencode_cli/agents/shared-alias.md, but AGENT_CONTEXT_DIR/shared-alias.md has already been overwritten with the rejected profile body.

Suggested fix: run _guard_opencode_agent_id_collision(agent_name, profile.name) before _write_context_file(profile.name, raw_content) for the OpenCode install path, or otherwise make the failed collision path atomic for the context file too.

Could you also add a regression assertion to the collision test that the context file remains unchanged after the rejected second install? The current tests cover the OpenCode agent file preservation, but not the shared context file.

[gutosantos82]

PR Review: #493 — fix(opencode): detect agent-id collisions instead of silently dropping profiles

• Author: tedswinyar · Head: b67ba963 · Base: main · Re-review (prior report at c6e1bba4: Approve with nits)
• Size: +281 / −1, 4 files · CI: all 13 checks green (unit tests 3.10/3.11/3.12) · reviewDecision: REVIEW_REQUIRED
• Files: services/install_service.py, utils/opencode_config.py, test/cli/commands/test_install_opencode.py, test/utils/test_opencode_config.py

What changed since the last review (c6e1bba → b67ba96)

The previous round's nit (1) — shared context file written before the guard, so a refused
install still corrupted AGENT_CONTEXT_DIR/<id>.md — is fixed. This was also raised as a
blocking comment by @haofeif (2026-07-22 03:49Z); the head commit (04:10Z) addresses it:

• context_file = _write_context_file(...) moved out of the early common path; on the
  opencode branch the guard now runs before the context write, so a rejected install
  leaves all files (provider-specific and shared) untouched.
• New regression assertion in test_same_resolved_name_different_stem_fails_and_preserves_first
  verifies the context file is byte-identical after the rejected second install.

Verified by direct read of the diff and by running the tests at head.

The underlying fix (unchanged, still sound in concept)

to_opencode_agent_id() is name.replace("/", "__") — many-to-one. The id keys three
on-disk locations (<id>.md, agent.<id> in opencode.json, --agent <id>), so before
this PR a second profile resolving to an existing id silently overwrote the first.
_guard_opencode_agent_id_collision() fails loud instead, raising a ValueError subclass
that the existing handler renders as a clean CLI Error:. Guard mechanics: excludes the
installing profile by stem, skips unloadable/unreadable siblings non-fatally, names both
offending profiles in the message.

BLOCKING: reinstall false-positive via the agent-context directory (new finding, reproduced)

The guard builds its candidate set from list_agent_profiles(), which — with real default
settings — scans the cao_installed provider dir, and that dir defaults to
AGENT_CONTEXT_DIR (settings_service._DEFAULTS: "cao_installed": .../agent-context).
install_agent() itself writes a full copy of the profile source there as
<resolved-name>.md. Consequence: for any profile whose file stem differs from its
frontmatter name:, the first install succeeds, and then every subsequent
reinstall/upgrade is blocked forever:

1. cao install ./my-dev.md where frontmatter says name: cool-agent
   → installs OK, writes agent-context/cool-agent.md (a copy of the source).
2. Reinstall my-dev → discovery finds a candidate with stem cool-agent
   (the context copy; stem ≠ my-dev, loadable=True since it parses fine),
   _read_agent_profile_source("cool-agent") finds it in the provider-dir search path,
   its resolved name cool-agent → id cool-agent == target id → raise.

Reproduced end-to-end at head b67ba963 with a fresh HOME and no settings overrides:

install #1 exit: 0   ✓ Agent 'cool-agent' installed successfully
install #2 exit: 0   Error: Failed to install agent: OpenCode agent id 'cool-agent' is
  produced by both the profile being installed ('my-dev.md', name 'cool-agent') and the
  existing profile 'cool-agent.md' (name 'cool-agent'). ...

The error is doubly confusing: the "existing profile" it names is the profile's own
context copy, and the suggested remedy (rename one frontmatter name:) cannot work —
the copy regenerates on every install.

Stem ≠ name is a first-class, documented flow: cao install ./my-custom-agent.md (CLI
copies by file stem, name comes from frontmatter) and URL installs (_download_agent
derives the stem from the URL filename). Reinstall-to-upgrade is also the normal update
path (an existing test, test_reinstall_without_mcp_removes_agent_tools, exists precisely
because reinstalls are expected).

Why the PR's tests miss it: the install_workspace fixture patches
settings_service.get_agent_dirs to {}, removing the cao_installed → agent-context
mapping that production defaults include. test_reinstall_same_profile_stays_idempotent_despite_guard
only covers stem == name, where the context copy dedups against the store entry by stem.

Suggested fix: exclude install-produced artifacts from the candidate set — e.g. skip
candidates whose discovery source label is "installed" (the cao_installed dir), or
exclude candidates whose resolved source path lives under AGENT_CONTEXT_DIR. Add a test
that keeps the default cao_installed mapping and reinstalls a stem≠name profile.

Non-blocking notes (carry-overs and nits)

1. Env-var asymmetry (carried over): the guard parses sibling profiles without
   resolve_env_vars, while the install path parses the resolved text. A profile with
   name: ${VAR} frontmatter is compared on the unresolved string. Edge case; worth a
   comment at most.
2. Same latent bug in other providers (carried over): kiro/copilot use the same
   non-injective profile.name.replace("/", "__") for safe_filename and remain
   unguarded. Deliberately out of scope here — suggest a follow-up issue.
3. Redundant if/else both assigning context_file = _write_context_file(...) — could be
   a single write after a guarded if. Cosmetic.
4. Stray blank line added in the opencode branch (+ line before agent_id = ...). Cosmetic.
5. Pre-existing (not this PR): _write_context_file with a /-containing name targets a
   nested path whose parent dir is never created; the new collision test works around it by
   pre-creating context_dir/a. The guard raising first makes this unreachable on the
   collision path, but the latent issue remains for a successful slash-named opencode
   install. Not blocking.

Verification performed

• Read the full diff and the surrounding upstream-main code it hooks into
  (install_agent, _read_agent_profile_source, list_agent_profiles/_scan_directory,
  settings_service._DEFAULTS, CLI install file-copy path).
• Ran both touched test files at head under a clean writable HOME: 64 passed, 1 skipped
  (an earlier all-fail run was this sandbox's read-only ~/.aws, not the PR).
• Reproduced the reinstall false-positive end-to-end at head with default dirs (script
  driving the real install CLI, fresh HOME).
• CI at head: all 13 checks green.
• Publish-gate sensitivity: touches services/install_service.py (not on the sensitive-path
  list), +281 lines / 4 files — under the size gate; moot given the verdict.

Verdict

Request changes — the guard's intent is right and the previous round's blocking issue is
properly fixed, but as written it permanently breaks reinstall/upgrade for any opencode
profile installed from a file or URL whose stem differs from its frontmatter name, with a
misleading error. One candidate-set exclusion plus a non-neutered-fixture test should get
this to approve.

[gutosantos82]

PR Review: #493 — fix(opencode): detect agent-id collisions instead of silently dropping profiles

• Author: tedswinyar · Head: f392fbfe · Base: main · Re-review (prior review at b67ba963: Request changes)
• Size: +1847 / −4, 6 files · reviewDecision: CHANGES_REQUESTED (stale — pre-dates this head) · mergeable: MERGEABLE
• Files: services/install_service.py, utils/opencode_config.py, schemas/agent_profile.schema.json, 3 test files (1 new)

Summary

This head resolves the blocking finding from the previous review round — the collision
guard's reinstall false-positive for profiles whose file stem differs from their frontmatter
name — via a provenance marker (x-cao-source-stem) stamped into install-produced context
copies, plus a hardened atomic, symlink-safe context writer with spoof-refusal readback. The
fix was dynamically verified end-to-end: the silent-overwrite bug reproduces at the merge
base and is blocked at this head, the previously-broken stem≠name reinstall now succeeds
under real default settings, and the trap case (deleting the local source then installing a
colliding profile) still raises. The code substance is merge-quality; what still needs work
before merge is the PR metadata and documentation trail (the description now covers a small
fraction of what shipped), plus two consistency seams worth an explicit decision.

Blocking (must fix before merge)

• [conversation] PR title/body are stale and materially understate scope — the diff grew
  ~6.5x since the last-reviewed head (+281/−1 across 4 files → +1847/−4 across 6), and the
  head commit is titled "stamp context-copy provenance and block source-collision spoofing",
  but the PR description still describes only the original narrow collision guard. The
  provenance mechanism, spoof-refusal gate, atomic/symlink-safe writer, mode preservation,
  and schema change — the majority of the change — are undescribed. Maintainers merging from
  this description would not know what they are merging. Please rewrite title/body to match
  what shipped.
• [conventions] No CHANGELOG [Unreleased] entry — the PR adds user-visible
  install-refusal behaviors (collision error, marker-spoof refusal, non-regular-target
  refusal) and a new schema field, but touches no CHANGELOG.md. The convention is active in
  this repo (the immediately-preceding merge added entries).

Important (should fix)

• [consistency] install_service.py:444 vs :550 — context-dir write path and guard read
  path can diverge. _write_context_file writes to the hard-coded AGENT_CONTEXT_DIR
  constant, while the new _installed_context_copy_path (and profile discovery) resolve the
  dir from the user-overridable cao_installed settings key. Defaults are identical so tests
  pass, but under a settings override the writer deposits copies where the guard and
  discovery never look, silently weakening the collision protection this PR introduces. The
  discovery/writer asymmetry pre-dates this PR, but the guard newly couples to it — align
  both on one source (both the setting, or both the constant).
• [consistency] Opencode-scoped fix changes behavior for ALL providers. Provenance
  stamping and the strict spoof-refusal readback run unconditionally in
  _write_context_file, so a kiro/copilot/codex profile carrying a conflicting
  x-cao-source-stem frontmatter key is now refused installation where it previously
  installed fine — while the guard that consumes the marker runs only for opencode. The
  fail-closed direction is defensible (the key is reserved), but the title says
  fix(opencode); either scope the stamping to opencode or state the universal behavior as
  intended in the PR body.
• [consistency/conventions] x-cao-source-stem schema/validate/install contradiction, and
  the doc-maintenance rule. Adding the key to agent_profile.schema.json (under
  additionalProperties: false) means cao profile validate accepts a source profile
  declaring it — yet install refuses that same profile via the spoof gate, and the
  pydantic model silently drops it. The field is CAO-generated provenance for the context
  copy, not authored input. Recommend a one-line schema description documenting it as
  reserved/CAO-written (user values rejected at install), and either a matching note in
  docs/agent-profile.md (per CODEBASE.md's documentation-maintenance rule for profile-field
  changes) or an explicit statement that it is internal-only.

Nits (optional)

• [conventions] install_service.py — _FRONTMATTER_DELIMITER_RE (~L245) and
  _TEMP_FILE_NAME_ATTEMPTS (~L480) are defined mid-file; the module groups constants near
  the top (L71–135).
• [consistency] kiro/copilot still use inline profile.name.replace("/", "__") while
  opencode routes through to_opencode_agent_id(), now documented as the single source of
  truth — and they share the same latent unguarded collision. Suggest a follow-up issue
  routing all providers through the helper (carried over from the prior round).
• [consistency] docs/opencode-cli.md documents the /→__ id derivation but not the
  new hard-fail collision behavior or the markerless-legacy-copy remedy message operators
  will now encounter.
• [tests] Minor coverage gaps — the nested <stem>/agent.md installed-copy path branch,
  the temp-file O_EXCL retry-exhaustion branch, and the FIFO/device arm of
  _non_regular_target_error are untested; the schema pattern for the marker diverges from
  what _yaml_single_quoted accepts (stems with :/quotes/spaces) without a test noting it.
• [conventions] Test DRY — test_install_opencode_provenance.py re-implements
  _line_body_and_ending locally instead of importing it from install_service, and both
  test files redefine a local _write_profile.
• [consistency] Pre-existing, acknowledged in the PR's own test comments: a slash-named
  profile's context copy targets a nested path whose parent dir _write_context_file never
  creates; tests work around it with mkdir. Unreachable on the collision path (guard raises
  first) but still latent for successful slash-name installs — fine as a follow-up issue.
• [conventions] Stray blank line before agent_id = to_opencode_agent_id(...) in the
  opencode branch (carried over).

Tests

Strong. 107 passed / 1 skipped (pre-existing optional live-binary smoke test) across the
three touched files. Both non-injective failure modes (a/b vs a__b; same resolved name,
different stems) are pinned separately; the fixed regression AND the bug it must not reopen
are both covered with byte-for-byte preservation assertions; the provenance-spoof matrix
(quoted keys, flow mappings, folded scalars, duplicate markers, 4-dash delimiters, BOM/CRLF)
is closed and regression-netted; mode-preservation and error-message hygiene (no temp path /
errno leakage) are asserted. Confirmed against the merge base that these are genuine
regression tests, not tautologies. Remaining gaps are edge branches listed under Nits.

Verification

Dynamic verification ran at this head (mise Python 3.12, isolated CAO_HOME_DIR, worktree
PYTHONPATH):

• ✓ VERIFIED — silent overwrite reproduces at merge base 0ac5cc92 (second colliding
  install exits 0, both artifacts clobbered) and is refused at head with both artifacts
  byte-identical to the first install.
• ✓ VERIFIED — stem≠name profile reinstalls repeatedly under real default settings
  (the prior round's blocker); body-change upgrade reinstall lands the new body.
• ✓ VERIFIED — trap case: install A, delete A's local source, install colliding B → still
  raises; A's installed artifact intact.
• ✓ VERIFIED — spoofed x-cao-source-stem (quoted-key variant) in a source profile is
  refused with nothing written.
• ✓ VERIFIED — a/b vs literal a__b collision blocked both ways; slash-named profile
  alone reinstalls idempotently.
• ✓ VERIFIED — schema change is load-bearing: a stamped copy's frontmatter validates at
  head, fails at merge base; malformed marker values rejected by the pattern.
• ✓ VERIFIED — symlink occupying the context path is refused; no leftover .tmp files
  after refused installs.
• ✓ VERIFIED — kiro install also stamps the marker (see the all-provider scope finding
  under Important).

Verdict

Request changes — the code substance is verified sound and the two prior blocking findings
are genuinely resolved, but the PR description now covers only a small fraction of what
ships, the CHANGELOG and profile-field documentation rule are unaddressed, and the
context-dir divergence and all-provider stamping scope deserve an explicit fix-or-justify
before merge. All asks are metadata/documentation-level plus two contained code seams; once
addressed this is approvable.