ci: guard the Flatpak sources against lockfile drift - #490
Merged
Conversation
Flathub builds with the network disabled, so every crate and npm tarball must be pre-declared in packaging/flatpak/generated/. Nothing enforced that: `chore: update dependencies` (23be643) bumped Cargo.lock without re-running generate-sources.sh, leaving 53 crates with no source entry, and CI stayed green -- the failure only surfaces inside Flathub's sandbox, on a crate nobody can download. check-sources.py verifies coverage: every registry crate in Cargo.lock has a source entry, every npm package in the Flatpak lockfile has a tarball. Offline, ~1s, no venv or npm. Replayed against 52f2b4a it reports the 53 missing crates and exits 1, so it does catch the case that motivated it. It deliberately does NOT regenerate-and-diff the tree. The generator runs `npm install --package-lock-only`, which re-resolves `^` ranges against the registry as it is right now, so such a check would go red whenever any transitive dependency publishes -- with nothing wrong in the repo. Freshness is a separate monthly job that regenerates and opens a PR (also workflow_dispatch-able); it runs on the default branch only, so no untrusted code reaches the generator. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
InstaZDLL
added
scope: ci
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLe changement ajoute un vérificateur hors ligne des sources Flatpak. La CI l’exécute pour les modifications concernées. Un workflow mensuel ou manuel régénère les sources et gère une pull request dédiée. ChangesSources Flatpak
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Déclencheur
participant Workflow
participant Génération
participant Validation
participant Git
participant PullRequest
Déclencheur->>Workflow: Lancer le rafraîchissement
Workflow->>Génération: Exécuter generate-sources.sh
Génération-->>Workflow: Retourner les fichiers générés
Workflow->>Validation: Exécuter check-sources.py
Validation-->>Workflow: Retourner le statut
Workflow->>Git: Créer et vérifier le commit signé
Git-->>Workflow: Publier ou ignorer la branche
Workflow->>PullRequest: Mettre à jour ou créer la pull request
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ci.yml:
- Around line 149-162: Disable checkout credential persistence in the checkout
step at .github/workflows/ci.yml:149-162 by adding persist-credentials: false.
Apply the same setting to the checkout step at
.github/workflows/flatpak-sources-refresh.yml:45-45; in that workflow’s push
step, run gh auth setup-git with GH_TOKEN before git push, while preserving
contents: write and pull-requests: write permissions.
In @.github/workflows/flatpak-sources-refresh.yml:
- Around line 75-89: Modifiez l’étape « Commit + open PR » pour obtenir l’OID
distant de la branche via git ls-remote avant le push. Utilisez cet OID comme
valeur explicite de force-with-lease, ou un lease vide si BRANCH n’existe pas
encore, afin d’éviter stale info malgré l’absence de branche amont configurée.
In `@packaging/flatpak/check-sources.py`:
- Around line 96-111: Update check_node to compare the root dependency fields
dependencies, devDependencies, optionalDependencies, and peerDependencies
between package.json and package-lock.json, reporting any mismatch. For
overrides, validate the requested versions against the resolved package entries
in the lockfile tree rather than expecting a root overrides field; preserve the
existing source URL validation.
🪄 Autofix
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: b4b6f4ce-3c46-439e-9e83-1255443559fd
📒 Files selected for processing (4)
.github/workflows/ci.yml.github/workflows/flatpak-sources-refresh.ymlCLAUDE.mdpackaging/flatpak/check-sources.py
Three review findings, all confirmed against the code. The push was broken from the second run on. A bare --force-with-lease compares against the remote-tracking ref, which actions/checkout never fetches for this branch, so once the branch exists remotely the push dies with "stale info" -- reproduced locally against a scratch remote before fixing. Now the lease carries the OID from git ls-remote, with the empty-OID "must not exist" form covering the first run. Both paths verified. Both checkouts drop persist-credentials, so the job token never lands in .git/config -- it matters most in the refresh job, which runs npm install and pip install right after. The push step re-supplies credentials through gh auth setup-git. check-sources.py gained the manifest half it was missing: npm ci refuses to run when package.json and package-lock.json disagree, so adding a dependency without regenerating breaks the Flathub build exactly like an undeclared crate does -- and the tarball coverage check can't see it, since the lockfile stays internally consistent. Overrides are validated against the resolved tree rather than a root `overrides` field, because npm doesn't echo one into the lockfile; the pin only means something if the resolved versions obey it. Verified on four scratch trees: added dep, changed spec and a violated fast-uri override all exit 1, an untouched tree still exits 0. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/flatpak-sources-refresh.yml:
- Around line 46-51: Configure the actions/checkout step to explicitly use the
main branch by adding ref: main alongside persist-credentials. Keep the existing
credential handling unchanged so the generator reads main’s lockfiles and the
pull request target remains consistent.
In `@packaging/flatpak/check-sources.py`:
- Around line 218-228: Update the resolved-package collection in the validation
flow to preserve every version found for the requested package instead of keying
entries by package name. In the block around resolved and the subsequent
version-validation loop, use a set of all matching pkg.get("version") values so
nested node_modules copies cannot overwrite one another, then validate each
collected version and retain the existing skipped behavior when none are found.
- Around line 190-196: Corrigez la logique de l’opérateur « ^ » dans la fonction
de comparaison des versions afin de respecter les bornes npm pour les versions
0.x : pour ^0.minor.patch, exigez la même version majeure et un minor égal à la
borne avec patch suffisant ; pour ^0.0.patch, exigez également le même patch ou
une version ultérieure compatible. Ajoutez des tests couvrant chaque borne et la
première version juste au-dessus.
🪄 Autofix
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: 0f2495a4-8207-41de-b012-b5dd9edece5c
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/flatpak-sources-refresh.ymlpackaging/flatpak/check-sources.py
Three review findings, all confirmed. workflow_dispatch can target any branch, but the PR is always opened against main -- generating from another branch's lockfiles would produce sources that don't match the base. The checkout now pins ref: main, which also makes the header's "default branch only" claim true. The override check kept one version per package name, so the dict overwrote entries: npm nests a second copy under node_modules/<dep>/node_modules/<name> when versions conflict, and an unpatched nested copy could slip through the very check meant to catch it (which survived depended on iteration order). It now collects every resolved version. Verified: a nested fast-uri 3.1.2 alongside a patched top-level copy is reported, where before it wasn't. The caret comparison was "same major", which is wrong below 1.0.0 -- npm pins the leftmost non-zero component there, so ^0.1.2 accepted 0.2.0 and ^0.0.3 accepted 0.1.0. It failed permissively, i.e. it would have blessed a version npm rejects. Now bounded properly, and a prerelease version returns "unverified" instead of being compared as if the suffix weren't there. A --self-test mode covers each boundary and the first version past it (21 cases), wired into CI; all four cases the old logic got wrong are in the table. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/flatpak-sources-refresh.yml (1)
134-135: 🩺 Stability & Availability | 🔵 TrivialVérifiez le déclenchement de la CI pour la PR automatisée.
GITHUB_TOKENne déclenche pas automatiquement les workflows pour la PR créée. Le workflow validecheck-sources.py, mais la CI complète ne s’exécute pas sur cette PR.Si la protection de
mainexige ces checks, la PR reste bloquée. Si elle ne les exige pas, elle peut être fusionnée sans validation complète. Utilisez un jeton GitHub App à permissions minimales, ou déclenchez une validation de confiance séparée. Vérifiez ce comportement sur une exécution réelle.🤖 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/flatpak-sources-refresh.yml around lines 134 - 135, Update the automated PR creation flow in flatpak-sources-refresh.yml so it does not rely on GITHUB_TOKEN, which cannot trigger the required CI workflows. Use a minimally permissioned GitHub App token, or add a separate trusted validation trigger that runs the full CI checks including check-sources.py. Verify the complete CI executes on an actual automated PR and satisfies main branch protection.
🤖 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.
Outside diff comments:
In @.github/workflows/flatpak-sources-refresh.yml:
- Around line 134-135: Update the automated PR creation flow in
flatpak-sources-refresh.yml so it does not rely on GITHUB_TOKEN, which cannot
trigger the required CI workflows. Use a minimally permissioned GitHub App
token, or add a separate trusted validation trigger that runs the full CI checks
including check-sources.py. Verify the complete CI executes on an actual
automated PR and satisfies main branch protection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a5e108f-eb36-44bd-90d3-5db92fde8438
📒 Files selected for processing (3)
.github/workflows/ci.yml.github/workflows/flatpak-sources-refresh.ymlpackaging/flatpak/check-sources.py
The Main ruleset turns out to require signatures, code scanning and code quality. Two consequences the automated PR fell foul of: A commit made with `git commit` in a runner is unsigned, and an unsigned commit in the branch blocks the merge -- so the PR was never mergeable in the first place, before any question of CI. The commit is now built through the Git Data API, which GitHub signs with its own key; same approach as release-please-bump-lockfile.yml, which exists for this reason. That also drops the credential helper and the force-with-lease dance, since nothing pushes over git any more. Events raised by GITHUB_TOKEN deliberately don't start workflow runs, so the required checks never ran. Setting APP_ID + APP_PRIVATE_KEY (a GitHub App with contents:write + pull-requests:write) now makes the PR be opened as that App and CI fires normally; without them the workflow still works and the body says CI has to be kicked off by a close/reopen. Three bugs in my own first cut of this, caught before pushing: a step's own `env` isn't in scope for its `if`, so the App-token guard had to move to job level or it would always skip; `token && '' || 'yes'` always yields 'yes' because the empty string is falsy; and the PR step now gates on the commit step's verdict, since regenerated files can be byte-identical to main even when git diff flagged them. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/flatpak-sources-refresh.yml:
- Around line 20-27: Update the commit creation flow using
github.rest.git.createCommit to produce a signed commit, then validate
commit.data.verification.verified before calling updateRef or createRef and fail
if verification is false. Correct the surrounding comments and pull-request body
so they accurately describe the signing behavior.
- Around line 67-72: Restreindre les permissions du jeton créé par l’étape
app-token en ajoutant uniquement permission-contents: write et
permission-pull-requests: write dans sa configuration with, sans accorder
d’autres permissions.
🪄 Autofix
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: ae3db05d-990f-4141-93d9-89f757693ef1
📒 Files selected for processing (1)
.github/workflows/flatpak-sources-refresh.yml
Both findings applied, one of them after checking the premise rather than the code. The claim my comments made -- that commits created through the Git Data API are signed by GitHub -- turns out to be correct: release-please's own API-created commit dcd7280 reports verified=true, reason=valid, with a signature present. So the comments stay as they are. But the property is load-bearing (an unsigned commit is unmergeable under the ruleset) and invisible if it ever changes, so the workflow now re-reads the commit after creating it and fails loudly, leaving the branch untouched, when verification.verified isn't true. Read back via getCommit rather than trusting the create response, so the check reflects what the API will report to the merge button. The App token is narrowed to permission-contents: write and permission-pull-requests: write -- what the job actually needs for the blob/tree/commit/ref writes and for `gh pr create`. Without those it inherits every permission the App holds on the repo. Both input names verified against the action's own action.yml, since an unknown input is ignored with a warning and would have looked like hardening while granting everything. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #489, which uncovered the problem.
The gap
Flathub builds with the network disabled, so every crate and every npm tarball the build pulls must be pre-declared in
packaging/flatpak/generated/bygenerate-sources.sh. Nothing enforced that.chore: update dependencies(23be643) bumpedCargo.lockwithout re-running the generator, leaving 53 crates with no source entry — wasmtime46.0.1declared against47.0.3in the lock — and CI stayed green the whole time. That failure mode only surfaces inside Flathub's sandbox, on a crate nobody can download.What this adds
packaging/flatpak/check-sources.py— verifies coverage: every registry crate inCargo.lockhas a source entry, every npm package in the Flatpak lockfile has a tarball. Offline, ~1 second, no venv / npm / network. Wired intoci.ymlbehind a paths filter (src-tauri/Cargo.lock,package.json,packaging/flatpak/**).Replayed against
52f2b4a(the commit before #489 fixed it), it reports the 53 missing crates and exits 1 — so it catches the exact case that motivated it, not a hypothetical one.flatpak-sources-refresh.yml— monthly (3rd, 04:00 UTC) +workflow_dispatch. Regenerates, re-runs the check on its own output, and opens/updates a PR when the manifests moved. This is the part that actually bumps the Flatpak dependency set.Why not "regenerate and diff the tree"
That's the obvious design and it would be wrong here.
generate-sources.shrunsnpm install --package-lock-only, which re-resolves^ranges against the registry as it is at that moment — so a regenerate-and-compare check would go red the day any transitive dependency publishes a patch, with nothing wrong in the repo. Contributors would learn to ignore it.So the two concerns are split: CI guards the invariant the build needs (nothing undeclared) on every relevant PR; freshness is a periodic chore. The invariant is deterministic and offline, the chore is neither.
Notes
npm install(with--ignore-scripts) andpip installfrom a commit-pinned upstream.GITHUB_TOKENdon't trigger workflows, so CI won't run on the refresh PR; the job re-runscheck-sources.pyitself before opening it, and the PR body says to close/reopen to kick CI off.setup-pythonSHA that doesn't exist).Checks
bash -n✅check-sources.pyexits 0 onmain, exits 1 with the 53 crates on52f2b4a✅Not verified by me: neither workflow has actually run — the CI job only executes once this lands on a PR that touches a lockfile, and the refresh job needs a dispatch. The refresh job is the riskier of the two (it pushes a branch and opens a PR); worth a manual
workflow_dispatchafter merge to confirm end to end.https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Summary by CodeRabbit
Nouvelles fonctionnalités
Améliorations