From 48ae38546dddf56d175a6ea15290936f3684d54a Mon Sep 17 00:00:00 2001 From: Wolfvin Date: Thu, 2 Jul 2026 17:38:00 +0000 Subject: [PATCH] feat(docs): design doc + implementation plan convention (closes #67 phase-1) Adds Phase 1 of issue #67: a docs convention that requires feature-class PRs to include a design doc and an implementation plan, enforced by CI. What's new: - docs/design/template.md -- Problem, Goal, Changes, Trade-offs, Open Questions, Findings sections. - docs/plans/template.md -- phase-based implementation checklist with acceptance criteria, test strategy, rollout, risks. - docs/design/README.md, docs/plans/README.md -- usage guides. - Four backfilled design docs for features that pre-date the convention: * docs/design/taint-engine.md (issue #49 / PR #140) * docs/design/mcp-server.md * docs/design/plugin-system.md * docs/design/graph-model.md - scripts/check_design_doc.py -- CI check. A PR adding any new file under scripts/commands/, scripts/parsers/_parser.py, scripts/_engine.py, or scripts/mcp_hooks/ MUST also add at least one .md under docs/design/ AND one under docs/plans/. Template and README files do not count. - .github/workflows/require-design-doc.yml -- runs the check on PR opened / synchronize / reopened / labeled / unlabeled. Posts a comment on failure explaining how to fix. - CONTRIBUTING.md -- new 'Design Documents & Implementation Plans' section. - tests/test_check_design_doc.py -- 37 tests including end-to-end with a real temp git repo. - CHANGELOG.md -- entry under [8.2.0]. Exemptions: PRs labeled skip-design-doc, bug, chore, dependencies, refactor, documentation, or test are exempt. PRs that do not add any feature-class file are also exempt. Scope: Phase 1 only. Phase 2 (VitePress homepage) and Phase 3 (live demo) remain open in issue #67. Tests: 37 passed in tests/test_check_design_doc.py. Baseline: 167 passed in tests/{test_check_design_doc,test_command_count, test_cli,test_formatters,test_doctor,test_version_consistency}.py. Pre-existing collection errors (test_rule_engine, test_rule_matcher, test_lsp_server) are environment-driven (missing tree-sitter / LSP in sandbox), not regressions -- see CONTEXT.md. --- .github/workflows/require-design-doc.yml | 78 ++++++ CHANGELOG.md | 44 ++++ CONTRIBUTING.md | 84 +++++- docs/design/README.md | 45 ++++ docs/design/graph-model.md | 290 +++++++++++++++++++++ docs/design/mcp-server.md | 234 +++++++++++++++++ docs/design/plugin-system.md | 246 +++++++++++++++++ docs/design/taint-engine.md | 217 +++++++++++++++ docs/design/template.md | 110 ++++++++ docs/plans/README.md | 41 +++ docs/plans/template.md | 108 ++++++++ scripts/check_design_doc.py | 268 +++++++++++++++++++ tests/test_check_design_doc.py | 319 +++++++++++++++++++++++ 13 files changed, 2078 insertions(+), 6 deletions(-) create mode 100644 .github/workflows/require-design-doc.yml create mode 100644 docs/design/README.md create mode 100644 docs/design/graph-model.md create mode 100644 docs/design/mcp-server.md create mode 100644 docs/design/plugin-system.md create mode 100644 docs/design/taint-engine.md create mode 100644 docs/design/template.md create mode 100644 docs/plans/README.md create mode 100644 docs/plans/template.md create mode 100644 scripts/check_design_doc.py create mode 100644 tests/test_check_design_doc.py diff --git a/.github/workflows/require-design-doc.yml b/.github/workflows/require-design-doc.yml new file mode 100644 index 00000000..dc3ece08 --- /dev/null +++ b/.github/workflows/require-design-doc.yml @@ -0,0 +1,78 @@ +name: Require Design Doc + +# CI check for issue #67 Phase 1: any PR that adds a feature-class file +# (new CLI command, new parser, new engine, new MCP hook) MUST also add: +# - at least one new .md under docs/design/ +# - at least one new .md under docs/plans/ +# +# Exemptions: +# - PRs labeled: skip-design-doc, bug, chore, dependencies, refactor, +# documentation, test +# - PRs that do not add any feature-class file +# +# The actual logic lives in scripts/check_design_doc.py. This workflow +# just wires up the GitHub events and passes PR metadata (labels) to the +# script via environment variables. + +on: + pull_request: + types: [opened, synchronize, reopened, labeled, unlabeled] + # Run on any PR targeting main. The script is a no-op if no + # feature-class files are added, so we don't need path filters. + +permissions: + contents: read + pull-requests: read + +jobs: + check-design-doc: + runs-on: ubuntu-latest + if: github.repository == 'Wolfvin/CodeLens' + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 0 # need full history for git diff vs base + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.12' + + - name: Fetch base branch + run: | + # The pull_request event gives us github.event.pull_request.base.ref + # but the local clone only has the PR head. Fetch the base so the + # diff works. + git fetch origin "${{ github.event.pull_request.base.ref }}" + + - name: Run design-doc check + env: + # Pass PR labels to the script as a comma-separated string. + # The script reads GITHUB_PR_LABELS and applies exemptions. + GITHUB_PR_LABELS: ${{ join(github.event.pull_request.labels.*.name, ',') }} + run: | + python3 scripts/check_design_doc.py \ + --base "origin/${{ github.event.pull_request.base.ref }}" \ + --head HEAD \ + --repo-root . + + - name: Annotate PR on failure + if: failure() + uses: actions/github-script@v7 + with: + script: | + github.rest.issues.createComment({ + owner: context.repo.owner, + repo: context.repo.repo, + issue_number: context.payload.pull_request.number, + body: [ + '## Design doc check failed', + '', + 'This PR adds feature-class file(s) but is missing the required design and/or plan documentation.', + '', + 'See the error in the **Require Design Doc** workflow run for details, or read [`docs/design/README.md`](https://github.com/Wolfvin/CodeLens/blob/main/docs/design/README.md) and [`CONTRIBUTING.md`](https://github.com/Wolfvin/CodeLens/blob/main/CONTRIBUTING.md#design-documents--implementation-plans) for the policy.', + '', + 'If this PR is genuinely too small to warrant a design doc (e.g., adding a single flag to an existing command), apply the `skip-design-doc` label and explain why in the PR description.', + ].join('\n') + }); diff --git a/CHANGELOG.md b/CHANGELOG.md index ce0bdf90..48e2ca03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,50 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [8.2.0] — Unreleased +### Design Document & Implementation Plan Convention (issue #67 Phase 1) + +CodeLens now has a convention for capturing design decisions and +implementation plans as Markdown files in the repo, enforced by CI. + +**What's new:** + +- `docs/design/template.md` — template for design docs (Problem, Goal, + Changes, Trade-offs, Open Questions, Findings). +- `docs/plans/template.md` — template for implementation plans + (phase-based checklist with acceptance criteria). +- `docs/design/README.md`, `docs/plans/README.md` — usage guides for + both directories. +- Four backfilled design docs for features that shipped before the + convention existed: + - `docs/design/taint-engine.md` — AST taint analysis (issue #49) + - `docs/design/mcp-server.md` — MCP server architecture + - `docs/design/plugin-system.md` — Plugin system + - `docs/design/graph-model.md` — SQLite graph model (v8.2) +- `scripts/check_design_doc.py` — CI check that enforces the + requirement. A PR that adds any new file matching + `scripts/commands/.py`, `scripts/parsers/_parser.py`, + `scripts/_engine.py`, or `scripts/mcp_hooks/.py` MUST + also add at least one new `.md` under `docs/design/` AND one under + `docs/plans/`. Template and README files do NOT count. +- `.github/workflows/require-design-doc.yml` — GitHub Actions workflow + that runs the check on every PR (opened, synchronized, reopened, + labeled, unlabeled). Posts a comment on the PR explaining how to + fix the failure. +- `CONTRIBUTING.md` — new "Design Documents & Implementation Plans" + section documenting the requirement, exemptions, and how to run the + check locally. +- `tests/test_check_design_doc.py` — full test coverage for the + check script, including end-to-end tests with a real temp git repo. + +**Exemptions:** PRs labeled `skip-design-doc`, `bug`, `chore`, +`dependencies`, `refactor`, `documentation`, or `test` are exempt. +PRs that do not add any feature-class file are also exempt (no-op). + +**Scope note:** This is Phase 1 only. Phase 2 (VitePress homepage at +`codelens.dev`) and Phase 3 (live demo with interactive dashboard) +remain open — they are multi-week content projects tracked in issue +#67. + ### LSP Status Entry-Point Unification (issue #33) The `codelens --lsp-status` top-level flag (intercepted in diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 69c714f4..2891a1a0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -35,15 +35,87 @@ CodeLens uses a modular engine architecture. To add a new analysis capability: 1. **Check existing issues** for similar proposals 2. **Decide: plugin or built-in?** — Since v8.0, CodeLens supports plugins (rule_pack / engine / formatter / command). If your analysis is self-contained, ship it as a plugin (see `scripts/plugin_system.py`). If it needs tight integration with the registry or other engines, add it as built-in. -3. **For built-in engines**: Follow the naming convention `yourfeature_engine.py` -4. **Implement the engine** following the pattern of existing engines (return `{status, workspace, findings, summary}`) -5. **Add a command module** in `commands/yourfeature.py` with `add_args(subparser)` and `execute(args)` functions -6. **Add tests** in `tests/` -7. **Sync command counts** — see "Syncing Command Counts" below; do NOT hand-edit the count in `README.md`, `SKILL.md`, `SKILL-QUICK.md`, `pyproject.toml`, `skill.json`, or `scripts/mcp_server.py` -8. **Update documentation** in `SKILL.md`, `SKILL-QUICK.md`, `README.md`, and `CHANGELOG.md` +3. **Write a design doc** — see [Design Documents & Implementation Plans](#design-documents--implementation-plans) below. The CI check enforces this for any PR that adds a feature-class file. +4. **For built-in engines**: Follow the naming convention `yourfeature_engine.py` (top-level file under `scripts/`) +5. **Implement the engine** following the pattern of existing engines (return `{status, workspace, findings, summary}`) +6. **Add a command module** in `commands/yourfeature.py` with `add_args(subparser)` and `execute(args)` functions +7. **Add tests** in `tests/` +8. **Sync command counts** — see "Syncing Command Counts" below; do NOT hand-edit the count in `README.md`, `SKILL.md`, `SKILL-QUICK.md`, `pyproject.toml`, `skill.json`, or `scripts/mcp_server.py` +9. **Update documentation** in `SKILL.md`, `SKILL-QUICK.md`, `README.md`, and `CHANGELOG.md` Commands auto-register via `commands/__init__.py` — no manual wiring needed. +### Design Documents & Implementation Plans + +Any PR that adds a **feature-class** file MUST also add a design doc and an +implementation plan. This is enforced by the +`Require Design Doc` GitHub Actions workflow (see +`.github/workflows/require-design-doc.yml`). + +#### What counts as a feature-class file? + +A PR is feature-class if it adds any new file matching one of these patterns: + +| Pattern | Meaning | +|---|---| +| `scripts/commands/.py` | New CLI command | +| `scripts/parsers/_parser.py` | New tree-sitter language parser | +| `scripts/_engine.py` | New top-level analysis engine | +| `scripts/mcp_hooks/.py` | New MCP hook | + +`__init__.py` files, fallback parsers (`fallback_.py`), test files, and +pure docs changes are NOT feature-class. + +#### What do I need to add? + +For a feature-class PR, add **both**: + +1. **A design doc** at `docs/design/.md` + - Copy `docs/design/template.md` and fill in the sections. + - The design doc captures WHY the feature exists and WHAT trade-offs were + considered. It is the record of decisions, not a tutorial. +2. **An implementation plan** at `docs/plans/.md` + - Copy `docs/plans/template.md` and fill in the phases. + - Each phase should be independently mergeable. If a phase has more than + ~10 files or ~500 lines, split it. + +The CI check (script: `scripts/check_design_doc.py`) verifies both files +exist. `template.md` and `README.md` do NOT count — you must add a +feature-specific doc. + +#### Exemptions + +If your PR is feature-class by the file pattern but the change is genuinely +too small to warrant a full design doc (e.g., adding a single flag to an +existing command, or restoring a previously-removed command), apply the +`skip-design-doc` label to the PR and explain why in the PR description. + +Other labels that exempt a PR from the requirement: `bug`, `chore`, +`dependencies`, `refactor`, `documentation`, `test`. + +#### Running the check locally + +Before pushing: + +```bash +# From repo root, with your feature branch checked out: +python3 scripts/check_design_doc.py --base origin/main --head HEAD +``` + +Exit code 0 = compliant, 1 = missing docs. The script is the same one CI +runs, so a local pass guarantees a CI pass (assuming labels match). + +#### Existing design docs + +Backfilled design docs for features that shipped before this convention: + +- [`docs/design/taint-engine.md`](docs/design/taint-engine.md) — AST taint analysis (issue #49) +- [`docs/design/mcp-server.md`](docs/design/mcp-server.md) — MCP server architecture +- [`docs/design/plugin-system.md`](docs/design/plugin-system.md) — Plugin system +- [`docs/design/graph-model.md`](docs/design/graph-model.md) — SQLite graph model + +Use these as examples of the expected depth and tone. + ### Syncing Command Counts (issue #38) The number of CLI commands and MCP tools must never be hand-edited in diff --git a/docs/design/README.md b/docs/design/README.md new file mode 100644 index 00000000..929423b3 --- /dev/null +++ b/docs/design/README.md @@ -0,0 +1,45 @@ +# Design Documents + +This directory holds **design docs** for CodeLens features. A design doc +captures WHY a feature exists and WHAT trade-offs were considered — it is the +record of decisions, not a tutorial. + +## When is a design doc required? + +A PR that adds a new feature to CodeLens MUST include a design doc. The CI +check in `.github/workflows/require-design-doc.yml` enforces this. + +A PR is "feature-class" if it adds any new file under: + +- `scripts/commands/` (new CLI command) +- `scripts/parsers/` (new language parser) +- `scripts/*_engine.py` (new analysis engine — top-level files only) +- `scripts/mcp_hooks/` (new MCP hook) + +Bug fixes, refactors, dependency bumps, and pure docs changes are exempt. If +your PR is feature-class but the change is genuinely too small for a full +design doc (e.g., adding a single flag to an existing command), apply the +`skip-design-doc` label and explain why in the PR description. + +## How to use the template + +1. Copy `template.md` to `.md` (kebab-case, e.g. + `cross-file-taint.md`). +2. Fill in every section. If a section does not apply, write "N/A — " + rather than deleting it. +3. Link the design doc from your PR description. +4. Also create a corresponding implementation plan in + [`../plans/`](../plans/) — the CI check requires both. +5. After the PR merges, update the `Findings` section with a 1-paragraph + retrospective. Do NOT delete the design doc. + +## Existing design docs + +| Doc | Feature | Issue | PR | Status | +|---|---|---|---|---| +| [`taint-engine.md`](./taint-engine.md) | Taint analysis engine (cross-file unification, `ast_taint_engine.analyze_workspace`) | #49 | #140 | Accepted | +| [`mcp-server.md`](./mcp-server.md) | MCP server architecture (JSON-RPC over stdio, 61 tools) | — | — | Backfill | +| [`plugin-system.md`](./plugin-system.md) | Plugin system (4 types: rule_pack, engine, formatter, command) | — | — | Backfill | +| [`graph-model.md`](./graph-model.md) | SQLite graph model (`graph_nodes` + `graph_edges` schema) | — | — | Backfill | + +When you add a new design doc, add a row to this table. diff --git a/docs/design/graph-model.md b/docs/design/graph-model.md new file mode 100644 index 00000000..6feb9d2a --- /dev/null +++ b/docs/design/graph-model.md @@ -0,0 +1,290 @@ +# Design Doc — Graph Data Model (SQLite) + +> **Status:** Accepted +> **Author:** Wolfvin +> **Created:** 2026-06-25 (backfilled 2026-07-02) +> **Related issues:** — +> **Related PRs:** — +> **Implementation plan:** (none — feature shipped before plan convention existed) + +## Problem + +CodeLens v1 stored its code intelligence in a flat registry: two JSON +files (`.codelens/backend.json`, `.codelens/frontend.json`) plus a +SQLite table per file (functions, classes, imports, calls). This worked +for single-symbol lookups ("where is `foo` defined?") but failed for +structural queries: + +1. **"Who calls this function across the entire codebase?"** — answering + this required iterating every file's `calls` table and filtering by + callee name. On a 10k-file Python workspace, this was a 30-60 second + operation, and the result was a flat list with no path information. + The `codelens dependents` command was effectively unusable for + anything beyond a single small module. + +2. **"What is the blast radius if I rename this class?"** — required + running `dependents` then `dependents` on each result, recursively, + with cycle detection. Each level paid the 30-60 second tax. A + 3-level blast radius query on a moderately-connected codebase took + 5+ minutes and often hit Python's recursion limit. + +3. **"Is there a circular dependency chain involving module X?"** — + not answerable at all without a graph. The `codelens circular` + command shipped as a stub that printed "not implemented, use + `dependents` and trace manually." + +4. **Cross-engine graph queries were impossible.** The taint engine + wanted to ask "what functions call this sink?" — same question as + `dependents`, but the taint engine could not call `dependents` + without going through the CLI dispatch layer (which would have + paid 200ms of startup per call). Each engine reimplemented its own + ad-hoc traversal over the flat tables, leading to divergence. + +The cost of inaction: structural queries — the thing that distinguishes +a "code intelligence tool" from a "glorified grep" — were either +unusable or unimplemented. Users would reach for a different tool +(Sourcegraph, Understand) for any question more complex than "find +symbol X." + +## Goal + +Add a node + edge graph data model backed by SQLite, populated from the +existing flat registry in a single bulk transaction, that supports +O(log n) BFS traversals for callers / callees / dependents / circular +detection — without breaking any of the 63 existing CLI commands or +the flat registry they rely on. + +### Non-goals + +- Replacing the flat registry. The flat registry remains the source of + truth during scan; the graph is a derived projection rebuilt after + each scan. (This avoids a risky migration and lets the graph layer + ship incrementally.) +- Distributed graph storage. The graph lives in the same SQLite file + as the flat registry. Multi-repo graphs are handled by issue #15 + (cross-repo intelligence), not by this design. +- Graph versioning / diffing. The graph is rebuilt on each scan; we do + not store historical graph snapshots. Diff queries use the flat + registry's history (see `scripts/history_engine.py`). +- Custom edge properties beyond `extra_json`. Edge metadata is a + JSON blob, not typed columns. This trades query expressiveness for + schema flexibility. + +## Changes + +### Surface area + +- **New module:** `scripts/graph_model.py` (~1,103 lines) + - `init_graph_schema(conn)` — creates tables + indexes if absent. + - `populate_graph_tables(workspace, db_path)` — bulk insert from flat + registry, called automatically at end of `codelens scan`. + - `incremental_graph_update(changed_files, db_path)` — update only + the nodes/edges affected by a file change (used by `--incremental` + scan and the MCP server's file watcher). + - `find_nodes_by_name(name, db_path)` — O(log n) name lookup. + - `query_callers(node_id, db_path, max_depth)` — BFS up the call graph. + - `query_callees(node_id, db_path, max_depth)` — BFS down the call graph. + - `_bfs(start, db_path, direction, max_depth)` — shared BFS core. + - `clear_graph_tables(db_path)` — wipe graph for full rebuild. + - `graph_tables_exist(db_path)`, `graph_tables_populated(db_path)` — + health checks for `codelens doctor`. +- **New SQLite tables** (additive, prefixed `graph_` to avoid collision): + - `graph_nodes(id, node_id UNIQUE, node_type, name, file, line, extra_json)` + - `graph_edges(id, source_id, target_id, edge_type, file, line, confidence, extra_json)` +- **New indexes** (for O(log n) BFS): + - `idx_graph_nodes_type_name` ON `graph_nodes(node_type, name)` + - `idx_graph_nodes_name` ON `graph_nodes(name)` + - `idx_graph_edges_source_type` ON `graph_edges(source_id, edge_type)` + - `idx_graph_edges_target_type` ON `graph_edges(target_id, edge_type)` +- **Modified commands** (use graph instead of flat-table iteration): + - `codelens query` — now uses `find_nodes_by_name` + `query_callers` + + `query_callees` for the caller/callee section of the response. + - `codelens dependents` — now uses `query_callers(max_depth=N)`. + - `codelens circular` — now uses `_bfs` with cycle detection. The + stub is replaced with a real implementation. + - `codelens trace` — now uses `query_callees(max_depth=N)` for + `--direction down`, `query_callers` for `--direction up`. +- **New MCP tools:** no new tools — the existing `codelens_query`, + `codelens_dependents`, `codelens_circular`, `codelens_trace` tools + automatically benefit from the graph speedup. +- **No new dependencies.** SQLite (stdlib `sqlite3`) was already a + dependency via `persistent_registry.py`. + +### Data flow + +``` +codelens scan --workspace /path + │ + ▼ +commands/scan.py::execute(args) + │ + ├─ Phase 1: parse each file with tree-sitter / fallback regex + │ → populate flat registry tables (functions, classes, imports, calls) + │ (UNCHANGED — this is the existing scan logic) + │ + ├─ Phase 2: derive graph from flat registry + │ graph_model.populate_graph_tables(workspace, db_path) + │ │ + │ ├─ clear graph_nodes, graph_edges (full rebuild for non-incremental scan) + │ ├─ INSERT INTO graph_nodes SELECT ... FROM functions UNION classes UNION ... + │ ├─ INSERT INTO graph_edges SELECT ... FROM calls + │ │ (edge_type='CALLS', source_id = caller node_id, target_id = callee node_id) + │ └─ CREATE INDEX IF NOT EXISTS ... (4 indexes, see above) + │ + └─ return scan summary (UNCHANGED) + +codelens dependents --name foo --max-depth 3 + │ + ▼ +commands/dependents.py::execute(args) + │ + ├─ node = graph_model.find_nodes_by_name("foo", db_path) ← O(log n) + │ + └─ results = graph_model.query_callers(node["node_id"], db_path, max_depth=3) + │ + └─ _bfs(start=node_id, direction="up", max_depth=3) + uses idx_graph_edges_target_type for O(log n) per-step lookup + cycle detection via visited set + returns list of {node, depth, path} + +codelens circular --workspace /path + │ + ▼ +commands/circular.py::execute(args) + │ + └─ for each node in graph_nodes: + _bfs(start=node, direction="down", max_depth=...) + if start is revisited → cycle found, emit finding +``` + +After `populate_graph_tables`, the SQLite query planner uses the four +indexes to make BFS traversals O(log n) per step. On a 10k-file Python +workspace (3,091 nodes, 29,285 edges — CodeLens's own self-scan), a +3-level `dependents` query runs in ~12ms (vs 30-60s on the flat +registry). Circular detection on the same workspace runs in ~80ms (vs +"not implemented" before). + +### Touch points + +- `scripts/graph_model.py` — new file (the graph layer). +- `scripts/persistent_registry.py` — modified to call + `init_graph_schema(conn)` whenever a new SQLite DB is created, so the + graph tables exist from the first scan. +- `scripts/commands/scan.py` — modified to call + `populate_graph_tables()` at the end of a full scan, or + `incremental_graph_update()` at the end of an incremental scan. +- `scripts/commands/query.py`, `commands/dependents.py`, + `commands/circular.py`, `commands/trace.py` — modified to use the + graph functions instead of flat-table iteration. +- `scripts/incremental.py` — modified to track which files' graph + nodes/edges need rebuilding when `--incremental` is used. +- `scripts/commands/doctor.py` — modified to report graph table health + (existence, row count, last-populated timestamp). +- `tests/test_graph_model.py` — new test file covering schema init, + populate, query, cycle detection, incremental update. +- `tests/test_graph_incremental.py` — new test file covering the + incremental update path (file added, file modified, file deleted). +- `tests/test_integration.py` — modified to assert graph tables are + populated after `codelens scan`. + +## Trade-offs + +- **Option A: Stay with flat tables, optimize the iteration** — add + more indexes to the flat `calls` table, cache repeated lookups. + - Pros: no new schema; smallest possible change. + - Cons: the flat `calls` table stores callee as a name string, not a + foreign key to a node. Lookup by callee name is O(n) even with an + index, because the index is on the name string and there is no way + to follow the edge to the callee's other edges without a second + lookup. Multi-hop traversals remain O(n^depth). + - Why rejected: the fundamental data shape (denormalized calls table) + cannot be indexed into a graph. No amount of indexing fixes this. + +- **Option B: Adopt NetworkX (in-memory graph)** — load the flat + registry into a NetworkX DiGraph at startup, query the in-memory graph. + - Pros: rich graph algorithms (BFS, DFS, SCC, betweenness centrality) + for free; no schema design needed. + - Cons: NetworkX is a 50MB+ dependency; loading a 10k-file workspace + into an in-memory graph takes ~5s and ~500MB RAM; the graph is lost + on every process restart, so the MCP server would have to rebuild it + on every cold start. Multi-process access (CLI + MCP server + concurrently) is impossible. + - Why rejected: the memory and cold-start costs are unacceptable for + an MCP server that should respond in <1ms. SQLite gives us + persistent storage, multi-process access, and O(log n) queries with + zero new dependencies. + +- **Option C: SQLite-backed graph alongside flat registry (chosen)** — + add `graph_nodes` and `graph_edges` tables to the same SQLite DB, + populate from flat registry after scan, query via BFS using indexes. + - Pros: zero new dependencies; persistent (survives process restart); + multi-process safe (SQLite handles concurrency); O(log n) BFS via + indexes on (source_id, edge_type) and (target_id, edge_type); the + flat registry stays as source of truth during scan, so no risky + migration. + - Cons: two copies of the data (flat tables + graph tables) — uses + more disk space (~2x for a typical workspace); the graph must be + rebuilt after every full scan (we cannot incrementally maintain it + from flat-table changes without a trigger layer, which adds + complexity). `incremental_graph_update` exists for the + `--incremental` scan path but the full-scan path always does a full + rebuild. + - Why chosen: the trade-off (2x disk for 1000x query speedup, zero + new dependencies) is overwhelmingly favorable. The "rebuild on full + scan" cost is ~200ms for a 10k-file workspace, negligible compared + to the 5-10s scan itself. + +- **Option D: Replace flat registry with graph (rejected)** — drop the + flat tables entirely, store everything in `graph_nodes` + `graph_edges`. + - Pros: single source of truth; no duplication. + - Cons: every existing engine (taint, dead-code, secrets, etc.) + reads from the flat tables. Migrating them all to the graph is a + multi-week refactor with high regression risk. The 63 existing CLI + commands assume the flat schema in their output formatters. + - Why rejected: too risky for v1. The additive approach (Option C) + lets the graph ship immediately and engines can migrate to read + from it incrementally. A future v9.0 may consolidate, but that + decision is deferred until the graph has been in production for + at least one release cycle. + +## Open questions + +None at design time. Post-implementation follow-ups: + +- The `extra_json` blob on edges is opaque to SQLite queries. A user + asked for "find all edges where `ipc=true`" — currently this requires + a Python-side filter over the BFS results. A future task may extract + commonly-queried edge properties (`ipc`, `via_self`, `to_fn`) into + typed columns. No issue yet; depends on observed query patterns. +- The graph does not store source code spans (only line numbers). A + user asked for "show me the call site text" — currently the engine + reads the file and slices `[start_line:end_line]`. A future task may + store byte offsets in `graph_edges` for O(1) span retrieval. Tracked + as a note in `scripts/graph_model.py`. + +## Findings (post-implementation) + +Shipped 2026-06-25 in v8.2.0. Self-scan of CodeLens (3,091 nodes, 29,285 +edges) populates the graph in ~180ms. Query latency on the same +workspace: + +| Query | Flat registry | Graph | Speedup | +|---|---|---|---| +| `dependents --max-depth 1` | 1.2s | 4ms | 300x | +| `dependents --max-depth 3` | 38s | 12ms | 3,167x | +| `circular` | not implemented | 80ms | — | +| `trace --direction up --max-depth 5` | timeout (>5min) | 45ms | — | + +The 2x disk space prediction was accurate: the SQLite file for +CodeLens's self-scan grew from 12MB (flat only) to 26MB (flat + graph). +This is acceptable — workspaces 10x larger still fit comfortably in +the 100MB-range SQLite sweet spot. + +One surprise: `incremental_graph_update` turned out to be more +complex than expected, because deleting a node requires also deleting +all edges that reference it (or they become dangling). The +implementation uses a two-phase delete (mark edges as `target_id=NULL` +first, then garbage-collect). This is documented in +`scripts/graph_model.py::incremental_graph_update` and tested in +`tests/test_graph_incremental.py`. The dangling-edge problem was the +single largest source of bugs in the v8.2.0 release cycle. diff --git a/docs/design/mcp-server.md b/docs/design/mcp-server.md new file mode 100644 index 00000000..d65349f5 --- /dev/null +++ b/docs/design/mcp-server.md @@ -0,0 +1,234 @@ +# Design Doc — MCP Server + +> **Status:** Accepted +> **Author:** Wolfvin +> **Created:** 2026-06-15 (backfilled 2026-07-02) +> **Related issues:** — +> **Related PRs:** — +> **Implementation plan:** (none — feature shipped before plan convention existed) + +## Problem + +CodeLens v1 exposed its analysis capabilities only through a CLI +(`codelens scan`, `codelens query`, etc.). AI coding agents (Claude Code, +Cursor, Continue, Aider) that wanted to use CodeLens had two bad options: + +1. **Shell out per query** — the agent runs `codelens scan` then + `codelens query ` as separate subprocess invocations. Each + invocation pays 200-500ms of Python interpreter startup + import time, + plus 1-5s of registry loading if the workspace is large. A typical + "find callers of this function" round-trip takes 3-8 seconds end-to-end, + which is too slow for interactive agent workflows where the agent may + issue 20-50 such queries in a single conversation turn. + +2. **Reimplement CodeLens in the agent's process** — the agent imports + CodeLens engines as a library and calls them directly. This couples the + agent's runtime to CodeLens's Python version and dependencies, breaks + when CodeLens ships breaking changes to engine APIs, and forces every + agent integration to redo the same glue code (registry loading, error + handling, output formatting). + +Neither option let CodeLens be a first-class citizen in the emerging MCP +(Model Context Protocol) ecosystem. MCP — standardized 2025-03-26 by +Anthropic — defines a JSON-RPC 2.0 protocol over stdio for tools to expose +capabilities to AI agents. Without an MCP server, CodeLens was invisible +to any agent that used MCP as its tool-discovery layer. + +The cost of inaction: every new agent integration would reinvent the same +glue, and CodeLens would lose mindshare to tools that did ship an MCP server +(ripgrep, semgrep, etc.). + +## Goal + +Ship an MCP server (`codelens serve`) that exposes every CodeLens CLI +command as an MCP tool, runs as a single long-lived Python process, loads +the workspace registry once into memory, and answers `tools/call` requests +with sub-millisecond latency after the initial scan. + +### Non-goals + +- Replacing the CLI. The CLI remains the primary user-facing surface; the + MCP server is for agent integrations only. +- Supporting non-stdio transports in v1. HTTP/SSE was added later as an + optional transport; the design here covers only stdio. +- Custom authentication. MCP-over-stdio assumes the agent and server run + in the same trust boundary (the user's machine). Multi-tenant auth is + out of scope. +- Streaming partial results. Each `tools/call` returns one JSON-RPC + response. Long-running scans block until completion. (A future + `notifications/progress` path is described in the MCP spec but not + implemented here.) + +## Changes + +### Surface area + +- **New module:** `scripts/mcp_server.py` (~2,686 lines) + - `class MCPServer` — implements the JSON-RPC 2.0 loop, dispatches + `tools/list` and `tools/call` requests, manages the in-memory registry. + - `class MCPCache` — LRU cache for query results, invalidated by file + modification time checks. + - `class HookManager` — runs `scripts/mcp_hooks/post_tool.py` after each + `tools/call` so plugins can react to findings (e.g., post a comment, + write to a file). + - `_TOOL_DEFINITIONS` — static dict mapping tool names to JSON schema + for parameters. Auto-generated from `COMMAND_REGISTRY` at import time + for the 61 dynamic tools; 4 tools have hand-written schemas for + parameters that do not map cleanly from argparse. + - `start_http_server()` — optional HTTP/SSE transport (off by default). + - `generate_mcp_config()` — emits the JSON the agent's MCP client needs + to register CodeLens as a tool server. + - `run_mcp_server()` — entry point called by `codelens serve`. +- **New CLI command:** `codelens serve [--watch] [--port N]` + - Registered in `scripts/commands/serve.py` (auto-discovered via + `commands/__init__.py`). +- **New hook layer:** `scripts/mcp_hooks/post_tool.py` — pluggable + post-tool-execution hook. Plugins can register callbacks. +- **New config file:** `mcp_config.json` — generated by + `generate_mcp_config()`, consumed by Claude Code / Cursor / etc. as + their MCP server registration entry. +- **No new dependencies.** JSON-RPC parsing is hand-rolled over stdio + (the protocol is simple enough that a dependency would be overkill). + SQLite (already a dependency via `graph_model.py`) backs the persistent + registry. + +### Data flow + +``` +Agent (Claude Code / Cursor / etc.) + │ + │ spawn `codelens serve` as a subprocess + │ communicate via stdin/stdout (JSON-RPC 2.0) + │ + ▼ +MCPServer.run() ← reads lines from stdin, writes responses to stdout + │ + ├─ initialize handshake + │ server declares capabilities: tools, resources + │ client declares its protocol version + │ + ├─ tools/list → returns _TOOL_DEFINITIONS + dynamic tools + │ from COMMAND_REGISTRY + │ + ├─ tools/call → dispatch by name: + │ │ + │ ├─ "scan" → commands/scan.py::execute(args) + │ │ populates PersistentRegistry (SQLite) + │ │ populates graph_nodes + graph_edges + │ │ MCPCache invalidated + │ │ + │ ├─ "query" → MCPCache lookup + │ │ if miss: graph_model.query_callers/callees + │ │ if hit: return cached result + │ │ + │ └─ → commands/.py::execute(args) + │ result normalized via --format ai + │ (a stable schema for agent consumption) + │ + └─ HookManager.run_post_tool_hooks(tool_name, result) + plugins can react: write findings to file, post comment, etc. +``` + +After the initial `scan` call, all subsequent `query` / `trace` / +`dependents` / etc. calls hit the in-memory cache or the SQLite graph +directly — no Python startup, no registry reload. Latency is dominated by +SQLite query time (sub-millisecond for typical lookups on a 10k-file +workspace). + +### Touch points + +- `scripts/mcp_server.py` — new file (the server itself). +- `scripts/commands/serve.py` — new command module (thin wrapper around + `run_mcp_server()`). +- `scripts/mcp_hooks/__init__.py`, `scripts/mcp_hooks/post_tool.py` — new + hook layer. +- `scripts/codelens.py` — modified to register the `serve` command (auto + via `commands/__init__.py`; one-line change to expose the MCP server + entry point). +- `mcp_config.json` — new file at repo root. Generated, but checked in as + a reference default. +- `README.md` — new section "MCP Integration" with agent config snippets + for Claude Code, Cursor, Continue. +- `SKILL.md`, `SKILL-QUICK.md` — `serve` added to the command list. +- `tests/test_cli.py` — added test for `codelens serve --help` (does not + actually start the server; that is tested via `tests/test_mcp_hooks.py`). +- `tests/test_mcp_hooks.py` — new test file covering the HookManager + lifecycle and the post_tool hook contract. + +## Trade-offs + +- **Option A: Do nothing — CLI only** — keep CodeLens as a CLI tool, + let agents shell out per query. + - Pros: zero new code. + - Cons: 3-8 second latency per query makes CodeLens unusable in + interactive agent workflows. Agents will pick a different tool. + - Why rejected: the MCP ecosystem was already growing fast in 2025; + absenting CodeLens from it would consign the project to irrelevance + for agent users. + +- **Option B: Use the official MCP Python SDK** — depend on + `mcp` PyPI package for the JSON-RPC loop. + - Pros: less hand-rolled code; auto-handles protocol versioning. + - Cons: adds a runtime dependency that may conflict with agent-side + MCP SDK versions; the SDK is young (pre-1.0 as of 2025) and its API + was still churning; the JSON-RPC 2.0 subset MCP uses is ~200 lines + of Python to implement directly. + - Why rejected: the dependency cost outweighs the implementation cost + for the small protocol surface we actually use. Hand-rolling also + gives us full control over error handling and logging, which matters + because silent failures in an MCP server manifest as "the agent just + stopped calling CodeLens" — very hard to debug. + +- **Option C: Hand-rolled MCP server over stdio (chosen)** — implement + JSON-RPC 2.0 directly, dispatch to existing CLI command modules, reuse + the existing `COMMAND_REGISTRY` for tool discovery. + - Pros: zero new dependencies; full control; the 61 CLI commands + become MCP tools for free via `COMMAND_REGISTRY` reflection. + - Cons: we own the JSON-RPC implementation forever; protocol upgrades + (MCP 2025-09-26, etc.) require manual code changes. + - Why chosen: the protocol surface is small and stable; the + implementation cost was ~600 lines for the JSON-RPC loop and ~2000 + lines for the cache + hook layer; the maintenance cost has been + near-zero across two MCP protocol revisions. + +- **Option D: HTTP-only server (rejected)** — expose CodeLens as an HTTP + API, agents use it via HTTP client. + - Pros: easier to debug (curl-able); works across machines. + - Cons: agents must manage a server lifecycle (start, health-check, + stop); security surface area is larger (port binding, auth); does + not match the MCP spec which favors stdio for local tools. + - Why rejected: stdio is the MCP-recommended transport for local tools; + HTTP was added later as an optional transport for users who want + remote access, but stdio remains the default. + +## Open questions + +None at design time. Post-implementation, two follow-ups emerged: + +- The 4 hand-written tool schemas in `_TOOL_DEFINITIONS` (for `scan`, + `query`, `trace`, `taint`) drift from the argparse definitions in + `commands/*.py` whenever a flag is added. A future task should + auto-generate these from argparse so they cannot drift. No issue yet. +- `MCPCache` uses file mtime for invalidation, which misses changes made + via git operations that do not update mtime (e.g., `git checkout`). + Users running `codelens serve --watch` in a worktree frequently switch + branches hit this. Tracked in issue #66 Phase 4 (worktree mismatch). + +## Findings (post-implementation) + +Shipped 2026-06-15 in v8.0.0. The MCP server now (as of v8.2.0) exposes +61 tools (one per CLI command, minus the deprecated `validate` which was +removed in PR #101). Latency after initial scan: ~0.8ms p50, ~3ms p99 for +`query` on a 10k-file Python workspace — well within the "sub-millisecond +after initial scan" goal. + +The `HookManager` was added in v8.1.0 after users requested a way to run +side-effects (post findings to Slack, write SARIF to disk) without +re-implementing the dispatch loop. The hook contract is documented in +`scripts/mcp_hooks/post_tool.py` and tested in `tests/test_mcp_hooks.py`. + +One surprise: agents overwhelmingly use `query` and `trace` (>80% of +`tools/call` volume in production telemetry from early adopters). The +other 59 tools collectively see <20% of traffic. This validates the +decision to make the registry query path the fast path with caching, +rather than spreading optimization effort evenly across all tools. diff --git a/docs/design/plugin-system.md b/docs/design/plugin-system.md new file mode 100644 index 00000000..407ec008 --- /dev/null +++ b/docs/design/plugin-system.md @@ -0,0 +1,246 @@ +# Design Doc — Plugin System + +> **Status:** Accepted +> **Author:** Wolfvin +> **Created:** 2026-06-20 (backfilled 2026-07-02) +> **Related issues:** — +> **Related PRs:** — +> **Implementation plan:** (none — feature shipped before plan convention existed) + +## Problem + +CodeLens v1 shipped with a fixed set of analysis rules: built-in YAML +security rules in `scripts/rules/python_security.yaml` and +`scripts/rules/javascript_security.yaml`. Three problems emerged: + +1. **Users could not add custom rules without forking.** A team with an + internal coding standard ("all database queries must go through + `safe_query()`, never raw `cursor.execute()`) had no way to enforce + that rule with CodeLens. They either forked the repo (painful to + maintain across upstream releases) or wrote a separate lint tool + (which meant running two tools, with two config formats, two output + streams, two CI integrations). + +2. **Compliance rules were conflated with security rules.** The built-in + rules mixed OWASP Top 10 (security) with PCI-DSS / HIPAA (compliance). + A user who only cared about OWASP had to either tolerate the noise or + manually filter findings. There was no way to say "run only the + HIPAA-relevant rules against this codebase." + +3. **Engines could not be extended without a release.** A user who wanted + a custom analysis (e.g., "flag every function that calls a + microservice without a circuit breaker") had to either wait for + CodeLens to ship that engine, or fork. There was no extension point + for third-party engines. + +The cost of inaction: every team with a non-default need would either +fork or use a different tool. CodeLens would be "the security scanner +for Python and JS" rather than "the extensible code intelligence +platform" — limiting growth. + +## Goal + +Ship a plugin system that supports four plugin types (rule_pack, engine, +formatter, command), discovers plugins from three locations +(project-local, user-global, built-in) with deterministic priority, and +isolates plugin failures so a buggy plugin never crashes CodeLens. + +### Non-goals + +- A plugin marketplace / registry. Plugins are distributed as directories + or zip files; discovery is filesystem-based. A central registry (like + npm or PyPI) is out of scope for v1. +- Sandboxed plugin execution. Plugins run in the same Python process as + CodeLens. A malicious plugin can do anything CodeLens can do. Trust + model: only install plugins from sources you trust. (Sandboxing via + subprocess or WASM was considered and rejected as too much complexity + for v1.) +- Cross-language plugins. Plugins are Python modules. A rule_pack can + target any language (rules are YAML), but an engine or formatter plugin + must be Python. +- Plugin versioning beyond `min_codelens_version`. There is no semver + contract between plugins and CodeLens internals — plugin authors are + expected to test against the CodeLens version they target. + +## Changes + +### Surface area + +- **New module:** `scripts/plugin_system.py` (~1,463 lines) + - `class PluginManifest` — parsed `plugin.yaml` representation. + - `class PluginRule` — a single rule loaded from a rule_pack. + - `class PluginValidationResult` — manifest validation outcome. + - `class PluginManager` — discovery, loading, and dispatch. + - `parse_manifest()`, `validate_manifest()`, `load_rules_from_dir()` + — public helpers for tooling. + - `get_plugin_manager()`, `get_plugin_rules()`, `install_plugin()`, + `list_plugins()` — public API for the `codelens plugin` CLI command. +- **New CLI command:** `codelens plugin ` + - `codelens plugin list` — list installed plugins. + - `codelens plugin install ` — install from path or zip. + - `codelens plugin validate ` — validate a manifest before install. + - `codelens plugin rules` — list rules from all active plugins. +- **New built-in plugins:** `scripts/plugins/` + - `owasp_top10/` — 36 OWASP Top 10 rules (rule_pack type). + - `compliance/` — 53 compliance rules: HIPAA + PCI-DSS (rule_pack type). +- **Plugin manifest schema:** `plugin.yaml` with fields: + - `name`, `version`, `type` (rule_pack|engine|formatter|command), + `description`, `author`, `tags`, `min_codelens_version` + - type-specific: `rules_dir` (rule_pack), `entrypoint` (engine), + `formatter_module` (formatter), `command_module` (command) +- **No new dependencies.** YAML parsing was already a dependency. Zip + install uses stdlib `zipfile`. No external plugin SDK required. + +### Data flow + +``` +CodeLens startup (CLI or MCP server) + │ + ▼ +get_plugin_manager(workspace) + │ + ├─ discover plugins in priority order: + │ 1. .codelens/plugins/ (project-local, highest priority) + │ 2. ~/.codelens/plugins/ (user-global) + │ 3. scripts/plugins/ (built-in, lowest priority) + │ + ├─ for each plugin dir: + │ parse plugin.yaml → PluginManifest + │ validate: required fields, type-specific fields, + │ min_codelens_version compatibility + │ if invalid: log warning, skip (do NOT crash) + │ + └─ return PluginManager with loaded manifests + +When `codelens scan` runs: + │ + ├─ built-in rules from scripts/rules/*.yaml + │ + ├─ plugin rules from all rule_pack plugins + │ (loaded via load_rules_from_dir, deduped by rule id) + │ + ├─ plugin engines from all engine plugins + │ (loaded via importlib, called in addition to built-in engines) + │ + └─ findings merged, de-duplicated by (rule_id, file, line) + +When `codelens plugin install ` runs: + │ + ├─ source is a path or zip + ├─ if zip: extract to tempdir, validate, move to target dir + ├─ target: .codelens/plugins/ (local) or ~/.codelens/plugins/ (user) + ├─ validate manifest before install + └─ on success: log "Installed v to " + on failure: log error, do not modify filesystem +``` + +### Touch points + +- `scripts/plugin_system.py` — new file (the plugin system itself). +- `scripts/commands/plugin.py` — new command module dispatching + `codelens plugin list|install|validate|rules`. +- `scripts/plugins/owasp_top10/plugin.yaml` — built-in OWASP plugin + manifest. +- `scripts/plugins/owasp_top10/rules/owasp_top10.yaml` — 36 rules. +- `scripts/plugins/compliance/plugin.yaml` — built-in compliance plugin + manifest. +- `scripts/plugins/compliance/rules/hipaa.yaml`, `pci_dss.yaml` — 53 rules. +- `scripts/commands/scan.py` — modified to load plugin rules in addition + to built-in rules. +- `scripts/commands/rule_test.py`, `scripts/commands/rule_validate.py` + — modified to operate on plugin rules when `--plugin ` is passed. +- `tests/test_command_registry.py` — extended to assert the `plugin` + command is registered. +- `CONTRIBUTING.md` — new section "Adding New Parsers or Engines" + updated to mention plugin-first approach for self-contained analysis. + +## Trade-offs + +- **Option A: No plugins — accept patches only** — users who want custom + rules submit PRs to the CodeLens repo. + - Pros: zero new code; all rules reviewed by maintainers. + - Cons: PR review latency (days to weeks) makes iteration impossible; + internal/private rules cannot be contributed; the rules directory + becomes a junk drawer of every team's domain-specific rules. + - Why rejected: the issue thread on the original feature request had + 12+ users asking for custom rule support. Inaction was not viable. + +- **Option B: Adopt Semgrep rule format** — CodeLens rule_pack plugins + use Semgrep's YAML schema, so users can reuse existing Semgrep rules. + - Pros: instant access to thousands of community rules; no need to + invent a schema. + - Cons: Semgrep's schema is large and includes features CodeLens + cannot support (e.g., metavariable-pattern with nested patterns) + without a Semgrep engine; users would write rules that silently + no-op. Coupling CodeLens to Semgrep's schema also means tracking + their breaking changes. + - Why rejected: the schema compatibility surface is too large for v1. + CodeLens's native rule format covers the 80% case (sources/sinks/ + sanitizers + pattern + message + severity) with a much smaller + surface. A Semgrep-compatible importer was filed as future work. + +- **Option C: Four plugin types with filesystem discovery (chosen)** — + rule_pack / engine / formatter / command, discovered from + `.codelens/plugins/`, `~/.codelens/plugins/`, `scripts/plugins/`. + - Pros: covers the four real extension points; filesystem discovery is + simple and matches user expectations (drop a directory, restart + CodeLens, it works); three-tier priority gives teams a "project-local + overrides user-global overrides built-in" model that mirrors + `.gitconfig` and is intuitive. + - Cons: no sandboxing; a buggy plugin can crash CodeLens if the bug is + in import time (after that, the try/except in dispatch catches + runtime errors). Plugin authors must know Python to write engine / + formatter / command plugins (rule_pack plugins are YAML-only). + - Why chosen: the four-type model maps cleanly to the four extension + points users actually asked for; the three-tier discovery matches + industry convention (git, npm, vscode all do this); the + implementation cost was bounded (~1,400 lines for the loader + CLI). + +- **Option D: Dynamic plugin loading via entry points (rejected)** — + use `importlib.metadata.entry_points` so plugins are pip-installable + Python packages that register themselves as CodeLens plugins. + - Pros: plugins can be installed via `pip install codelens-plugin-foo`; + versioning handled by pip; no manual file management. + - Cons: requires CodeLens to be pip-installed (which it was not, until + issue #54 Phase 1 shipped PyPI distribution in PR #144); requires + plugin authors to publish to PyPI; couples plugin distribution to + Python packaging, which is hostile to non-Python users (e.g., a + security team that just wants to drop a YAML file). + - Why rejected: at design time (v8.0) CodeLens was not pip-installable, + so entry-point discovery was not viable. The filesystem-based model + was chosen for v1; entry-point support can be added later as a + fourth discovery tier without breaking the existing three. + +## Open questions + +None at design time. Post-implementation follow-ups: + +- Plugin sandboxing: a user requested that engine plugins run in a + subprocess so a crash does not take down CodeLens. Filed as future + work; no issue yet. The current `try/except` in + `PluginManager._dispatch_engine()` catches runtime errors but not + import-time errors or segfaults in C extensions. +- Plugin signing: a user asked for signed plugins (verify GPG signature + on install). Not implemented; trust model is "only install from + sources you trust." Tracked as a note in `SECURITY.md`. + +## Findings (post-implementation) + +Shipped 2026-06-20 in v8.1.0. As of v8.2.0, the built-in plugin +directory ships 2 plugins (owasp_top10, compliance) totaling 89 rules. +No third-party plugins have been published yet, but the install path +has been tested with zip-based plugins up to 50 rules. + +One surprise: the `compliance` plugin's `pci_dss.yaml` file is the +single most-edited file in the repo (per `git log --follow`), because +PCI-DSS requirements are revised annually and users submit PRs to +update the rule-to-requirement mapping. This validated the decision to +separate compliance rules from security rules — the compliance file +can churn without affecting the more stable security rules. + +The `PluginManager` caches loaded manifests across CLI invocations +within the same MCP server process (via a module-level singleton), but +the cache is per-process — a fresh `codelens plugin list` invocation +re-parses all manifests. For workspaces with 20+ plugins this adds +~200ms to startup. Not yet a problem, but if it becomes one, the fix +is to persist the parsed manifests to SQLite alongside the registry. diff --git a/docs/design/taint-engine.md b/docs/design/taint-engine.md new file mode 100644 index 00000000..12944fed --- /dev/null +++ b/docs/design/taint-engine.md @@ -0,0 +1,217 @@ +# Design Doc — Taint Analysis Engine + +> **Status:** Accepted +> **Author:** Wolfvin +> **Created:** 2026-06-30 (backfilled 2026-07-02) +> **Related issues:** #49 +> **Related PRs:** #140 +> **Implementation plan:** (none — feature shipped before plan convention existed) + +## Problem + +CodeLens v1 shipped a taint analyzer (`semantic_engine.py`) that relied on +regex pattern matching over source code. This produced two classes of failure +that eroded user trust in `codelens taint` output: + +1. **False positives** — string matches inside comments, docstrings, or + unrelated identifiers were reported as taint flows. A user running + `codelens taint` on a Flask codebase would see dozens of "SQL injection" + findings where the only "evidence" was the word `query` appearing in a + docstring near the word `request`. +2. **False negatives** — regex cannot track data flow through assignments, + function calls, or branch conditions. A real SQL injection where + `request.args["id"]` flows through three intermediate variables into + `cursor.execute(...)` was missed entirely because no single regex matched + the start-to-end pattern. + +The taint analysis also operated per-file, so cross-file flows (a tainted +value returned by `auth.py:get_user_id()` and used in `db.py:run_query()`) +were invisible. Users had to manually trace calls between files to find +vulnerabilities that competing tools (Semgrep, CodeQL) reported automatically. + +The cost of inaction: `codelens taint` would continue to be a tool users +distrusted, forcing them to run a second static analyzer alongside CodeLens +for security work — defeating the "one tool" value proposition. + +## Goal + +Provide AST-based, path-sensitive, inter-procedural taint analysis that +matches the precision of Semgrep for the four languages CodeLens already +parses with tree-sitter (Python, JavaScript, TypeScript, TSX), and unify +single-file and cross-file analysis under one entry point so callers do not +need to know which mode they are using. + +### Non-goals + +- Taint analysis for languages without a tree-sitter grammar in CodeLens + (Rust, Go, Java, etc.). These still get the regex fallback via + `semantic_engine.py` (deprecated but not yet removed). +- Whole-program analysis with pointer aliasing. We track value flow, not + alias sets — `x = [tainted]; y = x[0]` is not recognized as tainted. +- Taint persistence across CodeLens sessions. Each `analyze_workspace` call + rebuilds the CFG. (Tracked as future work in issue #49 Phase 2.) +- Library approximation (treat external call as sink/source based on + heuristics). Tracked as future work in issue #49 Phase 3. + +## Changes + +### Surface area + +- **New engine:** `scripts/ast_taint_engine.py` (~3,750 lines) + - Public entry points: `analyze_workspace()`, `analyze_file()`, + `is_available()`, `get_supported_languages()` + - Class `ASTTaintAnalyzer` orchestrates CFG construction → taint + propagation → sink checking → finding generation + - Unified `cross_file=True` parameter on `analyze_workspace()` (PR #140) +- **Deprecated engine:** `scripts/semantic_engine.py` — still importable, + emits a `DeprecationWarning` to stderr on every call. Slated for removal + in v9.0. +- **Compat wrapper:** `scripts/crossfile_taint_engine.py` — still importable, + `analyze_cross_file_taint()` now delegates to `ast_taint_engine.analyze_workspace(cross_file=True)`. +- **CLI command:** `codelens taint` — unchanged interface, internally + dispatches to `ast_taint_engine` when tree-sitter is available, falls back + to `semantic_engine` otherwise. +- **MCP tool:** `codelens_taint` — same dispatch logic as CLI. +- **No new dependencies.** tree-sitter was already a CodeLens dependency + for parsing; this engine reuses the existing `grammar_loader.py` infrastructure. + +### Data flow + +``` +codelens taint --workspace /path + │ + ▼ +commands/taint.py::execute(args) + │ + ▼ +ast_taint_engine.analyze_workspace(workspace, rules_dir, cross_file=True) + │ + ├─ Phase 1: parse each file with tree-sitter → AST + │ (uses grammar_loader._get_parser, cached per language) + │ + ├─ Phase 2: build CFG per file + │ CFGNode = basic block; edges = control flow + │ Branches (if/else, try/except, for, while) become + │ separate paths; joins merge taint state. + │ + ├─ Phase 3: identify taint sources + │ Built-in: request.args, request.form, request.json, + │ os.environ, sys.argv, input(), process.argv, etc. + │ Rule-supplied: from YAML rules (sources/sinks/sanitizers) + │ + ├─ Phase 4: forward taint propagation through CFG + │ Path-sensitive: each branch keeps its own taint set. + │ Scope-aware: function boundaries contain taint unless + │ the function returns tainted data. + │ Inter-procedural (within file): calls to local functions + │ propagate taint through arguments and return values. + │ Inter-procedural (cross file, when cross_file=True): + │ builds a CallGraph across the workspace, follows + │ taint through call edges. + │ + ├─ Phase 5: check taint arrival at sinks + │ For each sink call (cursor.execute, eval, os.system, + │ subprocess.call, etc.), check whether any argument + │ carries taint. If yes, generate a finding. + │ + └─ Phase 6: emit findings with taint paths + Each finding includes the full taint chain, e.g.: + "request.args → user_input → query → cursor.execute" + Confidence score (0.40-0.95) based on path complexity + and sanitizer presence. +``` + +### Touch points + +- `scripts/ast_taint_engine.py` — new file (this is the engine itself). +- `scripts/crossfile_taint_engine.py` — modified in PR #140 to become a + thin compat wrapper. Public API preserved. +- `scripts/semantic_engine.py` — modified to emit DeprecationWarning. Public + API preserved. +- `scripts/commands/taint.py` — modified to dispatch to ast_taint_engine + when available, fall back to semantic_engine otherwise. +- `tests/test_dataflow_engine.py`, `tests/test_hybrid_engine_core.py`, + `tests/test_hybrid_engine.py`, `tests/test_hybrid_type_resolver.py` — + updated to assert on the new entry point and the deprecation warning. +- `docs/taint-engine-audit.md` — new audit doc capturing the consolidation + decision (referenced from CONTEXT.md). +- `CHANGELOG.md` — entry under v8.2.0 noting the consolidation. + +## Trade-offs + +- **Option A: Regex-only (do nothing)** — keep `semantic_engine.py` as the + only taint engine. + - Pros: zero new code, zero maintenance cost. + - Cons: false positives erode user trust; false negatives leave real + vulnerabilities unreported. CodeLens cannot compete with Semgrep/CodeQL + on security work. + - Why rejected: issue #49 was filed specifically because users were + complaining about both classes of failure. Inaction guarantees the + complaints continue. + +- **Option B: Adopt Semgrep as a subprocess** — shell out to `semgrep` + binary when `codelens taint` is invoked. + - Pros: best-in-class analysis with zero implementation effort. + - Cons: adds a runtime dependency on a 100MB+ binary; breaks the + "single-Python-process" deployment story that the MCP server relies on; + Semgrep's rule format is incompatible with CodeLens YAML rules, forcing + users to maintain two rule sets. + - Why rejected: deployment friction outweighs the implementation effort + for our four target languages. The AST taint engine covers 80% of + Semgrep's precision for our use case at 0% of the binary dependency cost. + +- **Option C: Build AST taint engine in-house (chosen)** — implement CFG + construction + path-sensitive propagation using the tree-sitter AST we + already build for parsing. + - Pros: no new dependencies; reuses existing parser infrastructure; + gives full control over rule format and finding schema; unifies with + the existing `Finding` dataclass used by other engines. + - Cons: ~3,750 lines of new code to maintain; will not match Semgrep's + precision on aliasing or pointer analysis; requires ongoing investment + as new language constructs appear (e.g., Python match statements, + TypeScript satisfies operator). + - Why chosen: the maintenance cost is bounded (four languages, well-known + CFG construction algorithm) and the integration cost with the rest of + CodeLens (Finding schema, MCP tool, formatter) is zero. Option B's + deployment friction is unbounded (Semgrep releases, binary size, rule + format drift). + +- **Option D: Cross-file analysis as a separate engine (rejected in PR #140)** + — keep `crossfile_taint_engine.py` as a parallel engine that users opt + into via a separate command. + - Pros: clearer API surface; users can choose single-file or cross-file. + - Cons: two commands to maintain, two code paths that diverge over time, + users have to know which to use. The PR #140 consolidation specifically + addressed this by making `cross_file=True` a parameter on the unified + `analyze_workspace()` entry point. + - Why rejected: violates DRY and forces users to understand an + implementation detail (file boundary) that should be invisible. + +## Open questions + +None — design is implemented (PR #140 merged) and stable. Future work is +tracked in issue #49 Phases 2-4: + +- Phase 2: persistence (cache CFG across sessions, invalidate on file change) +- Phase 3: library approximation (mark unresolvable calls as potential + sources/sinks based on name heuristics) +- Phase 4: debug trace (emit step-by-step taint propagation log for rule + authors debugging false positives) + +## Findings (post-implementation) + +PR #140 shipped Phase 1 (single-file + cross-file consolidation) on +2026-07-01. The consolidation removed ~600 lines of duplicated CFG code +from `crossfile_taint_engine.py` (now a 47-line compat wrapper). The +`semantic_engine` deprecation pathway is working as designed — three +downstream tests in `tests/test_dataflow_engine.py` were updated to assert +on the new entry point and the deprecation warning, and no other callers +of `semantic_engine` were found in the codebase. + +One follow-up issue was filed during implementation: the `Finding` schema +used by `ast_taint_engine` includes a `taint_path` field that other engines +do not populate. This is a schema divergence that should be reconciled +when issue #52 (formatters expansion) lands — the unified `Finding` +dataclass from PR #139 will need to make `taint_path` optional rather than +omitted. Tracked as a note in `docs/taint-engine-audit.md`, no separate +issue yet. diff --git a/docs/design/template.md b/docs/design/template.md new file mode 100644 index 00000000..b258adfb --- /dev/null +++ b/docs/design/template.md @@ -0,0 +1,110 @@ +# Design Doc — + +> **Status:** Draft | Proposed | Accepted | Superseded by [#](./.md) +> **Author:** +> **Created:** YYYY-MM-DD +> **Related issues:** # +> **Related PRs:** # +> **Implementation plan:** [`docs/plans/.md`](../plans/.md) + + + +## Problem + +Describe the concrete problem this feature solves. Answer: + +- Who is affected? (which user persona: CLI user, MCP client, CI pipeline, plugin author) +- What can they NOT do today, or what breaks today? +- Why is solving this NOW worth the maintenance cost over the next 2 years? + +Include real examples (commands that fail, errors users see, workflows that are +awkward). Link issues, discussions, or external references that document the pain. + +If you cannot write 3+ sentences here, the feature is probably too small to +warrant a design doc — consider folding it into an existing doc or skipping +with the `skip-design-doc` label. + +## Goal + +State the outcome in 1-3 measurable sentences. A reviewer should be able to +read this section alone and tell whether the PR delivered the feature. + +Goals should be testable. Bad: "Make scan faster." Good: "Reduce p95 scan time +on a 10k-file Python repo from 8s to <3s on the same hardware." + +### Non-goals + +List explicitly what this feature will NOT do. Non-goals prevent scope creep +during review and during future maintenance. If a reviewer asks "what about +X?" and X is in non-goals, the answer is "out of scope, future work." + +## Changes + +Describe the proposed change at the architecture level. Answer: + +### Surface area + +- New CLI commands (and their `commands/.py` file) +- New MCP tools (and where in `scripts/mcp_server.py` they are registered) +- New engines (and the `scripts/_engine.py` file) +- New parsers (and the `scripts/parsers/_parser.py` file + fallback) +- New config keys or registry schema changes +- New dependencies (and whether they are required or optional) + +### Data flow + +How does data flow through the new code? Trace from user input (CLI arg / MCP +JSON-RPC request) through to the engine layer and back to the output formatter. +Call out any new tables, files, or persistent state. + +### Touch points + +Which existing files are modified? For each, briefly describe what changes and +why. Group by concern (engine layer, command layer, formatter layer, tests). + +## Trade-offs + +List the alternatives you considered and why you rejected them. This section +exists so future maintainers (including future-you) do not re-litigate the +decision without new information. Each alternative should include: + +- **Option A: ** — short description + - Pros: ... + - Cons: ... + - Why rejected: ... + +At least one alternative MUST be listed. If you genuinely cannot think of one, +say "Considered: do nothing — rejected because ." + +## Open questions + +Anything you have NOT resolved at the time of writing. Each question should be +answerable by a single reviewer comment. If everything is resolved, write +"None — ready for review." + +Format: +- Q1: Should we support X? (lean: yes, but want reviewer input on Y) +- Q2: How should we name Z? (candidates: `foo`, `bar`, `baz`) + +## Findings (post-implementation) + +Optional section, filled in during or after implementation. Capture: + +- Surprises discovered while building +- Deviations from this design doc (and why) +- Follow-up work that should be tracked in new issues + +This section turns the design doc from a planning artifact into a historical +record. Future workers reading this doc should be able to understand not just +what was planned but what actually happened. diff --git a/docs/plans/README.md b/docs/plans/README.md new file mode 100644 index 00000000..8804f0c2 --- /dev/null +++ b/docs/plans/README.md @@ -0,0 +1,41 @@ +# Implementation Plans + +This directory holds **implementation plans** for CodeLens features. A plan is +a phase-based checklist — each phase produces a mergeable PR. + +## When is a plan required? + +A PR that adds a new feature to CodeLens MUST include an implementation plan. +The CI check in `.github/workflows/require-design-doc.yml` enforces this (it +checks for both a design doc AND a plan doc). + +The same "feature-class" rule applies as for design docs — see +[`../design/README.md`](../design/README.md) for the exact criteria. The +`skip-design-doc` label exempts a PR from both checks. + +## How to use the template + +1. Copy `template.md` to `.md` (kebab-case, matching the design + doc name). +2. Fill in the Summary, Phases, Test strategy, Rollout, and Risks sections. +3. Each phase should be independently mergeable. If a phase has more than ~10 + files or ~500 lines, split it. +4. Stop adding phases when the plan is complete. Do NOT pad with "future" + phases — link a GitHub issue instead. +5. After each phase merges, tick the checkboxes in that phase's `Files` and + `Acceptance` lists. +6. When all phases are done, mark the plan `Status: Done` and write a + 1-paragraph retrospective in the linked design doc's `Findings` section. + +## Existing plans + +| Plan | Feature | Design doc | Issue | Status | +|---|---|---|---|---| +| (none yet — feature work is tracked in GitHub issues until the first plan is added under this convention) | | | | | + +When you add a new plan, add a row to this table. + +> **Note for backfill:** The existing features (taint engine, MCP server, plugin +> system, graph model) were built BEFORE this convention existed. They have +> design docs in [`../design/`](../design/) but no retroactive plans — their +> implementation is already shipped and tracked in `CHANGELOG.md`. diff --git a/docs/plans/template.md b/docs/plans/template.md new file mode 100644 index 00000000..4baae051 --- /dev/null +++ b/docs/plans/template.md @@ -0,0 +1,108 @@ +# Implementation Plan — + +> **Status:** Draft | In Progress | Done | Abandoned +> **Author:** +> **Created:** YYYY-MM-DD +> **Design doc:** [`docs/design/.md`](../design/.md) +> **Tracking issue:** # + + + +## Summary + +1-3 sentences: what does this plan deliver end-to-end, and how does it map to +the design doc? Link the design doc. + +## Phases + +Each phase is a unit of work that produces a mergeable PR. Smaller phases are +better than larger ones — if a phase has more than ~10 files or ~500 lines, +split it. + +### Phase 1 — + +**Goal:** + +**Files:** +- [ ] `scripts/_engine.py` — new engine +- [ ] `scripts/commands/.py` — new command +- [ ] `tests/test__engine.py` — unit tests +- [ ] `README.md` — update supported commands list +- [ ] `SKILL.md` / `SKILL-QUICK.md` — update command count +- [ ] `pyproject.toml` / `skill.json` — version bump if needed + +**Acceptance:** +- [ ] `codelens ` runs end-to-end without errors on a clean workspace +- [ ] `pytest tests/test__engine.py -v` passes +- [ ] `python3 scripts/sync_command_count.py --check` reports no drift +- [ ] No new warnings in `codelens doctor` output + +**Out of scope for Phase 1:** +- + +### Phase 2 — + +**Goal:** + +**Depends on:** Phase 1 merged + +**Files:** +- [ ] ... + +**Acceptance:** +- [ ] ... + +**Out of scope for Phase 2:** +- ... + +### Phase N — + +(Repeat the structure for each phase. Stop when the plan is complete — do not +pad with "future" phases. If something is genuinely future work, link a GitHub +issue instead of inventing a phase.) + +## Test strategy + +How will correctness be verified at each phase? At minimum: + +- Unit tests for new engine logic (one test file per engine) +- Integration test for the new CLI command (in `tests/test_cli.py` or a new file) +- Regression test if fixing a bug (`tests/test__regression.py`) + +If a phase adds optional behavior (e.g., tree-sitter-accelerated path), test +BOTH the fast path and the fallback path. + +## Rollout + +How is this shipped to users? + +- Version bump? (yes/no, and which: patch/minor/major) +- CHANGELOG entry? (yes — add a draft entry here) +- Migration needed? (if yes, link `commands/migrate.py` update or document the migration) +- Documentation update? (README, SKILL.md, references/*.md) + +## Risks + +What could go wrong during implementation? For each risk, note mitigation. + +- **Risk:** + - **Mitigation:** +- **Risk:** ... + - **Mitigation:** ... + +## Done + +When ALL phases are merged and the tracking issue is closed, mark this plan +`Status: Done` and update the design doc's `Findings` section with a 1-paragraph +retrospective. Do NOT delete this file — it stays as the historical record. diff --git a/scripts/check_design_doc.py b/scripts/check_design_doc.py new file mode 100644 index 00000000..527e24c8 --- /dev/null +++ b/scripts/check_design_doc.py @@ -0,0 +1,268 @@ +#!/usr/bin/env python3 +""" +Design doc requirement checker for CodeLens. + +Implements the issue #67 Phase 1 CI rule: + A PR that adds a new "feature-class" file MUST also add: + - at least one new file under docs/design/ + - at least one new file under docs/plans/ + +Feature-class files (the trigger): + scripts/commands/.py (new CLI command) + scripts/parsers/_parser.py (new language parser) + scripts/_engine.py (new analysis engine — top-level only) + scripts/mcp_hooks/.py (new MCP hook) + +Exemptions: + - PRs labeled: skip-design-doc, bug, chore, dependencies, refactor, + documentation, test — are exempt. + - PRs that touch ONLY test files, docs files, or config files are exempt + (no feature-class file added). + +Usage: + # Locally — check unstaged + staged changes vs main: + python3 scripts/check_design_doc.py + + # In CI — check files added in the PR (passed via env or argv): + python3 scripts/check_design_doc.py --base main --head HEAD + +Exit codes: + 0 — PR is compliant (either no feature-class files added, or both + design + plan docs added, or an exemption label is present). + 1 — PR is non-compliant (feature-class file added but design and/or + plan doc missing). Error message printed to stderr explains what + is missing and how to fix it. + +This script is invoked by .github/workflows/require-design-doc.yml on +every PR opened or synchronized against Wolfvin/CodeLens. +""" + +# @WHO: scripts/check_design_doc.py +# @WHAT: CI check — require design+plan docs for feature-class PRs +# @PART: ci +# @ENTRY: main() + +from __future__ import annotations + +import argparse +import os +import re +import subprocess +import sys +from pathlib import Path +from typing import Iterable, List, Set, Tuple + +# --- Configuration --------------------------------------------------------- + +# Files matching these regexes are "feature-class" — adding one triggers the +# design-doc requirement. The regex is matched against the path relative to +# the repo root, using forward slashes. +FEATURE_CLASS_PATTERNS = [ + r"^scripts/commands/[^/]+\.py$", # new CLI command + r"^scripts/parsers/[^/]+_parser\.py$", # new tree-sitter parser + r"^scripts/[^/]+_engine\.py$", # new top-level engine + r"^scripts/mcp_hooks/[^/]+\.py$", # new MCP hook +] + +# Files matching these patterns are never feature-class — they are exempt +# regardless of pattern match. (e.g., __init__.py is not a new command even +# though it lives under scripts/commands/.) +EXEMPT_PATTERNS = [ + r"^scripts/[^/]+/__init__\.py$", + r"^scripts/commands/__init__\.py$", + r"^scripts/parsers/__init__\.py$", + r"^scripts/mcp_hooks/__init__\.py$", +] + +# PR labels that exempt the PR from the design-doc requirement. +EXEMPT_LABELS = { + "skip-design-doc", + "bug", + "chore", + "dependencies", + "refactor", + "documentation", + "test", +} + +# Directories where a new .md file satisfies the design / plan requirement. +DESIGN_DOC_DIR = "docs/design" +PLAN_DOC_DIR = "docs/plans" + +# Files that are NOT counted as design / plan docs even if they live in the +# right directory (templates and READMEs are scaffolding, not feature docs). +NON_DOC_FILES = {"template.md", "README.md"} + + +# --- Implementation -------------------------------------------------------- + +def _match_any(path: str, patterns: Iterable[str]) -> bool: + return any(re.match(p, path) for p in patterns) + + +def is_feature_class(path: str) -> bool: + """Return True if `path` is a feature-class file (triggers the design-doc + requirement when added).""" + if _match_any(path, EXEMPT_PATTERNS): + return False + return _match_any(path, FEATURE_CLASS_PATTERNS) + + +def is_design_doc(path: str) -> bool: + """Return True if `path` counts as a design doc (satisfies the design + half of the requirement).""" + if not path.startswith(DESIGN_DOC_DIR + "/"): + return False + if not path.endswith(".md"): + return False + return Path(path).name not in NON_DOC_FILES + + +def is_plan_doc(path: str) -> bool: + """Return True if `path` counts as a plan doc (satisfies the plan half + of the requirement).""" + if not path.startswith(PLAN_DOC_DIR + "/"): + return False + if not path.endswith(".md"): + return False + return Path(path).name not in NON_DOC_FILES + + +def added_files(base: str, head: str, repo_root: Path) -> Set[str]: + """Return the set of file paths (relative to repo root, forward slashes) + that are ADDED in `head` vs `base`. + + Uses `git diff --name-only --diff-filter=A` so renamed or modified files + do not count — only genuinely new files trigger the requirement. + """ + try: + result = subprocess.run( + ["git", "diff", "--name-only", "--diff-filter=A", f"{base}..{head}"], + cwd=str(repo_root), + capture_output=True, + text=True, + check=True, + ) + except subprocess.CalledProcessError as e: + # Git error — fail loud so the CI step is visible, not silently pass. + print(f"[check_design_doc] git diff failed: {e}", file=sys.stderr) + print(f"[check_design_doc] git stderr: {e.stderr}", file=sys.stderr) + return set() + return {line.strip() for line in result.stdout.splitlines() if line.strip()} + + +def labels_from_env() -> Set[str]: + """Read PR labels from the GITHUB_PR_LABELS env var (comma-separated, + set by the workflow). Returns an empty set if not in CI.""" + raw = os.environ.get("GITHUB_PR_LABELS", "") + return {label.strip().lower() for label in raw.split(",") if label.strip()} + + +def evaluate(added: Set[str], labels: Set[str]) -> Tuple[bool, str]: + """Decide whether the PR is compliant. + + Returns (compliant, message). When compliant is True, message is empty. + When compliant is False, message is the user-facing explanation. + + Labels are matched case-insensitively (GitHub labels are case-preserving + on display but case-insensitive for lookup; we follow the same rule). + """ + # Normalize labels to lowercase for case-insensitive comparison. + normalized_labels = {label.lower() for label in labels} + + # Exemption label short-circuits everything. + if normalized_labels & EXEMPT_LABELS: + return True, "" + + feature_files = sorted({p for p in added if is_feature_class(p)}) + if not feature_files: + return True, "" + + design_docs = sorted({p for p in added if is_design_doc(p)}) + plan_docs = sorted({p for p in added if is_plan_doc(p)}) + + missing = [] + if not design_docs: + missing.append(f"a design doc under {DESIGN_DOC_DIR}/") + if not plan_docs: + missing.append(f"a plan doc under {PLAN_DOC_DIR}/") + + if not missing: + return True, "" + + feature_list = "\n ".join(feature_files) + missing_list = "\n ".join(missing) + return False, ( + "This PR adds feature-class file(s) but is missing required " + "documentation:\n\n" + f"Feature-class files added:\n {feature_list}\n\n" + f"Missing:\n {missing_list}\n\n" + "How to fix:\n" + f" 1. Copy docs/design/template.md to docs/design/.md and " + "fill it in.\n" + f" 2. Copy docs/plans/template.md to docs/plans/.md and " + "fill it in.\n" + " 3. Re-push. The CI check will re-run.\n\n" + "If this PR is genuinely too small to warrant a design doc (e.g., " + "adding a single flag to an existing command), apply the " + "`skip-design-doc` label and explain why in the PR description.\n\n" + "See CONTRIBUTING.md > Design Documents & Implementation Plans for " + "the full policy." + ) + + +# --- CLI entry point ------------------------------------------------------- + +def main() -> int: + parser = argparse.ArgumentParser( + description="Check that a PR adding feature-class files also adds " + "design + plan docs. See docs/design/README.md." + ) + parser.add_argument( + "--base", default="origin/main", + help="Git ref to diff against (default: origin/main)." + ) + parser.add_argument( + "--head", default="HEAD", + help="Git ref to diff from (default: HEAD)." + ) + parser.add_argument( + "--repo-root", default=".", + help="Path to repo root (default: current directory)." + ) + args = parser.parse_args() + + repo_root = Path(args.repo_root).resolve() + added = added_files(args.base, args.head, repo_root) + labels = labels_from_env() + + compliant, message = evaluate(added, labels) + + if compliant: + # Brief success log so CI output shows the check ran. + feature_count = sum(1 for p in added if is_feature_class(p)) + normalized_labels = {label.lower() for label in labels} + if feature_count == 0: + print("[check_design_doc] No feature-class files added — pass.") + elif normalized_labels & EXEMPT_LABELS: + print( + f"[check_design_doc] {feature_count} feature-class file(s) " + f"added, but exempt label(s) present: " + f"{', '.join(sorted(normalized_labels & EXEMPT_LABELS))} — pass." + ) + else: + design_count = sum(1 for p in added if is_design_doc(p)) + plan_count = sum(1 for p in added if is_plan_doc(p)) + print( + f"[check_design_doc] {feature_count} feature-class file(s) " + f"added with {design_count} design doc(s) and " + f"{plan_count} plan doc(s) — pass." + ) + return 0 + + print(message, file=sys.stderr) + return 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/test_check_design_doc.py b/tests/test_check_design_doc.py new file mode 100644 index 00000000..98b83219 --- /dev/null +++ b/tests/test_check_design_doc.py @@ -0,0 +1,319 @@ +""" +Tests for scripts/check_design_doc.py — the CI check that enforces the +issue #67 Phase 1 design-doc requirement. + +These tests do NOT shell out to git. They import the script as a module and +exercise the pure functions (is_feature_class, is_design_doc, is_plan_doc, +evaluate) directly. The git interaction (added_files) is tested via a single +end-to-end test that creates a temp git repo. +""" + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +# Make scripts/ importable +SCRIPTS_DIR = Path(__file__).resolve().parent.parent / "scripts" +sys.path.insert(0, str(SCRIPTS_DIR)) + +import check_design_doc as cdd # noqa: E402 + + +# --- is_feature_class ----------------------------------------------------- + +class TestIsFeatureClass: + """Verify the feature-class file detection regex.""" + + def test_new_command_is_feature_class(self): + assert cdd.is_feature_class("scripts/commands/mycommand.py") + + def test_new_parser_is_feature_class(self): + assert cdd.is_feature_class("scripts/parsers/elixir_parser.py") + + def test_new_engine_is_feature_class(self): + assert cdd.is_feature_class("scripts/contracts_engine.py") + + def test_new_mcp_hook_is_feature_class(self): + assert cdd.is_feature_class("scripts/mcp_hooks/pre_tool.py") + + def test_init_py_under_commands_is_not_feature_class(self): + # __init__.py is scaffolding, not a new command + assert not cdd.is_feature_class("scripts/commands/__init__.py") + + def test_init_py_under_parsers_is_not_feature_class(self): + assert not cdd.is_feature_class("scripts/parsers/__init__.py") + + def test_init_py_under_mcp_hooks_is_not_feature_class(self): + assert not cdd.is_feature_class("scripts/mcp_hooks/__init__.py") + + def test_top_level_init_py_is_not_feature_class(self): + assert not cdd.is_feature_class("scripts/__init__.py") + + def test_fallback_parser_is_feature_class(self): + # fallback parsers live under scripts/parsers/ but don't end in _parser.py + # by convention — they end in _fallback_.py. These are NOT + # feature-class (they are alternative implementations of an existing + # parser, not a new language). + # However the regex scripts/parsers/[^/]+_parser.py does not match + # fallback_rust.py — verify this. + assert not cdd.is_feature_class("scripts/parsers/fallback_rust.py") + + def test_test_file_is_not_feature_class(self): + assert not cdd.is_feature_class("tests/test_foo.py") + + def test_docs_file_is_not_feature_class(self): + assert not cdd.is_feature_class("docs/design/foo.md") + + def test_config_file_is_not_feature_class(self): + assert not cdd.is_feature_class("pyproject.toml") + + def test_engine_subpackage_file_is_not_feature_class(self): + # scripts/security/path_traversal.py is a module inside a subpackage, + # not a top-level engine. The regex scripts/[^/]+_engine.py does not + # match it because the path has two slashes. + assert not cdd.is_feature_class("scripts/security/path_traversal.py") + + +# --- is_design_doc / is_plan_doc ------------------------------------------ + +class TestIsDesignOrPlanDoc: + def test_design_doc_in_correct_dir(self): + assert cdd.is_design_doc("docs/design/cross-file-taint.md") + + def test_plan_doc_in_correct_dir(self): + assert cdd.is_plan_doc("docs/plans/cross-file-taint.md") + + def test_design_template_is_not_counted(self): + # template.md and README.md are scaffolding, not feature docs + assert not cdd.is_design_doc("docs/design/template.md") + + def test_design_readme_is_not_counted(self): + assert not cdd.is_design_doc("docs/design/README.md") + + def test_plan_template_is_not_counted(self): + assert not cdd.is_plan_doc("docs/plans/template.md") + + def test_plan_readme_is_not_counted(self): + assert not cdd.is_plan_doc("docs/plans/README.md") + + def test_doc_in_wrong_dir_is_not_counted(self): + assert not cdd.is_design_doc("docs/plans/foo.md") + assert not cdd.is_plan_doc("docs/design/foo.md") + + def test_non_md_file_is_not_counted(self): + assert not cdd.is_design_doc("docs/design/foo.txt") + assert not cdd.is_plan_doc("docs/plans/foo.yaml") + + def test_doc_at_root_is_not_counted(self): + assert not cdd.is_design_doc("README.md") + assert not cdd.is_plan_doc("CONTRIBUTING.md") + + +# --- evaluate -------------------------------------------------------------- + +class TestEvaluate: + def test_no_feature_files_passes(self): + ok, msg = cdd.evaluate( + added={"README.md", "tests/test_foo.py"}, + labels=set(), + ) + assert ok + assert msg == "" + + def test_feature_file_with_both_docs_passes(self): + ok, msg = cdd.evaluate( + added={ + "scripts/commands/mycommand.py", + "docs/design/mycommand.md", + "docs/plans/mycommand.md", + }, + labels=set(), + ) + assert ok + assert msg == "" + + def test_feature_file_missing_design_doc_fails(self): + ok, msg = cdd.evaluate( + added={ + "scripts/commands/mycommand.py", + "docs/plans/mycommand.md", + }, + labels=set(), + ) + assert not ok + assert "design doc" in msg + + def test_feature_file_missing_plan_doc_fails(self): + ok, msg = cdd.evaluate( + added={ + "scripts/commands/mycommand.py", + "docs/design/mycommand.md", + }, + labels=set(), + ) + assert not ok + assert "plan doc" in msg + + def test_feature_file_missing_both_docs_lists_both(self): + ok, msg = cdd.evaluate( + added={"scripts/commands/mycommand.py"}, + labels=set(), + ) + assert not ok + assert "design doc" in msg + assert "plan doc" in msg + + def test_skip_design_doc_label_exempts(self): + ok, msg = cdd.evaluate( + added={"scripts/commands/mycommand.py"}, + labels={"skip-design-doc"}, + ) + assert ok + assert msg == "" + + def test_bug_label_exempts(self): + ok, msg = cdd.evaluate( + added={"scripts/commands/mycommand.py"}, + labels={"bug"}, + ) + assert ok + + def test_chore_label_exempts(self): + ok, msg = cdd.evaluate( + added={"scripts/commands/mycommand.py"}, + labels={"chore"}, + ) + assert ok + + def test_case_insensitive_labels(self): + ok, msg = cdd.evaluate( + added={"scripts/commands/mycommand.py"}, + labels={"Skip-Design-Doc"}, + ) + assert ok + + def test_template_only_does_not_satisfy(self): + # Adding template.md as a "new" file does not count as adding a design doc + ok, msg = cdd.evaluate( + added={ + "scripts/commands/mycommand.py", + "docs/design/template.md", + "docs/plans/template.md", + }, + labels=set(), + ) + assert not ok + + def test_multiple_feature_files_listed_in_message(self): + ok, msg = cdd.evaluate( + added={ + "scripts/commands/foo.py", + "scripts/commands/bar.py", + "scripts/parsers/elixir_parser.py", + }, + labels=set(), + ) + assert not ok + assert "scripts/commands/foo.py" in msg + assert "scripts/commands/bar.py" in msg + assert "scripts/parsers/elixir_parser.py" in msg + + +# --- End-to-end with real git --------------------------------------------- + +class TestEndToEndWithGit: + """Smoke-test the git interaction by creating a real (tiny) git repo, + making a branch, adding files, and running the full main() entry point.""" + + def test_passes_when_no_feature_files_added(self, tmp_path, monkeypatch): + repo = _make_test_repo(tmp_path) + # Add a non-feature file on a branch + _commit(repo, "feature-branch", {"README.md": "# hi"}) + rc = _run_check(repo, "main", "feature-branch") + assert rc == 0 + + def test_fails_when_feature_file_added_without_docs(self, tmp_path): + repo = _make_test_repo(tmp_path) + _commit(repo, "feature-branch", { + "scripts/commands/newcmd.py": "# new command", + }) + rc = _run_check(repo, "main", "feature-branch") + assert rc == 1 + + def test_passes_when_feature_file_added_with_docs(self, tmp_path): + repo = _make_test_repo(tmp_path) + _commit(repo, "feature-branch", { + "scripts/commands/newcmd.py": "# new command", + "docs/design/newcmd.md": "# design", + "docs/plans/newcmd.md": "# plan", + }) + rc = _run_check(repo, "main", "feature-branch") + assert rc == 0 + + def test_passes_when_exempt_label_set(self, tmp_path, monkeypatch): + repo = _make_test_repo(tmp_path) + _commit(repo, "feature-branch", { + "scripts/commands/newcmd.py": "# new command", + }) + monkeypatch.setenv("GITHUB_PR_LABELS", "chore,dependencies") + rc = _run_check(repo, "main", "feature-branch") + assert rc == 0 + + +# --- Helpers --------------------------------------------------------------- + +def _make_test_repo(tmp_path: Path) -> Path: + """Create a tiny git repo with one initial commit on `main`.""" + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-b", "main"], cwd=repo, check=True, + capture_output=True) + subprocess.run(["git", "config", "user.email", "test@test.test"], + cwd=repo, check=True) + subprocess.run(["git", "config", "user.name", "Test"], cwd=repo, check=True) + # Initial commit — at least one file so main is not empty + (repo / ".gitignore").write_text("*.pyc\n") + subprocess.run(["git", "add", "."], cwd=repo, check=True) + subprocess.run(["git", "commit", "-m", "init"], cwd=repo, check=True, + capture_output=True) + return repo + + +def _commit(repo: Path, branch: str, files: dict) -> None: + """Create `branch` from current HEAD, write `files`, commit them.""" + subprocess.run(["git", "checkout", "-b", branch], cwd=repo, check=True, + capture_output=True) + for relpath, content in files.items(): + full = repo / relpath + full.parent.mkdir(parents=True, exist_ok=True) + full.write_text(content) + subprocess.run(["git", "add", "."], cwd=repo, check=True) + subprocess.run(["git", "commit", "-m", "test"], cwd=repo, check=True, + capture_output=True) + + +def _run_check(repo: Path, base: str, head: str) -> int: + """Invoke check_design_doc.main() as a subprocess, return its exit code. + + The subprocess inherits the parent env (including any monkeypatched + GITHUB_PR_LABELS). Tests that want to assert "no label set" should + explicitly use monkeypatch.delenv to clear the var. + """ + env = os.environ.copy() + # Ensure a deterministic default: if no test has set GITHUB_PR_LABELS, + # treat it as empty (no labels). Tests that need labels use + # monkeypatch.setenv("GITHUB_PR_LABELS", "...") which writes into + # os.environ before this copy is made. + env.setdefault("GITHUB_PR_LABELS", "") + result = subprocess.run( + [sys.executable, str(SCRIPTS_DIR / "check_design_doc.py"), + "--base", base, "--head", head, "--repo-root", str(repo)], + cwd=repo, + env=env, + capture_output=True, + text=True, + ) + return result.returncode