Commit Graph
6 Commits
Author SHA1 Message Date
Garry TanandClaude Fable 5.1 3994ddfdd7 fix(design-detect): audit directories scan as dumps; scans print probe lines on stderr; refused base always exits 1; PATH loses project entries
Adversarial review (Claude subagent):

- A DIRECTORY target under designs/ (the audit dir, which the prose hands the
  agent as REPORT_DIR) classified as an artifact, so the engine walked its
  dom/ subtree WITH inline ignores honored. Any directory under designs/ is
  now scanned as dumps.
- A scan whose probe no longer finds an engine wrote its sentinel lines to
  stdout and exited 0, so `scan > "$_DJ"` captured "IMPECCABLE_NOT_AVAILABLE"
  as the scan result and the rendered bash read a clean scan. Probe lines go
  to stderr on every path; stdout is the JSON document or nothing.
- A refused --changed base exited 0/2 when explicit targets were also given;
  it folds into the exit code (1 over 2 over 0). A trailing --changed no
  longer defaults to main.
- A hand-edited `design_detector: Off` re-enabled the detector; the value is
  compared case-insensitively.
- The engine inherited PATH entries inside the project (a direnv .envrc
  adding node_modules/.bin); those are filtered like every other project path.
- DOM_DUMP_MISSING names the case where the dump script wrote nothing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-08 18:55:37 +00:00
Garry TanandClaude Fable 5.1 8d709c8f29 fix(design-detect): project means below HOME; only page dumps drop inline ignores; a whole-scan budget; prototype-safe rule counts
Third review cycle + Red Team, all reproduced before the fix:

- With no repository, the wrapper adopted cwd as the repo root, so a review
  launched from HOME (URL mode can run from anywhere) rejected every
  HOME-rooted install as "repository-local", reported the user's own skill
  install with the wrong hint, and, for targets, accepted all of HOME
  (~/.ssh/id_rsa scanned). A project directory is now one strictly below
  HOME: `git init ~` never turns the user's installs into repository files,
  and from HOME only the designs allow-list qualifies as a target.
- --no-inline-ignores keyed on "not inside the repo", which misclassified
  dumps when GSTACK_HOME sits under the repo and stripped the design-html
  gate's own `<!-- impeccable-disable -->` from finalized.html. Targets are
  classified as project / dom-dump (designs/<audit>/dom/**, the page's bytes)
  / artifact (other designs/ files, gstack-authored); only dumps drop inline
  ignores.
- A repository's .impeccable/config.json can hide rules from the review;
  detector.ignoreValues was never surfaced. The probe prints
  IMPECCABLE_IGNORED_VALUES beside the rules, and the prose stops calling
  repo-config ignores "a decision the user made".
- An engine id named `constructor` corrupted byRule through
  Object.prototype and `__proto__` counts vanished; byRule is a null-
  prototype object and an id that fails the shape check is `unmapped` as a
  key too.
- Batches ran with no total budget (10,000 un-ignored files: hours). The
  scan stops at 5x the per-batch timeout with DETECT_TIMEOUT and exit 1.
- The scan JSON carries an `untrusted` list of the engine- and page-derived
  fields, so the agent reading past the fenced DETECT_TOP block is told what
  is evidence.
- The PATH walk keeps launcher-present for a .cmd wrapper or a differently
  named real file (the name gate applies to READY only).

Tests: probe and scan from a fake HOME (cache READY, HOME file refused, dump
scanned without inline ignores), artifact vs dump batches, prototype-member
ids, the whole-scan budget over 11 batches, ignoreValues surfaced, the
`untrusted` field.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-08 18:33:22 +00:00
Garry TanandClaude Fable 5.1 b2e67d0097 fix(design-detect): an engine is a file named impeccable outside the project; DOM dumps scan without inline ignores
Second review cycle, security + checklist:

- IMPECCABLE_BIN=/bin/sh (or node) was READY, and `detect` with cwd=repoRoot
  made the interpreter run the repository's own `detect` file. Every engine
  candidate (env override, PATH entry, cache, sibling) is now judged by the
  realpath of the FILE and must be named impeccable[.exe]; PATH and cache
  candidates that resolve into the repository are skipped like the others.
  "Inside the project" means the repository, or cwd when cwd is a project
  directory: HOME and its ancestors are exempt, so a URL-mode review launched
  from HOME still finds the HOME-rooted installs.
- A base for --changed that starts with `-` was spliced into git argv
  (`--output=<file>` made git write a file and report no changes); an option-
  like or missing base is DETECT_REFUSED (not a ref name), exit 1, and the
  parser no longer defaults a missing value to main.
- DOM dumps are the audited page's bytes, so an in-file `impeccable-disable`
  comment there is page-controlled: batches under the designs root run with
  --no-inline-ignores, repository batches keep the project's own ignores.
- neutralizeSentinels covers the shapes it missed (bare sentinels such as
  DETECT_TOP total= and IMPECCABLE_DISABLED, the DETECT_EXIT_CODE= echo, the
  `[rule-id] impact=` group header) in one precompiled alternation instead of
  37 replaceAll passes per field; only kept findings are normalized, and the
  summary's total stays the engine's count.
- The minimal engine environment compares keys case-insensitively on Windows
  (process.env enumerates Path, SystemRoot there) and passes PATHEXT, COMSPEC,
  HOMEDRIVE, HOMEPATH, PROGRAMDATA.
- Bare 64s move into DETECT_LIMITS; the unused SentinelName type is gone; the
  header states the directory-target contract (the engine's own walk).

Tests: an interpreter as IMPECCABLE_BIN never runs the repo's detect file; a
PATH symlink into the repository is never READY; option-like and empty bases
are refused with no file written; the designs-root batch carries
--no-inline-ignores and the repo batch does not; the identity label is
deterministic per binary; the bare-sentinel and header shapes are neutralized;
the installed fake engine works without IMPECCABLE_FAKE_OUTPUT (the helper
copies the sample beside it); two tests clean up in finally.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-08 18:05:21 +00:00
Garry TanandClaude Fable 5.1 60758c5ddf fix(design-detect): never execute a repository-controlled engine; allow-list --changed targets; sanitize engine text
Pre-landing review findings (security + checklist), all reproduced before the fix:

- A checked-out branch could commit `.claude/skills/impeccable/scripts/bin/<os>-<arch>/impeccable`
  and the probe would report READY and `scan` would run it, with the agent's full
  environment. Launchers and sibling engines under the repo or cwd now count as
  "skill present" only (IMPECCABLE_NOT_CACHED: repository-local install, and the
  hint never names a repository-local launcher to run); only HOME-rooted installs,
  IMPECCABLE_BIN, the cache, and PATH entries outside the repo qualify, all by
  realpath. The engine now sees a minimal environment (PATH, HOME, TMPDIR, locale,
  IMPECCABLE_*), never the agent's tokens.
- `scan --changed <base>` pushed git-derived paths without the allow-list, so a
  committed symlink with a frontend extension handed a file outside the repo to
  the engine. Derived targets now go through the same allow-list as explicit ones
  and symlinks named by git are refused outright.
- A repo-controlled `scripts/VERSION` with embedded newlines forged probe lines;
  the version is trusted only when it is semver, and every printed version is
  sanitized. Engine text containing the untrusted-content fence or a
  `SENTINEL:` prefix is neutralized with a zero-width space
  (neutralizeSentinels in the contract), so page text cannot close the envelope
  or forge a probe line.
- A failing `git diff <base>...HEAD` (unknown or unfetched base) was swallowed
  and read as "no frontend changes"; it is now DETECT_REFUSED with exit 1.
- The scan allow-list root follows `${GSTACK_HOME:-$HOME/.gstack}` like the
  templates and gstack-slug (config.yaml keeps gstack-config's STATE_ROOT
  precedence); a quoted or commented design_detector value reads correctly.

Smaller: raw engine chunks are kept only in --format raw; diagnostics are
capped (200 kept, 20 echoed); the engine identity hash reads size + 4 MB, not
the whole binary; PROBE_STEP and ENGINE_STDERR are contract sentinels; the
--verbose gate covers every probe step; analytics use one sentinel vocabulary;
bare limits live in DETECT_LIMITS. The fake engine's knobs are IMPECCABLE_FAKE_*
(so they pass the minimal env) and a shared test helper installs it. New tests
cover each item above plus clean runs, `{}` parse errors, missing paths, and the
50 MB stdout cap.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-08 17:31:30 +00:00
Garry TanandClaude Fable 5.1 9a03aa5e4a test: touchfiles, tiers, shim E2E, real-engine fixture
Touchfiles: the catalog, contract, detector bin, checklist resolver, review-
army resolver, and DESIGN.md lib join the dep lists of review-design-lite,
design-review-fix, the design-consultation cases, and plan-design-review-no-
ui-scope, so editing any of them re-selects the tests that read their output.
Three new E2E keys: design-review-detector-shim (gate; source mode on a
feature-branch diff), design-review-detector-shim-dom (gate; DOM mode: the
slop fixture served on loopback, dumped through the browse binary with
lib/dom-dump.js, persisted under a GSTACK_HOME-scoped REPORT_DIR, scanned once;
self-skips when browse/dist/browse is absent), and design-html-slop-gate
(periodic; one fix pass, at most two scans, remaining findings accepted with
reason). Every case reaches the engine through test/fixtures/fake-impeccable.ts
via IMPECCABLE_BIN from outside the temp repo, reads extracted skill sections
(never a whole SKILL.md) with the installed bin path rewritten to this
checkout, and asserts the probe ran, the right scan verb ran, `npx impeccable`
never did, and the output carries FINDING rows tagged [ai-color-palette] and
[low-contrast]. review-design-lite gets the fake engine and an eighth tally
signal for a detector row; its 4-hit threshold is unchanged.

test/gstack-design-detect.test.ts evaluates design-review's REPORT_DIR
expression with GSTACK_HOME set and proves a dump under it is accepted by the
wrapper's allow-list. The sample fixtures were real captures from commit 1
(engine 0.1.3), so there is nothing hand-written left to swap.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-08 16:38:46 +00:00
Garry TanandClaude Fable 5.1 3867dae355 feat(bin): gstack-design-detect wrapper + design_detector config key
bin/gstack-design-detect.ts finds and runs an impeccable engine the user
installed; it never installs, downloads, or executes anything that could
download. `probe` reads only: config (design_detector off → DISABLED),
IMPECCABLE_BIN (absolute, realpath outside the repo and cwd), a PATH walk
(absolute entries outside the repo; a #! shim counts as launcher-present,
never READY), the ~/.impeccable/bin/<newest semver>/ cache, and the engine
installed beside a skill launcher (scripts/bin/<os>-<arch>/impeccable, the
layout a real install produced). It reports IMPECCABLE_SKILL, host-aware
IMPECCABLE_HOOK (+ HOOK_OTHER), the ignore lists from .impeccable/config*.json,
IMPECCABLE_ENGINE_UNTESTED for versions outside the fixture set, and a hint
only when a launcher exists without its engine. `scan` re-probes, refuses
URLs and anything outside the repo root or the design-report allow-list
(realpath, so symlinks cannot escape), derives `--changed <base>` targets
NUL-safely through git and lib/frontend-scope.ts, batches 100 absolute paths
per engine call with stdin ignored, a SIGKILL timeout, a 50 MB stdout cap, and
sanitized length-capped fields, then prints one normalized JSON document
(--format gstack) or the engine's bytes (--format raw); DETECT_TOP (fenced as
untrusted content), DETECT_SUMMARY, and DETECT_EXIT go to stderr; exit code
passes through with 1 over 2 over 0; exit 3 is a gstack bug. `rules` prints
the mapped set. Every run appends a content-free line to the local analytics
file.

lib/design-detect-contract.ts owns every sentinel string, the limits, and the
normalized-finding shape (pure module); test/design-detect-contract.test.ts
asserts every sentinel-shaped token the agent can read exists there.
lib/frontend-scope.ts mirrors gstack-diff-scope's frontend arm, pinned by a
parity test that runs the bash script. bin/gstack-config gains
design_detector (auto | off, default auto, invalid values rejected with the
file unchanged). test/fixtures/fake-impeccable.ts is the env-driven engine
stand-in; test/gstack-design-detect.test.ts covers READY/NOT_CACHED/
NOT_AVAILABLE/DISABLED, env trust (.env never loaded, in-repo IMPECCABLE_BIN
ignored), newest-semver cache, hook and ignore detection, refusals, exit
passthrough, raw byte-identity, normalization, the display cap, timeout,
parse errors, diagnostics, --changed, and analytics. The egress scanner test
records the wrapper as a documented non-sink.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2026-09-08 16:01:18 +00:00