feat(skills): skill-relative file read + multi-language script execution (closes #251)

[initializ-mk]

Closes #251.

Problem

A SKILL.md body references sibling files by a path relative to the skill's own directory ("read reference/runbook.md", "run scripts/check.py"). Those files ship in skills/<skill>/ and reach the agent, but nothing resolved a skill-relative reference: cli_execute and the skill-script executor run with CWD = agent root, and only .sh ## Tool: entries were runnable. A reasonable skill silently failed at runtime.

Change (approach 1 — skill-scoped tools)

• read_skill gains an optional file argument — reads a file relative to the skill's directory (confined; no ../absolute escape) instead of SKILL.md. Handles "read reference/runbook.md".
• New run_skill_script builtin — executes a bundled script by path relative to the skill dir, picking the interpreter by extension (.sh→bash, .py→python3, .js→node) and running it with the skill directory as CWD so the script's own relative reads resolve. JSON args are passed as the script's first positional argument ($1). Path-confined; the subprocess inherits the same egress proxy + env passthrough as cli_execute/skill scripts. Registered independently of cli_execute (a skill may ship only non-.sh helpers).
• Shared helpers builtins.SkillDir + builtins.SafeSkillJoin resolve a skill's loadable name → directory and confine a relative path, used by both.
• skillEntryHasScript stays .sh-aligned by design: a .py/.js tool is now runnable via run_skill_script while correctly staying in the catalog provides: list — closing the fix(skills): align skill catalog names with read_skill resolution #250 "falls between not-callable and in-provides" note without a code change (comment updated).

Acceptance criteria

• Relative file reference reads from the skill directory (read_skill file).
• Relative script executes with the correct interpreter by extension, CWD = skill dir, inheriting egress proxy + env passthrough.
• Path resolution confined to the skill directory (traversal / absolute-escape rejected), with tests.
• skillEntryHasScript / provides: classification reconciled (rationale documented; run_skill_script makes py/js runnable so provides: is correct as-is).
• Docs: Skill Designer prompt (forge-ui/skill_builder_context.go) + docs/skills/writing-custom-skills.md explain skill-relative references and runnable languages.
• Tests: read + execute for shell/python/javascript + traversal guard.

Tests

run_skill_script_test.go executes real shell/python/javascript scripts, each proving CWD=skill dir by reading a sibling reference/data.txt, plus traversal/absolute-escape rejection, unsupported-extension (.ts) error, and unknown-skill. read_skill_test.go adds the file-arg read + its traversal guard and the SkillDir/SafeSkillJoin helpers.

Full forge-core/tools/builtins, forge-cli/tools, and forge-cli/runtime suites green; gofmt + golangci-lint clean.

Notes / scope

• TypeScript is intentionally unsupported (node can't run raw .ts) — ship compiled .js. Error is explicit.
• This is the runtime resolution layer. Registering .py/.js ## Tool: entries as first-class callable tools (Model A) remains out of scope — run_skill_script (Model B) covers the referenced-by-path case the issue describes.

[naveen-kurra]

Review — 5 findings, 1 security, 4 cleanup / correctness / UX

Read the diff line-by-line, cross-checked path confinement, subprocess wire, and JSON return-value shape against the surrounding code, reproduced two of the concerns locally. This is a security-sensitive feature (new script executor) — bulk of my time went into the path guard.

Ranked most severe first:

1. forge-core/tools/builtins/read_skill.go:497 — SafeSkillJoin doesn't resolve symlinks, so a bundled symlink escapes confinement

SafeSkillJoin guards with filepath.Rel + textual .. checks:

full := filepath.Join(skillDir, rel)
within, err := filepath.Rel(skillDir, full)
if err != nil || within == ".." || strings.HasPrefix(within, ".."+string(filepath.Separator)) {
    return "", errors.New("path escapes the skill directory")
}
return full, nil

That is a textual guard — it never touches the filesystem. Both consumers then use symlink-following syscalls:

• read_skill.go line 435: os.ReadFile(full) — follows symlinks.
• run_skill_script.go line 185: os.Stat(full) — follows symlinks; then exec.Run(bash, ["link", …]) with CWD = skill dir, and bash resolves the link.

Attack: a skill package (which ships whatever the skill author writes via COPY . .) contains skills/owl/leak as a symlink to /etc/shadow / /root/.ssh/id_rsa / any host file readable by the container UID. SafeSkillJoin(dir, "leak") returns <dir>/leak — filepath.Rel says leak, no .., no absolute — allowed. os.ReadFile follows the symlink and returns the target contents. Same for run_skill_script with a link pointing at any executable script the container can reach.

Attacker trust model: yes, a malicious skill author can already ship arbitrary payloads. But symlinks are stealthier (they don't show up in cat review) and turn the tools into a general filesystem-read/execute primitive limited only by container FS perms. The tests only cover ../.., .., and /etc/hostname — no symlink case.

Fix: after filepath.Join, call filepath.EvalSymlinks(full) and re-run the Rel check against the resolved path. Reject if the resolved path is outside skillDir. Add a symlink test case.

2. forge-cli/tools/run_skill_script.go:210 — error return uses %q, which produces Go-quoted strings (\xff escapes) that fail JSON parsing

return fmt.Sprintf(`{"error": %q, "output": %q}`, runErr.Error(), truncate(out, 8192)), nil

Go's %q verb escapes non-printable bytes as \xNN. JSON accepts only \uNNNN. Reproduced locally:

fmt.Sprintf(`{"out": %q}`, "\xff\xfe hello")
  → {"out": "\xff\xfe hello"}
json.Unmarshal(...) → invalid character 'x' in string escape code

Any script that emits raw bytes on stderr (a binary tool, a curl -s | head -c1 sort of pipeline, or just a badly-truncated UTF-8 sequence at the 8 KiB boundary from truncate) returns a malformed JSON string. The Anthropic/OpenAI tool-result parser then either drops the response or errors.

Fix: build the return with json.Marshal(map[string]any{"error": runErr.Error(), "output": truncate(out, 8192)}). Handles all encoding edge cases and the JSON encoder replaces invalid UTF-8 with U+FFFD by default.

Note: line 191 (return fmt.Sprintf(\{"error": %q}`, ierr.Error())`) has the same shape but the input is a Go-crafted error string with only ASCII → not exploitable in practice.

3. forge-cli/tools/run_skill_script.go:138 — execution timeout hardcoded at 120s with no override; sibling cli_execute accepts a config knob

func NewRunSkillScriptTool(workDir, proxyURL string, envVars []string) *RunSkillScriptTool {
    return &RunSkillScriptTool{ …, timeout: 120 * time.Second }
}

cli_execute — the peer tool — accepts TimeoutSeconds from the tool config (forge-cli/tools/cli_execute.go:22 and :558). A skill script that legitimately runs longer than 120s (large data process, slow HTTP fetch, model inference) gets SIGKILLed with no way for the operator to lift the cap. This is a design inconsistency, not a bug per se, but users will hit it and there's no yaml knob to fix it.

Fix: thread a TimeoutSeconds int through NewRunSkillScriptTool (or a RunSkillScriptToolConfig struct), plumb from a forge.yaml block, and default to 120.

4. forge-core/tools/builtins/read_skill.go:457 — SkillDir duplicates the existing ReadSkillTool.resolvePath resolution algorithm

SkillDir and resolvePath both implement the same three-step lookup: (1) direct subdir with hyphen variant, (2) flat <name>.md for resolvePath (skipped by SkillDir since dir-only), (3) frontmatter-name normalized index. If someone ever fixes a bug in one — a new normalization rule, another separator variant, a case-sensitivity fix — they'll forget the other. Two divergent implementations of the same resolution over time.

Fix: keep SkillDir as the primitive; refactor resolvePath to build on top of it (e.g. if p := filepath.Join(SkillDir(...), "SKILL.md"); fileExists(p) { return p } after also trying <name>.md at the flat-skill layer).

5. forge-cli/tools/run_skill_script.go:157 + line ~195 — schema declares args: {type: object} but no runtime type check; non-object args land in the script's $1 verbatim

"args": {"type": "object", "description": "Optional JSON object …"}

jsonArgs := "{}"
if len(input.Args) > 0 && string(input.Args) != "null" {
    jsonArgs = string(input.Args)
}

If the LLM sends args: "hello" (a string) or args: 42 (a number) or args: [1,2] (an array), the code doesn't validate; the script's json.loads(sys.argv[1]) gets a non-dict and either crashes or misinterprets. The test scripts all send an object so this stays uncaught.

Fix: after unmarshal, peek the RawMessage's first non-whitespace byte. If it's not {, return {"error": "args must be a JSON object"}. Cheap defense.

---

Not blockers, but worth noting:

• Path arg passed to bash/python/node is input.Path verbatim — since it's SafeSkillJoin-validated and exec-based (no shell), no injection. Correct.
• interpreterForScript accepts .SH, .PY via strings.ToLower — matches builder prompt guidance.
• Test TestRunSkillScript_UnsupportedExt — a .ts file returns the clear error. Nice touch.
• skillEntryHasScript narrative in forge-cli/runtime/runner.go:3261 is now internally consistent — the comment now correctly explains why .py/.js scripts staying in provides: is not a gap.

Approving with the security fix + the JSON-safety fix requested (#1 and #2). #3–#5 are follow-ups.

[initializ-mk]

Thanks — genuinely useful review, especially the symlink attack path. Pushed 851b0bc addressing the two requested fixes plus the cheap #5:

#1 symlink escape (security). SafeSkillJoin now resolves symlinks — EvalSymlinks on the skill dir plus the deepest-existing part of the target — and re-checks containment against the resolved root, so a bundled skills/owl/leak -> /etc/shadow (leaf) or an intermediate symlinked directory is rejected. A symlink that stays inside the skill is still allowed. Non-existent targets anchor to the resolved root so a symlinked temp root (macOS /var -> /private/var) doesn't false-reject. New test covers leaf-escape, intermediate-escape, and in-skill-ok.

#2 JSON-safe output. All dynamic returns in run_skill_script now go through json.Marshal (jsonObj/jsonError) instead of fmt %q. New test runs a python script that emits \xff\xfe and exits non-zero, then asserts the tool result json.Unmarshals cleanly.

#5 args must be a JSON object. Non-object args (string/number/array/bool) are rejected before exec with a clear error; test covers all four shapes.

Deferred as you scoped them:

• Add Tavily web search integration and Telegram typing indicator #3 timeout knob — agreed; needs a forge.yaml block mirroring cli_execute's TimeoutSeconds. Follow-up.
• Tavily web search, Bubble Tea wizard, and bug fixes #4 dedup SkillDir/resolvePath — one caveat: resolvePath also resolves flat skills/<name>.md skills by their frontmatter name (via buildNameIndex, which indexes flat files), whereas SkillDir is dir-only. A naive fold would regress flat-skill-by-frontmatter-name resolution, so this needs a shared helper rather than layering one on the other. Follow-up.

go test ./forge-core/tools/... ./forge-cli/tools/... + golangci-lint clean.

[initializ-mk]

The two non-blocking items deferred from this review (per commit 851b0bc) are now tracked as backlog issues:

• feat(skills): make run_skill_script execution timeout configurable #272 — make run_skill_script execution timeout configurable (deferred item Add Tavily web search integration and Telegram typing indicator #3)
• refactor(skills): unify SkillDir / resolvePath skill-name→path resolution (flat-skill frontmatter caveat) #273 — unify SkillDir / resolvePath skill-name→path resolution, incl. the flat-skill frontmatter caveat (deferred item Tavily web search, Bubble Tea wizard, and bug fixes #4)

Both are follow-ups on top of this PR and depend on it merging first. The three blocking review points (symlink confinement, JSON-safe errors, args validation) are addressed in 851b0bc with tests.