Skip to content

cloudflare-worker: stop reporting things that are not true - #113

Open
BKPepe wants to merge 6 commits into
mainfrom
fix/audit-round-one
Open

cloudflare-worker: stop reporting things that are not true#113
BKPepe wants to merge 6 commits into
mainfrom
fix/audit-round-one

Conversation

@BKPepe

@BKPepe BKPepe commented Sep 9, 2026

Copy link
Copy Markdown
Member

Six findings from an independent audit, each a place where the bot told a contributor something that was not so.

A style rule OpenWrt does not have. The indentation check demanded two spaces on $(call Package/foo/Default), the line a package inherits shared metadata with. The tree does not agree on that style — 769 of those lines use two spaces, 601 sit at column 0, 148 start with a tab, and column 0 is the majority in the main repository. Neither does make: the block goes through $(eval), and three variants of the real gdb package differing only in that byte produce byte-identical metadata. That line is no longer judged. Ordinary metadata lines keep the two-space rule, which the tree does agree on, 13 219 against 1 603.

A green check over a file nobody read. When the check could not read a patch file at the head commit — a batched lookup answering 503, a ref gone after a force-push — the failure was swallowed and the file reported as a success. It is now counted and turns the conclusion neutral, the same state the run already uses for work the request budget cut short.

Labels that outlived what they described. add package, drop package and release/<version> were never taken off again, so a branch that dropped its package or was retargeted kept a label saying otherwise. All three are now withdrawn on the same terms they are applied. Labels from labeler.yml are left alone, the way GitHub's own labeler leaves them. A removal names the label the way the repository spells it — the packages feed calls it Add package.

One patch, two readings. The release audit and the hash audit each walked the commit patch themselves, and the walks had drifted; on 600 real commits one dropped content the other kept in 413 file/kind pairs. They now share one parser, which also stops at the mail signature GitHub appends to every commit patch. Read as diff content that signature is a deleted line, so a comment-only edit stopped counting as cosmetic and was told to bump PKG_RELEASE. All 120 sampled patches carried it.

A package that was never audited. package/utils/ucode is the ucode interpreter, but ucode also names a LuCI payload directory, so the walk to the package root skipped it, went up to the category and resolved to nothing. Every pull request touching it went unaudited without saying so. A directory directly under a category is now the package, not payload.

Conffiles rules the build system does not share. Make expansions were read as literal paths and all reported broken, though OpenWrt writes them throughout. Directories were required to end in a slash, which neither scripts/ipkg-build nor package-pack.mk can distinguish. Replaying the 408 Makefiles in the tree that define a conffiles block: 13 flagged before, 2 after, both genuinely indented.

Deploying now waits for the tests, which used to race it: both workflows triggered on the same push to main.

574 tests pass. Each fix has a test that fails when the fix is reverted.

Copilot AI lite review requested due to automatic review settings September 9, 2026 09:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

BKPepe and others added 3 commits September 9, 2026 11:59
A package inherits shared metadata by expanding another block as the
first line of its own - `$(call Package/foo/Default)` - and the
indentation check reported every one of those that was not written with
exactly two spaces. It is not a style the tree agrees on: counted over
package/ and the packages feed, 769 of these lines use two spaces, 601
sit at column 0 and 148 start with a tab, and inside openwrt/openwrt
itself the column-0 form is the majority, 209 against 57. Both
package/devel/gdb and package/kernel/mwlwifi write it that way.

The indentation of a bare make expansion inside a metadata block is
therefore no longer judged. Ordinary metadata lines keep the two-space
rule, which the tree really does agree on: 13 219 of them against 1 603
with a tab.

Replaying every one of the 2 049 package Makefiles in a checkout of the
tree through the check as if the pull request added it, this removes
755 of the 1 745 metadata-indentation errors.

Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A patch file the pull request only edits arrives as a few changed
lines; its Git headers live in the part of the file the diff does not
show, so the check reads the file at the head commit to judge them.
When that read failed - a batched lookup answering 503, a ref that
vanished under a force-push - the failure was swallowed and the file
was reported as a success reading "header validation skipped". The
Code Patches check then went green over a file nobody had looked at.

