Skip to content

sec(2157): the sync mints write only for the mode that writes - #355

Merged
LukasWodka merged 4 commits into
developfrom
sec/2157-scope-the-standards-sync-mint
Aug 27, 2026
Merged

sec(2157): the sync mints write only for the mode that writes#355
LukasWodka merged 4 commits into
developfrom
sec/2157-scope-the-standards-sync-mint

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What

Last of the four mints in backend#2157. standards-sync.yml minted the App's entire permission set across every repo the installation covers.

Why conditional, not a union

This workflow has two modes with genuinely different needs:

  • report (schedule) — reads every repo's CLAUDE.md and classifies it. Reads only.
  • --create-prs (manual dispatch) — PUTs CLAUDE.md and POSTs a branch ref in each drifted repo. Needs contents: write fleet-wide.

Scoping to the union would keep fleet-wide contents: write on every scheduled report — which is the mismatch, not the fix. So the contents scope follows the mode:

permission-contents: ${{ (github.event_name == 'workflow_dispatch' && inputs.create-prs == true) && 'write' || 'read' }}permission-pull-requests: readpermission-issues: write

On a schedule run inputs is empty, the condition is false, and the same job gets read.

Derived from the calls this token makes, one at a time

callscope
git/ref/heads/develop, repos/{r}, contents/CLAUDE.md?ref=contents: read
PUT contents/CLAUDE.md, POST git/refscreate-prs onlycontents: write
gh pr listpull-requests: read
gh issue comment 1602 (the drift report)issues: write

