Repository navigation
vendor: thirteen re.sub sites pass a regex flag as the positional count, capping substitutions at 8 #796
Description
Describe the bug
Thirteen re.sub call sites in pcapkit/vendor/ pass a regex flag where the fourth positional parameter is count. Enumerated by AST rather than grep, on 2d1c8675d:
pcapkit/vendor/default.py:343 pcapkit/vendor/ipv6/router_alert.py:55
pcapkit/vendor/hip/eddsa_curve.py:59 pcapkit/vendor/ipv6/tagger_id.py:55
pcapkit/vendor/http/frame.py:64 pcapkit/vendor/reg/ethertype.py:95
pcapkit/vendor/http/method.py:144 pcapkit/vendor/tcp/flags.py:139
pcapkit/vendor/http/status_code.py:148 pcapkit/vendor/tcp/mp_tcp_option.py:65
pcapkit/vendor/ipv4/router_alert.py:58 pcapkit/vendor/tcp/option.py:78
pcapkit/vendor/ipv6/option.py:89
re.sub(pattern, repl, string, count=0, flags=0) — so re.MULTILINE, which is 8, is read as "replace at most 8 occurrences". Measured:
>>> re.MULTILINE
8
>>> re.sub(r'\r*\n', ' ', 'a\n' * 12, re.MULTILINE) # flag as positional
'a a a a a a a a a\na\na\na\n' # <- stops after 8
>>> re.sub(r'\r*\n', ' ', 'a\n' * 12, flags=re.MULTILINE)
'a a a a a a a a a a a a '
Two defects in one line:
- The count is a real limit. Any IANA description with more than eight newlines keeps its ninth onward, so the generated
constmodule gets embedded newlines where it should have spaces. That output is committed to the tree, so the damage is visible rather than theoretical. - The flag never applies. For
r'\r*\n'that happens not to matter — no^/$in the pattern — so the intent was already a no-op. Check each site; if any pattern uses an anchor, its behaviour is wrong today too.
Also a deprecation: DeprecationWarning: 'count' is passed as positional argument fires on 3.13+, and it becomes a TypeError in a later release, so these break on a future interpreter regardless of semantics.
Expected behavior
flags=re.MULTILINE, or drop the flag where the pattern has no anchor — cleaner than passing one that does nothing.
Additional context
Correcting this issue's own first version, which said "six". A single-line grep found five; the other eight put the flag on a continuation line (), re.MULTILINE))) and were invisible to it. The AST sweep above is the reliable count.
Found by the cross-review on #789, which had just fixed the same shape in re.split at httpv1.py:310/:346. A re-grep found zero remaining re.split sites, so these are the rest of the family.
Two notes for whoever takes it: eight of the thirteen sit on lines also carrying '%s %s' % and a # pylint: disable=consider-using-f-string, so it overlaps #792's convention work — coordinate or sequence them. And regenerating to show the consequence will fetch live IANA registries and move unrelated rows; #792 records the hand-edit-plus-prove-equivalence route around that.
Activity
[-]vendor: six re.sub sites pass re.MULTILINE as the positional count, capping substitutions at 8[/-][+]vendor: thirteen re.sub sites pass a regex flag as the positional count, capping substitutions at 8[/+]JarryShaw commentedon Sep 25, 2026
Labelling
blockedso it does not read as unheld.Checkable blocker: #792 merged. Eight of the thirteen sites sit on lines that also carry
'%s %s' %formatting and a# pylint: disable=consider-using-f-string, and #792 is converting%-style to f-strings under the maintainer's convention. Both touch the same lines inpcapkit/vendor/, and two agents editing one line clobber each other silently — no error, the later write just wins.#792 is
wipwith a worker on it now, so this unblocks shortly. Its line numbers will shift when it lands; re-derive with the AST sweep in the body rather than trusting the list.JarryShaw commentedon Sep 25, 2026
Tightening this blocker, which I worded imprecisely. It said "Checkable blocker: #792 merged" — but #792 is an issue, and an issue does not merge. The thing to check is #797 merged (
053c0889d,refactor/const/review: pending), which is the PR that closes #792 and rewrites the lines this issue overlaps.gh pr view 797 -R JarryShaw/PyPCAPKit --json state,mergedAtis the one-command check.The overlap itself is narrower than I first implied, and worth recording so nobody re-derives it: #797 left all three
%-formatted dunders andextend_enum's'%d'calls alone, converting only the six raises. So the# pylint: disable=consider-using-f-stringstays earned, and the eight of this issue's thirteen sites that carry'%s %s' %formatting are not touched by #797 at all — they live inpcapkit/vendor/'s crawlers, not in theAppTypetemplate.Which means the real conflict risk is smaller than "same lines": it is that both changes edit
pcapkit/vendor/files in the same window. Still worth serialising, but this unblocks the moment #797 lands rather than needing a wider stand-down.refactor(const): convert AppType's six %-style raises to f-strings (#792
JarryShaw commentedon Sep 25, 2026
Correcting myself: #796 and #798 do overlap, on two files. My earlier comment said they "do not collide with each other, only with #797". That was wrong.
#798 sweeps the
%-style guards in the three bespoke templates —tcp/flags.py,ftp/command.py,http/method.py— and two of those three are also in this issue's thirteenre.subsites. Two agents on one file clobber each other with no error; the later write simply wins.Worth saying why I got it wrong, since the shape recurs: I ran the comparison as a shell loop and it printed nothing, and I took the empty output as "no overlap" rather than as "the loop did not work". Re-derived in Python, the overlap is two files. An empty result from a one-off script is not evidence until the script is shown to detect the thing it is looking for.
Blocker updated: #798 merged, not #797 — which has already merged, so that hold is spent and this one replaces it. #798 is the smaller change (3 files, one construct) and its line numbers move less, so it goes first.
Re-derive the thirteen sites with the AST sweep in the body when this starts — #798 will shift line numbers in two of them, and a grep misses the eight that put the flag on a continuation line.
JarryShaw commentedon Sep 25, 2026
Restating the blocker as the last comment, so it is checkable without reading up the thread.
Checkable blocker: #798 merged.
Reason, re-derived in Python after a shell loop misled me into reporting no overlap: #798 touches 3 files, this issue touches 13, and 2 are shared —
pcapkit/vendor/http/method.pyandpcapkit/vendor/tcp/flags.py. Both issues rewrite the sameraiselines in those two, so whichever lands second rewrites the other's work with no conflict marker to show it happened.#798 is the smaller change and has a worker on it now, so it goes first. This unblocks the moment it merges. Chain: #798 → this issue → #775.
JarryShaw commentedon Sep 25, 2026
Unblocked — #798 merged via #803 at 21:44:59Z as
4530424df.mainis now4530424df, and the two files that overlapped (pcapkit/vendor/http/method.py,pcapkit/vendor/tcp/flags.py) are free.Dispatching a worker. Two things from #803 that bear on this:
Call(re.sub/re.split)and self-test it against a known-positive and a known-negative — a one-line grep produced the original wrong count of 6.%-formatting in these same files, so re-derive every line number againstorigin/main(git show origin/main:<path>) rather than reusing any number from this thread.#804 is now blocked on this issue, since it also edits
vendor/http/method.py.JarryShaw commentedon Sep 25, 2026
Correcting this issue's own claim about the damage, found by #813's cross-review and verified by me.
The body says the generated
constmodule "gets embedded newlines where it should have spaces" and that the damage is visible rather than theoretical. That is wrong. All 13 sites feedVendor.wrap_comment(), which istextwrap.wrap(text.strip(), 76), andtextwrap.wrapdefaults toreplace_whitespace=True— so any newline surviving thecount=8cap is turned into a space before it reaches the generated file:The fix is still correct and still worth making — passing a flag as the positional
countis wrong regardless, it is aDeprecationWarningon 3.13+ and aTypeErrorlater, and #813 proves the flag was inert for this pattern at any count. But the justification is "the call means something other than what it says", not "the output is corrupted".The one genuinely observable residual is
\r\npast the 8th match, which leaves doubled spaces rather than newlines:#813's reviewer scanned all of
pcapkit/const/for that signature and found 25 hits, every one attributable to another cause — a[**]/[nn]footnote deleted bytcp/option.py's outerre.sub(3), a lone\rsurvivingname.replace('\n',' ')inreg/ethertype.py(1), and IANA's own intra-bracket whitespace insctp/parameter.pyandsctp/payload_protocol_identifier.py(21). So there is no evidence the cap ever bit in practice.Recording this because #813's
ast.parse-over-139-files check cannot fail on this defect and should not be read as evidence that it did not occur. The real evidence is the exhaustive 3,124-case proof thatMULTILINEis a no-op forr'\r*\n'.fix(vendor): pass regex flags as flags=, not positional count (#796) (#…
docs(changelog): account for the 8 merges since 4530424
docs(changelog): account for the 20 merges since 73f09ae, four breaking
docs(changelog): account for the 20 merges since 73f09ae, six breaking
docs(changelog): account for the 20 merges since 73f09ae, six breaking
docs(changelog): account for the 20 merges since 73f09ae, six breaking
docs(changelog): account for the 20 merges since 73f09ae, six breaking