skillsaw / rules

---
description: You must follow these instructions when developing skillsaw.
---

Development rules for skillsaw.

## Pre-push Checklist

<!-- skillsaw-disable-next-line content-hook-candidate -->
CRITICAL: You MUST always run these steps before pushing changes.

1. Ensure branch is up to date with UPSTREAM (stbenjam/skillsaw) — check git
   remotes then merge upstream's main.
2. Run `make test` — run the full test suite and ensure all tests pass. Never
   assume a problem is pre-existing; follow the boyscout rule and fix it anyway.
3. Run `make lint` — check formatting (or run `make format` to fix it).
4. Run `make update` — regenerate all generated files (must come after version bump).
5. Test against `openshift-eng/ai-helpers`: clone it, run `skillsaw`, and ensure exit 0.

## Pre-PR Checklist

Follow these checks before opening a PR:

- Check whether documentation is up to date for the changes on this branch.
    - Example: you add a new feature, flag, or lint type.
      Action: update `README.md` to include this.
    - Example: you encounter a problem during development you must keep for later.
      Action: update `.apm/instructions` and run `make update`.
- Verify test coverage on a new feature, flag, lint type or bug fix is complete.
    - Bug fixes must include regression protection.
    - New features, linters, and rules need integration test coverage WITH fixtures (see testing rules).

## Authoring Agentic Context

When you create or edit a skill, slash command, agent, hook, or instruction
file in this repo, load the `skillsaw-lint` skill and follow it: lint the
file with `.venv/bin/skillsaw lint <path>`, apply fixes, and re-lint until
clean before you finish. We ship the linter — our own context must pass it.

## Post-PR Checklist

After opening a PR, keep monitoring for feedback from CodeRabbit, Gemini,
and stbenjam. You may stop monitoring 20 minutes after you push a PR.
Review and handle valid feedback that comes in.

## Performance

Benchmark lint speed with the harness in `benchmarks/` (`make
benchmark`, `make profile`, `make benchmark-save` / `make benchmark-compare`
for regression checks — review `DEVELOPMENT.md`). When touching content rules,
the lint tree, or `utils.py` read paths, save a baseline on main and compare on your branch.

- **Avoid scanning every line against every regex pattern.**
  Run `patterns_matching_anywhere(body, patterns)` from `content_analysis`
  first to filter down to patterns that actually match the text, then scan
  line-by-line only with surviving patterns.
- **`lint_tree.find(NodeType)` is memoized per node** — keep it valid: anything
  that mutates tree structure outside a rebuild must call `invalidate_find_cache()`
  (see `FrontmatteredBlock.write_frontmatter_text`).
- **Read top-level frontmatter key lines from `frontmatter_key_line()`**,
  which uses a libyaml-backed line map with a ruamel fallback. Don't add
  per-key ruamel parses. Avoid ruamel round-trip parsing — it is ~30x slower and was the dominant cost of lint-tree construction.
- Keep per-blob work (whole-body `.lower()`, config-file stats) outside
  the per-block loop — run it once per `check()`.
- **Use fast string checks before running heavy regexes.** Simple checks
  like `str.isascii()`, `str.translate()`, or `"/" in text` quickly filter
  out clean text so regexes only run when necessary.
- **Use `paths.relative_to_str()` for repo-relative paths.**
  On Python 3.12+, `PurePath.relative_to` and `path.parents` allocate new
  `Path` objects for every ancestor, which adds significant overhead in large repos.
- **Probes take `resolve=`**; pass `context.resolve_path`, never a module-global cache.
- **Load YAML with `yaml.load(..., Loader=_SAFE_LOADER)` from `utils.py`**
  instead of bare `yaml.safe_load` — `_SAFE_LOADER` uses the fast C-based
  LibYAML loader when available (~10x faster) while maintaining safe loading behavior.
- **`Linter.run()` / `fix()` pause Python's cyclic garbage collector**
  during rule execution to avoid expensive GC pauses across millions of
  objects. Rules should avoid circular references so temporary data is
  freed immediately via reference counting.

