Skip to content

Fix two long-standing typo bugs in index.js#137

Open
dscho wants to merge 3 commits into
mainfrom
fix-typos
Open

Fix two long-standing typo bugs in index.js#137
dscho wants to merge 3 commits into
mainfrom
fix-typos

Conversation

@dscho

@dscho dscho commented May 5, 2026

Copy link
Copy Markdown
Member

Two long-standing bugs found while reviewing the codebase in preparation for the imminent Node.js 24 / ESM migration. Both have been in index.js since acdc390 ("Convert the Action to Javascript", 2021-03-20), neither is caught by ESLint, and neither is exercised by the existing test suite, so they have lived undetected through five years of releases.

core.debug$ typo on the content-pattern skip branch

if (contentPattern && !content.match(contentPattern)) {
  core.debug$(`Feed item skipped because it does not match the content pattern (${title})`)
  continue
}

The other five core.debug(...) calls in run() are spelled correctly; only this one carries the stray $. Effect: any workflow
that uses the content-pattern input crashes with TypeError: core.debug$ is not a function the moment a feed item fails the pattern; core.setFailed translates the rejection into a red workflow step.

parseDurationInMilliseconds() triplet regex

The function declares three regex variables (milliSeconds, seconds, minutes) all bound to the same pattern /(\d+)\s*m/, where /(\d+)\s*ms/, /(\d+)\s*s(?!\w)/, and /(\d+)\s*m(?!s)/ were intended. Effect:

input actual expected status
30ms 1,830,030 ms 30 ms ~61,000× off
30s 0 ms 30,000 ms totally lost
30m 1,830,030 ms 1,800,000 ms 1.7% over
30h 108,000,000 correct works
30d 2,592,000,000 correct works

Only h and d happen to land on correct values, and the existing tests use exactly 48h and 9999d, which is why CI never noticed. A user passing max-age: 30s got every feed item filtered out silently.

What this PR does

Two source commits, each followed by an npm run prepare per repo convention:

  1. Fix the regex copy-paste, with negative lookaheads that prevent the unit characters from overlapping
    (s(?!\w), m(?!s)). New regression test respects max-age when expressed in seconds pins the seconds case; verified to fail on the buggy code and pass on the fixed code. Verified outputs: 30ms→30, 30s→30000, 30m→1,800,000, 30h→108,000,000, 30d→2,592,000,000, 1h30m→5,400,000, 1d2h→93,600,000.
  2. Fix the core.debug$ typo. New regression test respects content-pattern filter configures content-pattern against a two-entry feed where one entry matches and one does not; verified to fail (with the actual TypeError: core.debug$ is not a function) on the buggy code and pass on the fixed code.

@dscho dscho marked this pull request as ready for review May 5, 2026 12:45
dscho added 3 commits May 5, 2026 13:04
`parseDurationInMilliseconds()` was introduced in commit acdc390
("Convert the Action to Javascript", 2021-03-20) with three
identical regular expressions where three different ones were
intended:

    const milliSeconds = text.match(/(\d+)\s*m/)
    if (milliSeconds) ms += parseInt(milliSeconds[1])
    const seconds = text.match(/(\d+)\s*m/)
    if (seconds) ms += parseInt(seconds[1]) * 1000
    const minutes = text.match(/(\d+)\s*m/)
    if (minutes) ms += parseInt(minutes[1]) * 60000

The unit characters in the patterns above were meant to be `ms`,
`s`, and `m` respectively, matching the suffixes on the action's
`max-age` input. Because all three patterns matched the same
substring (`\d+\s*m`), three things were wrong:

  - `30s` returned 0, because no pattern matched (`30s` has no
    `m`). Any user passing a `max-age` in seconds therefore had
    every feed item filtered out as "too old": the limit time
    became `Date.now()` itself.

  - `30m` returned 1,830,030 instead of 1,800,000, because the
    same `\d+\s*m` substring was counted three times: once as
    milliseconds (30), once as seconds (30 * 1000), and once as
    minutes (30 * 60000). The outcome was a ~1.7% overshoot, so
    the action accepted items up to ~30 seconds older than it
    should have.

  - `30ms` returned 1,830,030 (~30.5 minutes) for the same
    overcounting reason, an ~61,000x overshoot.

