Repository navigation
Trim unused nightly decide permissions - #30248
Conversation
|
Review (codex gpt-6.1-sol high): SHIP at 82cd657 Focused review of the exact permission diff:
|
There was a problem hiding this comment.
9 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/runners.json">
<violation number="1" location=".github/runners.json:436">
P2: This declaration permits `resolve-nightly-ghostty-cli-helper` to run on hosted Ubuntu while it publishes a GitHub artifact, contradicting the `hosted_light_jobs` security invariant. Remove this job from the hosted-light allowlist, or update the routing/policy so artifact-uploading jobs cannot take the hosted route.</violation>
</file>
<file name="tests/test_ci_portable_job_placement.py">
<violation number="1" location="tests/test_ci_portable_job_placement.py:283">
P2: This weaker assertion no longer verifies that early placement wins before the attempt-1 fallback; it only checks that the selected runner appears before Blacksmith. Assert that the selected runner precedes the hosted fallback as well, otherwise a routing regression can pass this test unnoticed.</violation>
</file>
<file name=".github/workflows/nightly.yml">
<violation number="1" location=".github/workflows/nightly.yml:194">
P2: This line changes first-attempt routing from the configured control/build runners to GitHub-hosted Linux, contrary to the stated intent that runner routing remain unchanged. Revert these routing changes, or explicitly document and validate the new hosted bootstrap path—including its different network, capacity, and cost behavior—in this PR.</violation>
</file>
<file name="tests/test_nightly_trusted_routing.py">
<violation number="1" location="tests/test_nightly_trusted_routing.py:122">
P2: This negative check misses write calls that use optional chaining, such as `github.rest.repos?.createRelease`, even though the decide script already uses optional chaining for REST access. Match optional namespace access and suffixed write methods so the permission regression test cannot silently pass.</violation>
</file>
<file name="scripts/ci/runner_routing_table.py">
<violation number="1" location="scripts/ci/runner_routing_table.py:119">
P1: This validation does not reject an unverified fallback inserted between early admission and the final Blacksmith label. Validate every fallback alternative, not just the first and last, so a missing early output cannot queue on an arbitrary runner.</violation>
<violation number="2" location="scripts/ci/runner_routing_table.py:384">
P1: The hosted-light policy allows `decide` to run on hosted Ubuntu even though the job passes `secrets.GLAEDA_ROUTE_APP_KEY`; the narrow secret-name filter misses this credential. Reject every `secrets.` reference (or explicitly exclude `decide`) before emitting a hosted-runner exemption.</violation>
<violation number="3" location="scripts/ci/runner_routing_table.py:401">
P1: The hosted exception can authorize issue-writing jobs because the denylist omits the declared report/close jobs and the sensitive scan ignores `permissions: write`. Reject write-scoped jobs (and other credentialed operations) before emitting an exemption.</violation>
</file>
<file name="scripts/ci/portable_job_placement.py">
<violation number="1" location="scripts/ci/portable_job_placement.py:167">
P1: This adds a runner-routing change to the permission-tightening PR: an unavailable runner API or stale snapshot now moves the nightly picker from the configured owned mini to hosted Linux. Preserve the existing fallback here, or explicitly update the workflow contract and PR scope for this routing change.</violation>
</file>
<file name="scripts/ci/nightly_linux_admission.py">
<violation number="1" location="scripts/ci/nightly_linux_admission.py:132">
P2: This override changes first-attempt routing for three early control jobs to GitHub-hosted Ubuntu when admission returns no receipt, instead of the prior owned/declared fallback. That contradicts the PR’s stated intent to leave runner routing unchanged; remove this override or document and separately approve the routing change.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| fork = "github.repository_owner != 'manaflow-ai' && 'ubuntu-24.04'" | ||
| hosted = "github.run_attempt == 1 && 'ubuntu-24.04'" | ||
| paid = {"'blacksmith-16vcpu-ubuntu-2404'", "'blacksmith-32vcpu-ubuntu-2404'"} | ||
| sensitive = re.compile(r"secrets\.(?:APPLE|SIGN|CMUX_RELEASE|NOTARY)|codesign|notarytool|signing-keychain|gh release upload|softprops/action-gh-release", re.I) |
There was a problem hiding this comment.
P1: The hosted-light policy allows decide to run on hosted Ubuntu even though the job passes secrets.GLAEDA_ROUTE_APP_KEY; the narrow secret-name filter misses this credential. Reject every secrets. reference (or explicitly exclude decide) before emitting a hosted-runner exemption.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/ci/runner_routing_table.py, line 384:
<comment>The hosted-light policy allows `decide` to run on hosted Ubuntu even though the job passes `secrets.GLAEDA_ROUTE_APP_KEY`; the narrow secret-name filter misses this credential. Reject every `secrets.` reference (or explicitly exclude `decide`) before emitting a hosted-runner exemption.</comment>
<file context>
@@ -363,9 +365,97 @@ def iter_jobs(workflows: Path = WORKFLOWS) -> Iterable[tuple[Job, bool]]:
+ fork = "github.repository_owner != 'manaflow-ai' && 'ubuntu-24.04'"
+ hosted = "github.run_attempt == 1 && 'ubuntu-24.04'"
+ paid = {"'blacksmith-16vcpu-ubuntu-2404'", "'blacksmith-32vcpu-ubuntu-2404'"}
+ sensitive = re.compile(r"secrets\.(?:APPLE|SIGN|CMUX_RELEASE|NOTARY)|codesign|notarytool|signing-keychain|gh release upload|softprops/action-gh-release", re.I)
+ for path in sorted(workflows.glob("*.y*ml")):
+ source = path.read_text()
</file context>
| return "early admission must fall back to fleet Linux without a paid literal" | ||
| if len(admitted_terms) != 1 or not re.fullmatch(r"needs\.decide\.outputs\.early_[a-z_]+_runner", admitted_terms[0]): | ||
| return "early admission must use only its verified runner output" | ||
| if not alternatives[-1:] or alternatives[-1] not in ("'blacksmith-16vcpu-ubuntu-2404'", "'blacksmith-32vcpu-ubuntu-2404'"): |
There was a problem hiding this comment.
P1: This validation does not reject an unverified fallback inserted between early admission and the final Blacksmith label. Validate every fallback alternative, not just the first and last, so a missing early output cannot queue on an arbitrary runner.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/ci/runner_routing_table.py, line 119:
<comment>This validation does not reject an unverified fallback inserted between early admission and the final Blacksmith label. Validate every fallback alternative, not just the first and last, so a missing early output cannot queue on an arbitrary runner.</comment>
<file context>
@@ -114,8 +114,10 @@ def portable_order(runs_on: str, nightly: bool = False) -> str:
- return "early admission must fall back to fleet Linux without a paid literal"
+ if len(admitted_terms) != 1 or not re.fullmatch(r"needs\.decide\.outputs\.early_[a-z_]+_runner", admitted_terms[0]):
+ return "early admission must use only its verified runner output"
+ if not alternatives[-1:] or alternatives[-1] not in ("'blacksmith-16vcpu-ubuntu-2404'", "'blacksmith-32vcpu-ubuntu-2404'"):
+ return "early admission must retain measured Linux overflow"
if "needs.decide.outputs.early_attempt == format('{0}', github.run_attempt)" not in admitted:
</file context>
| issue = "must name a runnable lightweight job" | ||
| elif job.get("environment") not in (None, "ci-routing-agent") or sensitive.search(json.dumps(job)): | ||
| issue = "cannot authorize release/signing credentials or operations" | ||
| elif re.search(r"(?:^|[-_])(sign|signer|signing|publish|promote|release)(?:[-_]|$)", str(name)) or str(name) in {"route-visual-gate-nightly", "nightly-identity-legacy-published-in"}: |
There was a problem hiding this comment.
P1: The hosted exception can authorize issue-writing jobs because the denylist omits the declared report/close jobs and the sensitive scan ignores permissions: write. Reject write-scoped jobs (and other credentialed operations) before emitting an exemption.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/ci/runner_routing_table.py, line 401:
<comment>The hosted exception can authorize issue-writing jobs because the denylist omits the declared report/close jobs and the sensitive scan ignores `permissions: write`. Reject write-scoped jobs (and other credentialed operations) before emitting an exemption.</comment>
<file context>
@@ -363,9 +365,97 @@ def iter_jobs(workflows: Path = WORKFLOWS) -> Iterable[tuple[Job, bool]]:
+ issue = "must name a runnable lightweight job"
+ elif job.get("environment") not in (None, "ci-routing-agent") or sensitive.search(json.dumps(job)):
+ issue = "cannot authorize release/signing credentials or operations"
+ elif re.search(r"(?:^|[-_])(sign|signer|signing|publish|promote|release)(?:[-_]|$)", str(name)) or str(name) in {"route-visual-gate-nightly", "nightly-identity-legacy-published-in"}:
+ issue = "cannot authorize release/signing or credentialed publication routes"
+ if issue:
</file context>
| problem = pool.snapshot_problem(snapshot, now) | ||
| if (runners is None or problem) and env.get("NIGHTLY") == "1" and env.get("NIGHTLY_WAIT_FOR_FLEET") == "1": | ||
| if ((runners is None or problem) and env.get("NIGHTLY") == "1" | ||
| and env.get("NIGHTLY_WAIT_FOR_FLEET") == "1" and env.get("REQUIRE_LIVE_CAPACITY") != "1"): |
There was a problem hiding this comment.
P1: This adds a runner-routing change to the permission-tightening PR: an unavailable runner API or stale snapshot now moves the nightly picker from the configured owned mini to hosted Linux. Preserve the existing fallback here, or explicitly update the workflow contract and PR scope for this routing change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/ci/portable_job_placement.py, line 167:
<comment>This adds a runner-routing change to the permission-tightening PR: an unavailable runner API or stale snapshot now moves the nightly picker from the configured owned mini to hosted Linux. Preserve the existing fallback here, or explicitly update the workflow contract and PR scope for this routing change.</comment>
<file context>
@@ -161,7 +163,8 @@ def done(why: str, mini: str = "", mini_jobs: tuple[str, ...] = ()) -> Placement
problem = pool.snapshot_problem(snapshot, now)
- if (runners is None or problem) and env.get("NIGHTLY") == "1" and env.get("NIGHTLY_WAIT_FOR_FLEET") == "1":
+ if ((runners is None or problem) and env.get("NIGHTLY") == "1"
+ and env.get("NIGHTLY_WAIT_FOR_FLEET") == "1" and env.get("REQUIRE_LIVE_CAPACITY") != "1"):
# An exhausted routing API is not evidence of a fleet no-start. Queue
# these secret-free controls on the already configured owned pool,
</file context>
| "nightly.yml#decide": {"class": "portable-checks", "reason": "routing bootstrap"}, | ||
| "nightly.yml#pick-nightly-build-runner": {"class": "portable-checks", "reason": "runner picker"}, | ||
| "nightly.yml#resolve-nightly-cmux-tui-client": {"class": "portable-checks", "reason": "client reuse lookup"}, | ||
| "nightly.yml#resolve-nightly-ghostty-cli-helper": {"class": "portable-checks", "reason": "helper reuse lookup"}, |
There was a problem hiding this comment.
P2: This declaration permits resolve-nightly-ghostty-cli-helper to run on hosted Ubuntu while it publishes a GitHub artifact, contradicting the hosted_light_jobs security invariant. Remove this job from the hosted-light allowlist, or update the routing/policy so artifact-uploading jobs cannot take the hosted route.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At .github/runners.json, line 436:
<comment>This declaration permits `resolve-nightly-ghostty-cli-helper` to run on hosted Ubuntu while it publishes a GitHub artifact, contradicting the `hosted_light_jobs` security invariant. Remove this job from the hosted-light allowlist, or update the routing/policy so artifact-uploading jobs cannot take the hosted route.</comment>
<file context>
@@ -428,5 +424,21 @@
+ "nightly.yml#decide": {"class": "portable-checks", "reason": "routing bootstrap"},
+ "nightly.yml#pick-nightly-build-runner": {"class": "portable-checks", "reason": "runner picker"},
+ "nightly.yml#resolve-nightly-cmux-tui-client": {"class": "portable-checks", "reason": "client reuse lookup"},
+ "nightly.yml#resolve-nightly-ghostty-cli-helper": {"class": "portable-checks", "reason": "helper reuse lookup"},
+ "nightly.yml#find-nightly-next-recovery": {"class": "portable-checks", "reason": "recovery lookup"},
+ "nightly.yml#visual-gate-nightly-verdict": {"class": "portable-checks", "reason": "visual verdict bookkeeping"},
</file context>
| selected = f"needs.decide.outputs.early_{name.replace('cmux-', '').replace('-', '_')}_runner" | ||
| self.assertIn(selected, route) | ||
| self.assertLess(route.index(selected), route.index("'cmux-fleet-linux-x64'")) | ||
| self.assertLess(route.index(selected), route.index("'blacksmith-16vcpu-ubuntu-2404'")) |
There was a problem hiding this comment.
P2: This weaker assertion no longer verifies that early placement wins before the attempt-1 fallback; it only checks that the selected runner appears before Blacksmith. Assert that the selected runner precedes the hosted fallback as well, otherwise a routing regression can pass this test unnoticed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/test_ci_portable_job_placement.py, line 283:
<comment>This weaker assertion no longer verifies that early placement wins before the attempt-1 fallback; it only checks that the selected runner appears before Blacksmith. Assert that the selected runner precedes the hosted fallback as well, otherwise a routing regression can pass this test unnoticed.</comment>
<file context>
@@ -264,7 +280,7 @@ def test_early_nightly_controls_try_fleet_before_blacksmith_without_signing_secr
selected = f"needs.decide.outputs.early_{name.replace('cmux-', '').replace('-', '_')}_runner"
self.assertIn(selected, route)
- self.assertLess(route.index(selected), route.index("'cmux-fleet-linux-x64'"))
+ self.assertLess(route.index(selected), route.index("'blacksmith-16vcpu-ubuntu-2404'"))
self.assertIn("needs.decide.outputs.track == 'nightly-next'", route)
self.assertNotIn("secrets.APPLE", yaml.safe_dump(job))
</file context>
| if: github.event_name != 'workflow_dispatch' || ((inputs.probe_mode || 'off') == 'off' && inputs.promote_nightly_next_sha == '') | ||
| runs-on: ${{ github.repository_owner != 'manaflow-ai' && 'ubuntu-24.04' || github.run_attempt == 1 && !startsWith(vars.CI_CONTROL_RUNNER, 'blacksmith-') && vars.CI_CONTROL_RUNNER || github.ref == 'refs/heads/nightly-next' && github.event_name == 'push' && github.run_attempt == 1 && vars.CI_PR_POOL_OWNED == '1' && vars.CI_NIGHTLY_NEXT_BUILD_RUNNER || vars.LINUX_RUNNER == 'blacksmith-16vcpu-ubuntu-2404' && 'blacksmith-16vcpu-ubuntu-2404' || 'blacksmith-32vcpu-ubuntu-2404' }} | ||
| # Bootstrap has no earlier live runner receipt; declared light work uses hosted Linux. | ||
| runs-on: ${{ github.repository_owner != 'manaflow-ai' && 'ubuntu-24.04' || github.run_attempt == 1 && 'ubuntu-24.04' || vars.LINUX_RUNNER == 'blacksmith-16vcpu-ubuntu-2404' && 'blacksmith-16vcpu-ubuntu-2404' || 'blacksmith-32vcpu-ubuntu-2404' }} |
There was a problem hiding this comment.
P2: This line changes first-attempt routing from the configured control/build runners to GitHub-hosted Linux, contrary to the stated intent that runner routing remain unchanged. Revert these routing changes, or explicitly document and validate the new hosted bootstrap path—including its different network, capacity, and cost behavior—in this PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At .github/workflows/nightly.yml, line 194:
<comment>This line changes first-attempt routing from the configured control/build runners to GitHub-hosted Linux, contrary to the stated intent that runner routing remain unchanged. Revert these routing changes, or explicitly document and validate the new hosted bootstrap path—including its different network, capacity, and cost behavior—in this PR.</comment>
<file context>
@@ -188,11 +188,10 @@ jobs:
if: github.event_name != 'workflow_dispatch' || ((inputs.probe_mode || 'off') == 'off' && inputs.promote_nightly_next_sha == '')
- runs-on: ${{ github.repository_owner != 'manaflow-ai' && 'ubuntu-24.04' || github.run_attempt == 1 && !startsWith(vars.CI_CONTROL_RUNNER, 'blacksmith-') && vars.CI_CONTROL_RUNNER || github.ref == 'refs/heads/nightly-next' && github.event_name == 'push' && github.run_attempt == 1 && vars.CI_PR_POOL_OWNED == '1' && vars.CI_NIGHTLY_NEXT_BUILD_RUNNER || vars.LINUX_RUNNER == 'blacksmith-16vcpu-ubuntu-2404' && 'blacksmith-16vcpu-ubuntu-2404' || 'blacksmith-32vcpu-ubuntu-2404' }}
+ # Bootstrap has no earlier live runner receipt; declared light work uses hosted Linux.
+ runs-on: ${{ github.repository_owner != 'manaflow-ai' && 'ubuntu-24.04' || github.run_attempt == 1 && 'ubuntu-24.04' || vars.LINUX_RUNNER == 'blacksmith-16vcpu-ubuntu-2404' && 'blacksmith-16vcpu-ubuntu-2404' || 'blacksmith-32vcpu-ubuntu-2404' }}
outputs:
early_build_nightly_inputs_runner: ${{ steps.place-early.outputs.build_nightly_inputs_runner }}
</file context>
| job = DOC['jobs']['decide'] | ||
| self.assertEqual(job['permissions'], {'actions': 'read', 'contents': 'read'}) | ||
| script = next(step['with']['script'] for step in job['steps'] if step.get('id') == 'decide') | ||
| self.assertNotRegex(script, r"github\.rest\.[A-Za-z]+\.(?:create|update|delete|upload)") |
There was a problem hiding this comment.
P2: This negative check misses write calls that use optional chaining, such as github.rest.repos?.createRelease, even though the decide script already uses optional chaining for REST access. Match optional namespace access and suffixed write methods so the permission regression test cannot silently pass.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/test_nightly_trusted_routing.py, line 122:
<comment>This negative check misses write calls that use optional chaining, such as `github.rest.repos?.createRelease`, even though the decide script already uses optional chaining for REST access. Match optional namespace access and suffixed write methods so the permission regression test cannot silently pass.</comment>
<file context>
@@ -115,42 +115,38 @@ def test_next_build_never_waits_on_one_label_with_no_fallback(self):
+ job = DOC['jobs']['decide']
+ self.assertEqual(job['permissions'], {'actions': 'read', 'contents': 'read'})
+ script = next(step['with']['script'] for step in job['steps'] if step.get('id') == 'decide')
+ self.assertNotRegex(script, r"github\.rest\.[A-Za-z]+\.(?:create|update|delete|upload)")
+ self.assertNotIn('core.exportVariable', script)
+
</file context>
| self.assertNotRegex(script, r"github\.rest\.[A-Za-z]+\.(?:create|update|delete|upload)") | |
| self.assertNotRegex(script, r"github\.rest(?:\.[A-Za-z_$][A-Za-z0-9_$]*\??)+\.(?:create|update|delete|upload)") |
| result = {job: "ubuntu-24.04" if request["run_attempt"] == 1 and f"nightly.yml#{job}" in light else fallback | ||
| for job in request["jobs"]} |
There was a problem hiding this comment.
P2: This override changes first-attempt routing for three early control jobs to GitHub-hosted Ubuntu when admission returns no receipt, instead of the prior owned/declared fallback. That contradicts the PR’s stated intent to leave runner routing unchanged; remove this override or document and separately approve the routing change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/ci/nightly_linux_admission.py, line 132:
<comment>This override changes first-attempt routing for three early control jobs to GitHub-hosted Ubuntu when admission returns no receipt, instead of the prior owned/declared fallback. That contradicts the PR’s stated intent to leave runner routing unchanged; remove this override or document and separately approve the routing change.</comment>
<file context>
@@ -117,18 +118,19 @@ def routes(env: Mapping[str, str], request: Mapping[str, Any], receipt: Mapping[
+ # The central table is the only authority for hosted-light eligibility.
+ table = json.loads((Path(__file__).resolve().parents[2] / ".github/runners.json").read_text())
+ light = table.get("hosted_light_jobs", {})
+ result = {job: "ubuntu-24.04" if request["run_attempt"] == 1 and f"nightly.yml#{job}" in light else fallback
+ for job in request["jobs"]}
for job in placed.mini_jobs:
</file context>
| result = {job: "ubuntu-24.04" if request["run_attempt"] == 1 and f"nightly.yml#{job}" in light else fallback | |
| for job in request["jobs"]} | |
| result = {job: fallback for job in request["jobs"]} |
82cd657 to
375ff19
Compare
|
Review (codex gpt-6.1-sol high): SHIP at 375ff19 Rechecked the exact final head after its one rebase onto the current base with the per-job secret allowlist. The diff remains only the decide permission reduction and focused regression. Decide uses read-only |
|
cmux status (one comment, edited in place; each line is updated by the job that owns it) Review App of this head (app.zip, kept 1 day)
Shipped in:
|
nightly.yml'sdecidejob only reads repository metadata and emits routing/build outputs, but it requestedcontents: write,attestations: write, andid-token: write. Remove the unused write grants and lock the job toactions: readpluscontents: read.Follow-up to #30173 and fixes #30235. The independent review receipt identified this exact least-privilege gap: “
decidekeeps unusedid-token: writeandattestations: write, and itscontents: writeis broad.”The decide script uses read-only refs, tags, release metadata, commit comparison, and commit timestamps. It does not create, update, delete, upload, export credentials, attest artifacts, or request an OIDC token. The placement steps continue to use their existing action and snapshot read paths. No runner routing or workflow behavior changes.
Validation:
python3 -m unittest tests/test_nightly_trusted_routing.py— 34 passed.git diff --checkpasses.Agent session: agent session
Need help on this PR? Tag
@codesmith-botwith what you need.Summary by cubic
Trims
nightly.yml'sdecidejob to read-only permissions (actions: read,contents: read), removing the unusedcontents: write,attestations: write, andid-token: writegrants flagged in #30235. The decide script only reads repository refs, tags, releases, and commit metadata and emits routing outputs; it never creates, updates, deletes, uploads, attests artifacts, or requests an OIDC token. Adds a regression test asserting the exact read-only permission map and rejecting write-style REST calls in the decide script.Written for commit 375ff19. Summary will update on new commits.