## Markdown: AST for reading, splice for writing

Read markdown structure (links, code spans, fences, HTML comments, headings)
from the markdown-it-py AST — read it via `block.markdown` (a
`skillsaw.markdown_doc.MarkdownDoc`). Never hand-roll per-line regexes for markdown structure.

- **Detection**: use the `MarkdownDoc` accessors — `links()`, `code_spans()`,
  `text_segments()`, `fences()`, `headings()`, `html_comments()`,
  `prose_lines()`. Scanning rules needing only prose read it automatically through `read_body(strip_code_blocks=True)`.
- **Fixes**: splice at the token spans the check matched using
  `markdown_doc.splice(content, edits)` — never re-locate fix targets with
  `line.find()` / `str.replace()` — avoid them; they corrupt substrings of other tokens.
- **Never render the AST back to markdown** — round-trip rendering reformats whole files and violates the scope-the-fix autofix rule.
- Use `markdown_doc.file_span()` to translate body-relative columns to file columns before splicing (YAML-embedded bodies are indented in the file);
  skip the edit — return early — when `file_span()` yields `None`.

## New Linter Rules

- **Make rules configurable when there are tuneable settings.**
- **Read declared options with `self.setting()`** so overrides, schema
  defaults, and explicit nulls follow one contract. Declare every literal
  config key the rule reads; `tests/test_rule_registry.py` enforces this for
  builtin rule class bodies.
- **Never break existing rules for users of skillsaw.**
- **Rules register themselves** — never hand-maintain import lists or config
  dicts. skillsaw auto-discovers any concrete `Rule` subclass under
  `src/skillsaw/rules/builtin/`. Keep defaults on the class: `default_enabled`
  and `default_severity()` define the single source of truth, and
  `LinterConfig.default()` is generated from them.
- **New rules default to `enabled: auto` or `enabled: false`**. Set
  `default_enabled = False` for opt-in; the base class default is `"auto"`.
  Never force-enable a new rule that could break existing users.
- **Use the lint tree for discovery** — call `context.lint_tree.find(NodeType)`.

### Making rules helpful by default

Setting `enabled: auto` means a rule provides clear, actionable value on typical repositories without extra setup. Before shipping a rule with `auto` enabled, test it against real-world repositories rather than just unit test fixtures.

Evaluate every new rule against these three practical questions:

1. **Is it actionable on common setups?** Findings should point to things developers genuinely want to fix. If a check only applies to niche or rare configurations, make it opt-in (`default_enabled = False`). Be especially mindful of JSON files, which lack inline disable comments and require configuration tweaks to silence.
2. **Is it accurate?** Test the rule against at least 10 real repositories. Validate edge cases directly against the official schema, loader, or CLI rather than memory. Aim for a false-positive rate under 10% for `auto` rules, and zero known false positives for `ERROR` rules.
3. **Is it high signal?** A rule that fires dozens of times across healthy, working projects quickly becomes noise. When a pattern appears frequently across a repository, aggregate related issues or provide sensible thresholds so the output stays helpful and focused.

**Match severity to impact:**
- **ERROR**: The tool will fail to load or run the file, or the issue presents a real security risk.
- **WARNING**: The file functions, but quality or maintainability is noticeably degraded.
- **INFO**: Gentle suggestions, style guidance, or advisory review prompts.

Always respect the configured severity: never hardcode `severity=` on the primary violation, so user overrides in `.skillsaw.yaml` work seamlessly.

**Keep findings concise and consolidated:**
- **One defect, one finding**: Group related issues together. For example, report invalid list items or a folder of unreferenced files as a single consolidated finding with examples.
- **Recognize common placeholders**: Understand standard templates and sample strings (such as `${VERSION}`, dummy auth tokens, or example endpoints) so helpful documentation isn't flagged as invalid.
- **Clear, actionable messages**: State what happened and how to fix it. Suggest rule configuration options (like exclusions or custom thresholds) before suggesting a disable comment. Focus checks on what files actually do rather than just where they are placed.

