diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 0d55e24..e755504 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -4,11 +4,17 @@ updates: directory: / schedule: interval: weekly + cooldown: + default-days: 7 - package-ecosystem: rust-toolchain directory: / schedule: interval: weekly + cooldown: + default-days: 7 - package-ecosystem: github-actions directory: / schedule: interval: weekly + cooldown: + default-days: 7 diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 98ee937..c1f5019 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -24,7 +24,7 @@ jobs: python-version: '3.13' - name: Check packaging policy run: | - python3 -m pip install PyYAML==6.0.3 + python3 -m pip install PyYAML==6.0.3 pre-commit==4.5.1 python3 packaging/test_policy.py # Dependabot raises version bumps. This reports whether the tree as locked carries a @@ -41,27 +41,19 @@ jobs: with: command: check - # Project invariants clippy cannot express. Not a security scan: CodeQL already - # analyses this crate, and CodeRabbit keeps running its own opengrep packs because the - # ruleset is deliberately not named so CodeRabbit adopts it. - rules: - name: Custom rules + # Workflow-level audit: permissions, injection, unpinned actions. The other gates + # look at the crate and the packages, not at the workflows themselves. + zizmor: + name: Workflow audit runs-on: ubuntu-latest steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false - - name: Install opengrep - env: - OPENGREP_VERSION: v1.30.0 - OPENGREP_SHA256: 35779bdd72e92129c8df2a77f0c55e8c08356801ea92591ef32108d6b28d564c - run: | - curl -fsSL -o /usr/local/bin/opengrep \ - "https://github.com/opengrep/opengrep/releases/download/${OPENGREP_VERSION}/opengrep_manylinux_x86" - echo "${OPENGREP_SHA256} /usr/local/bin/opengrep" | sha256sum -c - - chmod +x /usr/local/bin/opengrep - opengrep --version - - name: Run the rule-tests - run: ./scripts/opengrep-test.sh - - name: Run the custom ruleset - run: ./scripts/opengrep-scan.sh + # advanced-security uploads SARIF and needs security-events: write, which a called + # workflow cannot hold unless every caller grants it. The findings gate the job and + # appear as annotations, so the upload buys nothing worth widening the callers for. + - uses: zizmorcore/zizmor-action@cc914d7f3750a2d13d75c7f184a1060aa0e9d482 # v0.6.4 + with: + advanced-security: false + annotations: true diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 30ce03c..d22d007 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,7 +11,7 @@ permissions: jobs: check: - uses: ./.github/workflows/checks.yml + uses: $/.github/workflows/checks.yml packages: - uses: ./.github/workflows/packages.yml + uses: $/.github/workflows/packages.yml diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index cb23ba9..e8f13b4 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -12,7 +12,7 @@ jobs: # The release must not publish while the test gate is red. check: if: github.event_name == 'push' - uses: ./.github/workflows/checks.yml + uses: $/.github/workflows/checks.yml # build_command runs only during a release, so a broken one would surface on main # with the release already half done. Run it here against the version a release diff --git a/.opengrep/README.md b/.opengrep/README.md index beb67b7..628a2e0 100644 --- a/.opengrep/README.md +++ b/.opengrep/README.md @@ -14,11 +14,21 @@ stop coming back review after review. ## Relationship to CodeRabbit -CodeRabbit auto-detects an opengrep config only when it is named `opengrep.yml` or -`semgrep.yml` (and a few variants), and when it finds one it runs *that* **instead of** its -default packs. This ruleset deliberately avoids those names, so CodeRabbit keeps running -its own packs while these rules are enforced separately by `scripts/opengrep-scan.sh` and -the CI job. Both rulesets apply. +CodeRabbit steps aside from opengrep in two separate ways, and this setup avoids both. + +It auto-detects an opengrep config only when it is named `opengrep.yml` or `semgrep.yml` +(and a few variants), and when it finds one it runs *that* **instead of** its default +packs. This ruleset deliberately avoids those names. + +It also skips its own opengrep pass when it sees opengrep running in the workflows. These +rules therefore run from a **local pre-commit hook only**, never in CI. Running them in CI +would trade CodeRabbit's broad packs for this repo's two narrow rules, which is a straight +loss. A policy test asserts no workflow mentions opengrep. + +Install the hook with `pre-commit install --install-hooks`. Both hooks carry a `files` +filter, because the scripts scan the whole crate and would otherwise run on every commit: +the scan runs when `src/` or the rules change, the rule-tests only when the rules change. A +commit touching neither costs nothing. ## Layout @@ -26,8 +36,8 @@ the CI job. Both rulesets apply. | --- | --- | | `.opengrep/agentx-ifstack-rules.yaml` | The ruleset, and the single source of truth. Named so CodeRabbit does not adopt it. | | `.opengrep/tests/*.rs` | Rule-test fixtures. `// ruleid:` must match, `// ok:` must not. They violate the rules on purpose and are not part of the crate. | -| `scripts/opengrep-scan.sh` | Scan `src/`. Exits non-zero on any finding. | -| `scripts/opengrep-test.sh` | Run the rule-tests against the ruleset. | +| `scripts/opengrep-scan.sh` | Scan `src/`. Exits non-zero on any finding. Wired to pre-commit by `.pre-commit-config.yaml`. | +| `scripts/opengrep-test.sh` | Run the rule-tests against the ruleset. Runs when the rules change. | ## Rules diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..f032abb --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,27 @@ +# Local hooks only. The repository's other gates run in CI; this file exists so the +# custom opengrep ruleset runs somewhere, because CodeRabbit skips its own opengrep pass +# when it sees opengrep in the workflows. See .opengrep/README.md. +# +# Install with: pre-commit install --install-hooks +repos: + - repo: local + hooks: + # The scripts scan the whole crate, so filenames are not passed. `files` still + # decides whether the hook runs at all, which keeps a docs-only commit free. + - id: opengrep-rule-tests + name: opengrep rule-tests + entry: scripts/opengrep-test.sh + language: script + pass_filenames: false + files: ^\.opengrep/ + # Rules changing can flag code that already exists, so run the scan for those too. + - id: opengrep + name: opengrep (custom agentx-ifstack rules) + entry: scripts/opengrep-scan.sh + language: script + pass_filenames: false + files: ^(src/.*\.rs|\.opengrep/.*)$ + +default_install_hook_types: [pre-commit] +default_stages: [pre-commit] +fail_fast: false diff --git a/CLAUDE.md b/CLAUDE.md index aea541d..6e1bcb3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,7 +55,17 @@ as a separate job in `checks.yml`. Project invariants that clippy cannot express live in `.opengrep/agentx-ifstack-rules.yaml`. Run `scripts/opengrep-scan.sh` to check the source and `scripts/opengrep-test.sh` to check the rules themselves. Every rule needs a fixture in `.opengrep/tests/`, which the packaging -policy tests enforce. See `.opengrep/README.md` for why the filename matters. +policy tests enforce. + +These run from a local pre-commit hook, never in CI: CodeRabbit skips its own opengrep pass +when it sees opengrep in the workflows. Install with `pre-commit install --install-hooks`. +The hooks are filtered so a commit touching neither `src/` nor the rules costs nothing. See +`.opengrep/README.md` for that and for why the filename matters. + +`zizmor` audits the workflows themselves (permissions, injection, unpinned actions) in the +`Workflow audit` CI job. Run it locally with `uvx --native-tls zizmor .`, which is the +scope the CI job audits: the repo root, so `dependabot.yml` is included, not only +`.github/workflows/`. Export `GH_TOKEN` to add the online audits CI also runs. `rust-toolchain.toml` pins the toolchain, but a `RUSTUP_TOOLCHAIN` environment variable overrides it. Check that variable before blaming a build failure on the code. @@ -153,3 +163,21 @@ Every interface also yields the two zero-index boundary rows RFC 2863 requires. - net-snmp `local/snmp-bridge-mib`, Perl AgentX subagent reading `/sys/class/net`, BRIDGE-MIB only. - `snmp_rust_agent`, `sunt`. + +## Agent skills + +### Issue tracker + +Issues live as GitHub issues in this repo, driven through the `gh` CLI. External pull +requests are treated as a request surface and triaged alongside issues. See +`docs/agents/issue-tracker.md`. + +### Triage labels + +The five canonical roles, each label string equal to its name. See +`docs/agents/triage-labels.md`. + +### Domain docs + +Single-context: `CONTEXT.md` and `docs/adr/` at the repo root. Neither exists yet, and the +skills proceed silently rather than scaffolding them. See `docs/agents/domain.md`. diff --git a/README.md b/README.md index e2fb91d..8ac3bc7 100644 --- a/README.md +++ b/README.md @@ -243,7 +243,8 @@ cargo fmt --check ``` `python3 packaging/test_policy.py` checks the release gate, push triggers, and -service restart policy. It requires PyYAML 6.0.3. The shared checks workflow runs it. +service restart policy. It requires PyYAML 6.0.3, pre-commit 4.5.1, `gh`, `jq`, and Bash. +The shared checks workflow runs it. `tests/session.rs` runs the actual binary against a UnixListener. The master uses real AgentX PDUs. A fixture executable supplies `ip` output without diff --git a/docs/agents/domain.md b/docs/agents/domain.md new file mode 100644 index 0000000..3524904 --- /dev/null +++ b/docs/agents/domain.md @@ -0,0 +1,51 @@ +# Domain Docs + +How the engineering skills should consume this repo's domain documentation when exploring the codebase. + +## Before exploring, read these + +- **`CONTEXT.md`** at the repo root, or +- **`CONTEXT-MAP.md`** at the repo root if it exists: it points at one `CONTEXT.md` per context. Read each one relevant to the topic. +- **`docs/adr/`**: read ADRs that touch the area you're about to work in. In multi-context repos, also check `src//docs/adr/` for context-scoped decisions. + +If any of these files don't exist, **proceed silently**. Don't flag their absence; don't suggest creating them upfront. The `/domain-modeling` skill (reached via `/grill-with-docs` and `/improve-codebase-architecture`) creates them lazily when terms or decisions actually get resolved. + +## File structure + +Single-context repo (most repos): + +``` +/ +├── CONTEXT.md +├── docs/adr/ +│ ├── 0001-event-sourced-orders.md +│ └── 0002-postgres-for-write-model.md +└── src/ +``` + +Multi-context repo (presence of `CONTEXT-MAP.md` at the root): + +``` +/ +├── CONTEXT-MAP.md +├── docs/adr/ ← system-wide decisions +└── src/ + ├── ordering/ + │ ├── CONTEXT.md + │ └── docs/adr/ ← context-specific decisions + └── billing/ + ├── CONTEXT.md + └── docs/adr/ +``` + +## Use the glossary's vocabulary + +When your output names a domain concept (in an issue title, a refactor proposal, a hypothesis, a test name), use the term as defined in `CONTEXT.md`. Don't drift to synonyms the glossary explicitly avoids. + +If the concept you need isn't in the glossary yet, that's a signal: either you're inventing language the project doesn't use (reconsider) or there's a real gap (note it for `/domain-modeling`). + +## Flag ADR conflicts + +If your output contradicts an existing ADR, surface it explicitly rather than silently overriding: + +> _Contradicts ADR-0007 (event-sourced orders), but worth reopening because…_ diff --git a/docs/agents/issue-tracker.md b/docs/agents/issue-tracker.md new file mode 100644 index 0000000..6825009 --- /dev/null +++ b/docs/agents/issue-tracker.md @@ -0,0 +1,45 @@ +# Issue tracker: GitHub + +Issues and specs for this repo live as GitHub issues. Use the `gh` CLI for all operations. + +## Conventions + +- **Create an issue**: `gh issue create --title "..." --body "..."`. Use a heredoc for multi-line bodies. +- **Read an issue**: `gh issue view --json number,title,body,labels,comments --jq '...'`. `--comments` prints human-readable output, so it cannot feed `jq`; use `--json` for structured reads. +- **List issues**: `gh issue list --state open --json number,title,body,labels,comments --jq '[.[] | {number, title, body, labels: [.labels[].name], comments: [.comments[].body]}]'` with appropriate `--label` and `--state` filters. +- **Comment on an issue**: `gh issue comment --body "..."` +- **Apply / remove labels**: `gh issue edit --add-label "..."` / `--remove-label "..."` +- **Close**: `gh issue close --comment "..."` + +Infer the repo from `git remote -v`; `gh` does this automatically when run inside a clone. + +## Pull requests as a triage surface + +**PRs as a request surface: yes.** _(External pull requests are treated as feature requests and run through the same labels and states as issues; `/triage` reads this flag. Set to `no` to exclude them.)_ + +When set to `yes`, PRs run through the same labels and states as issues, using the `gh pr` equivalents: + +- **Read a PR**: `gh pr view --comments` and `gh pr diff ` for the diff. +- **List external PRs for triage**: `gh pr list --json` has no `authorAssociation` field and fails with "Unknown JSON field". Use the REST endpoint, which exposes `author_association`: `gh api "repos///pulls?state=open" --paginate --slurp | jq '[.[][] | select(.author_association | IN("CONTRIBUTOR","FIRST_TIME_CONTRIBUTOR","NONE")) | {number, title, author: .user.login, author_association}]'` (drop `OWNER`/`MEMBER`/`COLLABORATOR`). +- **Comment / label / close**: `gh pr comment`, `gh pr edit --add-label`/`--remove-label`, `gh pr close`. + +GitHub shares one number space across issues and PRs, so a bare `#42` may be either: resolve with `gh pr view 42` and fall back to `gh issue view 42`. + +## When a skill says "publish to the issue tracker" + +Create a GitHub issue. + +## When a skill says "fetch the relevant ticket" + +Run `gh pr view --comments` and fall back to `gh issue view --comments`. + +## Wayfinding operations + +Used by `/wayfinder`. The **map** is a single issue with **child** issues as tickets. + +- **Map**: a single issue labelled `wayfinder:map`, holding the Notes / Decisions-so-far / Fog body. `gh issue create --label wayfinder:map`. +- **Child ticket**: an issue linked to the map as a GitHub sub-issue (`gh api` on the sub-issues endpoint). Where sub-issues aren't enabled, add the child to a task list in the map body and put `Part of #` at the top of the child body. Labels: `wayfinder:` (`research`/`prototype`/`grilling`/`task`). Once claimed, the ticket is assigned to the driving dev. +- **Blocking**: GitHub's **native issue dependencies**, the canonical, UI-visible representation. Add an edge with `gh api --method POST repos///issues//dependencies/blocked_by -F issue_id=`, where `` is the blocker's numeric **database id** (`gh api repos///issues/ --jq .id`, _not_ the `#number` or `node_id`). GitHub reports `issue_dependencies_summary.blocked_by` (open blockers only, the live gate). Where dependencies aren't available, fall back to a `Blocked by: #, #` line at the top of the child body. A ticket is unblocked when every blocker is closed. +- **Frontier query**: read the map's children in order with `gh issue view --json subIssues`, then check each child's `gh issue view --json state,blockedBy,assignees,body`; skip any that is closed, has an open entry in `blockedBy`, or has an assignee. The first eligible child in map order wins. Do not use `gh issue list`, which returns the 30 most recent issues by default and does not preserve map order. Where sub-issues are unavailable, walk the task list in the map body in the same order. Read each child's `body` regardless of how the children were listed: dependencies and sub-issues are separate features, so a repository can list children natively while recording blockers as a `Blocked by: #` line, and such a child has an empty `blockedBy`, so treat an open issue named in `body` as a blocker. +- **Claim**: `gh issue edit --add-assignee @me`, the session's first write. +- **Resolve**: `gh issue comment --body ""`, then `gh issue close `, then append a context pointer (gist + link) to the map's Decisions-so-far. diff --git a/docs/agents/triage-labels.md b/docs/agents/triage-labels.md new file mode 100644 index 0000000..2ff1f28 --- /dev/null +++ b/docs/agents/triage-labels.md @@ -0,0 +1,18 @@ +# Triage Labels + +The skills speak in terms of five canonical triage roles. This file maps those roles to the actual label strings used in this repo's issue tracker. + +| Label in mattpocock/skills | Label in our tracker | Meaning | +| -------------------------- | -------------------- | ---------------------------------------- | +| `needs-triage` | `needs-triage` | Maintainer needs to evaluate this issue | +| `needs-info` | `needs-info` | Waiting on reporter for more information | +| `ready-for-agent` | `ready-for-agent` | Fully specified, ready for an AFK agent | +| `ready-for-human` | `ready-for-human` | Requires human implementation | +| `wontfix` | `wontfix` | Will not be actioned | + +When a skill mentions a role (e.g. "apply the AFK-ready triage label"), use the corresponding label string from this table. + +Edit the right-hand column to match whatever vocabulary you actually use. + +`wontfix` already existed in this repo, so it is reused rather than recreated. The other +four were created for these roles. diff --git a/packaging/test_policy.py b/packaging/test_policy.py index 8a42587..4dce84e 100644 --- a/packaging/test_policy.py +++ b/packaging/test_policy.py @@ -2,15 +2,29 @@ import ast import configparser +import contextlib +import functools +import json +import os import re +import shlex import shutil import subprocess +import sys import tempfile +import threading import unittest +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer from pathlib import Path +from urllib.parse import parse_qs, urlsplit import tomllib import yaml +from pre_commit.clientlib import load_config +from pre_commit.commands.run import Classifier +from pre_commit.hook import Hook +from pre_commit.prefix import Prefix +from pre_commit.repository import _hook ROOT = Path(__file__).resolve().parent.parent @@ -62,6 +76,130 @@ def workflow(name): ) +def effective_permissions(owner, inherited): + if "permissions" not in owner: + return inherited + declared = owner["permissions"] + if isinstance(declared, str): + return {"*": declared.removesuffix("-all")} + return dict(declared or {}) + + +INLINE_CODE = re.compile(r"(`+)(?!`)(.+?)(?= len(marker) and not fence[2].strip()): + spans.append("".join(fenced)) + fenced.clear() + marker = None + else: + fenced.append(line) + assert marker is None, f"{path}: unclosed code fence" + inline_spans() + return spans + + +def documented_shell_commands(path): + """Yield shell token lists, preserving quotes and splitting shell operators.""" + shell_parts = re.compile(r"""'[^']*'|"(?:\\.|[^"\\])*"|\\.|\#[^\n]*|[;&|()\n]+""") + for span in markdown_code_spans(path): + span = span.replace("\\\n", "") + start = 0 + for part in shell_parts.finditer(span): + if part[0][0] in ";&|()\n": + words = shlex.split(span[start:part.start()], comments=True) + if words: + yield words + start = part.end() + words = shlex.split(span[start:], comments=True) + if words: + yield words + + +@functools.cache +def gh_json_fields(group, subcommand): + """Field names gh accepts for `gh --json`, read from gh itself.""" + with tempfile.TemporaryDirectory() as config: + probe = subprocess.run( + ["gh", group, subcommand, "--json"], + capture_output=True, text=True, timeout=30, check=False, + env={**os.environ, "GH_TOKEN": "", "GH_CONFIG_DIR": config}, + ) + output = probe.stdout + probe.stderr + if "comma-separated fields" not in output: + raise AssertionError( + f"gh {group} {subcommand} --json did not list its fields: {output}" + ) + return frozenset( + line.strip() for line in output.splitlines() if line.startswith(" ") + ) + + +def documented_gh_json_commands(path): + """Yield (group, subcommand, fields) for each documented `gh ... --json` command.""" + yield from _gh_json_commands(documented_shell_commands(path)) + + +def documented_gh_json_commands_in(text): + """The same, for one Markdown fragment rather than a whole document.""" + yield from _gh_json_commands( + shlex.split(span) for span in INLINE_CODE.findall(text) for span in [span[1]] + ) + + +def _gh_json_commands(commands): + for words in commands: + if "gh" not in words: + continue + arguments = words[words.index("gh") + 1:] + if len(arguments) < 2: + continue + group, subcommand = arguments[:2] + _, options = shell_options(arguments[2:], {"--json"}) + for option, value in options: + if option == "--json" and value: + yield group, subcommand, value.split(",") + + +def shell_options(arguments, value_options): + """Return positional tokens and ordered option/value pairs for a CLI schema.""" + positional = [] + options = [] + words = iter(arguments) + for word in words: + if word == "--": + positional.extend(words) + break + if not word.startswith("-") or word == "-": + positional.append(word) + continue + option, equals, value = word.partition("=") + if not equals: + value = next(words, None) if option in value_options else None + options.append((option, value)) + return positional, options + + class PackagingPolicyTests(unittest.TestCase): def test_releases_come_from_main_and_not_from_a_pushed_tag(self): """semantic-release creates the tag, so a tag trigger would double-fire.""" @@ -115,6 +253,22 @@ def test_semantic_release_writes_the_manifest_and_syncs_the_rest(self): self.assertIs(release["major_on_zero"], False) self.assertIs(release["allow_zero_version"], True) + def test_a_merge_commit_subject_can_bump_the_version(self): + """Pull requests merge with merge_commit_title = PR_TITLE, so the conventional + subject lives on the merge commit. python-semantic-release ignores merge commits + by default, which silently drops that subject and bumps from the branch commits. + + Measured on a scratch repository tagged v0.1.0, with a `fix:` on the branch and a + `feat:` merge subject: the default yields 0.1.1, this setting yields 0.2.0. + """ + config = tomllib.loads((ROOT / "pyproject.toml").read_text()) + parser = config["tool"]["semantic_release"]["commit_parser_options"] + self.assertIs( + parser.get("ignore_merge_commits"), + False, + "merge commits carry the conventional subject here, so they must be parsed", + ) + def test_the_release_uploads_both_package_formats(self): """`version` builds dist/ and creates the release but uploads nothing from it. @@ -336,12 +490,228 @@ def test_third_party_actions_are_pinned_to_full_commit_shas(self): for reference in uses_references( yaml.load(path.read_text(), Loader=yaml.BaseLoader) ): - if reference.startswith("./"): - continue # a local reusable workflow is versioned with this repo + # Both local forms are versioned with this repo, so neither takes a SHA. + # "$/" is GitHub's self-repository syntax, which is itself a pinning form. + if reference.startswith(("./", "$/")): + continue if not PINNED_ACTION.match(reference): unpinned.append(f"{path.name} {reference}") self.assertEqual(unpinned, [], "third-party actions must be pinned to a SHA") + def test_ci_leaves_opengrep_to_coderabbit_and_the_local_hook(self): + """CodeRabbit skips its own opengrep pass when it sees opengrep in the workflows. + + Running it in CI therefore replaces CodeRabbit's broad coverage with this repo's + two narrow rules, which is a straight loss. The ruleset runs from a local + pre-commit hook instead, so both the custom rules and CodeRabbit's packs apply. + """ + for path in sorted(workflow_paths()): + text = path.read_text() + self.assertNotIn( + "opengrep", + text.lower(), + f"{path.name}: opengrep in a workflow suppresses CodeRabbit's own pass; " + "run it from the local pre-commit hook instead", + ) + + config = ROOT / ".pre-commit-config.yaml" + self.assertTrue(config.exists(), "the local hook config is missing") + config_data = yaml.safe_load(config.read_text()) + resolved_config = load_config(str(config)) + self.assertIn("pre-commit", resolved_config["default_install_hook_types"]) + resolved_hooks = { + hook["id"]: Hook.create( + repo["repo"], Prefix(str(ROOT)), _hook(hook, root_config=resolved_config) + ) + for repo in resolved_config["repos"] + for hook in repo["hooks"] + } + hooks = [ + hook for repo in config_data["repos"] for hook in repo.get("hooks", []) + ] + by_id = {h.get("id"): h for h in hooks} + # Read the paths from disk rather than sampling, so a filter narrowed to one file + # cannot pass while real files silently lose coverage. + sources = sorted(str(f.relative_to(ROOT)) for f in (ROOT / "src").rglob("*.rs")) + ruleset = sorted( + str(f.relative_to(ROOT)) for f in (ROOT / ".opengrep").rglob("*") if f.is_file() + ) + self.assertTrue(sources and ruleset, "expected source and ruleset files on disk") + + for hook_id, entry, must_match, must_skip in ( + ("opengrep", "scripts/opengrep-scan.sh", sources + ruleset, []), + ("opengrep-rule-tests", "scripts/opengrep-test.sh", ruleset, sources), + ): + hook = by_id.get(hook_id) + self.assertIsNotNone(hook, f"the {hook_id} hook is missing") + # A hook can be neutered without touching its files filter. + self.assertEqual(hook.get("entry"), entry, f"{hook_id} runs the wrong command") + self.assertEqual(hook.get("language"), "script", f"{hook_id} must run the script") + self.assertIsNone(hook.get("exclude"), f"{hook_id} must not exclude its inputs") + resolved = resolved_hooks[hook_id] + self.assertFalse(resolved.pass_filenames, f"{hook_id} must scan all inputs") + self.assertIn( + "pre-commit", + resolved.stages, + f"{hook_id} must run at the pre-commit stage", + ) + self.assertTrue((ROOT / entry).is_file(), f"{entry} does not exist") + self.assertTrue(os.access(ROOT / entry, os.X_OK), f"{entry} is not executable") + pattern = hook.get("files") + self.assertTrue(pattern, f"hook {hook_id} needs a files filter") + for candidate in must_match: + self.assertRegex(candidate, pattern, f"{hook_id} would skip {candidate}") + for candidate in must_skip: + self.assertNotRegex(candidate, pattern, f"{hook_id} runs for {candidate}") + with contextlib.chdir(ROOT): + selected = set( + Classifier.from_config( + sources + ruleset, + resolved_config["files"], + resolved_config["exclude"], + ).filenames_for_hook(resolved) + ) + self.assertTrue( + set(must_match) <= selected, + f"{hook_id} skips {set(must_match) - selected}", + ) + self.assertFalse(set(must_skip) & selected, f"{hook_id} selects excluded inputs") + + def test_a_called_workflow_never_outranks_its_callers(self): + """A reusable workflow's job cannot exceed the permissions of the job calling it. + + GitHub refuses the run rather than trimming the request. Resolve effective + permissions on both sides, because a job inherits the workflow's top-level block, + and compare access levels: a callee may ask for less than the caller grants, but + never more. + """ + levels = {"none": 0, "read": 1, "write": 2} + + def level_of(perms, scope): + return levels[perms.get(scope, perms.get("*", "none"))] + + callers = {} + for path in sorted(workflow_paths()): + data = workflow(path.name) + top = effective_permissions(data, {}) + for job in data.get("jobs", {}).values(): + target = str(job.get("uses", "")) + if not target.startswith(("./", "$/")): + continue + callers.setdefault(target.rsplit("/", 1)[-1], []).append( + (path.name, effective_permissions(job, top)) + ) + + for name, grants in sorted(callers.items()): + callee = workflow(name) + self.assertIn("permissions", callee, f"{name} declares no permissions") + callee_top = effective_permissions(callee, {}) + for job_name, job in callee.get("jobs", {}).items(): + wanted = effective_permissions(job, callee_top) + scopes = set(wanted).union(*(g for _, g in grants)) + for scope in sorted(scopes): + need = level_of(wanted, scope) + if need == 0: + continue + for caller_name, granted in grants: + self.assertGreaterEqual( + level_of(granted, scope), + need, + f"{name} job {job_name} requests {scope} level {need}, but " + f"{caller_name} grants level {level_of(granted, scope)}", + ) + + def test_the_workflows_are_audited_by_zizmor(self): + """Workflow permissions and injection risks are not covered by the other gates.""" + steps = [ + step + for job in workflow("checks.yml")["jobs"].values() + for step in job.get("steps", []) + ] + uses = " ".join(str(step.get("uses", "")) for step in steps) + self.assertIn("zizmor", uses, "no job audits the workflows with zizmor") + audits = [ + (job, step) + for job in workflow("checks.yml")["jobs"].values() + for step in job.get("steps", []) + if str(step.get("uses", "")).startswith("zizmorcore/zizmor-action@") + ] + self.assertTrue( + any( + "if" not in job + and "if" not in step + and job.get("continue-on-error", "false") == "false" + and step.get("continue-on-error", "false") == "false" + for job, step in audits + ), + "zizmor must run without conditions and propagate failures", + ) + + def test_the_documented_zizmor_command_matches_ci(self): + """A narrower local command passes while CI fails. + + The documented command audited `.github/workflows/` while the action defaults to + the repo root, so `dependabot.yml` was never audited locally and its three + cooldown findings only appeared in CI. + """ + step = next( + step + for job in workflow("checks.yml")["jobs"].values() + for step in job.get("steps", []) + if str(step.get("uses", "")).startswith("zizmorcore/zizmor-action@") + ) + options = step.get("with") or {} + # Defaults from the action's own action.yml at the pinned SHA. + expected_inputs = sorted(str(options.get("inputs", ".")).split()) + expected_persona = str(options.get("persona", "regular")) + + invocations = [] + for name in ("CLAUDE.md", "README.md"): + for words in documented_shell_commands(ROOT / name): + index = next( + ( + position + for position, word in enumerate(words) + if word == "zizmor" or word.startswith("zizmor@") + ), + None, + ) + if index is None or len(words) == 1: + continue # prose naming the tool, not a command + invocations.append((name, words, index)) + self.assertTrue(invocations, "no documented zizmor command to check") + + for name, words, index in invocations: + arguments = words[index + 1 :] + paths, parsed = shell_options(arguments, {"--persona", "--collect"}) + paths.sort() + self.assertEqual( + paths, + expected_inputs, + f"{name}: the documented zizmor command audits {paths}, " + f"but the CI job audits {expected_inputs}", + ) + persona = "regular" + collection = [] + for option, value in parsed: + if option == "--persona": + persona = value + elif option in ("--pedantic", "-p"): + persona = "pedantic" + elif option == "--collect": + self.assertIsNotNone(value, "--collect requires a value") + collection.extend(value.split(",")) + self.assertEqual( + collection or ["default"], ["default"], + f"{name}: documented collection must match CI's default collection", + ) + self.assertEqual( + persona, + expected_persona, + f"{name}: the documented zizmor command uses persona {persona}, " + f"but the CI job uses {expected_persona}", + ) + def test_no_workflow_runs_twice_for_one_push(self): """push on every branch plus pull_request runs every job twice on a PR branch. @@ -421,12 +791,9 @@ def test_the_custom_ruleset_adds_to_coderabbit_instead_of_replacing_it(self): fixture.exists(), f"rule {rule['id']} has no rule-test fixture" ) - steps = workflow("checks.yml")["jobs"]["rules"]["steps"] - commands = "\n".join(str(step.get("run", "")) for step in steps) - self.assertIn("opengrep-test.sh", commands, "CI must run the rule-tests") - self.assertIn("opengrep-scan.sh", commands, "CI must run the ruleset") - # The binary is fetched over the network, so pin it by digest. - self.assertIn("sha256sum -c -", commands, "pin the opengrep binary by checksum") + # Where the ruleset actually runs is asserted by + # test_ci_leaves_opengrep_to_coderabbit_and_the_local_hook: the pre-commit hook, + # never CI, because CodeRabbit skips its own pass when it sees opengrep there. def test_package_scripts_use_private_temporary_files(self): """A predictable temporary path lets a local user redirect a root-run write.""" @@ -519,5 +886,395 @@ def test_the_sync_script_makes_each_replacement_durable(self): "call sync_parent before the early return, so a retry repairs a failed sync", ) + def test_documented_gh_json_fields_exist(self): + """gh rejects an unknown --json field, so a wrong one makes the skill unusable. + + `gh pr list --json authorAssociation` shipped here once and fails with + "Unknown JSON field". gh validates the names locally, before auth and before + any request, and an empty --json prints the accepted set for a subcommand. + """ + documented = [] + for path in sorted((ROOT / "docs" / "agents").glob("*.md")): + for words in documented_shell_commands(path): + if "gh" not in words: + continue + arguments = words[words.index("gh") + 1:] + if len(arguments) < 2: + continue + group, subcommand = arguments[:2] + _, options = shell_options(arguments[2:], {"--json"}) + for option, value in options: + if option == "--json" and value: + documented.append( + (path.name, group, subcommand, value.split(",")) + ) + self.assertTrue(documented, "expected documented gh --json commands") + for name, group, subcommand, fields in documented: + for field in fields: + self.assertIn( + field, + gh_json_fields(group, subcommand), + f"{name}: gh {group} {subcommand} has no --json field {field!r}", + ) + + def test_documented_procedures_fetch_the_fields_they_read(self): + """A procedure that names a field in prose must request it in its own command. + + The frontier query listed `state,blockedBy,assignees` and then told the reader to + add `body`, so the documented command could not detect a `Blocked by:` fallback. + """ + incomplete = [] + for path in sorted((ROOT / "docs" / "agents").glob("*.md")): + for item in re.split(r"^(?=- )", path.read_text(), flags=re.MULTILINE): + requested = set() + accepted = set() + for group, subcommand, fields in documented_gh_json_commands_in(item): + requested.update(fields) + accepted |= gh_json_fields(group, subcommand) + mentioned = { + match[2] for match in INLINE_CODE.finditer(item) + } & (accepted - requested) + incomplete.extend( + f"{path.name}: {field!r} is read in prose but no --json list " + "in the same step requests it" + for field in sorted(mentioned) + ) + self.assertEqual(incomplete, []) + + +class GuardRegressionTests(unittest.TestCase): + def setUp(self): + global ROOT + original_root = ROOT + temporary = tempfile.TemporaryDirectory() + self.addCleanup(temporary.cleanup) + self.root = Path(temporary.name) + for name in (".github", ".opengrep", "src", "scripts", "docs"): + shutil.copytree(ROOT / name, self.root / name) + for name in (".pre-commit-config.yaml", "CLAUDE.md", "README.md"): + shutil.copy2(ROOT / name, self.root) + self.addCleanup(setattr, sys.modules[__name__], "ROOT", original_root) + ROOT = self.root + self.addCleanup(os.chdir, Path.cwd()) + os.chdir(ROOT) + self.policy = PackagingPolicyTests() + + def mutate(self, name, change): + path = ROOT / name + data = yaml.safe_load(path.read_text()) + change(data) + path.write_text(yaml.safe_dump(data)) + + def reject_hook(self, change): + self.mutate(".pre-commit-config.yaml", change) + with self.assertRaises(AssertionError): + self.policy.test_ci_leaves_opengrep_to_coderabbit_and_the_local_hook() + + def test_hook_rejects_effective_file_filters(self): + path = ROOT / ".pre-commit-config.yaml" + baseline = path.read_text() + for owner, field, value in ( + ("hook", "types", ["python"]), + ("hook", "types_or", ["python"]), + ("hook", "exclude_types", ["rust"]), + ("global", "exclude", ".*"), + ("global", "files", r"\.py$"), + ): + with self.subTest(owner=owner, field=field): + path.write_text(baseline) + + def change(config, owner=owner, field=field, value=value): + target = config if owner == "global" else config["repos"][0]["hooks"][1] + target[field] = value + + self.reject_hook(change) + + def test_hook_rejects_passed_filenames(self): + self.reject_hook( + lambda config: config["repos"][0]["hooks"][1].update(pass_filenames=True) + ) + + def test_hook_rejects_missing_install_stage(self): + self.reject_hook( + lambda config: config.update(default_install_hook_types=["pre-push"]) + ) + + def test_hook_accepts_effective_defaults(self): + def change(config): + config.pop("default_install_hook_types") + config.pop("default_stages") + config["repos"][0]["hooks"][1]["types_or"] = ["rust", "yaml", "markdown"] + + self.mutate(".pre-commit-config.yaml", change) + self.policy.test_ci_leaves_opengrep_to_coderabbit_and_the_local_hook() + + def test_permissions_reject_wildcard_increases(self): + for caller, callee in ( + ("read-all", "write-all"), + ({}, "read-all"), + ({"contents": "write"}, "read-all"), + ): + with self.subTest(caller=caller, callee=callee): + for name, permissions in ( + ("ci.yml", caller), ("release.yml", caller), ("checks.yml", callee) + ): + self.mutate( + f".github/workflows/{name}", + lambda data, permissions=permissions: data.update( + permissions=permissions + ), + ) + with self.assertRaisesRegex(AssertionError, "checks.yml"): + self.policy.test_a_called_workflow_never_outranks_its_callers() + + def test_permissions_accept_wildcard_reductions(self): + for name in ("ci.yml", "release.yml"): + self.mutate( + f".github/workflows/{name}", + lambda data: data.update(permissions="write-all"), + ) + self.mutate( + ".github/workflows/checks.yml", + lambda data: data.update(permissions="read-all"), + ) + self.policy.test_a_called_workflow_never_outranks_its_callers() + + def test_audit_rejects_disabled_or_nonblocking_execution(self): + path = ROOT / ".github/workflows/checks.yml" + baseline = path.read_text() + for owner, field, value in ( + ("job", "if", "false"), + ("job", "if", "${{ github.event_name == 'push' }}"), + ("step", "if", "${{ false }}"), + ("job", "continue-on-error", True), + ("step", "continue-on-error", True), + ): + with self.subTest(owner=owner, field=field, value=value): + path.write_text(baseline) + + def change(data, owner=owner, field=field, value=value): + job = data["jobs"]["zizmor"] + target = job if owner == "job" else job["steps"][-1] + target[field] = value + + self.mutate(".github/workflows/checks.yml", change) + with self.assertRaises(AssertionError): + self.policy.test_the_workflows_are_audited_by_zizmor() + + def test_gh_json_guard_rejects_a_field_the_cli_lacks(self): + document = ROOT / "docs/agents/issue-tracker.md" + document.write_text( + document.read_text().replace( + "--json subIssues", "--json subIssues,authorAssociation" + ) + ) + with self.assertRaisesRegex(AssertionError, "authorAssociation"): + self.policy.test_documented_gh_json_fields_exist() + + def test_zizmor_guard_rejects_a_documented_command_narrower_than_ci(self): + baseline = (ROOT / "CLAUDE.md").read_text() + for replacement, expected in ( + ("uvx --native-tls zizmor .github/workflows/", "audits"), + ("uvx --native-tls zizmor --persona=pedantic .", "persona"), + ): + with self.subTest(replacement=replacement): + (ROOT / "CLAUDE.md").write_text( + baseline.replace("uvx --native-tls zizmor .", replacement) + ) + with self.assertRaisesRegex(AssertionError, expected): + self.policy.test_the_documented_zizmor_command_matches_ci() + (ROOT / "CLAUDE.md").write_text(baseline) + + # The guard follows the job, so widening CI alone must also fail. + self.mutate( + ".github/workflows/checks.yml", + lambda data: data["jobs"]["zizmor"]["steps"][-1].update( + {"with": {"advanced-security": False, "inputs": ".github .opengrep"}} + ), + ) + with self.assertRaisesRegex(AssertionError, "audits"): + self.policy.test_the_documented_zizmor_command_matches_ci() + + def test_ticket_fetch_resolves_pr_before_issue(self): + document = (ROOT / "docs/agents/issue-tracker.md").read_text() + instruction = document.split('## When a skill says "fetch the relevant ticket"')[1] + instruction = instruction.split("##", 1)[0] + self.assertIn( + "gh pr view --comments` and fall back to " + "`gh issue view --comments", instruction, + ) + + def test_field_completeness_guard_checks_prose_against_the_command(self): + path = ROOT / "docs/agents/issue-tracker.md" + baseline = path.read_text() + for old, new, expected in ( + # A field dropped from the command while the prose still reads it. + ("--json state,blockedBy,assignees,body", "--json state,blockedBy,assignees", "body"), + # A field named in prose that no command in the step requests. + ("has an open entry in `blockedBy`", "has an open entry in `blockedBy` or `milestone`", + "milestone"), + ): + with self.subTest(expected=expected): + self.assertIn(old, baseline) + path.write_text(baseline.replace(old, new)) + with self.assertRaisesRegex(AssertionError, expected): + self.policy.test_documented_procedures_fetch_the_fields_they_read() + + # A mention satisfied by a different command in the same step is not a defect, + # and a mention that is not a real field is not one either: the external-PR step + # names authorAssociation precisely to say gh does not have it. + path.write_text(baseline.replace( + "The first eligible child in map order wins.", + "The first eligible child in map order wins, read from `subIssues`.", + )) + self.policy.test_documented_procedures_fetch_the_fields_they_read() + path.write_text(baseline) + self.policy.test_documented_procedures_fetch_the_fields_they_read() + + def test_gh_json_guard_checks_complete_fields_in_every_command(self): + path = ROOT / "docs/agents/issue-tracker.md" + baseline = path.read_text() + for addition, field in ( + ('`gh issue view 1 --json=authorAssociation`', "authorAssociation"), + ('`gh issue view 1 --json "authorAssociation"`', "authorAssociation"), + ('`gh issue view 1 --json title_typo`', "title_typo"), + ('`gh issue view 1 --json \\\n authorAssociation`', "authorAssociation"), + ('```sh\ngh issue view 1 --json title\ngh issue view 1 --json authorAssociation\n```', "authorAssociation"), + ('~~~sh\ngh issue view 1 --json authorAssociation\n~~~', "authorAssociation"), + *( + (f'`gh issue view 1 --json title {operator} gh issue view 1 --json title_typo`', "title_typo") + for operator in (";", "&&", "||", "|", "&") + ), + ): + with self.subTest(addition=addition): + path.write_text(baseline + "\n" + addition + "\n") + with self.assertRaisesRegex(AssertionError, field): + self.policy.test_documented_gh_json_fields_exist() + + def test_zizmor_guard_checks_collection_and_persona_options(self): + path = ROOT / "CLAUDE.md" + baseline = path.read_text() + for option, expected in ( + ("--collect=workflows", "collect"), + ('--collect "workflows"', "collect"), + ("--pedantic", "persona"), + ("-p", "persona"), + ): + with self.subTest(option=option): + path.write_text(baseline.replace( + "uvx --native-tls zizmor .", f"uvx --native-tls zizmor {option} ." + )) + with self.assertRaisesRegex(AssertionError, expected): + self.policy.test_the_documented_zizmor_command_matches_ci() + + def test_zizmor_guard_checks_tilde_fences_in_another_document(self): + path = ROOT / "README.md" + path.write_text(path.read_text() + + "\n~~~sh\nuvx --native-tls zizmor .github/workflows/\n~~~\n") + with self.assertRaisesRegex(AssertionError, "audits"): + self.policy.test_the_documented_zizmor_command_matches_ci() + + def test_external_pr_query_reads_later_pages(self): + document = (ROOT / "docs/agents/issue-tracker.md").read_text() + command = re.search( + r'`(gh api "repos///pulls\?state=open"[^`]+)`', document + )[1] + pages = [] + + class Handler(BaseHTTPRequestHandler): + def do_GET(self): + page = int(parse_qs(urlsplit(self.path).query).get("page", ["1"])[0]) + pages.append(page) + records = [ + { + "number": number, + "title": "Example", + "user": {"login": "contributor"}, + "author_association": "MEMBER" if number < 30 else "CONTRIBUTOR", + } + for number in (range(1, 31) if page == 1 else [31]) + ] + self.send_response(200) + self.send_header("Content-Type", "application/json") + if page == 1: + self.send_header("Link", f'<{endpoint}&page=2>; rel="next"') + self.end_headers() + self.wfile.write(json.dumps(records).encode()) + + def log_message(self, *_args): + pass + + with ThreadingHTTPServer(("localhost", 0), Handler) as server: + endpoint = ( + f"http://localhost:{server.server_port}" + "/repos/example/project/pulls?state=open" + ) + command = command.replace( + '"repos///pulls?state=open"', shlex.quote(endpoint) + ) + thread = threading.Thread(target=server.serve_forever) + thread.start() + try: + result = subprocess.run( + ["bash", "-o", "pipefail", "-c", command], + capture_output=True, text=True, timeout=15, check=False, + env={ + **os.environ, + "GH_TOKEN": "placeholder", + "GH_ENTERPRISE_TOKEN": "placeholder", + "GH_CONFIG_DIR": str(ROOT / "gh-config"), + "GH_DEBUG": "", + }, + ) + finally: + server.shutdown() + thread.join() + self.assertEqual(result.returncode, 0, result.stderr) + records = json.loads(result.stdout) + self.assertEqual([record["number"] for record in records], [30, 31], result.stdout) + self.assertEqual(pages, [1, 2]) + + def test_external_pr_query_requires_pagination_and_one_json_value(self): + path = ROOT / "docs/agents/issue-tracker.md" + baseline = path.read_text() + for old, new in ( + ("--paginate --slurp", "--slurp"), + ("--paginate --slurp | jq '[.[][]", "--paginate --jq '[.[]"), + ): + with self.subTest(replacement=new): + self.assertIn(old, baseline) + path.write_text(baseline.replace(old, new)) + with self.assertRaises((AssertionError, json.JSONDecodeError)): + self.test_external_pr_query_reads_later_pages() + + def test_documented_commands_preserve_valid_quotes_and_continuations(self): + path = ROOT / "docs/agents/issue-tracker.md" + path.write_text( + path.read_text() + '\n`gh issue view 1 --json="title,body"`\n' + '`gh issue view 1 --json "title,body"`\n' + '~~~sh\ngh issue view 1 --json \\\n title,body\n~~~\n' + '```sh\ngh issue view 1 --json title # fields\n' + 'gh issue view 1 --json body\n```\n' + ) + self.policy.test_documented_gh_json_fields_exist() + path = ROOT / "README.md" + path.write_text(path.read_text() + + '\n~~~sh\nuvx --native-tls zizmor \\\n' + ' --persona "regular" --collect=default .\n~~~\n') + self.policy.test_the_documented_zizmor_command_matches_ci() + + def test_shell_commands_keep_quoted_operators_as_arguments(self): + path = ROOT / "commands.md" + path.write_text('````sh\ngh issue view 1 --json="title,body" ' + '--jq ".[] | .title"; printf "%s" "|"\n````\n') + commands = list(documented_shell_commands(path)) + self.assertEqual(commands, [ + ["gh", "issue", "view", "1", "--json=title,body", "--jq", ".[] | .title"], + ["printf", "%s", "|"], + ]) + self.assertEqual(shell_options(commands[0][3:], {"--json", "--jq"}), + (["1"], [("--json", "title,body"), ("--jq", ".[] | .title")])) + + if __name__ == "__main__": unittest.main() diff --git a/pyproject.toml b/pyproject.toml index 0ad2b68..1cba55e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -30,3 +30,7 @@ allowed_tags = ["feat", "fix", "docs", "style", "refactor", "perf", "test", "bui minor_tags = ["feat"] patch_tags = ["fix", "perf"] parse_squash_commits = true +# Pull requests merge with merge_commit_title = PR_TITLE, so the conventional subject sits +# on the merge commit. The default ignores merge commits, which dropped that subject: a +# feat: merge released a patch. Measured on a scratch repo, default 0.1.1 against 0.2.0. +ignore_merge_commits = false