From c8b3a7428082778bfae8a37135d4bb64eeb8fe5e Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 12 Sep 2026 07:38:36 +0200 Subject: [PATCH 1/8] docs: record the issue tracker, triage labels and domain doc layout The engineering skills read per-repo configuration from docs/agents/ rather than guessing. Without it they cannot tell whether to call gh, write markdown under .scratch, or follow some other workflow. Issues live as GitHub issues, matching the remote. External pull requests are treated as a request surface, so they run through the same labels and states as issues. Triage labels keep the five canonical role names. wontfix already existed in this repo and is reused rather than recreated; needs-triage, needs-info, ready-for-agent and ready-for-human were created for the remaining roles. Domain docs are single-context: one CONTEXT.md and docs/adr/ at the root. Neither exists yet, and the consumer rules say to proceed silently rather than scaffold them, so no empty files are added here. --- CLAUDE.md | 18 +++++++++++++ docs/agents/domain.md | 51 ++++++++++++++++++++++++++++++++++++ docs/agents/issue-tracker.md | 45 +++++++++++++++++++++++++++++++ docs/agents/triage-labels.md | 18 +++++++++++++ 4 files changed, 132 insertions(+) create mode 100644 docs/agents/domain.md create mode 100644 docs/agents/issue-tracker.md create mode 100644 docs/agents/triage-labels.md diff --git a/CLAUDE.md b/CLAUDE.md index aea541d..29b0156 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -153,3 +153,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/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..b076c2f --- /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 --comments`, filtering comments by `jq` and also fetching labels. +- **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 --state open --json number,title,body,labels,author,authorAssociation,comments` then keep only `authorAssociation` of `CONTRIBUTOR`, `FIRST_TIME_CONTRIBUTOR`, or `NONE` (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 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**: list the map's open children (`gh issue list --state open`, scoped to the map's sub-issues / task list), drop any with an open blocker (`issue_dependencies_summary.blocked_by > 0`, or an open issue in the `Blocked by` line) or an assignee; first in map order wins. +- **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. From 3e0dd618a61d5ee4cd84a97f3dcc2064c517f270 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 12 Sep 2026 07:57:41 +0200 Subject: [PATCH 2/8] fix: let a merge commit subject bump the version Pull requests merge with merge_commit_title = PR_TITLE, so the conventional subject sits on the merge commit itself. python-semantic-release ignores merge commits by default, so that subject was dropped and the bump came from the branch commits instead, where the strongest was fix:. That is why both releases so far under-bumped: PR #1 titled "feat: Initial baseline" released 0.0.2, and PR #4 titled "feat: attach the packages to the release and gate the dependency tree" released 0.0.3. Both merge commits carry two parents, which is exactly how the parser detects them: def is_merge_commit(commit): return len(commit.parents) > 1 Measured on a scratch repository tagged v0.1.0, with a fix: commit on the branch and a feat: merge subject: the default prints 0.1.1, and this setting prints 0.2.0. The alternative, always squash merging, was rejected because the repository allows merge commits and the title setting already puts the subject where it needs to be read. The test asserts the setting rather than re-running the release tool, because the check job installs no uv and a skipped test would guard nothing in CI. --- packaging/test_policy.py | 16 ++++++++++++++++ pyproject.toml | 4 ++++ 2 files changed, 20 insertions(+) diff --git a/packaging/test_policy.py b/packaging/test_policy.py index 8a42587..f5d1c89 100644 --- a/packaging/test_policy.py +++ b/packaging/test_policy.py @@ -115,6 +115,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. 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 From 8db02c31e157c92bfafcf2de9682a530605a44f4 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 12 Sep 2026 12:27:40 +0200 Subject: [PATCH 3/8] docs: correct the gh commands in the issue tracker skill Three commands in the vendored issue-tracker template did not work against the real CLI. Each replacement is verified with gh. `gh issue view --comments` prints human-readable text, so it cannot feed jq. Read structured fields with `--json` instead. `gh pr list --json authorAssociation` fails with "Unknown JSON field". The REST pulls endpoint exposes `author_association`, so external-PR triage uses `gh api` with `--paginate`, because a repository can hold more open PRs than one page returns. The wayfinder frontier query used `gh issue list`, which returns the 30 most recent issues and does not preserve map order. It now reads the map's ordered `subIssues`. It also reads each child's `body`: sub-issues and dependencies are separate GitHub features, so a repository can list children natively while recording blockers as a `Blocked by: #` line, which leaves `blockedBy` empty. --- docs/agents/issue-tracker.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/agents/issue-tracker.md b/docs/agents/issue-tracker.md index b076c2f..a5e0c65 100644 --- a/docs/agents/issue-tracker.md +++ b/docs/agents/issue-tracker.md @@ -5,7 +5,7 @@ Issues and specs for this repo live as GitHub issues. Use the `gh` CLI for all o ## Conventions - **Create an issue**: `gh issue create --title "..." --body "..."`. Use a heredoc for multi-line bodies. -- **Read an issue**: `gh issue view --comments`, filtering comments by `jq` and also fetching labels. +- **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 "..."` @@ -20,7 +20,7 @@ Infer the repo from `git remote -v`; `gh` does this automatically when run insid 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 --state open --json number,title,body,labels,author,authorAssociation,comments` then keep only `authorAssociation` of `CONTRIBUTOR`, `FIRST_TIME_CONTRIBUTOR`, or `NONE` (drop `OWNER`/`MEMBER`/`COLLABORATOR`). +- **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 --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`. @@ -40,6 +40,6 @@ Used by `/wayfinder`. The **map** is a single issue with **child** issues as tic - **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**: list the map's open children (`gh issue list --state open`, scoped to the map's sub-issues / task list), drop any with an open blocker (`issue_dependencies_summary.blocked_by > 0`, or an open issue in the `Blocked by` line) or an assignee; first in map order wins. +- **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`; 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`. Add `body` to the child query and treat an open issue named there 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. From c35b8bdd42aff734af835d2d9202b8af34118ad7 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 12 Sep 2026 12:27:57 +0200 Subject: [PATCH 4/8] ci: move opengrep to a pre-commit hook and audit the workflows with zizmor CodeRabbit skips its own opengrep pass when it sees opengrep in the workflows. The CI job therefore traded CodeRabbit's broad packs for this repo's two narrow rules, which is a loss. The ruleset now runs from a local pre-commit hook, so both apply. Both hooks carry a `files` filter, so a commit that touches neither `src/` nor the rules costs nothing. The freed job slot runs zizmor, which audits the workflows themselves for permissions, injection and unpinned actions. No other gate looks at them. It runs with `advanced-security: false`: the SARIF upload needs `security-events: write`, which a called workflow cannot hold unless every caller grants it, and the findings already gate the job. zizmor's `self-repository` findings are applied. `uses: ./.github/workflows/x` became `uses: $/.github/workflows/x`, the self-repository syntax, which is itself a form of pinning. New policy guards, each verified to fail against the unfixed tree: - no workflow mentions opengrep, and the hook config resolves through pre-commit's own loader to hooks that run the right script at the pre-commit stage over every source and ruleset file on disk. Resolving the config catches a hook neutered by `types`, `types_or`, `exclude_types` or a global `files`/`exclude`, none of which touch the `files` filter. - a called workflow never requests more permission than any caller grants, compared on effective permissions so a job that inherits a top-level block is read correctly. GitHub refuses such a run rather than trimming it. - the zizmor step runs unconditionally and propagates failures. - the external-PR query pages past the first response, exercised against a local HTTP server that emits a `Link: rel="next"` header. The regression tests run each guard against a mutated copy of the tree, so a guard that stops guarding fails. `packaging/test_policy.py` now needs pre-commit and gh. checks.yml and README.md declare that. --- .github/workflows/checks.yml | 32 ++- .github/workflows/ci.yml | 4 +- .github/workflows/release.yml | 2 +- .opengrep/README.md | 24 ++- .pre-commit-config.yaml | 27 +++ CLAUDE.md | 10 +- README.md | 3 +- packaging/test_policy.py | 367 +++++++++++++++++++++++++++++++++- 8 files changed, 429 insertions(+), 40 deletions(-) create mode 100644 .pre-commit-config.yaml 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 29b0156..84fd90c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,7 +55,15 @@ 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 .github/workflows/`. `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. diff --git a/README.md b/README.md index e2fb91d..32ee0f0 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, and `gh`. +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/packaging/test_policy.py b/packaging/test_policy.py index f5d1c89..f09e514 100644 --- a/packaging/test_policy.py +++ b/packaging/test_policy.py @@ -2,15 +2,28 @@ import ast import configparser +import contextlib +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 +75,15 @@ 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 {}) + + 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.""" @@ -352,12 +374,163 @@ 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_no_workflow_runs_twice_for_one_push(self): """push on every branch plus pull_request runs every job twice on a PR branch. @@ -437,12 +610,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.""" @@ -535,5 +705,186 @@ def test_the_sync_script_makes_each_replacement_durable(self): "call sync_parent before the early return, so a retry repairs a failed sync", ) +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) + shutil.copy2(ROOT / ".pre-commit-config.yaml", 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_external_pr_query_reads_later_pages(self): + document = (ROOT / "docs/agents/issue-tracker.md").read_text() + command = shlex.split(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 page == 1 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[2] = endpoint + thread = threading.Thread(target=server.serve_forever) + thread.start() + try: + result = subprocess.run( + 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 = [] + output = result.stdout.strip() + while output: + page, end = json.JSONDecoder().raw_decode(output) + records.extend(page) + output = output[end:].strip() + self.assertEqual([record["number"] for record in records], [31], result.stdout) + self.assertEqual(pages, [1, 2]) + + if __name__ == "__main__": unittest.main() From 2e338996adc3d8b2365bd88874da1f8d2442f807 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 12 Sep 2026 12:31:39 +0200 Subject: [PATCH 5/8] test: check every documented gh --json field against the real CLI Three review findings on the issue-tracker skill shared one root cause: a `gh` command in the document named a `--json` field the CLI does not have. `gh pr list --json authorAssociation` fails with "Unknown JSON field", so the skill was unusable at that step. The guard reads every `gh ... --json ` command in docs/agents/, asks that subcommand for its accepted field set, and asserts each documented field is in it. An empty `--json` prints the set, and gh validates the names before auth and before any request, so the check needs no token and no network. Verified red against the pre-fix document: it reports "gh pr list has no --json field 'authorAssociation'". --- packaging/test_policy.py | 50 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/packaging/test_policy.py b/packaging/test_policy.py index f09e514..faf8e30 100644 --- a/packaging/test_policy.py +++ b/packaging/test_policy.py @@ -705,6 +705,56 @@ 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 span in re.findall(r"`([^`]*--json[^`]*)`", path.read_text()): + command = re.search(r"\bgh\s+(\w[\w-]*)\s+(\w[\w-]*)", span) + fields = re.search(r"--json\s+([A-Za-z][A-Za-z,]*)", span) + if command and fields: + documented.append( + (path.name, command[1], command[2], fields[1].split(",")) + ) + self.assertTrue(documented, "expected documented gh --json commands") + + accepted = {} + with tempfile.TemporaryDirectory() as config: + for _, group, subcommand, _fields in documented: + if (group, subcommand) in accepted: + continue + 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 + self.assertIn( + "comma-separated fields", + output, + f"gh {group} {subcommand} --json did not list its fields: {output}", + ) + accepted[(group, subcommand)] = { + line.strip() for line in output.splitlines() if line.startswith(" ") + } + + for name, group, subcommand, fields in documented: + for field in fields: + self.assertIn( + field, + accepted[(group, subcommand)], + f"{name}: gh {group} {subcommand} has no --json field {field!r}", + ) + + class GuardRegressionTests(unittest.TestCase): def setUp(self): global ROOT From 6a503e556c151ad81fd38a8277bb7ae88127e1d1 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 12 Sep 2026 12:36:55 +0200 Subject: [PATCH 6/8] ci: audit the whole repo with zizmor, and give Dependabot a cooldown The zizmor job failed on its first CI run. The action audits the repository root by default, so it reads `.github/dependabot.yml`, which the documented local command never did: it passed `.github/workflows/`. Three `dependabot-cooldown` findings were invisible locally and only appeared in CI. Each ecosystem now waits 7 days before Dependabot proposes a new release, so a compromised version has time to be yanked before it reaches a PR here. This is zizmor's own fix, applied with `--fix=all`. The documented command becomes `uvx --native-tls zizmor .`, which reproduces the CI file set exactly: dependabot.yml, the four workflows, and .pre-commit-config.yaml. New guard, verified red against the previous command: the documented zizmor invocation must audit the same inputs and use the same persona as the CI job, resolved against the action's own defaults. It follows the job, so widening CI alone also fails it. --- .github/dependabot.yml | 6 +++ CLAUDE.md | 4 +- packaging/test_policy.py | 94 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 102 insertions(+), 2 deletions(-) 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/CLAUDE.md b/CLAUDE.md index 84fd90c..6e1bcb3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,7 +63,9 @@ The hooks are filtered so a commit touching neither `src/` nor the rules costs n `.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 .github/workflows/`. +`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. diff --git a/packaging/test_policy.py b/packaging/test_policy.py index faf8e30..75c13b7 100644 --- a/packaging/test_policy.py +++ b/packaging/test_policy.py @@ -531,6 +531,63 @@ def test_the_workflows_are_audited_by_zizmor(self): "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 span in re.findall(r"`([^`]*zizmor[^`]*)`", (ROOT / name).read_text()): + words = shlex.split(span) + 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 = sorted(word for word in arguments if not word.startswith("-")) + self.assertEqual( + paths, + expected_inputs, + f"{name}: the documented zizmor command audits {paths}, " + f"but the CI job audits {expected_inputs}", + ) + persona = "regular" + for position, word in enumerate(arguments): + if word.startswith("--persona="): + persona = word.split("=", 1)[1] + elif word == "--persona" and position + 1 < len(arguments): + persona = arguments[position + 1] + 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. @@ -764,7 +821,8 @@ def setUp(self): self.root = Path(temporary.name) for name in (".github", ".opengrep", "src", "scripts", "docs"): shutil.copytree(ROOT / name, self.root / name) - shutil.copy2(ROOT / ".pre-commit-config.yaml", self.root) + 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()) @@ -873,6 +931,40 @@ def change(data, owner=owner, field=field, value=value): 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_external_pr_query_reads_later_pages(self): document = (ROOT / "docs/agents/issue-tracker.md").read_text() command = shlex.split(re.search( From 6819217724e63eeafc9b281fb0f08fa5b901a0fb Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 12 Sep 2026 12:55:04 +0200 Subject: [PATCH 7/8] fix: parse documented commands once, and page the external-PR query correctly Four review rounds found 4, 5, 6 and 3 defects, every one in a guard and none in product code. Round 4's three were one mistake in two places: each guard wrote its own extractor for commands documented in Markdown, and each got it wrong differently. The gh guard missed `--json=value` and `--json "value"`, and truncated `title_typo` to the valid field `title`. The zizmor guard discarded every token starting with `-`, so `--collect=workflows`, which excludes dependabot.yml and reopens the exact hole that guard exists to close, passed. Both read only backtick spans, so a `~~~` fence was invisible. One extractor now serves both. It reads inline code and both fence forms, joins line continuations, splits at shell operators so a second command in one block is checked too, tokenizes with shlex, and normalizes `--opt=value` and `--opt value` to one shape. A field matches only as a complete token. Each guard is a rule over that structure rather than a regex over raw text. Two documented commands were wrong: `gh api --paginate` emits one JSON value per page, so the external-PR query returned output that no single `json.loads` could read. `--slurp` collects the pages into one array, but gh rejects it with `--jq` ("the `--slurp` option is not supported with `--jq` or `--template`"), so the filter moves to a separate `jq` and flattens one extra level with `.[][]`. "Fetch the relevant ticket" ran `gh issue view` unconditionally, ignoring the PR-first rule this same document states: a bare number can be either. The pagination guard now proves both properties it only appeared to prove. Its fixture puts a matching record on each page, so reading page 1 alone fails, and it parses with one `json.loads`, so dropping `--slurp` fails. It runs the command through `bash -o pipefail`, so a gh failure cannot hide behind jq. Policy tests 34 to 41. Every guard verified against a mutated tree: the mutations round 4 proved, plus quoting, continuation and operator cases. The tests now need `jq` and Bash, which README.md records. --- README.md | 2 +- docs/agents/issue-tracker.md | 4 +- packaging/test_policy.py | 228 ++++++++++++++++++++++++++++++----- 3 files changed, 204 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 32ee0f0..8ac3bc7 100644 --- a/README.md +++ b/README.md @@ -243,7 +243,7 @@ cargo fmt --check ``` `python3 packaging/test_policy.py` checks the release gate, push triggers, and -service restart policy. It requires PyYAML 6.0.3, pre-commit 4.5.1, and `gh`. +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 diff --git a/docs/agents/issue-tracker.md b/docs/agents/issue-tracker.md index a5e0c65..5260147 100644 --- a/docs/agents/issue-tracker.md +++ b/docs/agents/issue-tracker.md @@ -20,7 +20,7 @@ Infer the repo from `git remote -v`; `gh` does this automatically when run insid 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 --jq '[.[] | select(.author_association | IN("CONTRIBUTOR","FIRST_TIME_CONTRIBUTOR","NONE")) | {number, title, author: .user.login, author_association}]'` (drop `OWNER`/`MEMBER`/`COLLABORATOR`). +- **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`. @@ -31,7 +31,7 @@ Create a GitHub issue. ## When a skill says "fetch the relevant ticket" -Run `gh issue view --comments`. +Run `gh pr view --comments` and fall back to `gh issue view --comments`. ## Wayfinding operations diff --git a/packaging/test_policy.py b/packaging/test_policy.py index 75c13b7..04c8833 100644 --- a/packaging/test_policy.py +++ b/packaging/test_policy.py @@ -84,6 +84,75 @@ def effective_permissions(owner, inherited): return dict(declared or {}) +def markdown_code_spans(path): + """Extract inline code and fenced blocks in document order.""" + spans = [] + prose = [] + fenced = [] + marker = None + + def inline_spans(): + spans.extend(match[2] for match in re.finditer( + 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 + + +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.""" @@ -551,8 +620,7 @@ def test_the_documented_zizmor_command_matches_ci(self): invocations = [] for name in ("CLAUDE.md", "README.md"): - for span in re.findall(r"`([^`]*zizmor[^`]*)`", (ROOT / name).read_text()): - words = shlex.split(span) + for words in documented_shell_commands(ROOT / name): index = next( ( position @@ -568,7 +636,8 @@ def test_the_documented_zizmor_command_matches_ci(self): for name, words, index in invocations: arguments = words[index + 1 :] - paths = sorted(word for word in arguments if not word.startswith("-")) + paths, parsed = shell_options(arguments, {"--persona", "--collect"}) + paths.sort() self.assertEqual( paths, expected_inputs, @@ -576,11 +645,19 @@ def test_the_documented_zizmor_command_matches_ci(self): f"but the CI job audits {expected_inputs}", ) persona = "regular" - for position, word in enumerate(arguments): - if word.startswith("--persona="): - persona = word.split("=", 1)[1] - elif word == "--persona" and position + 1 < len(arguments): - persona = arguments[position + 1] + 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, @@ -771,13 +848,19 @@ def test_documented_gh_json_fields_exist(self): """ documented = [] for path in sorted((ROOT / "docs" / "agents").glob("*.md")): - for span in re.findall(r"`([^`]*--json[^`]*)`", path.read_text()): - command = re.search(r"\bgh\s+(\w[\w-]*)\s+(\w[\w-]*)", span) - fields = re.search(r"--json\s+([A-Za-z][A-Za-z,]*)", span) - if command and fields: - documented.append( - (path.name, command[1], command[2], fields[1].split(",")) - ) + 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") accepted = {} @@ -965,11 +1048,63 @@ def test_zizmor_guard_rejects_a_documented_command_narrower_than_ci(self): 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_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 = shlex.split(re.search( + command = re.search( r'`(gh api "repos///pulls\?state=open"[^`]+)`', document - )[1]) + )[1] pages = [] class Handler(BaseHTTPRequestHandler): @@ -981,7 +1116,7 @@ def do_GET(self): "number": number, "title": "Example", "user": {"login": "contributor"}, - "author_association": "MEMBER" if page == 1 else "CONTRIBUTOR", + "author_association": "MEMBER" if number < 30 else "CONTRIBUTOR", } for number in (range(1, 31) if page == 1 else [31]) ] @@ -1000,12 +1135,15 @@ def log_message(self, *_args): f"http://localhost:{server.server_port}" "/repos/example/project/pulls?state=open" ) - command[2] = endpoint + command = command.replace( + '"repos///pulls?state=open"', shlex.quote(endpoint) + ) thread = threading.Thread(target=server.serve_forever) thread.start() try: result = subprocess.run( - command, capture_output=True, text=True, timeout=15, check=False, + ["bash", "-o", "pipefail", "-c", command], + capture_output=True, text=True, timeout=15, check=False, env={ **os.environ, "GH_TOKEN": "placeholder", @@ -1018,15 +1156,51 @@ def log_message(self, *_args): server.shutdown() thread.join() self.assertEqual(result.returncode, 0, result.stderr) - records = [] - output = result.stdout.strip() - while output: - page, end = json.JSONDecoder().raw_decode(output) - records.extend(page) - output = output[end:].strip() - self.assertEqual([record["number"] for record in records], [31], result.stdout) + 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() From 09139b94fbb0b569c04d49abea21210a60fc7b8b Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Sat, 12 Sep 2026 13:47:37 +0200 Subject: [PATCH 8/8] fix: fetch body in the frontier child query, and guard prose against command The frontier query listed `state,blockedBy,assignees` and then told the reader to add `body`. An agent running the documented command as written never fetched `body`, so it could not see a `Blocked by: #` fallback line and would treat a blocked child as ready. The command now requests `body`, and the trailing instruction to add it is gone rather than left beside a command that already has it. The existing guard could not catch this: it checks that every documented `--json` field exists in the CLI, not that a documented procedure fetches what its own prose says it reads. The new guard closes that: within one Markdown step, any backticked token that is a real JSON field for a gh subcommand used in that step must appear in some `--json` list in the same step. A mention satisfied by another command in the step is fine, which is why `subIssues` on the map query does not trip the child query, and a token that is not a real field is ignored, which is why the external-PR step can name `authorAssociation` to say gh lacks it. The gh field probe moved into one cached helper. Both guards now read the accepted field set from the same place instead of each running its own probe. The first version of this guard passed against the unfixed document because it read match group 1, the backticks, instead of group 2, the content. It was inert and looked correct. The regression covers a field dropped from the command, a field named only in prose, and both negative cases above. Policy tests 41 to 43. --- docs/agents/issue-tracker.md | 2 +- packaging/test_policy.py | 130 +++++++++++++++++++++++++++-------- 2 files changed, 103 insertions(+), 29 deletions(-) diff --git a/docs/agents/issue-tracker.md b/docs/agents/issue-tracker.md index 5260147..6825009 100644 --- a/docs/agents/issue-tracker.md +++ b/docs/agents/issue-tracker.md @@ -40,6 +40,6 @@ Used by `/wayfinder`. The **map** is a single issue with **child** issues as tic - **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`; 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`. Add `body` to the child query and treat an open issue named there as a blocker. +- **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/packaging/test_policy.py b/packaging/test_policy.py index 04c8833..4dce84e 100644 --- a/packaging/test_policy.py +++ b/packaging/test_policy.py @@ -3,6 +3,7 @@ import ast import configparser import contextlib +import functools import json import os import re @@ -84,6 +85,9 @@ def effective_permissions(owner, inherited): return dict(declared or {}) +INLINE_CODE = re.compile(r"(`+)(?!`)(.+?)(? --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 = [] @@ -862,38 +909,38 @@ def test_documented_gh_json_fields_exist(self): (path.name, group, subcommand, value.split(",")) ) self.assertTrue(documented, "expected documented gh --json commands") - - accepted = {} - with tempfile.TemporaryDirectory() as config: - for _, group, subcommand, _fields in documented: - if (group, subcommand) in accepted: - continue - 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 - self.assertIn( - "comma-separated fields", - output, - f"gh {group} {subcommand} --json did not list its fields: {output}", - ) - accepted[(group, subcommand)] = { - line.strip() for line in output.splitlines() if line.startswith(" ") - } - for name, group, subcommand, fields in documented: for field in fields: self.assertIn( field, - accepted[(group, subcommand)], + 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): @@ -1057,6 +1104,33 @@ def test_ticket_fetch_resolves_pr_before_issue(self): "`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()