Skip to content

fix(skills): substitute {{SKILLS_DIR}} in materialized skill files#43

Merged
Alezander9 merged 3 commits intomainfrom
fix/skills-dir-template-substitution
May 9, 2026
Merged

fix(skills): substitute {{SKILLS_DIR}} in materialized skill files#43
Alezander9 merged 3 commits intomainfrom
fix/skills-dir-template-substitution

Conversation

@Alezander9
Copy link
Copy Markdown
Member

What

browser_execute's tool description runs .replaceAll("{{SKILLS_DIR}}", impl.skillsDir) so the description sent to the LLM resolves correctly — but BROWSER.md on disk still contained literal {{SKILLS_DIR}} strings. The agent reads BROWSER.md via the standard read tool and was therefore told to read "{{SKILLS_DIR}}/cloud-browser.md" verbatim, which fails.

Surfaced during v0.1.0 Windows binary testing — the agent successfully read BROWSER.md (because the tool description gave it a resolved absolute path), then quoted the literal placeholder back when trying to follow internal cross-references.

How

Materialize skills to <dataDir>/skills/ in both dev and compiled modes and replace {{SKILLS_DIR}} in *.md files with the absolute target path during materialization.

  • packages/bcode-browser/src/skills.ts: unified materializeFromSource (dev) and materializeFromEmbed (compiled) with shared writeSkillFile doing the substitution. Cheap byte-level pre-check skips the UTF-8 decode for files that don't need it. Sentinel = "<bundleHash>:<target>" so a target change (or content change in dev) triggers re-extraction. dev mode hash is computed over the source tree, matching the build-time hash shape.
  • packages/opencode/src/agent/agent.ts: comment refresh — the browserSkillsGlob allow-list now matches in dev too (it always would have, since Wildcard.match treats * as .* greedy, but the previous comment said "no-op in dev").
  • packages/bcode-browser/test/skills.test.ts (new): three regression tests — substitution lands, sentinel makes second resolve idempotent (no rewrite), different dataDirs get correctly substituted with their own paths.

Verification

  • bun typecheck clean from both packages/bcode-browser/ and packages/opencode/.
  • bun test from packages/bcode-browser/: 9 pass + 5 chrome-gated skip (was 6 + 5; the three new skills tests bring the always-on count to 9). All pre-existing tests still pass.
  • End-to-end check: materializing into a fresh tmpdir produces a BROWSER.md with zero literal {{SKILLS_DIR}} matches and absolute-path cross-references that point at the materialized directory.

Notes

  • Single-package change in the Green zone (packages/bcode-browser/ plus a comment-level touch in opencode); zero upstream wire-protocol surface affected.
  • The materialize-in-dev change is a behavior shift (skills now also live under <dataDir>/skills/ during bun --cwd packages/opencode dev instead of being read from the worktree). The sentinel ensures dev edits land on the next launch. Trade-off accepted for uniform substitution semantics; alternative was carrying two divergent code paths.

The browser_execute tool description ran .replaceAll("{{SKILLS_DIR}}",
impl.skillsDir) so the description sent to the LLM resolved correctly,
but the BROWSER.md skill file on disk still contained literal
{{SKILLS_DIR}} strings. The agent reads BROWSER.md via the standard
`read` tool and was therefore told to `read "{{SKILLS_DIR}}/cloud-browser.md"`
verbatim, which fails.

Materialize skills to <dataDir>/skills/ in both dev and compiled modes
and replace {{SKILLS_DIR}} in *.md files with the absolute target path
during materialization. Sentinel = "<bundleHash>:<target>" so a target
change (or content change in dev) triggers re-extraction.

Surfaced during v0.1.0 Windows binary testing.
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

The first pass added unneeded machinery: a separate dev/compiled
materialize codepath each, hand-rolled byte-search to avoid UTF-8
decode, sentinel + content hash for warm-launch skip. The skills tree
is ~18 small markdown files (~100KB total) — rewriting them on every
launch is microseconds and removes the cache-invalidation surface.

Down to one straight-line resolver: read every file as text, write to
<dataDir>/skills/<rel> with  -> target path. In-process
cache dedupes concurrent calls within one launch. Tests trim to the
two assertions that actually exercise behavior (substitution lands;
different dataDirs get their own paths).
@Alezander9
Copy link
Copy Markdown
Member Author

