Add skill-runtime evals: execute an exported SKILL.md in a real runtime - #70
Open
Karim Mehalebi (karimad) wants to merge 6 commits into
Open
Add skill-runtime evals: execute an exported SKILL.md in a real runtime#70Karim Mehalebi (karimad) wants to merge 6 commits into
Karim Mehalebi (karimad) wants to merge 6 commits into
Conversation
Addresses the gap in microsoft#55 and adilei's microsoft#53 feedback: evals/skillbuilder/ only scores the builder's proposed PLAN TEXT (tool mentions, structure) — it never actually generates -> exports -> loads -> executes a skill in a target runtime and scores the resulting behavior. Adds evals/skill-runtime/, using only what's already required for the rest of this eval suite (a signed-in Copilot CLI, the already-vendored @github/copilot-sdk) — no new dependency, no new credential, no `claude` CLI: - fixtures/regenerate.ts + fixtures/github-issue-triage-agent-skill/SKILL.md — a real, already-built "agent-skill" artifact (ships as a static fixture, not regenerated per run, so a runtime-eval failure points at the runtime layer, not builder variance — same principle the rest of this suite already follows). - mocks/gh — a real mock `gh` executable (checked in, mirrors the existing evals/mocks/*.html pattern for a CLI instead of a web page). Returns two issues: #214 (unassigned bug, not yet triaged) and #220 (unassigned bug, already labeled needs-info) — a correct run must act on #214 only. - bash-tool.ts — the one capability the runtime session gets: a custom "Bash" tool scoped to a mocked PATH, so every invocation is captured for scoring. - run.ts — generates a fresh Copilot session (skillDirectories pointed at the fixture, MCP tools excluded so the built-in GitHub connector can't bypass the mock via the real GitHub API), sends the task, and scores the REAL resulting gh invocations against the rubric. Two real infra bugs found and fixed while building this (left as inline comments in run.ts): scoping `availableTools` to just "Bash" silently disabled every built-in including whatever loads skills from `skillDirectories`; and Copilot CLI's built-in GitHub MCP connector resolves repos against the real GitHub API, bypassing the shell (and any shell-based mock) entirely unless MCP tools are explicitly excluded. Verified: 3/3 consecutive runs pass, gh log shows the exact expected sequence (list -> comment 214 with the skill's fixed text -> label 214 needs-info, #220 correctly skipped).
This was referenced Aug 24, 2026
…, tool-scope
Correctness:
- score.ts: gh-log matching now requires an exact whitespace-delimited argv
token, not a raw substring — a call touching issue 2140 or 1214 could
previously satisfy a check written for 214 (and symmetrically false-fail a
forbiddenGhCalls check for 220 against a line containing 1220), undermining
the exact thing this eval verifies. forbiddenInCommands stays substring-based
intentionally (documented why: vendor-name detection needs to catch a prefix
like "workiq" inside "workiq_search_chats", which token-exact matching would
miss) — same trade-off evals/skillbuilder/score.ts already accepts.
- mocks/gh: `issue list` now actually simulates filtering instead of ignoring
every flag. Only returns the clean #214/#220 pair when the invocation asked
for `--label ... bug` + `no:assignee`; otherwise returns a noisier set
including two new distractors — #300 (already assigned) and #310 (wrong
label) — that a correctly-filtering skill must never touch. Strengthens the
"runtime conformance" claim: a skill that dropped its own filtering logic
now visibly fails instead of silently passing.
Maintainability:
- run.ts: the 3 mkdtempSync temp dirs per scenario run are now cleaned up in a
finally block unless --keep is passed — previously always leaked into /tmp.
fixtures/regenerate.ts: same fix for its own temp dirs.
- score.ts: new checkAllowedTools assertion parses the fixture's own
`allowed-tools` frontmatter and verifies every MUTATING Bash command (comment/
edit/create/close/...) matches a declared prefix — a regression in tool-gating
would now be caught. Scoped to mutating commands only, not every command:
gating on read-only reconnaissance (e.g. an occasional `gh repo view` sanity
check before triaging) would fail the suite on harmless model variance rather
than a real regression — same read-vs-mutating distinction this project's own
catalogues already draw ("Read tools are auto-approved; send/create/update/
delete need approval").
Minor:
- fixtures/regenerate.ts: comment on the renameSync call documenting the
assumption that `exportRoot` has exactly one child (the freshly-exported
skill dir) — true because it's a dir we just mkdtemp'd, but worth stating.
Verified: 3/3 consecutive full-suite passes after all fixes; confirmed the
mock's unfiltered branch actually returns the distractor issues; confirmed
--keep still preserves temp dirs and their absence otherwise.
… gaps Security (real gap, not just hardening): - bash-tool.ts previously spread the full process.env into the child shell and only prepended a mock bin dir to PATH, leaving the real PATH appended after it. excludedTools/enableHostGitOperations only restrict Copilot's built-in tools, not this custom one — an untrusted or off-spec SKILL.md could reach a real binary (curl, real gh, ...) with real env vars and real network access. - Fixed by making the custom Bash tool actually ENFORCE the fixture's own declared allowed-tools before executing anything, not just audit after the fact: a command that doesn't match a declared Bash(...) pattern is refused (exitCode 126) before /bin/sh ever runs, recorded separately in deniedTrace (visible under --keep) so a refusal is never confused with a scoring violation. This closes the companion scoring gap too — a real side effect via a non-gh command was previously invisible to the rubric; now it can't happen at all, because it can't execute. - Also stopped spreading process.env into the child process entirely (no inherited host secrets/tokens even if enforcement were ever bypassed by a future change) and narrowed PATH to the mock dir + /usr/bin:/bin only. - New evals/skill-runtime/allowed-tools.ts: shared parser/matcher used by both the enforcement layer (bash-tool.ts) and the redundant post-hoc safety-net check (score.ts), so they can't drift apart. Also fixes a real parsing gap: the old regex only matched entries ending in a literal `*)`; an entry like `Bash(gh issue view)` with no trailing wildcard was silently dropped from enforcement with no warning. Now parsed as an exact-match pattern instead. Correctness: - run.ts: tempDirs.push(...) previously ran only after all three mkdtempSync calls succeeded — if the 2nd or 3rd threw, the earlier-created dirs were never registered for cleanup and leaked. Now each dir is pushed immediately after its own creation. - mocks/gh: `issue view` always echoed hardcoded issue #214 regardless of the actual issue number requested ($3). Fixed to return the correct mock issue's data per number (214/220/300/310), or a "not found" error for anything else. Verified: 3/3 consecutive full-suite passes with enforcement active. Directly tested the Bash tool's enforcement in isolation: a curl attempt outside the declared allowed-tools is refused before /bin/sh ever runs (stays in deniedTrace, never touches trace or the network); a declared gh command still proceeds to execution normally.
Follows the same per-suite section pattern already used for the builder, skillbuilder, and sensitive harnesses: what it guards and why, how a run works, the rubric, current coverage, and how to add a scenario. Fills a real gap — the suite existed with no doc explaining how someone else (or a future me) would add a new fixture/mock/scenario to it.
Author
|
adilei can you please check this pr ? |
added 2 commits
August 25, 2026 16:12
commandMatchesAny() used a raw string prefix check on the full command, which was then passed unmodified to /bin/sh -c. An allowed prefix like `gh issue comment` could be used to smuggle a chained/injected command via shell metacharacters (&&, ;, |, $(), etc.), defeating the allowed-tools enforcement. Reject commands containing shell metacharacters and require prefix matches to land on a token boundary. Also adds unit test coverage, a configurable Bash timeout, and a clarifying comment on the unused "label" verb.
sendAndWait() can resolve to undefined (session timeout/close before any assistant message), but its result was dereferenced unconditionally for --keep debug logging. Guard the access and report the no-reply case explicitly instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses #55 (and the feedback that closed #53):
evals/skillbuilder/only scores the builder's proposed plan text — tool mentions, structure — never whether an actually-exportedSKILL.mdgets loaded and executed correctly by a real target runtime. This adds that missing layer: generate → export → load into a runtime → execute → score the real resulting behavior.Deliberately uses only what's already required for the rest of this eval suite — a signed-in Copilot CLI and the already-vendored
@github/copilot-sdk. No new dependency, no new credential.What's in
evals/skill-runtime/fixtures/regenerate.ts+fixtures/github-issue-triage-agent-skill/SKILL.md— a real, already-builtagent-skillartifact. Ships as a static, checked-in fixture rather than being regenerated on every run, so a runtime-eval failure points at the runtime, not builder variance.mocks/gh— a real, checked-in mockghexecutable. Actually simulates GitHub-side filtering, not just fixed replies:issue listonly returns the clean, intended result when the invocation's flags genuinely filter correctly; otherwise it returns a noisier set including issues a correct filter would have excluded (#300already assigned,#310wrong label).issue viewreturns the correct data per requested issue number.allowed-tools.ts— shared parser and matcher for a fixture'sallowed-toolsfrontmatter, used by both the enforcement layer and the scoring safety net so they can't drift apart. Rejects commands containing shell metacharacters (;,&&,|, backticks,$(...), redirection) so an allowed prefix can't be used to smuggle in a second, unchecked command; prefix matches require a token boundary, not just a raw string prefix.allowed-tools.test.ts— deterministic unit tests for the pattern parser and matcher, including the shell-injection-via-prefix-match case (see bug Bump fast-uri from 3.1.3 to 3.1.4 #3 below).bash-tool.ts— the one capability the runtime session gets: a customBashtool that enforces the fixture's own declaredallowed-toolsbefore executing anything (a command outside the declared patterns is refused before it ever reaches/bin/sh, not just flagged after the fact), runs with a minimal environment (no inherited host secrets/tokens, no realPATH), and records every invocation (and every refusal, separately) for scoring.run.ts— spins up a fresh Copilot session (separate from any builder session),skillDirectoriespointed at the fixture, sends a task prompt, and scores the real resulting invocations against the rubric. Cleans up its temp dirs unless--keepis passed.scenario.ts/scenarios.ts/score.ts— the type/rubric/scoring plumbing, same shape asevals/skillbuilder/.evals/README.md— new "Skill runtime evals" section documenting the lifecycle, rubric, security model, current coverage, and how to add a scenario.Real bugs found and fixed while building this
The ones with lasting behavioral/security consequences (see commit history for the rest — mock-log substring matching, an MCP-bypass fix, a temp-dir leak, a hardcoded mock issue number):
availableToolsto just["Bash"]silently disabled every built-in — including whatever loads skills fromskillDirectories. The skill was never actually being read.process.env/PATHinto the child shell with no gating beyond a mockedPATHprefix — an off-specSKILL.mdcould reach a real binary with real credentials/network access. Fixed by enforcingallowed-toolsbefore execution and dropping the inherited environment entirely.String.startsWithagainst the full command string, which was then handed unmodified to/bin/sh -c. A command likegh issue comment 214 --repo x --body "y" && rm -rf $HOMEstill starts with the allowed prefixgh issue comment, so the injected second command executed unchecked. Fixed by rejecting any command containing shell metacharacters (chaining/piping/substitution/redirection) before the prefix check, and by requiring prefix matches to land on a token boundary (sogh issue commentXYZno longer matches thegh issue commentprefix). Covered by new unit tests inallowed-tools.test.ts.Test plan
npm run typecheck— cleannpm test— includes 7 new deterministic unit tests inallowed-tools.test.tscovering pattern parsing, prefix/exact matching, and rejection of shell-metacharacter injection attemptsnpm run eval:skill-runtime— multiple consecutive full-suite passes across all rounds of fixescurlexfiltration attempt outside the declaredallowed-toolsis refused before/bin/shever runs (never touches the network); a declaredghcommand still proceeds to execution normally; a chained command appended to an allowed prefix (e.g.gh issue comment 214 ... && rm -rf $HOME) is now refused rather than executed#220/#300/#310while acting only on#214--keeppreserves temp dirs (and denied-attempt logs) for inspection; confirmed their absence otherwiseRelated: #55, #53