From d752b436ac51ca855f10164c04137b85e4fb372a Mon Sep 17 00:00:00 2001 From: Chris Arter Date: Fri, 17 Apr 2026 12:01:53 -0400 Subject: [PATCH 1/2] Land P1 review epic, dogfood ast rules, run CI on every push P1 epic from the principal-engineer review (TASK-1.1 through 1.4): - ship `bully` console entrypoint (`bully validate / lint / doctor / show-resolved-config`); hook.sh continues to call pipeline.py by absolute path so the hook env doesn't depend on $PATH - skills + docs no longer hardcode $HOME/.bully/pipeline/pipeline.py; primary path is `bully` with documented plugin/manual fallbacks - top-level `skip:` in .bully.yml + ~/.bully-ignore merge with built-in SKIP_PATTERNS so multi-language monorepos don't have to patch source - --explain flag surfaces per-rule verdict (fire/pass/skipped / dispatched); run_pipeline gains include_skipped=True; hook-mode shape unchanged Trust-gate docs in SECURITY.md; ast-engine + trust-gate tests land for the prior P0 work. Dogfood ourselves on the new tooling: - two engine: ast rules in .bully.yml (prefer-perf-counter, no-shell-true-subprocess) scoped to pipeline source; the legitimate shell=True in execute_script_rule carries an inline bully-disable - scripts/dogfood.sh prefers `bully lint` when on PATH and runs `bully validate` as a sanity preamble - CI installs ast-grep-cli so ast rules actually fire instead of silently skipping; runs on every push (not just main) Tests: 226 -> 243. Co-Authored-By: Claude Opus 4.7 (1M context) --- .bully.yml | 22 + .github/workflows/ci.yml | 4 +- .gitignore | 3 + CLAUDE.md | 29 + README.md | 45 +- SECURITY.md | 46 +- docs/design.md | 39 +- docs/rule-authoring.md | 78 ++- docs/telemetry.md | 4 +- examples/rules/react-ts.yml | 14 +- pipeline/__init__.py | 0 pipeline/pipeline.py | 811 ++++++++++++++++++++++- pipeline/tests/conftest.py | 12 + pipeline/tests/test_ast_engine.py | 437 ++++++++++++ pipeline/tests/test_bully_subcommands.py | 162 +++++ pipeline/tests/test_explain.py | 174 +++++ pipeline/tests/test_skip_config.py | 163 +++++ pipeline/tests/test_trust_gate.py | 249 +++++++ pyproject.toml | 15 +- scripts/dogfood.sh | 24 +- skills/bully-author/SKILL.md | 55 +- skills/bully/SKILL.md | 4 +- 22 files changed, 2286 insertions(+), 104 deletions(-) create mode 100644 CLAUDE.md create mode 100644 pipeline/__init__.py create mode 100644 pipeline/tests/conftest.py create mode 100644 pipeline/tests/test_ast_engine.py create mode 100644 pipeline/tests/test_bully_subcommands.py create mode 100644 pipeline/tests/test_explain.py create mode 100644 pipeline/tests/test_skip_config.py create mode 100644 pipeline/tests/test_trust_gate.py diff --git a/.bully.yml b/.bully.yml index 2d9cae0..6c94d00 100644 --- a/.bully.yml +++ b/.bully.yml @@ -72,3 +72,25 @@ rules: engine: semantic scope: "pipeline/tests/test_*.py" severity: warning + + prefer-perf-counter: + description: > + Use `time.perf_counter()` for measuring elapsed time, not `time.time()`. + `time.time()` is a wall-clock that can jump backwards on NTP sync; + `perf_counter()` is monotonic and ideal for latency measurement. + engine: ast + scope: ["pipeline/pipeline.py", "pipeline/analyzer.py"] + severity: error + pattern: "time.time()" + + no-shell-true-subprocess: + description: > + Do not pass `shell=True` to `subprocess.run`. Shell-mode invocation + mixes interpolation with shell parsing and is the most common path + to command injection. Pass a list of args instead, or if the call + is genuinely safe (input is `shlex.quote`'d, etc.), add a per-line + `bully-disable: no-shell-true-subprocess `. + engine: ast + scope: ["pipeline/pipeline.py", "pipeline/analyzer.py"] + severity: error + pattern: "subprocess.run($$$, shell=True, $$$)" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9af2952..23de475 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,7 +2,6 @@ name: CI on: push: - branches: [main] pull_request: jobs: @@ -31,5 +30,8 @@ jobs: - name: pytest run: pytest -q + - name: install ast-grep + run: pip install ast-grep-cli + - name: bully dogfood run: bash scripts/dogfood.sh diff --git a/.gitignore b/.gitignore index d2dbb1f..756b65e 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,9 @@ __pycache__/ # Per-developer telemetry (see docs/telemetry.md) .bully/ +# Backlog.md local state +.backlog/ + # Python build artifacts build/ dist/ diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..aa0426a --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,29 @@ + + + + + +## BACKLOG WORKFLOW INSTRUCTIONS + +This project uses Backlog.md MCP for all task and project management activities. + +**CRITICAL GUIDANCE** + +- If your client supports MCP resources, read `backlog://workflow/overview` to understand when and how to use Backlog for this project. +- If your client only supports tools or the above request fails, call `backlog.get_backlog_instructions()` to load the tool-oriented overview. Use the `instruction` selector when you need `task-creation`, `task-execution`, or `task-finalization`. + +- **First time working here?** Read the overview resource IMMEDIATELY to learn the workflow +- **Already familiar?** You should have the overview cached ("## Backlog.md Overview (MCP)") +- **When to read it**: BEFORE creating tasks, or when you're unsure whether to track work + +These guides cover: +- Decision framework for when to create tasks +- Search-first workflow to avoid duplicates +- Links to detailed guides for task creation, execution, and finalization +- MCP tools reference + +You MUST read the overview resource to understand the complete workflow. The information is NOT summarized here. + + + + diff --git a/README.md b/README.md index 56544a0..0c6591b 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ Bully is a lint pipeline for Claude Code. Every `Edit` / `Write` hits a `PostToo ## The config -A `.bully.yml` is a flat list of rules. Each rule says what to check, where it applies, how bad it is, and which engine runs it `script` (deterministic shell command) or `semantic` (natural-language rule the agent evaluates against the diff): +A `.bully.yml` is a flat list of rules. Each rule says what to check, where it applies, how bad it is, and which engine runs it -- `script` (deterministic shell command), `ast` (structural pattern via [ast-grep](https://ast-grep.github.io/)), or `semantic` (natural-language rule the agent evaluates against the diff): ```yaml schema_version: 1 @@ -28,6 +28,13 @@ rules: severity: error script: "grep -nE 'console\\.log\\(' {file} && exit 1 || exit 0" + no-any-cast: + description: "No `as any` casts -- use a precise type or `unknown` plus narrowing." + engine: ast + scope: ["src/**/*.ts", "src/**/*.tsx"] + severity: error + pattern: "$EXPR as any" + prefer-derived-state: description: > React components should not use `useEffect` to derive state from @@ -39,7 +46,9 @@ rules: severity: warning ``` -The first rule runs a grep on every edited `.ts`/`.tsx`; the second ships the diff to the agent with the description as the evaluation prompt. No plugins, no DSL -- just globs, shell, and prose. +The first rule runs a grep on every edited `.ts`/`.tsx`. The second matches the structural pattern with ast-grep -- ignores comments, strings, and formatting variants. The third ships the diff to the agent with the description as the evaluation prompt. No plugins, no DSL -- just globs, shell, ast patterns, and prose. + +`engine: ast` requires `ast-grep` on `$PATH` (`brew install ast-grep`, `cargo install ast-grep`, or `pip install ast-grep-cli`). If missing, ast rules are skipped at runtime with a one-line stderr hint and `bully doctor` flags it. Need to share rules across your own repos? `extends:` accepts any relative or absolute path to another `.bully.yml`: @@ -59,10 +68,10 @@ Local rules override inherited rules of the same id. Bully flow: Edit/Write → PostToolUse hook → script phase → semantic phase → block or pass

-1. **Script phase** -- deterministic checks (grep, awk, shell-out to a linter). Fast. Fails the tool call on error-severity violations via exit code 2. -2. **Semantic phase** -- if the script phase passes, the pipeline hands a unified diff plus rule descriptions to the evaluator subagent. Structured verdicts come back; the parent session surfaces them. +1. **Script + AST phase** -- deterministic checks. `script` rules shell out (grep, awk, linters); `ast` rules run structural patterns through ast-grep. Both are fast and fail the tool call on error-severity violations via exit code 2. +2. **Semantic phase** -- if the deterministic phase passes, the pipeline hands a unified diff plus rule descriptions to the evaluator subagent. Structured verdicts come back; the parent session surfaces them. -Deterministic rules stay as shell. Judgment rules ("inline single-use variables", "don't derive state with `useEffect`") live as plain English the agent evaluates against the diff. Same trigger, same output format, same fix loop -- across every language in the repo. +Deterministic rules stay as shell or ast patterns. Judgment rules ("inline single-use variables", "don't derive state with `useEffect`") live as plain English the agent evaluates against the diff. Same trigger, same output format, same fix loop -- across every language in the repo. ## Prerequisites @@ -87,10 +96,16 @@ To change the evaluator model, set the plugin's agent override or edit `model:` ### Verify the install ```bash -python3 "$(ls -d ~/.claude/plugins/cache/*/bully/*/ | tail -1)pipeline/pipeline.py" --doctor +bully doctor ``` -`--doctor` checks Python version, config presence and parse-ability, hook wiring, evaluator-agent registration, and each skill. One line per check, `[OK]` or `[FAIL]`. +`doctor` checks Python version, config presence and parse-ability, hook wiring, evaluator-agent registration, and each skill. One line per check, `[OK]` or `[FAIL]`. + +If `bully` isn't on `$PATH` (e.g., you skipped `pip install -e .`), call the pipeline directly as a fallback: + +```bash +python3 "$(ls -d ~/.claude/plugins/cache/*/bully/*/ | tail -1)pipeline/pipeline.py" --doctor +``` ### Manual install (fallback) @@ -140,7 +155,7 @@ The init skill detects your stack, scans for existing linter configs, asks a cou A fresh rule across an existing codebase lights up every pre-existing problem. Baseline the current state so only _new_ violations block edits: ```bash -python3 ~/.bully/pipeline/pipeline.py --baseline-init --glob "src/**/*.ts" +bully baseline-init --glob "src/**/*.ts" ``` That writes `.bully/baseline.json`. Future runs ignore anything recorded there. See [docs/design.md](docs/design.md) for the contract. @@ -178,15 +193,15 @@ The `bully-author` skill walks through engine choice, drafts the rule, tests it For authoring and debugging rules without triggering an Edit: ```bash -PIPE=~/.bully/pipeline/pipeline.py - -python3 "$PIPE" --validate # parse + enum checks -python3 "$PIPE" --file src/foo.php # full pipeline on a file -python3 "$PIPE" --file src/foo.php --rule no-compact # isolate one rule -python3 "$PIPE" --file src/foo.php --print-prompt # see the semantic prompt -python3 "$PIPE" --show-resolved-config # rules after extends: +bully validate # parse + enum checks +bully lint src/foo.php # full pipeline on a file +bully lint src/foo.php --rule no-compact # isolate one rule +bully lint src/foo.php --print-prompt # see the semantic prompt +bully show-resolved-config # rules after extends: ``` +`bully` is the console script installed by `pip install -e .`. If you can't install the package, call the pipeline directly: `python3 ~/.bully/pipeline/pipeline.py --validate` (or with `--file`, `--show-resolved-config`, etc.). + ## Uninstall Plugin install: diff --git a/SECURITY.md b/SECURITY.md index 5b17de3..b32be83 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -1,8 +1,42 @@ # Security Policy -## Reporting a Vulnerability +## Trust boundary: `.bully.yml` -bully runs deterministic shell commands and dispatches a Claude subagent. If you find a way to escape the sandboxed scope or inject commands through a crafted `.bully.yml` or through file contents that bypass the script-rule scoping, please report it privately. +**A `.bully.yml` can execute arbitrary shell commands on your machine.** Every `engine: script` rule runs its `script:` field through the shell against files you edit. That's the whole point of the tool -- but it also means a malicious or careless `.bully.yml` in a repo you clone is equivalent to running a setup script you didn't review. + +bully gates this with a per-machine trust allowlist: + +1. The first time you edit a file in a repo, the hook sees an untrusted `.bully.yml` and **refuses to execute any rules**. It prints a one-line stderr hint and does **not** block the edit itself -- your tool call succeeds, nothing runs. +2. To enable the config on this machine, review it, then run: + ``` + bully trust # from the repo root, or: --config + ``` + This records the SHA256 of the config (and every file it `extends:`) in `~/.bully-trust.json`. +3. Every subsequent run re-verifies the checksum. If the config changes, you see a "changed since last trust" message and rules stop running until you re-review: + ``` + bully trust --refresh + ``` + +The trust state is **machine-local**. It is never committed to repos and never shared between machines. A teammate cloning a repo you trust must trust it themselves. + +### Bypass for CI and automation + +Set `BULLY_TRUST_ALL=1` to disable the gate unconditionally. Use this only in environments where the config is already trusted through other means: + +- **CI pipelines** -- the config is reviewed as part of the repo's code review; the CI runner treats the repo as trusted. +- **Dogfood / test scripts** -- the `.bully.yml` is the repo's own, and changes arrive through normal PR review. + +Do **not** set `BULLY_TRUST_ALL=1` in an interactive developer shell as a default. That defeats the gate. + +### What trust does NOT protect + +- If you trust a config that was already malicious, you authorized the scripts. Trust gates subsequent changes, not initial review. +- Running `script:` rules that shell out to third-party linters (eslint, phpstan, etc.) inherits whatever risks those tools carry. Trust the tools, not just bully. +- The semantic evaluator subagent sees diff content. If your diffs contain secrets, those secrets are sent to the subagent. Linting is not exfiltration-safe by design. + +## Reporting a vulnerability + +bully runs deterministic shell commands and dispatches a Claude subagent. If you find a way to escape the sandboxed scope or inject commands through a crafted `.bully.yml`, through file contents that bypass the script-rule scoping, or through a bug in the trust gate, please report it privately. **Preferred:** [GitHub private vulnerability reporting](https://github.com/dynamik-dev/bully/security/advisories/new) @@ -13,8 +47,9 @@ Expected response: acknowledgement within 72 hours. ## Scope In scope: -- Command injection through config parsing or diff handling. -- Path traversal in rule `scope` globs. +- Command injection through config parsing, filename handling, or diff content. +- Path traversal in rule `scope` globs or `extends:` targets. +- Trust gate bypass: getting rules to run against an untrusted config without `BULLY_TRUST_ALL` and without an entry in `~/.bully-trust.json`. - Telemetry file tampering causing the analyzer to crash or misclassify. - Hook exit-code bypass. @@ -22,7 +57,8 @@ Out of scope: - Rules themselves being poorly written (that's a config bug, not a security issue). - The `bully` skill making a judgment error on a semantic rule. - Third-party linters invoked by a rule's `script:` field. +- A user trusting a malicious config after reviewing it. Trust is a gate, not a sandbox. -## Supported Versions +## Supported versions Only `main` is supported. Tagged releases may receive security fixes at maintainer discretion. diff --git a/docs/design.md b/docs/design.md index 7de5559..d2a2192 100644 --- a/docs/design.md +++ b/docs/design.md @@ -1,6 +1,6 @@ # Design -`bully` is a Claude Code skill system that enforces project coding standards through a two-phase evaluation pipeline: deterministic script checks for pattern-matchable rules, followed by LLM semantic evaluation for judgment-based rules. It fires on every `Edit` and `Write` tool call via the `PostToolUse` hook. +`bully` is a Claude Code skill system that enforces project coding standards through a two-phase evaluation pipeline: deterministic checks (`script` rules shell out; `ast` rules run structural patterns through ast-grep) for pattern-matchable rules, followed by LLM semantic evaluation for judgment-based rules. It fires on every `Edit` and `Write` tool call via the `PostToolUse` hook. ## Core principles @@ -21,10 +21,12 @@ Rules live in `.bully.yml` at the project root. |-------|----------|-------------| | `id` | yes | Unique identifier. Used in violation payloads and `passed_checks` context. | | `description` | yes | What the rule enforces. Natural language. For semantic rules, this IS the evaluation prompt. | -| `engine` | yes | `script` or `semantic`. | +| `engine` | yes | `script`, `ast`, or `semantic`. | | `scope` | yes | File glob or list of globs this rule applies to. See [Scope](#scope). | | `severity` | yes | `error` (blocks) or `warning` (reported, non-blocking). | | `script` | script engine only | Shell command to run. `{file}` is replaced with the file path. Diff provided on stdin. Exit non-zero on violation. | +| `pattern` | ast engine only | An [ast-grep pattern](https://ast-grep.github.io/guide/pattern-syntax.html). Literal code with `$NAME` for single-node captures, `$$$REST` for variadic. | +| `language` | ast engine, optional | Explicit ast-grep language (`ts`, `python`, `php`, ...). Inferred from the matched file extension when omitted. | ### Scope @@ -75,7 +77,8 @@ rules: - **No prescribed transformations.** `fix_hint` is a one-liner, not a codemod. The agent still performs the edit — prescribed transformations couple the rule to specific syntax and rot fast. - **No rule dependencies or ordering.** Rules are independent. If two rules interact, the LLM sees both via the `passed_checks` context. -- **No `language` field.** Scope globs handle targeting. +- **`language` is optional even on ast rules.** Scope globs do most of the targeting work; `language` is escape-hatch for cases where the file extension can't disambiguate (e.g. `.h` could be C or C++). +- **ast-grep is an optional runtime dep.** Without it, `engine: ast` rules are skipped at runtime with a stderr hint and `bully doctor` reports `[FAIL]`. The pipeline itself stays stdlib-only. ## Validation @@ -92,7 +95,7 @@ Checks performed on load: Run the checks without firing the hook: ```bash -python3 pipeline/pipeline.py --validate +bully validate ``` Exits 0 on a clean config, 1 with a human-readable report otherwise. `hook.sh` runs `--validate` once per session (keyed off a tmpdir marker) so malformed configs surface on the first edit instead of silently dropping a rule forever. @@ -159,15 +162,26 @@ Directives are scoped to the line they appear on. There is no file-level or bloc ## Short-circuit -Before any rule loads, the pipeline skips auto-generated files outright. The list lives in `pipeline/pipeline.py:AUTO_GENERATED_PATTERNS`: +Before any rule loads, the pipeline skips files matching a merged set of skip globs. The merge order is: -``` -package-lock.json, yarn.lock, pnpm-lock.yaml, poetry.lock, Cargo.lock, -composer.lock, Gemfile.lock, go.sum, *.min.*, *.generated.*, -dist/**, build/**, __pycache__/**, node_modules/**, vendor/** +1. **Built-in defaults** (`pipeline/pipeline.py:SKIP_PATTERNS`): + + ``` + package-lock.json, yarn.lock, pnpm-lock.yaml, poetry.lock, Cargo.lock, + *.min.js, *.min.css, *.min.*, dist/**, build/**, __pycache__/**, + *.generated.*, *.pb.go, *.g.dart, *.freezed.dart + ``` + +2. **User-global**: `~/.bully-ignore` (one glob per line, `#` comments allowed). Per-machine, never committed. +3. **Project-local**: a top-level `skip:` key in `.bully.yml` -- inline (`skip: ["_build/**", "vendor/**"]`) or block-list form. Inherited from anything the config `extends:`. + +```yaml +schema_version: 1 +skip: ["_build/**", "vendor/**"] +rules: ... ``` -Matches return `pass` before config parse, scope match, or script dispatch. No config flag, no override — lockfiles and build artifacts never benefit from lint and firing on them burns cycles on every edit. +Matches return `pass` before config parse, scope match, or script dispatch. The built-in list is always active; the user-global and project-local lists merge on top. Lockfiles and build artifacts never benefit from lint and firing on them burns cycles on every edit. ## Evaluation pipeline @@ -192,7 +206,10 @@ PostToolUse fires (Edit or Write) +-- No matching rules? --> exit 0 (pass) | v - Phase 1: run script rules (per-rule 30s timeout) + Phase 1: run script + ast rules (per-rule 30s timeout) + - script rules shell out (`{file}` substituted in) + - ast rules invoke ast-grep with rule.pattern + (skipped with stderr hint if ast-grep is missing) | +-- Any error-severity violations? | --> print text to stderr diff --git a/docs/rule-authoring.md b/docs/rule-authoring.md index 4a47ca3..17f7565 100644 --- a/docs/rule-authoring.md +++ b/docs/rule-authoring.md @@ -14,14 +14,15 @@ The skill applies the discipline described here. This doc is the reference; the ## Pick an engine -| Use a `script` rule when… | Use a `semantic` rule when… | -|---------------------------|-----------------------------| -| The violation is pattern-matchable (`grep`, `awk`, regex). | The violation requires judgment (e.g. "inline single-use variables"). | -| You want deterministic, fast enforcement. | A mechanical rule would have too many false positives. | -| You want the rule to work in CI without an LLM. | The rule depends on context (how a variable is used elsewhere). | -| You're shelling out to an existing tool (phpstan, eslint, pint). | The rule is a prose style guideline. | +| Use a `script` rule when… | Use an `ast` rule when… | Use a `semantic` rule when… | +|---|---|---| +| The violation is matchable with `grep`/`awk`/regex on raw text. | The violation is a **code-structure** pattern (call, cast, declaration) and grep would false-positive on strings/comments. | The violation requires **judgment** (e.g. "inline single-use variables"). | +| You're shelling out to an existing tool (phpstan, eslint, pint). | You want deterministic, fast, whitespace-invariant matching. | A mechanical rule would have too many false positives. | +| You want the rule to work in CI without any extra dependency. | You want the rule to work across formatting variants without regex acrobatics. | The rule depends on context (how a variable is used elsewhere) or is a prose style guideline. | -If you can express it as grep, do. Script rules cost milliseconds. Semantic rules cost an LLM turn. A semantic rule that evaluates cleanly to the same mechanical fix over and over is a candidate for promotion to a script rule. +**Order of preference when a rule could fit more than one engine:** `script` > `ast` > `semantic`. Script rules cost milliseconds. AST rules cost ~10-50 ms per file but eliminate the false-positive tax. Semantic rules cost an LLM turn. Promote noisy semantic rules to ast (or script) when they produce a stable mechanical fix. + +**AST prerequisite:** `ast-grep` must be on `$PATH` (`brew install ast-grep` or `cargo install ast-grep`). If it isn't, `engine: ast` rules are skipped at runtime with a one-line stderr hint — they do not block edits. Run `bully doctor` to see which rules would be skipped. ## Script rule skeleton @@ -106,6 +107,59 @@ The pipeline passes the string through unchanged as `suggestion` on every `Viola Keep hints short, mechanical, and universally applicable to the rule — anything that depends on surrounding code belongs in a semantic rule's `description` instead. There is no placeholder syntax; the hint is static text per rule. +## AST rule skeleton + +```yaml +rule-id: + description: "One-sentence description." + engine: ast + scope: ["*.ts", "*.tsx"] + severity: error + pattern: "$EXPR as any" + language: ts # optional; inferred from the scope's file extension when unambiguous +``` + +- `pattern` is an [ast-grep pattern](https://ast-grep.github.io/guide/pattern-syntax.html): literal code with `$NAME` for single-node captures and `$$$REST` for variadic captures. +- `language` picks the tree-sitter grammar (`ts`, `tsx`, `js`, `python`, `go`, `rust`, `php`, `csharp`, `java`, …). If omitted, bully infers it from the edited file's extension. Set it explicitly when a pack covers multiple extensions that map to different grammars (e.g. `.ts` vs `.tsx`). +- No `script` field. +- Exit is implicit: a non-empty match list = violations; empty = pass. +- Each rule has a 30-second timeout, same as `script`. + +### Minimal ast pattern + +```yaml +no-var-dump: + description: "Do not leave var_dump() calls in committed code." + engine: ast + scope: "*.php" + severity: error + pattern: "var_dump($$$)" +``` + +`var_dump($$$)` matches any call to `var_dump` regardless of argument shape or count, and ignores matches in strings or comments — the same rule as `grep 'var_dump'` but without the false positives. + +### Why prefer ast over grep for structural rules + +```yaml +# Fragile: grep matches inside strings and comments +no-db-facade-script: + engine: script + scope: "*.php" + script: "grep -n 'DB::' {file} && exit 1 || exit 0" + +# Precise: only real static method calls on the DB class match +no-db-facade-ast: + engine: ast + scope: "*.php" + pattern: "DB::$METHOD($$$)" +``` + +The grep version fires on `// the DB:: facade is banned` and on `$msg = "DB::something"`. The ast version matches only actual scope-resolution calls on the identifier `DB`. + +### When ast-grep isn't installed + +If you author an `engine: ast` rule but ast-grep isn't on `$PATH`, the pipeline prints a one-line stderr hint and skips the rule — it does not block the edit. `bully validate` surfaces this as a `[WARN]`; `bully doctor` surfaces it as a `[FAIL]` so installs can be caught in CI. + ## Semantic rule skeleton ```yaml @@ -217,7 +271,7 @@ Cycles and unknown references fail loud at parse time. See [design.md#extends](d Before committing a rule, run the validator: ```bash -python3 pipeline/pipeline.py --validate +bully validate ``` It parses `.bully.yml` (plus anything it extends) with the same hardened reader the hook uses. The validator reports: @@ -262,16 +316,16 @@ Without triggering an Edit, run the pipeline manually: ```bash # Full pipeline against a file -python3 pipeline/pipeline.py --config .bully.yml --file src/foo.php +bully lint src/foo.php # Just one rule (isolate it) -python3 pipeline/pipeline.py --config .bully.yml --file src/foo.php --rule no-compact +bully lint src/foo.php --rule no-compact # See the semantic evaluation prompt without calling an LLM -python3 pipeline/pipeline.py --config .bully.yml --file src/foo.php --print-prompt +bully lint src/foo.php --print-prompt # Supply a diff manually (bypasses stdin/file-state inference) -python3 pipeline/pipeline.py --config .bully.yml --file src/foo.php --diff "$(git diff src/foo.php)" +bully lint src/foo.php --diff "$(git diff src/foo.php)" ``` Exit codes: diff --git a/docs/telemetry.md b/docs/telemetry.md index b06f7ca..47e8981 100644 --- a/docs/telemetry.md +++ b/docs/telemetry.md @@ -88,7 +88,7 @@ The pipeline ships two extra record types that close the semantic-rule telemetry After the `bully` skill finishes evaluating a semantic payload, it calls: ```bash -python3 pipeline/pipeline.py --log-verdict \ +bully --log-verdict \ --rule inline-single-use-vars \ --file src/Evaluators/CachedEvaluator.php \ --verdict violation @@ -127,7 +127,7 @@ Before dispatching the evaluator the pipeline applies cheap "can't possibly matc ### Note on skill version -`semantic_verdict` depends on the `bully` skill being up to date — older versions do not call `--log-verdict`. If verdict records are missing for known-firing semantic rules, update the skill or bypass the evaluator manually (`pipeline.py --log-verdict` is a plain CLI). `semantic_skipped` is pipeline-side and independent of the skill. +`semantic_verdict` depends on the `bully` skill being up to date — older versions do not call `--log-verdict`. If verdict records are missing for known-firing semantic rules, update the skill or bypass the evaluator manually (`bully --log-verdict` is a plain CLI). `semantic_skipped` is pipeline-side and independent of the skill. ## Running the analyzer diff --git a/examples/rules/react-ts.yml b/examples/rules/react-ts.yml index c6552d2..e89de10 100644 --- a/examples/rules/react-ts.yml +++ b/examples/rules/react-ts.yml @@ -21,10 +21,20 @@ rules: Do not use `as any` casts. They silence the type checker and hide real bugs. Use a precise type, `unknown` plus a narrowing check, or a generic if the value is genuinely polymorphic. - engine: script + engine: ast scope: ["*.ts", "*.tsx"] severity: error - script: "grep -nE '\\bas[[:space:]]+any\\b' {file} && exit 1 || exit 0" + pattern: "$EXPR as any" + + no-dangerous-inner-html: + description: > + Do not use `dangerouslySetInnerHTML`. If you genuinely need raw + HTML, sanitize upstream and wrap the prop in a single reviewed + helper; never set it inline in components. + engine: ast + scope: "*.tsx" + severity: error + pattern: "<$TAG dangerouslySetInnerHTML={$_} />" no-ts-ignore: description: > diff --git a/pipeline/__init__.py b/pipeline/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/pipeline/pipeline.py b/pipeline/pipeline.py index 2538ddf..38ce817 100644 --- a/pipeline/pipeline.py +++ b/pipeline/pipeline.py @@ -14,6 +14,8 @@ import json import os import re +import shlex +import shutil import subprocess import sys import time @@ -25,10 +27,57 @@ # Config schema + parser # --------------------------------------------------------------------------- -VALID_ENGINES = {"script", "semantic"} +VALID_ENGINES = {"script", "semantic", "ast"} VALID_SEVERITIES = {"error", "warning"} -VALID_RULE_FIELDS = {"description", "engine", "scope", "severity", "script", "fix_hint"} -VALID_TOP_LEVEL = {"rules", "schema_version", "extends"} +VALID_RULE_FIELDS = { + "description", + "engine", + "scope", + "severity", + "script", + "fix_hint", + "pattern", + "language", +} +VALID_TOP_LEVEL = {"rules", "schema_version", "extends", "skip"} + +# User-global ignore file: one glob per line, blank lines and `#` comments +# allowed. Loaded by `effective_skip_patterns` and merged with the built-in +# `SKIP_PATTERNS` plus anything declared in `.bully.yml`. +USER_GLOBAL_IGNORE_FILENAME = ".bully-ignore" + +# ast-grep `--lang` values per file extension. +_AST_LANG_BY_EXT: dict[str, str] = { + ".ts": "ts", + ".tsx": "tsx", + ".js": "js", + ".jsx": "jsx", + ".mjs": "js", + ".cjs": "js", + ".py": "python", + ".rb": "ruby", + ".go": "go", + ".rs": "rust", + ".php": "php", + ".cs": "csharp", + ".java": "java", + ".kt": "kotlin", + ".swift": "swift", + ".c": "c", + ".h": "c", + ".cc": "cpp", + ".cpp": "cpp", + ".hpp": "cpp", + ".scala": "scala", + ".lua": "lua", + ".html": "html", + ".css": "css", + ".yaml": "yaml", + ".yml": "yaml", + ".json": "json", + ".sh": "bash", + ".bash": "bash", +} # Files we never want to lint -- lockfiles, minified bundles, generated code. SKIP_PATTERNS: tuple[str, ...] = ( @@ -69,6 +118,8 @@ class Rule: severity: str script: str | None = None fix_hint: str | None = None + pattern: str | None = None + language: str | None = None @dataclass @@ -148,6 +199,7 @@ class _ParsedConfig: rules: list[Rule] = field(default_factory=list) extends: list[str] = field(default_factory=list) + skip: list[str] = field(default_factory=list) schema_version: int | None = None @@ -167,6 +219,8 @@ def _parse_single_file(path: str) -> _ParsedConfig: seen_ids: set[str] = set() in_rules_block = False in_extends_block = False + in_skip_block = False + skip: list[str] = [] def finalize_rule() -> None: nonlocal current_id, fields, field_lines @@ -217,6 +271,15 @@ def finalize_rule() -> None: elif in_extends_block: in_extends_block = False + # Skip-block continuation: `- glob` at indent 2. + if in_skip_block and indent >= 2 and stripped.startswith("-"): + item = _parse_scalar(stripped[1:].strip()) + if item: + skip.append(item) + continue + elif in_skip_block: + in_skip_block = False + # Top-level key (indent 0). if indent == 0: if current_id is not None: @@ -253,6 +316,17 @@ def finalize_rule() -> None: in_extends_block = True else: raise ConfigError("extends must be a list like [pack-a, './local.yml']", lineno) + elif key == "skip": + as_list = _parse_inline_list(value_raw) + if as_list is not None: + skip.extend(g for g in as_list if g) + elif value_raw == "": + in_skip_block = True + else: + raise ConfigError( + 'skip must be a list like ["_build/**", "vendor/**"]', + lineno, + ) # `rules:` handled above; anything else would have raised already. continue @@ -313,7 +387,12 @@ def finalize_rule() -> None: if current_id is not None: finalize_rule() - return _ParsedConfig(rules=rules, extends=extends, schema_version=schema_version) + return _ParsedConfig( + rules=rules, + extends=extends, + skip=skip, + schema_version=schema_version, + ) def _build_rule( @@ -328,7 +407,7 @@ def _build_rule( engine = str(fields.get("engine", "script")) if engine not in VALID_ENGINES: raise ConfigError( - f"rule '{rule_id}': invalid engine {engine!r} (must be 'script' or 'semantic')", + f"rule '{rule_id}': invalid engine {engine!r} (must be 'script', 'semantic', or 'ast')", field_lines.get("engine", rule_line), ) @@ -340,6 +419,9 @@ def _build_rule( ) script_value = fields.get("script") + pattern_value = fields.get("pattern") + language_value = fields.get("language") + if engine == "script" and script_value is None: raise ConfigError( f"rule '{rule_id}': engine is 'script' but no 'script' field provided", @@ -351,6 +433,28 @@ def _build_rule( f"(contradiction -- remove one)", field_lines.get("script", rule_line), ) + if engine == "ast": + if pattern_value is None: + raise ConfigError( + f"rule '{rule_id}': engine is 'ast' but no 'pattern' field provided", + rule_line, + ) + if script_value is not None: + raise ConfigError( + f"rule '{rule_id}': engine is 'ast' but a 'script' field is set " + f"(contradiction -- use 'pattern' for ast rules)", + field_lines.get("script", rule_line), + ) + if engine != "ast" and pattern_value is not None: + raise ConfigError( + f"rule '{rule_id}': 'pattern' is only valid when engine is 'ast'", + field_lines.get("pattern", rule_line), + ) + if engine != "ast" and language_value is not None: + raise ConfigError( + f"rule '{rule_id}': 'language' is only valid when engine is 'ast'", + field_lines.get("language", rule_line), + ) fix_hint_value = fields.get("fix_hint") @@ -362,6 +466,8 @@ def _build_rule( severity=severity, script=str(script_value) if script_value is not None else None, fix_hint=str(fix_hint_value) if fix_hint_value is not None else None, + pattern=str(pattern_value) if pattern_value is not None else None, + language=str(language_value) if language_value is not None else None, ) @@ -382,6 +488,32 @@ def _resolve_extends_target(spec: str, config_path: str) -> Path: return (config_dir / p).resolve() +def _collect_config_files(path: str, visited: list[str] | None = None) -> list[Path]: + """Return the absolute paths of a config plus every file it extends. + + Resolution order matches `_load_with_extends`: parents first, then self. + Used by the trust gate to compute a single checksum over the full + effective config. + """ + visited = visited or [] + abs_path = Path(path).resolve() + if str(abs_path) in visited: + return [] + visited = visited + [str(abs_path)] + if not abs_path.is_file(): + return [] + try: + parsed = _parse_single_file(str(abs_path)) + except ConfigError: + return [abs_path] + collected: list[Path] = [] + for spec in parsed.extends: + target = _resolve_extends_target(spec, str(abs_path)) + collected.extend(_collect_config_files(str(target), visited)) + collected.append(abs_path) + return collected + + def parse_config(path: str) -> list[Rule]: """Parse .bully.yml into Rule objects, resolving `extends:` transitively. @@ -431,12 +563,15 @@ def _load_with_extends(path: str, visited: list[str]) -> list[Rule]: # --------------------------------------------------------------------------- -def _path_matches_skip(file_path: str) -> bool: - """Return True if the path matches any SKIP_PATTERNS entry.""" +def _path_matches_skip( + file_path: str, + extra_patterns: tuple[str, ...] | list[str] = (), +) -> bool: + """Return True if the path matches any built-in or extra skip pattern.""" p = PurePath(file_path) name = p.name posix = p.as_posix() - for pat in SKIP_PATTERNS: + for pat in (*SKIP_PATTERNS, *extra_patterns): # Match basename (covers `*.min.js`, `package-lock.json`, etc.) if fnmatch.fnmatch(name, pat): return True @@ -457,6 +592,73 @@ def _path_matches_skip(file_path: str) -> bool: return False +def _load_user_global_skips() -> list[str]: + """Load globs from `~/.bully-ignore` (one per line, `#` comments allowed). + + Missing or unreadable files yield an empty list -- this is a per-user + convenience, never a hard requirement. + """ + path = Path.home() / USER_GLOBAL_IGNORE_FILENAME + if not path.is_file(): + return [] + try: + raw = path.read_text() + except OSError: + return [] + out: list[str] = [] + for line in raw.splitlines(): + s = line.strip() + if not s or s.startswith("#"): + continue + out.append(s) + return out + + +def _collect_skip_with_extends(path: str, visited: list[str] | None = None) -> list[str]: + """Walk a config and its extends chain, collecting `skip:` entries in order. + + Parents are visited first so child configs can append (the merge order in + `effective_skip_patterns` makes both equivalent for matching, but we keep + declaration order for predictable doctor output). + """ + visited = visited or [] + abs_path = str(Path(path).resolve()) + if abs_path in visited: + return [] + visited = visited + [abs_path] + if not Path(abs_path).is_file(): + return [] + try: + parsed = _parse_single_file(abs_path) + except ConfigError: + return [] + out: list[str] = [] + for spec in parsed.extends: + target = _resolve_extends_target(spec, abs_path) + out.extend(_collect_skip_with_extends(str(target), visited)) + out.extend(parsed.skip) + return out + + +def effective_skip_patterns( + config_path: str, + *, + include_user_global: bool = True, +) -> tuple[str, ...]: + """Return the merged tuple of built-in + user-global + project skip globs. + + Order: built-in defaults, then `~/.bully-ignore` (when enabled), then + every `skip:` entry pulled from the config and its extends chain. + Duplicates are preserved -- `_path_matches_skip` short-circuits on the + first match. + """ + project: list[str] = [] + if config_path and Path(config_path).is_file(): + project = _collect_skip_with_extends(config_path) + user_global = _load_user_global_skips() if include_user_global else [] + return (*SKIP_PATTERNS, *user_global, *project) + + def filter_rules(rules: list[Rule], file_path: str) -> list[Rule]: """Return rules whose scope glob(s) match the given file path.""" path = PurePath(file_path) @@ -667,8 +869,9 @@ def parse_script_output(rule_id: str, severity: str, output: str) -> list[Violat def execute_script_rule(rule: Rule, file_path: str, diff: str) -> list[Violation]: """Run a script-engine rule against a file.""" - cmd = rule.script.replace("{file}", file_path) + cmd = rule.script.replace("{file}", shlex.quote(file_path)) try: + # bully-disable: no-shell-true-subprocess script-engine contract; cmd is shlex.quote'd above result = subprocess.run( cmd, shell=True, @@ -705,6 +908,138 @@ def execute_script_rule(rule: Rule, file_path: str, diff: str) -> list[Violation return [] +# --------------------------------------------------------------------------- +# AST rule execution (ast-grep) +# --------------------------------------------------------------------------- + + +_AST_GREP_INSTALL_HINT = "install ast-grep: brew install ast-grep (or: cargo install ast-grep)" + + +def _infer_ast_language(file_path: str) -> str | None: + """Infer the ast-grep --lang value from a file path. Returns None if unknown.""" + suffix = PurePath(file_path).suffix.lower() + return _AST_LANG_BY_EXT.get(suffix) + + +def ast_grep_available() -> bool: + """Return True iff `ast-grep` is on PATH.""" + return shutil.which("ast-grep") is not None + + +def _parse_ast_grep_json(rule_id: str, severity: str, stdout: str) -> list[Violation]: + """Parse ast-grep's --json output into Violations. + + ast-grep emits a JSON array. Each match has `range.start.line` (0-indexed) + and `lines` (the matched source text). An empty array means no matches. + """ + stripped = stdout.strip() + if not stripped: + return [] + try: + parsed = json.loads(stripped) + except json.JSONDecodeError: + return [] + if not isinstance(parsed, list): + return [] + violations: list[Violation] = [] + for item in parsed: + if not isinstance(item, dict): + continue + rng = item.get("range") or {} + start = rng.get("start") if isinstance(rng, dict) else None + line_i: int | None = None + if isinstance(start, dict): + raw_line = start.get("line") + if isinstance(raw_line, int): + # ast-grep line numbers are 0-indexed; convert to 1-indexed. + line_i = raw_line + 1 + matched = item.get("lines") or item.get("text") or "" + description = str(matched).splitlines()[0].strip() if matched else "" + violations.append( + Violation( + rule=rule_id, + engine="ast", + severity=severity, + line=line_i, + description=description[:500], + ) + ) + return violations + + +def execute_ast_rule(rule: Rule, file_path: str) -> list[Violation]: + """Run an ast-engine rule against a file via ast-grep. + + Caller is responsible for checking `ast_grep_available()` beforehand and + handling the missing-tool path. This function assumes the binary exists + and returns [] on any execution error (conservative: don't block edits + due to tooling failure). + """ + lang = rule.language or _infer_ast_language(file_path) + if lang is None: + return [ + Violation( + rule=rule.id, + engine="ast", + severity=rule.severity, + line=None, + description=( + f"ast-grep: could not infer --lang from path {file_path!r}; " + "set `language:` on the rule" + ), + ) + ] + + cmd = [ + "ast-grep", + "run", + "--pattern", + rule.pattern or "", + "--lang", + lang, + "--json=compact", + file_path, + ] + try: + result = subprocess.run( + cmd, + shell=False, + capture_output=True, + text=True, + timeout=30, + ) + except subprocess.TimeoutExpired: + return [ + Violation( + rule=rule.id, + engine="ast", + severity=rule.severity, + line=None, + description=f"ast-grep timed out after 30s for pattern: {rule.pattern!r}", + ) + ] + except FileNotFoundError: + # ast-grep disappeared between the PATH check and now. Treat as no-op. + return [] + + if result.returncode not in (0, 1): + # 0 = no matches, 1 = matches (or sometimes error). We only trust stdout. + stderr_tail = (result.stderr or "").strip().splitlines()[-1:] + hint = stderr_tail[0] if stderr_tail else "" + return [ + Violation( + rule=rule.id, + engine="ast", + severity=rule.severity, + line=None, + description=f"ast-grep failed (exit {result.returncode}): {hint}"[:500], + ) + ] + + return _parse_ast_grep_json(rule.id, rule.severity, result.stdout) + + # --------------------------------------------------------------------------- # Semantic payload + pipeline-side can't-match filters # --------------------------------------------------------------------------- @@ -909,6 +1244,145 @@ def _line_has_disable(file_path: str, line: int | None, rule_id: str) -> bool: return False +# --------------------------------------------------------------------------- +# Trust boundary: per-machine allowlist for .bully.yml configs +# --------------------------------------------------------------------------- +# +# A `.bully.yml` can execute arbitrary shell commands via `engine: script` +# rules. Cloning a repo with a malicious `.bully.yml` and making any edit +# would run attacker-controlled code in the developer's shell. The trust +# gate prevents this: the first time bully sees a config on a given machine, +# it refuses to execute any rules until the user runs `bully trust`. After +# trust, the gate verifies the checksum on every run -- any change to the +# config (or any extended config) re-requires explicit trust. +# +# Trust state is machine-local (`~/.bully-trust.json`), never committed to +# repos. `BULLY_TRUST_ALL=1` bypasses the gate for CI and first-time setup +# scripts that have already reviewed the config through other means. + + +_TRUST_ENV_VAR = "BULLY_TRUST_ALL" + + +def _trust_store_path() -> Path: + """Per-machine allowlist location.""" + override = os.environ.get("BULLY_TRUST_STORE") + if override: + return Path(override).expanduser().resolve() + return Path.home() / ".bully-trust.json" + + +def _config_checksum(config_path: str) -> str: + """SHA256 over the concatenated bytes of a config and all its `extends:` targets. + + Returns '' when the top-level config is unreadable. + """ + files = _collect_config_files(config_path) + if not files: + return "" + h = hashlib.sha256() + for f in files: + try: + h.update(f.read_bytes()) + # Domain separator prevents collisions across different file splits. + h.update(b"\x00") + except OSError: + return "" + return h.hexdigest() + + +def _load_trust_store() -> dict: + """Parse the trust store. Returns {} on any read or parse error.""" + p = _trust_store_path() + if not p.is_file(): + return {} + try: + data = json.loads(p.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError): + return {} + return data if isinstance(data, dict) else {} + + +def _save_trust_store(store: dict) -> None: + """Write the trust store, creating parent dirs as needed.""" + p = _trust_store_path() + p.parent.mkdir(parents=True, exist_ok=True) + tmp = p.with_suffix(p.suffix + ".tmp") + tmp.write_text(json.dumps(store, indent=2) + "\n", encoding="utf-8") + tmp.replace(p) + + +def _trust_status(config_path: str) -> tuple[str, str]: + """Return (status, detail). Status is one of: 'trusted', 'untrusted', 'mismatch'. + + 'untrusted' means the config has never been trusted on this machine. + 'mismatch' means it was trusted, but the contents have since changed. + """ + if os.environ.get(_TRUST_ENV_VAR) == "1": + return "trusted", "env:BULLY_TRUST_ALL" + abs_path = str(Path(config_path).resolve()) + checksum = _config_checksum(abs_path) + if not checksum: + return "untrusted", "cannot read config" + store = _load_trust_store() + entry = store.get("allowed", {}).get(abs_path) + if not isinstance(entry, dict): + return "untrusted", "never trusted" + recorded = entry.get("checksum", "") + if recorded != checksum: + return "mismatch", f"checksum changed (was {recorded[:12]}..., now {checksum[:12]}...)" + return "trusted", recorded[:12] + "..." + + +def _untrusted_stderr(config_path: str, status: str, detail: str) -> str: + """Rendered stderr message for untrusted/mismatched configs.""" + abs_path = Path(config_path).resolve() + if status == "mismatch": + headline = f"bully: {abs_path} changed since last trust ({detail})." + action = "Re-review the config, then run: bully trust --refresh" + else: + headline = f"bully: {abs_path} is not trusted on this machine." + action = "Review the config, then run: bully trust" + return ( + f"{headline}\n" + f"Scripts in .bully.yml execute on your machine. " + f"Until trusted, rules will not run. Edits are not blocked.\n" + f"{action}\n" + f"(To allow all configs unconditionally -- not recommended -- " + f"set {_TRUST_ENV_VAR}=1.)\n" + ) + + +def _cmd_trust(config_path: str | None, refresh: bool) -> int: + """Record the current config's checksum in the trust store.""" + path = config_path or ".bully.yml" + abs_path = Path(path).resolve() + if not abs_path.is_file(): + print(f"config not found: {abs_path}", file=sys.stderr) + return 1 + checksum = _config_checksum(str(abs_path)) + if not checksum: + print(f"cannot checksum config at {abs_path}", file=sys.stderr) + return 1 + + store = _load_trust_store() + allowed = store.setdefault("allowed", {}) + existing = allowed.get(str(abs_path)) + if isinstance(existing, dict) and existing.get("checksum") == checksum and not refresh: + print(f"already trusted: {abs_path} sha256={checksum[:12]}...") + return 0 + allowed[str(abs_path)] = { + "checksum": checksum, + "allowed_at": datetime.now(timezone.utc) + .isoformat(timespec="seconds") + .replace("+00:00", "Z"), + } + _save_trust_store(store) + verb = "updated" if existing else "trusted" + print(f"{verb}: {abs_path} sha256={checksum[:12]}...") + return 0 + + # --------------------------------------------------------------------------- # Telemetry # --------------------------------------------------------------------------- @@ -962,24 +1436,50 @@ def run_pipeline( file_path: str, diff: str, rule_filter: set[str] | None = None, + *, + include_skipped: bool = False, ) -> dict: """Full two-phase pipeline. Phase 1: script rules. If any error-severity violations, block. Phase 2: build semantic payload for remaining semantic rules. + + When `include_skipped=True`, the result dict gains two extra fields: + `semantic_skipped` (a list of `{"rule", "reason"}` for every semantic rule + the can't-match heuristics dropped) and `rules_evaluated` (a list of + `{"rule", "engine", "verdict", "reason"?}` for every rule in scope). + Both are intentionally gated -- hook-mode output stays unchanged. """ start = time.perf_counter() rule_records: list[dict] = [] log_path = _telemetry_path(config_path) - # Short-circuit auto-generated files. - if _path_matches_skip(file_path): + # Short-circuit auto-generated files (built-in + user-global + project skip). + extra_skip = effective_skip_patterns(config_path)[len(SKIP_PATTERNS):] + if _path_matches_skip(file_path, extra_patterns=extra_skip): elapsed_ms = int((time.perf_counter() - start) * 1000) result = {"status": "skipped", "file": file_path, "reason": "auto-generated"} if log_path is not None: _append_telemetry(log_path, file_path, "skipped", rule_records, elapsed_ms) return result + # Trust gate: refuse to execute any rules from an un-reviewed config. + trust_status, trust_detail = _trust_status(config_path) + if trust_status != "trusted": + elapsed_ms = int((time.perf_counter() - start) * 1000) + result = { + "status": "untrusted", + "file": file_path, + "config": str(Path(config_path).resolve()), + "trust_status": trust_status, + "trust_detail": trust_detail, + } + if log_path is not None: + _append_telemetry( + log_path, file_path, f"untrusted:{trust_status}", rule_records, elapsed_ms + ) + return result + rules = parse_config(config_path) matching = filter_rules(rules, file_path) if rule_filter: @@ -995,22 +1495,25 @@ def flush(status: str, result: dict) -> dict: return flush("pass", {"status": "pass", "file": file_path}) script_rules = [r for r in matching if r.engine == "script"] + ast_rules = [r for r in matching if r.engine == "ast"] semantic_rules = [r for r in matching if r.engine == "semantic"] all_violations: list[Violation] = [] passed_checks: list[str] = [] baseline = _load_baseline(config_path) - for rule in script_rules: + def _run_deterministic( + rule: Rule, + executor, + engine_label: str, + ) -> None: rule_start = time.perf_counter() - violations = execute_script_rule(rule, file_path, diff) + violations = executor() rule_ms = int((time.perf_counter() - rule_start) * 1000) - # Apply fix_hint as fallback suggestion. if rule.fix_hint: violations = [replace(v, suggestion=v.suggestion or rule.fix_hint) for v in violations] - # Filter per-line disables. filtered: list[Violation] = [] for v in violations: if _line_has_disable(file_path, v.line, rule.id): @@ -1025,7 +1528,7 @@ def flush(status: str, result: dict) -> dict: rule_records.append( { "id": rule.id, - "engine": "script", + "engine": engine_label, "verdict": "violation", "severity": rule.severity, "line": violations[0].line, @@ -1037,15 +1540,47 @@ def flush(status: str, result: dict) -> dict: rule_records.append( { "id": rule.id, - "engine": "script", + "engine": engine_label, "verdict": "pass", "severity": rule.severity, "latency_ms": rule_ms, } ) + for rule in script_rules: + _run_deterministic( + rule, + lambda r=rule: execute_script_rule(r, file_path, diff), + "script", + ) + + if ast_rules: + if ast_grep_available(): + for rule in ast_rules: + _run_deterministic( + rule, + lambda r=rule: execute_ast_rule(r, file_path), + "ast", + ) + else: + sys.stderr.write( + "bully: engine:ast rules matched but ast-grep not on PATH; skipping. " + f"{_AST_GREP_INSTALL_HINT}\n" + ) + for rule in ast_rules: + rule_records.append( + { + "id": rule.id, + "engine": "ast", + "verdict": "skipped", + "severity": rule.severity, + "reason": "ast-grep-not-installed", + } + ) + # Can't-match filters for semantic rules. dispatched_semantic: list[Rule] = [] + semantic_skipped: list[dict] = [] for rule in semantic_rules: ok, reason = _can_match_diff(rule, diff) if ok: @@ -1059,6 +1594,7 @@ def flush(status: str, result: dict) -> dict: } ) else: + semantic_skipped.append({"rule": rule.id, "reason": reason}) if log_path is not None: _append_record( log_path, @@ -1075,15 +1611,26 @@ def flush(status: str, result: dict) -> dict: blocking = [v for v in all_violations if v.severity == "error"] + def _decorate(result: dict) -> dict: + if not include_skipped: + return result + result["semantic_skipped"] = list(semantic_skipped) + result["rules_evaluated"] = _explain_rules_evaluated( + rule_records, semantic_skipped, dispatched_semantic + ) + return result + if blocking: - return flush( - "blocked", - { - "status": "blocked", - "file": file_path, - "violations": [asdict(v) for v in all_violations], - "passed": passed_checks, - }, + return _decorate( + flush( + "blocked", + { + "status": "blocked", + "file": file_path, + "violations": [asdict(v) for v in all_violations], + "passed": passed_checks, + }, + ) ) if dispatched_semantic: @@ -1093,12 +1640,92 @@ def flush(status: str, result: dict) -> dict: result["write_content"] = "truncated" if all_violations: result["warnings"] = [asdict(v) for v in all_violations] - return flush("evaluate", result) + return _decorate(flush("evaluate", result)) result = {"status": "pass", "file": file_path, "passed": passed_checks} if all_violations: result["warnings"] = [asdict(v) for v in all_violations] - return flush("pass", result) + return _decorate(flush("pass", result)) + + +def _explain_rules_evaluated( + rule_records: list[dict], + semantic_skipped: list[dict], + dispatched_semantic: list[Rule], +) -> list[dict]: + """Project the internal `rule_records` into a per-rule verdict line. + + Verdicts: `fire` (deterministic violation), `pass` (deterministic clean + or semantic dispatched-no-violation), `skipped` (can't-match heuristic + or ast-grep missing), `dispatched` (semantic rule sent to the evaluator). + """ + dispatched_ids = {r.id for r in dispatched_semantic} + out: list[dict] = [] + for rec in rule_records: + rule_id = rec.get("id", "") + engine = rec.get("engine", "") + record_verdict = rec.get("verdict", "") + if record_verdict == "violation": + out.append({"rule": rule_id, "engine": engine, "verdict": "fire"}) + elif record_verdict == "pass": + out.append({"rule": rule_id, "engine": engine, "verdict": "pass"}) + elif record_verdict == "evaluate_requested": + out.append( + { + "rule": rule_id, + "engine": engine, + "verdict": "dispatched" if rule_id in dispatched_ids else "pass", + } + ) + elif record_verdict == "skipped": + out.append( + { + "rule": rule_id, + "engine": engine, + "verdict": "skipped", + "reason": rec.get("reason", ""), + } + ) + for skip in semantic_skipped: + out.append( + { + "rule": skip["rule"], + "engine": "semantic", + "verdict": "skipped", + "reason": skip["reason"], + } + ) + return out + + +def _print_explain(result: dict, file_path: str) -> None: + """Render the --explain output: one line per rule in scope. + + Falls back to a clear one-liner when the result has a non-evaluating + status (skipped, untrusted, no rules in scope) so authors aren't left + staring at silence. + """ + status = result.get("status", "") + print(f"file: {file_path}") + print(f"status: {status}") + if status == "skipped": + print(f" pipeline skipped (reason: {result.get('reason', 'unknown')})") + return + if status == "untrusted": + print(f" config not trusted on this machine ({result.get('trust_detail', '')})") + return + rules = result.get("rules_evaluated", []) + if not rules: + print(" no rules matched the file's scope") + return + for r in rules: + verdict = r.get("verdict", "") + rule_id = r.get("rule", "") + engine = r.get("engine", "") + if verdict == "skipped": + print(f" [{engine}] {rule_id}: skipped ({r.get('reason', '')})") + else: + print(f" [{engine}] {rule_id}: {verdict}") def _was_write_truncated_for_path(file_path: str) -> bool: @@ -1171,9 +1798,44 @@ def _build_semantic_prompt(payload: dict) -> str: return "\n".join(lines) +# Subcommand verbs accepted as the first argv element. Each maps to either a +# flag or a small argv rewrite. Keeps the legacy `--validate`/`--doctor` flags +# and the legacy positional ` ` form (used by hook.sh) working. +_SUBCOMMAND_FLAGS = { + "validate": "--validate", + "doctor": "--doctor", + "show-resolved-config": "--show-resolved-config", + "baseline-init": "--baseline-init", + "trust": "--trust", +} + + +def _normalize_argv(argv: list[str]) -> list[str]: + """Translate `bully ...` shorthand into the underlying flag form. + + - `validate` / `doctor` / `show-resolved-config` / `baseline-init` / `trust` + become their `--verb` flag equivalents. + - `lint ` becomes `--file ` (the rest of argv is preserved). + - Anything else passes through unchanged so legacy positional and flag + invocations keep working. + """ + if not argv: + return argv + head = argv[0] + if head in _SUBCOMMAND_FLAGS: + return [_SUBCOMMAND_FLAGS[head], *argv[1:]] + if head == "lint": + rest = argv[1:] + if rest and not rest[0].startswith("-"): + return ["--file", rest[0], *rest[1:]] + return rest + return argv + + def _parse_args(argv: list[str]) -> argparse.Namespace: + argv = _normalize_argv(argv) parser = argparse.ArgumentParser( - prog="pipeline.py", + prog="bully", description="Agentic Lint pipeline. Runs script and semantic rules for a file.", ) parser.add_argument("positional", nargs="*", help=argparse.SUPPRESS) @@ -1227,6 +1889,23 @@ def _parse_args(argv: list[str]) -> argparse.Namespace: help="Append a semantic_verdict telemetry record.", ) parser.add_argument("--verdict", choices=("pass", "violation"), default=None) + parser.add_argument( + "--trust", + action="store_true", + help="Allow the given --config to execute rules on this machine. " + "Records a SHA256 checksum; edits to the config re-require --trust.", + ) + parser.add_argument( + "--refresh", + action="store_true", + help="With --trust: re-approve a changed config. Without --trust: no-op.", + ) + parser.add_argument( + "--explain", + action="store_true", + help="Print per-rule verdict (fire/pass/skipped /dispatched) for " + "every rule in scope, instead of the JSON pipeline result.", + ) args = parser.parse_args(argv) # Back-compat: accept positional args (used by hook) if args.positional and not args.config: @@ -1252,6 +1931,13 @@ def _cmd_validate(config_path: str | None) -> int: print(f"[OK] parsed {len(rules)} rule(s) from {path}") for r in rules: print(f" - {r.id} engine={r.engine} severity={r.severity} scope={list(r.scope)}") + ast_rule_ids = [r.id for r in rules if r.engine == "ast"] + if ast_rule_ids and not ast_grep_available(): + print( + f"[WARN] {len(ast_rule_ids)} engine:ast rule(s) will be skipped at runtime: " + f"ast-grep not on PATH. {_AST_GREP_INSTALL_HINT}", + file=sys.stderr, + ) return 0 @@ -1285,14 +1971,42 @@ def _cmd_doctor() -> int: ok = False # Config parses + parsed_rules: list[Rule] = [] if cfg.is_file(): try: - rules = parse_config(str(cfg)) - print(f"[OK] config parses ({len(rules)} rules)") + parsed_rules = parse_config(str(cfg)) + print(f"[OK] config parses ({len(parsed_rules)} rules)") except ConfigError as e: print(f"[FAIL] config parse error: {e}") ok = False + # Trust status for the local config (machine-local, not committed). + if cfg.is_file(): + status, detail = _trust_status(str(cfg)) + if status == "trusted": + print(f"[OK] config trusted on this machine ({detail})") + elif status == "mismatch": + print( + f"[WARN] config trusted but checksum changed: {detail}. Run: bully trust --refresh" + ) + else: + print( + f"[WARN] config not trusted on this machine ({detail}). " + "Rules will not run until you run: bully trust" + ) + + # ast-grep availability (only matters if engine:ast rules exist) + ast_rule_count = sum(1 for r in parsed_rules if r.engine == "ast") + if ast_rule_count > 0: + if ast_grep_available(): + print(f"[OK] ast-grep on PATH ({ast_rule_count} engine:ast rule(s))") + else: + print( + f"[FAIL] {ast_rule_count} engine:ast rule(s) need ast-grep. " + f"{_AST_GREP_INSTALL_HINT}" + ) + ok = False + # PostToolUse hook wired in .claude/settings.json hook_wired = False for settings in ( @@ -1380,11 +2094,12 @@ def _cmd_baseline_init(config_path: str | None, glob: str | None) -> int: root = cfg_abs.parent if not glob: glob = "**/*" + extra_skip = effective_skip_patterns(str(cfg_abs))[len(SKIP_PATTERNS):] entries: list[dict] = [] for candidate in root.glob(glob): if not candidate.is_file(): continue - if _path_matches_skip(str(candidate)): + if _path_matches_skip(str(candidate), extra_patterns=extra_skip): continue try: result = run_pipeline(str(cfg_abs), str(candidate), "") @@ -1471,6 +2186,15 @@ def _hook_mode() -> int: return 0 status = result.get("status", "pass") + if status == "untrusted": + sys.stderr.write( + _untrusted_stderr( + result.get("config", str(config)), + result.get("trust_status", "untrusted"), + result.get("trust_detail", ""), + ) + ) + return 0 if status == "blocked": sys.stderr.write(_format_blocked_stderr(result)) return 2 @@ -1495,6 +2219,8 @@ def main() -> None: args = _parse_args(sys.argv[1:]) # Subcommands. + if args.trust: + sys.exit(_cmd_trust(args.config, refresh=args.refresh)) if args.validate: sys.exit(_cmd_validate(args.config)) if args.doctor: @@ -1515,12 +2241,17 @@ def main() -> None: if args.hook_mode: sys.exit(_hook_mode()) + # Default config to ./.bully.yml when a target file is given but no + # config is specified -- lets `bully lint src/foo.py` work standalone. + if args.file_path and not args.config and os.path.exists(".bully.yml"): + args.config = ".bully.yml" + if not args.config or not args.file_path: print( json.dumps( { - "error": "Usage: pipeline.py --config --file " - "(or positional: pipeline.py )" + "error": "Usage: bully lint [--config ] " + "(or pipeline.py )" } ), file=sys.stderr, @@ -1563,11 +2294,16 @@ def main() -> None: file_path, diff, rule_filter=set(args.rule) if args.rule else None, + include_skipped=args.explain, ) except ConfigError as e: print(json.dumps({"status": "error", "error": str(e)}), file=sys.stderr) sys.exit(1) + if args.explain: + _print_explain(result, file_path) + return + if args.print_prompt: if result.get("status") == "evaluate": print(_build_semantic_prompt(result)) @@ -1585,6 +2321,15 @@ def main() -> None: print(json.dumps(result, indent=2)) + if result.get("status") == "untrusted": + sys.stderr.write( + _untrusted_stderr( + result.get("config", config_path), + result.get("trust_status", "untrusted"), + result.get("trust_detail", ""), + ) + ) + sys.exit(0) if result.get("status") == "blocked": sys.stderr.write(_format_blocked_stderr(result)) sys.exit(2) diff --git a/pipeline/tests/conftest.py b/pipeline/tests/conftest.py new file mode 100644 index 0000000..91c2cb2 --- /dev/null +++ b/pipeline/tests/conftest.py @@ -0,0 +1,12 @@ +"""Shared pytest fixtures. + +The trust gate refuses to execute rules from an un-reviewed .bully.yml. Every +test exercising run_pipeline would otherwise short-circuit with status +'untrusted' -- so we unconditionally allow configs inside the test process +via the BULLY_TRUST_ALL env var. Tests that explicitly validate the trust +gate must override this by temporarily unsetting the var with monkeypatch. +""" + +import os + +os.environ.setdefault("BULLY_TRUST_ALL", "1") diff --git a/pipeline/tests/test_ast_engine.py b/pipeline/tests/test_ast_engine.py new file mode 100644 index 0000000..b7daf51 --- /dev/null +++ b/pipeline/tests/test_ast_engine.py @@ -0,0 +1,437 @@ +"""Tests for the ast engine: parser, language inference, JSON adapter, +executor, and graceful skip when ast-grep is missing.""" + +import json +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import pipeline as pipeline_mod +from pipeline import ( + ConfigError, + Rule, + _infer_ast_language, + _parse_ast_grep_json, + execute_ast_rule, + parse_config, + run_pipeline, +) + +FIXTURES = Path(__file__).parent / "fixtures" + + +# ---- parser tests ------------------------------------------------------- + + +def _write_config(tmp_path: Path, body: str) -> Path: + p = tmp_path / ".bully.yml" + p.write_text(body) + return p + + +def test_parse_ast_rule_with_pattern(tmp_path: Path): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " no-any-cast:\n" + " description: No `as any`\n" + " engine: ast\n" + ' scope: ["*.ts", "*.tsx"]\n' + " severity: error\n" + ' pattern: "$EXPR as any"\n', + ) + rules = parse_config(str(cfg)) + assert len(rules) == 1 + r = rules[0] + assert r.id == "no-any-cast" + assert r.engine == "ast" + assert r.pattern == "$EXPR as any" + assert r.language is None # inferred at runtime + assert r.script is None + + +def test_parse_ast_rule_with_explicit_language(tmp_path: Path): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " no-var-dump:\n" + " description: No var_dump\n" + " engine: ast\n" + ' scope: "*.php"\n' + " severity: error\n" + ' pattern: "var_dump($$$)"\n' + " language: php\n", + ) + rules = parse_config(str(cfg)) + assert rules[0].language == "php" + assert rules[0].pattern == "var_dump($$$)" + + +def test_parse_ast_rule_without_pattern_raises(tmp_path: Path): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " bad:\n" + " description: missing pattern\n" + " engine: ast\n" + ' scope: "*.ts"\n' + " severity: error\n", + ) + with pytest.raises(ConfigError) as exc: + parse_config(str(cfg)) + assert "pattern" in str(exc.value) + + +def test_parse_ast_rule_with_script_is_contradiction(tmp_path: Path): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " bad:\n" + " description: contradiction\n" + " engine: ast\n" + ' scope: "*.ts"\n' + " severity: error\n" + ' pattern: "foo"\n' + ' script: "grep foo {file}"\n', + ) + with pytest.raises(ConfigError) as exc: + parse_config(str(cfg)) + assert "script" in str(exc.value).lower() + + +def test_pattern_on_script_engine_raises(tmp_path: Path): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " bad:\n" + " description: wrong engine for pattern\n" + " engine: script\n" + ' scope: "*.ts"\n' + " severity: error\n" + ' script: "true"\n' + ' pattern: "foo"\n', + ) + with pytest.raises(ConfigError) as exc: + parse_config(str(cfg)) + assert "pattern" in str(exc.value).lower() + + +def test_language_on_semantic_engine_raises(tmp_path: Path): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " bad:\n" + " description: wrong engine for language\n" + " engine: semantic\n" + ' scope: "*.ts"\n' + " severity: error\n" + " language: ts\n", + ) + with pytest.raises(ConfigError) as exc: + parse_config(str(cfg)) + assert "language" in str(exc.value).lower() + + +def test_unknown_engine_mentions_ast(tmp_path: Path): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " bad:\n" + " description: x\n" + " engine: banana\n" + ' scope: "*.ts"\n' + " severity: error\n", + ) + with pytest.raises(ConfigError) as exc: + parse_config(str(cfg)) + assert "ast" in str(exc.value) + + +# ---- language inference ------------------------------------------------- + + +def test_infer_ts_from_tsx(): + assert _infer_ast_language("src/App.tsx") == "tsx" + + +def test_infer_csharp_from_cs(): + assert _infer_ast_language("Services/User.cs") == "csharp" + + +def test_infer_php_from_php(): + assert _infer_ast_language("app/Models/User.php") == "php" + + +def test_infer_unknown_extension_returns_none(): + assert _infer_ast_language("notes.txt") is None + + +def test_infer_is_case_insensitive(): + assert _infer_ast_language("App.TSX") == "tsx" + + +# ---- JSON adapter ------------------------------------------------------- + + +def test_parse_ast_grep_json_empty_array(): + assert _parse_ast_grep_json("rule", "error", "[]") == [] + + +def test_parse_ast_grep_json_single_match(): + payload = json.dumps( + [ + { + "range": {"start": {"line": 41, "column": 0}, "end": {"line": 41, "column": 20}}, + "lines": " const x = foo as any;", + } + ] + ) + violations = _parse_ast_grep_json("no-any-cast", "error", payload) + assert len(violations) == 1 + v = violations[0] + assert v.rule == "no-any-cast" + assert v.engine == "ast" + assert v.severity == "error" + assert v.line == 42 # 0-indexed -> 1-indexed + assert "foo as any" in v.description + + +def test_parse_ast_grep_json_multiple_matches(): + payload = json.dumps( + [ + {"range": {"start": {"line": 0}}, "lines": "first match"}, + {"range": {"start": {"line": 10}}, "lines": "second match"}, + ] + ) + violations = _parse_ast_grep_json("r", "warning", payload) + assert [v.line for v in violations] == [1, 11] + assert all(v.engine == "ast" for v in violations) + + +def test_parse_ast_grep_json_malformed_returns_empty(): + assert _parse_ast_grep_json("r", "error", "not json at all") == [] + + +def test_parse_ast_grep_json_non_array_returns_empty(): + assert _parse_ast_grep_json("r", "error", '{"oops": true}') == [] + + +# ---- executor tests (mock subprocess) ---------------------------------- + + +class _FakeCompleted: + def __init__(self, stdout: str, stderr: str = "", returncode: int = 0): + self.stdout = stdout + self.stderr = stderr + self.returncode = returncode + + +def test_execute_ast_rule_no_match_returns_empty(tmp_path: Path): + target = tmp_path / "file.ts" + target.write_text("const x = 1;\n") + rule = Rule( + id="no-any-cast", + description="no as any", + engine="ast", + scope=("*.ts",), + severity="error", + pattern="$E as any", + ) + with patch.object(pipeline_mod.subprocess, "run") as mock_run: + mock_run.return_value = _FakeCompleted("[]", returncode=0) + violations = execute_ast_rule(rule, str(target)) + assert violations == [] + called_cmd = mock_run.call_args.args[0] + assert called_cmd[0] == "ast-grep" + assert "--pattern" in called_cmd + assert "$E as any" in called_cmd + assert "--lang" in called_cmd + assert "ts" in called_cmd + # shell=False for ast rules (unlike script rules). + assert mock_run.call_args.kwargs.get("shell") is False + + +def test_execute_ast_rule_match_produces_violation(tmp_path: Path): + target = tmp_path / "file.ts" + target.write_text("x;\n") + rule = Rule( + id="no-any-cast", + description="no as any", + engine="ast", + scope=("*.ts",), + severity="error", + pattern="$E as any", + ) + fake_out = json.dumps([{"range": {"start": {"line": 0}}, "lines": "const x = y as any;"}]) + with patch.object(pipeline_mod.subprocess, "run") as mock_run: + mock_run.return_value = _FakeCompleted(fake_out, returncode=1) + violations = execute_ast_rule(rule, str(target)) + assert len(violations) == 1 + assert violations[0].rule == "no-any-cast" + assert violations[0].engine == "ast" + assert violations[0].line == 1 + + +def test_execute_ast_rule_unknown_language_returns_config_violation(tmp_path: Path): + target = tmp_path / "weird.xyz" + target.write_text("data\n") + rule = Rule( + id="r", + description="desc", + engine="ast", + scope=("*.xyz",), + severity="error", + pattern="foo", + ) + violations = execute_ast_rule(rule, str(target)) + assert len(violations) == 1 + assert "could not infer" in violations[0].description + + +def test_execute_ast_rule_explicit_language_overrides_inference(tmp_path: Path): + target = tmp_path / "file.ts" + target.write_text("x;\n") + rule = Rule( + id="r", + description="desc", + engine="ast", + scope=("*.ts",), + severity="error", + pattern="foo", + language="tsx", + ) + with patch.object(pipeline_mod.subprocess, "run") as mock_run: + mock_run.return_value = _FakeCompleted("[]", returncode=0) + execute_ast_rule(rule, str(target)) + called_cmd = mock_run.call_args.args[0] + assert "tsx" in called_cmd + assert "ts" not in [c for c in called_cmd if c == "ts"] + + +def test_execute_ast_rule_tool_missing_returns_empty(tmp_path: Path): + target = tmp_path / "file.ts" + target.write_text("x;\n") + rule = Rule( + id="r", + description="desc", + engine="ast", + scope=("*.ts",), + severity="error", + pattern="$E as any", + ) + with patch.object(pipeline_mod.subprocess, "run", side_effect=FileNotFoundError): + assert execute_ast_rule(rule, str(target)) == [] + + +def test_execute_ast_rule_error_exit_surfaces_stderr(tmp_path: Path): + target = tmp_path / "file.ts" + target.write_text("x;\n") + rule = Rule( + id="r", + description="desc", + engine="ast", + scope=("*.ts",), + severity="error", + pattern="broken(pattern", + ) + with patch.object(pipeline_mod.subprocess, "run") as mock_run: + mock_run.return_value = _FakeCompleted("", stderr="error: invalid pattern\n", returncode=2) + violations = execute_ast_rule(rule, str(target)) + assert len(violations) == 1 + assert "ast-grep failed" in violations[0].description + assert "invalid pattern" in violations[0].description + + +# ---- pipeline integration: graceful skip when tool missing -------------- + + +def test_pipeline_skips_ast_rules_when_tool_missing(tmp_path: Path, capsys): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " no-any:\n" + " description: demo\n" + " engine: ast\n" + ' scope: "*.ts"\n' + " severity: error\n" + ' pattern: "$E as any"\n', + ) + target = tmp_path / "file.ts" + target.write_text("const x = 1;\n") + + with patch.object(pipeline_mod, "ast_grep_available", return_value=False): + result = run_pipeline(str(cfg), str(target), "") + + assert result["status"] == "pass" + captured = capsys.readouterr() + assert "ast-grep not on PATH" in captured.err + assert "brew install ast-grep" in captured.err + + +def test_pipeline_runs_ast_rules_when_tool_present(tmp_path: Path): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " no-any:\n" + " description: demo\n" + " engine: ast\n" + ' scope: "*.ts"\n' + " severity: error\n" + ' pattern: "$E as any"\n', + ) + target = tmp_path / "file.ts" + target.write_text("const x = foo as any;\n") + + fake_out = json.dumps([{"range": {"start": {"line": 0}}, "lines": "const x = foo as any;"}]) + + with ( + patch.object(pipeline_mod, "ast_grep_available", return_value=True), + patch.object(pipeline_mod.subprocess, "run") as mock_run, + ): + mock_run.return_value = _FakeCompleted(fake_out, returncode=1) + result = run_pipeline(str(cfg), str(target), "") + + assert result["status"] == "blocked" + assert result["violations"][0]["rule"] == "no-any" + assert result["violations"][0]["engine"] == "ast" + + +def test_pipeline_ast_warning_does_not_block(tmp_path: Path): + cfg = _write_config( + tmp_path, + "schema_version: 1\n" + "rules:\n" + " note-pattern:\n" + " description: demo\n" + " engine: ast\n" + ' scope: "*.ts"\n' + " severity: warning\n" + ' pattern: "$E as any"\n', + ) + target = tmp_path / "file.ts" + target.write_text("const x = foo as any;\n") + + fake_out = json.dumps([{"range": {"start": {"line": 0}}, "lines": "const x = foo as any;"}]) + + with ( + patch.object(pipeline_mod, "ast_grep_available", return_value=True), + patch.object(pipeline_mod.subprocess, "run") as mock_run, + ): + mock_run.return_value = _FakeCompleted(fake_out, returncode=1) + result = run_pipeline(str(cfg), str(target), "") + + assert result["status"] == "pass" + assert "warnings" in result and result["warnings"][0]["rule"] == "note-pattern" diff --git a/pipeline/tests/test_bully_subcommands.py b/pipeline/tests/test_bully_subcommands.py new file mode 100644 index 0000000..434f3e9 --- /dev/null +++ b/pipeline/tests/test_bully_subcommands.py @@ -0,0 +1,162 @@ +"""Tests for `bully` subcommand-style invocation. + +The console script entrypoint declared in pyproject.toml maps `bully` to +`pipeline.pipeline:main`, so `bully validate` is just `python pipeline.py +validate`. These tests exercise the subcommand normalization layer that +translates `validate`/`doctor`/`lint `/`show-resolved-config` into the +existing flag-based interface, while keeping the legacy positional form (used +by `hook.sh`) and the explicit `--validate`/`--doctor` flags working. +""" + +import json +import subprocess +import sys +from pathlib import Path + +PIPELINE = Path(__file__).resolve().parent.parent / "pipeline.py" +FIXTURES = Path(__file__).parent / "fixtures" + + +def _run(args, cwd: Path | None = None, stdin: str = "") -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(PIPELINE), *args], + input=stdin, + capture_output=True, + text=True, + timeout=10, + cwd=str(cwd) if cwd else None, + ) + + +# ---- validate subcommand ---- + + +def test_validate_subcommand_clean_config(): + r = _run(["validate", "--config", str(FIXTURES / "basic-config.yml")]) + assert r.returncode == 0, f"stderr={r.stderr}" + assert "[OK]" in r.stdout + + +def test_validate_subcommand_equals_flag_form(): + sub = _run(["validate", "--config", str(FIXTURES / "basic-config.yml")]) + flag = _run(["--validate", "--config", str(FIXTURES / "basic-config.yml")]) + assert sub.returncode == flag.returncode == 0 + assert sub.stdout == flag.stdout + + +def test_validate_subcommand_uses_default_config(tmp_path): + r = _run(["validate"], cwd=tmp_path) + assert r.returncode == 1 + assert ".bully.yml" in r.stderr + + +# ---- doctor subcommand ---- + + +def test_doctor_subcommand_runs(tmp_path): + r = _run(["doctor"], cwd=tmp_path) + # Doctor exits 1 in a directory with no .bully.yml -- we just want the + # subcommand to dispatch to _cmd_doctor (proven by the [FAIL] line). + assert "Python" in r.stdout # First doctor line is the python-version OK line + assert "no .bully.yml" in r.stdout + + +# ---- show-resolved-config subcommand ---- + + +def test_show_resolved_config_subcommand(): + r = _run(["show-resolved-config", "--config", str(FIXTURES / "basic-config.yml")]) + assert r.returncode == 0 + assert "engine=" in r.stdout + assert "scope=" in r.stdout + + +# ---- lint subcommand ---- + + +def test_lint_subcommand_with_explicit_config(): + r = _run( + [ + "lint", + str(FIXTURES / "violation.php"), + "--config", + str(FIXTURES / "basic-config.yml"), + ] + ) + # violation.php has a script-rule violation + assert r.returncode == 2 + + +def test_lint_subcommand_defaults_config_to_dot_bully_yml(tmp_path): + # Drop a config and a target file in tmp_path; run `pipeline.py lint ` + # from that directory. Should pick up ./.bully.yml automatically. + cfg = tmp_path / ".bully.yml" + cfg.write_text( + "schema_version: 1\n" + "rules:\n" + " banned-keyword:\n" + ' description: "no banned-token in source"\n' + " engine: script\n" + ' scope: ["*.py"]\n' + " severity: error\n" + ' script: "grep -nE banned-token {file} && exit 1 || exit 0"\n' + ) + target = tmp_path / "x.py" + target.write_text("print('banned-token here')\n") + + # Trust the config so the trust gate doesn't short-circuit. + trust_env = {"BULLY_TRUST_ALL": "1"} + r = subprocess.run( + [sys.executable, str(PIPELINE), "lint", str(target)], + capture_output=True, + text=True, + timeout=10, + cwd=str(tmp_path), + env={**__import__("os").environ, **trust_env}, + ) + assert r.returncode == 2, f"stdout={r.stdout}\nstderr={r.stderr}" + + +def test_lint_subcommand_passes_through_rule_filter(): + diff = ( + "--- a/violation.php\n+++ b/violation.php\n" + "@@ -10,2 +10,3 @@\n" + "+ $x = User::query()->get();\n" + "+ return $x;\n" + ) + r = _run( + [ + "lint", + str(FIXTURES / "violation.php"), + "--config", + str(FIXTURES / "basic-config.yml"), + "--rule", + "inline-single-use-vars", + "--diff", + diff, + ] + ) + assert r.returncode == 0 + payload = json.loads(r.stdout) + assert payload["status"] == "evaluate" + + +# ---- legacy paths preserved ---- + + +def test_legacy_flag_form_still_works(): + r = _run(["--validate", "--config", str(FIXTURES / "basic-config.yml")]) + assert r.returncode == 0 + + +def test_legacy_positional_form_still_works(): + r = _run([str(FIXTURES / "basic-config.yml"), str(FIXTURES / "violation.php")]) + assert r.returncode == 2 + + +def test_unknown_first_token_treated_as_positional_config(): + # Ensures we don't accidentally swallow paths that look like subcommand verbs. + # `nonexistent.yml` is a bare filename and must be passed through to the legacy + # positional handling, which then fails with the no-config-found pass. + r = _run(["/nonexistent/config.yml", "some/file.php"]) + assert r.returncode == 0 # pipeline returns pass when config missing diff --git a/pipeline/tests/test_explain.py b/pipeline/tests/test_explain.py new file mode 100644 index 0000000..7130ac1 --- /dev/null +++ b/pipeline/tests/test_explain.py @@ -0,0 +1,174 @@ +"""Tests for TASK-1.3: surfacing semantic skip reasons via --explain. + +Authors today have no way to tell whether a semantic rule "didn't fire because +it passed" vs "didn't fire because the can't-match heuristics dropped it." +This test suite covers the two-part fix: + +1. `run_pipeline(..., include_skipped=True)` adds `semantic_skipped` and + `rules_evaluated` to the result dict (gated so hook-mode output is + unchanged). +2. The `--explain` CLI flag prints a per-rule verdict line + (`fire` / `pass` / `skipped `) for every rule in scope. +""" + +import json +import subprocess +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from pipeline import run_pipeline # noqa: E402 + +PIPELINE = Path(__file__).resolve().parent.parent / "pipeline.py" +FIXTURES = Path(__file__).parent / "fixtures" + + +# ---- run_pipeline include_skipped flag ---- + + +def _write_semantic_only_config(tmp_path: Path) -> Path: + cfg = tmp_path / ".bully.yml" + cfg.write_text( + "schema_version: 1\n" + "rules:\n" + " prose-rule:\n" + ' description: "no jargon in committed source"\n' + " engine: semantic\n" + ' scope: ["*.py"]\n' + " severity: warning\n" + ) + return cfg + + +def test_run_pipeline_default_omits_semantic_skipped(tmp_path): + cfg = _write_semantic_only_config(tmp_path) + target = tmp_path / "x.py" + target.write_text("print('hi')\n") + + # Empty diff -> can't-match filter drops the semantic rule. + result = run_pipeline(str(cfg), str(target), "") + assert "semantic_skipped" not in result, "default callers must not see new field" + + +def test_run_pipeline_include_skipped_surfaces_reason(tmp_path): + cfg = _write_semantic_only_config(tmp_path) + target = tmp_path / "x.py" + target.write_text("print('hi')\n") + + result = run_pipeline(str(cfg), str(target), "", include_skipped=True) + assert "semantic_skipped" in result + skipped = result["semantic_skipped"] + assert len(skipped) == 1 + assert skipped[0]["rule"] == "prose-rule" + assert skipped[0]["reason"] == "empty-diff" + + +def test_run_pipeline_include_skipped_lists_evaluated_rules(tmp_path): + cfg = _write_semantic_only_config(tmp_path) + target = tmp_path / "x.py" + target.write_text("print('hi')\n") + + result = run_pipeline(str(cfg), str(target), "", include_skipped=True) + assert "rules_evaluated" in result + by_id = {r["rule"]: r for r in result["rules_evaluated"]} + assert "prose-rule" in by_id + assert by_id["prose-rule"]["verdict"] == "skipped" + assert by_id["prose-rule"]["reason"] == "empty-diff" + assert by_id["prose-rule"]["engine"] == "semantic" + + +def test_run_pipeline_include_skipped_records_passing_script(tmp_path): + cfg = tmp_path / ".bully.yml" + cfg.write_text( + "schema_version: 1\n" + "rules:\n" + " banned-token:\n" + ' description: "no banned-token in source"\n' + " engine: script\n" + ' scope: ["*.py"]\n' + " severity: error\n" + ' script: "grep -nE banned-token {file} && exit 1 || exit 0"\n' + ) + target = tmp_path / "x.py" + target.write_text("print('hi')\n") # no banned-token + + result = run_pipeline(str(cfg), str(target), "", include_skipped=True) + by_id = {r["rule"]: r for r in result["rules_evaluated"]} + assert by_id["banned-token"]["verdict"] == "pass" + assert by_id["banned-token"]["engine"] == "script" + + +# ---- --explain CLI ---- + + +def _run_cli(args, cwd: Path | None = None) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(PIPELINE), *args], + capture_output=True, + text=True, + timeout=10, + cwd=str(cwd) if cwd else None, + ) + + +def test_explain_flag_outputs_per_rule_verdict(tmp_path): + cfg = _write_semantic_only_config(tmp_path) + target = tmp_path / "x.py" + target.write_text("print('hi')\n") + + r = _run_cli(["lint", str(target), "--config", str(cfg), "--explain"]) + assert r.returncode == 0, f"stderr={r.stderr}" + assert "prose-rule" in r.stdout + assert "skipped" in r.stdout + assert "empty-diff" in r.stdout + + +def test_explain_flag_shows_pass_verdict_for_clean_script_rule(tmp_path): + cfg = tmp_path / ".bully.yml" + cfg.write_text( + "schema_version: 1\n" + "rules:\n" + " banned-token:\n" + ' description: "no banned-token in source"\n' + " engine: script\n" + ' scope: ["*.py"]\n' + " severity: error\n" + ' script: "grep -nE banned-token {file} && exit 1 || exit 0"\n' + ) + target = tmp_path / "x.py" + target.write_text("print('hi')\n") + + r = _run_cli(["lint", str(target), "--config", str(cfg), "--explain"]) + assert r.returncode == 0 + assert "banned-token" in r.stdout + assert "pass" in r.stdout + + +# ---- hook-mode output unchanged (no regressions) ---- + + +def test_hook_mode_output_does_not_include_semantic_skipped(tmp_path): + cfg = _write_semantic_only_config(tmp_path) + target = tmp_path / "x.py" + target.write_text("print('hi')\n") + payload = { + "tool_name": "Edit", + "tool_input": { + "file_path": str(target), + "old_string": "", + "new_string": "print('hi')\n", + }, + } + r = subprocess.run( + [sys.executable, str(PIPELINE), "--hook-mode"], + input=json.dumps(payload), + capture_output=True, + text=True, + timeout=10, + cwd=str(tmp_path), + ) + # Hook-mode output is plain text on stderr (or empty); semantic_skipped + # must not appear in either stream regardless of how the rule resolved. + assert "semantic_skipped" not in r.stdout + assert "semantic_skipped" not in r.stderr diff --git a/pipeline/tests/test_skip_config.py b/pipeline/tests/test_skip_config.py new file mode 100644 index 0000000..3e2d455 --- /dev/null +++ b/pipeline/tests/test_skip_config.py @@ -0,0 +1,163 @@ +"""Tests for TASK-1.1: project-level `skip:` and ~/.bully-ignore. + +The pipeline ships with a built-in `SKIP_PATTERNS` tuple covering common +auto-generated paths (lockfiles, minified bundles, dist/build dirs). This +test suite covers two ways to extend it without patching the source: + +1. A top-level `skip:` key in `.bully.yml` (inline list or block list). +2. A user-global `~/.bully-ignore` file with one glob per line. + +Both must merge with the built-ins; never replace them. +""" + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from pipeline import ( # noqa: E402 + SKIP_PATTERNS, + ConfigError, + _path_matches_skip, + effective_skip_patterns, + parse_config, + run_pipeline, +) + +RULE_BLOCK = ( + "rules:\n" + " always-fail:\n" + ' description: "any file fails this"\n' + " engine: script\n" + ' scope: "*"\n' + " severity: error\n" + ' script: "exit 1"\n' +) + + +# ---- parser ---- + + +def test_parse_skip_inline_list(tmp_path): + cfg = tmp_path / ".bully.yml" + cfg.write_text('skip: ["_build/**", "vendor/**"]\n' + RULE_BLOCK) + parse_config(str(cfg)) # must not raise + patterns = effective_skip_patterns(str(cfg), include_user_global=False) + assert "_build/**" in patterns + assert "vendor/**" in patterns + + +def test_parse_skip_block_list(tmp_path): + cfg = tmp_path / ".bully.yml" + cfg.write_text("skip:\n - _build/**\n - target/**\n" + RULE_BLOCK) + parse_config(str(cfg)) # must not raise + patterns = effective_skip_patterns(str(cfg), include_user_global=False) + assert "_build/**" in patterns + assert "target/**" in patterns + + +def test_skip_inherits_built_in_defaults(tmp_path): + cfg = tmp_path / ".bully.yml" + cfg.write_text('skip: ["custom/**"]\n' + RULE_BLOCK) + patterns = effective_skip_patterns(str(cfg), include_user_global=False) + # Project skip merged with the built-ins, not replaced. + for built_in in SKIP_PATTERNS: + assert built_in in patterns + assert "custom/**" in patterns + + +def test_skip_malformed_value_raises(tmp_path): + cfg = tmp_path / ".bully.yml" + cfg.write_text("skip: not-a-list\n" + RULE_BLOCK) + try: + parse_config(str(cfg)) + except ConfigError as e: + assert "skip" in str(e).lower() + assert "list" in str(e).lower() + else: + raise AssertionError("expected ConfigError for non-list skip value") + + +# ---- user-global ~/.bully-ignore ---- + + +def test_user_global_skips_loaded_when_present(tmp_path, monkeypatch): + cfg = tmp_path / ".bully.yml" + cfg.write_text(RULE_BLOCK) + + fake_home = tmp_path / "home" + fake_home.mkdir() + (fake_home / ".bully-ignore").write_text( + "# this is a comment\n" + "\n" + "node_modules/**\n" + "target/**\n" + ) + monkeypatch.setenv("HOME", str(fake_home)) + + patterns = effective_skip_patterns(str(cfg)) + assert "node_modules/**" in patterns + assert "target/**" in patterns + + +def test_user_global_skips_silent_when_absent(tmp_path, monkeypatch): + cfg = tmp_path / ".bully.yml" + cfg.write_text(RULE_BLOCK) + + fake_home = tmp_path / "home" + fake_home.mkdir() + monkeypatch.setenv("HOME", str(fake_home)) + + patterns = effective_skip_patterns(str(cfg)) + # Built-ins still present; nothing extra added. + for built_in in SKIP_PATTERNS: + assert built_in in patterns + + +# ---- no-config-yet behavior preserved ---- + + +def test_existing_configs_without_skip_unchanged(tmp_path): + cfg = tmp_path / ".bully.yml" + cfg.write_text(RULE_BLOCK) # no skip: key + patterns = effective_skip_patterns(str(cfg), include_user_global=False) + assert tuple(patterns) == SKIP_PATTERNS + + +# ---- _path_matches_skip with extra patterns ---- + + +def test_path_matches_skip_extra_patterns(): + assert not _path_matches_skip("_build/x.html") # not in built-ins + assert _path_matches_skip("_build/x.html", extra_patterns=("_build/**",)) + + +# ---- end-to-end: project skip suppresses rule firing ---- + + +def test_run_pipeline_skips_via_project_skip(tmp_path): + cfg = tmp_path / ".bully.yml" + cfg.write_text('skip: ["_build/**"]\n' + RULE_BLOCK) + target = tmp_path / "_build" / "out.html" + target.parent.mkdir() + target.write_text("\n") + + result = run_pipeline(str(cfg), str(target), "") + assert result["status"] == "skipped" + assert result["reason"] == "auto-generated" + + +def test_run_pipeline_skips_via_user_global(tmp_path, monkeypatch): + cfg = tmp_path / ".bully.yml" + cfg.write_text(RULE_BLOCK) + target = tmp_path / "node_modules" / "x.js" + target.parent.mkdir() + target.write_text("// vendor code\n") + + fake_home = tmp_path / "home" + fake_home.mkdir() + (fake_home / ".bully-ignore").write_text("node_modules/**\n") + monkeypatch.setenv("HOME", str(fake_home)) + + result = run_pipeline(str(cfg), str(target), "") + assert result["status"] == "skipped" diff --git a/pipeline/tests/test_trust_gate.py b/pipeline/tests/test_trust_gate.py new file mode 100644 index 0000000..222be3e --- /dev/null +++ b/pipeline/tests/test_trust_gate.py @@ -0,0 +1,249 @@ +"""Tests for the .bully.yml trust boundary gate. + +These tests override the global conftest.py behavior that sets +BULLY_TRUST_ALL=1, because the whole point is to exercise the gate itself. +""" + +import json +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import pipeline as pipeline_mod +from pipeline import ( + _cmd_trust, + _config_checksum, + _trust_status, + run_pipeline, +) + + +def _write_config(tmp_path: Path, body: str = None) -> Path: + p = tmp_path / ".bully.yml" + p.write_text( + body + or ( + "schema_version: 1\n" + "rules:\n" + " tiny:\n" + " description: match nothing\n" + " engine: script\n" + ' scope: "*.txt"\n' + " severity: warning\n" + ' script: "true"\n' + ) + ) + return p + + +@pytest.fixture +def isolated_trust_store(tmp_path, monkeypatch): + """Redirect the trust store to a test-local file and drop BULLY_TRUST_ALL.""" + store = tmp_path / "bully-trust.json" + monkeypatch.setenv("BULLY_TRUST_STORE", str(store)) + monkeypatch.delenv("BULLY_TRUST_ALL", raising=False) + return store + + +# ---- _trust_status ------------------------------------------------------ + + +def test_status_untrusted_when_never_allowed(tmp_path, isolated_trust_store): + cfg = _write_config(tmp_path) + status, detail = _trust_status(str(cfg)) + assert status == "untrusted" + assert "never" in detail + + +def test_status_trusted_after_cmd_trust(tmp_path, isolated_trust_store, capsys): + cfg = _write_config(tmp_path) + _cmd_trust(str(cfg), refresh=False) + status, _ = _trust_status(str(cfg)) + assert status == "trusted" + + +def test_status_mismatch_when_config_changes(tmp_path, isolated_trust_store): + cfg = _write_config(tmp_path) + _cmd_trust(str(cfg), refresh=False) + cfg.write_text(cfg.read_text() + "# mutated\n") + status, detail = _trust_status(str(cfg)) + assert status == "mismatch" + assert "checksum changed" in detail + + +def test_status_trusted_via_env_override(tmp_path, monkeypatch, isolated_trust_store): + cfg = _write_config(tmp_path) + # Even without a store entry, the env override grants trust. + monkeypatch.setenv("BULLY_TRUST_ALL", "1") + status, detail = _trust_status(str(cfg)) + assert status == "trusted" + assert "BULLY_TRUST_ALL" in detail + + +def test_env_override_value_must_be_one(tmp_path, monkeypatch, isolated_trust_store): + cfg = _write_config(tmp_path) + # Any value other than the literal "1" is not a bypass. + monkeypatch.setenv("BULLY_TRUST_ALL", "true") + status, _ = _trust_status(str(cfg)) + assert status == "untrusted" + + +# ---- checksum ------------------------------------------------------------ + + +def test_checksum_changes_when_contents_change(tmp_path): + cfg = _write_config(tmp_path) + first = _config_checksum(str(cfg)) + cfg.write_text(cfg.read_text() + "# mutated\n") + second = _config_checksum(str(cfg)) + assert first != second + assert len(first) == 64 # SHA256 hex + + +def test_checksum_returns_empty_for_missing_file(tmp_path): + assert _config_checksum(str(tmp_path / "does-not-exist.yml")) == "" + + +def test_checksum_includes_extends_targets(tmp_path): + base = tmp_path / "base.yml" + base.write_text( + "schema_version: 1\n" + "rules:\n" + " base-rule:\n" + " description: base\n" + " engine: script\n" + ' scope: "*.txt"\n' + " severity: warning\n" + ' script: "true"\n' + ) + child = tmp_path / ".bully.yml" + child.write_text(f'schema_version: 1\nextends: ["{base.as_posix()}"]\nrules: {{}}\n') + before = _config_checksum(str(child)) + # Mutate the base: the child's own bytes are unchanged, but the + # composite checksum must drift because the extends target changed. + base.write_text(base.read_text() + "# tweak\n") + after = _config_checksum(str(child)) + assert before != after + + +# ---- _cmd_trust --------------------------------------------------------- + + +def test_cmd_trust_writes_store_entry(tmp_path, isolated_trust_store, capsys): + cfg = _write_config(tmp_path) + rc = _cmd_trust(str(cfg), refresh=False) + assert rc == 0 + data = json.loads(isolated_trust_store.read_text()) + entry = data["allowed"][str(cfg.resolve())] + assert len(entry["checksum"]) == 64 + assert entry["allowed_at"].endswith("Z") + captured = capsys.readouterr() + assert "trusted" in captured.out + + +def test_cmd_trust_idempotent_without_refresh(tmp_path, isolated_trust_store, capsys): + cfg = _write_config(tmp_path) + _cmd_trust(str(cfg), refresh=False) + capsys.readouterr() # drain first call output + rc = _cmd_trust(str(cfg), refresh=False) + assert rc == 0 + assert "already trusted" in capsys.readouterr().out + + +def test_cmd_trust_refresh_updates_checksum(tmp_path, isolated_trust_store, capsys): + cfg = _write_config(tmp_path) + _cmd_trust(str(cfg), refresh=False) + first = json.loads(isolated_trust_store.read_text()) + first_sum = first["allowed"][str(cfg.resolve())]["checksum"] + + cfg.write_text(cfg.read_text() + "# mutated\n") + _cmd_trust(str(cfg), refresh=True) + second = json.loads(isolated_trust_store.read_text()) + second_sum = second["allowed"][str(cfg.resolve())]["checksum"] + + assert first_sum != second_sum + + +def test_cmd_trust_missing_config_errors(tmp_path, isolated_trust_store, capsys): + rc = _cmd_trust(str(tmp_path / "nope.yml"), refresh=False) + assert rc == 1 + assert "not found" in capsys.readouterr().err + + +# ---- run_pipeline gate --------------------------------------------------- + + +def test_pipeline_returns_untrusted_status(tmp_path, isolated_trust_store): + cfg = _write_config(tmp_path) + target = tmp_path / "a.txt" + target.write_text("hello\n") + + result = run_pipeline(str(cfg), str(target), "") + assert result["status"] == "untrusted" + assert result["trust_status"] == "untrusted" + assert str(cfg.resolve()) in result["config"] + + +def test_pipeline_runs_after_trust(tmp_path, isolated_trust_store): + cfg = _write_config(tmp_path) + target = tmp_path / "a.txt" + target.write_text("hello\n") + + _cmd_trust(str(cfg), refresh=False) + result = run_pipeline(str(cfg), str(target), "") + assert result["status"] in ("pass",) + + +def test_pipeline_blocks_after_config_mutation(tmp_path, isolated_trust_store): + cfg = _write_config(tmp_path) + target = tmp_path / "a.txt" + target.write_text("hello\n") + + _cmd_trust(str(cfg), refresh=False) + cfg.write_text(cfg.read_text() + "# tweak\n") + result = run_pipeline(str(cfg), str(target), "") + assert result["status"] == "untrusted" + assert result["trust_status"] == "mismatch" + + +def test_pipeline_env_override_skips_gate(tmp_path, monkeypatch, isolated_trust_store): + cfg = _write_config(tmp_path) + target = tmp_path / "a.txt" + target.write_text("hello\n") + + monkeypatch.setenv("BULLY_TRUST_ALL", "1") + result = run_pipeline(str(cfg), str(target), "") + assert result["status"] != "untrusted" + + +# ---- hook_mode integration ---------------------------------------------- + + +def test_hook_mode_untrusted_writes_stderr_and_exits_zero( + tmp_path, monkeypatch, isolated_trust_store, capsys +): + cfg = _write_config(tmp_path) + target = tmp_path / "a.txt" + target.write_text("hello\n") + + payload = { + "tool_name": "Edit", + "tool_input": { + "file_path": str(target), + "old_string": "hello", + "new_string": "hello2", + }, + } + monkeypatch.setattr(pipeline_mod, "_read_stdin_payload", lambda: payload) + monkeypatch.setattr(pipeline_mod, "_find_config_upward", lambda _p: cfg) + + rc = pipeline_mod._hook_mode() + captured = capsys.readouterr() + assert rc == 0 + assert "not trusted" in captured.err + assert "bully trust" in captured.err + # Hook-mode must not emit a semantic evaluation payload on stdout. + assert captured.out.strip() == "" diff --git a/pyproject.toml b/pyproject.toml index 437d74f..dca48bb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -20,14 +20,15 @@ dev = [ "hypothesis>=6.100", ] +[project.scripts] +bully = "pipeline.pipeline:main" + [tool.setuptools] -# This repo is a flat layout of runnable scripts (pipeline/pipeline.py, -# pipeline/analyzer.py, pipeline/hook.sh). `pip install -e ".[dev]"` -# installs dev tooling (ruff, shellcheck-py, pytest, pre-commit) without -# pretending to be an importable package. Run scripts directly: -# python3 pipeline/pipeline.py ... -# python3 pipeline/analyzer.py ... -packages = [] +# `pip install -e .` makes the `pipeline` package importable so the +# `bully` console script can resolve `pipeline.pipeline:main`. The hook +# (`pipeline/hook.sh`) still invokes `pipeline.py` by absolute path -- a +# console script may not be on $PATH inside the Claude Code hook env. +packages = ["pipeline"] [tool.ruff] line-length = 100 diff --git a/scripts/dogfood.sh b/scripts/dogfood.sh index 11edab2..62e5dea 100755 --- a/scripts/dogfood.sh +++ b/scripts/dogfood.sh @@ -13,13 +13,35 @@ if [[ ! -f "$CONFIG" ]]; then exit 0 fi +# The repo's own .bully.yml is trusted by definition: the dogfood script runs +# what the repo committed. Skip the machine-local trust gate. +export BULLY_TRUST_ALL=1 + +# Resolve the bully command. Prefer the installed console script (eats our own +# dog food about the new entrypoint); fall back to the in-tree pipeline.py for +# fresh checkouts where `pip install -e .` has not been run yet. +if command -v bully >/dev/null 2>&1; then + BULLY=(bully lint) +else + BULLY=(python3 "$REPO_DIR/pipeline/pipeline.py" --file) +fi + +# Sanity preamble: confirm the config parses cleanly before iterating. Bails +# out fast on a malformed `.bully.yml` instead of failing on every file with +# the same parse error. +if command -v bully >/dev/null 2>&1; then + bully validate --config "$CONFIG" >/dev/null +else + python3 "$REPO_DIR/pipeline/pipeline.py" --validate --config "$CONFIG" >/dev/null +fi + fail=0 err_file=$(mktemp) trap 'rm -f "$err_file"' EXIT # macOS default bash (3.2) lacks mapfile, so stream from find into a loop. while IFS= read -r file; do - if ! python3 pipeline/pipeline.py --config "$CONFIG" --file "$file" >/dev/null 2>"$err_file"; then + if ! "${BULLY[@]}" "$file" --config "$CONFIG" >/dev/null 2>"$err_file"; then echo "-- $file" cat "$err_file" fail=1 diff --git a/skills/bully-author/SKILL.md b/skills/bully-author/SKILL.md index 9b96728..92c5335 100644 --- a/skills/bully-author/SKILL.md +++ b/skills/bully-author/SKILL.md @@ -30,9 +30,20 @@ Not triggered by bootstrap (`bully-init`), audit (`bully-review`), or hook-outpu ## Engine choice - `script` -- greppable: banned names, banned imports, required headers, forbidden strings, formatting shell-outs. +- `ast` -- structural, deterministic: "no `as any` cast", "no empty catch", "no public mutable property", "no `var_dump` call". Uses `ast-grep` to match code structure, so it ignores comments, strings, and formatting. Prefer this over `script` when grep would produce false positives on strings/comments or miss formatting variants. - `semantic` -- judgment-based: "inline single-use vars", "extract complex logic", "prefer contracts over concretes". -If unsure, ask the user. Do not auto-promote semantic to script without confirmation. +**Preferring ast over script.** If the rule is structural (match a call, cast, declaration, or method shape) rather than textual, `ast` is usually cleaner. Only stay with `script` if you need to shell out to an existing linter, read the diff from stdin, or do something grep-native that ast-grep can't express. + +**ast-grep dependency pre-flight.** Before proposing an `engine: ast` rule, probe availability: + +```bash +command -v ast-grep >/dev/null && echo OK || echo MISSING +``` + +If `MISSING`, do not silently draft an `engine: ast` rule. Tell the user: "This rule would work best as `engine: ast`, but ast-grep isn't installed. Either: (a) run `brew install ast-grep` (or `cargo install ast-grep`) and I'll proceed, or (b) I'll fall back to `engine: script` with a grep pattern (with the usual false-positive tradeoffs)." Wait for their choice before drafting. + +If unsure about engine choice, ask the user. Do not auto-promote semantic to script or ast without confirmation. ## Scope globs @@ -60,19 +71,34 @@ Never write a rule to `.bully.yml` without running this protocol first. 4. Run the pipeline with `--rule` against each fixture: ```bash # Script rule -- violating must exit 2, clean must exit 0 - python3 pipeline/pipeline.py \ + bully lint /tmp/bully-probe-violating. \ --config /tmp/bully-draft.yml \ - --file /tmp/bully-probe-violating. \ --rule - python3 pipeline/pipeline.py \ + bully lint /tmp/bully-probe-clean. \ --config /tmp/bully-draft.yml \ - --file /tmp/bully-probe-clean. \ --rule ``` 5. For **semantic rules**, use `--print-prompt` instead of asserting exit codes. Read the rendered prompt and confirm it would correctly judge both fixtures. If unclear, sharpen the description and re-test. -6. Only on pass, proceed to the write step. -7. Clean up: `rm -f /tmp/bully-probe-*.* /tmp/bully-draft.yml`. + + Then run `--explain` against the violating fixture to confirm the rule is actually being dispatched, not silently dropped by the can't-match heuristics: + + ```bash + bully lint /tmp/bully-probe-violating. \ + --config /tmp/bully-draft.yml \ + --rule \ + --explain + ``` + + The line for `` must show `dispatched`, not `skipped (empty-diff)` or `skipped (too-few-added-lines)`. If skipped, add lines to the fixture or supply a `--diff` that has more added lines. +6. For **ast rules**, the same exit-code protocol as script rules: violating must exit 2, clean must exit 0. Additionally verify the pattern directly with ast-grep before writing to the draft: + ```bash + ast-grep run --pattern '' --lang /tmp/bully-probe-violating. + ast-grep run --pattern '' --lang /tmp/bully-probe-clean. + ``` + The first invocation must print at least one match; the second must print nothing. +7. Only on pass, proceed to the write step. +8. Clean up: `rm -f /tmp/bully-probe-*.* /tmp/bully-draft.yml`. Invariants: fixtures exist before testing; both violating and compliant fixtures (or `--print-prompt`) are exercised; the draft config is used, not the real one; exit codes match expectations before writing. @@ -83,10 +109,12 @@ The parser is fixed-indent. Do not reformat the file. ```yaml rule-id: # 2-space indent, trailing colon description: … # 4-space indent - engine: script | semantic + engine: script | semantic | ast scope: "*.ext" # or ["*.a", "*.b"] severity: warning | error script: "…{file}… && exit 1 || exit 0" # script rules only + pattern: "$EXPR as any" # ast rules only + language: ts # ast rules only (optional; inferred from scope) ``` - 2-space indent for rule ids, 4-space for fields, 6+ for folded scalar continuations. @@ -97,13 +125,13 @@ The parser is fixed-indent. Do not reformat the file. ## Adding a new rule -1. Classify (script vs semantic). -2. Collect `id` (kebab-case, unique), `description`, `engine`, `scope`, `severity`, and `script` if applicable. +1. Classify (script vs ast vs semantic). If ast, confirm ast-grep is installed (see pre-flight in "Engine choice" above). +2. Collect `id` (kebab-case, unique), `description`, `engine`, `scope`, `severity`, plus `script` (script rules), `pattern` + optional `language` (ast rules), or no extra field (semantic rules). 3. Run the fixture-testing protocol. 4. Edit `.bully.yml` to append the rule. 5. Sanity-check against 2-3 existing project files: ```bash - python3 pipeline/pipeline.py --config .bully.yml --file --rule + bully lint --rule ``` In this repo, also run `bash scripts/dogfood.sh`. If the rule mass-flags the codebase, narrow it or treat the flags as real cleanup. 6. Report and invite the user to review before committing. @@ -132,7 +160,7 @@ The parser is fixed-indent. Do not reformat the file. ```bash bash scripts/dogfood.sh # or - python3 pipeline/pipeline.py --config .bully.yml --file + bully lint ``` ## Applying review recommendations @@ -146,7 +174,8 @@ Apply one recommendation at a time. Test each before moving on. Never batch. | Dead rule, scope wrong | Broaden the scope; if still dead, propose removal. | | Dead rule, obsolete | Remove. | | Slow rule | Demote to `warning` or move to CI. | -| Semantic rule with stable mechanical fix | Draft an equivalent script rule, test, layer it alongside -- do not replace. | +| Semantic rule with stable mechanical fix | Draft an equivalent script or ast rule, test, layer it alongside -- do not replace. | +| Script rule noisy due to string/comment false positives | Convert to `engine: ast` with a structural `pattern:`. Verify ast-grep is installed first. | ## Troubleshooting diff --git a/skills/bully/SKILL.md b/skills/bully/SKILL.md index ce8b7ee..f222650 100644 --- a/skills/bully/SKILL.md +++ b/skills/bully/SKILL.md @@ -81,10 +81,10 @@ For each entry in `VIOLATIONS:`, look up severity in the original `evaluate` arr After parsing VIOLATIONS / NO_VIOLATIONS (whether from the subagent or from inline eval), record each rule's verdict. For every rule id in the original `evaluate` array, invoke the Bash tool once with: ``` -python3 $HOME/.bully/pipeline/pipeline.py --log-verdict --rule --verdict --file +bully --log-verdict --rule --verdict --file ``` -Use `violation` if the rule appears in VIOLATIONS, `pass` if it appears in NO_VIOLATIONS. This is a no-op when telemetry is disabled, so always invoke. Adjust the path if the pipeline is installed elsewhere. +Use `violation` if the rule appears in VIOLATIONS, `pass` if it appears in NO_VIOLATIONS. This is a no-op when telemetry is disabled, so always invoke. If `bully` is not on `$PATH`, fall back to invoking the pipeline directly: `python3 "$(ls -d ~/.claude/plugins/cache/*/bully/*/ 2>/dev/null | tail -1)pipeline/pipeline.py"` for plugin installs or `python3 ~/.bully/pipeline/pipeline.py` for the manual install. ## passed_checks From 6f6684319bcd6e43d32b26709e82d2acac796c8f Mon Sep 17 00:00:00 2001 From: Chris Arter Date: Fri, 17 Apr 2026 12:04:23 -0400 Subject: [PATCH 2/2] Fix ruff F841 + format on P1 epic test files CI surfaced an unused `cfg` binding in test_hook_mode_output_does_not_include_semantic_skipped (the helper writes the file as a side effect; the variable wasn't read). Also pick up ruff format's reflow on pipeline.py and test_skip_config.py. Co-Authored-By: Claude Opus 4.7 (1M context) --- pipeline/pipeline.py | 4 ++-- pipeline/tests/test_explain.py | 2 +- pipeline/tests/test_skip_config.py | 7 +------ 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/pipeline/pipeline.py b/pipeline/pipeline.py index 38ce817..f52e3c0 100644 --- a/pipeline/pipeline.py +++ b/pipeline/pipeline.py @@ -1455,7 +1455,7 @@ def run_pipeline( log_path = _telemetry_path(config_path) # Short-circuit auto-generated files (built-in + user-global + project skip). - extra_skip = effective_skip_patterns(config_path)[len(SKIP_PATTERNS):] + extra_skip = effective_skip_patterns(config_path)[len(SKIP_PATTERNS) :] if _path_matches_skip(file_path, extra_patterns=extra_skip): elapsed_ms = int((time.perf_counter() - start) * 1000) result = {"status": "skipped", "file": file_path, "reason": "auto-generated"} @@ -2094,7 +2094,7 @@ def _cmd_baseline_init(config_path: str | None, glob: str | None) -> int: root = cfg_abs.parent if not glob: glob = "**/*" - extra_skip = effective_skip_patterns(str(cfg_abs))[len(SKIP_PATTERNS):] + extra_skip = effective_skip_patterns(str(cfg_abs))[len(SKIP_PATTERNS) :] entries: list[dict] = [] for candidate in root.glob(glob): if not candidate.is_file(): diff --git a/pipeline/tests/test_explain.py b/pipeline/tests/test_explain.py index 7130ac1..91251a4 100644 --- a/pipeline/tests/test_explain.py +++ b/pipeline/tests/test_explain.py @@ -149,7 +149,7 @@ def test_explain_flag_shows_pass_verdict_for_clean_script_rule(tmp_path): def test_hook_mode_output_does_not_include_semantic_skipped(tmp_path): - cfg = _write_semantic_only_config(tmp_path) + _write_semantic_only_config(tmp_path) target = tmp_path / "x.py" target.write_text("print('hi')\n") payload = { diff --git a/pipeline/tests/test_skip_config.py b/pipeline/tests/test_skip_config.py index 3e2d455..bad0153 100644 --- a/pipeline/tests/test_skip_config.py +++ b/pipeline/tests/test_skip_config.py @@ -87,12 +87,7 @@ def test_user_global_skips_loaded_when_present(tmp_path, monkeypatch): fake_home = tmp_path / "home" fake_home.mkdir() - (fake_home / ".bully-ignore").write_text( - "# this is a comment\n" - "\n" - "node_modules/**\n" - "target/**\n" - ) + (fake_home / ".bully-ignore").write_text("# this is a comment\n\nnode_modules/**\ntarget/**\n") monkeypatch.setenv("HOME", str(fake_home)) patterns = effective_skip_patterns(str(cfg))