From eaf1701da2d68b5c03136ff530aa228013eedb7c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 5 Nov 2026 16:38:43 -0800 Subject: [PATCH] QM edition: three gstack skills QM loads on demand (GST-1492) (#4049) * 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> * test: name the QM edition's manifest and shipped script Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * paid lane: classify the QM edition as free-only (no paid case reads qm/) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes diff-base.sh names the untracked files past the first 30 (UNTRACKED_MORE) instead of dropping them silently; the gstack-review first step tells QM to read every untracked file. Review-Finding: p1/outside-voice#1 reader=outside-voice Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: point the QM row in docs/hosts.md at the QM edition Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test: record the non-behavioral HARNESS_VERSION decision for the qm/** free-only row CI run 42135033713 failed test/harness-version.test.ts: scripts/test-pr-profile.ts is a pinned shared-harness file and 87ed675 changed it without a decision. The row only classifies qm/ as free-only; no paid case reads qm/. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gsta-320-qm-edition.md | 1 + docs/hosts.md | 2 +- hosts/qm.ts | 234 +++++++++++++++ hosts/qm/scripts/diff-base.sh | 52 ++++ qm/README.md | 33 +++ qm/edition.json | 34 +++ qm/sandbox/skills/gstack-investigate/SKILL.md | 93 ++++++ qm/sandbox/skills/gstack-plan/SKILL.md | 130 +++++++++ qm/sandbox/skills/gstack-review/SKILL.md | 157 ++++++++++ .../skills/gstack-review/scripts/diff-base.sh | 52 ++++ scripts/gen-qm-edition.ts | 273 ++++++++++++++++++ .../scripts/test-pr-profile.ts.json | 2 +- scripts/test-pr-profile.ts | 1 + test/context-budget-ratchet.test.ts | 4 + test/helpers/capture-context-budget.ts | 11 +- test/qm-edition.test.ts | 169 +++++++++++ 16 files changed, 1245 insertions(+), 3 deletions(-) create mode 100644 changelog.d/gsta-320-qm-edition.md create mode 100644 hosts/qm.ts create mode 100644 hosts/qm/scripts/diff-base.sh create mode 100644 qm/README.md create mode 100644 qm/edition.json create mode 100644 qm/sandbox/skills/gstack-investigate/SKILL.md create mode 100644 qm/sandbox/skills/gstack-plan/SKILL.md create mode 100644 qm/sandbox/skills/gstack-review/SKILL.md create mode 100644 qm/sandbox/skills/gstack-review/scripts/diff-base.sh create mode 100644 scripts/gen-qm-edition.ts create mode 100644 test/qm-edition.test.ts diff --git a/changelog.d/gsta-320-qm-edition.md b/changelog.d/gsta-320-qm-edition.md new file mode 100644 index 000000000..9f681d181 --- /dev/null +++ b/changelog.d/gsta-320-qm-edition.md @@ -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) diff --git a/docs/hosts.md b/docs/hosts.md index 4c465c7f1..c5a5bf05c 100644 --- a/docs/hosts.md +++ b/docs/hosts.md @@ -64,7 +64,7 @@ characters, the part of a description Hermes's skills index shows. | OpenClaw | default `--skills hybrid`: `gstack-review`, `gstack-investigate`, `gstack-qa`, `gstack-ship` and `gstack-plan-eng-review` as top-level skills plus one `gstack` router skill whose body lists every other skill and its path, in `${OPENCLAW_STATE_DIR:-~/.openclaw}/skills`. With OpenClaw 2026.10.6's 19 eligible bundled skills, hybrid's skills section is 10,290 characters, full format, under its 18,000-character budget; `--skills all` (17,971, compact format, descriptions cut) lists every skill as its own folder, `--skills router` (6,701) only the router. OpenClaw serves a registered skill's `SKILL.md` whole or refuses it ("Skill instructions cannot be partially served"), at most 32,768 bytes when its model catalog has no context window for the model and 32,000 characters with a 100K-200K-token window, so any registered skill over 32,000 bytes (four of the five in hybrid, 26 of 63 in `all` today) is a short entry `SKILL.md` with the same name and description that sends the agent to its full `SKILL.md` in a plain copy of every skill (the router's `gstack/skills/`, or `.gstack-skills/` under `--skills all`, a folder OpenClaw's skill scan skips), which OpenClaw reads in pages. A context window under 100K tokens lowers that budget to 16,000 characters, which this limit does not cover | `./setup --host openclaw`, or `./setup` inside OpenClaw | `openclaw skills list --json` shows `gstack` and the five task skills (source `openclaw-managed`, eligible) | `gstack-harness unregister openclaw` | | Hermes | every skill, as `gstack-` in one `gstack` category, in `${HERMES_HOME:-~/.hermes}/skills/gstack`. Hermes's skills index lists only skills registered as folders, so a router would leave the rest out of the listing (`skill_view` still loads an unlisted skill by name, but nothing tells the model it exists) | `./setup --host hermes`, or `./setup` inside Hermes | `hermes skills list` shows the `gstack-*` skills in category `gstack` | `gstack-harness unregister hermes` | | dots (feder-cr) | the `gstack` router in `/home/dot/skills/gstack`, which the Dot lists every turn | `./setup` on the Dot's computer, or `gstack-harness register dots` | the Dot's Skills page, or `GET /skills` | `gstack-harness unregister dots` | -| QM | **not available yet.** QM imports a skill pack by cloning the whole repository and caps it at 32 MiB and 5,000 files; gstack's repository is about 73 MiB, so `gstack-harness register qm --yes` stops before sending anything ([why](troubleshooting.md#harness-qm-pack-too-large)) | meanwhile, paste `gstack-harness register generic --print` into the agent's instructions | ask it for a gstack skill | delete the pasted text | +| QM | three skills (`gstack-investigate`, `gstack-review`, `gstack-plan`) in QM's own skill format, from `qm/sandbox/skills/` ([qm/README.md](../qm/README.md)). `gstack-harness register qm --yes` still stops before sending anything: QM imports a skill pack by cloning the whole repository and caps it at 32 MiB and 5,000 files, and gstack's repository is about 73 MiB ([why](troubleshooting.md#harness-qm-pack-too-large)) | copy `qm/sandbox/skills/gstack-*` into `/sandbox/skills/` and run `qm up` | ask it for a gstack skill | remove the folders and run `qm up` again | | Muse Code, Grok Build | nothing: both read `~/.claude/skills`, so the Claude install (`./setup`) is what they see, with Claude tool names | `./setup` | `gstack-harness status` runs `muse skills list` / `grok inspect` and reports *discovered (Claude-rendered, unverified)* | `gstack-uninstall` | | Grok Bot, any chat-only skill library | nothing on disk | `gstack-harness register generic --print`, then paste the router and block into the app | ask it for a gstack skill | delete the pasted text | | any other harness | the router in an Agent Skills folder you name, and/or a 600-byte routing block between `` markers in a context file you name | `gstack-harness register generic --skills-dir ` and/or `--context `; `gstack-harness manifest generic` prints every skill as JSON | the harness's own skill list | `gstack-harness unregister generic` | diff --git a/hosts/qm.ts b/hosts/qm.ts new file mode 100644 index 000000000..fcb6a7924 --- /dev/null +++ b/hosts/qm.ts @@ -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///`. 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// 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: 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; +} + +/** 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', '', '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//` 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//scripts/`. + */ +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 -- ', + '```', + '', + '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 -- ', 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', ''), + '```', + '', + 'It prints the base, the merge base, the commits since the base and the diff stat, then `NEXT: git diff `; run that and read the whole diff and every untracked file before you comment (on `UNTRACKED_MORE:`, list the rest with the command it names). 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 ', + '```', + '', + '## 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', ''), 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: {}, + }, +); diff --git a/hosts/qm/scripts/diff-base.sh b/hosts/qm/scripts/diff-base.sh new file mode 100644 index 000000000..3972c62aa --- /dev/null +++ b/hosts/qm/scripts/diff-base.sh @@ -0,0 +1,52 @@ +#!/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 ) 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 +untracked_count=$(printf '%s\n' "$untracked" | grep -c . || true) +if [ "$untracked_count" -gt 30 ]; then + echo "UNTRACKED_MORE: $((untracked_count - 30)) more untracked files; list them all with git ls-files --others --exclude-standard" +fi +if git diff --quiet "$merge_base" -- && [ -z "$untracked" ]; then + echo "NOTHING_TO_REVIEW" + exit 0 +fi +echo "NEXT: git diff $merge_base" diff --git a/qm/README.md b/qm/README.md new file mode 100644 index 000000000..c7c3871d2 --- /dev/null +++ b/qm/README.md @@ -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 /sandbox/skills +cp -R qm/sandbox/skills/gstack-* /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. diff --git a/qm/edition.json b/qm/edition.json new file mode 100644 index 000000000..25dcc5626 --- /dev/null +++ b/qm/edition.json @@ -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 -- ", + "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 ", + "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" + } + ] +} diff --git a/qm/sandbox/skills/gstack-investigate/SKILL.md b/qm/sandbox/skills/gstack-investigate/SKILL.md new file mode 100644 index 000000000..a83316f7b --- /dev/null +++ b/qm/sandbox/skills/gstack-investigate/SKILL.md @@ -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)" +--- + + +# 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 -- +``` + +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 diff --git a/qm/sandbox/skills/gstack-plan/SKILL.md b/qm/sandbox/skills/gstack-plan/SKILL.md new file mode 100644 index 000000000..b1b80dbee --- /dev/null +++ b/qm/sandbox/skills/gstack-plan/SKILL.md @@ -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)" +--- + + +# 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. + + +### 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. diff --git a/qm/sandbox/skills/gstack-review/SKILL.md b/qm/sandbox/skills/gstack-review/SKILL.md new file mode 100644 index 000000000..d67761744 --- /dev/null +++ b/qm/sandbox/skills/gstack-review/SKILL.md @@ -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)" +--- + + +# 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 +``` + +It prints the base, the merge base, the commits since the base and the diff stat, then `NEXT: git diff `; run that and read the whole diff and every untracked file before you comment (on `UNTRACKED_MORE:`, list the rest with the command it names). 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 +``` + +## 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 `