Not pull-requests: writepr create and both pr edit calls run on SYNC_PR_AUTHOR_TOKEN, not on this token (backend#2590). The App only lists.

Two corrections to what I said on the ticket earlier

Both from reading the file rather than my own summary:

  1. The minting job is gated on schedule || workflow_dispatch (job-level if:), so it never minted on push/pull_request/issues at all. The exposure was real, but narrower than the "nearly every run" I claimed.
  2. == true rather than a bare truthiness test, matching the CREATE_PRS line below it. Equivalent for a type: boolean input — but if it ever became a string, 'false' is truthy and this would mint write on every scheduled run while still looking conditional.

Closes the set, but carries no closing keyword

With #349, #352 and #353, this empties EXEMPT entirely: 4 unscoped mints and 4 exemptions → 0 and 0.

Deliberately no Closes keyword: merge order is not guaranteed, and if this landed first it would close #2157 with three mints still open. Close the ticket by hand once all four are in.

Test plan

  • make lint — clean
  • make selftests — 29 passed, 0 failed
  • actionlint — clean (the expression parses)
  • make mint-scope — 3 unscoped / 3 exempted / 0 findings on this branch alone
  • Live: the next scheduled report exercises the read arm; a create-prs: true dispatch exercises the write arm. The write arm is the one that cannot be rehearsed without opening real PRs — so it should be dispatched deliberately, watched, and not left to the cron.

Part of tracebloc/backend#2157


Note

Medium Risk
Changes fleet-wide App token permissions for a workflow that writes repo contents in one mode; a wrong condition could break remediation or over-mint write on scheduled runs.

Overview
Scopes the org-standards sync workflow’s GitHub App installation token so scheduled report runs mint contents: read only, while a manual workflow_dispatch with create-prs: true mints contents: write for the fleet CLAUDE.md push path. The step now sets explicit permission-pull-requests: read and permission-issues: write (drift report on issue 1602), with == true on the boolean input so a future string 'false' would not silently grant write.

Completes the mint-scope burn-down by removing the last standards-sync.yml row from EXEMPT in scripts/mint-scope.py, leaving an empty map that still flags any re-introduced full-grant mints.

Reviewed by Cursor Bugbot for commit 785465b. Bugbot is set up for automated code reviews on this repo. Configure here.

backend#2157, LAST OF FOUR. With #349, #352 and #353 this empties EXEMPT.
Scoping this to the UNION would have kept fleet-wide contents:WRITE on every
scheduled report -- which is the mismatch, not the fix, since `report` mode
only reads. So the contents scope is conditional on the mode:
permission-contents: ${{ (dispatch && create-prs == true) && write || read }}
On a schedule run `inputs` is empty, the condition is false, and the same job
gets READ.
DERIVED FROM THE CALLS THIS TOKEN MAKES, checked one at a time:
git/ref/heads/develop, repos/{r}, contents/CLAUDE.md?ref= -> contents: read
PUT contents/CLAUDE.md, POST git/refs (create-prs only) -> contents: WRITE
gh pr list -> pull-requests: read
gh issue comment 1602 (the drift report) -> issues: WRITE
NOT pull-requests:write: `pr create` and both `pr edit` calls run on
SYNC_PR_AUTHOR_TOKEN, not on this token (backend#2590). The App only LISTS.
TWO CORRECTIONS TO WHAT I SAID EARLIER ON THE TICKET, both from reading the
file rather than the summary:
* The minting job is gated on `schedule || workflow_dispatch`, so it never
minted on push/pull_request/issues. The exposure was real but narrower
than "nearly every run".
* `== true` rather than a bare truthiness test, matching the CREATE_PRS line
below it. Equivalent for a `type: boolean` input -- but if it ever became
a string, `false` is TRUTHY and this would mint write on every scheduled
run while still looking conditional.
Verified: make lint clean, selftests 29 passed, actionlint clean, mint-scope
3 unscoped / 3 exempted / 0 findings on this branch (the other three are on
their own branches; together they reach 0 and 0).
Part of tracebloc/backend#2157 -- the LAST of four. Merge order is not
guaranteed, so this does not carry a closing keyword: if it landed first it
would close the ticket with three mints still unscoped. Close #2157 by hand
once #349, #352, #353 and this one are all in.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 26, 2026
backend#2157. `set-pr-status.yml` landed on develop (#349) and dropped its
own EXEMPT row; this branch drops `standards-sync.yml`'s. Adjacent line
deletions, so git called it a conflict -- both are correct and the
resolution keeps NEITHER row.
EXEMPT is now fr-gate + kanban-reconcile, and the prose above it is updated
to say two rather than four. It already said four on develop while three
remained; this fixes that drift rather than adding a second wrong number.
Verified: make lint exit 0 (mint-scope 2 unscoped / 2 exempted / 0
findings, "exemptions all still apply"), make selftests exit 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
saadqbal
saadqbal previously approved these changes Aug 26, 2026

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional rather than union is the right shape here — a union would have kept fleet-wide contents:write on every scheduled report, which is the mismatch and not the fix.

I went to check inputs.create-prs == true for the classic trap and found you'd already reasoned it out in the comment. Worth stating the direction explicitly, because it's the whole point: the input is type: boolean today so == true is exact, and if it ever became a string, == true fails to read (write denied, create-prs breaks loudly) while a bare truthiness test fails to write'false' being truthy — minting fleet-wide write on every scheduled run while still looking conditional. You picked the direction that fails closed on a permission. That's the correct trade and the comment should survive any future refactor.

Confirmed pull_request and push events also land on read, since the condition pins github.event_name to workflow_dispatch. 👍

backend#2157. #353 (kanban-reconcile) landed on develop and dropped its
EXEMPT row; this branch drops standards-sync's. Same adjacent-deletion
conflict as the last merge -- both deletions are correct, keep NEITHER.
fr-gate is now the ONLY exemption left, so the prose says one rather than
two and names why fr-gate is last: it is a REQUIRED check on every
promotion branch, where a bad scope reddens every hop at once.
Verified: make lint exit 0 (mint-scope 1 unscoped / 1 exempted / 0
findings, "exemptions all still apply"), make selftests 29 passed 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LukasWodka added a commit that referenced this pull request Aug 27, 2026
backend#2157. #353 (kanban-reconcile) landed on develop and dropped its
EXEMPT row; this branch drops fr-gate's. Adjacent-line deletions, so git
called it a conflict -- both are correct, keep NEITHER.
standards-sync is now the only exemption left, so the prose says one rather
than two and names why it is last: a contents:write sweep whose risky mode
cannot be rehearsed without opening 19 real PRs.
NOTE FOR WHOEVER MERGES SECOND: #355 drops standards-sync and resolves the
same dict to {fr-gate}. The two are mutually exclusive by construction --
whichever lands first, the other needs one more develop merge. That is the
cost of scoping four mints in four PRs and is not a defect in either.
Verified: make lint exit 0 (mint-scope 1 unscoped / 1 exempted / 0
findings, "exemptions all still apply"), make selftests 29 passed 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
saadqbal
saadqbal previously approved these changes Aug 27, 2026

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving. My earlier stamp was dismissed by the develop merge; I checked what moved rather than assuming — bdd979cd came in from develop, and the PR's own diff picked up scripts/mint-scope.py: the standards-sync.ymlEXEMPT row removal and the prose rewrite. Both belong here, since scoping the workflow without dropping its row would fail the staleness check.

The conditional permission-contents I verified last time is unchanged, and the == true reasoning still reads correctly against the type: boolean declaration.

One thing for the pair.#352 rewrites the same comment block and the same dict, mirror-image to this one: it says "the ONE left … standards-sync", this says "the ONE left … fr-gate". Each holds only while the other is open. So whichever lands second conflicts here — and if that resolution keeps the other's EXEMPT row, mint-scope fails on stale, which is the correct direction and is caught. What isn't caught: once both land, EXEMPT is {} while the surviving prose still says one exemption remains. The guard looks for rows that no longer mint, not for prose describing rows that don't exist. Best fixed in the second PR's conflict resolution — turn "the ONE left" into the retrospective of all four.

Given "a stale exemption is cover for the next unscoped mint in that file" is the thesis, ending the burn-down with a stale description would be a slightly ironic place to stop. 👍

The conflict is in `mint-scope.py`'s EXEMPT map, and it is a real one rather
than a textual collision: #352 scoped fr-gate and left standards-sync as the
last row, this branch scopes standards-sync and left fr-gate as the last row.
Each side is correct about its own workflow and wrong about the other's only
because it was written before the other landed.
Resolved to the state both sides were walking toward: EXEMPT is now EMPTY. The
two were split into separate PRs precisely because each needed its own window
-- fr-gate is a REQUIRED check on every promotion branch fleet-wide, and
standards-sync is a contents:write sweep whose risky mode cannot be rehearsed
without opening 19 real PRs -- so both landing is what completes the burn-down,
not an accident of merge order.
Taking either side's row verbatim would have re-exempted a workflow that is now
scoped, which `stale_exemptions` would then correctly report as a finding. The
prose above the map is rewritten to say the burn-down is finished and, more
importantly, that an empty map is not an inert one: `stale_exemptions` compares
against the live unscoped set, so a re-introduced full-grant mint has no row to
hide behind. It also says outright not to add a row back to green a red run --
scope the workflow instead, which is what every row here was eventually
replaced by.
No tally was introduced; the file's own header forbids one (saadqbal, #287) and
the count is printed from `len(_exempt())`.
Verified on the merged tree: `mint-scope.py` reports 17 mint steps across 37
workflow files, 0 unscoped / 0 exempted / 0 findings -- so the empty map is
measured, not asserted. `make check` green (ruff, shellcheck, house-rules,
action-pins, mint-scope, actionlint, mutations-dry, and all selftests: 29
passed, 0 failed).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Re-requesting review: develop moved under this branch when #352 merged, and the merge conflicted in scripts/mint-scope.py's EXEMPT map.

The conflict was real rather than textual. #352 scoped fr-gate and left standards-sync as the last exempt row; this branch scopes standards-sync and left fr-gate as the last row. Each side was right about its own workflow and stale about the other's only because it was written first.

Resolved to EXEMPT = {} — the state both sides were walking toward. Taking either row verbatim would have re-exempted a workflow that is now scoped, which stale_exemptions would then correctly report as a finding.

Measured on the merged tree rather than asserted: mint-scope.py reports 17 mint steps across 37 workflow files, 0 unscoped / 0 exempted / 0 findings. make check green (29 selftests passed, 0 failed).

The prose above the map is rewritten to say the burn-down is complete and that an empty map is not an inert one — stale_exemptions still compares against the live unscoped set, so a re-introduced full-grant mint has no row to hide behind. It also says not to add a row back to green a red run.

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving — and the prose resolution is better than what I asked for. I suggested turning "the ONE left" into a retrospective; "NONE ARE LEFT, and that is a burn-down completing rather than a list being deleted", with both windows and their reasons named, is the version that will still make sense to someone reading it cold in six months.

The part I'd have missed if you hadn't written it: AN EMPTY MAP IS NOT AN INERT ONE. I checked it rather than taking it, because an empty collection quietly disabling a guard is the exact shape of bug I've been finding elsewhere this week — and here it's the opposite. EXEMPT: dict = {} with offenders = [w for w in unscoped if w not in _exempt()] means an empty map is the guard's strictest state: every unscoped mint is an offender, with no row to hide behind. stale_exemptions correctly reports nothing, since there's nothing that could go stale. So the burn-down ends with the check tighter than it started, and the comment says so where the next person will look.

Green, no threads, and the #352 conflict resolved cleanly. 👍

@LukasWodka
LukasWodka merged commit 65cabec into developAug 27, 2026
14 checks passed
@LukasWodka
LukasWodka deleted the sec/2157-scope-the-standards-sync-mint branch August 27, 2026 05:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@saadqbal