feat: universal hooks for Codex, OpenCode, and Cline#94
Merged
WellDunDun merged 26 commits intomainfrom Apr 6, 2026
Merged
Conversation
Add multi-platform hook abstraction layer so selftune can capture real-time telemetry from non-Claude-Code agents. Includes shared hook types/normalizers (hooks-shared/), per-platform adapters with install commands, CLI routing, SKILL.md workflow, and doc updates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSplit CLI hooks into shared utilities and per‑platform adapters; add Codex/OpenCode/Cline hook handlers and installers; centralize normalization, output formatting, git/commit parsing, session‑state persistence, agent CLI call logic, tests, and docs; update CLI routing to dispatch platform subcommands and add related workflow docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Platform as Platform Host (Codex / OpenCode / Cline)
participant Adapter as Adapter CLI ("selftune <platform> hook")
participant Normalizer as hooks-shared/normalize
participant Processors as shared processors (guards/tool handlers/commit-track/session-stop)
participant Storage as session-state / commit DB
Note over Platform,Adapter: Platform pipes native JSON payload to adapter stdin
Platform->>Adapter: stdin (native JSON)
Adapter->>Normalizer: normalizeHookEvent(payload, platform, nativeEvent)
Normalizer-->>Adapter: UnifiedHookEvent
Adapter->>Processors: dispatch(UnifiedHookEvent)
Processors->>Storage: optional writes (session-state files, commit records)
Processors-->>Adapter: HookResponse + advisory via stderr
Adapter->>Platform: platform-specific stdout/stderr + exit code
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/cline/hook.ts`:
- Around line 48-50: The PostToolUse shortcut that uses the 4096-byte preview
causes false negatives; update the adapter to always use the full stdin payload
read by readStdin() instead of branching on preview content. Specifically,
remove or disable any early-return/shortcut logic that checks preview (the code
path using raw.slice(0,4096) from readStdin and any conditional that triggers
PostToolUse based on preview) and ensure downstream logic (the PostToolUse
handling around the same file where preview-based checks exist) consumes the
returned full string (`full`) from readStdin() for decision-making and parsing.
In `@cli/selftune/adapters/cline/install.ts`:
- Around line 180-182: Wrap the direct-entry call guarded by import.meta.main in
a try/catch so failures print a single-line error and exit with code 1 instead
of leaking a stack trace; specifically, call await cliMain() inside try, catch
any thrown error, output a one-line failure (e.g., console.error or the module
logger with a message that includes error.message but not the full stack) and
call process.exit(1); ensure you reference the import.meta.main guard and the
cliMain function when making this change.
- Around line 26-34: The PostToolUse branch of hookScript is emitting a second
JSON document ("{'cancel': false}") after piping the hook output, causing
concatenated JSON; modify the PostToolUse block in function hookScript so it
does not echo or print '{"cancel": false}' (leave the MARKER and the npx
selftune cline hook invocation intact), ensuring only the hook.ts output is
written to stdout and preventing double JSON documents.
In `@cli/selftune/adapters/codex/install.ts`:
- Around line 107-132: The helper and remover functions use the inconsistent
name isSelftunHook / removeSelftunHooks while the marker field is _selftune;
rename the functions to isSelftuneHook and removeSelftuneHooks and update all
call sites (e.g., the filter in mergeHooks and its use in removeSelftuneHooks)
to use the new names so identifiers consistently match the `_selftune` field;
ensure exports and any tests or imports are updated accordingly.
In `@cli/selftune/adapters/opencode/hook.ts`:
- Around line 65-78: The fast-path uses a 4096-byte preview which can miss
relevant tokens that occur later; update the preview window in
cli/selftune/adapters/opencode/hook.ts by increasing the slice size from 4096 to
8192 (adjust the preview = raw.slice(0, 4096) to preview = raw.slice(0, 8192))
so the isBefore/mightBeInteresting check catches more cases, or alternatively
move the relevance check (the isBefore / mightBeInteresting logic) to after the
full parse call so you only short-circuit with outputResponse({ modified: false
}) when you’ve examined the parsed payload; make sure to update references to
preview, isBefore, mightBeInteresting and the early return path accordingly.
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 55-60: readConfig currently treats parse errors as an empty object
which lets install blindly overwrite unrelated hooks; change readConfig to
detect malformed JSON and return null/undefined (or throw) instead of {} so
callers know config is invalid and won't mutate it, then in the
install/uninstall flows (functions referenced as doInstall/doUninstall and the
code that writes back the OpenCodeConfig) only mutate config when it's a real
object (create a fresh config if none exists but avoid touching malformed
files), ensure you merge-preserve existing hooks (only set hooks[event] when it
doesn't exist or when it already points to your shim), and update doUninstall to
delete a hook entry only if config.hooks[event]?.command === shimPath to avoid
removing unrelated handlers.
In `@skill/SKILL.md`:
- Around line 124-130: Update the trigger keywords so the PlatformHooks routing
row mirrors the six concrete commands in the Quick Reference: add symmetric
phrases for each platform (codex, opencode, cline) including both "hook(s)" and
"install" variants so phrases like "codex install", "opencode hooks", and
"install cline hooks" map to PlatformHooks; locate references to PlatformHooks
and the Quick Reference entries in SKILL.md and expand the routing keywords to
include these explicit variants for each platform.
In `@skill/Workflows/PlatformHooks.md`:
- Around line 11-26: Add a structured flags table under the "Install hooks for a
platform" section in PlatformHooks.md next to the install command example (the
"selftune <platform> install [--dry-run] [--uninstall]" block); create a
2-column markdown table listing `--dry-run` (Preview what would be installed
without writing) and `--uninstall` (Remove selftune hooks from the platform) so
the inline flags are documented in a clear, consistent format under the same
heading.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a8c8fd9-a568-41fe-81c9-eb66feabb257
📒 Files selected for processing (21)
AGENTS.mdARCHITECTURE.mdREADME.mdcli/selftune/adapters/cline/hook.tscli/selftune/adapters/cline/install.tscli/selftune/adapters/codex/hook.tscli/selftune/adapters/codex/install.tscli/selftune/adapters/opencode/hook.tscli/selftune/adapters/opencode/install.tscli/selftune/hooks-shared/git-metadata.tscli/selftune/hooks-shared/hook-output.tscli/selftune/hooks-shared/normalize.tscli/selftune/hooks-shared/session-state.tscli/selftune/hooks-shared/skill-paths.tscli/selftune/hooks-shared/stdin-dispatch.tscli/selftune/hooks-shared/types.tscli/selftune/index.tsdocs/exec-plans/deferred/universal-hooks-multi-agent.mdpackage.jsonskill/SKILL.mdskill/Workflows/PlatformHooks.md
- Remove false-negative-prone preview shortcut in Cline hook adapter - Fix double JSON output in Cline PostToolUse install shim - Add error handler on Cline install direct entry - Fix isSelftunHook/removeSelftunHooks typo in Codex install - Increase OpenCode hook preview window to 8192 bytes - Guard OpenCode install against malformed config and foreign hooks - Expand PlatformHooks routing keywords for symmetric coverage - Add flags table and fix markdown in PlatformHooks workflow doc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/cline/install.ts`:
- Around line 72-99: The chmodSync call is redundant because writeFileSync(...,
{ mode: 0o755 }) already sets file permissions; remove the chmodSync(hookPath,
0o755) call in the install loop (the block that creates new hooks using HOOKS,
CLINE_HOOKS_DIR, MARKER and hookScript) so creation uses writeFileSync alone and
avoid the extra filesystem call.
In `@cli/selftune/adapters/codex/install.ts`:
- Around line 276-279: Wrap the top-level direct-entry invocation (the
import.meta.main block) in a try-catch that awaits cliMain() and on error logs a
clear actionable message (including relevant error details) and exits with
process.exit(1); on success allow normal exit (0). Specifically, update the
import.meta.main branch that calls cliMain() to mirror cline/install.ts by
surrounding await cliMain() with try { ... } catch (err) { console.error("Failed
to run cliMain:", err); process.exit(1); } so parsing or early failures produce
a user-friendly error and proper exit code.
In `@cli/selftune/adapters/opencode/hook.ts`:
- Around line 94-108: The switch handling of eventName is inconsistent because
handleToolBefore currently calls outputResponse({ modified: false }) internally
while the switch explicitly calls outputResponse after handleToolAfter and
handleSessionIdle; pick one consistent pattern and apply it: either remove the
internal outputResponse call from handleToolBefore and let the switch invoke
outputResponse({ modified: false }) for the "tool.execute.before" case (update
the case to await handleToolBefore(input); then outputResponse(...)), or move
the outputResponse call into each handler (add outputResponse(...) to
handleToolAfter and handleSessionIdle and remove the switch-level calls) so that
outputResponse is always invoked by handlers (reference: handleToolBefore,
handleToolAfter, handleSessionIdle, outputResponse, and the switch over
eventName).
In `@skill/Workflows/PlatformHooks.md`:
- Around line 3-46: Add a blank line after each Markdown heading that currently
has content immediately following it to fix MD022 violations: ensure there is
one empty line after headings "## Purpose", "## When to Use", "## Commands",
"### Install hooks for a platform", "### Hook handler (called by the agent, not
the user)", "## Platform Details", "### Codex", "### OpenCode", and "### Cline"
so each heading is separated from its paragraph or list content (update the
PlatformHooks.md headings accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56d0c8e5-c20e-4221-aa25-9350690d92c9
📒 Files selected for processing (7)
cli/selftune/adapters/cline/hook.tscli/selftune/adapters/cline/install.tscli/selftune/adapters/codex/install.tscli/selftune/adapters/opencode/hook.tscli/selftune/adapters/opencode/install.tsskill/SKILL.mdskill/Workflows/PlatformHooks.md
- Remove redundant chmodSync in Cline install (writeFileSync mode suffices) - Add try/catch error handler on Codex install direct entry - Make outputResponse pattern consistent in OpenCode hook switch - Fix markdown heading spacing in PlatformHooks workflow doc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/cline/install.ts`:
- Around line 77-83: When refreshing an existing hook (checked with
existing.includes(MARKER)) the code calls writeFileSync(hookPath,
hookScript(hook.name), { mode: 0o755 }) which only applies the mode on creation;
update the file to restore the execute bit by importing and calling
chmodSync(hookPath, 0o755) immediately after writeFileSync (but only when not
dryRun), so that the hook at hookPath is executable when updated; keep the
dryRun branch unchanged.
In `@cli/selftune/adapters/codex/install.ts`:
- Around line 91-103: readHooksFile currently swallows parse/schema errors and
returns { hooks: [] }, which lets the installer overwrite a malformed but
non-empty hooks.json; change readHooksFile to only return an empty hooks list
when the file is missing or empty, but rethrow or propagate JSON parse and
schema validation errors (i.e., do not catch them silently); specifically update
the function readHooksFile(path: string): CodexHooksFile to allow JSON.parse
errors and cases where parsed.hooks is not an array to surface an error (throw
or return an Error) so the install flow (caller of readHooksFile) can surface a
repairable error instead of overwriting user hooks.
- Around line 46-47: The HOOK_COMMAND string uses unquoted parameter expansion
which allows word-splitting and argument injection; update the HOOK_COMMAND
value so the shell expansion of SELFTUNE_CLI_PATH is quoted (i.e., use
"$SELFTUNE_CLI_PATH" inside the bash -c command) so paths with spaces remain a
single argument and cannot inject extra parameters into the exec call.
In `@skill/Workflows/PlatformHooks.md`:
- Around line 40-42: Update the documentation entry that currently says "npx
selftune codex hook" to match the installer behavior in
cli/selftune/adapters/codex/install.ts: state that the installed wrapper prefers
the SELFTUNE_CLI_PATH environment variable and otherwise runs "npx -y
selftune@latest codex hook" (including the -y and `@latest`) so hooks.json
reflects the exact command used by the installer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5142f46-0503-4d0d-94ba-1e51f582f0a9
📒 Files selected for processing (4)
cli/selftune/adapters/cline/install.tscli/selftune/adapters/codex/install.tscli/selftune/adapters/opencode/hook.tsskill/Workflows/PlatformHooks.md
- Restore chmodSync on existing hook updates (writeFileSync mode only applies at creation) - Quote $SELFTUNE_CLI_PATH in Codex hook command to prevent word-splitting - Surface parse errors from malformed hooks.json instead of silently overwriting - Fix Codex install command description in PlatformHooks workflow doc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/cline/install.ts`:
- Around line 107-114: The success footer currently prints whenever installed >
0 and !dryRun, even if some hooks were skipped; change the condition that
displays the "Cline will now track commits and record session telemetry."
message to require skipped === 0 (i.e., only show when no hooks were skipped).
If skipped > 0, instead print a concise partial-install warning that
telemetry/tracking may be incomplete (mention that PostToolUse, TaskComplete, or
TaskCancel could have been skipped if you can access which hooks were skipped)
so users know the install was not fully applied; update the logic around the
skipped, dryRun, and installed variables accordingly.
- Around line 26-43: The hookScript function currently inlines a hardcoded "npx
selftune cline hook" command; instead build a hook command string that prefers a
local override (use SELFTUNE_CLI_PATH if set, otherwise fall back to the current
CLI binary path or npx), then reuse that hook command in both branches
(PostToolUse and background). Update hookScript to compute a single hookCmd
variable (used with MARKER and the existing input/capture logic) so the
generated shims honor SELFTUNE_CLI_PATH as in
cli/selftune/adapters/codex/install.ts.
In `@cli/selftune/adapters/codex/install.ts`:
- Around line 108-116: The fallback in isSelftuneHook currently treats any
entry.command containing both "selftune" and "codex hook" as owned by selftune;
narrow this by replacing the broad includes check in function isSelftuneHook
with either (a) only honoring entry._selftune true, or (b) checking
entry.command against a small explicit allowlist of exact legacy command strings
(e.g., const LEGACY_SELF_TUNE_CMDS = [ "selftune codex hook", ... ] and using
LEGACY_SELF_TUNE_CMDS.includes(entry.command)). Update isSelftuneHook to use
that exact-match allowlist and ensure mergeHooks() / removeSelftuneHooks() will
only delete entries that match _selftune === true or the exact legacy strings.
In `@skill/Workflows/PlatformHooks.md`:
- Around line 28-34: The doc currently only shows Codex install examples and
omits OpenCode/Cline install and hook usage; update the "Hook handler (called by
the agent, not the user)" workflow to include complete examples for all
documented commands: add at least one OpenCode (or Cline) install example for
the "selftune <platform> install" variant and include a usage example for
"selftune <platform> hook" (or alternatively remove "hook" from the commands
list and keep it only in the handler section); ensure the examples mirror the
Codex example structure and include the required flags table entries so the
workflow documents every command form it mentions (reference commands: "selftune
<platform> install" and "selftune <platform> hook").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 00bd6b85-40fa-48ab-993a-0ec870b87908
📒 Files selected for processing (3)
cli/selftune/adapters/cline/install.tscli/selftune/adapters/codex/install.tsskill/Workflows/PlatformHooks.md
- Cline shims now honor SELFTUNE_CLI_PATH for local/dev override - Partial install prints warning instead of false success banner - Codex isSelftuneHook uses exact legacy command allowlist, not broad includes - PlatformHooks.md adds examples for all three platforms + hook handler Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Setup workflow now detects all installed agent CLIs and asks the user which platforms to install hooks for, instead of only configuring the primary detected agent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract processAutoActivate() from auto-activate.ts for reuse, then call it from the Codex SessionStart handler. Codex now has 100% hook coverage matching Claude Code. README platforms section updated to a table with honest coverage notes per platform. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/codex/hook.ts`:
- Line 47: The Codex hook types are not forwarding permission_mode into the
shared payloads, so update the payload construction in hook.ts to include
permission_mode from the CodexHookPayload into the derived PreToolUsePayload,
PostToolUsePayload and StopPayload; locate where CodexHookPayload is mapped to
the shared payloads (references: CodexHookPayload, CommonHookPayload,
PreToolUsePayload, PostToolUsePayload, StopPayload) and add permission_mode to
the objects passed into the shared handlers so the common hook context is
preserved.
- Around line 233-235: The writes that emit JSON then immediately call
process.exit(0) (notably the calls to writeResponse(EMPTY_RESPONSE) and other
writeResponse invocations in this file) can truncate stdout when piped; change
the logic so the process only exits after the stdout write finishes by using the
callback form of process.stdout.write or by modifying writeResponse to accept a
completion callback and call process.exit(0) from that callback; update each
affected branch (the writeResponse/EMPTY_RESPONSE call at lines around 233–235
and the other writeResponse calls indicated) to perform the exit inside the
write completion callback to ensure the buffer flushes before exiting.
In `@cli/selftune/hooks/auto-activate.ts`:
- Around line 159-163: The helper processAutoActivate currently hardcodes
CLAUDE_SETTINGS_PATH as the default settingsPath which causes non-Claude callers
(like Codex) to run the Claude PAI coexistence check; remove the Claude-specific
default by changing the signature to require an explicit settingsPath (e.g.,
processAutoActivate(sessionId: string, settingsPath: string): Promise<string[]>)
or make settingsPath optional/undefined by default and treat undefined as “no
default”, and then update the Claude entrypoint to call
processAutoActivate(sessionId, CLAUDE_SETTINGS_PATH); ensure references to
checkPaiCoexistence(settingsPath) use the passed-in value and update all callers
to pass the appropriate settings path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca28a0b8-c8dd-4742-8da0-5c86180859e6
📒 Files selected for processing (3)
README.mdcli/selftune/adapters/codex/hook.tscli/selftune/hooks/auto-activate.ts
… platform-neutral auto-activate - Forward permission_mode from CodexHookPayload into all shared payloads - Replace writeResponse + process.exit with writeResponseAndExit callback to prevent stdout truncation when piped - Make processAutoActivate settingsPath optional (no default) so Codex callers don't run Claude-specific PAI coexistence check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/hooks/auto-activate.ts`:
- Around line 168-175: ActivationContext.settings_path is declared as a required
string but you pass settingsPath (string | undefined) into ctx in
processAutoActivate, so when processAutoActivate(sessionId) is called without a
settings path you assign undefined; fix by ensuring ctx.settings_path is always
a string — for a minimal change set ctx.settings_path to a safe fallback (e.g.,
settingsPath ?? ''), referencing ActivationContext and settings_path in the ctx
literal inside processAutoActivate (where settingsPath is used); alternatively,
if semantically appropriate, update the ActivationContext type to make
settings_path optional so the field can legally be undefined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 535dd27b-8a03-4b46-b6fb-20c7cae62e3b
📒 Files selected for processing (2)
cli/selftune/adapters/codex/hook.tscli/selftune/hooks/auto-activate.ts
…tivate Prevents undefined from violating the ActivationContext.settings_path required string type when called without a settings path (Codex). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wire up subagent workflows (evolution-reviewer, diagnosis-analyst, pattern-analyst, integration-guide) for OpenCode and Codex platforms. - Fix callViaAgent() opencode command from broken `-p -f -q` flags to `opencode run [--model] <prompt>` per current OpenCode CLI docs - Extend callViaSubagent() to auto-detect and dispatch to all three platforms: Claude Code (--agent), OpenCode (run --agent), Codex (agent instructions inlined into prompt) - Add OpenCode model alias resolution (haiku/sonnet/opus → provider/model) - Register selftune agents in OpenCode config during `selftune opencode install` with proper tool permissions and model mapping - Clean up agents on `selftune opencode install --uninstall` - Update README platforms table with new Eval/Optimizer Agents column Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cli/selftune/utils/llm-call.ts (1)
172-177:⚠️ Potential issue | 🟠 MajorStop round-tripping the prompt through a timestamped temp file.
The subprocess never sees
promptFile; this code reads it straight back and still passes the prompt as argv. UsingDate.now()here is not unique enough for concurrent calls, so one request can overwrite another's prompt or remove the file during a sibling call. BuildpromptContentin memory instead.Also applies to: 249-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/utils/llm-call.ts` around lines 172 - 177, The code currently writes the prompt to a timestamped temp file (promptFile using tmpdir() and Date.now()) and then immediately reads it back (readFileSync), which is unnecessary and race-prone; instead construct promptContent in memory by concatenating systemPrompt and userPrompt and use that directly for the subprocess call, removing writeFileSync/readFileSync and any tmpdir/join/Date.now() usage; apply the same change to the similar block around the other occurrence (lines referencing the second promptFile usage) so no temp files are created and concurrent calls cannot clobber each other.tests/utils/llm-call.test.ts (1)
192-220:⚠️ Potential issue | 🟡 MinorExercise the new dispatch surface, not just one argv token.
This only locks in
capturedCmd[1] === "run". The PR also added OpenCode model alias resolution and the newcallViaSubagent()backend dispatch, so a broken alias map or bundled-agent lookup would still pass this suite. Please add at least one mapped-model case and one subagent path regression test.As per coding guidelines,
tests/**/*.ts: Coverage of error paths, not just happy paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/utils/llm-call.test.ts` around lines 192 - 220, The test only validates the first two argv tokens and misses the new dispatch surface (model alias resolution and callViaSubagent path); update tests around callViaAgent to assert the full constructed command and add two cases: one where an OpenCode model alias is mapped (exercise the alias-to-bundled-agent lookup and assert the resolved argv contains the expected resolved model token and subsequent args), and one that triggers the subagent dispatch by calling callViaSubagent (assert the argv contains the subagent entry point and any forwarded args). Also add at least one negative path (e.g., unknown alias or failed subagent lookup) to ensure error handling is covered. Ensure you modify the mocked Bun.spawn capture to validate multiple argv indices, not only capturedCmd[1], and reference callViaAgent and callViaSubagent in the new tests.
♻️ Duplicate comments (2)
cli/selftune/hooks/auto-activate.ts (1)
159-165:⚠️ Potential issue | 🟠 MajorRequire explicit coexistence-check intent in
processAutoActivate.Line 161 makes
settingsPathoptional, which allows callers to silently bypass PAI coexistence checks by omission (Codex currently does this). Make the parameter explicit (string | null) so each caller intentionally opts in/out.Proposed fix
export async function processAutoActivate( sessionId: string, - settingsPath?: string, + settingsPath: string | null, ): Promise<string[]> { - // Only check PAI coexistence when a settings path is provided (platform-specific) - if (settingsPath && checkPaiCoexistence(settingsPath)) return []; + // Check coexistence when caller provides a platform settings path. + if (settingsPath !== null && checkPaiCoexistence(settingsPath)) return [];As per coding guidelines,
cli/**/*.tsrequires strict typing in CLI code; this API boundary should force explicit caller intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/hooks/auto-activate.ts` around lines 159 - 165, The function processAutoActivate currently declares settingsPath?: string which lets callers omit the argument and silently bypass the PAI coexistence check; change the signature to settingsPath: string | null and update the runtime check to explicitly test for null (e.g., if (settingsPath !== null && checkPaiCoexistence(settingsPath)) return []); then update all callers of processAutoActivate to pass either a string path or null explicitly so intent is clear. Ensure references to checkPaiCoexistence remain and adjust any tests or call sites (including where the arg was omitted) to provide null when they mean "opt out."cli/selftune/adapters/opencode/install.ts (1)
260-267:⚠️ Potential issue | 🟠 MajorDon't overwrite or delete foreign agent entries by name.
config.agentis shared user state. Install replaces any existingevolution-reviewer/etc. entry, and uninstall later deletes that key unconditionally, so user-defined or locally customized agents are lost. Namespace selftune's agent IDs or persist a marker/fingerprint and only update/remove entries that still match it.Also applies to: 316-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 260 - 267, The current loop copies agentEntries into the shared config.agent unconditionally, which can overwrite user-defined agents (e.g., evolution-reviewer) and later cause unconditional removals; modify the install logic that iterates over agentEntries to either prefix/namespace agent IDs (e.g., add a "selftune:" or similar prefix) or write a small metadata marker/fingerprint alongside each entry, then only add keys that do not already exist (or that match the fingerprint) and update existing selftune-marked entries; mirror this in the uninstall/remove code so it only deletes keys that have the selftune namespace or matching fingerprint instead of deleting by bare name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 237-244: The code currently writes the shim (buildShimContent(),
writeFileSync(shimPath)) before validating the opencode config, so a thrown
readConfig(configPath) leaves the shim on disk; change the order to read and
validate the config first by calling readConfig(configPath) (and any validation
logic) before creating shimDir or calling writeFileSync(shimPath,
buildShimContent(), ...), and only create/write the shim after readConfig
returns successfully; keep existing shimDir/mkdirSync and file mode logic but
move them after the successful readConfig call so failed config parsing does not
leave artifacts.
- Around line 40-45: The generated shim in buildShimContent embeds a call to
`npx selftune opencode hook`, which breaks Bun-only environments; update
buildShimContent to avoid hardcoding `npx` by either using `bunx` when Bun is
available or resolving the installed `selftune` executable path at build time
and embedding that path, keeping the SELFTUNE_TAG header intact; specifically
modify the string returned by buildShimContent so the hook invocation uses the
resolved binary command (or `bunx selftune opencode hook`) instead of `npx
selftune opencode hook`.
In `@cli/selftune/utils/llm-call.ts`:
- Around line 304-343: detectAgent() can pick a backend (codex/opencode) that
doesn't support subagent options (allowedTools, appendSystemPrompt, maxTurns);
update llm-call.ts to fail closed when those options are present for an
unsupported agent: inside the opencode and codex branches (and before
constructing cmd), check for presence of allowedTools, appendSystemPrompt,
maxTurns (and any other subagent-only flags like --dangerously-skip-permissions
semantics), and throw a clear Error if they are set, or alternatively require an
explicit backend override flag from the caller before proceeding; reference
detectAgent(), agent === "opencode", agent === "codex", allowedTools,
appendSystemPrompt, and maxTurns to locate where to add these guards.
- Around line 55-67: BUNDLED_AGENT_DIR is resolved one level too shallow so
loadAgentInstructions() looks in cli/skill/agents instead of
repo-root/skill/agents; update the path resolution for the BUNDLED_AGENT_DIR
constant (the resolve(...) call that defines BUNDLED_AGENT_DIR) to add one more
parent directory segment so it resolves from the repository root, ensuring
loadAgentInstructions(agentName) can find bundled markdown agent files.
In `@README.md`:
- Around line 166-170: The OpenCode config path in the platform table is
incorrect; update the OpenCode row (the cell under "Config Location") to show
both possible locations used by selftune opencode install — prefer a local
./opencode.json if present, otherwise ~/.config/opencode/config.json — and
ensure the "Real-time Hooks" or install command text still references `selftune
opencode install` so readers can correlate the installer behavior with the two
config locations.
---
Outside diff comments:
In `@cli/selftune/utils/llm-call.ts`:
- Around line 172-177: The code currently writes the prompt to a timestamped
temp file (promptFile using tmpdir() and Date.now()) and then immediately reads
it back (readFileSync), which is unnecessary and race-prone; instead construct
promptContent in memory by concatenating systemPrompt and userPrompt and use
that directly for the subprocess call, removing writeFileSync/readFileSync and
any tmpdir/join/Date.now() usage; apply the same change to the similar block
around the other occurrence (lines referencing the second promptFile usage) so
no temp files are created and concurrent calls cannot clobber each other.
In `@tests/utils/llm-call.test.ts`:
- Around line 192-220: The test only validates the first two argv tokens and
misses the new dispatch surface (model alias resolution and callViaSubagent
path); update tests around callViaAgent to assert the full constructed command
and add two cases: one where an OpenCode model alias is mapped (exercise the
alias-to-bundled-agent lookup and assert the resolved argv contains the expected
resolved model token and subsequent args), and one that triggers the subagent
dispatch by calling callViaSubagent (assert the argv contains the subagent entry
point and any forwarded args). Also add at least one negative path (e.g.,
unknown alias or failed subagent lookup) to ensure error handling is covered.
Ensure you modify the mocked Bun.spawn capture to validate multiple argv
indices, not only capturedCmd[1], and reference callViaAgent and callViaSubagent
in the new tests.
---
Duplicate comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 260-267: The current loop copies agentEntries into the shared
config.agent unconditionally, which can overwrite user-defined agents (e.g.,
evolution-reviewer) and later cause unconditional removals; modify the install
logic that iterates over agentEntries to either prefix/namespace agent IDs
(e.g., add a "selftune:" or similar prefix) or write a small metadata
marker/fingerprint alongside each entry, then only add keys that do not already
exist (or that match the fingerprint) and update existing selftune-marked
entries; mirror this in the uninstall/remove code so it only deletes keys that
have the selftune namespace or matching fingerprint instead of deleting by bare
name.
In `@cli/selftune/hooks/auto-activate.ts`:
- Around line 159-165: The function processAutoActivate currently declares
settingsPath?: string which lets callers omit the argument and silently bypass
the PAI coexistence check; change the signature to settingsPath: string | null
and update the runtime check to explicitly test for null (e.g., if (settingsPath
!== null && checkPaiCoexistence(settingsPath)) return []); then update all
callers of processAutoActivate to pass either a string path or null explicitly
so intent is clear. Ensure references to checkPaiCoexistence remain and adjust
any tests or call sites (including where the arg was omitted) to provide null
when they mean "opt out."
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 924ea74e-eced-4deb-b482-a93bd1d6df6f
📒 Files selected for processing (5)
README.mdcli/selftune/adapters/opencode/install.tscli/selftune/hooks/auto-activate.tscli/selftune/utils/llm-call.tstests/utils/llm-call.test.ts
Comment on lines
+304
to
343
| const agent = detectAgent(); | ||
| if (!agent || (agent !== "claude" && agent !== "opencode" && agent !== "codex")) { | ||
| throw new Error( | ||
| `Subagent calls require 'claude', 'opencode', or 'codex' CLI in PATH (detected: ${agent ?? "none"})`, | ||
| ); | ||
| } | ||
| if (allowedTools && allowedTools.length > 0) { | ||
| cmd.push("--allowedTools", ...allowedTools); | ||
|
|
||
| let cmd: string[]; | ||
|
|
||
| if (agent === "opencode") { | ||
| cmd = ["opencode", "run", "--agent", agentName]; | ||
| if (modelFlag) { | ||
| cmd.push("--model", resolveOpenCodeModel(modelFlag)); | ||
| } | ||
| cmd.push(prompt); | ||
| } else if (agent === "codex") { | ||
| // Codex has no --agent flag; inline the agent instructions into the prompt | ||
| const agentInstructions = loadAgentInstructions(agentName); | ||
| const fullPrompt = agentInstructions ? `${agentInstructions}\n\n---\n\n${prompt}` : prompt; | ||
| cmd = ["codex", "exec", "--skip-git-repo-check", fullPrompt]; | ||
| } else { | ||
| // Claude Code | ||
| cmd = ["claude", "-p", prompt, "--agent", agentName, "--max-turns", String(maxTurns)]; | ||
|
|
||
| if (appendSystemPrompt) { | ||
| cmd.push("--append-system-prompt", appendSystemPrompt); | ||
| } | ||
| if (modelFlag) { | ||
| const resolved = resolveModelFlag(modelFlag); | ||
| cmd.push("--model", resolved); | ||
| } | ||
| if (effort) { | ||
| cmd.push("--effort", effort); | ||
| } | ||
| if (allowedTools && allowedTools.length > 0) { | ||
| cmd.push("--allowedTools", ...allowedTools); | ||
| } | ||
| // Skip permissions since this runs non-interactively in a pipeline | ||
| cmd.push("--dangerously-skip-permissions"); | ||
| } |
There was a problem hiding this comment.
Fail closed when the detected backend cannot honor subagent options.
detectAgent() prefers codex over opencode, and both fallback branches ignore allowedTools, appendSystemPrompt, and maxTurns. The provided caller in cli/selftune/evolution/evolve-body.ts:547-552 relies on allowedTools for the reviewer path, so this changes behavior silently once Claude is absent. Either let callers pick the backend explicitly or throw when unsupported options are present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/utils/llm-call.ts` around lines 304 - 343, detectAgent() can
pick a backend (codex/opencode) that doesn't support subagent options
(allowedTools, appendSystemPrompt, maxTurns); update llm-call.ts to fail closed
when those options are present for an unsupported agent: inside the opencode and
codex branches (and before constructing cmd), check for presence of
allowedTools, appendSystemPrompt, maxTurns (and any other subagent-only flags
like --dangerously-skip-permissions semantics), and throw a clear Error if they
are set, or alternatively require an explicit backend override flag from the
caller before proceeding; reference detectAgent(), agent === "opencode", agent
=== "codex", allowedTools, appendSystemPrompt, and maxTurns to locate where to
add these guards.
- Replace hardcoded `npx` in OpenCode shim with SELFTUNE_CLI_PATH → bunx → npx fallback chain (matches Codex adapter pattern) - Move readConfig() before shim write so invalid config doesn't leave orphaned shim files on disk - Fix BUNDLED_AGENT_DIR path in llm-call.ts (was one `..` short, resolving to cli/skill/agents instead of repo root) - Add warnings when unsupported subagent options (allowedTools, appendSystemPrompt) are silently ignored on codex/opencode - Fix OpenCode config location in README to show both ./opencode.json and ~/.config/opencode/config.json Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Main added `eval family-overlap` command row; branch replaced platform paragraphs with a table. Kept both: the new eval row merged cleanly, and the branch's platform table supersedes main's per-platform paragraphs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
cli/selftune/utils/llm-call.ts (1)
304-335:⚠️ Potential issue | 🟠 MajorFail closed when the chosen backend can't enforce subagent constraints.
detectAgent()can still chooseopencodeorcodex, and these branches continue after droppingallowedTools,appendSystemPrompt, and the unsupported turn/effort controls.cli/selftune/evolution/evolve-body.ts:545-552passesallowedToolsspecifically to sandbox the reviewer, so warning-and-continue weakens that safety boundary. Throw here unless the caller explicitly opted into that degradation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/utils/llm-call.ts` around lines 304 - 335, The code currently warns and continues when opencode or codex backends can't enforce subagent constraints (dropping allowedTools, appendSystemPrompt, maxTurns, effort), which weakens sandboxing; change the opencode and codex branches in the function that calls detectAgent() to throw an Error if unsupported options are present (e.g., allowedTools?.length, appendSystemPrompt, maxTurns, effort) unless the caller explicitly passed an opt-in flag (add a boolean parameter like allowDegradedSubagent or use an existing override) — update checks around agent === "opencode" and agent === "codex" (references: detectAgent, agentName, allowedTools, appendSystemPrompt, modelFlag, resolveOpenCodeModel, loadAgentInstructions) to enforce failing closed by throwing a descriptive Error when unsupported features would be dropped and the opt-in is not set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 60-72: The Change Propagation Map / Mandatory Rules must be
updated so hook-doc propagation covers the new layout: scan and include hooks
under hooks-shared/ (e.g., normalize.ts, hook-output.ts, stdin-dispatch.ts) and
adapters/*/hook.ts (per-adapter hook handlers) in addition to the old hooks/*.ts
pattern; update the propagation rules and any regex/patterns that detect hook
files so that changes to hooks-shared/* or adapters/*/hook.ts trigger updates to
skill/Workflows/Initialize.md (hook table) and skill/settings_snippet.json, and
add a note in the map indicating that behavioral changes in hooks-shared/* and
adapters/*/hook.ts require corresponding edits to Initialize.md and
settings_snippet.json.
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 101-105: parseFlags currently only picks known flags and lets
everything else through, so implement strict validation and a real --help path:
update parseFlags (and any other parsing usage at the other location) to first
detect a "--help" and return a special signal (or throw/exit) so the caller
prints usage and returns before any install logic runs; also validate args
against an allowed set ["--dry-run","--uninstall","--help"] and if any unknown
flag is present, print a usage/error and exit non-zero without touching the
filesystem; ensure callers of parseFlags check the help/validation result before
performing install/uninstall side-effects.
- Around line 299-307: The code currently unlinks the shim (shimPath) before
validating or updating opencode.json (readConfig/configPath), risking a
half-uninstalled state if readConfig throws; reorder and guard the operations so
you first check existsSync(configPath), call readConfig(configPath) and
validate/update the hook/agent entries (and write the cleaned config back) and
only after successful config manipulation remove the shim via
unlinkSync(shimPath); ensure readConfig errors are caught/propagated and that
you do not delete the shim unless config updates complete successfully.
- Around line 262-269: The install/uninstall logic currently writes and removes
agents by raw key names (agentEntries -> config.agent[name]) which can clobber
user-managed agents; change it to only mutate agents that are provably
selftune-managed by either (A) namespacing the keys you write (e.g., prefix keys
with "selftune/" when storing into config.agent) or (B) embedding a marker in
the agent entry (e.g., set entry.managedBy = "selftune") and on install only
overwrite if the target key is absent or already has that same marker, and on
uninstall only remove keys that have that marker; apply the same guarded logic
to both the install loop (the agentEntries -> config.agent assignment) and the
uninstall loop (the removal at the referenced lines 318-323), using the symbols
agentEntries and config.agent to locate the code paths to change.
In `@cli/selftune/index.ts`:
- Around line 94-99: The fast-path commands still trigger analytics because the
command_run tracking imports ./analytics.js before the FAST_COMMANDS guard;
update the start-up flow so analytics (and any import of ./analytics.js or
execution of command_run) is only loaded/executed after checking FAST_COMMANDS
and the --help flags (i.e., reuse the same guard used for autoUpdate), by moving
the import/command_run call behind the if (command &&
!FAST_COMMANDS.has(command) && command !== "--help" && command !== "-h") block
or wrapping the analytics import/command_run in the same conditional so
hook/codex/opencode/cline events do not load analytics.js.
In `@README.md`:
- Around line 171-176: Update the platform support matrix so only "Claude Code"
remains labeled "Full" and mark "Codex", "OpenCode", and the "Cline"/"OpenClaw"
adapters as "Experimental" (or similar) to avoid overpromising; edit the table
rows for the platforms ("Claude Code", "Codex", "OpenCode", "Cline"/"OpenClaw")
accordingly and optionally add a short note that all adapters write to the same
shared log schema to clarify compatibility.
In `@skill/Workflows/Initialize.md`:
- Around line 151-178: The workflow is hardcoded to list Codex/OpenCode/Cline
and leaves the fallback ambiguous; update the AskUserQuestion branch to
dynamically enumerate only the detected platforms (use the same detection array
that drives the UI), present the three options (Yes — install hooks for all
detected, Let me pick — show checkboxes generated from the detectedPlatforms
list, No — skip), and for each selected platform run the platform-specific
installer command (invoke selftune <platform> install for each selected
platform, supporting --dry-run when the user requests a preview and writing the
same hooks.json / shell shim / hook scripts as described in
Workflows/PlatformHooks.md). Also replace the vague “Batch ingest…” line with an
explicit fallback command per platform (e.g., document the concrete batch-ingest
command pattern such as selftune <platform> ingest-batch or the actual batch
command used by your codebase) so the document names the exact command to run
when real-time hooks aren’t available.
---
Duplicate comments:
In `@cli/selftune/utils/llm-call.ts`:
- Around line 304-335: The code currently warns and continues when opencode or
codex backends can't enforce subagent constraints (dropping allowedTools,
appendSystemPrompt, maxTurns, effort), which weakens sandboxing; change the
opencode and codex branches in the function that calls detectAgent() to throw an
Error if unsupported options are present (e.g., allowedTools?.length,
appendSystemPrompt, maxTurns, effort) unless the caller explicitly passed an
opt-in flag (add a boolean parameter like allowDegradedSubagent or use an
existing override) — update checks around agent === "opencode" and agent ===
"codex" (references: detectAgent, agentName, allowedTools, appendSystemPrompt,
modelFlag, resolveOpenCodeModel, loadAgentInstructions) to enforce failing
closed by throwing a descriptive Error when unsupported features would be
dropped and the opt-in is not set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 880e199f-cd92-475e-b9b4-6c401dbdd50f
📒 Files selected for processing (8)
AGENTS.mdREADME.mdcli/selftune/adapters/opencode/install.tscli/selftune/index.tscli/selftune/utils/llm-call.tspackage.jsonskill/SKILL.mdskill/Workflows/Initialize.md
…hooks # Conflicts: # package.json
- Extend hook-doc propagation rules to cover hooks-shared/ and adapters/*/hook.ts in AGENTS.md change propagation map - Add --help and strict flag validation to opencode install - Add _selftune marker to agent entries; guard install/uninstall to not clobber user-managed agents with the same name - Reorder uninstall to validate config before removing shim (avoids half-uninstalled state if readConfig throws) - Skip analytics import for fast-path hook commands (codex/opencode/cline) - Mark Codex/OpenCode/Cline/OpenClaw as Experimental in README table - Make Initialize.md multi-platform prompt dynamic (list only detected platforms) and add explicit batch ingest fallback commands Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 70-73: detectConfigPath() currently prefers a repo-local
PROJECT_CONFIG which can cause uninstall to operate on the wrong config when run
from a different cwd; change the uninstall logic so it does not rediscover the
target from CWD. Specifically, stop relying on detectConfigPath() during
uninstall and instead (a) persist the original chosen install target at install
time (e.g., record whether the shim was written to USER_CONFIG or
PROJECT_CONFIG) or (b) during uninstall, inspect both known locations
(PROJECT_CONFIG and USER_CONFIG) and only remove entries that reference the
exact colocated shim path. Update the uninstall routine (the function that calls
detectConfigPath() for teardown) to use the persisted target or the two-location
check and only modify the config that contains the matching shim entry.
- Around line 249-312: Dry-run currently returns before reading/parsing the real
config and before checking for existing hooks/agents, and the final summary
prints the desired actions rather than what was actually installed/skipped;
change install flow so readConfig(configPath) is called even when options.dryRun
is true (or otherwise load/validate the actual config) and then compute concrete
installed/skipped sets while iterating HOOK_EVENTS and agentEntries (use the
same conflict checks around config.hooks[event] and config.agent[name] /
existing._selftune as in the current loops), populate arrays like
installedHooks/skippedHooks and installedAgents/skippedAgents, and use those
arrays to (a) perform writes only when !options.dryRun and
writeConfig(configPath, config) as before, and (b) always print a summary
derived from the installed/skipped sets rather than assuming everything
succeeded; keep references to shimPath, buildShimContent(), writeFileSync,
writeConfig, HOOK_EVENTS, agentEntries, config.hooks and config.agent so
reviewers can find the exact change points.
- Around line 75-84: readConfig currently only checks JSON.parse success but
accepts invalid shapes (e.g. null, arrays, or fields with wrong types); update
the readConfig function to validate the parsed root is a plain non-null object
(typeof === "object" && !Array.isArray), and ensure parsed.hooks and
parsed.agent, if present, are also plain objects (not arrays/null/primitives).
If any validation fails, throw the same config-invalid Error mentioning
configPath so callers don't try to mutate or stringify malformed shapes;
reference the readConfig function and the OpenCodeConfig shape when adding these
guards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 465699fd-e08d-4610-b1ab-d5517f4528c2
📒 Files selected for processing (7)
AGENTS.mdARCHITECTURE.mdREADME.mdcli/selftune/adapters/opencode/install.tscli/selftune/index.tspackage.jsonskill/Workflows/Initialize.md
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cli/selftune/adapters/opencode/install.ts (2)
356-363:⚠️ Potential issue | 🟠 MajorUse the recorded install target exclusively when it exists.
When
selftune-install-target.jsonis present, uninstall should be scoped to that recorded config. The current candidate list still includes the cwd project config and the user config, so one uninstall can tear down a second selftune installation or fail on an unrelated malformedopencode.json.🛠️ Proposed fix
function getCandidateUninstallConfigPaths(): string[] { const recordedConfig = readInstallState()?.configPath; - const candidates = [recordedConfig, getProjectConfigPath(), getUserConfigPath()].filter( - (value): value is string => typeof value === "string" && value.length > 0, - ); + if (recordedConfig) { + return [recordedConfig]; + } + + const candidates = [getProjectConfigPath(), getUserConfigPath()].filter( + (value): value is string => typeof value === "string" && value.length > 0, + ); return [...new Set(candidates)]; }Also applies to: 469-485
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 356 - 363, If readInstallState() returns a recordedConfig (i.e., selftune-install-target.json exists), restrict uninstalls to that single path: update getCandidateUninstallConfigPaths to return only recordedConfig when it's a non-empty string instead of including getProjectConfigPath() and getUserConfigPath(); apply the same change to the other identical logic block around the 469-485 region so recorded install target is used exclusively for uninstall scope.
230-240:⚠️ Potential issue | 🟠 MajorReject unexpected positional arguments too.
This only rejects unknown
--prefixed tokens, soselftune opencode install uninstallorselftune opencode install foostill performs a real install. This subcommand has no positional arguments; anything outsideKNOWN_FLAGSshould fail before touching disk.🛠️ Proposed fix
- const unknown = args.filter((a) => a.startsWith("-") && !KNOWN_FLAGS.has(a)); + const unknown = args.filter((a) => !KNOWN_FLAGS.has(a)); if (unknown.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 230 - 240, The code only rejects unknown dash-prefixed flags; also reject unexpected positional arguments by checking any arg that does not start with "-" and is not an allowed positional (this subcommand allows none). Update the existing validation near the unknown variable (which uses args and KNOWN_FLAGS) to also compute e.g. unexpectedPositional = args.filter(a => !a.startsWith("-")) and if unexpectedPositional.length > 0 then print an error and exit; keep the existing unknown check and then return the options object (dryRun/uninstall) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 429-446: The shim file is created whenever shimChanged is true
even if no hooks reference it; change the logic so the shim
(writeFileSync(shimPath, shimContent, ...)) is only created when at least one
hook uses it (e.g., installedHooks.length + unchangedHooks.length > 0) in
addition to shimChanged, and likewise adjust the uninstall/remove path to only
unlink the shim when no hooks remain referencing it (check the same
installed/unchanged hooks counts before unlinking); update the conditions around
shimDir/shimPath handling to gate creation and removal by hook usage rather than
agent installation state.
---
Duplicate comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 356-363: If readInstallState() returns a recordedConfig (i.e.,
selftune-install-target.json exists), restrict uninstalls to that single path:
update getCandidateUninstallConfigPaths to return only recordedConfig when it's
a non-empty string instead of including getProjectConfigPath() and
getUserConfigPath(); apply the same change to the other identical logic block
around the 469-485 region so recorded install target is used exclusively for
uninstall scope.
- Around line 230-240: The code only rejects unknown dash-prefixed flags; also
reject unexpected positional arguments by checking any arg that does not start
with "-" and is not an allowed positional (this subcommand allows none). Update
the existing validation near the unknown variable (which uses args and
KNOWN_FLAGS) to also compute e.g. unexpectedPositional = args.filter(a =>
!a.startsWith("-")) and if unexpectedPositional.length > 0 then print an error
and exit; keep the existing unknown check and then return the options object
(dryRun/uninstall) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c8a34ba7-aa2a-495f-bec5-f46ba7f64c51
📒 Files selected for processing (2)
cli/selftune/adapters/opencode/install.tstests/init/opencode-install.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 413-416: The equality check using JSON.stringify(existing) ===
JSON.stringify(entry) is ordering-sensitive; replace it with a proper structural
comparison (e.g., use a deep-equal utility like lodash/isEqual or implement a
canonicalization that sorts object keys before stringify) so semantically
identical agent objects aren't treated as different; update the condition in the
block that references existing, entry, unchangedAgents, and name to use the deep
equality function instead of JSON.stringify.
- Around line 287-312: The current parseFrontmatter function naively splits
lines on ":" and fails for quoted values, multiline strings, or nested YAML;
replace this manual parser by using a proper YAML parser (e.g., js-yaml) inside
parseFrontmatter: import and call safeLoad/safeLoadAll or load on the matched
frontmatter block, catch parse errors and return null on failure, then map the
resulting object to the AgentFrontmatter shape (ensure tools and disallowedTools
are normalized to string[] if present). Reference parseFrontmatter, match, and
the returned fields (name, description, tools, disallowedTools, model) when
implementing the change.
- Around line 519-522: The shimRemoved field currently uses shimExists but
should reflect whether the shim was actually unlinked; update the code that
builds the result object (where removedHooks, removedAgents, shimRemoved are
returned) to set shimRemoved to a boolean that indicates actual removal (e.g.,
use removedHooks.length > 0 or the unlinkShim/unlink result) instead of
shimExists; reference the removedHooks array and the shimExists variable (or the
unlinkShim call) in the assignment for shimRemoved so it accurately reports when
the shim was deleted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70b185b6-dd31-4d9f-a758-071a01afc319
📒 Files selected for processing (2)
cli/selftune/adapters/opencode/install.tstests/init/opencode-install.test.ts
Comment on lines
+287
to
+312
| /** Parse YAML-like frontmatter from agent markdown files. */ | ||
| function parseFrontmatter(content: string): AgentFrontmatter | null { | ||
| const match = content.match(/^---\n([\s\S]*?)\n---/); | ||
| if (!match) return null; | ||
|
|
||
| const fm: Record<string, string> = {}; | ||
| for (const line of match[1].split("\n")) { | ||
| const colonIdx = line.indexOf(":"); | ||
| if (colonIdx === -1) continue; | ||
| const key = line.slice(0, colonIdx).trim(); | ||
| const value = line.slice(colonIdx + 1).trim(); | ||
| fm[key] = value; | ||
| } | ||
|
|
||
| if (!fm.name) return null; | ||
|
|
||
| return { | ||
| name: fm.name, | ||
| description: fm.description, | ||
| tools: fm.tools ? fm.tools.split(",").map((t) => t.trim()) : undefined, | ||
| disallowedTools: fm.disallowedTools | ||
| ? fm.disallowedTools.split(",").map((t) => t.trim()) | ||
| : undefined, | ||
| model: fm.model, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Frontmatter parsing is naive but functional.
The YAML parsing at lines 293-298 uses simple colon-split which won't handle quoted values, multi-line strings, or nested structures. This is acceptable for the current simple frontmatter schema but may need enhancement if agent definitions become more complex.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/adapters/opencode/install.ts` around lines 287 - 312, The
current parseFrontmatter function naively splits lines on ":" and fails for
quoted values, multiline strings, or nested YAML; replace this manual parser by
using a proper YAML parser (e.g., js-yaml) inside parseFrontmatter: import and
call safeLoad/safeLoadAll or load on the matched frontmatter block, catch parse
errors and return null on failure, then map the resulting object to the
AgentFrontmatter shape (ensure tools and disallowedTools are normalized to
string[] if present). Reference parseFrontmatter, match, and the returned fields
(name, description, tools, disallowedTools, model) when implementing the change.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 490-497: The uninstall loop in uninstallHooks() currently reads
and mutates targets as it iterates (using getCandidateUninstallConfigPaths(),
readConfig(), getShimPath(), installState, etc.), which can leave partial state
if a later readConfig() fails; change the logic to a two-pass approach: first
iterate all candidate paths and load/validate configs (collect parsed config
objects, shim paths, flags, and intended removals) without making filesystem
changes, failing early if any readConfig()/validation throws; then in a second
pass apply deletions (remove shims, update
HOOK_EVENTS/removedHooks/removedAgents, and rewrite config files) and only clear
or update installState after all deletions succeed; apply the same two-pass
refactor to the similar block around the 531-536 region.
- Around line 563-566: The log should report shim ownership instead of using
result.shimChanged; update the messages that reference result.shimChanged (e.g.,
the two console.log calls shown and the similar lines at 636-637) to use the
hook ownership signal returned by the install process (e.g., result.ownsShim or
result.hookManaged) so that prints reflect whether selftune actually manages the
shim rather than whether content differs; keep options.dryRun logic but swap the
condition to check the ownership flag and still include result.shimPath for
context, and ensure messaging for installHooks() and uninstall flows uses the
same ownership flag.
- Around line 505-507: The early continue is skipping uninstall when
install-state is missing even though entries marked with _selftune prove
ownership; change the guard so we only continue when there are no hook removals
AND no install-state AND none of the target entries are selftune-owned.
Concretely, replace the condition using viaInstallState and removedHooks with a
check like: if (removedHooks.length === 0 && !viaInstallState && !entries.some(e
=> e._selftune)) continue; and apply the same pattern to the similar block
around the 518-527 area so agent-only entries with _selftune are still processed
even if install-state is missing or corrupted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 82ef357f-b294-402b-8a80-0132dcd3e11b
📒 Files selected for processing (2)
cli/selftune/adapters/opencode/install.tstests/init/opencode-install.test.ts
Comment on lines
+490
to
+497
| for (const configPath of getCandidateUninstallConfigPaths()) { | ||
| const viaInstallState = installState?.configPath === configPath; | ||
| const shimPath = getShimPath(configPath); | ||
| const configExists = existsSync(configPath); | ||
| const shimExists = existsSync(shimPath); | ||
| const removedHooks: (typeof HOOK_EVENTS)[number][] = []; | ||
| const removedAgents: string[] = []; | ||
| let config = configExists ? readConfig(configPath) : null; |
There was a problem hiding this comment.
Preflight every uninstall candidate before the first write.
uninstallHooks() mutates targets as it iterates them. If a later candidate opencode.json is malformed, readConfig() throws after an earlier target has already been rewritten and its shim may already be gone, but the install-state file is never cleared. Load/validate all existing candidates first, then apply deletions in a second pass so uninstall is all-or-nothing across the scanned locations.
Also applies to: 531-536
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/adapters/opencode/install.ts` around lines 490 - 497, The
uninstall loop in uninstallHooks() currently reads and mutates targets as it
iterates (using getCandidateUninstallConfigPaths(), readConfig(), getShimPath(),
installState, etc.), which can leave partial state if a later readConfig()
fails; change the logic to a two-pass approach: first iterate all candidate
paths and load/validate configs (collect parsed config objects, shim paths,
flags, and intended removals) without making filesystem changes, failing early
if any readConfig()/validation throws; then in a second pass apply deletions
(remove shims, update HOOK_EVENTS/removedHooks/removedAgents, and rewrite config
files) and only clear or update installState after all deletions succeed; apply
the same two-pass refactor to the similar block around the 531-536 region.
Comment on lines
+563
to
+566
| console.log(`[selftune] OpenCode install target: ${result.configPath}`); | ||
| console.log( | ||
| `[selftune] Shim ${options.dryRun ? (result.shimChanged ? "would be written" : "already current") : result.shimChanged ? "written" : "already current"}: ${result.shimPath}`, | ||
| ); |
There was a problem hiding this comment.
Report shim status from actual hook ownership, not from change flags.
shimChanged only means the generated shim content differs, so agent-only installs can print “written”/“would be written” even though installHooks() never creates a shim. Likewise agent-only uninstall can print “not present” while preserving an existing user shim. These messages need a separate “selftune manages a hook here” / “shim still exists” signal.
Also applies to: 636-637
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/selftune/adapters/opencode/install.ts` around lines 563 - 566, The log
should report shim ownership instead of using result.shimChanged; update the
messages that reference result.shimChanged (e.g., the two console.log calls
shown and the similar lines at 636-637) to use the hook ownership signal
returned by the install process (e.g., result.ownsShim or result.hookManaged) so
that prints reflect whether selftune actually manages the shim rather than
whether content differs; keep options.dryRun logic but swap the condition to
check the ownership flag and still include result.shimPath for context, and
ensure messaging for installHooks() and uninstall flows uses the same ownership
flag.
Collaborator
Author
|
Keeping this open for now. Development has moved to the private monorepo with a one-way export workflow. This feature will be imported there when ready. |
… config key OpenCode v1.3.15+ uses a strict config schema that rejects unknown keys like `hooks` and `_selftune`. The old installer wrote these directly into config.json, breaking OpenCode startup with "Unrecognized key: hooks". The fix: - Hooks are now a TypeScript plugin file (selftune-opencode-plugin.ts) registered via the `plugin` array, matching OpenCode's actual API - Agents stay in the `agent` config key (which IS valid) but use a `[selftune]` description prefix for identification instead of the rejected `_selftune` custom field - Install/uninstall handle the plugin file lifecycle correctly - Legacy shim (selftune-opencode-hook.sh) is cleaned up on uninstall - Tests updated for the new plugin-based approach Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
cli/selftune/adapters/opencode/install.ts (3)
149-155:⚠️ Potential issue | 🟠 MajorValidate
plugin/agentfield shapes before mutating config.
readConfig()only validates the root object. Ifpluginis non-array oragentis non-object, install/uninstall can throw or silently corrupt config writes. This regression reopens a previously addressed class of failures.🔧 Suggested guard
function readConfig(configPath: string): OpenCodeConfig { @@ if (!isPlainRecord(parsed)) { @@ } + + if (parsed.plugin !== undefined && !Array.isArray(parsed.plugin)) { + throw new Error( + `OpenCode config at ${configPath} has non-array "plugin"; refusing to overwrite it.`, + ); + } + if (parsed.agent !== undefined && !isPlainRecord(parsed.agent)) { + throw new Error( + `OpenCode config at ${configPath} has non-object "agent"; refusing to overwrite it.`, + ); + } return parsed as OpenCodeConfig; }Also applies to: 362-366, 371-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 149 - 155, readConfig() currently only asserts the root is a plain object (OpenCodeConfig) but doesn't validate shapes of nested fields, so ensure you validate that parsed.plugin is either undefined or an array and parsed.agent is either undefined or a plain object before returning; update readConfig() (the function that returns OpenCodeConfig) to throw a descriptive Error when parsed.plugin is not an array or parsed.agent is not an object, and apply the same guards in the install/uninstall routines that mutate the config (references: plugin, agent, install, uninstall) to prevent writes when shapes are invalid.
344-350:⚠️ Potential issue | 🟡 MinorDry-run output over-reports agent registrations.
Dry-run currently prints every discovered agent as installable, but real install skips user-managed collisions (Lines 376-379). Preview should reflect installed vs skipped outcomes.
Also applies to: 376-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 344 - 350, Dry-run currently logs every agent in agentEntries as "would register" even though the real install checks for user-managed collisions and skips some agents; update the dry-run branch (where options.dryRun is checked and console.log is used with pluginPath, configPath, pluginRelative) to mirror the actual install decision logic: iterate agentEntries and for each agent run the same collision check used in the real install (the same condition that leads to the skip in the real path), printing "would register agent '<name>'" only when it would be installed and printing "would skip agent '<name>' (user-managed collision)" or similar when the real install would skip it. Ensure you reference and reuse the same collision-detection variables/functions so dry-run output matches actual behavior.
132-136:⚠️ Potential issue | 🟠 MajorUninstall can operate on the wrong config when cwd differs from install location.
doUninstall()re-discovers target via project-first lookup. If install happened at user scope and uninstall runs from a repo containing./opencode.json, cleanup can miss the real installation.Also applies to: 397-399
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 55-63: The fallback currently returned by resolveSelftune() is a
single string ("npx -y selftune@latest") which, when interpolated into Bun's
shell template, becomes one escaped token and breaks command execution; change
resolveSelftune to return either (A) a tuple/array [command, ...args] (e.g.,
["npx","-y","selftune@latest"]) or (B) return just the executable name and
separate args, and then update the call sites that use the interpolated
selftuneBin in the Bun shell templates to use Bun.spawn/Bun.spawnSync with an
argv array or interpolate command and each arg separately so arguments are not
escaped into one token (update all usages of resolveSelftune in the opencode
hook invocation points).
In `@skill/Workflows/PlatformHooks.md`:
- Around line 70-73: Update the example comments for the OpenCode installer to
reflect the current plugin-based lifecycle: replace references to "shim" with
"plugin" and explicitly mention the TypeScript plugin file name
`selftune-opencode-plugin.ts`, and clarify that the commands manage the plugin
file plus plugin/config entries; specifically change the three example lines for
the `selftune opencode install`, `--dry-run`, and `--uninstall` cases to say
they install/preview/remove the plugin and its config entries (not a shim) and
ensure the wording matches the installer behavior managing
`selftune-opencode-plugin.ts`.
In `@tests/init/opencode-install.test.ts`:
- Line 17: Replace the non-null assertion used on the test variable
`entry.prompt!` with optional chaining `entry.prompt?` as required by tests
guidelines; specifically update the assertion in
tests/init/opencode-install.test.ts that references `entry.prompt!` (the symbol
`entry.prompt`) to use `entry.prompt?.length` so the check becomes
optional-chained while keeping the existing toBeGreaterThan(0) expectation.
---
Duplicate comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 149-155: readConfig() currently only asserts the root is a plain
object (OpenCodeConfig) but doesn't validate shapes of nested fields, so ensure
you validate that parsed.plugin is either undefined or an array and parsed.agent
is either undefined or a plain object before returning; update readConfig() (the
function that returns OpenCodeConfig) to throw a descriptive Error when
parsed.plugin is not an array or parsed.agent is not an object, and apply the
same guards in the install/uninstall routines that mutate the config
(references: plugin, agent, install, uninstall) to prevent writes when shapes
are invalid.
- Around line 344-350: Dry-run currently logs every agent in agentEntries as
"would register" even though the real install checks for user-managed collisions
and skips some agents; update the dry-run branch (where options.dryRun is
checked and console.log is used with pluginPath, configPath, pluginRelative) to
mirror the actual install decision logic: iterate agentEntries and for each
agent run the same collision check used in the real install (the same condition
that leads to the skip in the real path), printing "would register agent
'<name>'" only when it would be installed and printing "would skip agent
'<name>' (user-managed collision)" or similar when the real install would skip
it. Ensure you reference and reuse the same collision-detection
variables/functions so dry-run output matches actual behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 753add5c-c615-4e6a-b56d-79f29e9ec664
📒 Files selected for processing (3)
cli/selftune/adapters/opencode/install.tsskill/Workflows/PlatformHooks.mdtests/init/opencode-install.test.ts
- Fix Bun shell argument escaping: resolveSelftune() now returns an argv array instead of a single string. Plugin uses Bun.spawn() with explicit argv instead of shell template interpolation, which would treat "npx -y selftune@latest" as one escaped token. - Update PlatformHooks.md examples: "shim" → "plugin" terminology - Replace non-null assertion with optional chaining in test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
cli/selftune/adapters/opencode/install.ts (2)
403-449:⚠️ Potential issue | 🟠 MajorUninstall can still clean up the wrong directory.
doUninstall()rediscovers its target viadetectConfigPath(), which prefers./opencode.json, then unlinkspluginPathand the legacy shim in that directory whether or not this installation ever owned them. Install against the user config, then run uninstall inside a repo with its ownopencode.json, and this will rewrite the repo config plus delete adjacent artifacts instead of removing the original install. Persist the chosen target or scan both candidate configs and only clean up the one that actually references the selftune plugin/agents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 403 - 449, doUninstall currently rediscovers a config via detectConfigPath() and blindly removes files adjacent to that config; change it to only modify configs and delete files that actually reference the selftune plugin/agents or to use the exact install target persisted at install time. Specifically, update doUninstall to (a) load both candidate configs instead of a single detectConfigPath() result and check each config for references using isSelftunePlugin and isSelftuneAgent before calling writeConfig or deleting entries, and (b) only call unlinkSync on getPluginPath(configPath) and the legacy shim (join(dirname(configPath), "selftune-opencode-hook.sh")) for the config(s) that contained selftune references; alternatively, persist the chosen install target at install time and have doUninstall read that persisted target instead of calling detectConfigPath(). Ensure you update references to getPluginPath, isSelftunePlugin, isSelftuneAgent, writeConfig, and unlinkSync accordingly so only the owning config/artifacts are changed or removed.
144-161:⚠️ Potential issue | 🟠 MajorReject malformed
pluginandagentfields before mutation.
readConfig()only validates the root object.{ "plugin": "foo" }will blow up at Line 371 / Line 419 when.some()or.filter()runs, and{ "agent": [] }will be treated like a mutable record during install/uninstall. Fail closed on non-string-arrayplugin, non-objectagent, and non-object agent entries before any mutation happens.Suggested guard
+function isStringArray(value: unknown): value is string[] { + return Array.isArray(value) && value.every((item) => typeof item === "string"); +} + +function isAgentRecord( + value: unknown, +): value is Record<string, OpenCodeAgentConfig> { + return ( + isPlainRecord(value) && + Object.values(value).every((entry) => isPlainRecord(entry)) + ); +} + function readConfig(configPath: string): OpenCodeConfig { if (!existsSync(configPath)) return {}; let parsed: unknown; @@ if (!isPlainRecord(parsed)) { throw new Error( `OpenCode config at ${configPath} must be a JSON object; refusing to overwrite it.`, ); } - return parsed as OpenCodeConfig; + const record = parsed as Record<string, unknown>; + if (record.plugin !== undefined && !isStringArray(record.plugin)) { + throw new Error(`OpenCode config at ${configPath} has an invalid "plugin" field.`); + } + if (record.agent !== undefined && !isAgentRecord(record.agent)) { + throw new Error(`OpenCode config at ${configPath} has an invalid "agent" field.`); + } + + return record as OpenCodeConfig; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 144 - 161, readConfig currently only checks the root is a plain object but must also validate and reject malformed plugin and agent fields before any mutation: ensure that if parsed has "plugin" it is an array of strings (reject otherwise), and if parsed has "agent" it is a plain record whose values are plain objects (reject non-object or non-record entries); update readConfig (return type OpenCodeConfig) to throw a descriptive Error for invalid "plugin" or "agent" shapes so downstream code that calls install/uninstall (which uses .some()/.filter() on plugin and treats agent entries as mutable objects) never receives bad types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 41-43: Change the config filename to match OpenCode docs by
updating getUserConfigPath to return ~/.config/opencode/opencode.json (replace
"config.json"), and in the plugin install/registration code paths (the blocks
that create/write the plugin file and update the config around the other flagged
sections) write the plugin file into the documented plugin directory (prefer
~/.config/opencode/plugins or the project-local ./.opencode/plugins) instead of
next to the config file, create that plugins directory if missing, and stop
adding relative/local file paths into the config's reserved "plugin" array;
instead register the local plugin using OpenCode's documented mechanism for
local plugins (i.e., place the file under the plugins directory and update the
documented local-plugin registration entry or key), leaving the npm-package
"plugin" array entries unchanged.
In `@skill/Workflows/PlatformHooks.md`:
- Around line 23-26: The docs omit the built-in help flags and use an incorrect
example that pipes the literal '$PAYLOAD'; update the flags table to include
--help and -h (as parsed by parseFlags()), add them to the examples, and replace
any hook piping examples that use echo '$PAYLOAD' with a safe form that expands
the variable such as printf '%s\n' "$PAYLOAD" or a heredoc; make the same
corrections for the other example block referenced (around the second snippet at
lines ~87-90) so all skill/Workflows/*.md docs show the documented flags and
valid hook examples.
In `@tests/init/opencode-install.test.ts`:
- Around line 5-45: Add tests that cover the installer-path and
install/uninstall behavior (not just buildAgentEntries): create temporary
directories (use a temp-dir fixture) and exercise the install helpers or cliMain
to simulate installing agents from a given installer-path, including cases for
malformed agent configs, existing-agent conflicts, and uninstall-target
selection; assert correct filesystem outcomes and that errors are reported (use
bun:test style like the existing file), and add complementary negative tests
that call uninstallAgent/cliMain to ensure correct target resolution and safe
failure when targets are missing or ambiguous. Ensure you reference and call
buildAgentEntries where appropriate to validate packaging, and include cleanup
of temp dirs between tests.
---
Duplicate comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 403-449: doUninstall currently rediscovers a config via
detectConfigPath() and blindly removes files adjacent to that config; change it
to only modify configs and delete files that actually reference the selftune
plugin/agents or to use the exact install target persisted at install time.
Specifically, update doUninstall to (a) load both candidate configs instead of a
single detectConfigPath() result and check each config for references using
isSelftunePlugin and isSelftuneAgent before calling writeConfig or deleting
entries, and (b) only call unlinkSync on getPluginPath(configPath) and the
legacy shim (join(dirname(configPath), "selftune-opencode-hook.sh")) for the
config(s) that contained selftune references; alternatively, persist the chosen
install target at install time and have doUninstall read that persisted target
instead of calling detectConfigPath(). Ensure you update references to
getPluginPath, isSelftunePlugin, isSelftuneAgent, writeConfig, and unlinkSync
accordingly so only the owning config/artifacts are changed or removed.
- Around line 144-161: readConfig currently only checks the root is a plain
object but must also validate and reject malformed plugin and agent fields
before any mutation: ensure that if parsed has "plugin" it is an array of
strings (reject otherwise), and if parsed has "agent" it is a plain record whose
values are plain objects (reject non-object or non-record entries); update
readConfig (return type OpenCodeConfig) to throw a descriptive Error for invalid
"plugin" or "agent" shapes so downstream code that calls install/uninstall
(which uses .some()/.filter() on plugin and treats agent entries as mutable
objects) never receives bad types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 00627844-2943-4ebf-a400-cdbb87b7562c
📒 Files selected for processing (3)
cli/selftune/adapters/opencode/install.tsskill/Workflows/PlatformHooks.mdtests/init/opencode-install.test.ts
- Config path: ~/.config/opencode/opencode.json (not config.json) - Plugin: written to ~/.config/opencode/plugins/ directory (auto-discovered by OpenCode at startup) instead of the plugin array in config (which is reserved for npm packages) - Remove all plugin array manipulation — agents go in config, plugin goes in plugins dir - Uninstall cleans up legacy config.json, shim, and plugin array entries from previous installer versions - PlatformHooks.md: add --help flag, fix config path, fix hook piping examples (printf instead of echo with single quotes) - Add integration tests: install/uninstall lifecycle, user-agent conflict skip, malformed config error handling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cli/selftune/adapters/opencode/install.ts (3)
432-476:⚠️ Potential issue | 🟠 MajorDon't pick a single uninstall target from the current cwd.
doUninstall()still relies ondetectConfigPath(), which prefers./opencode.json. If selftune was installed into the user config and uninstall later runs inside a repo with a project config, this path cleans the wrong file and leaves the real[selftune]agents behind. At minimum, inspect both the project-local and user-level configs and remove managed entries from any file that exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 432 - 476, doUninstall() currently calls detectConfigPath() and only edits a single config, which can modify the wrong file; update doUninstall to inspect and update both project-local and user-level config files (where present) instead of a single detected path: for each config path found, call readConfig(), remove legacy plugin entries (using PLUGIN_FILENAME), remove selftune-managed agents (using isSelftuneAgent), track changed state and call writeConfig() if modified, and log per-path removals; keep the existing dry-run behavior but reflect both targets.
372-378:⚠️ Potential issue | 🟡 MinorReport skipped agent conflicts in the preview and final summary.
The conflict check skips user-managed agents, but both the dry-run output and the final
Registered agentsblock still print every name fromagentEntries. A partial install therefore looks identical to a full install. Track installed/skipped agent sets and print those instead.Also applies to: 394-402, 421-428
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 372 - 378, The dry-run and final summary currently iterate over agentEntries and print every agent name even when the conflict logic skips user-managed agents; change the flow to maintain two sets/arrays (e.g., installedAgents and skippedAgents) populated by the existing conflict/install logic (the code paths that decide whether to write a plugin to pluginPath or skip an agent and that reference agentEntries), then, in the options.dryRun block and in the final "Registered agents" summary, print installedAgents and skippedAgents separately (labeling skipped ones as "skipped due to conflict" or similar) instead of iterating directly over Object.keys(agentEntries); ensure the same tracking is used for the three places noted (the dry-run preview, the mid-run messages, and the final summary).
156-173:⚠️ Potential issue | 🟠 MajorReject non-object
agentfields before mutating the config.
readConfig()only validates the root object. Ifopencode.jsoncontains"agent": []or"agent": "foo", the later install/uninstall paths either throw or write string-keyed properties thatJSON.stringify()drops on save. Fail early here unlessagentis a plain object.Suggested guard
if (!isPlainRecord(parsed)) { throw new Error( `OpenCode config at ${configPath} must be a JSON object; refusing to overwrite it.`, ); } + + if (parsed.agent !== undefined && !isPlainRecord(parsed.agent)) { + throw new Error( + `OpenCode config at ${configPath} has a non-object "agent" field; refusing to overwrite it.`, + ); + } return parsed as OpenCodeConfig;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/selftune/adapters/opencode/install.ts` around lines 156 - 173, The readConfig function currently only validates the root parsed value; add a guard that checks if the parsed object has an "agent" property and, if present, verifies it's a plain object (using the existing isPlainRecord helper) and throw a clear error if it's not; update readConfig to refuse to load configs where parsed.agent is an array, string, or other non-object so downstream code in install/uninstall that mutates parsed.agent won't crash or produce invalid JSON.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/init/opencode-install.test.ts`:
- Around line 199-214: The test currently passes an async function reference to
expect(...).toThrow(), so the rejection from cliMain() is never awaited; make
the test function async, invoke the async block and assert using await
expect(invokedAsyncFn).rejects.toThrow(...). Specifically, change the test to
async, move the import and process.argv manipulation into an invoked async
function (or directly call cliMain inside the expect), and use await
expect(promise).rejects.toThrow(/not valid JSON/) referencing cliMain and
getUserConfigPath so the malformed-config path is actually awaited and its
rejection asserted.
---
Duplicate comments:
In `@cli/selftune/adapters/opencode/install.ts`:
- Around line 432-476: doUninstall() currently calls detectConfigPath() and only
edits a single config, which can modify the wrong file; update doUninstall to
inspect and update both project-local and user-level config files (where
present) instead of a single detected path: for each config path found, call
readConfig(), remove legacy plugin entries (using PLUGIN_FILENAME), remove
selftune-managed agents (using isSelftuneAgent), track changed state and call
writeConfig() if modified, and log per-path removals; keep the existing dry-run
behavior but reflect both targets.
- Around line 372-378: The dry-run and final summary currently iterate over
agentEntries and print every agent name even when the conflict logic skips
user-managed agents; change the flow to maintain two sets/arrays (e.g.,
installedAgents and skippedAgents) populated by the existing conflict/install
logic (the code paths that decide whether to write a plugin to pluginPath or
skip an agent and that reference agentEntries), then, in the options.dryRun
block and in the final "Registered agents" summary, print installedAgents and
skippedAgents separately (labeling skipped ones as "skipped due to conflict" or
similar) instead of iterating directly over Object.keys(agentEntries); ensure
the same tracking is used for the three places noted (the dry-run preview, the
mid-run messages, and the final summary).
- Around line 156-173: The readConfig function currently only validates the root
parsed value; add a guard that checks if the parsed object has an "agent"
property and, if present, verifies it's a plain object (using the existing
isPlainRecord helper) and throw a clear error if it's not; update readConfig to
refuse to load configs where parsed.agent is an array, string, or other
non-object so downstream code in install/uninstall that mutates parsed.agent
won't crash or produce invalid JSON.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f2f4371d-c63e-4430-80ef-855590625edf
📒 Files selected for processing (4)
README.mdcli/selftune/adapters/opencode/install.tsskill/Workflows/PlatformHooks.mdtests/init/opencode-install.test.ts
The previous test used expect(async () => {...}).toThrow() which never
awaits the async function, so the rejection was silently swallowed and
the test passed vacuously. Now properly invokes the async IIFE and
uses await expect(...).rejects.toThrow().
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/init/opencode-install.test.ts`:
- Around line 43-49: The test in opencode-install.test.ts uses a weak assertion
on entry.model (inside the test "agent entries use provider/model format for
model") by checking entry.model with toContain("/"); update the assertion to
validate both sides are non-empty by matching entry.model against an anchored
regex for "provider/model" form (e.g., ensure entry.model matches a pattern that
requires at least one character before and after the slash); locate
buildAgentEntries() usage and replace the toContain("/") assertion on
entry.model with a regex match that enforces non-empty provider and model parts.
- Around line 27-30: The test uses a hardcoded absolute path "/nonexistent/path"
which can exist in some environments; update the test to generate a
guaranteed-unique temp path (e.g., using os.tmpdir() + a random/Date.now()
suffix or fs.mkdtempSync pattern) and assert fs.existsSync(tempPath) is false
before calling buildAgentEntries(tempPath), then call
buildAgentEntries(tempPath) and assert Object.keys(entries).length === 0;
reference the test and the buildAgentEntries call so the change is applied to
the existing test block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d86f9c62-beaf-4853-b5d5-4aa3b0af5a10
📒 Files selected for processing (1)
tests/init/opencode-install.test.ts
- Use unique temp path instead of hardcoded /nonexistent/path - Validate provider/model format with anchored regex (non-empty both sides) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/init/opencode-install.test.ts`:
- Around line 34-41: The allowlist in the test "agent entries do not contain
non-standard keys" is too permissive because it includes "name"; update the test
to remove "name" from the validKeys Set so only
["description","mode","model","prompt","tools"] are allowed and ensure the test
still calls buildAgentEntries() and iterates over Object.values(entries) and
Object.keys(entry) as before (reference symbols: buildAgentEntries, validKeys,
the test name).
- Around line 172-199: Add a new test variant in
tests/init/opencode-install.test.ts that sets up a mixed agents config (one
user-defined agent and one selftune-tagged agent) in the file at
getUserConfigPath() prior to running the uninstall flow; call the same cliMain()
from "../../cli/selftune/adapters/opencode/install.js" to perform install then
uninstall, and after uninstall assert that existsSync(getPluginPath()) is false,
that readJson(getUserConfigPath()).agent no longer contains entries with the
selftune tag but still contains the original user-defined agent entry. Use the
existing helpers readJson/getUserConfigPath/getPluginPath and process.argv
manipulation pattern from the current test to install/uninstall and ensure the
test cleans up and restores process.argv in finally blocks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c0ff1aa6-a39b-4a1e-a5b1-f43e12d24e8f
📒 Files selected for processing (1)
tests/init/opencode-install.test.ts
- Remove 'name' from valid entry keys (it's only used as the map key, not a property on the entry object) - Add test verifying uninstall preserves user-defined agents while removing only [selftune]-prefixed entries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
hooks-shared/) with universal types, platform normalizers, session state, git metadata extraction, and skill path utilitiesadapters/codex/,adapters/opencode/,adapters/cline/) withhookandinstallsubcommands for eachselftune codex|opencode|clinecommands through the CLI with fast-path auto-update skipPlatformHooks.mdworkflow doc and updates SKILL.md routing table, README, ARCHITECTURE.md, and AGENTS.mdTest plan
selftune codex install --dry-runshows expected hooks.json changesselftune opencode install --dry-runshows expected shell shimselftune cline install --dry-runshows expected hook scriptsselftune codex install --uninstall --dry-runreverses installbun testpassesbunx oxlintclean🤖 Generated with Claude Code