JS-1487 Fix FP in S125: suppress commented code with TODO/FIXME task markers by sonar-nigel[bot] · Pull Request #6648 · SonarSource/SonarJS

sonar-nigel · 2026-03-19T21:06:08Z

S125 was incorrectly raising issues on commented-out code that was accompanied by task markers (TODO, FIXME, HACK, XXX, NOTE), which indicate the code is intentionally deferred rather than forgotten dead code.

Changes

Add a per-line task marker check to suppress S125 issues when a comment group contains a task marker on a non-code line
Avoid false suppression when task marker keywords (e.g. NOTE, TODO) appear as JavaScript identifiers inside commented-out code (e.g. // const NOTE = 'padding')
Support @TODO prefix and doubly-nested task markers (e.g. // // TODO: ...)
Scope task marker detection per comment group: a marker in an adjacent but separate group (blank-line-separated) does not suppress issues in the other group

Validation

Ruling analysis over 383 entries confirmed correctness: all 379 genuine commented-out code cases raise as expected, and the 4 cases with TODO/FIXME markers in the same comment group are correctly suppressed.

Relates to JS-1487

Proposed rspec changes

diff --git a/rules/S125/description.adoc b/rules/S125/description.adoc
index 412fbb2..2f88c3f 100644
--- a/rules/S125/description.adoc
+++ b/rules/S125/description.adoc
@@ -1,3 +1,5 @@
 Commented-out code distracts the focus from the actual executed code. It creates a noise that increases maintenance code. And because it is never executed, it quickly becomes out of date and invalid.
 
 Commented-out code should be deleted and can be retrieved from source control history if required.
+
+An exception is made when the commented-out code is accompanied by a task marker such as ``TODO``, ``FIXME``, ``HACK``, ``XXX``, or ``NOTE`` on a non-code line within the same comment block. Such markers indicate that the code is intentionally deferred or documented, not accidentally left as dead code.

Tests cover the scenario where commented-out code is accompanied by a task marker (TODO, FIXME, HACK, XXX, NOTE) on a non-code line within the same comment group. The tests verify that each marker keyword suppresses the issue, and that marker keywords used as JavaScript identifiers inside commented code (e.g. const NOTE = 'padding') do not suppress the issue. Relates to JS-1487

S125 incorrectly flagged commented-out code that was accompanied by task markers (TODO, FIXME, HACK, XXX, NOTE), which indicate the code is intentionally deferred rather than forgotten dead code. The fix adds a per-line task marker check using the existing containsCode() and injectMissingBraces() helpers. A comment group is suppressed only when at least one line matches TASK_MARKER_PATTERN and that line is not itself parsable as code — avoiding false suppression when keywords like NOTE or TODO appear as JavaScript identifiers inside commented-out code (e.g., `// const NOTE = 'padding';`). Implementation follows the approved proposal algorithm from proposed-approach-20260319-150304.md. Relates to JS-1487

@todo

Ruling analysis over 383 entries confirmed the implementation is correct: all 379 genuine commented-out code cases raise as expected, and the 4 cases with TODO/FIXME markers in the same comment group are correctly suppressed. Added 3 new test cases derived from ruling patterns: - @todo prefix (e.g. // @todo: ...) is recognized as a task marker and suppresses the issue, same as TODO - Doubly-nested TODO comment (// // TODO: ...) is still recognized as a task marker via the per-line check - Task marker in an adjacent but separate comment group (blank line separator) does NOT suppress the commented code in the other group, demonstrating the per-group scoping of task marker detection

Ticket: JS-1487

github-actions · 2026-03-19T21:15:18Z

Ruling Report

Code no longer flagged (4 issues)

S125

Ghost/core/test/functional/client/content_test.js:70

    68 |     casper.waitForSelector('#entry-title');
    69 | 
>   70 |     // // TODO readd this test when #3811 is fixed
    71 |     // // Change post to static page
    72 |     // casper.thenClick('.post-settings');

Ghost/core/test/unit/permissions_spec.js:323

   321 |     //        .then(function (updatedUser) {
   322 | 
>  323 |     //            // TODO: Verify updatedUser.related('permissions') has the permission?
   324 |     //            var canThisResult = permissions.canThis(updatedUser.id);
   325 |

eigen/src/app/Scenes/MyCollection/Screens/ArtworkForm/Form/useArtworkForm.tests.tsx:5

     3 | })
     4 | 
>    5 | /*
     6 | // TODO: Reenable once we figure out circular dep issue involving GlobalStore
     7 |

postgraphql/src/postgres/inventory/collection/tests/PgCollection-test.js:107

   105 |   }))
   106 | 
>  107 |   // TODO: reimplement
   108 |   // test('paginator will have the same name and type', () => {
   109 |   //   expect(collection1.paginator.name).toBe(collection1.name)

Fix two external SonarQube issues (S1134, S1135) in the JSDoc comment for hasTaskMarker() in rule.ts. The comment listed "TODO, FIXME, HACK, XXX, NOTE" as examples, which caused SonarQube to flag the word "TODO" as an unresolved action item. The fix rewrites the comment to reference the TASK_MARKER_PATTERN constant instead of enumerating the keywords, removing the false positive without changing any runtime behavior.

sonar-review-alpha · 2026-03-19T21:56:59Z

Summary

S125 rule now suppresses false positives on commented-out code that includes task markers (TODO, FIXME, HACK, XXX, NOTE). The implementation adds a per-line check to detect task markers only on non-code lines, avoiding false suppressions when these keywords appear as JavaScript identifiers within commented code itself. Comment group boundaries (blank lines) are respected, ensuring markers in one group don't suppress issues in a separate group. Ruling analysis confirms 4 false positives fixed with no impact on legitimate detections.

What reviewers should know

Start with the hasTaskMarker() function (rule.ts:125-133) — the core logic splits each comment group by lines and checks both that the line contains a task marker AND that the line itself doesn't contain code. This dual check is what prevents false suppressions.

Key nuance: the condition at line 101 gates on !hasTaskMarker() for the entire comment group. Each line in the group is evaluated independently, so a task marker on a "marker line" (like // TODO: pending) suppresses the whole group, but if the marker keyword appears in a line that's itself code (like // const NOTE = 'padding'), it doesn't suppress.

Test coverage is thorough. The valid cases test all five marker types plus @TODO variant and doubly-nested comments. The two invalid cases (lines 371–387) are critical: they verify that NOTE-as-identifier and cross-group markers don't cause suppression — watch these if the logic is refactored.

Ruling removals show the fix in action: 4 previous false positives (lines 70, 323, 107 across three projects) are gone. The rspec description update explains the exception to end-users.

Generate Walkthrough
Generate Diagram

🗣️ Give feedback

sonar-review-alpha

Conclusion: The approach is clean and fits the codebase, but there's one real bug with the case-insensitive flag that needs fixing before merge.

🗣️ Give feedback

packages/jsts/src/rules/S125/rule.ts

sonar-nigel · 2026-03-19T22:29:41Z

Thanks for the summary — the description accurately captures the implementation. No action needed on my end.

…ules/S125/rule.ts Comment: The `/i` flag is too broad for `NOTE` (and to a lesser extent `HACK`). Both are common English prose words. A comment block like: ``` // Note that we disabled this temporarily. // const x = processVector(name, type); ``` will match `\bNote\b` (case-insensitive), `containsCode` returns false for the first line, so `hasTaskMarker` returns `true` and the genuine commented-out code is silently suppressed. `TODO`, `FIXME`, and `XXX` are almost never used as natural English prose in comments, so `/i` is low-risk for those. `NOTE` is different — it appears constantly in developer prose (`// Note that...`, `// Note:`, `// NOTE:` are all equivalent under `/i`). Remove the `/i` flag and match uppercase only, which is the de-facto convention for all five task markers. The `@TODO` test case still passes because `\b` matches before the `T` in `@TODO`. If lowercase support is truly needed, enumerate the exact mixed-case forms you want rather than accepting all cases. - [ ] Mark as noise

sonar-nigel · 2026-03-19T22:31:40Z

Fixed in the latest commit. The /i flag has been removed from TASK_MARKER_PATTERN, so it now matches uppercase-only task markers (TODO, FIXME, HACK, XXX, NOTE). This prevents prose words like "Note that..." from being incorrectly treated as task markers and suppressing genuine commented-out code issues.

sonarqube-next · 2026-03-19T22:45:07Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

sonar-review-alpha

LGTM! ✅

🗣️ Give feedback

Vibe Bot added 4 commits March 19, 2026 20:32

Sync expected ruling files after rule-specific refinement

9605a6e

Ticket: JS-1487

sonar-nigel bot marked this pull request as ready for review March 19, 2026 21:56

sonar-nigel bot requested a review from a team March 19, 2026 21:56

sonar-review-alpha bot reviewed Mar 19, 2026

View reviewed changes

packages/jsts/src/rules/S125/rule.ts Outdated Show resolved Hide resolved

sonar-review-alpha bot reviewed Mar 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JS-1487 Fix FP in S125: suppress commented code with TODO/FIXME task markers#6648

JS-1487 Fix FP in S125: suppress commented code with TODO/FIXME task markers#6648
sonar-nigel[bot] wants to merge 6 commits intomasterfrom
fix/JS-1487-fix-fp-on-s125-commented-code-with-todofixme-markers-incorrectly-flagged-sonnet

sonar-nigel bot commented Mar 19, 2026

Uh oh!

github-actions bot commented Mar 19, 2026

Uh oh!

sonar-review-alpha bot commented Mar 19, 2026 •

edited

Loading

Uh oh!

sonar-review-alpha bot left a comment

Uh oh!

Uh oh!

sonar-nigel bot commented Mar 19, 2026

Uh oh!

sonar-nigel bot commented Mar 19, 2026

Uh oh!

sonarqube-next bot commented Mar 19, 2026

Uh oh!

sonar-review-alpha bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

Conversation

sonar-nigel bot commented Mar 19, 2026

Changes

Validation

Uh oh!

github-actions bot commented Mar 19, 2026

Ruling Report

Code no longer flagged (4 issues)

S125

Uh oh!

sonar-review-alpha bot commented Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

What reviewers should know

Uh oh!

sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonar-nigel bot commented Mar 19, 2026

Uh oh!

sonar-nigel bot commented Mar 19, 2026

Uh oh!

sonarqube-next bot commented Mar 19, 2026

Quality Gate passed

Uh oh!

sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

0 participants

sonar-review-alpha bot commented Mar 19, 2026 •

edited

Loading