### Line numbers and the parse tree

- **Always report line numbers** on every violation traceable to a specific line, except whole-file violations.
- **Use `read_yaml_commented()`** (from `utils.py`) for YAML — never use
  `yaml.safe_load()` or `read_yaml()`. Keep line numbers via the ruamel.yaml
  objects `read_yaml_commented()` returns.
- **Use `commented_key_line(node, key)` / `commented_item_line(node, index)`**
  for 1-based line numbers from ruamel structures.
- **Use `read_toml()`** (from `utils.py`) for TOML. It returns
  `(data, error)`.
- **Never fabricate line numbers** — if a field is missing, omit the line.
- **Declare `repo_types`** to control when `enabled: auto` fires.
- **Declare `config_schema`** when the rule accepts parameters.
- **Always keep EVERYTHING part of the parse tree.**
- **Keep the prose/config split in the block hierarchy** — `ContentBlock`
  subclasses are prose for an agent's context window and get every
  content-quality rule automatically, so never put a config file under one:
  content rules would lint it as instruction text. Structured JSON (settings,
  hooks, MCP) subclasses `JsonConfigBlock`; structured YAML or TOML gets a
  direct `LintTarget` subclass, since that hierarchy is JSON-specific —
  `OpenAIMetadataBlock` and `GrokConfigBlock` are the patterns. One exception,
  `CodexConfigHooksBlock`: a hooks file in another syntax stays a
  `HooksBlock` — the hooks rules iterate that hierarchy and no hooks role
  mixin exists — and declares its syntax on the block (`syntax_name`,
  `mapping_noun`) so no rule branches on it.

Never require line numbers for JSON or TOML: neither parser gives a
position, so both report at file level.

## Autofix invariants

