fix(orca-skills): let Orca own its own skills — enableOrca + perSkillLinks - #49
Conversation
…Links Orca has reported all three vendored skills as "Skipped — the copy here doesn't match the official version" since they were vendored, and the earlier diagnosis in CREDITS.md (the tree is ahead of the binary; pin the revision) was wrong. Reading the app's scanner settles it: observeSkillPackage() throws skill-package-link on any file with nlink != 1, the catch reports the skill as status "unrecognized", and that is the row in the dialog. Store optimisation hardlinks store files — these sat at nlink 5-7 — so no revision pin can clear it. computer-use and orchestration were byte-identical to the installed app's manifest and were flagged just the same. Store files are mode 444 besides, so classifyHomeSkillTopology would mark them read-only even past the byte check. The install location is the problem, so hand it to Orca: - enableOrca (default off) makes the vendored leaves opt-in rather than merged unconditionally. On a host with the Orca app, Orca installs and updates its own copies; turn the flag on only where nothing else provides them. - perSkillLinks renders ~/.agents/skills as a real directory of per-skill symlinks instead of one directory symlink, so `orca skills install` (npx skills add) has somewhere writable to own computer-use/, orca-cli/ and orchestration/ as real nlink=1 directories. A real directory on a skill's name is left alone and reported, never clobbered, and pruning is limited to symlinks pointing into this module's own tree. - packages.skills-with-orca builds the merged tree for the opt-in case. Docs follow the change: a new AGENTS.md section on the Orca tree, a README section, and CREDITS.md's revision guidance corrected — a manifest match is still how the vendored bytes are verified, but it was never what the dialog was measuring. The fan-out section also drops a stale description of a per-skill HM layout this host has not used since the mutable pointer landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M94JK8t6zguGCTDP7udiUv
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 655d652616
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Under perSkillLinks the entry below renders ~/.agents/skills | ||
| # as a real directory, so the pointer stops at <stateDir> and | ||
| # this link is not made. | ||
| + lib.optionalString (!cfg.perSkillLinks.enable) '' |
There was a problem hiding this comment.
When both mutablePointer.enable and perSkillLinks.enable are true, this conditional suppresses only the final ln; the earlier block at lines 394–397 still moves any real ~/.agents/skills directory aside on every activation. After the first activation, that directory is precisely the canonical directory containing Orca-owned real skills, so the next rebuild removes those skills from every harness path and recreates the canonical directory without them. Gate the move-aside block on !cfg.perSkillLinks.enable as well.
AGENTS.md reference: AGENTS.md:L286-L291
Useful? React with 👍 / 👎.
| "$src"/*) | ||
| [ -d "$src/$(basename "$l")" ] || $DRY_RUN_CMD rm -f "$l" |
There was a problem hiding this comment.
When perSkillLinks.enable is used without mutablePointer and a package update removes a skill, its existing link still targets the previous immutable store path while $src now names the new store path. The case therefore does not match, so the removed skill remains visible indefinitely and keeps the old package reachable; pruning needs to recognize links created from prior Construct package generations rather than only the current $src.
AGENTS.md reference: AGENTS.md:L290-L291
Useful? React with 👍 / 👎.
#49 made the ~/.agents/skills link in the pointer entry conditional by appending a lib.optionalString to the activation script — but appended it to the RESULT of lib.hm.dag.entryAfter, which is the DAG entry set, not the script. Any consumer with mutablePointer on then failed to evaluate: error: cannot coerce a set to a string: { after = «thunk»; before = [ ]; data = «thunk»; } The two string literals now compose inside the entryAfter call. Caught by evaluating home.activation from a real Home Manager configuration; `nix flake check` cannot see it, since this flake has no Home Manager to instantiate the module against. Claude-Session: https://claude.ai/code/session_01M94JK8t6zguGCTDP7udiUv Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Orca has listed
computer-use,orca-cliandorchestrationas Skipped — the copy here doesn't match the official version in Settings → Update skills since they were vendored in #44. The diagnosis recorded inCREDITS.md(the tree is ahead of the binary, so pin the revision the app expects) is wrong, and #46's re-pin did not clear it.What the app actually does
From the installed Orca's own scanner (
resources/app.asar, v1.4.186):Store optimisation hardlinks identical files, so every store-served
SKILL.mdhasnlink > 1— ours measured 7, 5 and 7 at~/.codex/skills/*/SKILL.md. The throw is caught and reported asunrecognized, which is exactly that dialog row. The scanner never reaches the byte comparison, which is whycomputer-useandorchestrationwere flagged while being byte-identical to the installed app's manifest (c4a11596…rev 8,9ca22813…rev 28, both confirmed againstcurrent-manifest.jsonand upstreammain). Store files are also mode 444, soclassifyHomeSkillTopologymarks the pathread-onlyeven past the byte check.No revision pin can fix this. The install location is the problem.
The change
enableOrca(default off) — the vendored Orca leaves become opt-in instead of merged unconditionally. On a host running Orca, Orca installs and updates its own copies (orca skills install→npx skills add); turn the flag on only where nothing else provides them (no Orca app, air-gapped host, container image).perSkillLinks.enable— renders~/.agents/skillsas a real directory of per-skill symlinks rather than one directory symlink, so an installer that owns its own skills has somewhere writable to put them. A real directory on a skill's name is left alone and reported, never clobbered; pruning is limited to symlinks pointing into this module's own tree. UndermutablePointerthe links go through…/current, so pointer swaps still work (a skill the swapped-in tree adds is linked at the next activation).packages.skills-with-orcabuilds the merged tree for the opt-in case.mkSkillsgainsorca ? false;baseSourcesbecomes a function of that flag.Docs
New
AGENTS.mdandREADME.mdsections for the Orca tree,orca-skills/README.mdupdated, andCREDITS.md's revision guidance corrected — the manifest check is still how the vendored bytes are verified, it was just never what the dialog was measuring. The fan-out section inAGENTS.mdalso loses a stale description of a per-skill HM layout this host stopped using when the mutable pointer landed (#39).Verified
nix flake check --no-buildpasses;nix build .#skillshas no Orca leaves,.#skills-with-orcahas all three.reuse lintand.github/check-license-files.pypass.Follow-up (not in this PR)
Bravais sets
perSkillLinks.enable = trueafter this merges andnix flake update construct; thenorca skills install --skill computer-use --skill orca-cli --skill orchestrationhands the three to Orca for good.