feat(skillopt): wire pi + generalize skill detection + CodeQL fix#244
Merged
Conversation
…ables pi/codex) Claude invokes skills via a Skill tool; pi/codex have no Skill tool — they READ the skill's SKILL.md file. Generalize armSkillOptOnSkillUse to also arm on a 'read' tool whose path matches */skills/<ref>/SKILL.md, recovering the name--author ref from the dir segment (markSkillPending still gates org-shape + manifest, so bare/non-org dirs are rejected). Restricted to read tools so EDITING a SKILL.md doesn't arm. Makes pi wireable now, and codex wireable once its capture records the SKILL.md read. Tests: SKILL.md-read arming, non-read/non-SKILL.md rejection, skillRefFromSkillFileRead path extraction (10 hook tests).
…and-injection)
CodeQL flagged resolveAgentBin: `execSync(\`command -v ${AGENT_CMD[agent] ?? agent}\`)`
as 'indirect uncontrolled command line' — `agent` traces back to the HIVEMIND_SKILLOPT_AGENT
env var, and the `?? agent` fallback let an arbitrary value (e.g. 'x; rm -rf ~') reach a shell.
Fix: drop the fallback (resolve KNOWN agents only) and walk $PATH in Node — no shell, no
subprocess — so no command line is ever built from env-derived input. Still finds nvm/volta/fnm
installs (the reason the old `command -v` existed), since those are on PATH. Verified: a known
agent resolves to its bin, an injection payload resolves to undefined.
…n worker pi has no first-class Skill tool — it USES an org skill by READING its SKILL.md — so the pi extension now arms SkillOpt on a tool_result whose path is .../skills/<name--author>/SKILL.md and, on the next user prompt (the reaction), spawns the bundled skillopt-worker (agent=pi) to judge + improve. Logic is inlined into the raw-.ts extension (zero non-builtin deps, can't import the trigger); the HIVEMIND_SKILLOPT_* env names are the cross-process contract with the worker. Gated on org-shape, the kill switch, and the in-worker recursion guard; both call sites sit after the captureEnabled check so the worker's own pi-judge subprocess can't re-fire. - esbuild.config.mjs: build skillopt-worker into pi/bundle - install-pi.ts: install skillopt-worker.js into ~/.pi/agent/hivemind (cleanup already covers it) - pi extension: skilloptArm (tool_result) + skilloptReact (input) + detached worker spawn - tests: 5 source-level guards for the wiring (runtime-loaded → source is the bundle guard) NOTE: codex still blocked — its exec read of SKILL.md isn't captured (0.136.0 PostToolUse gap). NOTE: pi wiring needs a LIVE pi-session E2E before merge (runtime-loaded; only source-tested here).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects SKILL.md loads to arm a per-session SkillOpt window, persists pending state in the pi extension, reacts on next user input by spawning a detached skillopt-worker, wires codex/hermes/pi hooks, updates bundling/install, replaces worker shell resolution with PATH-walking, and adds tests. ChangesSkillOpt integration into pi extension
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 8 files changed
Generated for commit 576fba6. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/pi/pi-extension-source.test.ts (1)
197-204: ⚡ Quick winMake SkillOpt wiring assertions specific (not substring-based).
The current checks can pass with incorrect logic. For this block, assert the exact arming condition and path shape instead of broad substrings.
Proposed test tightening
- expect(PI_SRC).toContain("/skills/"); // the ref-from-path matcher targets …/skills/<ref>/SKILL.md - expect(PI_SRC).toContain("SKILL"); + expect(PI_SRC).toMatch(/if\s*\(\s*event\.isError\s*!==\s*true\s*\)\s*skilloptArm\(sessionId,\s*event\.input,\s*event\.toolCallId\)/); + expect(PI_SRC).toMatch(/\/skills\/\(\[\^\/\]\+\)\/SKILL\\\.md\$/);As per coding guidelines:
tests/**should prefer specific values/patterns over generic substrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/pi/pi-extension-source.test.ts` around lines 197 - 204, The test uses broad substring checks on PI_SRC which can give false positives; update the assertions to match the exact arming condition and path shape instead of loose substrings: assert that PI_SRC contains the exact call expression "skilloptArm(sessionId, event.input, event.toolCallId)" guarded by the exact condition "event.isError !== true" together in the same conditional, and assert the ref-from-path matches the precise pattern for SKILL.md under a skills folder (e.g. a regex like /\/skills\/[^\/]+\/SKILL\.md/); use exact string equality or a targeted regex match on PI_SRC rather than simple expect(...).toContain checks to ensure the wiring is correct.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pi/extension-source/hivemind.ts`:
- Line 480: Replace the hard-coded SKILLOPT_PENDING_DIR that points to
join(homedir(), ".deeplake", "state", "skillify", "skillopt", "pending") with
the shared state-dir contract: call getStateDir() (which must treat
empty/whitespace HIVEMIND_STATE_DIR as unset and otherwise honor the override)
and build the pending path relative to that result (e.g., join(getStateDir(),
"skillify", "skillopt", "pending")); update any references to
SKILLOPT_PENDING_DIR to use this new computed path so HIVEMIND_STATE_DIR
overrides are respected.
- Around line 501-505: Restrict arming to read-only tool results: inside
skilloptArm, add a guard that returns unless the incoming tool result explicitly
represents a "read" operation (check the appropriate field on toolInput such as
toolInput.toolName / toolInput.name / toolInput.action or equivalent used in
callers) before computing ref via skilloptRefFromPath; and update the other call
site that invokes skilloptArm for successful tool results so it only calls
skilloptArm when that tool result is a read operation as well.
---
Nitpick comments:
In `@tests/pi/pi-extension-source.test.ts`:
- Around line 197-204: The test uses broad substring checks on PI_SRC which can
give false positives; update the assertions to match the exact arming condition
and path shape instead of loose substrings: assert that PI_SRC contains the
exact call expression "skilloptArm(sessionId, event.input, event.toolCallId)"
guarded by the exact condition "event.isError !== true" together in the same
conditional, and assert the ref-from-path matches the precise pattern for
SKILL.md under a skills folder (e.g. a regex like
/\/skills\/[^\/]+\/SKILL\.md/); use exact string equality or a targeted regex
match on PI_SRC rather than simple expect(...).toContain checks to ensure the
wiring is correct.
🪄 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: CHILL
Plan: Pro Plus
Run ID: b970a107-71da-4386-9c21-d9cfb67b94a8
📒 Files selected for processing (7)
esbuild.config.mjspi/extension-source/hivemind.tssrc/cli/install-pi.tssrc/hooks/shared/skillopt-hook.tssrc/skillify/skillopt-worker.tstests/pi/pi-extension-source.test.tstests/shared/skillopt-hook.test.ts
codex has no Skill/read tool — it SHELLS a read of …/skills/<name--author>/SKILL.md, so the path lives in the command string. Generalize detection to match a SKILL.md path inside a shell `command` (pathToSkillRef, shared by the hook + the worker's invokedSkillRef so the window finds it), and wire codex's hooks: pre-tool-use arms on the Bash command, capture reacts on UserPromptSubmit (agent=codex — judging runs on the codex CLI, already verified). esbuild builds skillopt-worker into codex's bundle (install-codex copyDir already ships it). - skillopt-hook: arm on a shell command reading SKILL.md (codex/hermes), via shared pathToSkillRef - skill-invocations: invokedSkillRef recognizes read-path OR shell-command SKILL.md loads - codex pre-tool-use: armSkillOptOnSkillUse in main(); codex capture: reactSkillOpt on UserPromptSubmit - tests: shell-command arming + invocation recognition NOTE: needs a live codex E2E (build + install the codex bundle, then a real codex session).
codex reads org skills via a Bash command like `sed -n '1,220p' ~/.codex/skills/.system/<name--author>/SKILL.md` (confirmed from a real codex session). The detector regex only matched a DIRECT …/skills/<ref>/SKILL.md, missing the intermediate `.system/` segment. Allow intermediate dirs, capturing the dir right before SKILL.md; markSkillPending still gates org-shape so `.system`/bare system-skill dirs are rejected.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/codex/pre-tool-use.ts`:
- Around line 372-375: Wrap the armSkillOptOnSkillUse call in a local try/catch
inside main so it cannot throw and short‑circuit the flow: call
armSkillOptOnSkillUse(...) inside try, on error catch and log the error (using
the existing logging facility) and set a local failClosed flag or skillOptArmed
= false to ensure the system treats the skill as unarmed/unsafe; then always
await processCodexPreToolUse(...) afterward (never skip it), and ensure no
exception from armSkillOptOnSkillUse bubbles out of main. Reference:
armSkillOptOnSkillUse, processCodexPreToolUse, main.
In `@src/skillify/skill-invocations.ts`:
- Around line 68-77: The SQL prefilter that currently only selects rows where
tool_name = "Skill" must be broadened so SKILL.md-based invocations aren't
dropped before invokedSkillRef() runs; update the query used where rows are
fetched (the code that narrows results before calling invokedSkillRef) to also
include tool calls whose tool_input/tool_path/tool_command reference "SKILL.md"
(or otherwise match pathToSkillRef semantics), e.g., change the WHERE clause to
accept tool_name = 'Skill' OR the tool_input/path/command contains the SKILL.md
marker so pathToSkillRef(io?.path|command) can detect them; adjust the fetching
logic in the same module (skill-invocations.ts) where the coarse SQL prefilter
is implemented.
🪄 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: CHILL
Plan: Pro Plus
Run ID: c4e719ed-1acd-44cc-9e83-2341e62fec74
📒 Files selected for processing (7)
esbuild.config.mjssrc/hooks/codex/capture.tssrc/hooks/codex/pre-tool-use.tssrc/hooks/shared/skillopt-hook.tssrc/skillify/skill-invocations.tstests/shared/skill-invocations.test.tstests/shared/skillopt-hook.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/shared/skillopt-hook.test.ts
- src/hooks/shared/skillopt-hook.ts
…ompt hermes uses an org skill the same way codex does — by shelling a read of its SKILL.md via the 'terminal' tool (path in tool_input.command). Reuse the shared shell-command detection: pre-tool-use arms via armSkillOptOnSkillUse on the terminal command, capture reacts on the pre_llm_call user prompt (agent=hermes — judging runs on the hermes CLI). esbuild builds skillopt-worker into hermes's bundle (install-hermes copyDir already ships it). No new detection code — hermes rides the codex generalization (pathToSkillRef on a shell command). Source-tested; like pi, needs a live hermes session E2E before merge.
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ools (coderabbit) CodeRabbit flagged two issues in the pi wiring: (1) the pending dir hard-coded ~/.deeplake/state/skillify/... and ignored HIVEMIND_STATE_DIR, splitting pi's state from the rest of Skillify (and from test-isolation overrides); now mirrors getStateDir()'s contract. (2) skilloptArm fired on ANY tool_result with a matching path, so an edit/write of a SKILL.md could open a judgment window; now passes the toolName and arms only on read tools. Test updated.
…ool gate (coderabbit) armSkillOptOnSkillUse is internally swallowed, but its call in main() was unguarded — a throw would skip processCodexPreToolUse and the top-level catch exits 0 (fail-open for the memory-path gate). Wrap the call so the SkillOpt side-effect is fail-closed and can't affect the tool decision.
…ads (coderabbit) The coarse SQL prefilter matched only '"Skill"' tool_calls, dropping the newly-supported SKILL.md read/shell invocations (pi/codex/hermes) before invokedSkillRef could evaluate them. Prefilter now matches a Skill tool_call OR a /SKILL.md path. Test asserts both branches.
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Add unit coverage for paths the dependency-injected tests never exercised: the REAL fileStore + spawnWorker + defaultIsOrgSkill/defaultHasCreds in skillopt-trigger (was 27% func/61% branch → 91%/86%); readCurrentSkillRow column-parsing fallbacks (install/scope/version ternaries, contributors as array vs JSON-string vs non-JSON vs object); the improve flow's empty-reaction, proposer-no-change, dedup, and session_id-mismatch branches; agent-model's codex model-override; parseMessage non-object input; summarizeEdit's no-content/no-target halves; and windowing's session-collision + empty-turn drops. 86 skillopt unit tests, all green.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/shared/skillopt-trigger.test.ts (2)
121-122: ⚡ Quick winRestore the previous
HIVEMIND_STATE_DIRinstead of always deleting it.Current cleanup can leak global env mutation when a value existed before this suite.
Proposed change
describe("default fileStore + spawnWorker (real implementations)", () => { let tmp: string; - beforeEach(() => { tmp = mkdtempSync(join(tmpdir(), "skillopt-trig-")); process.env.HIVEMIND_STATE_DIR = tmp; spawnMock.mockClear(); }); - afterEach(() => { delete process.env.HIVEMIND_STATE_DIR; }); + let prevStateDir: string | undefined; + beforeEach(() => { + prevStateDir = process.env.HIVEMIND_STATE_DIR; + tmp = mkdtempSync(join(tmpdir(), "skillopt-trig-")); + process.env.HIVEMIND_STATE_DIR = tmp; + spawnMock.mockClear(); + }); + afterEach(() => { + if (prevStateDir === undefined) delete process.env.HIVEMIND_STATE_DIR; + else process.env.HIVEMIND_STATE_DIR = prevStateDir; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/skillopt-trigger.test.ts` around lines 121 - 122, The test suite currently overwrites process.env.HIVEMIND_STATE_DIR in beforeEach and unconditionally deletes it in afterEach, which can leak global env mutation; modify the hooks so beforeEach saves the original value (e.g., const prevHivemindStateDir = process.env.HIVEMIND_STATE_DIR) before assigning tmp (created via mkdtempSync and stored in tmp) and calling spawnMock.mockClear(), and then have afterEach restore process.env.HIVEMIND_STATE_DIR to that saved prevHivemindStateDir (or delete it only if prev was undefined) instead of always deleting it.
134-145: ⚡ Quick winMake the worker script assertion path-specific (not substring-based).
Use
basenameand exact match so path-shape changes don’t mask regressions.Proposed change
-import { join } from "node:path"; +import { join, basename } from "node:path"; @@ - expect(args[0]).toContain("skillopt-worker.js"); + expect(basename(args[0])).toBe("skillopt-worker.js");As per coding guidelines,
tests/**should prefer asserting on specific values (paths, messages) over generic substrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/skillopt-trigger.test.ts` around lines 134 - 145, Replace the substring assertion on the worker script with a path-specific exact match: in tests/shared/skillopt-trigger.test.ts where spawnWorker is exercised via runEventTrigger and you inspect spawnMock.mock.calls (args[0]), call path.basename(args[0]) and assert it equals "skillopt-worker.js" (expect(path.basename(args[0])).toBe("skillopt-worker.js")) so path-shape changes won’t hide regressions; ensure you import/require the path module in the test file.Source: Coding guidelines
tests/shared/skill-invocations.test.ts (1)
123-135: ⚡ Quick winStrengthen this with an exact transcript assertion.
This can pass while order/format regresses. Assert the full output string instead of partial
toContainchecks.Proposed change
it("drops rows from a different session_id (path-LIKE collision) and empty-content turns", async () => { @@ const out = await windowAroundInvocation(fn, TABLE, inv, { before: 5, after: 5 }); - expect(out).not.toContain("another session"); // OTHER session_id dropped - expect(out).toContain("real"); - expect(out).toContain("pushback"); + expect(out).toBe("USER: real\n\nUSER: pushback"); });As per coding guidelines,
tests/**should prefer asserting on specific values (paths, messages) over generic substrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/skill-invocations.test.ts` around lines 123 - 135, The test uses loose partial assertions (expect(...).toContain) which allows ordering/format regressions; update the test in tests/shared/skill-invocations.test.ts that calls mockQuery and windowAroundInvocation (symbols: mockQuery, windowAroundInvocation, TABLE, inv) to assert the exact transcript output instead of substrings — replace the three toContain checks with a single precise assertion (e.g., expect(out).toEqual(...) or expect(out).toMatchInlineSnapshot(...)) that includes the full expected output string/format including the retained "real" and "pushback" lines and the exact representation for the tool_call, ensuring the dropped OTHER session and skipped empty assistant message are absent.Source: Coding guidelines
tests/shared/skillopt-improve.test.ts (1)
88-93: ⚡ Quick winAssert the “no reaction appended” behavior directly.
This test currently proves improve/publish still happens, but not the claimed window behavior. Spy on
judgeinput and assert the whitespace-reaction path passes the same window asreaction: undefined.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/skillopt-improve.test.ts` around lines 88 - 93, The test should additionally verify that a whitespace reaction is treated like undefined by spying on the input passed to judge: call improveSkillIfFailed(base(query, { reaction: " " })) and capture the arguments given to the judge function (or the helper that forwards to judge) and assert that the window/body passed equals the window/body when calling improveSkillIfFailed(base(query, { reaction: undefined })); update the test to create a judge spy (or mock) and compare its received window for the two calls so you directly assert the "no reaction appended" behavior alongside the existing assertions about judged/failed/improved and inserts length.tests/shared/skillopt-meta.test.ts (1)
35-41: ⚡ Quick winUse exact summaries instead of regex on joined output.
Prefer asserting the concrete
priorentries directly (e.g., exact array/elements) rather than joined-string regex matches, so this test fails only on real formatting regressions.Proposed assertion tightening
- expect(prior.join(" ")).toMatch(/delete @"old rule"/); - expect(prior.join(" ")).toMatch(/append: x/); + expect(prior).toEqual(['delete @"old rule"', "append: x"]);As per coding guidelines,
tests/**should prefer asserting on specific values over generic substrings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/shared/skillopt-meta.test.ts` around lines 35 - 41, Replace the loose regex assertions that inspect prior.join(" ") with exact-value assertions on the prior array produced by priorEditSummaries (created via metaEntryFor) so the test fails only on real output changes: locate the test case using metaEntryFor("p","k",...) and the prior = priorEditSummaries(...) call, then assert prior equals the expected array (or assert specific prior[0] and prior[1] strings) that contain the exact "delete @\"old rule\"" and "append: x" entries instead of using toMatch on the joined string.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/shared/skill-invocations.test.ts`:
- Around line 123-135: The test uses loose partial assertions
(expect(...).toContain) which allows ordering/format regressions; update the
test in tests/shared/skill-invocations.test.ts that calls mockQuery and
windowAroundInvocation (symbols: mockQuery, windowAroundInvocation, TABLE, inv)
to assert the exact transcript output instead of substrings — replace the three
toContain checks with a single precise assertion (e.g., expect(out).toEqual(...)
or expect(out).toMatchInlineSnapshot(...)) that includes the full expected
output string/format including the retained "real" and "pushback" lines and the
exact representation for the tool_call, ensuring the dropped OTHER session and
skipped empty assistant message are absent.
In `@tests/shared/skillopt-improve.test.ts`:
- Around line 88-93: The test should additionally verify that a whitespace
reaction is treated like undefined by spying on the input passed to judge: call
improveSkillIfFailed(base(query, { reaction: " " })) and capture the arguments
given to the judge function (or the helper that forwards to judge) and assert
that the window/body passed equals the window/body when calling
improveSkillIfFailed(base(query, { reaction: undefined })); update the test to
create a judge spy (or mock) and compare its received window for the two calls
so you directly assert the "no reaction appended" behavior alongside the
existing assertions about judged/failed/improved and inserts length.
In `@tests/shared/skillopt-meta.test.ts`:
- Around line 35-41: Replace the loose regex assertions that inspect
prior.join(" ") with exact-value assertions on the prior array produced by
priorEditSummaries (created via metaEntryFor) so the test fails only on real
output changes: locate the test case using metaEntryFor("p","k",...) and the
prior = priorEditSummaries(...) call, then assert prior equals the expected
array (or assert specific prior[0] and prior[1] strings) that contain the exact
"delete @\"old rule\"" and "append: x" entries instead of using toMatch on the
joined string.
In `@tests/shared/skillopt-trigger.test.ts`:
- Around line 121-122: The test suite currently overwrites
process.env.HIVEMIND_STATE_DIR in beforeEach and unconditionally deletes it in
afterEach, which can leak global env mutation; modify the hooks so beforeEach
saves the original value (e.g., const prevHivemindStateDir =
process.env.HIVEMIND_STATE_DIR) before assigning tmp (created via mkdtempSync
and stored in tmp) and calling spawnMock.mockClear(), and then have afterEach
restore process.env.HIVEMIND_STATE_DIR to that saved prevHivemindStateDir (or
delete it only if prev was undefined) instead of always deleting it.
- Around line 134-145: Replace the substring assertion on the worker script with
a path-specific exact match: in tests/shared/skillopt-trigger.test.ts where
spawnWorker is exercised via runEventTrigger and you inspect
spawnMock.mock.calls (args[0]), call path.basename(args[0]) and assert it equals
"skillopt-worker.js" (expect(path.basename(args[0])).toBe("skillopt-worker.js"))
so path-shape changes won’t hide regressions; ensure you import/require the path
module in the test file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 562b933c-1584-404e-955c-9ab0c56ef902
📒 Files selected for processing (6)
tests/shared/agent-model.test.tstests/shared/skill-invocations.test.tstests/shared/skill-org-publish.test.tstests/shared/skillopt-improve.test.tstests/shared/skillopt-meta.test.tstests/shared/skillopt-trigger.test.ts
efenocchi
approved these changes
Jun 8, 2026
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
2 participants
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.
Combined follow-up to the SkillOpt release (3 items in one PR per request).
1. Other agents — generalize detection + wire pi
armSkillOptOnSkillUsenow arms on EITHER aSkilltool (claude) OR areadof…/skills/<name--author>/SKILL.md(agents with no first-class Skill tool). markSkillPending still gates org-shape + pull manifest.tool_resultSKILL.md read and, on the next user prompt (the reaction), spawns the bundledskillopt-worker(agent=pi). Logic inlined into the raw-.ts extension (zero-dep);HIVEMIND_SKILLOPT_*env names are the cross-process contract. esbuild builds + install-pi installs the worker for pi.exec_commandread of SKILL.md isn't captured (0.136.0 PostToolUse gap) — nothing to detect yet. Filed as a prerequisite.2. CodeQL — command-injection fix
resolveAgentBindidexecSync(\command -v ${AGENT_CMD[agent] ?? agent}`);agenttraces toHIVEMIND_SKILLOPT_AGENTand the?? agent` fallback let an arbitrary value reach a shell. Now resolves KNOWN agents only by walking $PATH in Node (no shell/subprocess). Verified: known agent → bin path, injection payload → undefined.3. Coverage
tsc clean (changed files); 69 tests green.
pi wiring needs a live pi-session E2E — the extension is loaded + compiled by pi's runtime, so it's only source-tested here. Same pattern we used for claude (which is now live-verified). Don't merge until a real pi session has been driven through arm→react→publish.
Summary by CodeRabbit
New Features
Improvements
Tests