stack 1/5: add contribution intake firewall - #900
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds a contribution firewall. It defines submission requirements, validates attestations and approved issues, automates trusted PR admission, manages admission labels and comments, and closes inactive author-action pull requests. ChangesContribution firewall
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant pr_admission_workflow
participant GitHubAPI
participant pr_admission
PullRequest->>pr_admission_workflow: Submit or update pull request
pr_admission_workflow->>GitHubAPI: Retrieve metadata, files, permissions, and linked issues
pr_admission_workflow->>pr_admission: Assess admission data
pr_admission-->>pr_admission_workflow: Return admission result
pr_admission_workflow->>GitHubAPI: Update labels and status comment
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Stack navigation
Review and merge bottom-up. Each PR targets the preceding stack branch, so its Files changed view contains only that layer. |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/pr-admission.cjs:
- Around line 18-25: The duplicated attestation requirements need an automated
parity check against the pull request template. Add a test in
pr-admission.test.cjs that reads .github/PULL_REQUEST_TEMPLATE.md and verifies
every entry in REQUIRED_ATTESTATIONS appears on a line formatted as a checked
checkbox, while preserving the existing validation behavior.
In @.github/workflows/pr-admission.yml:
- Around line 3-5: Update the workflow triggers to add workflow_dispatch with a
required pull_request_number input, and update the admission script’s
pull_number resolution to use github.event.inputs.pull_request_number for manual
runs while retaining context.payload.pull_request.number for pull request
events. Ensure the existing admission checks continue operating on the resolved
PR number.
- Around line 3-16: Set workflow-level permissions to an empty map, then move
the existing contents, issues, and pull-requests permissions into the admission
job only. Add a brief comment on the job permissions documenting that issues and
pull-requests write access is required for label and comment management, and
obtain the required maintainer security sign-off before merging.
In @.github/workflows/stale-author-prs.yml:
- Around line 9-11: Update the stale workflow permissions and configuration:
change issues permission from write to read, retain pull-requests write access,
and set remove-issue-stale-when-updated to false so updated issues cannot lose
the stale label.
- Around line 9-11: Add actions: write to the workflow permissions block in
stale-author-prs.yml alongside issues and pull-requests, enabling actions/stale
state persistence while preserving the existing permissions.
- Line 29: Update the stale-pr-label setting in the workflow configuration to
use the workflow-specific label awaiting-author-stale instead of stale, while
leaving the surrounding actions/stale behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e14c0bd3-d4e3-45a6-bc57-40631896ec42
📒 Files selected for processing (7)
.github/PULL_REQUEST_TEMPLATE.md.github/scripts/pr-admission.cjs.github/scripts/pr-admission.test.cjs.github/workflows/pr-admission.yml.github/workflows/stale-author-prs.ymldocs/superpowers/plans/2026-08-02-contribution-firewall.mddocs/superpowers/specs/2026-08-02-contribution-firewall-design.md
| permissions: | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/stale-author-prs.yml
printf '%s\n' '--- stale action references ---'
rg -n --glob '*.yml' --glob '*.yaml' 'actions/stale|remove-(issue|pr)-stale-when-updated|close-(issue|pr)|stale-issue|stale-pr' .github
printf '%s\n' '--- pinned action metadata ---'
curl -fsSL https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/action.yml | sed -n '1,220p'Repository: lidge-jun/opencodex
Length of output: 14525
🏁 Script executed:
#!/bin/bash
set -eu
sha=1e223db275d687790206a7acac4d1a11bd6fe629
printf '%s\n' '--- action documentation permissions ---'
curl -fsSL "https://github.com/actions/stale/$sha/README.md" \
| rg -n -i -C 4 'permissions:|issues: write|pull-requests: write|only-pr-labels|remove-issue-stale-when-updated'
printf '%s\n' '--- action source references ---'
curl -fsSL "https://github.com/actions/stale/$sha/dist/index.js" \
| rg -n -o '.{0,180}(removeIssueStaleWhenUpdated|removePrStaleWhenUpdated|onlyPrLabels|onlyIssueLabels|issues|pullRequests|pull_request|issues.createComment|issues.update|issues.addLabels|pulls.update).{0,260}' \
| head -n 120
printf '%s\n' '--- repository workflow permissions context ---'
cat -n .github/workflows/stale-needs-info.yml | sed -n '1,115p'Repository: lidge-jun/opencodex
Length of output: 23157
🏁 Script executed:
#!/bin/bash
set -eu
sha=1e223db275d687790206a7acac4d1a11bd6fe629
curl -fsSL "https://github.com/actions/stale/$sha/dist/index.js" > /tmp/actions-stale-index.js
printf '%s\n' '--- issue filtering and processing ---'
sed -n '480,640p' /tmp/actions-stale-index.js
printf '%s\n' '--- stale-removal option resolution ---'
sed -n '1080,1140p' /tmp/actions-stale-index.js
sed -n '1185,1250p' /tmp/actions-stale-index.js
printf '%s\n' '--- mutation branches ---'
sed -n '840,1010p' /tmp/actions-stale-index.jsRepository: lidge-jun/opencodex
Length of output: 25624
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import re
import urllib.request
workflow = open(".github/workflows/stale-author-prs.yml", encoding="utf-8").read()
action = urllib.request.urlopen(
"https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/action.yml",
timeout=10,
).read().decode()
def value(name, text):
match = re.search(r"(?m)^\s+" + re.escape(name) + r":\s*(.*)$", text)
return match.group(1).strip() if match else None
configured = {
name: value(name, workflow)
for name in (
"only-pr-labels",
"days-before-issue-stale",
"days-before-issue-close",
"days-before-pr-stale",
"days-before-pr-close",
"remove-stale-when-updated",
"remove-issue-stale-when-updated",
"remove-pr-stale-when-updated",
)
}
default_remove = value("default", action[action.index(" remove-stale-when-updated:"):])
issue_override = configured["remove-issue-stale-when-updated"]
pr_override = configured["remove-pr-stale-when-updated"]
effective_issue_remove = (issue_override or default_remove).strip("'\"")
effective_pr_remove = (pr_override or default_remove).strip("'\"")
print("configured:", configured)
print("action default remove-stale-when-updated:", default_remove)
print("effective issue stale removal:", effective_issue_remove)
print("effective PR stale removal:", effective_pr_remove)
print("issues have only-label filter:", bool(value("only-issue-labels", workflow)))
print("issue stale/close timers disabled:",
configured["days-before-issue-stale"] == "-1" and
configured["days-before-issue-close"] == "-1")
assert effective_issue_remove == "true"
assert effective_pr_remove == "true"
assert not value("only-issue-labels", workflow)
assert configured["days-before-issue-stale"] == "-1"
assert configured["days-before-issue-close"] == "-1"
PYRepository: lidge-jun/opencodex
Length of output: 2443
Remove issue write access from this PR-only workflow.
only-pr-labels does not exclude issues. With no only-issue-labels, an updated issue carrying stale can still lose that label because remove-stale-when-updated defaults to true. Set remove-issue-stale-when-updated: false and change issues: write to issues: read. Retain pull-requests: write for PR labels, comments, and closure. This .github/** permission change requires explicit security review.
🧰 Tools
🪛 zizmor (1.28.0)
[error] 10-10: overly broad permissions (excessive-permissions): issues: write is overly broad at the workflow level
(excessive-permissions)
[error] 11-11: overly broad permissions (excessive-permissions): pull-requests: write is overly broad at the workflow level
(excessive-permissions)
[warning] 10-10: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/stale-author-prs.yml around lines 9 - 11, Update the stale
workflow permissions and configuration: change issues permission from write to
read, retain pull-requests write access, and set remove-issue-stale-when-updated
to false so updated issues cannot lose the stale label.
Sources: Path instructions, Linters/SAST tools
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/stale-author-prs.yml
printf '%s\n' '--- related workflow configuration ---'
rg -n --glob '.github/**' 'actions/stale|operations-per-run|remove-stale-when-updated|stale-issue|stale-pr|cache|permissions:' .
printf '%s\n' '--- pinned action source references ---'
curl -L --fail --silent --show-error \
https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/src/main.ts |
rg -n -C 4 'cache|operations-per-run|state|save|restore' || trueRepository: lidge-jun/opencodex
Length of output: 1948
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/stale-author-prs.yml
printf '%s\n' '--- action source and documentation ---'
curl -L --fail --silent --show-error \
https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/src/main.ts |
rg -n -C 8 'cache|operations-per-run|state|save|restore' || true
curl -L --fail --silent --show-error \
https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/action.yml |
rg -n -C 4 'permissions|cache|operations-per-run|state' || trueRepository: lidge-jun/opencodex
Length of output: 5264
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- state service ---'
curl -L --fail --silent --show-error \
https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/src/services/state.service.ts |
cat -n
printf '%s\n' '--- package dependencies ---'
curl -L --fail --silent --show-error \
https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/package.json |
rg -n -C 3 '`@actions/cache`|dependencies|version'
printf '%s\n' '--- action documentation for permissions and state ---'
curl -L --fail --silent --show-error \
https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/README.md |
rg -n -i -C 5 'permission|cache|operations-per-run|state' || true
printf '%s\n' '--- GitHub cache permission documentation ---'
curl -L --fail --silent --show-error \
https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows |
rg -n -i -C 4 'actions: write|cache.*permission|permission.*cache|GITHUB_TOKEN' || trueRepository: lidge-jun/opencodex
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- state service ---'
curl -L --fail --silent --show-error \
https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/src/services/state.service.ts |
cat -n
printf '%s\n' '--- cache dependency and implementation references ---'
curl -L --fail --silent --show-error \
https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/package.json |
rg -n -C 3 '`@actions/cache`|dependencies'
curl -L --fail --silent --show-error \
https://api.github.com/repos/actions/stale/contents/src/services?ref=1e223db275d687790206a7acac4d1a11bd6fe629 |
jq -r '.[].name'Repository: lidge-jun/opencodex
Length of output: 977
🏁 Script executed:
#!/bin/bash
set -eu
for file in \
src/classes/state/state-cache-storage.ts \
src/classes/state/state.ts \
src/interfaces/issues-processor-options.ts \
README.md \
action.yml
do
printf '\n--- %s ---\n' "$file"
curl -L --fail --silent --show-error \
"https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/$file" |
rg -n -i -C 8 'cache|restore|save|delete|operations-per-run|permission|write|state' || true
done
printf '\n--- actions/cache source permission-related behavior ---\n'
curl -L --fail --silent --show-error \
https://github.com/actions/cache/v5.0.5/src/cache.ts |
rg -n -i -C 8 'saveCache|delete|reserve|ACTIONS_RUNTIME_TOKEN|permission|403' || trueRepository: lidge-jun/opencodex
Length of output: 26291
🌐 Web query:
actions/stale v10.4.0 state cache actions write permission operations-per-run
💡 Result:
In actions/stale v10.4.0, the actions: write permission is required to manage state via the GitHub Actions Cache API [1][2]. Specifically, the action needs this permission to delete existing cache entries when conflicts occur, ensuring the action can properly maintain and update its state across workflow runs [1][2]. Regarding the operations-per-run parameter: - Purpose: It limits the number of GitHub API operations (such as fetching issues, adding/removing labels, commenting, or closing issues) performed in a single workflow run [3][4]. This helps prevent the action from exceeding GitHub's API rate limits [3][4]. - Default Value: The default is 30 operations per run [5][3][4]. - Behavior: When the limit is reached, the action stops processing further items [3][4]. The state is preserved (via the aforementioned cache), allowing the next scheduled run to resume from the point where the previous run stopped [5]. - Recommendation: If logs indicate that the operation limit is being hit, the operations-per-run value can be increased (e.g., to 1000) to process more items in a single run, provided it remains within the context of your repository's rate limit capacity [3][4].
Citations:
- 1: Update README.md actions/stale#1248
- 2: Error delete _state: [403] Resource not accessible by integration actions/stale#1133
- 3: https://github.com/actions/stale?tab=readme-ov-file
- 4: https://github.com/actions/stale/tree/refs/heads/main
- 5: https://github.com/actions/stale
🌐 Web query:
GitHub Actions cache GITHUB_TOKEN actions write permission save cache
💡 Result:
In GitHub Actions, the ability to save to the cache is determined by the permissions granted to the GITHUB_TOKEN and the trigger context of the workflow, rather than a specific "cache write" permission setting you can toggle in the workflow YAML [1][2][3]. Key points regarding cache write permissions: 1. Relationship with GITHUB_TOKEN: While you can use the permissions block in your workflow to control access to various GitHub API scopes (such as contents or packages), cache access is governed by policy-driven restrictions [3][4]. You cannot explicitly grant or revoke "cache write" access via the standard permissions block [3]. 2. Read-Only Restrictions: Certain workflow runs are automatically restricted to read-only cache access [1][2]. The most common example is a workflow triggered by a pull request from a fork [1][2]. In these cases, the job can restore existing caches, but it does not have the permissions to save new ones [1][2]. 3. Behavior on Denial: If a workflow run with a read-only token attempts to save a cache using the actions/cache action, the action will not fail the entire job [1][2][4]. Instead, it will log a warning (e.g., "cache write denied") and proceed, allowing the workflow to continue without the cache save [1][2][4]. 4. Security Best Practices: GitHub restricts cache write access for lower-trust triggers (such as external PRs) to prevent malicious cache poisoning, where a compromised workflow could overwrite or manipulate cache contents used by trusted jobs [5]. To maintain security, avoid storing sensitive information in cached paths, as anyone with read access to the repository can generally access those caches [5].
Citations:
- 1: https://github.com/actions/cache/?tab=readme-ov-file
- 2: https://github.com/actions/cache/blob/main/README.md
- 3: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
- 4: Handle cache write error due to read-only token actions/toolkit#2429
- 5: https://github.com/github/docs/blob/main/content/actions/reference/workflows-and-actions/dependency-caching.md
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from urllib.request import urlopen
import re
base = "https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629/"
files = [
"src/classes/issues-processor.ts",
"src/classes/issue.ts",
"src/classes/state/state.ts",
]
patterns = re.compile(
r"operationsPerRun|operations per run|persist\(|reset\(|"
r"isIssueProcessed|addIssueToProcessed|processedIssues",
re.I,
)
for name in files:
text = urlopen(base + name).read().decode()
print(f"\n--- {name} ---")
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if patterns.search(line):
lo, hi = max(1, i - 4), min(len(lines), i + 6)
for n in range(lo, hi + 1):
print(f"{n:4}: {lines[n-1]}")
print()
PYRepository: lidge-jun/opencodex
Length of output: 2444
🏁 Script executed:
#!/bin/bash
set -eu
base='https://github.com/actions/stale/1e223db275d687790206a7acac4d1a11bd6fe629'
for file in \
src/classes/issues-processor.ts \
src/classes/issue.ts \
src/classes/state/state.ts
do
printf '\n--- %s ---\n' "$file"
curl -L --fail --silent --show-error "$base/$file" |
nl -ba |
rg -n -i -C 6 'operationsPerRun|operations per run|persist\(|reset\(|isIssueProcessed|addIssueToProcessed|processedIssues'
doneRepository: lidge-jun/opencodex
Length of output: 309
Grant actions: write for state persistence.
actions/stale stores progress in the Actions cache and requires this permission to replace the cache. Without it, persistence can fail with a warning. At operations-per-run: 60, later awaiting-author pull requests can remain unprocessed across scheduled runs. Add actions: write at .github/workflows/stale-author-prs.yml:9-11.
🧰 Tools
🪛 zizmor (1.28.0)
[error] 10-10: overly broad permissions (excessive-permissions): issues: write is overly broad at the workflow level
(excessive-permissions)
[error] 11-11: overly broad permissions (excessive-permissions): pull-requests: write is overly broad at the workflow level
(excessive-permissions)
[warning] 10-10: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment
(undocumented-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/stale-author-prs.yml around lines 9 - 11, Add actions:
write to the workflow permissions block in stale-author-prs.yml alongside issues
and pull-requests, enabling actions/stale state persistence while preserving the
existing permissions.
Source: Path instructions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbe6887302
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [opened, reopened, edited, synchronize, ready_for_review] |
There was a problem hiding this comment.
When an external implementation PR is waiting for scope approval and a maintainer adds approved-for-work to its linked issue, none of these pull_request_target events fires, so the failed check and awaiting-author label remain unchanged. The scheduled workflow can then close an actually approved PR after five days unless someone knows to edit or push to the PR solely to retrigger this workflow; add a trusted issue-label reevaluation path for affected PRs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 9e9db374: pr-admission.yml now accepts workflow_dispatch with a required pull_request_number input so a maintainer can re-run admission after the linked issue gains approved-for-work. Dispatch runs are restricted to the repository default branch via rejectsWorkflowDispatchNonDefaultBranch, matching the enforce-issue-quality pattern.
| } catch (error) { | ||
| core.warning( | ||
| `Could not load linked issue #${issue_number}: ${error.message}`, | ||
| ); |
There was a problem hiding this comment.
If issues.get fails transiently for a valid linked issue, this catch omits it from linkedIssues and continues, causing assessAdmission to report missing_issue or issue_not_approved; the workflow then removes intake: admitted, applies awaiting-author, and may auto-close the valid PR without any automatic retry. Treat lookup failures as infrastructure failures and abort before mutating admission labels rather than converting them into author-action failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 9e9db374: non-404 issues.get failures now core.setFailed and return before any label or comment mutation; 404s for nonexistent references remain warnings (author problem). Verified by an end-to-end harness simulation of the workflow script.
| function issueIsApproved(issue) { | ||
| return issue.labels.some((label) => { | ||
| const name = typeof label === "string" ? label : label?.name; | ||
| return name === "approved-for-work"; |
There was a problem hiding this comment.
For an external implementation PR, this predicate accepts any issue that still carries approved-for-work, even if that issue was already closed after another implementation landed. Because GitHub does not automatically remove labels on close, contributors can repeatedly reference an old completed issue and bypass the agreed-scope intake gate; carry the issue state into the validator and require the approved issue to remain open.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 4d8a9d04: issueIsApproved now requires state === "open", the workflow carries issue.state into the validator, and a regression test covers an approved-but-closed issue. The design record now says "open issue".
|
|
||
| const failures = assessAdmission({ | ||
| body: pr.body, | ||
| changedFiles: files.map((file) => file.filename), |
There was a problem hiding this comment.
For a renamed file, the pull-files API reports the destination in filename and the source in previous_filename, but this passes only the destination to the classifier. A PR whose sole change moves src/router.ts to a documentation path therefore appears documentation-only even though it removes runtime code, allowing an external author to bypass the approved-issue requirement; include previous_filename for renamed entries.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 9e9db374: the workflow now classifies both filename and previous_filename for changed files, so a rename out of an implementation path no longer reads as documentation-only. Covered by an end-to-end harness probe.
| const IMPLEMENTATION_FILES = new Set([ | ||
| "package.json", | ||
| "bun.lock", | ||
| "tsconfig.json", | ||
| ]); |
There was a problem hiding this comment.
An external PR that changes only bunfig.toml is classified as documentation/policy work and does not need an approved issue, even though this repository uses that file to set the Bun test root and preload the real-home write guard. Removing or changing those settings materially alters test execution and safety, so add bunfig.toml to the implementation-file set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 4d8a9d04: bunfig.toml is now in IMPLEMENTATION_FILES (it sets the Bun test root and preloads the real-home write guard), with a classification test and a matching design-doc scope update.
|
|
||
| Closes #<!-- issue number --> | ||
|
|
||
| Implementation pull requests must reference an issue labeled `approved-for-work`. Documentation-only and maintainer-owned integration changes are exempt. |
There was a problem hiding this comment.
This introduces mandatory approved-issue attestations and a five-day automatic-close policy, but neither CONTRIBUTING.md nor docs-site/src/content/docs/contributing.md and its translations describe those requirements. Contributors following the published guide therefore learn the contract only after their PR fails and is placed on the closure timer; update the English contributor documentation and keep translated versions consistent.
AGENTS.md reference: AGENTS.md:L200-L201
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Declined here because it is already implemented later in this stack: #905 adds the CONTRIBUTING.md "Pull request contract" section, the docs-site page contributing/pr-quality.md, and MAINTAINERS.md policy notes covering approved-for-work, attestations, and the automatic-close window. Keeping the docs change in #905 avoids duplicating it in #900.
| } | ||
|
|
||
| if (failures.length === 0) { | ||
| await removeLabel("awaiting-author"); |
There was a problem hiding this comment.
Once intake passes, this unconditionally removes awaiting-author, although the stale workflow's own message says that label also covers unresolved CI failures and review feedback. If a maintainer or another gate applies the label for one of those reasons, the author's next push reruns admission and removes it merely because the intake-specific checks pass, disabling the intended inactivity handling while feedback remains unresolved; use an intake-specific failure label or track whether this workflow added the shared label before removing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Declined with rationale: awaiting-author is shared, stack-owned state. #901's readiness workflow manages the label lifecycle (it re-applies awaiting-author when admission or checks fail and removes it on awaiting-maintainer), and #905 adds review: round-* labels for maintainer feedback. Admission only transitions the label on its own intake outcome; ownership tracking inside #900 would duplicate the readiness layer's reconcile. Manual maintainer feedback should use the #905 review-round labels.
|
|
||
| const issueNumbers = extractLinkedIssueNumbers(pr.body); | ||
| const linkedIssues = []; | ||
| for (const issue_number of issueNumbers) { |
There was a problem hiding this comment.
An external author can place thousands of unique Refs #N entries in the PR body, and this trusted write-token workflow performs one sequential API request for every extracted number with no limit or job timeout. A single edited PR can therefore exhaust the repository's GitHub API allowance and keep a runner occupied, disrupting admission and other automation; reject an excessive reference count or cap lookups to a small documented maximum.
AGENTS.md reference: .github/AGENTS.md:L16-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 9e9db374: linked-issue lookups are capped at 10 per run with a warning when truncated, so untrusted PR text cannot drive unbounded sequential API calls from this write-token workflow. Verified by an end-to-end harness probe (12 refs -> 10 lookups + warning).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-admission.yml:
- Around line 40-42: Preserve the security review gate in the PR admission
workflow while using authorHasPushPermission from pr-quality.cjs for permission
checks. Require explicit security review, at least one maintainer approval, and
successful required CI checks before allowing a merge; keep permission lookup
failures treated as read-only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c55efb1e-bd35-4052-91e4-a8a2f6bdd1c1
📒 Files selected for processing (2)
.github/scripts/pr-admission.test.cjs.github/workflows/pr-admission.yml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/stale-author-prs.yml (1)
27-35: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove
awaiting-author-stalewhen admission succeeds.
.github/workflows/pr-admission.yml:237-240removesawaiting-authorbut does not removeawaiting-author-stale. The next.github/workflows/stale-author-prs.ymlrun excludes the admitted PR becauseonly-pr-labelsrequiresawaiting-author; thereforeremove-pr-stale-when-updatedcannot clear the stale label. Removeawaiting-author-stalein the admission success path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/stale-author-prs.yml around lines 27 - 35, Update the successful admission path in the pr-admission workflow to remove both awaiting-author and awaiting-author-stale labels. Keep the existing admission behavior unchanged, ensuring admitted PRs no longer retain the stale label that stale-author-prs.yml manages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-admission.yml:
- Around line 51-53: Move rejectsWorkflowDispatchNonDefaultBranch out of
issue-quality.cjs into a clearly named shared workflow-security or PR-admission
module, then update the require in the PR-admission workflow to import it from
the new module. Preserve the helper’s behavior and exported API while removing
its ownership from the issue-quality module.
In @.github/workflows/stale-author-prs.yml:
- Around line 10-12: Move the actions, issues, and pull-requests write
permissions from workflow scope into the stale job’s permissions block, and
remove the top-level permissions block. Preserve all three permissions for the
job using actions/stale.
---
Outside diff comments:
In @.github/workflows/stale-author-prs.yml:
- Around line 27-35: Update the successful admission path in the pr-admission
workflow to remove both awaiting-author and awaiting-author-stale labels. Keep
the existing admission behavior unchanged, ensuring admitted PRs no longer
retain the stale label that stale-author-prs.yml manages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce3a66b3-2e73-48d4-a27e-ab14d586806e
📒 Files selected for processing (5)
.github/scripts/pr-admission.cjs.github/scripts/pr-admission.test.cjs.github/workflows/pr-admission.yml.github/workflows/stale-author-prs.ymldocs/superpowers/specs/2026-08-02-contribution-firewall-design.md
| const { rejectsWorkflowDispatchNonDefaultBranch } = require( | ||
| path.join(process.cwd(), ".github", "scripts", "issue-quality.cjs"), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider relocating rejectsWorkflowDispatchNonDefaultBranch to a more clearly-named shared module.
The function name and purpose (guarding workflow_dispatch branch trust for PR-admission) don't match its home file, issue-quality.cjs. Placing a PR-admission security guard in an issue-quality module obscures ownership and makes the security boundary harder to audit at a glance.
Move this helper into a shared workflow-security or admission-shared module so its name and location match its responsibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-admission.yml around lines 51 - 53, Move
rejectsWorkflowDispatchNonDefaultBranch out of issue-quality.cjs into a clearly
named shared workflow-security or PR-admission module, then update the require
in the PR-admission workflow to import it from the new module. Preserve the
helper’s behavior and exported API while removing its ownership from the
issue-quality module.
[shipping-github] Verdict: approve-commentPR: Semantic propagation
UsefulnessReal, documented maintainer-time problem; the mechanism is concrete (scope + ownership attestation before review), follows the repo's established trusted-workflow pattern ( Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
GateNone (draft cleared). Simplification (approved S1 + S2)
Bottom lineUseful, clean after the fix batch, and safe to review/merge by the requested maintainers. Automated review approves; this is not yet a merge-ready claim: |
|
Reviewed the whole stack together and measured the lane first, so this is a policy answer rather than a code review. The mechanism here is sound and the tests are real. What I am not ready to merge is the policy it encodes: an external implementation PR requires an open That is a genuine decision about what this project is, not a CI tuning knob. Every fix in this repo's recent history that I actually wanted — the DeepSeek reasoning replay, the Cursor Grok parameters, the AgentRouter EOF tolerance, the tool-result image forwarding — arrived as an unplanned PR from someone who hit the bug and fixed it. Under this gate each of those would have been held until I pre-approved an issue, and closed if I was slow. I would rather absorb the triage cost than lose that class of contribution. The parts I would take today, if you want to split them:
Holding the admission gate itself pending an explicit decision from me on the contribution model. Not a reflection on the implementation — For sequencing context: I am landing #899 first (Windows was the last job to finish in 23 of 23 recent CI runs, median 17m41s against ubuntu 5m58s — that is the actual bottleneck), then #903, which is deterministic and reduces review load without gating on approval. |
|
Closing this one on the policy, not the implementation. Requiring an open What I took instead, in #920: sponsorship for the surfaces where a bad merge is genuinely expensive (auth, workflows, release automation, dependencies), which is the part of the intake problem that is about blast radius rather than about trust. Plus the deterministic hygiene gate from #903, already merged as #918 — between them, most of what the admission firewall was proxying for is now enforced mechanically, without anyone needing permission to fix a bug. The PR template attestations I did not take either: they overlap the hygiene checks and the sponsorship rule, and a checkbox is weaker evidence than a check that reads the diff. |
…n the docs Consolidates the parts of @Wibias's five-PR governance stack (lidge-jun#900, lidge-jun#902, lidge-jun#905) that reduce risk without adding friction, and drops the parts that do the opposite. The measurement behind the ordering: Windows was the last job to finish in 23 of 23 recent CI runs at a 17m41s median, so lidge-jun#899 was the actual bottleneck and everything here is judged by whether it makes the lane worse. Kept, from lidge-jun#902's trust lane: authentication, credential handling, GitHub Actions workflows, release automation, and dependency installation need a maintainer to sponsor the change before it merges. MAINTAINERS.md already requires security review for exactly these; this makes the requirement visible on the pull request instead of relying on a reviewer noticing. It runs inside the existing hygiene job rather than adding a workflow, and it applies to every contributor — blast radius does not depend on how many PRs someone has merged, which is why the upstream first-timer exemption is gone. Dropped, from the same PR: the 500-line cap and the one-open-PR limit. A provider preset with its registry rows, adapter wiring, tests, and five locales clears 500 lines by itself, and several good first contributions here have. Telling a newcomer their fix is too big is a worse failure than reviewing a large diff. Dropped, from lidge-jun#900: the admission gate requiring a pre-approved issue, and the five-day auto-close. The DeepSeek reasoning replay, the Cursor Grok parameters, the AgentRouter EOF tolerance, the tool-result image forwarding — every one arrived as an unplanned PR from someone who hit the bug. A gate that required a planning discussion first would have lost all of them. Kept, from lidge-jun#905: CODEOWNERS entries for the high-impact runtime directories, and the contributor documentation — rewritten to describe what is actually enforced. The submitted version documented the approved-for-work gate, the size caps, and the automatic closure timers, none of which exist here, and publishing rules the repository does not enforce is worse than publishing none. Not included: lidge-jun#901's readiness gate. It makes CodeRabbit's judgment blocking and triggers per check_run, which scales with the job count lidge-jun#899 just raised. Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com>
Stack
1/5 — contribution intake firewall
Base:
devNext: readiness and CodeRabbit gate
Summary
approved-for-workawaiting-authorandintake: admittedintake statesWhy
Maintainer time is being consumed by unscoped, unvalidated, and abandoned pull requests. This first stack layer prices author responsibility before human review instead of attempting unreliable AI-origin detection or treating commit count as a quality metric.
Verification
node --test .github/scripts/pr-admission.test.cjs— 10 passed, 0 faileddevand zero commits behindRollout notes
pull_request_targetand scheduled workflows do not become live until promoted to the repository default branchPR admission / admissiona required check until the synthetic fork test passesSummary by CodeRabbit
New Features
Documentation
Tests