Not looking is not the same as looking and finding it correct. Such a
file is now counted, said out loud in the check's details, and turns
the conclusion neutral - the same honest third state the run already
uses for work the request budget cut short. A file that could be read
is still judged, in the same run.

Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bot applied "add package", "drop package" and release/<version>
whenever the condition held, and never took any of them off again. A
branch that dropped its new package during review, or was retargeted
from one release to another, kept a label saying otherwise - and the
nightly scan and everyone reading the list of pull requests believed
it. Only "stale" and the guidelines label were ever withdrawn.

All three are read straight off the pull request's own content, so the
bot now withdraws them on the same terms it applies them. Labels
matched from labeler.yml are deliberately left alone, the way GitHub's
own labeler action leaves them by default: a path label is as often put
on by hand as derived, and taking those back would fight whoever set
them.

A removal names the label the way the repository spells it rather than
the way the constant is written - openwrt/packages calls it "Add
package" - which the comparison, being case-insensitive, hid until now.

Deploying is also held back until the tests pass. deploy.yml and
tests.yml both triggered on a push to main, so they started in the same
second and raced: the Worker went live whether or not the suite ever
went green.

Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@BKPepe
BKPepe force-pushed the fix/audit-round-one branch from 20f1e83 to 88f1783 Compare September 9, 2026 10:00
BKPepe and others added 3 commits September 9, 2026 13:39
The release audit and the hash audit each walked the commit patch
themselves to collect the added and deleted lines per file, and the two
walks had drifted: one skipped the four file-header spellings, the
other skipped every line opening with `---` or `+++` wherever it sat.
Refreshing an embedded patch rewrites exactly such lines, and on 600
real commits the second walk dropped content from 413 file/kind pairs
the first one kept. Both now share one parser, which ends the preamble
where git ends it, at the header pair or the first hunk.

That parser also stops at the signature git puts after the last file.
GitHub serves a commit as a mail, so every patch ends with `-- ` and a
version, and read as diff content that is a deleted line holding `- `.
It is neither blank nor a comment, so a comment-only edit stopped
counting as cosmetic: all 120 sampled patches carried the phantom line,
25 of them into a Makefile, and a package whose only change was a fixed
typo in a comment was told to bump PKG_RELEASE.

Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Walking up from a changed file to its package root skips the payload
directories a package carries - patches, files, src, and LuCI's root,
htdocs, luasrc, ucode and po. A directory sitting directly under a
category is not payload, though, it is the package: package/utils/ucode
is the ucode interpreter. It was skipped, the walk went up to the
category, and the file resolved to no package at all, so every pull
request touching it went unaudited and nothing said so. It is the only
such collision in the tree, and it holds in both layouts the bot sees.

A payload directory is now one whose parent is not a category level,
which the candidate filter further down applies too - it had the same
hole. ucode/ inside a LuCI application is still payload.

Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Two of the rules on a conffiles block do not hold. Entries were read as
literal paths, so every make expansion was reported broken, and OpenWrt
writes plenty: $(CONF_DIR)/my.cnf, $(config_directory), whole blocks
pulled in as $(Package/busybox/conffiles/crond), conditionals whose
spaces also tripped the indentation rule. What such a line becomes is
settled at build time, so it is left to the build.

A directory was required to end in a slash. Neither build path can tell
the difference: scripts/ipkg-build runs find over the entry, which
walks a directory either way, and package-pack.mk keeps what [ -f ]
accepts, which skips one either way. The tree writes it both ways too,
/etc/ipsec.d beside /etc/dnsmasq.d/, so the demand is dropped. The
reverse rule stays, because a slash on an individual file matches
neither test and leaves the file silently untracked.

Replaying all 408 Makefiles in the tree that define a conffiles block:
13 were flagged before, 2 after, and both of those are genuinely
indented paths.

Signed-off-by: Josef Schlehofer <pepe.schlehofer@gmail.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to 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