- **Avoid negative lookarounds around backtrackable quantifiers** in patterns
  that feed autofixes. A trailing `(?!\))` after `\.\w{1,10}` never rejects the
  match — the engine backtracks into a *truncated* match and the fix splices a
  corrupted span (issue #321). Match greedily, then check the characters
  adjacent to the full match in code and reject there.
- **Fixes that add a frontmatter field must guard against the key already
  existing** — `check()` may report "missing" for an empty/null value while
  the `key:` line is still present. Use `replace_frontmatter_field()` first
  and fall back to `prepend_frontmatter_fields()`, or the fix prepends a
  duplicate key on every run and never converges (issue #321).
- **Add new SAFE-autofix edge cases** to the `tests/fixtures/autofix/safe-idempotency`
  fixture so `TestSafeAutofixIdempotency` guards them; update `EXPECTED_SAFE_VIOLATIONS` when the fixture grows.
- **Autofix stands down entirely for plugins installed under `.codex/plugins/`**
  — vendor-managed content is diagnostic-only, so every rule's `fix()` silently
  no-ops there, by design. A new rule's fix needs no guard of its own for that
  case, and a "fix didn't apply" report from such a path is expected behavior,
  not a bug.

## Ecosystem provenance

"Which ecosystem owns this plugin directory" is decided in exactly one
place: `RepositoryContext.provenance()`, which returns a cached
`PluginProvenance` record per directory. It lives on
`RepositoryProvenanceMixin` in `repository_provenance.py`, alongside
`in_format_scope`, `is_codex_only_plugin`, and the containment helpers;
`RepositoryContext` mixes that in, so every caller still reaches it as
`RepositoryContext.provenance()`. Never answer an ownership
question with a fresh filesystem probe in a rule or in the tree builder —
two call sites probing independently is how a directory falls between
per-ecosystem attach paths and loses its content silently.

- **Discovery is state-free and lives in `src/skillsaw/discovery/`**
  (`claude.py`, `codex.py`, `agent_plugins.py`, `openclaw.py`, `detect.py`, and
  `excludes.py`): functions take
  a root path and callbacks and return data, holding no caches and importing
  nothing from `context`.
  `RepositoryContext` is the stateful orchestrator — its methods wrap
  the discovery functions with the per-context caches and render the
  provenance verdicts over the evidence they gather.
- **Evidence is filesystem-first and `--type`-invariant.** An override
  changes what discovery walks, not what the author declared, so
  provenance reads markers, contained manifests, and catalog files
  directly (`_codex_catalog_files()`), never discovery output.
- **The lint tree builds plugins in ONE pass** over the union of claimed
  directories. Each directory gets one container; prose (`commands/`,
  `agents/`, `rules/`, README) attaches once with containment; config
  files attach per claiming ecosystem through the contained helpers.
  Never add a second per-ecosystem plugin loop.
- **Format rules gate on provenance declaratively, never with inline
  guards.** A rule that enforces one ecosystem's conventions declares
  `provenance_scope = "claude"` on its class and iterates targets
  through `self.scoped_find(context, NodeType)`; the filtering happens
  in exactly one place (`RepositoryContext.in_format_scope`, reading
  each node's `provenance_dir()`), so a Codex-only directory is exempt
  while dual-manifest and unclaimed directories keep every check.
  `TestProvenanceScopeMechanism` pins which rules declare the scope.
  The mechanism fails open by node type: `LintTarget.provenance_dir()`
  returns `None` unless the node type overrides it. Grep
  `def provenance_dir` for the current set — today `PluginNode`,
  `AgentPluginNode`, and `AgentPluginConfigNode` in `lint_target.py`,
  plus `CommandBlock` and `AgentBlock` in the frontmatter blocks
  module. A scope-declaring rule iterating a node type without the
  override silently gets no filtering, so add the override first.
- **`provenance_scope` is for rules iterating SHARED node types.** A
  rule reading `CommandBlock` or `PluginNode` — types every ecosystem
  populates — needs the declaration to stay out of another ecosystem's
  directories. A rule iterating an ecosystem-exclusive node type
  (`AgentPluginConfigNode`, `CodexPluginConfigNode`) is already scoped
  by its target, and declaring `provenance_scope` there is a bug: under
  a forced `--type` with no filesystem claim the scope filter skips the
  node, so the rule never reports a missing or malformed manifest. That
  is why `agent-plugin-json-valid` and `agent-plugin-mcp-valid` use
  plain `find(AgentPluginConfigNode)`.
- **Conditional strictness is not a skip.** The ecosystem-tightened MCP
  shape checks stay `provenance_scope = None` and gate the tightened
  checks alone on `context.in_codex_only_plugin(path)` — the one
  spelling of "does a Codex-exclusive plugin own this file" — so
  dual-manifest plugins keep their established Claude results
  (`TestDualManifestBackwardCompat` pins this).
- **Hooks get one subclass per host instead of conditional strictness.**
  Each host's hooks file is its own `HooksBlock` subclass — Claude, Codex,
  Muse, Cursor, Grok, Antigravity — chosen from provenance in
  `build_lint_tree`, with a `<host>-hooks-valid` rule iterating it.
  `hooks-dangerous` and `hooks-prohibited` read the shared `HooksBlock`
  base, so a new host needs no changes there. Exception:
  `GrokPluginHooksBlock` has no shape rule — another adapter loads a Grok
  plugin's hooks file, and 1.0.13 shows no observable.

**Ecosystems and editor tools are different problems.** An *ecosystem*
packages and installs content (Claude plugins, Codex, Grok Build, Agent
Plugins, Antigravity, OpenClaw), so it needs provenance: two can claim one directory,
and the format rules must stay out of each other's trees. An *editor tool* (Cursor,
Copilot, Cline, Qwen) reads its own configuration locations, which no other
tool claims, so it needs no provenance machinery at all. Both are
`RepositoryType` members — detection produces one set, and that enum is the
only vocabulary. Pick the recipe that matches; following the ecosystem one
for an editor tool builds machinery that design does not need.

**Adding an editor tool** (Cursor is the worked example): add its directory
name to `AGENT_TOOL_DIR_NAMES` in the `detect.py` discovery module if it reads a
directory rather than a single root file, so one walk finds it anywhere in
the tree; add its skill directory to `CONVENTIONAL_SKILL_DIRS` in the
`discovery` package, plus `NESTED_TOOL_SKILL_DIRS` in `detect.py` when a
package can carry its own `<dir>/skills/` — `CONVENTIONAL_SKILL_DIRS` is
root-anchored and reaches only the repository root's copy; add a
`RepositoryType` member in `repository_types.py`,
list it in `TOOL_REPO_TYPES`, give it a slot in
`RepositoryContext._TYPE_PRIORITY` (`context.py`) or the JSON and SARIF
reports' `repo_type` falls through to `unknown`; key its evidence by that member's value in
`_TOOL_EVIDENCE` (or a `marker()` check) in `tool_types()` — **detection
must agree with attachment**, or the lint tree grows blocks no gated rule
ever looks at; add block classes whose `category` encodes the budget role (`command`
for on-demand prompts, `instruction` for always-on prose, `agent` for
subagents); attach them in the editor-directory loop in `build_lint_tree`.
Structural rules for the tool declare
`repo_types = frozenset({RepositoryType.<TOOL>})` and iterate their own
block type — never `provenance_scope`, which is for shared node types only.
A tool that routes agents or commands by description also needs its type in
`content-description-routing`'s `repo_types` and those block types in that
rule's traversal list — it walks an explicit list, so attaching is not
enough.
Prefer subclassing a shared block
(`VsCodeMcpBlock(McpBlock)`) over editing existing rule files, so the
security rules pick the tool up without a visit.

Four more, from OpenCode (`formats/opencode.py`, `rules/builtin/opencode/`):
check `APM_COMPILED_DIR_TARGETS` for a tool whose directory is also a
compile target (`.opencode` is, resolved on APM's evidence alone); set
`jsonc: ClassVar[bool] = True` on a JSONC host's block, never a
Claude-family one; keep configuration vocabulary — key sets, alias tables —
in `formats/<tool>.py`, not restated in rules; and when a dialect diverges
enough that a shared rule's every check misfires, subclassing isn't
enough — defer to the tool's own, as `mcp-valid-json` does for Agent
Plugins, OpenCode and Grok. Declare that deferral on the block, never as a
branch in the rule: `McpShapeDeferral` carries the gating repository type,
whether the dialect-neutral checks survive, and which rule owns the syntax
failure.

**Adding an ecosystem** (see Codex and Agent Plugins):
put state-free discovery, catalog enumeration, source resolution and install
helpers in
`src/skillsaw/discovery/<ecosystem>.py`; add its evidence probe to
`provenance()` in `repository_provenance.py`, and its
cached, type-gated wrappers in `context.py` or a
`repository_<ecosystem>.py` mixin when its line cap bites;
a `RepositoryType` member per packaging claim, in `SKILL_REPO_TYPES` when
its plugins carry skills — which turns the `agentskill-*` rules on but
discovers none until `discover_skills` and `context._discover_skills` gain
a leg — and in `_TYPE_PRIORITY`, or `repo_type` reads `unknown`; its marker in
`PLUGIN_MARKER_DIR_NAMES` (`discovery/detect.py`) for directory markers;
collect file markers in the same walk (see `openclaw_manifest_files`);
attach its config files in the single plugin pass in `build_lint_tree`
through a contained helper;
teach `in_format_scope` nothing — it already reads the
provenance claim set, so every `provenance_scope`-declaring rule honors
the new claim the moment the evidence probe lands; give the new ecosystem's
format rules `provenance_scope = "<ecosystem>"` only where they iterate a
shared node type — rules that iterate the ecosystem's own node type gate by
node type instead (see the scope rule above);
extend the union in `merge_plugin_dirs` callers if it discovers plugin
directories of its own. Prose needs no work — every claimed directory
already gets it, and no existing rule needs a *scope* visit: scope is
declared per rule class. A rule keying on an explicit `repo_types` list is
the exception.

For Pi discovery, read `.apm/skills/skillsaw-maintenance/references/pi.md`
(relative to the repository root).