mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-27 22:15:33 +00:00
* 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>
117 lines
4.7 KiB
TypeScript
117 lines
4.7 KiB
TypeScript
/**
|
|
* E2E Minions Shell Handler — PGLite / --follow inline execution path
|
|
*
|
|
* Closes the T4 gap surfaced during PR #381 eng review. The sibling file
|
|
* test/e2e/minions-shell.test.ts covers the Postgres + persistent-worker-daemon
|
|
* path. This file covers the PGLite path documented in the minion-orchestrator
|
|
* skill: `gbrain jobs submit shell ... --follow` runs inline because
|
|
* `gbrain jobs work` (daemon) is not available on PGLite (exclusive file lock).
|
|
*
|
|
* Mirrors the Postgres test's structure but runs in-memory against PGLiteEngine.
|
|
* No DATABASE_URL required, no Docker — runs in CI unconditionally.
|
|
*
|
|
* Run: bun test test/e2e/minions-shell-pglite.test.ts
|
|
*/
|
|
|
|
import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test';
|
|
import { PGLiteEngine } from '../../src/core/pglite-engine.ts';
|
|
import { MinionQueue } from '../../src/core/minions/queue.ts';
|
|
import { MinionWorker } from '../../src/core/minions/worker.ts';
|
|
import { registerBuiltinHandlers } from '../../src/commands/jobs.ts';
|
|
|
|
let engine: PGLiteEngine;
|
|
let originalAllowShellJobs: string | undefined;
|
|
|
|
async function waitTerminal(queue: MinionQueue, id: number, timeoutMs = 15000): Promise<string> {
|
|
const deadline = Date.now() + timeoutMs;
|
|
while (Date.now() < deadline) {
|
|
const j = await queue.getJob(id);
|
|
if (j && ['completed', 'failed', 'dead', 'cancelled'].includes(j.status)) return j.status;
|
|
await new Promise((r) => setTimeout(r, 50));
|
|
}
|
|
const j = await queue.getJob(id);
|
|
throw new Error(`job ${id} did not reach terminal state in ${timeoutMs}ms; last status=${j?.status}`);
|
|
}
|
|
|
|
beforeAll(async () => {
|
|
// registerBuiltinHandlers gates shell handler on GBRAIN_ALLOW_SHELL_JOBS=1.
|
|
// Mirror the real --follow path by setting the env var; restore on cleanup
|
|
// so other tests see their original environment.
|
|
originalAllowShellJobs = process.env.GBRAIN_ALLOW_SHELL_JOBS;
|
|
process.env.GBRAIN_ALLOW_SHELL_JOBS = '1';
|
|
|
|
engine = new PGLiteEngine();
|
|
await engine.connect({}); // in-memory PGLite
|
|
await engine.initSchema(); // installs pages, minion_jobs, config, etc.
|
|
});
|
|
|
|
afterAll(async () => {
|
|
await engine.disconnect();
|
|
if (originalAllowShellJobs === undefined) {
|
|
delete process.env.GBRAIN_ALLOW_SHELL_JOBS;
|
|
} else {
|
|
process.env.GBRAIN_ALLOW_SHELL_JOBS = originalAllowShellJobs;
|
|
}
|
|
});
|
|
|
|
describe('E2E: Minions shell handler on PGLite (--follow inline path)', () => {
|
|
// Mirror the Postgres sibling's per-test reset. The engine is shared across
|
|
// both tests via beforeAll; without this, completed jobs from one test leak
|
|
// into minion_jobs and future test additions hit order-dependency.
|
|
beforeEach(async () => {
|
|
const db = (engine as any).db;
|
|
await db.exec(`DELETE FROM minion_attachments; DELETE FROM minion_inbox; DELETE FROM minion_jobs;`);
|
|
});
|
|
|
|
test('submit → worker registered via registerBuiltinHandlers → shell runs → completes', async () => {
|
|
const queue = new MinionQueue(engine);
|
|
const job = await queue.add(
|
|
'shell',
|
|
{ cmd: 'echo hello', cwd: '/tmp' },
|
|
{},
|
|
{ allowProtectedSubmit: true },
|
|
);
|
|
expect(job.name).toBe('shell');
|
|
expect(job.status).toBe('waiting');
|
|
|
|
// This is the exact dispatch path --follow takes (src/commands/jobs.ts:207).
|
|
// Gates shell on GBRAIN_ALLOW_SHELL_JOBS=1 (set in beforeAll above).
|
|
const worker = new MinionWorker(engine, { pollInterval: 100, lockDuration: 30000 });
|
|
await registerBuiltinHandlers(worker, engine);
|
|
expect(worker.registeredNames).toContain('shell');
|
|
|
|
const runPromise = worker.start();
|
|
try {
|
|
const status = await waitTerminal(queue, job.id, 20000);
|
|
expect(status).toBe('completed');
|
|
const final = await queue.getJob(job.id);
|
|
expect((final!.result as any).exit_code).toBe(0);
|
|
expect((final!.result as any).stdout_tail).toBe('hello\n');
|
|
} finally {
|
|
worker.stop();
|
|
await runPromise;
|
|
}
|
|
}, 30000);
|
|
|
|
test('GBRAIN_ALLOW_SHELL_JOBS unset → shellHandler rejects at execution time', async () => {
|
|
// v0.20.3+: shell handler is always registered (so claimed jobs emit a clear
|
|
// rejection log), but the runtime env guard lives inside the handler itself.
|
|
// Prove the guard rejects when the env var is unset.
|
|
const { shellHandler } = await import('../../src/core/minions/handlers/shell.ts');
|
|
const saved = process.env.GBRAIN_ALLOW_SHELL_JOBS;
|
|
delete process.env.GBRAIN_ALLOW_SHELL_JOBS;
|
|
try {
|
|
const ctx: any = {
|
|
id: 1,
|
|
name: 'shell',
|
|
data: { cmd: 'echo hi', cwd: '/tmp' },
|
|
attempt: 1,
|
|
engine,
|
|
};
|
|
await expect(shellHandler(ctx)).rejects.toThrow(/GBRAIN_ALLOW_SHELL_JOBS=1/);
|
|
} finally {
|
|
process.env.GBRAIN_ALLOW_SHELL_JOBS = saved;
|
|
}
|
|
});
|
|
});
|