feat(cli): infer a metadata.forge block when importing a plain skill folder (#412) - #415
Conversation
…folder (#412) Real customer skills usually ship a plain SKILL.md (name + description only, no metadata.forge), so `forge skills import` vendored them but wired nothing — the operator had to hand-add requires.bins/egress/env. Infer it. When the imported SKILL.md has no metadata.forge: - Derive requires.bins from script interpreters (.py→python3, .js→node; requirements.txt→python3+pip) — deterministic, high confidence. - Scan scripts for http(s) hosts (→ candidate egress_domains) and env reads (os.environ/os.getenv/process.env → candidate requires.env) — HEURISTIC, reported for review, never auto-declared (a templated host or a URL in a comment must not silently widen egress). - By default print a paste-ready suggested block (bins active; egress/env as commented candidates). - `--write-forge-meta` injects requires.bins into the vendored SKILL.md frontmatter — only when it has no metadata: block at all (a second metadata: key would be invalid YAML → skip with a merge-by-hand message). Egress/env are not injected. Clears the now-stale "python3 not declared" follow-up after a successful write. Wired on both `forge skills import` and `forge init --from-skill-dir`. Verified against a real customer skill (ai-budget-increase-evaluator, plain SKILL.md + 2 stdlib Python scripts, Datadog via connector): default prints `requires.bins: [python3]`; `--write-forge-meta` injects it and the frontmatter still parses. Tests: interpreter/egress/env inference, suggested- block shape (commented candidates), injection into plain frontmatter with body intact, skip-when-forge-meta-exists, skip-when-any-metadata-key-exists. Docs: skills-cli.md, cli-reference.md, forge.md.
…ference (#412) Extend requires.env candidate detection to .sh/.bash scripts: capture $VAR / ${VAR} reads of uppercase names, excluding (a) names assigned locally in the script (NAME=, export, local, read, for … in) so a local var isn't reported as an env requirement, and (b) common shell/OS variables (PATH, HOME, PWD, USER, IFS, …). Positionals ($1) and short names are already excluded by the ≥3-char uppercase pattern. Env candidates stay review-only (commented, never auto-declared), so the noisier shell surface can't silently widen anything. Tests: shellEnvReads filtering (required vars kept; locals/common/positional dropped) + full-inference path for a shell-backed skill (env + egress candidates surface, no requires.bins since bash is built in). Docs updated.
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review — infer a metadata.forge block for a plain skill folder (closes #412)
Clean, security-conscious extension of the #405/#406 import path. Traced to the branch source; CI all green (Build ×6, Integration, Lint, Test, Doc-link).
Verified — the "never silently widen egress" invariant holds end to end:
- Inferred egress hosts are emitted as commented YAML candidates,
injectForgeMetaBinswritesrequires.binsonly, andinferred.EgressCandidatesis never passed toMergeEgressDomains(that call usesreqInfo.EgressDomains— the skill's own declared egress, empty for a plain skill). So a plain import merges no egress. - Inference runs only when the SKILL.md has no
metadata.forge, so it never overrides an author's explicit config.
Frontmatter injection is well-guarded — three independent skips (already-has-forge via the parser; no --- frontmatter; any top-level metadata: key via ^metadata:), so it can't produce a duplicate-key/invalid-YAML double block. I walked the string surgery: from valid frontmatter, splicing a column-0 metadata: block after the existing body (with the "\n" separator) is always valid YAML, and the block-scalar/no-trailing-newline edges are handled. Go's RE2 regexes are linear-time (no ReDoS), and scanning reads only the already-confined vendored scripts.
Two optional hardening notes inline — nothing blocking.
| front := content[:len("---\n")+end] | ||
| tail := content[len("---\n")+end:] // starts with "\n---" | ||
| newContent := front + "\n" + block.String() + strings.TrimPrefix(tail, "\n") | ||
| if err := os.WriteFile(path, []byte(newContent), 0o644); err != nil { |
There was a problem hiding this comment.
Optional hardening — atomic + re-parse the write. Two small robustness ideas for the one place this feature modifies the vendored SKILL.md:
- Non-atomic write.
os.WriteFilehere can leave a truncated/corrupted SKILL.md if it fails mid-write. The repo already has the atomic pattern inMemoryStore.Save(temp → fsync → rename) — worth reusing, since this rewrites a file the import just vendored. - No re-parse after write. From valid frontmatter the splice is provably valid YAML, so this is belt-and-suspenders — but a quick
parser.ParseWithMetadataonnewContent(warn / don't write if it fails) would keep the injection safe against unusual frontmatter shapes and any future change to the splicer, turning "should be valid" into "verified valid before we overwrite the author's file."
Neither blocks — the guards already prevent the realistic failure modes.
| } | ||
| } | ||
| if len(m.EgressCandidates) > 0 { | ||
| b.WriteString(" # egress hosts detected in scripts — uncomment the ones it actually calls:\n") |
There was a problem hiding this comment.
Verified this is the load-bearing part of the "never auto-widen egress" stance: egress hosts scanned from scripts are surfaced only as commented candidates here, and EgressCandidates never reaches MergeEgressDomains — so a templated host or a URL in a comment can't become a real allowlist entry. Nice: requires.bins (the deterministic, safe-to-write part) is the only thing --write-forge-meta injects. 👍
…(PR #415 review) Two optional-but-worth-it robustness notes from review, on the one place this feature mutates the vendored SKILL.md: - Re-parse before overwrite: verify the spliced result parses (with the forge block present) via parser.ParseWithMetadata before writing — turns "the splice should be valid" into "verified valid," safe against unusual frontmatter shapes or a future splicer change. Skips with a by-hand message if it wouldn't parse. - Atomic write: temp file in the same dir → fsync → rename, so a mid-write failure can't leave a truncated SKILL.md. Mirrors the repo's MemoryStore atomic-save pattern. Test: atomicWriteFile writes/overwrites correctly and leaves no .tmp residue. The existing write test now exercises both new steps end-to-end.
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Re-review — hardening commit ab1060b. Both optional notes addressed; CI all green (Build ×6, Integration, Lint, Test, Doc-link).
Both were non-blocking suggestions, and both are handled cleanly:
- Re-parse before write ✅.
injectForgeMetaBinsnow runsparser.ParseWithMetadata(newContent)and only writes if the spliced result parses and actually contains the forge block (meta.Metadata["forge"] != nil) — so it verifies the injection took effect, not merely that it parses, and skips with a clear "add it by hand" message otherwise. Turns "provably valid from valid input" into "verified valid before we overwrite the author's file." - Atomic write ✅. New
atomicWriteFile=CreateTempin the same dir → Write → Sync → Close → Chmod → Rename, withdefer os.Removecleanup. Correct temp+fsync+rename pattern: same-dir so the rename is atomic, perms set before the rename, no temp-file leak on any error path.TestAtomicWriteFilecovers create + atomic overwrite + no-leftover-temps.
Core inference/injection logic unchanged and verified last round; the egress-never-auto-widened invariant still holds. LGTM — good to merge from my side.
…(PR #415 review) Two optional-but-worth-it robustness notes from review, on the one place this feature mutates the vendored SKILL.md: - Re-parse before overwrite: verify the spliced result parses (with the forge block present) via parser.ParseWithMetadata before writing — turns "the splice should be valid" into "verified valid," safe against unusual frontmatter shapes or a future splicer change. Skips with a by-hand message if it wouldn't parse. - Atomic write: temp file in the same dir → fsync → rename, so a mid-write failure can't leave a truncated SKILL.md. Mirrors the repo's MemoryStore atomic-save pattern. Test: atomicWriteFile writes/overwrites correctly and leaves no .tmp residue. The existing write test now exercises both new steps end-to-end.
1 participant
Implements #412 — infer a
metadata.forgeblock when importing a plain skill folder.Validated against a real customer skill (
ai-budget-increase-evaluator): a plainSKILL.md(onlyname+description), two stdlib Python scripts, Datadog access via a connector. Before this, import vendored it and warned thatpython3wasn't declared; now it infers and (optionally) writes it.Behavior
When the imported
SKILL.mdhas nometadata.forgeblock:requires.bins— derived from script interpreters (.py→python3,.js→node; arequirements.txt→python3+pip). Deterministic, high-confidence.egress_domains— http(s) hosts scanned from scripts, reported as candidates.requires.env— env reads (os.environ/os.getenv/process.env), reported as candidates.--write-forge-meta: injectsrequires.binsinto the vendoredSKILL.mdfrontmatter — only when it has nometadata:key at all (a secondmetadata:would be invalid YAML → skips with a merge-by-hand message). Egress/env are not injected. Clears the now-stale "python3 not declared" follow-up after a write. Wired on bothforge skills importandforge init --from-skill-dir.Real-skill demo
Design stance (from #412)
Print by default; inject only under the flag; never auto-widen egress — the heuristic parts (egress/env) are always review candidates, only the deterministic interpreter part is written.
Tests
Interpreter/egress/env inference; suggested-block shape (commented candidates); injection into plain frontmatter with the body intact + still parses; skip when
metadata.forgeexists; skip when anymetadata:key exists (no invalid-YAML double key).gofmt,golangci-lint(0 issues), fullcmdsuite pass. Docs:skills-cli.md,cli-reference.md,forge.md.Closes #412.