Pushed 46ca8c8a collapsing the diff. Down to net ~50 added lines.

Why the first pass was bloated. I treated the materializer like it had to be production-grade — separate dev/compiled codepaths, hand-rolled byte-search to avoid UTF-8 decode on a small minority of files, sentinel + content hash to skip warm launches. None of it earns its keep: skills are ~18 small markdown files totalling ~100KB; rewriting them on every launch is microseconds and removes the invalidation surface entirely.

What it is now. One straight-line resolver: readAllSkills() returns {rel: text} from either the build-time embed or the worktree; resolveSkillsDir writes each file to <dataDir>/skills/<rel> with {{SKILLS_DIR}} substituted. In-process cache still dedupes concurrent calls within a launch. Tests trim to the two that actually exercise behavior (substitution lands in BROWSER.md; different dataDirs get their own substituted paths).

Verification unchanged. bun typecheck clean, bun test from packages/bcode-browser/ is 8 pass + 5 chrome-gated skip (lost one trivial idempotency test that was tied to the removed sentinel). End-to-end against tmpdir still produces zero literal {{SKILLS_DIR}} matches.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/bcode-browser/src/skills.ts">

<violation number="1" location="packages/bcode-browser/src/skills.ts:52">
P2: This now rewrites the entire skills directory on every launch because the persisted sentinel/idempotence guard was removed. That adds unnecessary startup I/O and disk churn for unchanged skills.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread packages/bcode-browser/src/skills.ts Outdated
Cubic flagged that the previous simplification rewrites the entire
skills tree on every launch — small but unnecessary. Compiled binaries
already carry a precomputed build hash from script/embed-skills.ts;
use it. Warm launches now read one ~80-byte sentinel file at
<target>/.bcode-build and short-circuit (no skills content read, no
writes) when it records the current <buildHash>:<target>.

Dev launches keep the always-rewrite behavior so editor saves to
worktree skill files land on the next bun run dev without an
invalidation step. The "dev" stamp never matches a written sentinel
because we never write one in dev — single check, no special case.

84 lines total (was 67). Brings cubic's concern + admin's elegance
ask to a stable equilibrium.
@Alezander9
Copy link
Copy Markdown
Member Author

Addressed cubic's startup-I/O finding in 3e1373af5. Cubic was right.

The fix. Compiled binaries already carry a precomputed build hash from script/embed-skills.ts, sitting on the embedded bcode-skills.gen.ts. Use it. Warm launches now read one ~80-byte sentinel at <target>/.bcode-build, compare it to <buildHash>:<target>, and short-circuit on hit — no skills content read, no writes.

Dev launches keep always-rewriting, deliberately. bun run dev is the iteration loop; if a developer edits a skill file they want it picked up on next launch without an invalidation step. The dev stamp is "dev", we never write that sentinel, so dev never matches and never short-circuits — single code path, no special case.

Cost shape now:

  • First launch on a fresh dataDir, or first launch after a bcode upgrade: read all skills + write all + write sentinel.
  • Every subsequent launch on the same install: one Bun.file().text() of the sentinel.
  • Dev launch: read all + write all (no sentinel involvement).

Lesson. I overcorrected after the diff-bloat feedback — interpreted "more elegant" as "strip everything that isn't the literal substitution". The sentinel was bloated in implementation (hand-rolled SHA-256 over sorted entries with NUL separators, two parallel codepaths, byte-search optimization), not in concept. Removing the concept threw out the warm-launch optimization that compiled binaries actually benefit from. Back to the sentinel concept with a minimal impl: the build hash is already computed at build time, so it's a one-line read + compare.

Verification. bun typecheck clean. bun test from packages/bcode-browser/: 8 pass + 5 chrome-gated skip. End-to-end against tmpdir still produces zero literal {{SKILLS_DIR}} matches; in dev mode the sentinel is correctly absent.

Net branch diff: 49 lines in skills.ts, 44 in tests, 8 in agent.ts comment refresh.

@Alezander9 Alezander9 merged commit 50afa3b into main May 9, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant