QM edition: three gstack skills QM loads on demand (GST-1492)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
garrytanandcapy-ai[bot] committed 2026-11-05 23:36:46 +00:00
1 parent a339b93597
commit 85e751e8c5
13 files changed
+1220 -1

No files matched your search

+1
View File
@@ -0,0 +1 @@
- **gstack for QM: three skills QM loads on demand.** `qm/sandbox/skills/` holds `gstack-investigate`, `gstack-review` and `gstack-plan` in QM's skill format, generated from gstack's own investigate, review and plan-eng-review templates by `scripts/gen-qm-edition.ts` (spec in `hosts/qm.ts`). Install them as a QM deployment layer with no install step. Each file fits QM's whole-file read (under 15,000 characters), each first step is a command QM's sandbox has, and `test/qm-edition.test.ts` fails on a stale edition, an oversized file or a command QM can't run. `qm/edition.json` names each skill's first command for evals. (GST-1492)
+234
View File
@@ -0,0 +1,234 @@
/**
* QM edition: three gstack skills in QM's own skill format.
*
* QM (a work assistant) lists skills in a name-and-description index with a
* 12,000-char budget and loads a body only when the agent asks for it through
* its `skill` tool, which also syncs the skill's files into
* `.agent-skills/<conversation>/<name>/`. Tool results over about 16,384
* chars are cut, and QM's `read` tool has no offset, so every file here must
* be readable whole. QM's sandbox has no bun, no gstack checkout and no
* install hook, so every command a skill names is a POSIX utility, git, or a
* POSIX sh script shipped inside the skill.
*
* This is not a HostConfig in hosts/index.ts: it renders three skills by
* section selection, without the shared preamble, instead of the whole
* catalog through gen-skill-docs. scripts/gen-qm-edition.ts reads this spec
* and writes qm/sandbox/skills/<id>/ plus qm/edition.json; test/qm-edition.test.ts enforces the
* limits and the command rule. Self-contained: tests copy hosts/*.ts alone.
*/
export interface QmPart {
/** Template or reference file, relative to the gstack root. */
file: string;
/** Exact heading text (without the leading #s) whose section is kept, subsections included. */
heading: string;
/** Output heading level for the kept heading; its subsections shift with it. */
level: number;
/** Subsection headings removed from the kept section. */
skipHeadings?: string[];
/** A paragraph, list item or fenced block containing any of these strings is removed. */
dropBlocks?: string[];
/** Exact text replaced in the kept section, after drops; each must match. */
replaceText?: Array<[string, string]>;
}
export interface QmSkill {
/** QM skill name and folder: qm/sandbox/skills/<id>/. */
id: string;
/** The gstack skill this one is derived from. */
source: string;
/** Index description: when to use it, within limits.descriptionChars. */
description: string;
/** QM-authored start: title and the first step, a command that runs in QM's sandbox. */
header: string;
parts: QmPart[];
/** The first command the header tells QM to run, and a regex over QM's executed command that marks it ran. */
firstStep: { command: string; marker: string };
/** Scripts shipped in the skill: output name -> file under QM_EDITION.scriptSourceDir. */
scripts: Record<string, string>;
}
/** POSIX.1-2017 utilities and sh special/regular builtins the edition may name. */
export const QM_POSIX_COMMANDS: readonly string[] = [
'alias', 'awk', 'basename', 'break', 'cat', 'cd', 'chmod', 'cmp', 'command', 'comm', 'continue', 'cp', 'cut',
'date', 'diff', 'dirname', 'echo', 'env', 'eval', 'exec', 'exit', 'export', 'expr', 'false', 'find', 'getopts',
'grep', 'head', 'id', 'kill', 'ln', 'ls', 'mkdir', 'mktemp', 'mv', 'nl', 'od', 'paste', 'printf', 'ps', 'pwd',
'read', 'readonly', 'return', 'rm', 'rmdir', 'sed', 'set', 'sh', 'shift', 'sleep', 'sort', 'tail', 'tee', 'test',
'touch', 'tr', 'trap', 'true', 'uname', 'uniq', 'unset', 'wait', 'wc', 'xargs', ':', '.', '[',
];
/** Non-POSIX binaries the edition relies on QM's sandbox image to have. */
export const QM_IMAGE_COMMANDS: readonly string[] = ['git'];
export const QM_EDITION = {
outDir: 'qm/sandbox/skills',
scriptSourceDir: 'hosts/qm/scripts',
manifest: 'qm/edition.json',
limits: {
/** Per shipped file, under QM's ~16,384-char inline tool-result cap with headroom. */
fileChars: 15_000,
descriptionChars: 160,
/** The edition's share of QM's 12,000-char skills index (name + description per skill). */
indexChars: 600,
},
/** Applied to kept gstack text, in order: host tool names become QM's. */
rewrites: [
['use AskUserQuestion', 'ask the user'],
['AskUserQuestion', 'ask the user'],
['the Bash tool', 'the execute tool'],
['the Read tool', 'the read tool'],
] as Array<[string, string]>,
/** Text that points at gstack machinery or files the edition does not ship. */
forbiddenText: ['~/.claude', '$GSTACK', 'GSTACK_', '{{', 'CLAUDE_SKILL_DIR', '<skill dir>', 'checklist.md', 'greptile',
'review-log', 'Step 4.', 'Step 5.8', 'Review Army', 'Outside Voice', 'Decision procedure', 'Aside'],
/** Template placeholders a kept section may contain; anything else fails generation. */
dropResolvers: [
'LEARNINGS_LOG', 'GBRAIN_SAVE_RESULTS', 'FLAKE_FIX_NUMBER', 'SHARED_LIBS_RUBRIC', 'TEST_COVERAGE_AUDIT_PLAN',
'PLAN_REALITY_ROWS', 'CODEX_PLAN_REVIEW', 'CONFIDENCE_CALIBRATION',
],
skills: [] as QmSkill[],
};
/**
* QM rewrites the literal text `skills/<name>/` in SKILL.md to the absolute
* folder its `skill` tool synced the files into (materialize.ts:22-33), and
* the shell starts in the workspace root with scripts not executable, so a
* skill runs its script as `sh skills/<id>/scripts/<file>`.
*/
export const qmScriptCommand = (id: string, script: string, args = ''): string =>
['sh', `skills/${id}/scripts/${script}`, args].filter(Boolean).join(' ');
QM_EDITION.skills.push(
{
id: 'gstack-investigate',
source: 'investigate',
description: 'Use when a test fails, code errors or output is wrong: find and prove the root cause before any fix, then fix it with a regression test. (gstack)',
header: [
'# Investigate: root cause before any fix',
'',
'**Start here, before reading code.** In the repository (`cd` into it first: the shell starts in the workspace root), run with `execute`, adding the failing file or directory if you know it:',
'',
'```sh',
'git status --short',
'git log --oneline -20 -- <path>',
'```',
'',
'Then reproduce the symptom with the project\'s own test or run command, and write down what you saw, before you form a hypothesis.',
].join('\n'),
parts: [
{ file: 'investigate/SKILL.md.tmpl', heading: 'Iron Law', level: 2 },
{ file: 'investigate/SKILL.md.tmpl', heading: 'How to work', level: 2 },
{ file: 'investigate/sections/race.md.tmpl', heading: 'Race recipe', level: 2 },
{
file: 'investigate/SKILL.md.tmpl', heading: 'Report', level: 2,
dropBlocks: ['Scope Lock', 'Log the investigation as a learning', 'gstack-learnings-log'],
},
{ file: 'investigate/SKILL.md.tmpl', heading: 'Important Rules', level: 2 },
],
firstStep: { command: 'git log --oneline -20 -- <path>', marker: '\\bgit\\s+log\\b' },
scripts: {},
},
{
id: 'gstack-review',
source: 'review',
description: 'Use before merging a branch or PR: review the diff for bugs that pass CI but break production, fix the obvious ones and ask about the rest. (gstack)',
header: [
'# Review: find what passes CI but breaks in production',
'',
'**Start here.** In the repository (`cd` into it first: the shell starts in the workspace root), run with `execute` (pass the base branch if you know it):',
'',
'```sh',
qmScriptCommand('gstack-review', 'diff-base.sh', '<base>'),
'```',
'',
'It prints the base, the merge base, the commits since the base and the diff stat, then `NEXT: git diff <merge base>`; run that and read the whole diff and the untracked files it lists before you comment. On `NOTHING_TO_REVIEW`, say so and stop. On `BASE: none`, ask the user for the base branch or review the uncommitted changes it shows.',
'',
'QM fills in the absolute path of the folder its `skill` tool synced (it prints that path too). If the script is missing, run this instead:',
'',
'```sh',
'git merge-base origin/main HEAD',
'git diff --stat <merge-base>',
'```',
'',
'## How to review',
'',
'1. Read the full diff, then the unchanged callers of anything it changes.',
'2. Apply the CRITICAL categories, then the INFORMATIONAL ones, below. Respect the suppressions.',
'3. Classify each finding with the Fix-First Heuristic: fix AUTO-FIX items directly; for ASK items, ask the user in one message (a numbered list, each with a recommended fix) and stop until they answer.',
'4. Verify each claim (below), then report: `CRITICAL` and `INFORMATIONAL` lists, one line problem and one line fix each, with file:line, then what you fixed. `NO FINDINGS` is a complete result.',
].join('\n'),
parts: [
{
file: 'review/checklist.md', heading: 'Review Categories', level: 2,
dropBlocks: ['gstack-timing-ok', 'CC+gstack'],
replaceText: [["route it to /investigate's flake recipe (timing flakes)", 'find its cause first (gstack-investigate)']],
skipHeadings: [
'Gate Integrity', 'Approval Classes and Data Repairs', 'Dead Code & Consistency (version/changelog only — other items handled by maintainability specialist)',
'Keyboard-Count Test Loops', 'Distribution & CI/CD Pipeline', 'Repo Conventions & Platform Fit (runs on every PR type)',
],
},
{
file: 'review/checklist.md', heading: 'Fix-First Heuristic', level: 2,
dropBlocks: ['This heuristic is referenced by', 'Severity glossary', '**blocker**', '**should**', '**nit**'],
replaceText: [
['├─ Anything changing user-visible', '└─ Anything changing user-visible'],
['│ behavior\n └─ Gate Integrity (always ASK; four\n gate options, never Fix/Skip)', ' behavior'],
],
},
{
file: 'review/checklist.md', heading: 'Suppressions — DO NOT flag these', level: 2,
dropBlocks: ['Eval threshold changes', 'gstack-shortcut'],
},
{ file: 'review/SKILL.md.tmpl', heading: 'Verification of claims', level: 2 },
{
file: 'review/SKILL.md.tmpl', heading: 'Important Rules', level: 2,
dropBlocks: ['greptile-triage.md', 'Optional extractions stay advisory'],
replaceText: [["that's /ship's job", 'leave that to the user']],
},
],
firstStep: { command: qmScriptCommand('gstack-review', 'diff-base.sh', '<base>'), marker: 'diff-base\\.sh|\\bgit\\s+(diff|merge-base)\\b' },
scripts: { 'diff-base.sh': 'diff-base.sh' },
},
{
id: 'gstack-plan',
source: 'plan-eng-review',
description: 'Use before building a feature: challenge the scope, then review architecture, code quality, tests and performance, and end with a plan to build. (gstack)',
header: [
'# Plan a feature: scope first, then the engineering review',
'',
'Review a plan, not code: do not build the feature. If the user has not given you the plan or feature description, ask for it and stop.',
'',
'**Start here.** In the repository (`cd` into it first: the shell starts in the workspace root), run with `execute` to see what already exists:',
'',
'```sh',
'git log --oneline -10',
'ls',
'```',
'',
'Then read the files the plan touches. Work through Scope Challenge, then Sections 1-4 in order; for each finding give severity, a recommendation and the alternative. Ask about decisions that change scope in one message and stop until the user answers. Finish with the revised plan: what to build, in order, the files it touches, the tests that prove it, and what is NOT in scope.',
].join('\n'),
parts: [
{ file: 'plan-eng-review/SKILL.md.tmpl', heading: 'My engineering preferences (use these to guide your recommendations):', level: 2 },
{
file: 'plan-eng-review/sections/review-sections.md.tmpl', heading: 'A. Assess the target', level: 2,
dropBlocks: ['Search check', 'TODOS cross-reference', 'CC+gstack'],
replaceText: [
['Complete these checks before the complexity decision in B. Do not apply scope\nchanges or write findings into the plan yet.', 'Complete these checks before proposing scope changes. Do not write findings into the plan yet.'],
[' Use these counts in B.', ' At 8+ files or 2+ new classes or services, propose a smaller arrangement and ask before changing scope.'],
],
},
{
file: 'plan-eng-review/sections/review-sections.md.tmpl', heading: 'Review Sections (after scope is agreed)', level: 2,
skipHeadings: ['Continue after Outside Voice'],
dropBlocks: ['Decision procedure', 'Test Plan Artifact', 'Confidence Calibration format'],
replaceText: [
[', using the rubric below', ''],
['Rejected extractions still need coverage for real duplicated-code defects.', 'Rejected extractions still need coverage for real duplicated-code defects.\n\nFor every new codepath, branch and error path in the plan, name the test that proves it (unit, integration or end-to-end) and the regression it would catch. Missing tests become plan items.'],
],
},
],
firstStep: { command: 'git log --oneline -10', marker: '\\bgit\\s+log\\b' },
scripts: {},
},
);
+48
View File
@@ -0,0 +1,48 @@
#!/bin/sh
# gstack-review first step: find the base, the merge base and what changed.
# Usage: sh diff-base.sh [base] (POSIX sh and git only)
set -u
if ! git rev-parse --is-inside-work-tree >/dev/null 2>&1; then
echo "NOT_A_GIT_REPO: $(pwd)"
exit 0
fi
no_base() {
echo "BASE: none ($1)"
echo "== uncommitted changes"
git status --short | head -n 40
if git rev-parse --verify --quiet HEAD >/dev/null; then
echo "== diff stat against HEAD"
git diff --stat HEAD --
fi
echo "NEXT: pass the base branch (sh diff-base.sh <base>) or review the uncommitted changes with git diff HEAD"
exit 0
}
base=${1:-}
if [ -z "$base" ]; then
base=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null || true)
fi
if [ -z "$base" ]; then
for candidate in origin/main main origin/master master; do
if git rev-parse --verify --quiet "$candidate^{commit}" >/dev/null; then
base=$candidate
break
fi
done
fi
[ -n "$base" ] || no_base "no origin/HEAD, main or master"
merge_base=$(git merge-base "$base" HEAD 2>/dev/null) || no_base "$base has no merge base with HEAD"
echo "BRANCH: $(git symbolic-ref --quiet --short HEAD 2>/dev/null || echo detached)"
echo "BASE: $base"
echo "MERGE_BASE: $merge_base"
echo "== commits since base"
git log --oneline "$merge_base..HEAD" -- | head -n 30
echo "== diff stat (working tree against merge base)"
git diff --stat "$merge_base" --
untracked=$(git ls-files --others --exclude-standard)
echo "== untracked files"
printf '%s\n' "$untracked" | head -n 30
if git diff --quiet "$merge_base" -- && [ -z "$untracked" ]; then
echo "NOTHING_TO_REVIEW"
exit 0
fi
echo "NEXT: git diff $merge_base"
+33
View File
@@ -0,0 +1,33 @@
# gstack for QM
Three gstack skills in QM's own skill format: `gstack-investigate` (find and prove a bug's root cause before fixing it), `gstack-review` (review a branch before merge) and `gstack-plan` (review a feature plan before building it).
QM lists each skill by name and description and loads the body only when it's needed, so nothing goes into the org SOUL and nothing is installed. Each file stays under 15,000 characters so QM's `read` and `skill` tools return it whole, and every command a skill names is a POSIX utility, `git`, or a POSIX `sh` script shipped inside the skill. QM's sandbox images have no bun and no gstack checkout.
## Install as a QM deployment layer
Copy the skill folders into your QM deployment directory and sync it:
```sh
mkdir -p <deploy dir>/sandbox/skills
cp -R qm/sandbox/skills/gstack-* <deploy dir>/sandbox/skills/
qm up
```
The skills install into the org scope. Removing the folders and running `qm up` again archives them.
A git pack that points at this whole repository won't work: QM's pack fetcher stops at 32 MiB and 5,000 files, and it would pick up every other gstack `SKILL.md` too. To ship it as a git pack, put `qm/sandbox/skills/` in a small repository of its own.
## What each skill runs first
`qm/edition.json` lists each skill's first command and a regex that matches it in QM's executed commands. Evals use it to score whether the skill's first step ran.
## Change it
The skills are generated. Don't edit `qm/` by hand. The edition spec is `hosts/qm.ts` (the sections kept from each gstack skill, the QM header, the limits and the allowed commands), the review script is `hosts/qm/scripts/diff-base.sh`, and the rest comes from the gstack skill templates. Regenerate with:
```sh
bun run scripts/gen-qm-edition.ts
```
`test/qm-edition.test.ts` fails when `qm/` is stale, when a file or description is over its limit, or when a skill names a command QM's sandbox doesn't have. Generation fails as soon as a kept heading, dropped block or replaced phrase stops matching its source template, so an upstream edit gets an explicit decision.
+34
View File
@@ -0,0 +1,34 @@
{
"edition": "gstack-qm",
"layer_root": "qm/sandbox/skills",
"skills": [
{
"id": "gstack-investigate",
"source": "investigate",
"files": [
"SKILL.md"
],
"first_step": "git log --oneline -20 -- <path>",
"first_step_marker": "\\bgit\\s+log\\b"
},
{
"id": "gstack-review",
"source": "review",
"files": [
"SKILL.md",
"scripts/diff-base.sh"
],
"first_step": "sh skills/gstack-review/scripts/diff-base.sh <base>",
"first_step_marker": "diff-base\\.sh|\\bgit\\s+(diff|merge-base)\\b"
},
{
"id": "gstack-plan",
"source": "plan-eng-review",
"files": [
"SKILL.md"
],
"first_step": "git log --oneline -10",
"first_step_marker": "\\bgit\\s+log\\b"
}
]
}
@@ -0,0 +1,93 @@
---
name: gstack-investigate
description: "Use when a test fails, code errors or output is wrong: find and prove the root cause before any fix, then fix it with a regression test. (gstack)"
---
<!-- Generated by scripts/gen-qm-edition.ts from hosts/qm.ts and investigate/SKILL.md.tmpl, investigate/sections/race.md.tmpl. Do not edit. -->
# Investigate: root cause before any fix
**Start here, before reading code.** In the repository (`cd` into it first: the shell starts in the workspace root), run with `execute`, adding the failing file or directory if you know it:
```sh
git status --short
git log --oneline -20 -- <path>
```
Then reproduce the symptom with the project's own test or run command, and write down what you saw, before you form a hypothesis.
## Iron Law
**NO FIXES WITHOUT ROOT CAUSE INVESTIGATION FIRST.**
Fixing symptoms creates whack-a-mole debugging. Every fix that doesn't address root cause makes the next bug harder to find. Find the root cause, then fix it.
## How to work
Find the root cause, fix it with the smallest change, prove the fix, and finish with the DEBUG REPORT below. The order, tools and depth are yours. Three lessons that cost real time before:
- Before calling two queries, helpers or code paths duplicates, read their call sites: what looks identical usually differs in a caller's guard, transaction or scope, and the "duplicate" is the one that is right.
- Before timing anything that spawns git in a benchmark or bisect worktree, run `git update-index --refresh` there: a stale index makes the first `git status`/`git diff` pay a full refresh and the measurement lies.
- Two fixes that shared one premise failed the same gate — write the premise down in one sentence and test it before a third fix. The evidence already points away from it.
Three guards:
- **3-strike rule:** if 3 hypotheses fail, **STOP** and ask the user: A) continue with a new hypothesis (describe it), B) escalate for human review, C) add logging and wait.
- **Blast radius:** if the fix touches more than 5 files, ask the user: A) proceed, the root cause spans these files, B) split, fixing the critical path now, C) rethink for a more targeted approach.
- **Web searches:** search the generic error class, library and version, never the raw error text, hostnames, IPs, file paths, SQL, customer data or secrets.
---
## Race recipe
A race is two actors touching shared state with an ordering nobody enforces.
Make it deterministic before fixing it.
1. **Timing probes.** Log a monotonic timestamp (`performance.now()`) and the
actor id at every read and write of the shared state (file, row, map, lock).
A race shows as interleaved writes or a read between another actor's
check and its write. Keep the probe output as evidence.
2. **Forced interleaving.** Inject an `await` or sleep at the suspected point
in one actor (behind a test-only hook or env var) so the other actor runs in
the window. The bug must now appear every time; if it does not, the window is
elsewhere.
3. **Concurrent-writer template.** Drive N writers at once and assert the
invariant, not the absence of an error:
```ts
const results = await Promise.allSettled(Array.from({ length: N }, (_, i) => writer(i)));
// exactly one winner | all N applied | file or row intact | lock released and owner recorded
```
Run with N ≥ 8 under the forced interleaving; the template becomes the
regression test with the interleaving still injected.
4. **Fix shape.** An atomic primitive: create with `wx`, compare-and-swap,
`UPDATE … WHERE old_value = ?`, a lock with an owner token and a stale-owner
rule. A check-then-act with a smaller window is not a fix. Rerun step 3
after the change and paste both outputs.
## Report
Output a structured debug report:
```
DEBUG REPORT
════════════════════════════════════════
Symptom: [what the user observed]
Root cause: [what was actually wrong]
Fix: [what was changed, with file:line references]
Evidence: [test output, reproduction attempt showing fix works]
Regression test: [file:line of the new test]
Related: [TODOS.md items, prior bugs in same area, architectural notes]
Status: DONE | DONE_WITH_CONCERNS | BLOCKED
════════════════════════════════════════
```
---
## Important Rules
- **3+ failed fix attempts → STOP and question the architecture.** Wrong architecture, not failed hypothesis.
- **Never apply a fix you cannot verify.** If you can't reproduce and confirm, don't ship it.
- **Never say "this should fix it."** Verify and prove it. Run the tests.
- **If fix touches >5 files → ask the user** about blast radius before proceeding.
- **Completion status:**
- DONE — root cause found, fix applied, regression test written, all tests pass
- DONE_WITH_CONCERNS — fixed but cannot fully verify (e.g., intermittent bug, requires staging)
- BLOCKED — root cause unclear after investigation, escalated
+130
View File
@@ -0,0 +1,130 @@
---
name: gstack-plan
description: "Use before building a feature: challenge the scope, then review architecture, code quality, tests and performance, and end with a plan to build. (gstack)"
---
<!-- Generated by scripts/gen-qm-edition.ts from hosts/qm.ts and plan-eng-review/SKILL.md.tmpl, plan-eng-review/sections/review-sections.md.tmpl. Do not edit. -->
# Plan a feature: scope first, then the engineering review
Review a plan, not code: do not build the feature. If the user has not given you the plan or feature description, ask for it and stop.
**Start here.** In the repository (`cd` into it first: the shell starts in the workspace root), run with `execute` to see what already exists:
```sh
git log --oneline -10
ls
```
Then read the files the plan touches. Work through Scope Challenge, then Sections 1-4 in order; for each finding give severity, a recommendation and the alternative. Ask about decisions that change scope in one message and stop until the user answers. Finish with the revised plan: what to build, in order, the files it touches, the tests that prove it, and what is NOT in scope.
## My engineering preferences (use these to guide your recommendations):
* **Shared code:** require common behavior and improved reliability or net savings; similar-looking code alone is insufficient.
* **Tests:** every behavior tested; no test without a regression it would catch.
* **Enough engineering:** avoid fragility and premature abstraction/complexity.
* **Edge cases:** thorough handling over speed.
* **Explicit over clever.**
* **Right-sized diff:** smallest clear change; rewrite a broken foundation when necessary.
* **Data model:** storage follows the data's queries, integrity rules and update boundaries, not file count: fields queried, constrained or updated on their own get explicit columns or tables; JSON suits a document read and written whole.
## A. Assess the target
Complete these checks before proposing scope changes. Do not write findings into the plan yet.
- **What already solves each sub-problem?** Inspect helpers, libraries, callers and reusable outputs: behavior and dependency/deployment boundaries. Cite authored sources; label proposed callers with their motivating plan requirement and assumptions.
- **What minimum changes achieve the goal?** Flag work deferrable without blocking it; challenge scope creep.
- **Complexity check:** Count the selected work, not files read only as evidence:
for a plan, its proposed changed files and new classes/services; for a diff,
changed files and classes/services introduced by that diff; for a file/directory,
files in that selected scope and any explicitly proposed new classes/services.
Count each once, label estimates, and seek fewer moving parts. At 8+ files or 2+ new classes or services, propose a smaller arrangement and ask before changing scope.
- **Distribution check:** For new artifacts, verify build/publish CI/CD, target
OS/architectures and download/install channels. Put deferrals in "NOT in scope".
## Review Sections (after scope is agreed)
Evaluate Architecture → Code Quality → Tests → Performance. Never condense,
abbreviate or skip a section, including strategy/spec/infra plans.
### 1. Architecture review
Evaluate:
* System/component boundaries, dependencies and coupling.
* Data flow, bottlenecks, scaling and single points of failure.
* Security: auth, data access and API boundaries.
* Key flows needing ASCII diagrams in plans/code.
* One realistic production failure per new path/integration; does the plan handle it?
* **Distribution architecture:** New artifacts' build, publish and update paths; included or deferred CI/CD.
<!-- "Design it twice" and the red flags derive from pstack's architect skill (Lauren Tan, MIT), rewritten for gstack. See NOTICE.md. -->
### Two shapes before one (design-twice)
Run this only when the plan introduces a new module, service, package or
boundary: a public surface other code calls; otherwise say so in one line.
**Ask for the second shape.** A plan with one design usually took the first
shape that compiled in the author's head. Before judging it, require a second
shape that is structurally different: another owner for the state, another
split of responsibilities, another place for the boundary. A point fix inside
the first shape is not a second candidate. When the plan has none, sketch one
in five lines (types and signatures, bodies `not implemented`) and record it
as the alternative. Compare the candidates on interface depth: how much
behavior and policy each hides behind how small a public surface, and prefer
the one that hides more behind less. A deep module is not a deep call chain:
the chain scatters the understanding across layers, the module concentrates
it behind one interface.
**The eight red flags.** Screen the chosen shape before Section 1 closes.
Assume the next contributor is an agent that sees only the files it opened,
copies the nearest example and takes the shortest path that compiles; prefer
the design where a change that looks right from one file is right for the
whole repo. A flag is a finding with a file or symbol and a reason to revise
the shape, not a comment asking for care.
1. **Shallow module.** A large interface over little hidden complexity:
several calls per operation, options that expose internal stages.
2. **Information leakage.** One decision (a representation, a policy, a wire
or storage detail) lives in more than one module. Re-exported transport
types are leakage; parse external data into domain types at the boundary.
3. **Temporal decomposition.** Modules split by execution order (load,
validate, transform, save) instead of by the knowledge they own.
4. **Pass-through method.** A function that forwards the same arguments to
another with the same shape; remove it or move the responsibility.
5. **Split ownership.** Two modules write the same state or each keep a copy;
an agent editing one cannot see the other. One owner per piece of state.
6. **Two ways to do one task.** An agent copies whichever it meets first.
Keep one; move the callers and delete the rest in the same change.
7. **Importable internals.** The shortest path that compiles imports them and
they become interface. Make an outside import fail the build.
8. **Hand-synced list.** The same items in two places; an agent that sees one
list edits one. Derive the second, or fail the build when they disagree.
Record under Architecture: both shapes, the pick, the depth argument for it,
and each flag with its fix.
### 2. Code quality review
Evaluate:
* Organization and module structure.
* Shared-code opportunities in the target and related callers. No standalone history/PR sweep or quotas. Check proposed caller assumptions against existing interfaces.
* Explicitly flag error handling gaps and missing edge cases.
* Technical debt, fragility and needless complexity per engineering preferences.
* Accuracy of touched files' ASCII diagrams.
### 3. Test review
For shared-code changes, audit existing/missing shared-contract tests (behavior,
errors, side effects, boundaries) and each migrated caller's integration/differences.
Rejected extractions still need coverage for real duplicated-code defects.
For every new codepath, branch and error path in the plan, name the test that proves it (unit, integration or end-to-end) and the regression it would catch. Missing tests become plan items.
### 4. Performance review
Evaluate:
* N+1 queries and database access patterns.
* Memory usage.
* Caching opportunities.
* Slow or complex paths.
On per-request or looped paths, check queries in loops, unbounded
queries or caches, missing indexes, whole-file loads and blocking calls without
timeouts. Give each finding's scale (rows, requests/s, bytes) or mark it
unknown; never invent benchmarks.
+157
View File
@@ -0,0 +1,157 @@
---
name: gstack-review
description: "Use before merging a branch or PR: review the diff for bugs that pass CI but break production, fix the obvious ones and ask about the rest. (gstack)"
---
<!-- Generated by scripts/gen-qm-edition.ts from hosts/qm.ts and review/checklist.md, review/SKILL.md.tmpl. Do not edit. -->
# Review: find what passes CI but breaks in production
**Start here.** In the repository (`cd` into it first: the shell starts in the workspace root), run with `execute` (pass the base branch if you know it):
```sh
sh skills/gstack-review/scripts/diff-base.sh <base>
```
It prints the base, the merge base, the commits since the base and the diff stat, then `NEXT: git diff <merge base>`; run that and read the whole diff and the untracked files it lists before you comment. On `NOTHING_TO_REVIEW`, say so and stop. On `BASE: none`, ask the user for the base branch or review the uncommitted changes it shows.
QM fills in the absolute path of the folder its `skill` tool synced (it prints that path too). If the script is missing, run this instead:
```sh
git merge-base origin/main HEAD
git diff --stat <merge-base>
```
## How to review
1. Read the full diff, then the unchanged callers of anything it changes.
2. Apply the CRITICAL categories, then the INFORMATIONAL ones, below. Respect the suppressions.
3. Classify each finding with the Fix-First Heuristic: fix AUTO-FIX items directly; for ASK items, ask the user in one message (a numbered list, each with a recommended fix) and stop until they answer.
4. Verify each claim (below), then report: `CRITICAL` and `INFORMATIONAL` lists, one line problem and one line fix each, with file:line, then what you fixed. `NO FINDINGS` is a complete result.
## Review Categories
### Pass 1 — CRITICAL
#### SQL & Data Safety
- String interpolation in SQL (even if values are `.to_i`/`.to_f` — use parameterized queries (Rails: sanitize_sql_array/Arel; Node: prepared statements; Python: parameterized queries))
- TOCTOU races: check-then-set patterns that should be atomic `WHERE` + `update_all`
- Bypassing model validations for direct DB writes (Rails: update_column; Django: QuerySet.update(); Prisma: raw queries)
- N+1 queries: Missing eager loading (Rails: .includes(); SQLAlchemy: joinedload(); Prisma: include) for associations used in loops/views
#### Race Conditions & Concurrency
- Read-check-write without uniqueness constraint or catch duplicate key error and retry (e.g., `where(hash:).first` then `save!` without handling concurrent insert)
- find-or-create without unique DB index — concurrent calls can create duplicates
- Status transitions that don't use atomic `WHERE old_status = ? UPDATE SET new_status` — concurrent updates can skip or double-apply transitions
- Unsafe HTML rendering (Rails: .html_safe/raw(); React: dangerouslySetInnerHTML; Vue: v-html; Django: |safe/mark_safe) on user-controlled data (XSS)
#### LLM Output Trust Boundary
- LLM-generated values (emails, URLs, names) written to DB or passed to mailers without format validation. Add lightweight guards (`EMAIL_REGEXP`, `URI.parse`, `.strip`) before persisting.
- Structured tool output (arrays, hashes) accepted without type/shape checks before database writes.
- LLM-generated URLs fetched without allowlist — SSRF risk if URL points to internal network (Python: `urllib.parse.urlparse` → check hostname against blocklist before `requests.get`/`httpx.get`)
- LLM output stored in knowledge bases or vector DBs without sanitization — stored prompt injection risk
#### Shell Injection (Python-specific)
- `subprocess.run()` / `subprocess.call()` / `subprocess.Popen()` with `shell=True` AND f-string/`.format()` interpolation in the command string — use argument arrays instead
- `os.system()` with variable interpolation — replace with `subprocess.run()` using argument arrays
- `eval()` / `exec()` on LLM-generated code without sandboxing
#### Enum & Value Completeness
When the diff introduces a new enum value, status string, tier name, or type constant, or loosens what an input accepts (a new MIME type, extension or flag, or a widened validator/parser/guard):
- **Trace it through every consumer.** Read (don't just grep — READ) each file that switches on, filters by, or displays that value. If any consumer doesn't handle the new value, flag it. Common miss: adding a value to the frontend dropdown but the backend model/compute method doesn't persist it.
- **Check allowlists/filter arrays.** Search for arrays or `%w[]` lists containing sibling values (e.g., if adding "revise" to tiers, find every `%w[quick lfg mega]` and verify "revise" is included where needed).
- **Check `case`/`if-elsif` chains.** If existing code branches on the enum, does the new value fall through to a wrong default?
- **Loosened acceptance breaks unchanged consumers.** Grep every consumer of the same field and confirm each handles the newly accepted shape; the broken code is usually outside the diff.
To do this: use Grep to find all references to the sibling values (e.g., grep for "lfg" or "mega" to find all tier consumers). Read each match. This step requires reading code OUTSIDE the diff.
### Pass 2 — INFORMATIONAL
#### Async/Sync Mixing (Python-specific)
- Synchronous `subprocess.run()`, `open()`, `requests.get()` inside `async def` endpoints — blocks the event loop. Use `asyncio.to_thread()`, `aiofiles`, or `httpx.AsyncClient` instead.
- `time.sleep()` inside async functions — use `asyncio.sleep()`
- Sync DB calls in async context without `run_in_executor()` wrapping
#### Column/Field Name Safety
- Verify column names in ORM queries (`.select()`, `.eq()`, `.gte()`, `.order()`) against actual DB schema — wrong column names silently return empty results or throw swallowed errors
- Check `.get()` calls on query results use the column name that was actually selected
- Cross-reference with schema documentation when available
#### LLM Prompt Issues
- 0-indexed lists in prompts (LLMs reliably return 1-indexed)
- Prompt text listing available tools/capabilities that don't match what's actually wired up in the `tool_classes`/`tools` array
- Word/token limits stated in multiple places that could drift
#### Completeness Gaps
- Shortcut implementations where the complete version would cost <30 minutes CC time (e.g., partial enum handling, incomplete error paths, missing edge cases that are straightforward to add)
- Test coverage gaps where adding the missing tests is a "lake" not an "ocean" (e.g., missing negative-path tests, missing edge case tests that mirror happy-path structure)
- Features implemented at 80-90% when 100% is achievable with modest additional code
#### Time Window Safety
- Date-key lookups that assume "today" covers 24h — report at 8am PT only sees midnight→8am under today's key
- Mismatched time windows between related features — one uses hourly buckets, another uses daily keys for the same data
#### Timing-Dependent Tests
- A diff that raises an existing timing bound or adds a retry to make a flake pass: the bound is not the cause; find its cause first (gstack-investigate)
#### Type Coercion at Boundaries
- Values crossing Ruby→JSON→JS boundaries where type could change (numeric vs string) — hash/digest inputs must normalize types
- Hash/digest inputs that don't call `.to_s` or equivalent before serialization — `{ cores: 8 }` vs `{ cores: "8" }` produce different hashes
#### View/Frontend
- Inline `<style>` blocks in partials (re-parsed every render)
- O(n*m) lookups in views (`Array#find` in a loop instead of `index_by` hash)
- Ruby-side `.select{}` filtering on DB results that could be a `WHERE` clause (unless intentionally avoiding leading-wildcard `LIKE`)
## Fix-First Heuristic
```
AUTO-FIX (agent fixes without asking): ASK (needs human judgment):
├─ Dead code / unused variables ├─ Security (auth, XSS, injection)
├─ N+1 queries (missing eager loading) ├─ Race conditions
├─ Stale comments contradicting code ├─ Design decisions
├─ Magic numbers → named constants ├─ Large fixes (>20 lines)
├─ Missing LLM output validation ├─ Enum completeness
├─ Version/path mismatches ├─ Removing functionality
├─ Variables assigned but never read └─ Anything changing user-visible
└─ Inline styles, O(n*m) view lookups behavior
```
**Rule of thumb:** If the fix is mechanical and a senior engineer would apply it
without discussion, it's AUTO-FIX. If reasonable engineers could disagree about
the fix, it's ASK.
**Critical findings default toward ASK** (they're inherently riskier).
**Informational findings default toward AUTO-FIX** (they're more mechanical).
---
## Suppressions — DO NOT flag these
Zero findings is a valid outcome. Flag only what a senior engineer on this team would change in review; if the diff is clean, say so.
- "X is redundant with Y" when the redundancy is harmless and aids readability (e.g., `present?` redundant with `length > 20`)
- "Add a comment explaining why this threshold/constant was chosen" — thresholds change during tuning, comments rot
- "This assertion could be tighter" when the assertion already covers the behavior
- Suggesting consistency-only changes (wrapping a value in a conditional to match how another constant is guarded)
- "Regex doesn't handle edge case X" when the input is constrained and X never occurs in practice
- "Test exercises multiple guards simultaneously" — that's fine, tests don't need to isolate every guard
- Harmless no-ops (e.g., `.reject` on an element that's never in the array)
- ANYTHING already addressed in the diff you're reviewing — read the FULL diff before commenting
- "Consider adding error handling" on a call whose errors the caller or framework already owns (error middleware, an error boundary, a top-level try/catch, an upstream `.catch`)
- "Function too long" for an exhaustive `switch`, a config object, a test table or generated code — length is not complexity
- "Possible null dereference" when a type guard or `if` check in scope already narrows the value — trace the type flow
- "N+1 query" on a loop with a small fixed cardinality, or on a path that is already batched
- "Hardcoded value" in test fixtures, example code or docs — tests are supposed to hardcode their expectations
- `Math.random()` in a non-cryptographic context (jitter, sampling, animation) — security-sensitive randomness is still flagged
## Verification of claims
Before final output, cite the line proving a safety claim, read and cite any
handling code you rely on, and name the test file and method for coverage claims.
Verify claims or flag them as unknown; "this looks fine" is not evidence.
## Important Rules
- **Read the FULL diff before commenting.** Do not flag issues already addressed in the diff.
- **Fix-first, not read-only.** AUTO-FIX items are applied directly, ASK items only after user approval. Never commit, push, or create PRs — leave that to the user.
- **Be terse.** One line problem, one line fix. No preamble.
- **Only flag real problems.** Skip anything that's fine.
@@ -0,0 +1,48 @@
#!/bin/sh
# gstack-review first step: find the base, the merge base and what changed.
# Usage: sh diff-base.sh [base] (POSIX sh and git only)
set -u
if ! git rev-parse --is-inside-work-tree >/dev/null 2>&1; then
echo "NOT_A_GIT_REPO: $(pwd)"
exit 0
fi
no_base() {
echo "BASE: none ($1)"
echo "== uncommitted changes"
git status --short | head -n 40
if git rev-parse --verify --quiet HEAD >/dev/null; then
echo "== diff stat against HEAD"
git diff --stat HEAD --
fi
echo "NEXT: pass the base branch (sh diff-base.sh <base>) or review the uncommitted changes with git diff HEAD"
exit 0
}
base=${1:-}
if [ -z "$base" ]; then
base=$(git symbolic-ref --quiet --short refs/remotes/origin/HEAD 2>/dev/null || true)
fi
if [ -z "$base" ]; then
for candidate in origin/main main origin/master master; do
if git rev-parse --verify --quiet "$candidate^{commit}" >/dev/null; then
base=$candidate
break
fi
done
fi
[ -n "$base" ] || no_base "no origin/HEAD, main or master"
merge_base=$(git merge-base "$base" HEAD 2>/dev/null) || no_base "$base has no merge base with HEAD"
echo "BRANCH: $(git symbolic-ref --quiet --short HEAD 2>/dev/null || echo detached)"
echo "BASE: $base"
echo "MERGE_BASE: $merge_base"
echo "== commits since base"
git log --oneline "$merge_base..HEAD" -- | head -n 30
echo "== diff stat (working tree against merge base)"
git diff --stat "$merge_base" --
untracked=$(git ls-files --others --exclude-standard)
echo "== untracked files"
printf '%s\n' "$untracked" | head -n 30
if git diff --quiet "$merge_base" -- && [ -z "$untracked" ]; then
echo "NOTHING_TO_REVIEW"
exit 0
fi
echo "NEXT: git diff $merge_base"
+273
View File
@@ -0,0 +1,273 @@
#!/usr/bin/env -S bun --no-env-file --config=/dev/null
/**
* Generate the QM edition (hosts/qm.ts) into qm/sandbox/skills/<id>/ and qm/edition.json.
*
* Each skill is a QM-authored header plus sections cut by heading from its
* gstack sources, so source edits flow in on the next run. Generation fails
* closed: a kept heading, skipped heading or dropped block that no longer
* exists, an unmapped {{RESOLVER}}, a file over the size limit, forbidden
* text, or a command QM's sandbox can't run stops it.
*
* bun run scripts/gen-qm-edition.ts write the edition
* bun run scripts/gen-qm-edition.ts --check exit 1 when the committed edition is stale
*/
import * as fs from 'node:fs';
import * as path from 'node:path';
import { QM_EDITION, QM_IMAGE_COMMANDS, QM_POSIX_COMMANDS, type QmPart, type QmSkill } from '../hosts/qm';
const ROOT = path.resolve(import.meta.dir, '..');
const HEADING = /^(#{1,6}) +(.+?) *$/;
const FENCE = /^ *(```|~~~)/;
const GENERATED = '<!-- Generated by scripts/gen-qm-edition.ts';
const SHELL_FENCE = /^ *```(sh|bash|shell|zsh)\b/;
interface Line { text: string; level: number; title: string }
/** Lines with their heading level; a # line inside a code fence is never a heading. */
function scanLines(text: string): Line[] {
let open = false;
return text.split('\n').map(line => {
if (FENCE.test(line)) open = !open;
const m = !open && !FENCE.test(line) ? HEADING.exec(line) : null;
return { text: line, level: m ? m[1]!.length : 0, title: m ? m[2]! : '' };
});
}
function sectionRange(lines: Line[], title: string, where: string): [number, number] {
const hits = lines.flatMap((l, i) => (l.title === title ? [i] : []));
if (hits.length !== 1) throw new Error(`${where}: heading "${title}" found ${hits.length} times (want 1)`);
const start = hits[0]!;
let end = start + 1;
while (end < lines.length && !(lines[end]!.level && lines[end]!.level <= lines[start]!.level)) end++;
return [start, end];
}
/** Blocks: a fenced block, a list item with its continuation, a heading, or a paragraph; blank lines kept as ''. */
function splitBlocks(text: string): string[] {
const blocks: string[] = [];
let current: string[] = [];
let inFence = false;
const flush = () => { if (current.length) blocks.push(current.join('\n')); current = []; };
for (const line of text.split('\n')) {
if (inFence) {
current.push(line);
if (FENCE.test(line)) { inFence = false; flush(); }
} else if (FENCE.test(line)) {
flush(); current.push(line); inFence = true;
} else if (line.trim() === '') {
flush(); blocks.push('');
} else {
if (/^ {0,3}([-*]|\d+\.) /.test(line) || HEADING.test(line)) flush();
current.push(line);
}
}
flush();
const merged: string[] = [];
for (const block of blocks) {
const prev = merged.findLastIndex(b => b !== '');
const continues = /^\s+\S/.test(block) && prev >= 0 && /^ {0,3}([-*]|\d+\.) |^\s+\S/.test(merged[prev]!);
if (continues) merged.splice(prev, merged.length - prev, [merged[prev], ...merged.slice(prev + 1), block].join('\n'));
else merged.push(block);
}
return merged;
}
function cutPart(part: QmPart, where: string): string {
const at = `${where} ${part.file}`;
const lines = scanLines(fs.readFileSync(path.join(ROOT, part.file), 'utf-8'));
const [start, end] = sectionRange(lines, part.heading, at);
const keep = lines.slice(start, end);
for (const skip of part.skipHeadings ?? []) {
const [s, e] = sectionRange(keep, skip, `${at} skipHeadings`);
keep.splice(s, e - s);
}
const shift = part.level - keep[0]!.level;
let text = keep.map(l => (l.level ? `${'#'.repeat(l.level + shift)} ${l.title}` : l.text)).join('\n');
for (const needle of part.dropBlocks ?? []) {
const blocks = splitBlocks(text);
const kept = blocks.filter(b => !b.includes(needle));
if (kept.length === blocks.length) throw new Error(`${at}: dropBlocks "${needle}" matched nothing`);
text = kept.join('\n');
}
for (const [from, to] of part.replaceText ?? []) {
if (!text.includes(from)) throw new Error(`${at}: replaceText "${from}" matched nothing`);
text = text.split(from).join(to);
}
return text;
}
function applyResolvers(text: string, where: string): string {
return text.replace(/\{\{([A-Z_]+)(:[^}]*)?\}\}/g, (match, name: string) => {
if (!QM_EDITION.dropResolvers.includes(name)) {
throw new Error(`${where}: unmapped placeholder ${match}; add it to QM_EDITION.dropResolvers or drop its block`);
}
return '';
});
}
function tidy(text: string): string {
return text.split('\n').map(l => l.replace(/[ \t]+$/, '')).join('\n').replace(/\n{3,}/g, '\n\n').trim() + '\n';
}
/** One skill's files, keyed by path inside its folder. */
export function renderSkill(skill: QmSkill): Map<string, string> {
const where = `qm edition ${skill.id}`;
let body = applyResolvers(skill.parts.map(p => cutPart(p, where)).join('\n\n'), where);
for (const [from, to] of QM_EDITION.rewrites) body = body.split(from).join(to);
const sources = [...new Set(skill.parts.map(p => p.file))].join(', ');
const files = new Map<string, string>();
files.set('SKILL.md', tidy([
'---',
`name: ${skill.id}`,
`description: "${skill.description}"`,
'---',
`${GENERATED} from hosts/qm.ts and ${sources}. Do not edit. -->`,
'',
skill.header,
'',
body,
].join('\n')));
for (const [name, src] of Object.entries(skill.scripts)) {
files.set(`scripts/${name}`, fs.readFileSync(path.join(ROOT, QM_EDITION.scriptSourceDir, src), 'utf-8'));
}
return files;
}
const KEYWORDS = new Set(['if', 'then', 'else', 'elif', 'do', 'while', 'until', '!', '{', 'time']);
const CLOSERS = new Set(['fi', 'done', 'esac', '}']);
const NOT_QM = new Set(['bun', 'bunx', 'node', 'npm', 'npx', 'pnpm', 'yarn', 'python', 'python3', 'pip', 'jq', 'curl', 'gh', 'rg', 'claude', 'codex', 'make']);
const ALLOWED = new Set([...QM_POSIX_COMMANDS, ...QM_IMAGE_COMMANDS]);
/** The words of every simple command in a shell snippet: pipeline stages, list members and $( ) substitutions. */
export function commandWords(snippet: string): { functions: string[]; commands: string[][] } {
const extra: string[] = [];
const functions: string[] = [];
let text = snippet.replace(/(^|\s)#.*$/gm, '$1').replace(/'[^']*'/g, "''").replace(/\d*>&\d*-?/g, ' ');
text = text.replace(/"((?:[^"\\]|\\.)*)"/g, (_m, inner: string) => {
for (const sub of inner.matchAll(/\$\(([^()]*)\)/g)) extra.push(sub[1]!);
return '""';
});
text = text.replace(/^\s*([A-Za-z_]\w*)\s*\(\)/gm, (_m, name: string) => { functions.push(name); return ''; });
const commands: string[][] = [];
for (const segment of [text, ...extra].join('\n').split(/\|\||&&|\$\(|[|;&()`\n]/)) {
const words = segment.trim().split(/\s+/).filter(Boolean);
while (words.length && (KEYWORDS.has(words[0]!) || /^[A-Za-z_]\w*=/.test(words[0]!))) words.shift();
if (!words.length || CLOSERS.has(words[0]!) || words[0] === 'for' || words[0] === 'case') continue;
commands.push(words);
}
return { functions, commands };
}
/** Why the files of skill `id` name something QM's sandbox can't run or the edition doesn't ship; empty when clean. */
export function qmCommandViolations(id: string, files: Map<string, string>): string[] {
const shipped = new Set([...files.keys()].filter(f => f.startsWith('scripts/')).map(f => f.slice('scripts/'.length)));
const ids = new Set(QM_EDITION.skills.map(s => s.id));
const problems: string[] = [];
const check = (snippet: string, where: string) => {
const { functions, commands } = commandWords(snippet);
for (const [cmd, arg] of commands) {
if (cmd === 'sh') {
const script = (arg ?? '').replace(/^"|"$/g, '');
const prefix = `skills/${id}/scripts/`;
if (!script.startsWith(prefix) || !shipped.has(script.slice(prefix.length))) {
problems.push(`${where}: sh runs "${script}", not a script shipped in ${prefix}`);
}
} else if (!ALLOWED.has(cmd!) && !functions.includes(cmd!)) {
problems.push(`${where}: "${cmd}" is neither POSIX, an image binary (${QM_IMAGE_COMMANDS.join(', ')}) nor a shipped script`);
}
}
};
for (const [file, content] of files) {
const where = `${id}/${file}`;
const prose = content.split('\n').filter(l => !l.startsWith(GENERATED)).join('\n');
for (const token of QM_EDITION.forbiddenText) {
if (prose.includes(token)) problems.push(`${where}: names "${token}"`);
}
for (const m of prose.matchAll(/\bgstack-[a-z0-9-]+/g)) {
if (!ids.has(m[0])) problems.push(`${where}: names "${m[0]}", a gstack command QM doesn't have`);
}
if (file.startsWith('scripts/')) { check(content, where); continue; }
let fence: string[] | null = null;
for (const line of content.split('\n')) {
if (fence) {
if (FENCE.test(line)) { check(fence.join('\n'), `${where} shell block`); fence = null; } else fence.push(line);
continue;
}
if (SHELL_FENCE.test(line)) { fence = []; continue; }
for (const span of line.matchAll(/`([^`]+)`/g)) {
const first = span[1]!.trim().split(/\s+/)[0]!;
if (ALLOWED.has(first) || NOT_QM.has(first)) check(span[1]!, `${where} inline`);
}
}
}
return problems;
}
/** Every file of the edition, keyed by path relative to the gstack root; throws when any rule fails. */
export function renderQmEdition(): Map<string, string> {
const out = new Map<string, string>();
const problems: string[] = [];
const { fileChars, descriptionChars, indexChars } = QM_EDITION.limits;
let index = 0;
const manifest = QM_EDITION.skills.map(skill => {
const files = renderSkill(skill);
problems.push(...qmCommandViolations(skill.id, files));
const d = skill.description;
if (d.length > descriptionChars) problems.push(`${skill.id}: description is ${d.length} chars (limit ${descriptionChars})`);
if (d.includes('"') || d.includes('\n')) problems.push(`${skill.id}: description must be one line without double quotes`);
const skillMd = files.get('SKILL.md')!;
const firstDerived = skillMd.indexOf(skill.parts[0]!.heading);
const step = skillMd.indexOf(skill.firstStep.command);
if (step < 0 || step > firstDerived) problems.push(`${skill.id}: first step "${skill.firstStep.command}" is not in the header`);
if (!new RegExp(skill.firstStep.marker).test(skill.firstStep.command)) problems.push(`${skill.id}: marker does not match the first step`);
index += skill.id.length + 3 + d.length;
for (const [file, content] of files) {
if (content.length > fileChars) problems.push(`${skill.id}/${file}: ${content.length} chars (limit ${fileChars})`);
out.set(path.posix.join(QM_EDITION.outDir, skill.id, file), content);
}
return { id: skill.id, source: skill.source, files: [...files.keys()], first_step: skill.firstStep.command, first_step_marker: skill.firstStep.marker };
});
if (index > indexChars) problems.push(`skills index entries total ${index} chars (limit ${indexChars})`);
if (problems.length) throw new Error(`QM edition is invalid:\n ${problems.join('\n ')}`);
out.set(QM_EDITION.manifest, JSON.stringify({ edition: 'gstack-qm', layer_root: QM_EDITION.outDir, skills: manifest }, null, 2) + '\n');
return out;
}
function onDisk(): string[] {
const dir = path.join(ROOT, QM_EDITION.outDir);
const files = fs.existsSync(dir)
? (fs.readdirSync(dir, { recursive: true }) as string[]).map(f => path.posix.join(QM_EDITION.outDir, f.split(path.sep).join('/')))
: [];
return [...files, QM_EDITION.manifest].filter(f => fs.existsSync(path.join(ROOT, f)) && fs.statSync(path.join(ROOT, f)).isFile());
}
/** Paths whose committed bytes differ from the generator's, or that only one side has. */
export function staleQmFiles(): string[] {
const want = renderQmEdition();
const stale = onDisk().filter(f => !want.has(f));
for (const [f, content] of want) {
const abs = path.join(ROOT, f);
if (!fs.existsSync(abs) || fs.readFileSync(abs, 'utf-8') !== content) stale.push(f);
}
return stale.sort();
}
if (import.meta.main) {
if (process.argv.includes('--check')) {
const stale = staleQmFiles();
if (stale.length) {
console.error(`QM edition is stale (${stale.join(', ')}). Run: bun run scripts/gen-qm-edition.ts`);
process.exit(1);
}
console.log('QM edition: fresh');
} else {
const want = renderQmEdition();
for (const f of onDisk()) if (!want.has(f)) fs.rmSync(path.join(ROOT, f));
for (const [f, content] of want) {
fs.mkdirSync(path.dirname(path.join(ROOT, f)), { recursive: true });
fs.writeFileSync(path.join(ROOT, f), content);
}
console.log(`QM edition: wrote ${want.size} files`);
}
}
+4
View File
@@ -32,6 +32,7 @@ import {
buildRatchetBill,
captureContextBudget,
isFixtureSkill,
isQmEditionSkill,
toPosixName,
BUDGET_FIXTURE_PATH,
ALWAYS_ON_HEADROOM,
@@ -116,6 +117,9 @@ describe('context-budget ratchet', () => {
expect(isFixtureSkill(['test', 'fixtures', 'x'].join(path.sep))).toBe(true);
expect(isFixtureSkill('test/fixtures/context-bill/tree-a/alpha')).toBe(true);
expect(isFixtureSkill('openclaw/skills/gstack-openclaw-retro')).toBe(false);
expect(isQmEditionSkill(['qm', 'sandbox', 'skills', 'gstack-review'].join(path.sep))).toBe(true);
expect(isQmEditionSkill('openclaw/skills/gstack-openclaw-retro')).toBe(false);
expect(bill.skills.some((s) => s.name.startsWith('qm/'))).toBe(false);
expect(bill.skills.every((s) => !s.name.includes('\\'))).toBe(true);
});
+10 -1
View File
@@ -50,6 +50,15 @@ export function isFixtureSkill(name: string): boolean {
return toPosixName(name).startsWith('test/fixtures/');
}
/**
* The QM edition (qm/, from scripts/gen-qm-edition.ts) ships to QM as a
* deployment layer; no gstack host's skill scanner loads it. Its own limits
* (per-file and skills-index chars) are enforced by test/qm-edition.test.ts.
*/
export function isQmEditionSkill(name: string): boolean {
return toPosixName(name).startsWith('qm/');
}
export interface ContextBudget {
_comment: string;
alwaysOnTotal: number;
@@ -84,7 +93,7 @@ export function buildRatchetBill(root: string = REPO_ROOT): Bill {
...s,
name: s.dir === bill.root ? ROOT_SKILL_KEY : toPosixName(s.name),
}))
.filter((s) => !isFixtureSkill(s.name));
.filter((s) => !isFixtureSkill(s.name) && !isQmEditionSkill(s.name));
// One ceiling per PHYSICAL skill: group by realpath, prefer the entry whose
// dir IS the realpath (the real dir) over symlink aliases.
const byReal = new Map<string, (typeof candidates)[number]>();
+155
View File
@@ -0,0 +1,155 @@
import { describe, expect, test } from 'bun:test';
import { spawnSync } from 'node:child_process';
import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';
import { QM_EDITION, type QmSkill } from '../hosts/qm';
import { commandWords, qmCommandViolations, renderQmEdition, renderSkill, staleQmFiles } from '../scripts/gen-qm-edition';
const ROOT = path.resolve(import.meta.dir, '..');
const edition = renderQmEdition();
const skillFiles = (id: string) => new Map([...edition].filter(([f]) => f.startsWith(`${QM_EDITION.outDir}/${id}/`))
.map(([f, c]) => [f.slice(`${QM_EDITION.outDir}/${id}/`.length), c]));
describe('QM edition (hosts/qm.ts)', () => {
test('committed qm/ matches the generator (run: bun run scripts/gen-qm-edition.ts)', () => {
expect(staleQmFiles()).toEqual([]);
});
test('every shipped file fits QM\'s inline read cap, and descriptions fit its skills index', () => {
const { fileChars, descriptionChars, indexChars } = QM_EDITION.limits;
expect(fileChars).toBeLessThan(16_384);
for (const [file, content] of edition) expect({ file, over: content.length > fileChars }).toEqual({ file, over: false });
for (const s of QM_EDITION.skills) expect(s.description.length).toBeLessThanOrEqual(descriptionChars);
const index = QM_EDITION.skills.reduce((n, s) => n + s.id.length + 3 + s.description.length, 0);
expect(index).toBeLessThanOrEqual(indexChars);
});
test('frontmatter carries exactly the name and description QM\'s layer reader requires', () => {
for (const s of QM_EDITION.skills) {
const md = edition.get(`${QM_EDITION.outDir}/${s.id}/SKILL.md`)!;
const fm = /^---\n([\s\S]*?)\n---\n/.exec(md)![1]!.split('\n');
expect(fm).toEqual([`name: ${s.id}`, `description: "${s.description}"`]);
expect(s.id).toMatch(/^gstack-[a-z0-9-]+$/);
expect(md.slice(md.indexOf('\n---\n') + 5).trim().length).toBeGreaterThan(0);
}
});
test('each first step is literal, in the header and listed with its marker in qm/edition.json', () => {
const manifest = JSON.parse(edition.get(QM_EDITION.manifest)!);
expect(manifest.layer_root).toBe(QM_EDITION.outDir);
for (const s of QM_EDITION.skills) {
const md = edition.get(`${QM_EDITION.outDir}/${s.id}/SKILL.md`)!;
expect(md.indexOf(s.firstStep.command)).toBeGreaterThan(0);
expect(md.indexOf(s.firstStep.command)).toBeLessThan(md.indexOf(s.parts[0]!.heading));
const row = manifest.skills.find((r: { id: string }) => r.id === s.id);
expect(row.first_step).toBe(s.firstStep.command);
expect(new RegExp(row.first_step_marker).test(s.firstStep.command)).toBe(true);
}
});
test('every command a QM skill names is POSIX, git or a script the skill ships', () => {
for (const s of QM_EDITION.skills) expect(qmCommandViolations(s.id, skillFiles(s.id))).toEqual([]);
});
test('the command check refuses what QM\'s sandbox lacks (negative controls)', () => {
const id = 'gstack-review';
const md = (body: string) => new Map([['SKILL.md', body], ['scripts/diff-base.sh', '#!/bin/sh\necho ok\n']]);
const cases: Array<[string, RegExp]> = [
['```bash\ngstack-skill-start --skill review\n```\n', /gstack-skill-start/],
['Run `bun run test` first.\n', /"bun"/],
['```sh\ngit diff | jq .\n```\n', /"jq"/],
['```sh\nsh skills/gstack-review/scripts/missing.sh\n```\n', /not a script shipped/],
['```sh\nsh skills/gstack-plan/scripts/diff-base.sh\n```\n', /not a script shipped/],
['Read ~/.claude/skills/gstack/review/checklist.md\n', /~\/\.claude/],
['{{PREAMBLE}}\n', /\{\{/],
];
for (const [body, want] of cases) expect(qmCommandViolations(id, md(body)).join('\n')).toMatch(want);
const script = new Map([['SKILL.md', 'x\n'], ['scripts/a.sh', '#!/bin/sh\nnode -e 1\n']]);
expect(qmCommandViolations(id, script).join('\n')).toMatch(/"node"/);
expect(qmCommandViolations(id, md('```sh\nsh skills/gstack-review/scripts/diff-base.sh main\n```\n'))).toEqual([]);
});
test('commandWords reads pipelines, substitutions, functions and redirections', () => {
const { functions, commands } = commandWords('f() {\n echo "$(git rev-parse HEAD)" 2>&1 | head -n 1\n}\nx=$(git log) || f "a b"\n');
expect(functions).toEqual(['f']);
expect(commands.map(w => w[0])).toEqual(['echo', 'head', 'git', 'f', 'git']);
});
test('generation fails closed when a source moves', () => {
const base = QM_EDITION.skills[0]!;
const variant = (patch: Partial<QmSkill['parts'][number]>): QmSkill => ({ ...base, parts: [{ ...base.parts[0]!, ...patch }] });
expect(() => renderSkill(variant({ heading: 'No Such Heading' }))).toThrow(/found 0 times/);
expect(() => renderSkill(variant({ dropBlocks: ['text that is not there'] }))).toThrow(/matched nothing/);
expect(() => renderSkill(variant({ replaceText: [['text that is not there', 'x']] }))).toThrow(/matched nothing/);
expect(() => renderSkill(variant({ skipHeadings: ['No Such Heading'] }))).toThrow(/found 0 times/);
expect(() => renderSkill(variant({ heading: 'Systematic Debugging', level: 1 }))).toThrow(/unmapped placeholder/);
// A "# ..." comment inside a code fence is never a heading.
expect(() => renderSkill(variant({ heading: '$HOME-anchored like the careful/freeze frontmatter hooks: early skill bash runs' }))).toThrow(/found 0 times/);
});
});
describe('diff-base.sh (gstack-review first step)', () => {
const script = path.join(ROOT, QM_EDITION.scriptSourceDir, 'diff-base.sh');
const shell = fs.existsSync('/bin/dash') ? '/bin/dash' : 'sh';
const git = (cwd: string, ...args: string[]) => {
const r = spawnSync('git', ['-c', 'user.name=qm-test', '-c', 'user.email=qm-test@example.com', '-c', 'init.defaultBranch=main', ...args], { cwd, encoding: 'utf-8', timeout: 30_000 });
if (r.status !== 0) throw new Error(`git ${args.join(' ')}: ${r.stderr}`);
return r.stdout.trim();
};
const run = (cwd: string, ...args: string[]) => {
const r = spawnSync(shell, [script, ...args], { cwd, encoding: 'utf-8', timeout: 30_000 });
expect(r.status).toBe(0);
return r.stdout;
};
const repo = (branch = 'main') => {
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'qm-diff-base-'));
git(dir, 'init', '-q', '-b', branch);
fs.writeFileSync(path.join(dir, 'a.txt'), 'one\n');
git(dir, 'add', '.');
git(dir, 'commit', '-qm', 'init');
return dir;
};
test('parses under sh -n', () => {
expect(spawnSync(shell, ['-n', script], { timeout: 30_000 }).status).toBe(0);
});
test('a feature branch: base, merge base, diff stat and the next command', () => {
const dir = repo();
const mb = git(dir, 'rev-parse', 'HEAD');
git(dir, 'checkout', '-qb', 'feature');
fs.writeFileSync(path.join(dir, 'a.txt'), 'two\n');
git(dir, 'commit', '-qam', 'change');
fs.writeFileSync(path.join(dir, 'new.txt'), 'new\n');
const out = run(dir);
expect(out).toContain('BASE: main\n');
expect(out).toContain(`MERGE_BASE: ${mb}\n`);
expect(out).toContain('a.txt');
expect(out).toContain('new.txt');
expect(out).toContain(`NEXT: git diff ${mb}\n`);
expect(run(dir, 'main')).toContain('BASE: main\n');
fs.rmSync(path.join(dir, 'new.txt'));
expect(run(dir, 'feature')).toContain('NOTHING_TO_REVIEW');
});
test('origin/HEAD wins when the repository is a clone', () => {
const upstream = repo();
const clone = fs.mkdtempSync(path.join(os.tmpdir(), 'qm-diff-base-clone-'));
git(clone, 'clone', '-q', upstream, '.');
expect(run(clone)).toContain('BASE: origin/main\n');
});
test('clean base branch, no main or master, unrelated history, empty repo and no repo all exit 0 with guidance', () => {
expect(run(repo())).toContain('NOTHING_TO_REVIEW');
expect(run(repo('trunk'))).toContain('BASE: none (no origin/HEAD, main or master)');
const orphan = repo();
git(orphan, 'checkout', '-q', '--orphan', 'other');
git(orphan, 'commit', '-qm', 'unrelated');
expect(run(orphan)).toContain('BASE: none (main has no merge base with HEAD)');
const empty = fs.mkdtempSync(path.join(os.tmpdir(), 'qm-diff-base-empty-'));
git(empty, 'init', '-q');
expect(run(empty)).toContain('BASE: none');
expect(run(fs.mkdtempSync(path.join(os.tmpdir(), 'qm-diff-base-none-')))).toContain('NOT_A_GIT_REPO');
});
});