The two units that work correctly in the existing implementation
(`h` and `d`) are exactly the two that the existing tests
exercise: `48h` and `9999d`. The bug therefore went undetected by
CI for the entire lifetime of the JavaScript port.

Fix by giving each unit its own regex with a negative lookahead so
that the unit characters do not overlap:

    /(\d+)\s*ms/      -> matches "30ms"
    /(\d+)\s*s(?!\w)/ -> matches "30s" but not the "s" inside "30ms"
    /(\d+)\s*m(?!s)/  -> matches "30m" but not the "m" inside "30ms"

The `s(?!\w)` guard prevents seconds from being matched inside
plain English words like "30 minutes" (where the `s` of "minutes"
would otherwise count). The `m(?!s)` guard prevents minutes from
being matched inside `ms`. The `h` and `d` patterns are unchanged
because their unit characters do not collide with anything else
the function recognises.

Verified outputs of the fixed function:

    30ms     ->          30 ms
    30s      ->      30,000 ms
    30m      ->   1,800,000 ms
    30h      -> 108,000,000 ms
    30d      -> 2,592,000,000 ms
    1h30m    ->   5,400,000 ms (the function was already designed
                                 to accept compound forms)
    1d2h     ->  93,600,000 ms

Add a regression test that pins the seconds case: an item
published 5 seconds ago with `max-age: 30s` must be kept. With the
buggy regex the test fails because the item is silently filtered
out (`max-age` parses to 0 ms); with the fixed regex it passes.
The new test resets `core.__INPUTS__` first because earlier tests
in the file mutate it without cleaning up.

Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The "skip this item because its content does not match
`content-pattern`" branch in `run()` has called a misspelled
`core.debug$(...)` ever since the JavaScript port (commit acdc390,
2021-03-20). The other five `core.debug(...)` calls in the same
function are spelled correctly; only this one carries a stray `$`,
introduced presumably as a finger slip while retyping the
equivalent log line from the deleted Go original.

What kept this from being noticed:

  - ESLint's `no-undef` checks identifiers, not member accesses;
    `core.debug$` is a syntactically valid (dynamic) property
    lookup whose value just happens to be `undefined`. The crash
    only occurs when the property is then *called*, which is at
    runtime.

  - The bug only triggers when a workflow sets the action's
    `content-pattern` input AND a feed item's content fails the
    pattern. The existing test suite never set `content-pattern`,
    so the failing branch was never exercised in CI.

When the branch does fire, `run()` rejects with
`TypeError: core.debug$ is not a function`, the action's runtime
catch translates that into `core.setFailed(e.message)`, and the
workflow step fails with a confusing error message.

Fix by removing the stray `$`, and add a regression test that
configures `content-pattern` against a two-entry feed where one
entry matches and one does not. The test asserts that exactly one
issue is created (the matching one), which is only true once the
typo is gone: with the typo present, processing the second entry
crashes before any assertions can run.

Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

@rimrul rimrul left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The s(?!\w) guard prevents seconds from being matched inside
plain English words like "30 minutes" (where the s of "minutes"
would otherwise count).

This example in the ca70045 commit message is an odd choice. minutes wouldn't match because e is neither numeric nor whitespace, not because of the '(?!\w)'

Comment thread __tests__/index.test.js
<published>${date}</published>
<content type="html">x</content>
</entry>
</feed>`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldn't the test include at least one entry on either side of the limit to effectively test that the limit is correctly interpreted? at the moment we just test that '30s' isn't interpreted as a vastly to small time span, but it could in theory still be 30 days or even no limit, for all this test catches.

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