feat: OpenCode as a managed non-routable host adapter (ADR-0017) - #67
Conversation
e124fce to
53f41d6
Compare
pacphi
left a comment
There was a problem hiding this comment.
Requesting changes after an independent documentation, command-surface, and dashboard review.
Summary:
- Documentation precedent is not met yet: the new opencode section is useful, but canonical command/provider/upgrade docs still describe a Claude/Codex-only lifecycle and contradict the actual opencode opt-in path.
- The implementation touches the relevant lifecycle paths (setup, status, sync, uninstall, provider status/pick/off, guidance/nudge), but command-level integration coverage is incomplete.
- The dashboard implementation appears to preserve the core read-only/status-row contract and places opencode under Hosts, but its tests do not yet prove rendered behavior or managed-version drift/update-banner behavior.
- There is a current merge blocker:
pnpm run checkfails in ESLint.
Verification performed on head 53f41d6:
pnpm test: PASS (including 58 dashboard tests)pnpm run test:surface: PASS (22 tests)pnpm run check: FAIL (twono-unused-varserrors atsrc/commands/x/provider.mjs:20)- Dashboard visual harness: timed out waiting for
networkidle; I am not treating that timeout as a product finding because it was not isolated from the harness/environment.
Please address the inline blockers and documentation/test gaps. In particular, update docs/PROVIDERS.md and docs/UPGRADING.md even though they are not currently in this diff: both are canonical adoption docs and presently direct users to provider pick, which deliberately refuses opencode.
| import { loadKitConfig, saveKitConfig } from '../../lib/config.mjs'; | ||
| import { undoOpencode, removeArtifacts } from '../../lib/opencode.mjs'; | ||
| import { ok, warn, fail, info, dim, bold, yellow } from '../../lib/output.mjs'; | ||
| import { installedVersion, cmpVersions } from '../../lib/versions.mjs'; |
There was a problem hiding this comment.
Blocking: pnpm run check fails here because both installedVersion and cmpVersions are unused. Please remove this import (or restore the intended use) so the required check gate passes.
| ### opencode host (opt-in) | ||
|
|
||
| [opencode](https://opencode.ai) is a third host alongside claude/codex — wired through its own | ||
| native surfaces rather than env flags. `ak setup --opencode` (or `providers.hosts.opencode: true` |
There was a problem hiding this comment.
Documentation precedent is not met by adding only the later opencode section. These canonical verb rows still say setup enables Codex only, status covers Claude/Codex only, and sync installs Claude/Codex only (lines 71-73); the Frontier hosts section below also says “one or both” and enumerates only those two. Please make the command table and host model internally consistent with the third host. Also update docs/PROVIDERS.md and docs/UPGRADING.md: they currently teach ak x provider pick as the host opt-in switch, while this PR deliberately rejects opencode in that command. Existing users otherwise have no accurate canonical adoption path.
| assert.deepEqual(mcpCommandFor({ binPresent: false, nestedPath: nested }), ['node', nested], 'nested absolute path when no bin (fresh ruflo-only machine)'); | ||
| assert.deepEqual(mcpCommandFor({ binPresent: false, nestedPath: path.join(d, 'absent.js') }), ['ruflo', 'mcp', 'start'], 'last resort matches the claude/codex registration'); | ||
| rm(d); | ||
| }); |
There was a problem hiding this comment.
The helper/owner-module cases are strong, but this suite never executes the new command orchestration. This PR adds machine-mutating branches to setup, sync, uninstall, and provider off/pick plus a new status collector path. Repository precedent explicitly treats those as high-blast-radius command paths. Please add sandboxed command-level tests for enable → setup/sync → status → off/uninstall, including --dry-run, absent opencode CLI, persistence ordering, and teardown. Helper tests alone will not catch a command forgetting a gate, save, or dry-run boundary.
| const r = await get(url); | ||
| contains(r.body, 'opencode:"hosts"'); | ||
| // and it must sort with the host MCP subsystems, not at the unknown end | ||
| contains(r.body, '"codex-mcp","opencode"'); |
There was a problem hiding this comment.
This proves payload pass-through and that two mapping strings exist in served JavaScript, but not the dashboard behavior required by docs/MANAGED-TOOLS.md. Please add behavior-level coverage that an opencode row renders in the Hosts group with its level/detail/fix, plus drift/update-banner coverage for the managed opencode-ai version (including external-install/unknown-version behavior as applicable). The implementation looks directionally correct; the gap is that the tests do not lock the user-visible contract.
| home seam), `src/lib/blocks.mjs` (`agents-opencode` target, new registry rows), | ||
| `src/commands/{sync,status,setup,uninstall}.mjs`, `src/commands/x/provider.mjs` | ||
| (`off` teardown + `pick` preservation), `src/lib/nudge.mjs` (shared targets), | ||
| `src/lib/dashboard-server.mjs` (Hosts-tab categorization), |
There was a problem hiding this comment.
This reference is inaccurate: Hosts-tab categorization is implemented in src/lib/dashboard/client.mjs (GROUP/PREF), not src/lib/dashboard-server.mjs. Please point the ADR to the actual decision-bearing code so future audits can follow the precedent.
|
Thanks for the substantial work here. The owner module and ownership model are thoughtful, and I do not want the follow-up to become a redesign of the adapter. The remaining work is about making this a first-class Below is the maintainer remediation plan I recommend. Please treat items 1–5 as required for another review. The final section explicitly identifies what should remain out of scope. 1. Restore the required quality gate first
Required:
pnpm run check
pnpm run test:surfaceAcceptance:
2. Make the host model and adoption path coherentThe current PR calls opencode a third host, but the ordinary host-management command deliberately refuses it. That creates two competing models:
My preferred resolution is to make
Concretely, in
If there is a concrete upstream limitation that makes this unsafe, the acceptable fallback is to add a dedicated post-install command (not a manual Acceptance:
3. Add command-level lifecycle tests
Please extend the existing command suites rather than putting every scenario into the owner-module test:
Use the existing home sandbox/fake-bin patterns. Tests must not touch the contributor's real home, global npm tree, or opencode configuration. Required command scenarios: Setup
Provider selection
Sync
Status
Uninstall
The goal is not to duplicate every helper test. One or two end-to-end sandbox scenarios per command, plus the critical negative/dry-run cases, are enough to prove orchestration. 4. Bring the canonical documentation up to precedentThe new README section is useful, but a later feature section cannot contradict the command table and the canonical provider/upgrading guides. Required documentation updates:
|
|
Remediation complete on head 1. Quality gate
2. Host model + adoption path (
|
|
Follow-up filed: the one out-of-scope finding from the round-3 cross-vendor review (routing-host exclusion leaves codex routes/bridges live — pre-existing, predates this PR) is now tracked as #73 with a fix sketch and acceptance criteria. Keeping it out of this PR per your scope instruction; can pick it up next. |
|
With #74 now merged, this PR should be rebased onto current Recommended order:
The earlier requested-change items appear addressed at the pre-rebase head, but the review state should remain open until the combined post-#74 implementation is verified. Please avoid resolving conflicts by retaining both capability systems; the registry/lifecycle model now on |
…n wiring (ADR-0015)
opencode (opencode.ai) joins claude and codex behind the host-adapter
abstraction, wired through its own native surfaces (never env flags, which
don't exist for it):
- ~/.config/opencode/opencode.json: claude-flow + ruvnet-brain MCP servers,
skills.paths, permission patterns — merged backup-first with VALUE-precise
ownership (collisions preserved, priors restored on teardown, stale ak
entries pruned only while == written; scalar permission lifted to {"*": v};
JSONC refused, never clobbered)
- plugins/ruflo-hooks.js: opencode's plugin events mapped to ruflo's local
hook verbs (session lifecycle, pre-bash safety, edit/task learning, route
context) — the only hook surface opencode has
- agents/: ruflo's agent set converted (frontmatter shape, YAML-safe
descriptions, all three mcp__ spellings rewritten, collision-prefixed,
documentation skipped), drift-stamped against a lazily-resolved catalog
source (override → RUFLO_REPO → marketplace clone → npm direct/nested)
- AGENTS.md guidance: new agents-opencode target + opencode-flavored block
templates, same never-mkdir gate as ~/.codex
- lifecycle: status subsystem (deep convergence probe), sync apply (after
hosts install, CLI-gated), setup --opencode, x provider off / uninstall
teardown, dashboard Hosts-tab categorization, nudge consistency via the
shared guidanceTargets composition, pick preserves the opencode flag,
detectHosts reads config-file wired state
25 new kit tests + dashboard/opencode cases; suites green: 605 + 189 tests,
eslint, tsc, markdownlint. Cross-vendor reviewed (21 findings addressed).
…eview (round 2)
Address all seven round-2 findings:
- ownership: a collision the user later aligns to the desired value stays
unmanaged (never adopted with a stale prior); desired-set shrink RESTORES
priors instead of deleting entries ak didn't author; permission scalar
origin is recorded explicitly (a pre-existing {"*": v} object survives
undo as an object)
- guidance: the opencode blocks are gated on enablement (opencodeEnabled
flag, same mechanism as dualMode) — an installed-but-disabled host no
longer receives guidance asserting active wiring, and off → sync strips it
- lifecycle: uninstall --purge no longer recreates kit.json via the opencode
teardown save; isDefault counts hosts.opencode (claude+opencode is not
'claude-only'); injected routing parts use schema-valid PartIDs (prt prefix)
- tests: 6 round-2 cases (collision-align, shrink-restore, wildcard-object
preservation, flag-gated blocks, isDefault, PartID) — 31 in the suite,
611+189 across the repo, tsc/eslint/markdownlint clean
- mcpCommandFor: claude-flow MCP command resolves bin-on-PATH → nested ruflo/node_modules/@claude-flow/cli/bin/mcp-server.js (absolute node path, no PATH/cwd dependence — the plain 'npm i -g ruflo' layout, verified) → ruflo mcp start. A fresh ruflo-only machine gets the purpose-built stdio server instead of the last-resort path. - setup prints the restart hint after wiring: opencode loads config/plugins/MCP/agents once at startup (observed live: sessions miss hooks/tools until restarted; the guidance template now says the same). - TROUBLESHOOTING.md: four opencode rows (startup loading, JSONC refusal, no-clobber 'foreign', missing catalog source). - test: three-way mcpCommandFor selection — 32 in the suite, 612+189 repo-wide.
…code as integration host The maintainer's core review point: the PR called opencode a third host but the ordinary host-management command refused it, leaving existing users no first-class adoption path. Model is now coherent: - Managed host integrations (install/config/guidance/status/sync/teardown): claude, codex, opencode — derived from the HOSTS[].routing capability flag, not hardcoded id lists (the seam issue pacphi#71's registry lands on). - Routing hosts (primaryHost + per-activity seeds): claude, codex only. pick: --host accepts opencode as the complete desired enabled-host set; --primary-host stays claude/codex; opencode never enters dualRouting; an unknown --host token is a hard error before any mutation; interactive defaults are enabled hosts ∪ newly-detected routing hosts (an enabled-but- absent host is never dropped by a bare enter, an installed-but-disabled integration host is never opted in by one). Excluding opencode disables it via the same marker-gated teardown as off/uninstall. Every pick rewrite preserves ALL ownership markers (codexMcp/rufloCodexMcp included — the data-loss class the review called out). setup/sync/pick/off/uninstall now share ONE owner-module composition (opencodeStack / retireOpencode / reconcileOpencodeGuidance) — no merge or ownership logic in commands. opencodeStack reports markersChanged so a converged file with stale markers still re-persists the teardown proof; retireOpencode clears markers on absent config and fails honestly on JSONC (markers retained, wiring reported active, manual remediation named). sync's blocks branch runs after the opencode branch with an opencode guard, so a fresh enable converges guidance in one sync (second sync is a true no-op). Also removes the unused versions.mjs imports that broke pnpm run check. Cross-vendor review (Codex, round 3): 4 findings, all fixed here or in the accompanying tests; codex-side bridge teardown on routing-host exclusion deliberately left as-is (pre-existing behavior; 'provider off' is the documented full reset).
…regressions The owner-module suite alone could not catch orchestration mistakes, so each mutating command now has sandboxed end-to-end scenarios (home sandbox + fake-bin patterns; no real home/global npm/PATH is ever touched): - setup: --opencode persists the host and wires it when the CLI is present, restart guidance emitted; --dry-run writes nothing anywhere; an absent CLI never fabricates ~/.config/opencode. - provider pick (real CLI spawns): enable wires config/plugin/agents/skill + guidance and preserves user config; exclusion disables surgically; --primary-host opencode rejected; retune preserves every ownership marker; unknown --host fails before mutation; JSONC/absent-config disables report honestly; interactive defaults never drop an enabled-but-absent host nor opt in a disabled one; stale markers re-persist on a converged re-pick. - sync: enabled+drifted converges after hosts, before final verification; converged claude guidance + fresh enable lands opencode guidance on the SAME sync; second sync is a true no-op; absent CLI skips without creating the config home; disabled makes no wiring writes and strips only enablement-gated guidance; --dry-run mutates no opencode surface. - status: converged/drifted/JSONC-refused/foreign-plugin/CLI-absent rows are honest non-OK with a specific fix; disabled is silent with the pick hint; --json carries the dashboard-consumed shape; strictly read-only. - uninstall: restores owned config values, removes only marked artifacts, user-edited/marker-less files survive; repeated runs harmless; --purge reads ownership before removing kit.json; --dry-run writes nothing. - providers lib: routing capability flag pinned; dualRouting seeds claude/codex routes ONLY with opencode co-enabled. Plus codex-review regressions: marker persistence on converged files (opencodeStack markersChanged, with destination seams so the test can never escape the sandbox — r4) and npm-managed vs external opencode-ai update ownership (new opencode-version-drift suite).
…te banner under test The previous tests proved payload pass-through and the presence of mapping literals, not rendered behavior. The classification/grouping/card/notice logic now lives in src/lib/dashboard/groups.mjs (pure, no DOM) — one source of truth imported by node tests and serialized verbatim into the served <script> (byte-stable tables, so the shipped JS and the tested JS cannot drift). New tests prove, without a browser: a fixture opencode row is grouped under Hosts & Routing, ordered worst-first then preferred, rendered with its original level/message/fix; an outdated npm-managed opencode-ai names itself in the update banner while a current install fabricates none; the served bundle parses with every interpolated function present. External (non-npm) installs remain unclaimed (versions.mjs coverage in the previous commit). Dashboard stays read-only, offline-first, token-gated, dependency-free.
…, UPGRADING, ADR-0015, help Canonical docs no longer contradict the command surface: - README: setup/status/sync/uninstall verb rows include opencode behavior; the host model names three managed integrations with the claude/codex routing pair distinguished; the adoption command is provider pick (setup --opencode stays the bootstrap path); the honest-absence list is explicit (no statusline, session detection, routing/primary, aqe type, or usage/cost attribution). - PROVIDERS.md: the host model expands beyond Claude/Codex with enable / status / disable examples; the ak-way↔raw-tool-way table gains the opencode row; a value-precise ownership table documents what is ak-owned, user-owned, restored, or preserved across every opencode surface incl. collision, JSONC-refusal, and absent-config behavior. - UPGRADING.md: worked example for adopting opencode on an existing install via ak x provider pick; the sync vs setup vs pick table matches final CLI behavior and states sync never opts a host in. - ADR-0015: the pick section is rewritten for the two-tier model; the dashboard reference is corrected to src/lib/dashboard/client.mjs; the enablement-gated guidance detectors are described as shipped; the shared stack composition, marker persistence, retire honesty, and capability-flag derivation are recorded. - TROUBLESHOOTING: the ADR link is corrected to 0015 (was 0011). - bin/agentic-kit.mjs + setup/provider --help: the two-tier distinction is visible in help output, and the post-install adoption path is named.
4f1ddae to
b7519b3
Compare
|
Rebase/remediation complete on What changed to align this PR with current
Verification:
PR is now GitHub-mergeable; CI is running. |
|
Windows CI exposed a portability issue in the OpenCode CLI test fixtures: they created only POSIX executables, so |
|
Follow-up |
|
Final verification: CI is green on Ubuntu, macOS, and Windows for Node 22/24/26, plus quality and internal-link checks. GitHub reports this PR as |
Summary
Adds opencode (opencode.ai) as a third host adapter alongside
claudeandcodex, wired through opencode's own native surfaces — the same "write the host's own config, never a parallel config layer" rule the codex adapter established. After this,ak syncconverges opencode with the same guarantees claude/codex already have: ruflo upgrades re-key its agents/skills/plugin, brain updates hot-swap through the stable-spine shim, andak statustells the truth about all of it.Design record:
docs/adr/0017-opencode-host.md(context / decision / consequences / alternatives).What opencode gets (all opt-in,
hosts.opencode: falsedefault)~/.config/opencode/opencode.jsonclaude-flowMCP (viaclaude-flow-mcp,ruflo mcp startfallback) +ruvnet-brainMCP (stable-spine shim),skills.paths,permissionallow-patterns~/.config/opencode/plugins/ruflo-hooks.jspre-bashsafety, edit/task learning, route context)~/.config/opencode/agents/mcp__*spellings rewritten toclaude-flow_*, collision-prefixed,type: documentationskipped~/.config/opencode/AGENTS.mdagents-opencodeguidance target + opencode-flavored block templates (same never-mkdirgate as~/.codex)~/.config/opencode/skills/ruflo/SKILL.md, stamped to the catalog sourceThe catalog source resolves lazily:
opencodeCatalogDiroverride →$RUFLO_REPO→ claude marketplace clone (full catalog, auto-updated) → published@claude-flow/cli(substrate) → nestedruflo/node_moduleslayout. Drift is detected by stamp comparison (source id + deployed file list), so a ruflo upgrade or marketplace pull triggers re-conversion on the next sync.Safety contracts (the part I'd want reviewed hardest)
~/.config/opencode); sync's opencode step runs after the hosts install branch.uninstallreads ownership markers before anykit.jsonremoval.Adjacent fixes folded in (each justified in the ADR)
nudge.mjsnow uses the sharedguidanceTargets+retiredForTargetcomposition (its contract is "never disagrees withak status"; the hardcoded subset also missed codex'sagents-user— pre-existing gap, now closed).x provider pickpreserveshosts.opencode+ ownership markers instead of rebuildingproviderswithout them; opencode is excluded from the pickable routing hosts with a warning.detectHostsreads the config-file host's wired state from its own config (wasenv[null]→ permanently "not wired").opencodesubsystem into the Hosts tab (its designed fallback is Runtime, so nothing was ever dropped — this is categorization, plus tests pinning it).hostAuthStategains ahometest seam; fixes a pre-existing env-dependent test that failed on any machine with file-based claude credentials (it failed onmainhere).Deliberately out of scope (matches codex's own asymmetry)
Routing-table integration (
routing.mjsuntouched), aqe provider wiring (no opencode provider type exists), statusline (no upstream surface),drivingHostsession detection (opencode sets no session env marker), and usage/cost attribution (usage-index.mjsreads claude/codex transcripts only — the pricing surface has no opencode input and shows nothing for it).Verification
Final head:
f1e538b