SkillOpt: Deeplake-centric direct-publish + multi-agent scorer + targeted event trigger#240
Conversation
…ocal disk
Detection is org-wide (live sessions query) but the cycle read the skill body
from local disk (readSkillBodyViaManifest), so a deficient org skill the worker's
machine doesn't have installed could never be improved — detection scope did not
match improvement scope.
Add readSkillBodyFromOrgTable: SELECT body, version FROM skills WHERE name/author
ORDER BY version DESC LIMIT 1 (latest wins, mirroring pull). The table's body
column is already frontmatter-less, which is exactly what the proposer consumes.
The cycle's readSkillBody dep is now async returning { body, version }; baseVersion
is threaded into the proposal so the upcoming publish step can land the edit as
version+1. The worker now reads from Deeplake; the local-disk manifest path is gone.
Tests: org-table reader (latest-version read + null/empty-body branches); existing
cycle tests updated to the new reader shape.
… in the org table The optimization loop is Deeplake-centric: read the body from the skills table (prior commit), improve it, and now publish the result straight back as a new version. No approval gate by design — detect → improve → publish. skill-org-publish.ts: - readCurrentSkillRow: fetch the FULL latest-version row so a republish preserves every field it isn't changing (parses the JSON contributors/source_sessions). - publishImprovedSkill: INSERT version+1 via insertSkillRow (append-only), with scope promoted to `team`, the triggering user + a `skillopt` marker appended to contributors (original author kept first, deduped), name/author unchanged. The skill keeps its original --author identity; the loop is recorded as a contributor, mirroring cross-author MERGE provenance (#118/#125). Concurrency: caller reads `current` fresh right before publish; a racing writer at worst yields two rows at one version — both preserved, readers take the latest. Tests: latest-version read + JSON parsing + legacy/empty-column tolerance; INSERT asserts version+1, scope=team, new body present / old body absent, contributor append+dedup, and the [author]-seed for legacy rows with no contributors.
…oposal-writing) The cycle's terminal step is now a DIRECT publish, not a review proposal. Rename the `writeProposal` dep to `publish` and await it; the worker's publish reads the full current row (readCurrentSkillRow) and lands the improved body as version+1 in the org skills table (scope→team, +collaborator/skillopt contributors), so teammates re-pull it. Drop writeProposalToDisk and the local proposals dir — there is no review/approval gate by design (detect → improve → publish). Tests: engine cycle updated to the publish dep; full skillopt sweep green (29).
…blish The E2E test surfaced a read-after-write disagreement: the cycle read the body once for the proposer, then the publisher read the row AGAIN — and under Deeplake eventual consistency the two reads could disagree, so the proposer would edit a skill the publisher then reported "not in table", wasting the LLM call and silently skipping the publish. Fix + simplification: readSkill now returns the FULL current row, and the cycle threads that SAME row to publish — one read, no second query, no disagreement. Drop the now-redundant readSkillBodyFromOrgTable (readCurrentSkillRow supersedes it). Tests updated to the readSkill (full row) + publish(current, rec) shape.
… own agent
The judge/proposer defaulted to claude (success-judge haiku, proposer sonnet), so a
codex/hermes/cursor/pi user with no local `claude` got nothing. Add agent-model.ts:
agentModel({agent, role}) dispatches to each agent's CLI in its safest NO-TOOLS mode
(claude `--tools ""`, codex `-s read-only --skip-git-repo-check`, hermes `-z` via
provider, cursor/pi `--print`) — the scorer feeds UNTRUSTED transcript text into the
prompt, so no tool use is allowed. Downstream parsers already extract JSON from
arbitrary text, so each agent only emits JSON in stdout — no per-agent output schema.
Model is per-agent+role with env overrides. detectScorerAgent resolves the host agent
(explicit HIVEMIND_SKILLOPT_AGENT override, then claude/codex env signatures, default
claude_code).
Worker scores on the detected agent. spawn injectable → per-agent argv + parsing
unit-tested with zero real CLI calls.
Tests: agent-model (11) — each agent's safe args, output parsing, env override,
non-zero exit, detection; full skillopt sweep green (57).
…der configurable Two issues the real codex/hermes CLI test surfaced: 1. findAgentBin's static path list misses nvm/volta/fnm installs — codex lives at ~/.nvm/.../bin/codex, so the worker ENOENT'd. The worker now resolves the agent bin via `command -v` (mirrors the wiki spawn helpers) and passes it to agentModel; undefined falls back to findAgentBin. agent-model.ts stays PATH-free for the openclaw bundle; the standalone worker can use `command -v`. 2. hermes provider was hardcoded to openrouter — wrong for a user on AWS Bedrock (or any other provider). Add HIVEMIND_SKILLOPT_<AGENT>_PROVIDER override. Verified: codex scores correctly through agentModel (read-only sandbox → clean JSON). hermes stays real-CLI-unverified (this machine has no hermes model configured). Tests: provider-override case (12 agent-model tests).
…ocument hermes Bedrock model ids hermes verified end-to-end on AWS Bedrock through agentModel (real verdict). The debug surfaced two robustness gaps: - hermes on a dead/legacy Bedrock model exits 0 with EMPTY stdout, swallowing the provider error to a dump file. agentModel now rejects on empty stdout so a misconfigured scorer surfaces loudly; callers already swallow it safely (judge stays conservative, proposer reports a failed call) instead of silently resolving "" into an unparseable verdict / no edits forever. - the hermes default model is openrouter-style and invalid on other providers; documented that Bedrock (etc.) users must set HIVEMIND_SKILLOPT_HERMES_PROVIDER + _MODEL — Bedrock needs an inference-profile id (us.anthropic.claude-haiku-4-5-...). Tests: empty-exit-0 rejection (13 agent-model tests).
…erence-profile id) pi verified end-to-end through agentModel (real verdict). Like hermes, pi's google/ gemini default needs a Google API key; a user on another provider (e.g. AWS Bedrock) must set HIVEMIND_SKILLOPT_PI_PROVIDER + _MODEL. cursor's dispatch flags are verified correct (--print --model --force --output-format text) — it was only auth-blocked. A wrong default now fails loudly via the exit-code / empty-output guards.
… not weekly The event trigger's core: replace the weekly time-throttle with accumulation. recordAnchored folds free-anchor-detected pushbacks into a per-skill count (deduped by sessionId#index so repeated UserPromptSubmit firings don't double-count); when a skill crosses the fire threshold (default 5, HIVEMIND_SKILLOPT_FIRE_COUNT) the worker fires for the org cycle and that skill's count resets. Cheap by design — the increment is a no-LLM pure state update; only the rare crossing spawns the worker. Tests: accumulate-below / fire-and-reset / dedup / independent skills / custom threshold / malformed-skip / tally-restart-after-fire (8).
…org-skill invocations #4b: anchoredOrgSkillsInSession reads one session's turns from Deeplake (capture normalises every agent into the sessions table, so multi-agent for free), locates each org-skill Skill invocation in a single pass, windows it, and runs the FREE heuristic anchor (no LLM) to find which skills the user pushed back on. Each anchored invocation gets a stable key sessionId#ordinal so the counter dedups it across repeated UserPromptSubmit/SessionEnd firings. Exports parseMessage/ParsedMsg from skill-invocations for reuse. Tests: anchored / satisfied / two-invocations-distinct-keys / org-skills-only / no-pre-invocation-misattribution / empty-session-id (6).
#4c-1: runEventTick is the cheap no-LLM step that runs (detached) on UserPromptSubmit/ SessionEnd — scan the session for pushed-back org skills (free anchor) -> fold into the per-skill counter -> report which skills crossed the threshold. The caller runs the paid org cycle only when something fired, so the per-turn path stays free. Replaces the weekly time-throttle: firing is driven by accumulated pushback, not the calendar. Adds atomic load/saveCounterState under the HIVEMIND_STATE_DIR-aware skillopt/counter.json. Tests: no-pushback no-op / record-below-threshold / cross-threshold-across-ticks / empty-session no-op (4); persistence round-trip + torn-file tolerance (2).
…cle only on fire #4c-2: when spawned with HIVEMIND_SKILLOPT_SESSION (the hook path), the worker runs the event tick first — scan that session for pushed-back org skills (free anchor), fold into the per-skill counter — and only proceeds to the paid org cycle when a skill crossed the fire threshold; otherwise it exits cheaply. Under event-firing the engine's >=N-deficient- SKILLS gate is redundant (the counter is the pattern signal), so fireThreshold defaults to 1 in event mode. A bare run (no session) stays a manual invocation with the engine default.
… improve if it failed Redesign to immediate + targeted (not accumulate-then-org-sweep). When an org skill X was invoked and the user reacts, judge X's window — the LLM is the only evaluator now, since a regex can't catch 'you fucked up again' — with the just-submitted reaction appended (it lags Deeplake, so the hook passes it in). If the judge says FAILED, read X's current body, propose a bounded edit, publish v+1 right then. meta-dedup stops re-publishing the same edit when the next reaction re-judges the same window. findLatestInvocation targets the invocation the user is reacting to. Reuses the existing judge / proposer / publisher / multi-agent scorer / windowing / meta. Tests: located-invocation, failed->improve+publish, ok->noop, not-org-skill, not-in-session, skill-absent, dedup (8).
…rg-wide cycle Per the redesign (immediate, K=3, LLM-judge-as-detector): an org skill call opens a K-message judgment window; the user's next reactions fire the worker, which judges that ONE skill and improves it on the first confirmed failure. No regex anchor, no per-skill counter, no org-wide all-skills scan, no session-end wait. - skillopt-trigger: markSkillPending (open a K=3 window on org-skill PreToolUse) + runEventTrigger(session, reaction) (fire per reaction, decrement the budget). Spawns the worker with SESSION/SKILL/REACTION/AGENT env. - skillopt-worker: targeted — reads the env inputs and calls improveSkillIfFailed. - hooks: PreToolUse -> markSkillPending; UserPromptSubmit (capture) -> runEventTrigger with the prompt as the reaction; SessionEnd skillopt call removed (fires on the reaction now). - remove the obsolete org-wide/accumulate machinery: skillopt-engine (runSkillOptCycle), skillopt-counter, skillopt-tick, skillopt-session-scan, deficiency-detector, skill-edit-gate, session-anchor (the dropped regex) + their tests. Reuses judge / proposer / publisher / multi-agent scorer / windowing / meta-dedup. 80 skillopt tests green; tsc clean.
Extract armSkillOptOnSkillUse (PreToolUse → open a window on an ORG Skill tool_use) and reactSkillOpt (UserPromptSubmit → fire the worker to judge the pending skill against the prompt, on the user's agent) into src/hooks/shared/skillopt-hook.ts, so each agent's hook wires SkillOpt with one swallowed call instead of copy-pasting. The base claude PreToolUse + capture hooks now use it. NOTE: per-agent (codex/hermes/cursor) trigger wiring is deliberately NOT done here — those agents don't surface org-skill invocation as a uniform 'Skill' tool_use (codex=command, hermes=terminal, cursor=Shell), so arming needs per-agent skill-invocation detection first. Tests: helper guards (Skill-only, disabled, no-prompt, internal-worker) — 6.
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSkillOpt is converted from a weekly batch flow to event-driven per-reaction handling: hooks arm pending windows and trigger targeted workers, a file-backed pending store manages budgets, agent-model abstraction handles judge/proposer runs, and successful judge failures can publish improved skill rows. ChangesEvent-Driven SkillOpt Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Per-agent trigger investigation (the #4d follow-up)Question: for codex/hermes/cursor/pi, does invoking an org skill produce a detectable signal — in a hook (to arm the trigger) and in the captured session (so the worker can locate the window)? Q1 — distribution (code): skills are distributed as Q2 — capture shape (code, decisive): every agent's Q3 — invocation mechanism (needs empirical): the code shows distribution (files), not invocation. Whether codex/hermes/pi invoke a discovered Verdict
Remaining work = one empirical check for codex/hermes/pi: install an org skill, drive the real CLI to use it, grep the captured rows for a OpenClaw gap (omitted, flagged)OpenClaw is a gateway/host that bundles hivemind's worker under ClawHub's static scanner — exactly why |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🧹 Nitpick comments (2)
tests/shared/skillopt-hook.test.ts (1)
36-44: ⚡ Quick winAdd explicit empty/whitespace prompt coverage for
reactSkillOpt.Current tests at Line 36-44 cover
undefinedonly. Add""and" "cases to lock in no-op behavior for non-reactions and protect budget accounting.Suggested test additions
describe("reactSkillOpt", () => { @@ it("does nothing when there is no prompt (not a UserPromptSubmit)", () => { reactSkillOpt("s1", undefined, "codex"); expect(runEventTrigger).not.toHaveBeenCalled(); }); + it("does nothing for empty or whitespace-only prompt values", () => { + reactSkillOpt("s1", "", "codex"); + reactSkillOpt("s1", " ", "codex"); + expect(runEventTrigger).not.toHaveBeenCalled(); + }); it("does nothing inside an internal worker call (HIVEMIND_WIKI_WORKER=1)", () => {🤖 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-hook.test.ts` around lines 36 - 44, Add two test cases to cover empty and whitespace prompts so reactSkillOpt doesn't trigger reactions or affect budget: duplicate the existing spec blocks around reactSkillOpt("s1", undefined, "codex") and reactSkillOpt("s1", "hi", "claude_code") using reactSkillOpt("s1", "", "codex") and reactSkillOpt("s1", " ", "codex") (and likewise for the HIVEMIND_WIKI_WORKER=1 scenario if desired), and assert runEventTrigger.not.toHaveBeenCalled(); ensure you set/restore process.env.HIVEMIND_WIKI_WORKER in the internal worker test as the existing tests do and place the new cases alongside the existing tests referencing reactSkillOpt and runEventTrigger.src/hooks/session-start.ts (1)
271-273: ⚡ Quick winUpdate this comment to match the actual trigger path.
runEventTriggeris driven byUserPromptSubmit/capture, andsrc/hooks/session-end.tsexplicitly says SessionEnd does nothing for SkillOpt. Mentioning SessionEnd and “accumulated pushback” here will send the next maintainer to the wrong hook when debugging this flow.🤖 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 `@src/hooks/session-start.ts` around lines 271 - 273, Update the comment in SessionStart to reflect the real trigger path: remove the reference to SessionEnd and “accumulated pushback” and state that SkillOpt is event-driven and armed via PreToolUse but that runEventTrigger is driven by UserPromptSubmit (capture) — mention that src/hooks/session-end.ts intentionally does nothing for SkillOpt so maintainers should look at UserPromptSubmit/ capture and PreToolUse when debugging the SkillOpt flow.
🤖 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/shared/skillopt-hook.ts`:
- Around line 28-29: The guard currently only checks prompt === undefined so
empty or whitespace-only prompts still call runEventTrigger; update the early
return to treat empty strings and strings containing only whitespace as falsy by
trimming or using a utility (e.g., replace the prompt === undefined check with a
trim-based check such as if (!prompt || String(prompt).trim() === "" ||
process.env.HIVEMIND_WIKI_WORKER === "1") return) so runEventTrigger(sessionId,
prompt, { agent }) is only invoked for non-empty, non-whitespace prompts; ensure
you reference the existing prompt variable and runEventTrigger call in
skillopt-hook.ts.
In `@src/skillify/agent-model.ts`:
- Around line 124-139: The child process is still inheriting global process.env
instead of the injected env (const env = opts.env ?? process.env), so propagate
that injected env into the spawn environment: in the spawnFn call that creates
child (use symbols spawnFn, bin, args, child), replace the env object currently
built from process.env with one that uses the local env variable (merged with
any required HIVEMIND_* overrides and fallback to process.env if needed) so the
spawned agent receives the same scoped credentials/config passed via opts.env.
- Around line 70-79: Detect when a config overrides only provider but leaves
model as the default for provider-specific entries (the objects with buildArgs,
model, provider in this file), and fail fast: inside each problematic
model-definition (the entries that define buildArgs, parse, model, provider —
e.g. the Hermes and PI scorer configs) add a validation step that checks if
provider is one of the Hermes/PI-specific providers and model() returns
undefined or the default provider-incompatible model ID; if so throw or return a
clear configuration error (or call the existing logger/error path) explaining
that a provider override requires an appropriate model override. Apply the same
guard to the other scorer objects referenced around the other blocks (lines
noted in the review) so provider-only environment overrides cannot produce
incompatible default buildArgs.
In `@src/skillify/skill-org-publish.ts`:
- Around line 101-117: publishImprovedSkill currently computes version as
opts.current.version + 1 in-memory which races; move version allocation into the
database atomically by either (a) allocating the next version inside a
transaction (SELECT MAX(version) FROM <tableName> FOR UPDATE then INSERT with
max+1) or (b) doing an atomic INSERT that computes MAX(version)+1 server-side or
(c) add a unique constraint and retry-on-conflict to let the DB pick a winner;
also ensure readers and writers use the same deterministic tie-break (e.g. ORDER
BY version DESC, created_at DESC or a monotonic id) so insertSkillRow and the
reader that uses ORDER BY version DESC LIMIT 1 resolve ties the same way; update
publishImprovedSkill to call insertSkillRow with the DB-assigned version (or
implement retry) and keep appendUnique/SKILLOPT_CONTRIBUTOR logic unchanged.
In `@src/skillify/skillopt-improve.ts`:
- Around line 32-50: findLatestInvocation currently sets found.ts to m.timestamp
or empty string which breaks downstream windowing when timestamps are missing;
update the function so that when parseMessage(m).timestamp is absent you instead
use a stable fallback from the row (e.g., r.creation_date) and store that value
in the returned SkillInvocation.ts; ensure the discriminator is set from either
m.timestamp or r.creation_date (both as strings) and then update the downstream
windowing logic that consumes SkillInvocation.ts to match on that same field so
the same stable value is used for identifying the latest invocation.
- Around line 104-109: The publish already succeeds in publishImprovedSkill (the
returned version is live), so make opts.recordEdit a best-effort call: wrap the
call to opts.recordEdit(parts.name, parts.author, p.edits) in a try/catch, log
or record any error but do not rethrow it, and always return the success object
(judged: true, failed: true, improved: true, version, reason) even if recordEdit
fails; keep references to publishImprovedSkill, opts.recordEdit, parts.name,
parts.author, and p.edits so the change is easy to locate.
In `@src/skillify/skillopt-trigger.ts`:
- Around line 58-68: loadState/saveState are doing read-modify-write on the same
JSON without inter-process locking which lets concurrent hook processes clobber
state.pending; wrap the entire transaction in an OS-level file lock or switch to
per-session pending files and a safe merge strategy. Concretely: in
saveState/loadState (and the similar logic referenced at the other occurrences),
acquire an exclusive advisory lock (flock) on the state file before reading,
perform the read-modify-write of state.pending, fsync the parent dir after
rename, and release the lock; alternatively implement per-session files for
pending (e.g. pending.<sessionId>.json), write atomically then merge/increment
counters under a single locked merge step when reading the global state. Ensure
the locking/merge is applied to the code paths that touch state.pending in
loadState, saveState and the other noted blocks.
In `@tests/shared/agent-model.test.ts`:
- Around line 84-89: The test "env provider override (e.g. AWS Bedrock instead
of openrouter) is incorrect because the implementation requires an explicit
model override for non-OpenRouter Hermes providers; update the test to set
HIVEMIND_SKILLOPT_HERMES_MODEL in the env alongside
HIVEMIND_SKILLOPT_HERMES_PROVIDER (use fakeSpawn, agentModel, argVal, calls as
in the test) and then assert that both the provider (--provider) equals
"bedrock" and the model argument matches the HIVEMIND_SKILLOPT_HERMES_MODEL
value so the test reflects the documented validation rules.
---
Nitpick comments:
In `@src/hooks/session-start.ts`:
- Around line 271-273: Update the comment in SessionStart to reflect the real
trigger path: remove the reference to SessionEnd and “accumulated pushback” and
state that SkillOpt is event-driven and armed via PreToolUse but that
runEventTrigger is driven by UserPromptSubmit (capture) — mention that
src/hooks/session-end.ts intentionally does nothing for SkillOpt so maintainers
should look at UserPromptSubmit/ capture and PreToolUse when debugging the
SkillOpt flow.
In `@tests/shared/skillopt-hook.test.ts`:
- Around line 36-44: Add two test cases to cover empty and whitespace prompts so
reactSkillOpt doesn't trigger reactions or affect budget: duplicate the existing
spec blocks around reactSkillOpt("s1", undefined, "codex") and
reactSkillOpt("s1", "hi", "claude_code") using reactSkillOpt("s1", "", "codex")
and reactSkillOpt("s1", " ", "codex") (and likewise for the
HIVEMIND_WIKI_WORKER=1 scenario if desired), and assert
runEventTrigger.not.toHaveBeenCalled(); ensure you set/restore
process.env.HIVEMIND_WIKI_WORKER in the internal worker test as the existing
tests do and place the new cases alongside the existing tests referencing
reactSkillOpt and runEventTrigger.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d4b92a9-df0e-4727-ba41-15c509baa8a2
📒 Files selected for processing (24)
src/hooks/capture.tssrc/hooks/pre-tool-use.tssrc/hooks/session-end.tssrc/hooks/session-start.tssrc/hooks/shared/skillopt-hook.tssrc/skillify/agent-model.tssrc/skillify/deficiency-detector.tssrc/skillify/session-anchor.tssrc/skillify/skill-edit-gate.tssrc/skillify/skill-invocations.tssrc/skillify/skill-org-publish.tssrc/skillify/skillopt-engine.tssrc/skillify/skillopt-improve.tssrc/skillify/skillopt-trigger.tssrc/skillify/skillopt-worker.tstests/shared/agent-model.test.tstests/shared/deficiency-detector.test.tstests/shared/session-anchor.test.tstests/shared/skill-edit-gate.test.tstests/shared/skill-org-publish.test.tstests/shared/skillopt-engine.test.tstests/shared/skillopt-hook.test.tstests/shared/skillopt-improve.test.tstests/shared/skillopt-trigger.test.ts
💤 Files with no reviewable changes (8)
- tests/shared/skill-edit-gate.test.ts
- src/skillify/skill-edit-gate.ts
- src/skillify/skillopt-engine.ts
- tests/shared/session-anchor.test.ts
- tests/shared/deficiency-detector.test.ts
- src/skillify/deficiency-detector.ts
- tests/shared/skillopt-engine.test.ts
- src/skillify/session-anchor.ts
| * Concurrency: the caller reads `current` fresh right before this, so a racing | ||
| * publisher at worst produces two rows at the same version — both preserved | ||
| * (append-only), readers take the latest. Returns the published version. | ||
| */ | ||
| export async function publishImprovedSkill(opts: { | ||
| query: (sql: string) => Promise<unknown>; | ||
| tableName: string; | ||
| workspaceId: string; | ||
| current: CurrentSkillRow; | ||
| newBody: string; | ||
| collaborator?: string; // the triggering user (whose corpus/agent drove it); optional | ||
| now: string; | ||
| }): Promise<{ version: number }> { | ||
| const version = opts.current.version + 1; | ||
| const base = opts.current.contributors.length ? opts.current.contributors : [opts.current.author]; | ||
| const contributors = appendUnique(base, [opts.collaborator, SKILLOPT_CONTRIBUTOR]); | ||
| await insertSkillRow({ |
There was a problem hiding this comment.
Make version allocation atomic instead of current.version + 1.
Line 114 computes the next version in memory, so two workers that read the same row will both publish the same version. Lines 61-66 then resolve "latest" with ORDER BY version DESC LIMIT 1, which makes the winner arbitrary on a tie. That can branch future edits from the wrong body/provenance. Please move version allocation into Deeplake atomically, or add a deterministic tie-break that every read path uses.
🤖 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 `@src/skillify/skill-org-publish.ts` around lines 101 - 117,
publishImprovedSkill currently computes version as opts.current.version + 1
in-memory which races; move version allocation into the database atomically by
either (a) allocating the next version inside a transaction (SELECT MAX(version)
FROM <tableName> FOR UPDATE then INSERT with max+1) or (b) doing an atomic
INSERT that computes MAX(version)+1 server-side or (c) add a unique constraint
and retry-on-conflict to let the DB pick a winner; also ensure readers and
writers use the same deterministic tie-break (e.g. ORDER BY version DESC,
created_at DESC or a monotonic id) so insertSkillRow and the reader that uses
ORDER BY version DESC LIMIT 1 resolve ties the same way; update
publishImprovedSkill to call insertSkillRow with the DB-assigned version (or
implement retry) and keep appendUnique/SKILLOPT_CONTRIBUTOR logic unchanged.
There was a problem hiding this comment.
Partially addressed. The dominant same-machine race (K=3 reactions → multiple workers per skill) is removed by a per-skill tryAcquireWorkerLock in 7377b47, and readCurrentSkillRow now orders by version DESC, created_at DESC so equal versions resolve deterministically. Fully-atomic cross-machine allocation needs a DB-level uniqueness/sequence Deeplake doesn't offer (append-only table, no unique constraint); a matching pull-side tie-breaker is the foundation-level follow-up. Append-only history preserves every version.
There was a problem hiding this comment.
@kaghni, thanks for the detailed breakdown. The same-machine dominant race being eliminated via tryAcquireWorkerLock and the deterministic version DESC, created_at DESC ordering for equal-version ties are meaningful mitigations given the constraints of an append-only table. Understood that a DB-level unique constraint or sequence isn't available in Deeplake, so a fully atomic allocation would require either a separate coordination primitive or a foundation-level schema change — both reasonable to defer.
Would you like me to open a tracking GitHub issue for the pull-side tie-breaker follow-up so it doesn't get lost?
🧠 Learnings used
Learnt from: kaghni
Repo: activeloopai/hivemind PR: 97
File: src/hooks/shared/autoupdate.ts:122-146
Timestamp: 2026-05-07T08:28:25.207Z
Learning: In `src/hooks/shared/autoupdate.ts`, the `autoUpdate` function intentionally does NOT include a machine-wide lock/cooldown. An initial draft had a flock-style lock at `~/.deeplake/.autoupdate.lock`, but after a latency-fix refactor to fire-and-forget detached spawn (commit 601aa5c), the lock is released immediately after dispatching the child — defeating the purpose. The proper home for the lock is `src/cli/update.ts:runUpdate()` so the long-running `hivemind update` process owns the lock for its full lifetime. `npm install -g` has its own internal locking that prevents corruption in the meantime, so concurrent invocations are either both no-ops (already at latest) or one wins idempotently. This is tracked as a separate follow-up.
| export async function findLatestInvocation( | ||
| query: QueryFn, sessionsTable: string, sessionId: string, name: string, author: string, | ||
| ): Promise<SkillInvocation | null> { | ||
| const sid = sqlStr(likeEscape(sessionId)); | ||
| const rows = await query( | ||
| `SELECT message FROM "${sqlIdent(sessionsTable)}" WHERE path LIKE '/sessions/%${sid}%' ESCAPE '\\' ORDER BY creation_date ASC`, | ||
| ); | ||
| let found: SkillInvocation | null = null; | ||
| for (const r of rows) { | ||
| const m = parseMessage(r.message); | ||
| if (!m) continue; | ||
| if (typeof m.session_id === "string" && m.session_id !== sessionId) continue; | ||
| const ref = invokedSkillRef(m); | ||
| if (!ref) continue; | ||
| const p = splitOrgSkill(ref); | ||
| if (!p || p.name !== name || p.author !== author) continue; | ||
| found = { sessionId, name, author, ts: typeof m.timestamp === "string" ? m.timestamp : "" }; // keep last → latest | ||
| } | ||
| return found; |
There was a problem hiding this comment.
Preserve a stable discriminator for the latest invocation.
findLatestInvocation keeps the last matching row, but Line 48 only returns m.timestamp. When that field is missing, ts becomes empty, and the downstream windowing path falls back to the first matching invocation for the same skill. In a session that invokes the same skill multiple times without message timestamps, this will judge the wrong occurrence and can publish an edit from stale context. Carry a fallback from the queried row (for example creation_date) and match on that in the windowing path as well.
🤖 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 `@src/skillify/skillopt-improve.ts` around lines 32 - 50, findLatestInvocation
currently sets found.ts to m.timestamp or empty string which breaks downstream
windowing when timestamps are missing; update the function so that when
parseMessage(m).timestamp is absent you instead use a stable fallback from the
row (e.g., r.creation_date) and store that value in the returned
SkillInvocation.ts; ensure the discriminator is set from either m.timestamp or
r.creation_date (both as strings) and then update the downstream windowing logic
that consumes SkillInvocation.ts to match on that same field so the same stable
value is used for identifying the latest invocation.
There was a problem hiding this comment.
Acknowledged. Captured Skill rows carry message.timestamp in practice, so findLatestInvocation resolves the latest correctly; the empty-ts path (sessionTurns falling back to the first matching invocation) only bites if the timestamp is absent. Tracked as a follow-up to thread a row-order discriminator.
There was a problem hiding this comment.
@kaghni, understood — the practical guarantee that captured Skill rows always carry message.timestamp makes the empty-ts branch a true edge case rather than an active risk. The deferred follow-up (threading creation_date as a stable fallback discriminator) is a reasonable scoping decision for this PR.
Would you like me to open a GitHub issue to track the follow-up? I can link it back to this comment so the context isn't lost.
…odex P2) Codex flagged that markSkillPending/runEventTrigger load-modify-overwrite a single shared skillopt/state.json, so two concurrent sessions arming/reacting can clobber each other's pending entry (the later save drops the other session's). Store pending state PER SESSION (one small file each, sessionId sanitised to a safe filename) behind an injectable PendingStore — two sessions now touch different files, so neither can drop the other's window. Tests: a cross-session no-clobber case + the store-injected suite (trigger + hook, 16).
…ex P2) Codex flagged that concurrent workers can each read the same current row and publish version+1, creating duplicate version rows whose visible body is nondeterministic. - The K=3 reactions spawn up to K workers for the SAME skill (dominant case): wrap improveSkillIfFailed in a per-skill tryAcquireWorkerLock so only one reads-and-publishes at a time; the loser skips, and a later worker re-reads the now-improved skill (meta-dedup no-op). Removes the same-machine duplicate-version race. - readCurrentSkillRow now orders by version DESC, created_at DESC — so a cross-machine race that lands equal versions resolves the SAME row deterministically. Append-only history preserves every version; a matching pull-side tie-breaker is a foundation-level follow-up.
…nv, post-publish meta (coderabbit) - skillopt-hook: skip empty/whitespace reactions (don't spend the judgment budget / spawn a worker on a non-reaction). - agent-model: fail fast when a hermes/pi provider is overridden WITHOUT a model — the default model id is provider-specific, so a bare _PROVIDER=bedrock would silently send a wrong id. - agent-model: spawn the child with the injected env (not just global process.env), so a caller-scoped cred/config reaches it. - skillopt-improve: swallow a recordEdit (meta) failure AFTER a successful publish — reporting failure there would drop the dedup marker and invite a re-publish. - agent-model tests: provider override now paired with a model; add fail-fast + env-propagation cases.
|
@coderabbitai review |
✅ Action performedReview finished.
|
The redesign removed the weekly runWeeklySkillOpt call from session-start (SkillOpt is event-driven now via PreToolUse/UserPromptSubmit), but session-start-hook.test.ts still asserted the old fired/swallowed branches and mocked the removed runWeeklySkillOpt export — failing CI's full test run. Remove the two obsolete tests + the orphaned mock.
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 16 files changed
Generated for commit ed1f4f1. |
…by tool_use_id (codex P2) Two data-integrity findings on the event path: - markSkillPending was a syntax-only name--author check, so a LOCAL skill named like an org skill could arm and get the shared org-table row of that name edited. Gate on the pull manifest (authoritative org source) — only arm for actually-pulled org skills. - the worker resolved the 'latest' invocation at query time, so a quick re-invocation of the same skill could make it judge the wrong window. Capture tool_use_id at PreToolUse, thread it arm -> pending -> worker, and findInvocation now pins THAT invocation (falling back to latest if it isn't captured yet). Tests: manifest-gate (local-shadow rejected), tool_use_id stored + passed + pinned-lookup.
|
@coderabbitai review |
✅ Action performedReview finished.
|
| env: { | ||
| ...process.env, | ||
| HIVEMIND_SKILLOPT_WORKER: "1", | ||
| HIVEMIND_SKILLOPT_SESSION: sessionId, | ||
| HIVEMIND_SKILLOPT_SKILL: skill, | ||
| HIVEMIND_SKILLOPT_REACTION: (reaction ?? "").slice(0, MAX_REACTION), | ||
| ...(toolUseId ? { HIVEMIND_SKILLOPT_TOOL_USE_ID: toolUseId } : {}), | ||
| ...(agent ? { HIVEMIND_SKILLOPT_AGENT: agent } : {}), |
There was a problem hiding this comment.
I think is better to define a file with the global variable to avoid code duplication, I'm not sure if HIVEMIND_SKILLOPT_WORKER: "1", should be in another place.
@CodeRabbit wdyt?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg 'HIVEMIND_SKILLOPT_' --type ts -nLength of output: 8110
🏁 Script executed:
#!/bin/bash
rg 'HIVEMIND_SKILLOPT_' -nLength of output: 8429
@efenocchi Good call — there's a real duplication problem here, and HIVEMIND_SKILLOPT_WORKER is actually the tip of the iceberg. Looking at the full picture:
| Env var | Written in | Read in |
|---|---|---|
HIVEMIND_SKILLOPT_WORKER |
skillopt-trigger.ts:159 |
skillopt-trigger.ts:134 |
HIVEMIND_SKILLOPT_SESSION |
skillopt-trigger.ts:160 |
skillopt-worker.ts:42 |
HIVEMIND_SKILLOPT_SKILL |
skillopt-trigger.ts:161 |
skillopt-worker.ts:43 |
HIVEMIND_SKILLOPT_REACTION |
skillopt-trigger.ts:162 |
skillopt-worker.ts:44 |
HIVEMIND_SKILLOPT_TOOL_USE_ID |
skillopt-trigger.ts:163 |
skillopt-worker.ts:45 |
HIVEMIND_SKILLOPT_AGENT |
skillopt-trigger.ts:164 |
agent-model.ts:177 |
HIVEMIND_SKILLOPT_DISABLED |
skillopt-trigger.ts:133 |
skillopt-hook.ts:15 |
The trigger→worker pairs (SESSION, SKILL, REACTION, TOOL_USE_ID) are the riskiest: a typo in either side breaks the IPC contract silently at runtime. A shared constants file solves exactly that:
// src/skillify/skillopt-env-keys.ts
/** IPC env vars passed from skillopt-trigger → skillopt-worker */
export const ENV = {
WORKER: "HIVEMIND_SKILLOPT_WORKER",
SESSION: "HIVEMIND_SKILLOPT_SESSION",
SKILL: "HIVEMIND_SKILLOPT_SKILL",
REACTION: "HIVEMIND_SKILLOPT_REACTION",
TOOL_USE_ID:"HIVEMIND_SKILLOPT_TOOL_USE_ID",
AGENT: "HIVEMIND_SKILLOPT_AGENT",
DISABLED: "HIVEMIND_SKILLOPT_DISABLED",
JUDGE_WINDOW: "HIVEMIND_SKILLOPT_JUDGE_WINDOW",
} as const;Then in skillopt-trigger.ts:
import { ENV } from "./skillopt-env-keys.js";
// spawn env:
HIVEMIND_SKILLOPT_WORKER: "1" → [ENV.WORKER]: "1"
HIVEMIND_SKILLOPT_SESSION: sessionId → [ENV.SESSION]: sessionId
// ...
// recursion guard:
if (env[ENV.WORKER] === "1") ...
if (env[ENV.DISABLED] === "1") ...And in skillopt-worker.ts:
import { ENV } from "./skillopt-env-keys.js";
const sessionId = process.env[ENV.SESSION] ?? "";
const skillRef = process.env[ENV.SKILL] ?? "";
// ...I'd recommend scoping the constants file to the IPC contract only — the dynamic HIVEMIND_SKILLOPT_<AGENT>_MODEL/PROVIDER family in agent-model.ts is assembled via template literals and doesn't benefit from this pattern.
🧬 Code Graph Analysis Results
Relevant snippets
src/skillify/skillopt-trigger.ts (context for env-var placement/duplication)
- Not re-included here: the relevant
HIVEMIND_SKILLOPT_WORKERcheck/spawn behavior is already visible in the providedskillopt-trigger.tscontents.
src/skillify/skill-invocations.ts — lines 57-66
export function splitOrgSkill(skill: string): { name: string; author: string } | null {
if (skill.includes(":")) return null; // plugin-namespaced (e.g. hivemind:hivemind-memory)
// name/author are used to build filesystem paths (skills dir, proposals dir), so a
// captured tool_input must not smuggle path separators / traversal — same untrusted
// treatment the pull path applies to these segments.
if (skill.includes("/") || skill.includes("\\") || skill.includes("..")) return null;
const i = skill.lastIndexOf("--");
if (i <= 0 || i + 2 >= skill.length) return null; // bare or malformed
return { name: skill.slice(0, i), author: skill.slice(i + 2) };
}src/skillify/manifest.ts — lines 76-136 (summary)
/**
* loadManifest(path?: string): PulledManifest
*
* Params:
* - path: optional manifest path (defaults to manifestPath()).
*
* Returns:
* - A validated PulledManifest object with shape { version: 1, entries: PulledEntry[] }.
*
* Error handling / defensive behavior:
* - If the manifest file is missing or unreadable, returns emptyManifest().
* - If JSON is invalid or not the expected shape/version, returns emptyManifest().
* - Validates each entry:
* - Requires dirName/name/author/installRoot/install fields to have expected types/values.
* - Rejects dirName containing path separators or `..`.
* - Validates symlinks list as absolute paths without `..`; otherwise uses [].
* - On any parse/validation exception, returns emptyManifest() (fail-safe).
*/src/skillify/state-dir.ts — lines 27-39
export function getStateDir(): string {
// Trim before truthy-check so `HIVEMIND_STATE_DIR=""` or
// `HIVEMIND_STATE_DIR=" "` (forgotten value in CI config, accidental
// empty pass-through) does NOT win the `??` arm. An empty string is
// a perfectly valid env value — `??` would accept it — but downstream
// `join("", ".deeplake", ...)` resolves relative to the worker's cwd
// and silently pollutes whatever directory the process was started in.
// Treat blank as unset.
const override = process.env.HIVEMIND_STATE_DIR?.trim();
return override && override.length > 0
? override
: join(homedir(), ".deeplake", "state", "skillify");
}src/config.ts — lines 31-76 (summary)
/**
* loadConfig(): Config | null
*
* Inputs:
* - Reads credentials from ~/.deeplake/credentials.json if it exists.
* - Uses process.env.HIVEMIND_* (with possible fallback to creds file values).
*
* Returns:
* - Config object containing token/orgId and many table/path settings, or null if missing required creds.
*
* Error handling:
* - If credentials.json exists but is invalid JSON, returns null.
* - If required fields (token, orgId) are missing, returns null.
*/src/utils/debug.ts — lines 22-25
export function log(tag: string, msg: string) {
if (!isDebug()) return;
appendFileSync(LOG, `${new Date().toISOString()} [${tag}] ${msg}\n`);
}🧠 Learnings used
Learnt from: efenocchi
Repo: activeloopai/hivemind PR: 98
File: codex/bundle/stop.js:1402-1408
Timestamp: 2026-05-06T23:01:21.197Z
Learning: In `codex/bundle/stop.js` (and `src/hooks/codex/stop.ts`), `forceSessionEndTrigger` is called unconditionally on every Codex `Stop` event by deliberate design. Codex has no separate `session_end` hook — `Stop` fires on every assistant turn and is the only available signal for session completion. Using `tryStopCounterTrigger` instead would silently break skill mining on short sessions (counter never reaches threshold, session closes, worker never fires). The per-projectKey worker lock (`tryAcquireWorkerLock` with a 10-min stale TTL in `src/skilify/state.ts`) makes the over-triggering cheap: subsequent Stops within the same session find nothing past the watermark and exit in <1s. This matches the pattern used by Cursor, Hermes, and Pi agents.
Learnt from: kaghni
Repo: activeloopai/hivemind PR: 97
File: src/hooks/shared/autoupdate.ts:122-146
Timestamp: 2026-05-07T08:28:25.207Z
Learning: In `src/hooks/shared/autoupdate.ts`, the `autoUpdate` function intentionally does NOT include a machine-wide lock/cooldown. An initial draft had a flock-style lock at `~/.deeplake/.autoupdate.lock`, but after a latency-fix refactor to fire-and-forget detached spawn (commit 601aa5c), the lock is released immediately after dispatching the child — defeating the purpose. The proper home for the lock is `src/cli/update.ts:runUpdate()` so the long-running `hivemind update` process owns the lock for its full lifetime. `npm install -g` has its own internal locking that prevents corruption in the meantime, so concurrent invocations are either both no-ops (already at latest) or one wins idempotently. This is tracked as a separate follow-up.
Learnt from: kaghni
Repo: activeloopai/hivemind PR: 0
File: :0-0
Timestamp: 2026-05-18T06:05:29.499Z
Learning: In the activeloopai/hivemind repository, the non-openclaw agent bundles (e.g., `hermes/bundle/skillify-worker.js`, `cursor/bundle/skillify-worker.js`, etc.) intentionally read `process.env.HIVEMIND_*` variables at runtime as a supported tuning surface (e.g., `HIVEMIND_CURSOR_MODEL`, `HIVEMIND_HERMES_PROVIDER`). These bundles are NOT subject to ClawHub static-scan rules — only the openclaw bundle is scanned. Do not flag literal `process.env.HIVEMIND_*` reads in non-openclaw bundles as policy violations.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Good call — fixed in 814a1b4. Added src/skillify/skillopt-env.ts as the single source of truth: SKILLOPT_ENV constants for the fixed names (DISABLED/WORKER/SESSION/SKILL/REACTION/TOOL_USE_ID/AGENT/JUDGE_WINDOW) plus modelEnvNames()/providerEnvName() helpers for the dynamic per-agent overrides. The trigger (writer) and worker (reader) now reference the same constants, so they can't drift. HIVEMIND_SKILLOPT_WORKER lives there too — it's the recursion guard the trigger stamps onto the spawned worker's env, so it belongs with the rest of the hand-off vars. Only doc comments keep the literal names (intentional: they document the user-facing knobs). No behavior change; 47 skillopt unit tests green.
…ts (efenocchi) @efenocchi flagged that the SkillOpt env-var string literals were duplicated between the trigger (which writes them onto the spawned worker's env) and the worker (which reads them back), plus the agent-model overrides — a typo in one place would silently break the hand-off. Introduce src/skillify/skillopt-env.ts as the single source of truth: SKILLOPT_ENV constants for the fixed names (DISABLED/WORKER/SESSION/SKILL/REACTION/TOOL_USE_ID/AGENT/JUDGE_WINDOW) and modelEnvNames()/providerEnvName() helpers for the dynamic per-agent <AGENT>_<ROLE>_MODEL / <AGENT>_PROVIDER overrides. All four call sites now reference it; only doc comments keep the literal names (intentional — they document the knobs for users). No behavior change. tsc clean (pre-existing tree-sitter errors only); 47 skillopt unit tests green.
| export const DEFAULT_JUDGE_WINDOW = 3; | ||
| export function judgeWindow(env: NodeJS.ProcessEnv = process.env): number { | ||
| const n = Number(env.HIVEMIND_SKILLOPT_JUDGE_WINDOW); | ||
| const n = Number(env[SKILLOPT_ENV.JUDGE_WINDOW]); |
There was a problem hiding this comment.
@CodeRabbit can't we use env.SKILLOPT_ENV.JUDGE_WINDOW? why do we have the square brackets?
There was a problem hiding this comment.
@efenocchi Great question! The two syntaxes mean very different things here:
-
env.SKILLOPT_ENV.JUDGE_WINDOW— dot notation treats the part after the dot as a literal property name. This would try to readenv["SKILLOPT_ENV"](i.e. look for a process env var literally namedSKILLOPT_ENV), then access.JUDGE_WINDOWon that result — which is not what we want at all. -
env[SKILLOPT_ENV.JUDGE_WINDOW]— bracket notation evaluates the expression inside the brackets first.SKILLOPT_ENV.JUDGE_WINDOWis a constant imported fromskillopt-env.tswhose value is the string"HIVEMIND_SKILLOPT_JUDGE_WINDOW". So this expands toenv["HIVEMIND_SKILLOPT_JUDGE_WINDOW"], which correctly reads that env variable.
In short: SKILLOPT_ENV.JUDGE_WINDOW is a variable holding the env var name as a string — you need bracket/computed property access to use a variable as a key. Dot notation only works for literal, hard-coded property names. The old code env.HIVEMIND_SKILLOPT_JUDGE_WINDOW worked with dot notation because the key was literally written out; the refactor centralised those string constants into SKILLOPT_ENV, which requires the bracket form.
🧠 Learnings used
Learnt from: efenocchi
Repo: activeloopai/hivemind PR: 98
File: codex/bundle/stop.js:1402-1408
Timestamp: 2026-05-06T23:01:21.197Z
Learning: In `codex/bundle/stop.js` (and `src/hooks/codex/stop.ts`), `forceSessionEndTrigger` is called unconditionally on every Codex `Stop` event by deliberate design. Codex has no separate `session_end` hook — `Stop` fires on every assistant turn and is the only available signal for session completion. Using `tryStopCounterTrigger` instead would silently break skill mining on short sessions (counter never reaches threshold, session closes, worker never fires). The per-projectKey worker lock (`tryAcquireWorkerLock` with a 10-min stale TTL in `src/skilify/state.ts`) makes the over-triggering cheap: subsequent Stops within the same session find nothing past the watermark and exit in <1s. This matches the pattern used by Cursor, Hermes, and Pi agents.
Learnt from: kaghni
Repo: activeloopai/hivemind PR: 97
File: src/hooks/shared/autoupdate.ts:122-146
Timestamp: 2026-05-07T08:28:25.207Z
Learning: In `src/hooks/shared/autoupdate.ts`, the `autoUpdate` function intentionally does NOT include a machine-wide lock/cooldown. An initial draft had a flock-style lock at `~/.deeplake/.autoupdate.lock`, but after a latency-fix refactor to fire-and-forget detached spawn (commit 601aa5c), the lock is released immediately after dispatching the child — defeating the purpose. The proper home for the lock is `src/cli/update.ts:runUpdate()` so the long-running `hivemind update` process owns the lock for its full lifetime. `npm install -g` has its own internal locking that prevents corruption in the meantime, so concurrent invocations are either both no-ops (already at latest) or one wins idempotently. This is tracked as a separate follow-up.
There was a problem hiding this comment.
ok so it's correct but is it a good design overall? Or is it better to have env.key structure (as it was before)? Is it necessarily needed for the purpose of corresponding (last) commit?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
No it was a question to you @coderabbitai
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd "skillopt-env" --exec cat {}Length of output: 1908
@kaghni — great design question. The short answer: the centralized pattern is the right call here specifically, and the file itself explains why.
Why this case justifies the centralization
These env var names are a cross-process contract: the trigger (skillopt-trigger.ts) writes them onto the spawned worker's environment, and skillopt-worker.ts reads them back. If the writer spells "HIVEMIND_SKILLOPT_JUDGE_WINDOW" one way and the worker spells it another, the handoff silently fails — the worker starts with the wrong env and you'd never see a thrown error, just a wrong/missing value.
The skillopt-env.ts comment acknowledges this exact concern (and even notes it was raised in this PR):
"Keeping the literals here stops the writer and reader from drifting (a typo in one place would silently break the hand-off)."
When direct strings are fine vs. when to centralize
| Scenario | Direct env.KEY_NAME |
Centralized env[CONSTANT] |
|---|---|---|
| Used in one place only | ✅ clear, no indirection | Overkill |
| Same-process, same file | ✅ readable | Marginal benefit |
| Cross-process contract (write here, read elsewhere) | ❌ drift risk, silent failures | ✅ correct choice |
| Many callers in different files | ❌ refactoring pain | ✅ single rename |
Was it strictly necessary for this commit?
Technically no — the feature (manifest gating + pinned tool_use_id) works either way. But since this PR introduced the skillopt-env.ts file in response to a review concern, and all the trigger→worker env passing happens in this same PR, it makes sense to apply it consistently while everything is being touched. Bundling it here is cleaner than a follow-up.
🧠 Learnings used
Learnt from: efenocchi
Repo: activeloopai/hivemind PR: 98
File: codex/bundle/stop.js:1402-1408
Timestamp: 2026-05-06T23:01:21.197Z
Learning: In `codex/bundle/stop.js` (and `src/hooks/codex/stop.ts`), `forceSessionEndTrigger` is called unconditionally on every Codex `Stop` event by deliberate design. Codex has no separate `session_end` hook — `Stop` fires on every assistant turn and is the only available signal for session completion. Using `tryStopCounterTrigger` instead would silently break skill mining on short sessions (counter never reaches threshold, session closes, worker never fires). The per-projectKey worker lock (`tryAcquireWorkerLock` with a 10-min stale TTL in `src/skilify/state.ts`) makes the over-triggering cheap: subsequent Stops within the same session find nothing past the watermark and exit in <1s. This matches the pattern used by Cursor, Hermes, and Pi agents.
Learnt from: kaghni
Repo: activeloopai/hivemind PR: 97
File: src/hooks/shared/autoupdate.ts:122-146
Timestamp: 2026-05-07T08:28:25.207Z
Learning: In `src/hooks/shared/autoupdate.ts`, the `autoUpdate` function intentionally does NOT include a machine-wide lock/cooldown. An initial draft had a flock-style lock at `~/.deeplake/.autoupdate.lock`, but after a latency-fix refactor to fire-and-forget detached spawn (commit 601aa5c), the lock is released immediately after dispatching the child — defeating the purpose. The proper home for the lock is `src/cli/update.ts:runUpdate()` so the long-running `hivemind update` process owns the lock for its full lifetime. `npm install -g` has its own internal locking that prevents corruption in the meantime, so concurrent invocations are either both no-ops (already at latest) or one wins idempotently. This is tracked as a separate follow-up.
2 participants
Builds the actual SkillOpt engine on top of the foundation (#230). Stacked: base is
feat/skillopt-foundation.What this does
When a user invokes an org skill and pushes back on its output, SkillOpt judges that one skill against the reaction and — if it failed — improves it and publishes a new version to the org skills table, immediately. No weekly batch, no proposals-for-review, no org-wide sweep.
Pieces
skillstable (not local disk, so a deficient org skill is improvable even if it isn't installed on this machine) and publishversion+1straight back (append-only row; scope→team; the optimizer + triggering user appended tocontributors; name/author kept). No approval gate by design.markSkillPending(PreToolUse, org-skill-gated) opens a K-message window;runEventTrigger(UserPromptSubmit) fires the worker against the reaction. Replaces the weekly SessionStart throttle entirely. Shared hook helper so each agent wires it with one swallowed call.Verification
tscclean (only the pre-existing tree-sitter native-dep errors).skilloptorg: invoke a bad PostHog skill → profane pushback ("ah man you fucked it up again") → LLM judge confirms failure (a regex would miss it) → publishes v2 with correct provenance (scope=team,contributors=["synthbot","kamo.aghbalyan","skillopt"], rule rewritten "Do NOT mock the client").agentModel: claude (--tools ""), codex (-s read-only), hermes (Bedrock inference-profile), cursor (default flags), pi (OpenRouter + Bedrock).Known follow-ups (NOT in this PR — see the investigation comment below)
Skilltool_use; whether the other agents surface skill invocation as a detectabletool_name="Skill"event (vs. context-injection) is an open empirical question. Cursor is out (no native skill discovery). The scorer already works on all of them; only the auto-trigger is claude-only today.skillopt-worker.tsusesexecSync(command -vagent-bin resolution); if SkillOpt ever ships inside the openclaw bundle, that +spawn/fetchwould trip ClawHub's static scanner (the constraintgate-runner.tsalready avoids). Needs the same createRequire/no-PATH treatment or exclusion from that bundle.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests