Skip to content

Fix YAML escape miscompile and harden init/doctor flow#4

Merged
christopherarter merged 4 commits into
mainfrom
bug/parse-issue
Apr 21, 2026
Merged

Fix YAML escape miscompile and harden init/doctor flow#4
christopherarter merged 4 commits into
mainfrom
bug/parse-issue

Conversation

@christopherarter

Copy link
Copy Markdown
Contributor

Summary

Addresses review feedback from a real bully-init session. Priority-ordered:

P0 — silent miscompile

  • _parse_scalar now processes YAML double-quoted escapes (\\, \", \n, \t, \r, \/, \0) and single-quoted '''. Previously it stripped outer quotes only, so "\\." stayed as two literal backslashes. Every shipped examples/rules/*.yml regex (react-ts, rust-cli, nextjs, django, go, fastapi, rails) exited 0 on grep's parentheses not balanced error — silent pass.
  • filter_rules handles ** recursively via a new _scope_matches helper. PurePath.match only gained recursive ** in Python 3.13, so apps/**/*.ts was broken on 3.10–3.12 despite docs promising otherwise.
  • Doctor's Python check actually compares sys.version_info against 3.10 instead of printing [OK] unconditionally.

P1 — UX

  • Doctor accepts skills + the evaluator agent at either the legacy path or ~/.claude/plugins/cache/*/bully/*/{skills,agents}/... — plugin-installed users no longer get five bogus [FAIL] lines.
  • bully shell wrapper at repo root so plugin users can alias/symlink without pip install.
  • bully-init Step 6 now runs trust → doctor → telemetry dir + .gitignore (gated on Step 3 answer) → per-rule smoke tests before handing off. Would have caught the parser bug above at author time.

P2

  • bully validate --execute-dry-run runs each script rule against /dev/null to surface shell/regex syntax errors at config time.
  • docs/rule-authoring.md documents YAML escapes, ** compatibility, --explain, and --execute-dry-run.

Test plan

  • Full suite: 297 passing (+5 new tests — escape handling, recursive globs, version gating, plugin paths, dry-run execution)
  • Manual: ./bully doctor under system Python 3.9 → [FAIL] Python 3.9 < 3.10 -- upgrade required
  • Manual: ./bully doctor under venv Python 3.12 → all [OK] including plugin-path skill detection ((plugin install) annotation)
  • Manual: ./bully validate --execute-dry-run on repo's own .bully.yml → 6 script rules clean, ast/semantic rules correctly skipped
  • Verified: shipped examples/rules/react-ts.yml console.log pattern now parses to console\.log\( instead of console\\.log\\( and grep runs cleanly
  • Reviewer smoke-test on a fresh plugin install (no pip install) — confirm bully doctor passes without legacy-path files present

Chris Arter and others added 4 commits April 21, 2026 10:53
Addresses review feedback from a real init session.

Parser (silent miscompile, highest impact):
- `_parse_scalar` processes YAML double-quoted escapes (\\, \", \n,
  \t, \r, \/, \0) and single-quoted `''`. Previously stripped outer
  quotes only, so `"\\."` stayed as two backslashes -- every shipped
  `examples/rules/*.yml` pattern silently exited 0 on grep's
  "parentheses not balanced" error.
- `filter_rules` now handles `**` recursively on all supported Python
  versions via `_scope_matches` (was broken on 3.10-3.12; `PurePath.match`
  only gained recursive `**` in 3.13). Matches the `_path_matches_skip`
  fallback pattern for consistency.

Doctor:
- Actually compares `sys.version_info` against 3.10 instead of
  printing `[OK] Python X.Y >= 3.10` unconditionally.
- Accepts skills and the evaluator agent at either the legacy path
  (`~/.claude/skills/...`) or the plugin cache (`~/.claude/plugins/
  cache/*/bully/*/{skills,agents}/...`).

New features:
- `bully validate --execute-dry-run` runs each script rule against
  /dev/null to surface shell/regex syntax errors at config time.
- `bully` shell wrapper at repo root so plugin-installed users can
  alias or symlink without needing `pip install`.

Skill + docs:
- `bully-init` Step 6 now runs trust, doctor, telemetry-dir setup,
  and per-rule smoke tests before handing off.
- `docs/rule-authoring.md` documents YAML escapes, `**` compatibility,
  and the `--explain` / `--execute-dry-run` flags.

Tests: 297 passing (+5 new suites covering escapes, recursive globs,
version gating, plugin paths, and dry-run execution).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's `ruff check .` flagged 3 SIM rules that the hook's dogfood
`ruff-clean` missed because ruff isn't on PATH in the hook's env
(graceful fallback exits 0 when the binary is missing).

- SIM110: replace for-loop with `all()` in `_segment_matches`
- SIM102: combine nested `if` statements with `and` in
  `_match_glob_segments`
- SIM108: use ternary for SKILL.md vs `<name>.md` path selection in
  `_plugin_cache_candidates`

Behaviour unchanged; 297 tests still pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous rule exited 0 when `ruff` wasn't on PATH -- which
includes Claude Code's hook env (venv never activated). That meant
local edits silently passed ruff checks while CI caught them.

Now the rule prefers `ruff` on PATH but falls back to
`.venv/bin/ruff` before the graceful "not found" exit. Smoke-tested
both paths: with and without PATH containing ruff, a SIM-violating
file correctly exits 1 so the hook blocks the edit.

Keeps the graceful no-venv behaviour for contributors who don't have
a local install.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@christopherarter
christopherarter merged commit c4b1430 into main Apr 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant