ci: vendor governance-enforce — this repo was never in the A_BLOCK ruleset - #84
ci: vendor governance-enforce — this repo was never in the A_BLOCK ruleset#84yakimoto wants to merge 3 commits into
Conversation
…never in the ruleset
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_aa33bf01-e471-4bdd-ab5e-0768a697b982) |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 44 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
ApprovabilityVerdict: Needs human review Unable to check for correctness in 0b8cf79. Multiple unresolved review comments identify potential bugs where this security gate could pass without actually scanning code (fork auth failures, false-green on force-pushes/branch creation). These concerns should be addressed before a required security check is merged. You can customize Macroscope's approvability policy. Learn more. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
PR Summary by QodoCI: add governance-enforce A_BLOCK diff-scoped secrets/path gate
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| - name: fetch governance enforcer (isolated install) | ||
| run: | | ||
| mkdir -p "$RUNNER_TEMP/gov" && cd "$RUNNER_TEMP/gov" | ||
| printf '@wave-av:registry=https://npm.pkg.github.com\n//npm.pkg.github.com/:_authToken=${NODE_AUTH_TOKEN}\n' > .npmrc | ||
| npm install @wave-av/governance@^0.4.4 --no-save --no-audit --no-fund |
There was a problem hiding this comment.
🔍 Fork PRs may not be able to authenticate to the private GitHub Packages registry
The install step authenticates to npm.pkg.github.com with secrets.GITHUB_TOKEN to pull @wave-av/governance. This repo is public and CONTRIBUTING.md instructs external contributors to fork before opening a PR. For pull_request runs originating from forks the token is read-only and package access depends on the package being explicitly linked/granted to this repository; if it is not, the isolated install will 401/403 and the enforce job will always fail for community PRs. Worth confirming before the check is added to the governance-a-block-enforce ruleset as required, otherwise fork contributions become unmergeable.
Was this helpful? React with 👍 or 👎 to provide feedback.
Code Review by Qodo
1. Fork PR gate deadlock
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| run: | | ||
| mkdir -p "$RUNNER_TEMP/gov" && cd "$RUNNER_TEMP/gov" | ||
| printf '@wave-av:registry=https://npm.pkg.github.com\n//npm.pkg.github.com/:_authToken=${NODE_AUTH_TOKEN}\n' > .npmrc | ||
| npm install @wave-av/governance@^0.4.4 --no-save --no-audit --no-fund |
There was a problem hiding this comment.
1. Fork pr gate deadlock 🐞 Bug ☼ Reliability
The workflow always installs @wave-av/governance from GitHub Packages using GITHUB_TOKEN auth; for pull_request events from forks, that token commonly cannot read org packages, so npm install fails and the enforce check will go red for external contributors. This repo’s contributing guide explicitly requires a fork-based PR flow, so once this check is required it can block those PRs from merging.
Agent Prompt
## Issue description
`governance-enforce` installs an org-scoped package from GitHub Packages using `GITHUB_TOKEN`. On `pull_request` events from forks, the token frequently lacks access to org packages, causing the workflow to fail and (once required) blocking fork-based contributions.
## Issue Context
The repo’s documented contribution flow is to fork and open a PR.
## Fix Focus Areas
- .github/workflows/governance-enforce.yml[18-59]
## Suggested fix
Add a fork-aware guard so the job still reports a green/usable check for fork PRs, e.g.:
- Add a job-level `if:` or step-level `if:` that detects forks:
- `github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository`
- For fork PRs, either:
- Run a safe, vendored alternative (e.g., reuse/duplicate the existing public-repo-guard logic), or
- No-op with a clear `::notice::` explaining that the governance gate can’t run on forks due to package auth constraints.
(If you consider `pull_request_target` to regain package access, keep it strictly non-executing: do not run repository-provided scripts, and carefully check out the head SHA without evaluating PR-controlled code beyond read-only scanning.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer✅ Merged (0) · ☑ Fixed (0) Process
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…othing Five defects, none of them cosmetic. Refs wave-av/claude-workstation#1747. 1. FAIL-OPEN DIFF BASE. `BASE=$(git rev-parse HEAD~1 2>/dev/null || git rev-parse HEAD)` — on a root commit `git rev-parse HEAD~1` prints its unresolved argument to stdout AND fails, so the `||` branch appends and BASE becomes a two-line string. `git diff` then exits 128, and the pinned enforcer turned that into zero files and a green check. Now: a reachability-checked base (a force-push can leave `github.event.before` pointing at a commit this checkout does not have), and with no resolvable base at all it diffs against the EMPTY TREE so the whole repo is scanned rather than nothing. 2. THE PINNED ENFORCER ITSELF FAILED OPEN. `^0.4.4` resolved to 0.4.4, whose file lister is `catch { return []; }` — any git error became zero files and rendered as `OK[enforce]: 0 changed file(s) scanned — 0 A_BLOCK violations`. A git error and a clean diff were byte-identical in the output. The fix had sat unreleased on claude-workstation main since 2026-07-29 because no `governance-v*` tag was ever pushed. Released now as 0.4.6 and pinned exactly here. 3. TOKEN IN SCOPE FOR THE WRONG STEPS. `NODE_AUTH_TOKEN` was job-level, so it was also in the environment of the step that executes the downloaded package. Now step-scoped, and the .npmrc holding it is removed on exit. 4. INSTALL SCRIPTS RAN WITH THAT TOKEN. `npm install` runs preinstall/postinstall by default. Added `--ignore-scripts`. 5. CANCELLED PUSH RUNS WERE SCANNED BY NOBODY. `cancel-in-progress: true` applied to push runs, and each push run only diffs its own before..HEAD range — so a cancelled run's commits were never examined by anything. Now PR-only. Also: `timeout-minutes: 10` and `set -euo pipefail`. Receipt, against a scratch repo whose root commit carries a no-hardcoded-paths violation, simulating a branch-creation push (`before` = all zeros): old logic -> malformed base -> caught error -> [] -> OK, 0 files scanned, PASS new logic -> "no diff base resolved ... scanning the whole tree" -> BLOCK, exit 1 Credit where it is due: several of these were found by the review bots on the sibling vendoring PRs and are folded in here — the step-scoped token, the .npmrc cleanup, the exact pin, `--ignore-scripts`, the force-push reachability check, `timeout-minutes`, and the concurrency hole (5), which was crest-console#7's catch and which I had missed entirely.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_87d6d646-71f5-4228-9e49-7d4b2b84949d) |
| [ -n "$BASE" ] || BASE="${PUSH_BEFORE_SHA:-}" | ||
| # A base can be PRESENT and still unusable: a force-push leaves `github.event.before` | ||
| # pointing at a commit this checkout no longer contains. | ||
| if [ -z "$BASE" ] || [ "$BASE" = "0000000000000000000000000000000000000000" ] \ |
There was a problem hiding this comment.
🟠 High workflows/governance-enforce.yml:78
When a force-push makes github.event.before unreachable, the fallback to HEAD~1 only scans the latest commit, so any other commits introduced by the force-push are never examined while the gate can pass green. The HEAD~1 fallback narrows the diff to one commit instead of scanning from the empty tree like the documented fail-closed path. Remove the HEAD~1 fallback so an unreachable push base falls through to the empty-tree scan, or fail the job.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/governance-enforce.yml around line 78:
When a force-push makes `github.event.before` unreachable, the fallback to `HEAD~1` only scans the latest commit, so any other commits introduced by the force-push are never examined while the gate can pass green. The `HEAD~1` fallback narrows the diff to one commit instead of scanning from the empty tree like the documented fail-closed path. Remove the `HEAD~1` fallback so an unreachable push base falls through to the empty-tree scan, or fail the job.
Evidence trail:
.github/workflows/governance-enforce.yml:74-93 at cc0e626f
.git diff MERGE_BASE REVIEWED_COMMIT -- .github/workflows/governance-enforce.yml
| if [ -z "$BASE" ]; then | ||
| BASE="$(git hash-object -t tree /dev/null)" | ||
| echo "::notice::no diff base resolved (root commit or unreachable before-sha) — scanning the whole tree against the empty tree" | ||
| fi |
There was a problem hiding this comment.
🔍 Empty-tree fallback base may not be accepted by the enforcer's diff path
The fail-closed fallback sets BASE to the empty-tree object id (git hash-object -t tree /dev/null) and passes it to enforce.mjs --changed "$BASE". Git special-cases the empty tree so git diff <empty-tree> HEAD works even though the object was never written, but this only holds if the enforcer runs a two-dot git diff. If enforce.mjs validates the argument as a commit (rev-parse <base>^{commit}, cat-file -e ...^{commit}) or uses a three-dot range (which requires a merge-base), the empty tree id will error out. Given 0.4.6 is described as failing closed on git errors, the job would then hard-fail rather than scan the tree — the fallback path would never actually deliver a full-tree scan. Worth confirming against the 0.4.6 implementation, since this path is precisely the one that only fires in rare situations (root commit, force-pushed before-sha) and would otherwise go untested.
Was this helpful? React with 👍 or 👎 to provide feedback.
…l a false pass Correction to the previous commit on this branch. Refs wave-av/claude-workstation#1747. That commit replaced the fail-open `BASE=HEAD` with a fallback to `HEAD~1`. `HEAD~1` is also wrong: it scans exactly ONE commit, so a five-commit push whose base is indeterminate (branch creation, force-push, unreachable `github.event.before`) examines the last commit and reports a confident pass on the other four. A narrowed scan reported as a full pass is the same defect in a quieter costume. Receipt — scratch repo, five-commit push, violation planted in commit 1: HEAD~1 base -> OK[enforce]: 1 changed file(s) scanned -> PASS (never saw it) empty-tree base -> 5 changed file(s) scanned -> BLOCK[enforce]: no-hardcoded-paths, exit 1 Now: with no resolvable base of any kind, diff against git's empty-tree object so every tracked file reads as added and the whole repo is scanned. Loud, never partial, never empty. Credit: wave-av/wave-rig's copy on main already had this right, with the reasoning in a comment ("HEAD~1 would skip earlier commits in a multi-commit push and let a violation through"). The fan-out copied the broken shape from elsewhere and I did not check the one repo that had already solved it. Also from wave-rig: `merge_group` is now a declared trigger and `github.event.merge_group. base_sha` joins the base chain. None of these repos runs a merge queue today, so the trigger is inert — but a required check that never reports on an event the repo actually uses is a permanent deadlock, and this closes that in advance rather than after someone hits it.
| - name: fetch governance enforcer (isolated install) | ||
| # FIX 1 — token scoped to THIS STEP. At job level it was also in scope for the step that | ||
| # executes the downloaded package, and for anything else the job ever grows. | ||
| env: | ||
| NODE_AUTH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| run: | | ||
| set -euo pipefail | ||
| mkdir -p "$RUNNER_TEMP/gov" && cd "$RUNNER_TEMP/gov" | ||
| trap 'rm -f "$RUNNER_TEMP/gov/.npmrc"' EXIT | ||
| printf '@wave-av:registry=https://npm.pkg.github.com\n//npm.pkg.github.com/:_authToken=${NODE_AUTH_TOKEN}\n' > .npmrc | ||
| # FIX 2 — --ignore-scripts. npm runs preinstall/install/postinstall by default, so this | ||
| # step would execute dependency-authored code with the registry token in its environment. | ||
| # FIX 3 — exact pin, and 0.4.6 specifically. `^0.4.4` resolved to 0.4.4, whose file lister | ||
| # is `catch { return []; }` — ANY git error became zero files and rendered as | ||
| # `OK[enforce]: 0 changed file(s) scanned`. 0.4.6 fails closed on a git error instead. | ||
| # A caret is also a standing authorization for whatever is published next; a bump is now | ||
| # a visible commit in this file. | ||
| npm install @wave-av/governance@0.4.6 --no-save --no-audit --no-fund --ignore-scripts |
There was a problem hiding this comment.
🟡 Pull requests from forks will always fail the new governance check
The new check downloads a private organization package using the automatic per-run token (npm install @wave-av/governance@0.4.6 at .github/workflows/governance-enforce.yml:70), which forked pull requests do not have permission to read, so every outside contribution fails this check with an install error instead of a real result.
Impact: External contributors following the documented fork-and-PR flow get a permanently red, unfixable check on their pull requests.
Why the token cannot read the package on fork PRs
For pull_request events triggered from a fork, GitHub issues a GITHUB_TOKEN scoped to the fork repository with read-only contents permission; it is not granted read access to private packages owned by wave-av. The .npmrc written at .github/workflows/governance-enforce.yml:62 authenticates to npm.pkg.github.com with that token, so npm install returns 401/404 and, with set -euo pipefail, the step fails. CONTRIBUTING.md documents fork-based contributions as the expected flow, and README/repo guard indicate this is a public repo, so this path is reachable.
Mitigations would be to skip or soft-fail the job when github.event.pull_request.head.repo.fork is true (while keeping it enforcing for same-repo PRs and pushes), or to make the package publicly readable / vendor the enforcer.
Prompt for agents
The governance-enforce workflow installs the private @wave-av/governance package from GitHub Packages using secrets.GITHUB_TOKEN. On pull_request events originating from forks (the flow documented in CONTRIBUTING.md for this public repo), that token is scoped to the fork and cannot read org-private packages, so the install step fails and the check is permanently red for external contributors. Consider gating the job on github.event.pull_request.head.repo.full_name == github.repository (running it only for same-repo PRs, pushes and merge_group), or moving the enforcement to a pull_request_target/scheduled job, or publishing the enforcer where fork PRs can fetch it. Note the workflow header explicitly warns against adding this repo to the required-check ruleset until the check is observed green, so the fork behaviour should be decided before that happens.
Was this helpful? React with 👍 or 👎 to provide feedback.
| PR_BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| MERGE_BASE_SHA: ${{ github.event.merge_group.base_sha }} | ||
| PUSH_BEFORE_SHA: ${{ github.event.before }} | ||
| run: | | ||
| set -euo pipefail | ||
| ENFORCE="$RUNNER_TEMP/gov/node_modules/@wave-av/governance/bin/enforce.mjs" | ||
| BASE="${PR_BASE_SHA:-}" | ||
| [ -n "$BASE" ] || BASE="${MERGE_BASE_SHA:-}" | ||
| [ -n "$BASE" ] || BASE="${PUSH_BEFORE_SHA:-}" |
There was a problem hiding this comment.
🔍 PR base sha may drag in unrelated base-branch commits into the scanned diff
github.event.pull_request.base.sha is the base commit at the time the PR event fired, and the checkout for a pull_request event is the merge commit. If the enforcer diffs two-dot (base..HEAD), any commits merged into main since the PR was opened appear in the diff, so unrelated legacy debt from other people's commits can fail this PR's gate. The workflow header explicitly claims "blocks NEW violations without failing on legacy debt", which only holds if the enforcer uses a merge-base (three-dot) diff. Note this expectation conflicts with ANALYSIS-0001's requirement that a bare tree object be acceptable — the two fallback paths want opposite diff semantics, so both should be confirmed against 0.4.6's implementation.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Adds the
governance-enforceA_BLOCK gate (secrets / hardcoded-paths, diff-scoped) to this repo. Part of claude-workstation#1624 E4 T4.9a, following thewave-av/cli#20pilot.Why this repo had no secrets scan
The org ruleset
governance-a-block-enforce(17901847) requires anenforcecheck across the fleet. Its scope is an explicit include list of 112 hand-maintained repository names — and every one of them matcheswave-*.The 16 public repos absent from that list are exactly the 16 not named
wave-*:.github,adk,api-spec,cli,companion-module-wave,create-wave-app,crest-console,dispatch-edge,examples,mcp-server,obs-wave-plugin,sdk,sdk-python,sdks,vmix-wave-integration,workflow-sdk.Read the intersection: the repos that publish our npm packages are precisely the repos running with no A_BLOCK secrets scan. Nobody excluded them. A naming convention silently became a security boundary, and it drew the line in the worst possible place.
Why the workflow lands before the ruleset entry
Adding a repo to a
required_status_checksruleset before it emits that check is a permanent deadlock — a required check that never reports can never go green, and every PR on the repo becomes unmergeable. So the order is: vendor the workflow, observe it green, then extend the list. Doing it the intuitive way round would have bricked all sixteen.This PR is also its own liveness drill. The workflow triggers on
pull_request, so it runs on the PR that adds it. Ifenforcereports green here, the vendored shape works in this repo. If it does not, nothing was required and nothing is blocked — which is the point of this ordering.Proven before fan-out, not assumed
@wave-av/governanceis aninternal-visibility package owned byclaude-workstation, so whether a public repo'sGITHUB_TOKENcan read it was the one real assumption. Rather than fan out on the inference, it was piloted on a single repo first:That is the receipt this PR rides on. The shape is copied verbatim from
wave-av/wave-moq-edge(public, 12/12 green), which matters becauseauto-approve.ymlfails silently on every public repo — it calls a reusable workflow in the privatewave-foundation, and a public repo cannot do that (parse-time failure, zero jobs, no annotation). This workflow calls nothing cross-repo, so that trap does not apply.Security properties, unchanged from the source:
actions/checkout@df4cb1c,actions/setup-node@48b55a0)persist-credentials: falseon checkoutcontents: read+packages: readRUNNER_TEMP,--no-save, so nothing touches this repo's dependency tree.npmrcis written with a literal${NODE_AUTH_TOKEN}(single-quotedprintf) which npm expands at run time — no secret value is ever written to disk or a log${{ }}inputs (base.sha,event.before) are routed throughenv:and read as"$VAR", never interpolated into the script bodyDiff-scoped by design: it blocks new violations without failing on legacy debt.
Refs wave-av/claude-workstation#1624.
Note
Low Risk
CI-only change with no application runtime impact; hardening reduces false-pass risk on the security gate itself.
Overview
Adds
.github/workflows/governance-enforce.yml, bringing this repo under the same diff-scoped A_BLOCK scan (secrets, hardcoded paths) that org rulesetgovernance-a-block-enforceexpects via anenforcejob—repos not namedwave-*were previously off that hand-maintained list.The workflow runs on pull requests and pushes to
main/master: full-history checkout, isolated install of@wave-av/governance@0.4.6intoRUNNER_TEMPwith registry token limited to the install step,--ignore-scripts, andenforce.mjs --changed <base>. Concurrency only cancels in-progress runs on PRs so push runs are not skipped. Base resolution validates the SHA, then fails closed to the empty tree (full scan) instead of a no-opHEADdiff.Do not add this repo to the ruleset until
enforceis green here; the PR description treats this merge as the liveness drill before making the check required.Reviewed by Cursor Bugbot for commit cc0e626. Configure here.
Note
Add governance enforcement workflow to scan diffs on PRs and pushes to main
Adds governance-enforce.yml, which runs
@wave-av/governance@0.4.6on pull requests, merge groups, and pushes to main/master. The enforcer scans only changed files using a computed diff base; if no valid base is found, it falls back to the empty tree and scans the full repository. Push and merge group runs are not cancelable to ensure no committed ranges are skipped.Macroscope summarized 0b8cf79.