chore(flathub): run Flathub's own submission linter (#449) - #614
Conversation
CI has validated the desktop entry and the metainfo for a while. Those answer whether two files are well-formed. Flathub's linter answers whether a submission would be accepted — reproducible sources, permissions Flathub actually grants, and a long list of listing rules no XML validator knows about — and nothing here had ever run it. Wires all three modes into the Flatpak build job: the manifest before the build, and the exported repository plus the AppStream catalogue appstreamcli compose produced (which is not the file appstreamcli validate checked) after the export. The linter comes from org.flatpak.Builder, deliberately unpinned: Flathub reviews with whatever is current, so pinning would make this green against a linter nobody uses. The runner prints every version before any result, so a rule change is visible rather than mysterious. scripts/flathub_builder_lint.py exists for what it refuses to call a pass: - a missing linter exits 2 and says nothing was linted, rather than reporting a clean run of nothing; - any finding without a written reason in flathub-lint-exceptions.json fails, warnings included; - a reason for a finding the linter no longer reports also fails, so the file cannot rot into a suppression list; - an exception with an empty reason is rejected outright. The exceptions file ships empty, which is the state #456 requires. One real fix found without the tool: the summary was 36 characters against Flathub's 35-character cap. Now "Play your own music collection" (30). test/tooling/flathub_metadata_guardrails_test.dart holds that rule and the other cheap ones — trailing period, leading article, repeated app name, name length, required elements, desktop-entry visibility and icon name — so the next one is caught on its own PR instead of in a submission review. Two findings are expected and are not excepted, because neither is the linter being wrong: the manifest CI lints is the development manifest, which builds the working checkout rather than a tagged release (#451), and the metainfo carries no screenshots (#437, #450). Both belong to their issues. Not run here: the linter itself. This container has no flatpak, so the first real output is CI on this PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
) The first CI run answered the question this PR existed to ask, and caught a bug in the asking. flatpak-builder-lint 3.0.0 ran, found nothing in the manifest, printed nothing at all, and exited 0 — and the runner read that empty report as "the linter is probably not installed" and failed the job. Which was exactly backwards: the log line above it printed `flatpak-builder-lint 3.0.0.post798.dev0+5181352`. Silence is now read in both directions. Exit 0 with no output is a clean report, and cannot be confused with a missing tool, because main() has already established that org.flatpak.Builder is installed before any mode runs. A non-zero exit with no output still explains nothing and is still refused. Both cases are tests now rather than an assumption about a tool's output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
…real findings (#449) The lint now runs end to end and has produced its first real output. manifest: clean. appstream: clean. repo: two errors, both the missing screenshots — appstream-screenshots-not-mirrored-in-ostree and metainfo-missing-screenshots. Second runner bug, same shape as the first: not every mode speaks JSON. The appstream mode hands the catalogue to appstreamcli and prints its verdict as text, and the runner refused "Validation was successful." as unreadable output and exited 2 on a mode that had passed. For text output the exit code is now the verdict and the text is printed either way, so a real appstreamcli failure still has to be looked at. The two screenshot errors are not excepted and will not be. Flathub is right to require a screenshot, inventing one from the Android set would misrepresent the desktop window, and the fix is real Linux captures (#437) landed through the metadata pass (#450). Until then the repo mode is red, which is the honest state of the packaging rather than something to paper over — an exceptions entry would only record that we would rather not see it. Also worth recording: the linter does *not* flag the manifest's `type: dir` source. The submission manifest (#451) is still needed, and this check will not tell you so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
First real linter output, and why this PR stays red
Against
Why I am not fixing this hereBoth errors are the same missing thing: the metainfo carries no screenshots. Flathub is right to require one, and the fix is real Linux captures of the desktop window — that is #437, landing through the metadata pass in #450. Inventing them from the Android set would misrepresent the app, and this PR is about the lint, not the listing. I have also not added an exception. So the honest state is: the check exists, it is repeatable, it runs all three modes, and it has found something real that is not fixed yet. Two runner bugs it exposed along the way are fixed ( One thing worth knowing for #451The linter does not flag the manifest's What turns it green#437 → #450 → the screenshots land → the Generated by Claude Code |
Main squash-merged #609, so this branch's copies of the audio smoke's workflow entries conflicted with it. Both sides are kept: the smoke manifest generator test and the Flathub lint runner test both run, and both path filters list both scripts. House style is commas, colons and parentheses rather than em dashes. Prose and comments only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
Repository integrity reviewCLEAN Previously reported repository-integrity findings are resolved. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb65c9aa26
ℹ️ 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".
A failed step ends the job, and the submission lint sits ahead of the launch smoke. So an open finding we are deliberately carrying (missing screenshots, today) does not just turn the badge red: it stops the package being installed and launched in CI at all, on this branch and on main once this merges. That trades real coverage for a red badge. The two answer different questions. The lint answers "would Flathub accept this?", the launch smoke answers "does the package work?". The second is the one worth keeping answered while the first is knowingly open, so it runs first. No change to what either step does, and none to the lint's strictness. Asserted rather than left to a comment: the ordering has a guardrail, verified to fail when the steps are swapped back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
|
You marked this ready, so one consequence of merging it as it stood is worth flagging, and I have pushed a fix for it in f6bb6b2. The lint would have taken the launch smoke down with it. The step order was:
A failed step ends the job. The lint is a hard-failing The two steps answer different questions. The lint answers "would Flathub accept this?", the launch smoke answers "does the package work?". While the first is knowingly open, the second is the one worth keeping answered, so it now runs first. Nothing about the lint's strictness changed, and neither step's behaviour changed. The exceptions file is still empty. The ordering now has a guardrail in Rebuilt on top of your main merge (bb65c9a). This PR still goes red on Generated by Claude Code |
…er failure (#449) Two Codex findings, both real, both latent only because the exceptions file is still empty. They would have bitten the first time anyone wrote a real exception, which is the worst moment for the mechanism to be broken. **Staleness compared across modes that never ran together.** CI lints the manifest before the build and the repo and catalogue after it, in two invocations, and each loads the whole file. So a repo exception looked stale during the manifest run and failed the build, and vice versa. That made documented exceptions unusable in CI. Exception keys now carry their mode ("repo/some-finding"), the staleness check covers only the modes a given invocation linted, and a reason written for one mode no longer accepts a same-named finding from another. An unqualified or unknown-mode key is rejected with a message saying why, rather than silently meaning something. **Text-mode failures collapsed to one name.** Any non-JSON, non-zero result became "<mode>-lint-failed", so a reason attached to it would keep matching after the original problem was fixed and a different one appeared, defeating the guarantee that a reason cannot outlive its problem. Those are now refused outright: the name means the linter itself failed, which is not a submission finding to document. Refused both when the file is loaded and again where findings are matched. The committed exceptions file is still empty, and the policy text and docs/flathub-builder-lint.md now state both rules. 28 runner tests, 469 tooling tests, ruff clean. The cross-mode regression test was verified to fail against the previous logic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
…hots (#449) This PR was red, and the failure was never about the tooling. The repo and appstream modes both report metainfo-missing-screenshots, which is a real submission blocker fixed by taking screenshots (#437), not by anything in the linter or its runner. Holding the whole PR hostage to that helped nobody. The two ways to make it green now are both worse than splitting it: - leave the job red, which trains everyone to ignore the check and buries any genuinely new finding under the one everybody already knows about; - record an exception, which the exceptions file explicitly forbids for a finding that is simply not fixed yet, and which #456 would require removing again anyway. So the capability lands and the gate follows. Kept here: the linter, the runner, its 28 tests, the docs, the exceptions mechanism, and the manifest mode wired into CI, which runs before the build and is green. Removed: the post-build step, and the ordering guardrail that only had something to order. Nothing is suppressed. The runner still drives all three modes, and `--repo`/`--builddir` works today; it is the CI wiring that waits, in #628, alongside the screenshots that let it pass. A guardrail asserts the post-build modes stay unwired until then, so turning them on has to delete a test and is a decision someone makes rather than something that drifts in. 469 tooling tests, 28 runner tests, analyze, format and ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
|
Split this so the tooling can land green, in 5048a92. The red was never about the tooling. What stays: the linter, What moves to #628: the post-build step that lints the exported repo and the AppStream catalogue. Both report Nothing is suppressed. The runner still drives all three modes and The reasoning, since "we'll turn it on later" is exactly the kind of promise that quietly never happens:
So there is a guardrail asserting the post-build modes stay unwired until #628: expect(
workflow,
isNot(contains('--repo flatpak/repo-ci')),
reason: 'wiring the repo mode in needs #628, and this test with it',
);
expect(workflow, contains('#628'),
reason: 'the workflow must say where the missing gate went');Turning them on has to delete that test, in a diff a reviewer reads. That makes it a decision rather than something that drifts in, and it means the missing gate cannot be forgotten. I also removed the launch-smoke-before-lint ordering guardrail I added earlier, since with the post-build lint gone it had nothing left to order. #628 carries it as a step, because the reason still holds: a failed step ends the job, so the lint must not sit ahead of the launch smoke. 469 tooling tests, 28 runner tests, analyze, format and ruff all clean. Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5048a92c7e
ℹ️ 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".
…ummary (#449) Three Codex findings on the split, all correct. **The appstream mode is clean, so it should gate.** I deferred both post-build modes on the premise that both reported the screenshot findings. The CI log says otherwise: both errors come from `repo`, while `appstream` reported "no findings / Validation was successful". So I deferred a passing check for no reason, leaving a future regression in the generated catalogue free to land. It gates now, via --builddir, and only `repo` waits for #628. That also restores the ordering guardrail, which had nothing to order while there was no post-build lint: the launch smoke runs first, because a failed step ends the job. **The summary guardrail measured XML, not text.** tagValue returned the serialized value, so a summary containing a required entity counted "&" as five characters instead of the one a software centre shows. A valid 35-character summary would have failed. Entities are decoded before measuring, and length is counted in runes rather than UTF-16 code units so a non-BMP character does not count twice. Verified with a summary that renders at 35 and spells at 39: it passes now, and would not have. **The documented local sequence could not run.** flatpak-ci.md ends with `cd flatpak` and never returns, so the commands beneath it resolved scripts/ and flatpak/ one directory too deep and failed before reaching the linter. It says `cd ..` now. The docs table also said the wrong thing about which mode reports what, which is what let the first mistake through. It now states per mode what gates and what it reports, and says plainly that appstream and repo are different checks despite both mentioning screenshots. 471 tooling tests, 28 runner tests, analyze, format and ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92ecb7e0eb
ℹ️ 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".
… repo (#449) Two more Codex findings, both correct. **The docs claimed CI lints the exported repository.** The "Where this runs" section still described the job as linting the repo after export, which stopped being true when that mode was deferred to #628 two commits ago. That is the worst kind of stale doc: a maintainer would read a green CI run as evidence the publishable OSTree had been checked, when nothing checked it. It now says plainly that the repo mode is not wired up, that CI passing is not evidence about the OSTree, and where to get that evidence instead. I had corrected the table above it and missed the prose below it, which is exactly how a document ends up contradicting itself. **The summary guardrail decoded only named entities.** `&` and `🎵` are legal ways to write characters that are one glyph on screen, and leaving them encoded counted six or nine characters for one, so a valid summary near the limit would have been rejected. Numeric references now decode first, before the named ones, with an unparseable or out-of-range reference left exactly as written rather than throwing inside a guardrail. Verified both forms against a summary that renders at 35, and confirmed a genuinely over-length summary is still rejected, so this loosens the measurement without loosening the rule. 471 tooling tests, analyze, format clean, metainfo still validates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b34a5e614
ℹ️ 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".
…failure exit (#449) Two Codex findings. The first is a bug I introduced one commit ago while fixing the previous one. **Chained decoding decoded twice.** Running the numeric pass before the named one means `&lt;` becomes `<` becomes `<`: written as nine characters, rendered as nine characters, counted as six. Undercounting is the dangerous direction, because it lets an overlong summary through the cap it exists to enforce. Both kinds of reference are now decoded in a single non-recursive pass. Confirmed against a summary that renders at 42 characters and was previously counted as 26: it is correctly rejected now. **A non-zero exit with structured output read as clean.** If the linter returned valid JSON carrying neither errors nor warnings alongside a failure exit, the exit status was discarded, findings() saw an empty report and the run printed PASS. The text and empty-output paths already refuse an unexplained failure; JSON now does too, as an unexceptable "<mode>-lint-failed" finding, while a report that does explain itself keeps its own findings. Worth recording a process failure alongside these. My first negative control for the second fix silently did nothing: ruff had reformatted the line I was trying to revert onto one line, my replace did not match, and the suite passed for the wrong reason. I read that as evidence the test was weak, when in fact the control was. Reverting again, with an assertion that the revert actually applied, showed the test failing as intended. 30 runner tests, 471 tooling tests, analyze, format and ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
2 participants
Works #449. Not closing it — see "What is still open" below.
CI has validated the desktop entry and the metainfo for a while, with
desktop-file-validateandappstreamcli validate. Those answer whether two files are well-formed. Flathub's linter answers whether a submission would be accepted: reproducible sources, permissions Flathub actually grants, and a long list of listing rules no XML validator knows about. Nothing here had ever run it.The exact commands
The linter ships inside
org.flatpak.Builder, which is how Flathub's own docs run it — the same tool a reviewer uses.All three now run in the Flatpak build job: manifest before the build, repo and AppStream catalogue after the export. That third file is what a software centre actually reads, and it is not the file
appstreamcli validatechecked.Version assumptions
org.flatpak.Builderis deliberately not pinned. Flathub reviews with whatever is current, so pinning would make this check green against a linter nobody uses. The cost is that a new linter release can turn the build red for a reason the PR did not cause — the runner printsflatpak --version, the ref, the commit and the linter version before any result, so that is visible rather than mysterious, and the fix is to fix the finding.What the runner refuses
scripts/flathub_builder_lint.pyexists for what it will not call a pass, not to wrap three commands:org.flatpak.Builder, or no report → exit 2, saying nothing was linted. A check that silently succeeds when its tool is missing is worse than no check, because it gets reported as evidence.flatpak/flathub-lint-exceptions.json, warnings included. Reviewers read warnings, and [Flathub] Run flathub-builder-lint cleanly #449 is about a submission-quality result rather than an exit code.The exceptions file ships empty, which is the state #456 requires.
One real fix, found without the tool
The summary was 36 characters against Flathub's 35-character cap. Now
Play your own music collection(30).test/tooling/flathub_metadata_guardrails_test.dartholds that rule and the other cheap, stable, textual ones — trailing period, leading article, repeated app name, 20-char name cap, required metainfo elements, desktop-entry visibility and icon name — so the next one is caught on the PR that causes it rather than in a submission review. It is explicitly not a reimplementation of the linter; everything else is the linter's job.What is still open
Two findings are expected, and neither is excepted, because neither is a case of the linter being wrong:
type: dir,path: ..), which is right for testing a PR and wrong for a submission, which must build a tagged release from immutable sources. That is [Flathub] Prepare Flathub submission repository #451.So this PR makes the check exist, repeatable and honest. It does not yet make it clean, and I would rather leave #449 open than close it on a check that has not returned green once.
What was actually validated
Run locally:
python3 test/tooling/flathub_builder_lint_test.py— 17 passing (missing linter, undocumented finding, stale exception, empty reason, malformed file, warnings counted as findings)flutter test test/tooling/— 430 passing, including the 13 new metadata guardrailsflutter analyze,dart format --set-exit-if-changed test— cleanruff check/ruff format --checkonscripts tool tools— cleanpython3 scripts/check_release_metadata_sync.py,python3 scripts/check_linux_runner.py— clean after the summary changescripts/check_secrets.sh— cleanNot run here: the linter itself. This container has no
flatpak, so the first real output is CI on this PR. I expect it to be red on the two findings above, and I will report exactly what comes back rather than guessing at it now.🤖 Generated with Claude Code
https://claude.ai/code/session_01QLDN6yzLpSdXoFHYJNqyyp
Generated by Claude Code