mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-10 18:50:18 +01:00
QM edition no longer ships the review skill (GST-2306) (#4279)
* Drop the review skill from the QM edition A benchmark of the QM edition found gstack-review flagged too many problems without making the reviewed work better, so the edition now ships only gstack-plan. Regenerated qm/ and removed the review-only first-step script. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Regenerate the QM edition without the review skill and update its docs and tests Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Give the deleted QM review script an empty test lane so the pre-merge check accepts its removal Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * Point the deleted QM review script at the QM edition test instead of no test lane 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>
This commit is contained in:
Garry Tanandcapy-ai[bot] authored and GitHub committed
1 parent
6c23638d6d
commit
ee3bae4028
11 files changed
+17
-435
No files matched your search
@@ -14,7 +14,8 @@
|
||||
"LICENSE": [],
|
||||
"licenses/**": [],
|
||||
"contrib/**": [],
|
||||
"supabase/**": []
|
||||
"supabase/**": [],
|
||||
"qm/sandbox/skills/gstack-review/scripts/diff-base.sh": ["test/qm-edition.test.ts"]
|
||||
},
|
||||
"selection": {
|
||||
"free-tests.yml/free-suite": "bun run scripts/test-free-shards.ts --list"
|
||||
|
||||
@@ -0,0 +1 @@
|
||||
- **The QM edition no longer includes `gstack-review`.** QM's default gstack skills are now just `gstack-plan`. A benchmark of the edition inside QM found that `gstack-review` flagged too many problems without making the reviewed work any better, so it was cut. To update a QM deployment, delete its `sandbox/skills/gstack-review/` folder and run `qm up`. gstack's own `/review` skill is unchanged.
|
||||
+1
-1
@@ -64,7 +64,7 @@ characters, the part of a description Hermes's skills index shows.
|
||||
| OpenClaw | default `--skills hybrid`: `gstack-review`, `gstack-qa` and `gstack-ship` 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 stays in 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 (three of the four in hybrid, 25 of 62 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 four task skills (source `openclaw-managed`, eligible) | `gstack-harness unregister openclaw` |
|
||||
| Hermes | every skill, as `gstack-<skill>` 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 | two skills (`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 `<deploy dir>/sandbox/skills/` and run `qm up` | ask it for a gstack skill | remove the folders and run `qm up` again |
|
||||
| QM | one skill (`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 `<deploy dir>/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 `<!-- gstack:begin harness-routing -->` markers in a context file you name | `gstack-harness register generic --skills-dir <dir>` and/or `--context <file>`; `gstack-harness manifest generic` prints every skill as JSON | the harness's own skill list | `gstack-harness unregister generic` |
|
||||
|
||||
+2
-73
@@ -1,5 +1,5 @@
|
||||
/**
|
||||
* QM edition: two gstack skills in QM's own skill format.
|
||||
* QM edition: one gstack skill (gstack-plan) 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
|
||||
@@ -10,7 +10,7 @@
|
||||
* 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 two skills by
|
||||
* This is not a HostConfig in hosts/index.ts: it renders its 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
|
||||
@@ -89,78 +89,7 @@ export const QM_EDITION = {
|
||||
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-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 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 <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 the timing cause and fix that, with a test that fails on it first']],
|
||||
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',
|
||||
|
||||
@@ -1,52 +0,0 @@
|
||||
#!/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
|
||||
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"
|
||||
+6
-4
@@ -1,12 +1,14 @@
|
||||
# gstack for QM
|
||||
|
||||
Two gstack skills in QM's own skill format: `gstack-review` (review a branch before merge) and `gstack-plan` (review a feature plan before building it).
|
||||
One gstack skill in QM's own skill format: `gstack-plan` (review a feature plan before building it).
|
||||
|
||||
The edition no longer ships `gstack-review`. A benchmark of the edition in QM found that it flagged too many problems without making the reviewed work any better, so it was cut. gstack's own `/review` skill is unchanged.
|
||||
|
||||
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:
|
||||
Copy the skill folder into your QM deployment directory and sync it:
|
||||
|
||||
```sh
|
||||
mkdir -p <deploy dir>/sandbox/skills
|
||||
@@ -14,7 +16,7 @@ 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.
|
||||
The skill installs into the org scope. Removing the folder and running `qm up` again archives it.
|
||||
|
||||
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.
|
||||
|
||||
@@ -24,7 +26,7 @@ A git pack that points at this whole repository won't work: QM's pack fetcher st
|
||||
|
||||
## 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:
|
||||
The skill is 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), and the rest comes from the gstack skill templates. Regenerate with:
|
||||
|
||||
```sh
|
||||
bun run scripts/gen-qm-edition.ts
|
||||
|
||||
@@ -2,16 +2,6 @@
|
||||
"edition": "gstack-qm",
|
||||
"layer_root": "qm/sandbox/skills",
|
||||
"skills": [
|
||||
{
|
||||
"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",
|
||||
|
||||
@@ -1,157 +0,0 @@
|
||||
---
|
||||
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 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 <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 the timing cause and fix that, with a test that fails on it first
|
||||
|
||||
#### 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.
|
||||
@@ -1,52 +0,0 @@
|
||||
#!/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
|
||||
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"
|
||||
@@ -117,7 +117,7 @@ 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(['qm', 'sandbox', 'skills', 'gstack-plan'].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);
|
||||
|
||||
+4
-84
@@ -1,7 +1,5 @@
|
||||
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';
|
||||
@@ -15,8 +13,6 @@ describe('QM edition (hosts/qm.ts)', () => {
|
||||
test('committed qm/ matches the generator (run: bun run scripts/gen-qm-edition.ts)', () => {
|
||||
expect(staleQmFiles()).toEqual([]);
|
||||
expect(JSON.parse(fs.readFileSync(path.join(ROOT, 'qm/edition.json'), 'utf-8')).edition).toBe('gstack-qm');
|
||||
expect(fs.readFileSync(path.join(ROOT, 'qm/sandbox/skills/gstack-review/scripts/diff-base.sh'), 'utf-8'))
|
||||
.toBe(fs.readFileSync(path.join(ROOT, 'hosts/qm/scripts/diff-base.sh'), 'utf-8'));
|
||||
});
|
||||
|
||||
test('every shipped file fits QM\'s inline read cap, and descriptions fit its skills index', () => {
|
||||
@@ -56,21 +52,21 @@ describe('QM edition (hosts/qm.ts)', () => {
|
||||
});
|
||||
|
||||
test('the command check refuses what QM\'s sandbox lacks (negative controls)', () => {
|
||||
const id = 'gstack-review';
|
||||
const id = 'gstack-plan';
|
||||
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/],
|
||||
['```sh\nsh skills/gstack-plan/scripts/missing.sh\n```\n', /not a script shipped/],
|
||||
['```sh\nsh skills/gstack-other/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([]);
|
||||
expect(qmCommandViolations(id, md('```sh\nsh skills/gstack-plan/scripts/diff-base.sh main\n```\n'))).toEqual([]);
|
||||
});
|
||||
|
||||
test('commandWords reads pipelines, substitutions, functions and redirections', () => {
|
||||
@@ -91,79 +87,3 @@ describe('QM edition (hosts/qm.ts)', () => {
|
||||
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('more than 30 untracked files: the first 30 are listed and the rest are named, never dropped silently', () => {
|
||||
const dir = repo();
|
||||
git(dir, 'checkout', '-qb', 'feature');
|
||||
for (let i = 0; i < 31; i++) fs.writeFileSync(path.join(dir, `u${String(i).padStart(2, '0')}.txt`), 'u\n');
|
||||
const out = run(dir);
|
||||
expect(out).toContain('u29.txt');
|
||||
expect(out).not.toContain('u30.txt');
|
||||
expect(out).toContain('UNTRACKED_MORE: 1 more untracked files; list them all with git ls-files --others --exclude-standard\n');
|
||||
expect(run(repo())).not.toContain('UNTRACKED_MORE');
|
||||
});
|
||||
|
||||
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');
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user