ReviewOS

stacks/logsmith

parseReferences matches inside hex colours and strips the owner from cross-repo references

#3402
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Observed

parseReferences treats any # followed by digits as an issue number. Two consequences reached published GitHub releases of stacksjs/system-cleaner, in v0.3.2 and again in v0.3.4:

A CSS hex colour becomes an issue link. A commit whose body discusses #6c6c70 and #98989d — it was about dimming a dashboard panel — produced links to issues #6 and #98989. A commit describing "the #000 luminance mask" produced #000.

A cross-repo reference loses its owner. A body reading stacksjs/stx#2040 and #2041 are both fixed upstream produced links to this repo's #2040 and #2041. pantry-pm/pantry#242 produced this repo's #242. None of the four exists.

Reproduction

Running the two patterns from packages/logsmith/src/utils.ts#L244-L245 against the real commit bodies:

commit body contains/#(\d+)/g yields
#6c6c70, #6e6e73, #98989d6, 98989
#000 (a CSS colour), Closes #20000, 20
stacksjs/stx#2040, #20412040, 2041
pantry-pm/pantry#242242

Only #20 in the second row is a real reference.

parseReferences('dim it to #6c6c70 rather than #98989d')
// [{ type: 'issue', id: '6' }, { type: 'issue', id: '98989' }]

parseReferences('stacksjs/stx#2040 and #2041 are both fixed upstream')
// [{ type: 'issue', id: '2040' }, { type: 'issue', id: '2041' }]  — wrong repo

Verified source path

parseReferences runs two patterns:

const issuePattern = /(?:fixes?|closes?|resolves?|refs?)\s+#(\d+)/gi
const simpleIssuePattern = /#(\d+)/g

The first is anchored to a keyword and behaves correctly. The second has no left context at all, so it matches inside a longer token: #6c6c70 matches as #6 because \d+ simply stops at c, and pantry-pm/pantry#242 matches as #242 because nothing looks left of the #.

The existing tests at test/utils.test.ts#L100 cover fixes #123 and resolves #456 and a bare #789. Every case is a reference in isolation, so neither failure mode is exercised.

Suggested contract

  • A match needs a word boundary to its left. # preceded by /, a letter, or a digit is not this repo's issue. That alone fixes both the hex case (#6c6c70 has digits after the captured run) and the cross-repo case.
  • Require the digits to be the whole token: #(\d+)(?![0-9a-fA-F]) rejects #6c6c70 and #98989d while keeping #242.
  • Treat a leading zero as not-an-issue. #000 is a colour; GitHub has no issue #000.
  • owner/repo#N is a real and useful reference — consider capturing it as a cross-repo link rather than discarding it, since the alternative is silently pointing at the wrong tracker.
  • Worth covering in tests: a hex colour, a cross-repo reference, and #000.

Why it is worth fixing rather than editing the output

The second time this shipped, the offending entry was generated from the commit that removed the first batch. That commit had to name #98989, #000, #2040 and #2041 to explain what it was deleting, and the generator scraped them straight back out of the prose and linked them again. Correcting the changelog by hand reintroduces the problem for the next release.

Found while cutting releases of stacksjs/system-cleaner; logsmith 0.2.12.

Sign in to comment on this issue.