* feat: merge gbrain-jobs into minion-orchestrator — single unified minions skill * fix(skill/minion-orchestrator): correct MCP boundary, real handler names, PGLite path The initial merge commit a51c737 documented `submit_job name="shell"` as agent-callable, but src/core/operations.ts:1106 rejects protected names from MCP callers (shell is in src/core/minions/protected-names.ts:16) — shell-job submission is CLI-only. Subagent examples referenced non-existent handler names (`research`, `orchestrate`) instead of the real `subagent` / `subagent_aggregator` handlers. PGLite section wrongly told users to migrate to Supabase when `gbrain jobs submit ... --follow` inline mode works per docs/guides/minions-shell-jobs.md:15. Contract section canonized "every task through Minions" against the `pain_triggered` default in skills/conventions/subagent-routing.md:16,27. Rewrite addresses all four: - Shell Jobs section is explicit about CLI-only submission; agents observe via get_job / list_jobs / get_job_progress (non-protected). - Subagent examples route through `gbrain agent run` (user-facing CLI) with raw handler names documented as the power-user path. - PGLite gets --follow inline execution, not migration friction. - Contract softened to point at subagent-routing.md convention. Also adds a Preconditions block for Shell Jobs (env gate, RCE warning, execution-mode choice, verification command), narrows the frontmatter "gbrain jobs" trigger to "gbrain jobs submit" + "submit a gbrain job" (bare was too broad — CLI namespace covers 9 subcommands), inlines a "replaces older gbrain-jobs routing intent" note in the description, and removes non-existent `get_job_stats` from the tools list (CLI is `gbrain jobs stats`; no MCP equivalent). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(resolver): narrow "gbrain jobs" trigger to specific intents Replace bare "gbrain jobs" in the routing table with "gbrain jobs submit" + "submit a gbrain job". The bare phrase was too broad — the CLI namespace covers 9 subcommands (submit, list, get, retry, delete, prune, stats, smoke, work). Users asking about stats/prune/retry now fall through to `gbrain --help` instead of getting misrouted to minion-orchestrator, which only documents shell execution and subagent orchestration. Matches the frontmatter trigger narrow in minion-orchestrator/SKILL.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(resolver): add round-trip + skill-example-name validator Two new assertion blocks in test/resolver.test.ts: 1. RESOLVER.md trigger round-trip: every quoted phrase in a routing-table row has a fuzzy match in the target skill's frontmatter `triggers:` list. Catches RESOLVER ↔ frontmatter drift that checkResolvable's reachability check doesn't. Fuzzy match is case-insensitive, trailing-punctuation- insensitive, and splits on "/" for compound phrases like "pause/resume agent" — accommodates RESOLVER.md's natural-language summary style without allowing real drift through. 2. Skill example-name validator: every `name="<word>"` reference in any SKILL.md body must resolve to either a declared operation in src/core/operations.ts or a known Minions handler in PROTECTED_JOB_NAMES. Would have caught the `name="research"` / `name="orchestrate"` drift that slipped through the first review — nothing in CI caught those handler names referencing non-existent handlers until a Codex cold-read found them. This test closes that class of regression gap. 51 / 51 tests pass locally. Full E2E suite (bun run test:e2e) still passes 197 / 197 across 19 files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(e2e): PGLite shell-job --follow inline path Closes the T4 coverage gap surfaced during PR #381 eng review. The sibling test/e2e/minions-shell.test.ts covers Postgres + persistent-daemon; this file covers the PGLite + --follow path the minion-orchestrator skill now documents. Two assertions: 1. submit → registerBuiltinHandlers → worker.start → shell runs → completes with exit_code 0 and stdout_tail "hello\n". Exercises the exact dispatch path src/commands/jobs.ts:207 takes when --follow is set, including the GBRAIN_ALLOW_SHELL_JOBS=1 gate. 2. With GBRAIN_ALLOW_SHELL_JOBS unset, registerBuiltinHandlers leaves the shell handler unregistered. Confirms the env gate from src/commands/jobs.ts:611 works. Runs in-memory against PGLiteEngine — no DATABASE_URL, no Docker, runs in CI unconditionally. Completes in ~1.2s. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: pre-landing review fixes Pre-landing review caught 4 doc bugs + 2 test fragilities + 2 pre-existing drift cases. All auto-fix category (clear correct answer, single obvious fix). minion-orchestrator/SKILL.md: - Shell submit examples used nonexistent `--cmd`/`--argv`/`--cwd` flags. Real CLI takes `--params '{"cmd":"...","cwd":"..."}'` (src/commands/jobs.ts:55-85). Examples now match `gbrain jobs submit --help` output. - `--tools "search,web_search"` referenced `web_search` which isn't in BRAIN_TOOL_ALLOWLIST (src/core/minions/tools/brain-allowlist.ts:47-59). Swapped to `search,query`. Added a full allowlist enumeration so readers don't have to grep. - `gbrain agent run` flags section listed `--queue`, `--priority`, `--max-attempts`, `--delay` — none of these exist on that command (src/commands/agent.ts:105-129). Replaced with the real flag set (`--subagent-def`, `--model`, `--max-turns`, `--tools`, `--timeout-ms`, `--fanout-manifest`, `--follow`, `--no-follow`, `--detach`) and a note about using `gbrain jobs submit` for queue tuning. - MCP boundary claim "returns permission_denied" was imprecise. Reworded: throws an OperationError with code permission_denied. test/resolver.test.ts: - D5/C row regex required the backtick-quoted skill path to be followed immediately by `|`, silently skipping rows with trailing parentheticals (e.g., `` `skills/maintain/SKILL.md` (extraction sections) |``). Broadened to `[^|]*\|` so every row gets audited. test/e2e/minions-shell-pglite.test.ts: - Shared engine across both tests with no per-test reset. Future test additions would hit order-dependency. Added beforeEach TRUNCATE on minion_jobs / minion_inbox / minion_attachments, matching the Postgres sibling at test/e2e/minions-shell.test.ts:55-58. skills/query/SKILL.md: - Added 4 triggers RESOLVER.md routes to this skill but the frontmatter never declared: "who knows who", "relationship between", "connections", "graph query". Pre-existing drift — the broadened D5/C regex surfaced it. skills/maintain/SKILL.md: - Added 6 triggers with the same pre-existing drift: "extract links", "build link graph", "populate timeline", "populate links", "backfill graph", "extract timeline entries". 57/57 tests pass on the fixed tree. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: second-pass review fixes — stale CLI flag + handler name Two more stale references caught by specialist re-dispatch on the fixed tree: skills/minion-orchestrator/SKILL.md:72 — Routing table row described shell jobs as taking `--cmd` or `--argv` as CLI flags. Same class of bug as M1 from the prior fix commit but in a different location. Now says `--params` with `cmd` or `argv`, matching the corrected submit examples (lines 112-120). skills/conventions/subagent-routing.md:82 — "Check `get_job_stats` queue_health.active" referenced an MCP operation that doesn't exist in src/core/operations.ts. The new minion-orchestrator skill cross-references this convention file, so agents following the routing pointer would hit a non-existent op. Replaced with the real ops: `list_jobs --status active` (MCP) or `gbrain jobs stats` (CLI). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: adversarial pass cleanups — manifest.json + anti-pattern scope Claude adversarial subagent caught two last consistency gaps: skills/manifest.json:135 — Skill description still read "Manage background agents via Minions job queue" (subagent-only framing), out of sync with the reframed SKILL.md frontmatter. Manifest is what the skill registry indexes; leaving this stale meant shell-job-intent routers would miss it. Updated to match the unified wording. skills/minion-orchestrator/SKILL.md:288 — Anti-pattern line "Don't use sessions_spawn with runtime: subagent when Minions is available" was subagent-lane-specific inside the now-consolidated skill, reading like the one rule in the skill but only addressing one lane. Scoped to "For subagent work" and pointed at `gbrain agent run` so the rule doesn't confuse shell-job readers. Two investigate-class items deferred to follow-up: - D13 regex could false-positive on future skills with unrelated `name="..."` usage. Today clean; scope to backtick-fenced snippets if it bites. - PGLite E2E env-var race if bun:test ever goes file-parallel. Today isolated per file; add helper + comment when needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: bump version and changelog (v0.19.2) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: update README + CLAUDE.md for v0.19.2 Minions consolidation - Skill count 28 -> 29 across README and CLAUDE.md (adds smoke-test from v0.19.1 to the Skills section, closes a prior drift). - README minion-orchestrator row rewritten to name both lanes (shell jobs via `gbrain jobs submit shell`, LLM subagents via `gbrain agent run`) so the surface matches the consolidated skill file. - README Operational table gains a smoke-test row. - CLAUDE.md key-files entry for minion-orchestrator now describes the v0.19.2 consolidation, trust boundary (MCP permission_denied on protected names), and the narrowed trigger set. - CLAUDE.md Skills section notes the consolidation and the new v0.19.1 smoke-test skill. - CLAUDE.md test inventory picks up `test/e2e/minions-shell-pglite.test.ts` and the v0.19.2 round-trip + name-validator additions in `test/resolver.test.ts`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(ci): update PGLite test for new env-gate behavior + regenerate llms-full.txt CI caught two issues: 1. `test/e2e/minions-shell-pglite.test.ts` — the "GBRAIN_ALLOW_SHELL_JOBS unset → shell handler not registered" test was written against pre-v0.20.3 `registerBuiltinHandlers` behavior (env gate at registration time). Master's queue-resilience merge moved the gate from registration to execution: shell handler is now always registered so claimed jobs emit a clear rejection log, and `shellHandler` itself throws UnrecoverableError when GBRAIN_ALLOW_SHELL_JOBS != '1' (see src/core/minions/handlers/shell.ts:210). Updated the test to invoke shellHandler directly with a minimal ctx and assert the throw. Preserves the test's intent (prove the guard works) under the new control flow. 2. `llms-full.txt` drift — README.md + CLAUDE.md updates in v0.19.2 and v0.20.4 updated the skill count to 29 and rewrote the minion-orchestrator description, but the committed `llms-full.txt` bundle still reflected the pre-consolidation content. Regenerated via `bun run build:llms`. The third CI failure (`planInstall + applyInstall D-CX-11`) passes cleanly locally (26/26 in test/skillpack-install.test.ts). The 1ms runtime in CI suggests a filesystem-mtime flake, not a real regression from this branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(skillpack): treat future-mtime lock as stale (CI race fix) D-CX-11 ("--force-unlock overrides a stale lock") flaked in CI with a 1ms runtime. Root cause: on fast CI filesystems (ext4 with high-resolution mtimes on GitHub runners), `writeFileSync` can set a lock's mtime a few microseconds ahead of the subsequent `Date.now()`, making `age` negative. Old logic: const stale = age >= staleMs; With `staleMs: 0` and `age = -0.3ms`: `-0.3 >= 0` is false → NOT stale → the `!stale` branch throws `lock_held` before reaching the force-unlock path. Test failed at the first ms, never exercised the actual unlock logic. Fix (src/core/skillpack/installer.ts:189): const stale = age < 0 || age >= staleMs; Treats negative age (future mtime) as stale. Safe: if the lock's mtime is in the future, either the filesystem clock just jumped forward or the lock was written by a racing process; either way it's not a live, healthy lock and the stale path is the correct branch. Passes locally (26/26 in test/skillpack-install.test.ts). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: root <root@localhost> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
206 lines
8.4 KiB
TypeScript
206 lines
8.4 KiB
TypeScript
import { describe, test, expect } from "bun:test";
|
|
import { readFileSync, existsSync, readdirSync, statSync } from "fs";
|
|
import { join } from "path";
|
|
import { checkResolvable } from "../src/core/check-resolvable.ts";
|
|
import { PROTECTED_JOB_NAMES } from "../src/core/minions/protected-names.ts";
|
|
|
|
const SKILLS_DIR = join(import.meta.dir, "..", "skills");
|
|
const RESOLVER_PATH = join(SKILLS_DIR, "RESOLVER.md");
|
|
const OPERATIONS_PATH = join(import.meta.dir, "..", "src", "core", "operations.ts");
|
|
|
|
describe("RESOLVER.md", () => {
|
|
test("exists", () => {
|
|
expect(existsSync(RESOLVER_PATH)).toBe(true);
|
|
});
|
|
|
|
const resolverContent = existsSync(RESOLVER_PATH)
|
|
? readFileSync(RESOLVER_PATH, "utf-8")
|
|
: "";
|
|
|
|
test("references only existing skill files", () => {
|
|
// Delegates to checkResolvable — no reimplemented parsing logic
|
|
const report = checkResolvable(SKILLS_DIR);
|
|
const missingFiles = report.issues.filter(i => i.type === "missing_file");
|
|
expect(missingFiles.length).toBe(0);
|
|
});
|
|
|
|
test("has categorized sections", () => {
|
|
expect(resolverContent).toContain("## Always-on");
|
|
expect(resolverContent).toContain("## Brain operations");
|
|
expect(resolverContent).toContain("## Content & media ingestion");
|
|
expect(resolverContent).toContain("## Operational");
|
|
});
|
|
|
|
test("has disambiguation rules", () => {
|
|
expect(resolverContent).toContain("## Disambiguation rules");
|
|
});
|
|
|
|
test("references conventions", () => {
|
|
expect(resolverContent).toContain("conventions/quality.md");
|
|
expect(resolverContent).toContain("_brain-filing-rules.md");
|
|
});
|
|
|
|
test("every manifest skill is reachable from resolver", () => {
|
|
// Delegates to checkResolvable — the shared function handles all validation
|
|
const report = checkResolvable(SKILLS_DIR);
|
|
const unreachable = report.issues.filter(i => i.type === "unreachable");
|
|
if (unreachable.length > 0) {
|
|
const names = unreachable.map(i => `${i.skill}: ${i.action}`).join("\n ");
|
|
throw new Error(`Unreachable skills:\n ${names}`);
|
|
}
|
|
expect(report.summary.unreachable).toBe(0);
|
|
});
|
|
});
|
|
|
|
// D5/C — resolver round-trip: every quoted trigger in a RESOLVER.md table row
|
|
// must appear in the target skill's frontmatter `triggers:` list. Catches
|
|
// trigger/frontmatter drift that `checkResolvable` reachability doesn't.
|
|
describe("RESOLVER.md trigger round-trip (D5/C)", () => {
|
|
type Row = { triggers: string[]; skillPath: string };
|
|
|
|
const rows: Row[] = (() => {
|
|
if (!existsSync(RESOLVER_PATH)) return [];
|
|
const content = readFileSync(RESOLVER_PATH, "utf-8");
|
|
// Tolerate trailing annotations after the backtick path (e.g.,
|
|
// `` `skills/maintain/SKILL.md` (extraction sections) |``). The path cell
|
|
// starts with a backtick-quoted `.md` ref; anything between that and the
|
|
// closing `|` is free-form prose and is intentionally ignored.
|
|
const rowRe = /^\s*\|\s*([^|]+?)\s*\|\s*`([^`]+\.md)`[^|]*\|\s*$/gm;
|
|
const out: Row[] = [];
|
|
let m: RegExpExecArray | null;
|
|
while ((m = rowRe.exec(content)) !== null) {
|
|
const rawTriggers = m[1];
|
|
const skillPath = m[2];
|
|
const triggerStrings = Array.from(rawTriggers.matchAll(/"([^"]+)"/g)).map(t => t[1]);
|
|
if (triggerStrings.length > 0) {
|
|
out.push({ triggers: triggerStrings, skillPath });
|
|
}
|
|
}
|
|
return out;
|
|
})();
|
|
|
|
test("at least one routing row parses from RESOLVER.md", () => {
|
|
expect(rows.length).toBeGreaterThan(0);
|
|
});
|
|
|
|
for (const row of rows) {
|
|
test(`every RESOLVER trigger for ${row.skillPath} is declared in its frontmatter`, () => {
|
|
const skillFullPath = join(SKILLS_DIR, "..", row.skillPath);
|
|
expect(existsSync(skillFullPath)).toBe(true);
|
|
|
|
const skillContent = readFileSync(skillFullPath, "utf-8");
|
|
const fmMatch = skillContent.match(/^---\n([\s\S]*?)\n---/);
|
|
if (!fmMatch) {
|
|
throw new Error(`No YAML frontmatter in ${row.skillPath}`);
|
|
}
|
|
const frontmatter = fmMatch[1];
|
|
// Parse frontmatter triggers: list. Match "..." OR '...' items separately
|
|
// so apostrophes inside double-quoted values don't truncate the capture.
|
|
const triggersBlock = frontmatter.match(/triggers:\s*\n((?:\s*-\s*(?:"[^"]*"|'[^']*')\s*\n?)+)/);
|
|
const declaredTriggers = triggersBlock
|
|
? Array.from(triggersBlock[1].matchAll(/-\s*(?:"([^"]*)"|'([^']*)')/g))
|
|
.map(m => m[1] ?? m[2])
|
|
: [];
|
|
|
|
// Fuzzy match: RESOLVER.md phrases are natural-language summaries of the
|
|
// skill's intent; frontmatter triggers are the agent-facing phrase set.
|
|
// Match is case-insensitive, trailing-punctuation-insensitive, and supports
|
|
// "/"-split compounds (e.g., "pause/resume agent" → ["pause", "resume agent"]).
|
|
const normalize = (s: string) => s.toLowerCase().replace(/[?!.,]+$/, "").trim();
|
|
const declaredLower = declaredTriggers.map(normalize);
|
|
|
|
function matchesAny(phrase: string): boolean {
|
|
const p = normalize(phrase);
|
|
if (declaredLower.includes(p)) return true;
|
|
for (const ft of declaredLower) {
|
|
if (ft.includes(p) || p.includes(ft)) return true;
|
|
}
|
|
// Slash-split compound: every part should have some fuzzy frontmatter hit
|
|
if (p.includes("/")) {
|
|
const parts = p.split("/").map(s => s.trim()).filter(Boolean);
|
|
const allParts = parts.every(part =>
|
|
declaredLower.some(ft => ft.includes(part) || part.includes(ft))
|
|
);
|
|
if (allParts) return true;
|
|
}
|
|
return false;
|
|
}
|
|
|
|
const missing = row.triggers.filter(t => !matchesAny(t));
|
|
if (missing.length > 0) {
|
|
throw new Error(
|
|
`RESOLVER.md routes ${JSON.stringify(missing)} to ${row.skillPath}, but the ` +
|
|
`skill's frontmatter has no fuzzy match. Declared: ${JSON.stringify(declaredTriggers)}`
|
|
);
|
|
}
|
|
});
|
|
}
|
|
});
|
|
|
|
// D13 — skill-example-name validator: any `name="<word>"` reference inside a
|
|
// SKILL.md body must resolve to either a declared operation in operations.ts
|
|
// or a known Minions handler name in PROTECTED_JOB_NAMES. Catches T2-class
|
|
// bugs where docs reference handler names that don't exist (e.g., the
|
|
// `name="research"` / `name="orchestrate"` bug from PR #381 pre-reframe).
|
|
describe("Skill example-name validator (D13)", () => {
|
|
const opNames: string[] = (() => {
|
|
if (!existsSync(OPERATIONS_PATH)) return [];
|
|
const content = readFileSync(OPERATIONS_PATH, "utf-8");
|
|
return Array.from(content.matchAll(/^\s+name:\s*'([a-z_]+)',/gm)).map(m => m[1]);
|
|
})();
|
|
|
|
const knownNames = new Set<string>([...opNames, ...PROTECTED_JOB_NAMES]);
|
|
|
|
test("operation names extracted from operations.ts", () => {
|
|
// Sanity check: operations.ts should declare dozens of ops
|
|
expect(opNames.length).toBeGreaterThan(10);
|
|
});
|
|
|
|
test("PROTECTED_JOB_NAMES is non-empty", () => {
|
|
expect(PROTECTED_JOB_NAMES.size).toBeGreaterThan(0);
|
|
});
|
|
|
|
function walkSkills(dir: string): string[] {
|
|
if (!existsSync(dir)) return [];
|
|
const out: string[] = [];
|
|
for (const entry of readdirSync(dir)) {
|
|
const p = join(dir, entry);
|
|
const s = statSync(p);
|
|
if (s.isDirectory()) {
|
|
out.push(...walkSkills(p));
|
|
} else if (entry === "SKILL.md") {
|
|
out.push(p);
|
|
}
|
|
}
|
|
return out;
|
|
}
|
|
|
|
const skillFiles = walkSkills(SKILLS_DIR);
|
|
|
|
test("at least one SKILL.md found", () => {
|
|
expect(skillFiles.length).toBeGreaterThan(0);
|
|
});
|
|
|
|
for (const skillFile of skillFiles) {
|
|
const rel = skillFile.replace(SKILLS_DIR, "skills");
|
|
test(`${rel}: every name="<word>" reference resolves to a real op or handler`, () => {
|
|
const content = readFileSync(skillFile, "utf-8");
|
|
// Strip YAML frontmatter so `name: <skillname>` isn't mis-captured.
|
|
const body = content.replace(/^---\n[\s\S]*?\n---\n/, "");
|
|
// Match only `name=` (with equals, not colon) to avoid YAML false positives
|
|
// if the frontmatter strip ever breaks. Captures quoted word values.
|
|
const refs = Array.from(body.matchAll(/name\s*=\s*["']([a-z_][a-z_0-9]*)["']/gi))
|
|
.map(m => m[1]);
|
|
const unique = [...new Set(refs)];
|
|
const unknown = unique.filter(n => !knownNames.has(n));
|
|
if (unknown.length > 0) {
|
|
throw new Error(
|
|
`${rel}: references name="..." values not declared in src/core/operations.ts or ` +
|
|
`PROTECTED_JOB_NAMES: ${JSON.stringify(unknown)}. ` +
|
|
`Known: ${JSON.stringify([...knownNames].sort())}`
|
|
);
|
|
}
|
|
});
|
|
}
|
|
});
|