Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion .github/workflows/release-please.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,25 @@ jobs:
release-please:
if: github.repository == 'sentdm/sent-dm-ruby'
runs-on: ubuntu-latest
env:
HAS_APP: ${{ secrets.SDK_APP_ID != '' && secrets.SDK_APP_PRIVATE_KEY != '' }}

steps:
- name: Mint the release token from the GitHub App
# A tag or release created with the built-in token starts no
# workflow, so the publish workflow would never run; a personal
# access token expires or dies with its owner's account. The App's
# token does neither.
id: app
if: env.HAS_APP == 'true'
uses: actions/create-github-app-token@fee1f7d63c2ff003460e3d139729b119787bc349 # v3.3.0
with:
app-id: ${{ secrets.SDK_APP_ID }}
private-key: ${{ secrets.SDK_APP_PRIVATE_KEY }}
owner: sentdm
repositories: ${{ github.event.repository.name }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 The PR description and commit message break the public-repo rule by naming an internal repository and tracker. They name sent-dm-sdk-pipeline and say "Part of EPIC-4427". CLAUDE.md: "This repository is public. Nothing in it, including PR text, CHANGELOG.md and release notes, links to or names Sent's private repositories, internal systems or providers." The commit body is also kept permanently in public history. Fix: before merging, reword the commit message and PR description so they say "the GitHub App" and drop the private repo name and the epic ID.

Why this was flagged

Commit 1949b43's message says "The token now comes from the sent-dm-sdk-pipeline App", and sentdm/sent-dm-sdk-pipeline is the private config repo (see CONFIG_REPO in .github/workflows/seal-dispatch.yml). The PR description repeats that name and adds the internal tracker ID EPIC-4427. CLAUDE.md's Commits and pull requests section forbids naming private repositories or internal systems anywhere in the public repo, PR text included. REVIEW.md lists this as Important. Once the PR merges, the commit message is part of public history on main and can no longer be removed without rewriting history. The diff lines themselves only use the secret names and owner: sentdm, so the leak is in the commit and PR text, not in the workflow code.

Verification: nit. Trigger: the PR merges with its current commit message and description. Commit 1949b43's body says "The token now comes from the sent-dm-sdk-pipeline App". The PR description repeats that name and adds "Part of EPIC-4427", which is an internal tracker ID. The same repo name is already committed in the public base, at seal-dispatch.yml lines 37 and 48.


Comment on lines +24 to +32

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 (optional) If the App is not installed on sent-dm-ruby with contents and pull-requests write, the release job now fails and still cuts no release. The step mints a token for ${{ github.event.repository.name }}. The only existing App use, seal-dispatch.yml, mints for sent-dm-sdk-pipeline alone, so access to this repo is unproven. A failed step stops the job, so the || secrets.RELEASE_PLEASE_TOKEN fallback on line 36 never runs. Fix: confirm the App installation covers this repository with contents: write and pull-requests: write, or pass those as permission-* inputs so a missing grant fails loudly at minting time.

Why this was flagged

Trigger condition (not verifiable from this checkout): the SDK_APP_ID App installation does not include sentdm/sent-dm-ruby, or it lacks Contents and Pull requests write permission. The only other caller, .github/workflows/seal-dispatch.yml:43-48, mints with repositories: sent-dm-sdk-pipeline and needs only dispatch access to that repo. Because HAS_APP is true once the secrets exist (line 16), the step at line 24 runs. actions/create-github-app-token errors when the repo is not in the installation, and with no continue-on-error the job stops before release-please runs. If the token mints but lacks write scopes, release-please fails on tag, release or PR creation instead. Either way the merge to main still cuts no tag and publish-gem.yml (release: trigger) never runs, which is the same outcome the PR says it fixes.

Verification: Line 16 sets HAS_APP to true whenever both App secrets exist, so the mint step (lines 19-31) runs. It has no continue-on-error. If the App is not installed on the repo, the step fails and the job stops, so line 36's || secrets.RELEASE_PLEASE_TOKEN fallback never runs. With a valid PAT restored, the job would still fail here, where the base would succeed.

- uses: googleapis/release-please-action@5c625bfb5d1ff62eadeeb3772007f7f66fdcf071 # v4.4.1
id: release
with:
token: ${{ secrets.RELEASE_PLEASE_TOKEN }}
token: ${{ steps.app.outputs.token || secrets.RELEASE_PLEASE_TOKEN }}
Comment on lines +15 to +36

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Every SDK's release can be blocked when the generator next regenerates this release workflow, which this PR edits by hand. Release Please may also go back to the revoked PAT. release-please.yml is generator-owned per CLAUDE.md, and this hand-written commit has no Stainless-Generated-From trailer. It also changes the generator-written token: line at :36. CLAUDE.md: "Changing lines the generator writes conflicts the next time the API document changes, and a conflict blocks every SDK's release". Fix: move the App-token step and the token: change into the SDK configuration so the generator emits them, and keep this repo's copy identical to generated output.

Why this was flagged

The workflow at .github/workflows/release-please.yml is on CLAUDE.md's list of generator-owned files: "the publish, release and sync workflows ... The next generated commit rewrites them, and an edit conflicts." Commit 1949b43 is hand-written with no Stainless-Generated-From trailer. It adds the env/step block at lines 15-32 and rewrites the generator-emitted line token: ${{ secrets.RELEASE_PLEASE_TOKEN }} to token: ${{ steps.app.outputs.token || secrets.RELEASE_PLEASE_TOKEN }} at line 36. On the next regeneration from the pipeline, this patch either conflicts, which per CLAUDE.md blocks every SDK's release, or gets overwritten. If it is overwritten, Release Please goes back to RELEASE_PLEASE_TOKEN, which the PR says is revoked, and tags and gem publishing stop again with "Bad credentials". The base branch has no hand-edited line here, so it has no regeneration conflict. REVIEW.md lists both "A change to a generator-owned file ... outside a generated commit" and "A hand edit to a line the generator writes" as Important.

Verification: It is triggered by the next generated commit that touches the release workflow. Commit 1949b43 has no Stainless-Generated-From trailer. It also rewrites an existing line at .github/workflows/release-please.yml:36. If the generator overwrites the file, :36 goes back to secrets.RELEASE_PLEASE_TOKEN, the PAT the PR says is revoked, and releases fail again with Bad credentials.

Comment on lines +24 to +36

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟣 pre-existing, not blocking: Maintainers can still get the same "Bad credentials" failure and no release whenever the App secrets are missing. The job falls back to a token that no longer exists. When HAS_APP is not 'true', the mint step is skipped and token: falls back to secrets.RELEASE_PLEASE_TOKEN, the revoked PAT. Nothing checks that a usable token was chosen. Fix: drop the dead fallback and fail fast with a clear error when HAS_APP is not 'true'. A guard step like seal-dispatch.yml's [ -n "${DISPATCH_TOKEN:-}" ] || exit 1 would do it. Then a missing-secret setup cannot fail silently as the old credential error, and cannot later pass an empty token once that secret is deleted.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

The trigger is any push to main where secrets.SDK_APP_ID or secrets.SDK_APP_PRIVATE_KEY reads as empty for this job. Examples: org secrets not shared with sent-dm-ruby, or a rotated or renamed secret. Whether they are visible here cannot be checked from this checkout. In that case HAS_APP at release-please.yml:16 is 'false', the app step at :24 is skipped, and steps.app.outputs.token is empty. So :36 passes secrets.RELEASE_PLEASE_TOKEN, which the PR description says was revoked. Release Please fails with the same Bad credentials as on the base branch. The merged release PR (0.42.0, f1d650e) still cuts no tag, and publish-gem.yml never runs. Nothing in the log points to the App secrets. The commit message calls the PAT a fallback "until it is removed". In fact it cannot work, so it is not a safeguard. Once the PAT is deleted, the expression gives an empty token and there is still no guard. seal-dispatch.yml has a check for exactly this case before it uses its fallback token. This file has none.

Verification: pre-existing. Trigger: SDK_APP_ID or SDK_APP_PRIVATE_KEY reads as empty for this job. Line 36 of release-please.yml, token: ${{ steps.app.outputs.token || secrets.RELEASE_PLEASE_TOKEN }}, then passes the PAT the PR description says was revoked. No step checks that a usable token was chosen before release-please-action runs. Graded against the base, this is not a regression.