JS-2407 Implement rule S9383: Promises should not be left unhandled by martin-strecker-sonarsource · Pull Request #7896 · SonarSource/SonarJS

martin-strecker-sonarsource · 2026-09-07T15:50:48Z

Part of JS-2407

Summary

Implements S9383, wrapping typescript-eslint's no-floating-promises the same way S6544 wraps no-misused-promises.
Default options are inherited from upstream unchanged (ignoreVoid: true, checkThenables: false, ignoreIIFE: false, allowForKnownSafeCalls/allowForKnownSafePromises empty).
Upstream's "add await"/"add void" suggestions are passed through unmodified (not stripped), following the established pattern for decorated wraps of suggestion-producing typescript-eslint rules (e.g. S6572, S6582). RSPEC declares quickfix: covered to match this codebase's validate-quickfix.ts, which only accepts covered as the pass state for hasSuggestions=true (not partial, even though that's also a documented RSPEC value).
Fixes a false positive found during the manual ruling review directly in the decorator: upstream's isValidRejectionHandler() misclassifies an any-typed rejection handler as "not a function", since any structurally has zero call signatures despite being callable at runtime. Filed typescript-eslint/typescript-eslint#12848 with a proposed upstream fix — closed by the maintainer as working-as-intended (deliberate FP/FN tradeoff on their side, not a bug). S9383/rule.ts now intercepts the two floatingUselessRejectionHandler(Void) messageIds and suppresses the report locally when the handler's resolved type is exactly any, covering all 4 real-world occurrences from the ruling review (see follow-up comment below for details).
RSPEC is merged: SonarSource/rspec#8128 landed the S9383 rule metadata on the RSPEC default branch. The local RSPEC mirror (S9383.json) and the temporary root rspec.sha pin (used per docs/BUILD.md#baseline-ci-mismatches / docs/DEV.md while the rule wasn't yet upstream) have both been removed now that CI's prepare_rspec_rule_data job picks up the real, merged metadata.

Ruling

Ran the full ruling suite; every project producing an S9383 hit had a new baseline added (25 files across 24 projects, 791 total findings).
Spot-checked the two highest-volume projects before syncing:
- desktop (360 issues/76 files): unawaited async conditionalVersion(...) calls inside DB constructors (Dexie migration setup) — real fire-and-forget async work.
- eigen (257 issues/186 files): spread ~1/file, e.g. unawaited async calls inside useEffect and unawaited navigate(...) in ternaries — genuine floating promises, not a detection bug.
npm run ruling is green (59/59) after ruling-sync; re-verified green after the suggestion pass-through change (locations/messages unaffected).
Full manual TP/FP review of all 791 ruling findings is in progress; will post results as a follow-up comment.

Test plan

npx tsx --test packages/analysis/src/jsts/rules/S9383/cb.test.ts passes (12 comment-based scenarios: floating statement, awaited, returned, .catch(), .then() with/without rejection handler, void-ignored, floating promise array vs. Promise.all(), floating async IIFE, any-typed rejection handler (not reported), non-function rejection handler (still reported)), including exact quick-fix output assertions ([[qfN]]/fix@/edit@) for the scenarios that carry suggestions.
npm run bbf builds clean.
mvn install -DskipTests builds clean (validated validate-quickfix and RSPEC refresh against the repinned commit).
npm run ruling green after baseline sync.
Merge/sync SonarSource/rspec#8128 before merging this PR.
Remove root rspec.sha pin before merging to master.

🤖 Generated with Claude Code

Wraps typescript-eslint's no-floating-promises, following the S6544 wrapping pattern. Suggestions (add await/void) are stripped for now since quickfix support isn't implemented yet.

New expected ruling output for S9383 across the JS/TS ruling corpus, generated with npm run ruling / ruling-sync. Spot-checked the two highest-volume projects (desktop: 360 issues, eigen: 257 issues) against source - both are genuine fire-and-forget async calls (unawaited async methods in constructors, unawaited calls in useEffect/JSX handlers), not a false-positive pattern.

hashicorp-vault-sonar-prod · 2026-09-07T15:51:00Z

JS-2407

Pins rspec.sha to SonarSource/rspec#8128 (still unmerged) so CI's prepare_rspec_rule_data job fetches the real RSPEC metadata for S9383 instead of failing with ENOENT on resources/rule-data/javascript/S9383.json, per docs/BUILD.md's documented process for a SonarJS PR that depends on an in-flight RSPEC change. Remove this file once SonarSource/rspec#8128 merges to the RSPEC default branch.

github-actions · 2026-09-08T07:07:37Z

Ruling Report

New issues flagged (787 issues)

S9383

ace/src/keyboard/vim.js:2066

  2064 |         }
  2065 |         if (registerName === '+') {
> 2066 |           navigator.clipboard.writeText(text);
  2067 |         }
  2068 |         // The unnamed register always has the same value as the last used

ace/src/keyboard/vim.js:3602

  3600 |             actionArgs.registerName);
  3601 |         if (actionArgs.registerName === '+') {
> 3602 |           navigator.clipboard.readText().then((value) => {
  3603 |             this.continuePaste(cm, actionArgs, vim, value, register);
  3604 |           })

ace/src/mouse/touch_handler.js:50

    48 |             }
    49 |             if (action == "paste") {
>   50 |                 clipboard.readText().then(function (text) {
    51 |                     editor.execCommand(action, text);
    52 |                 });

ace/src/mouse/touch_handler.js:57

    55 |                 if (action == "cut" || action == "copy") {
    56 |                     if (clipboard)
>   57 |                         clipboard.writeText(editor.getCopyText());
    58 |                     else
    59 |                         document.execCommand("copy");

animate.css/docs/modules/playground.mjs:41

    39 |     if (el.classList.contains('copy-icon')) {
    40 |       const animation = `animate__${el.parentElement.getAttribute('data-animation')}`;
>   41 |       navigator.clipboard.writeText(animation);
    42 |     }
    43 |   });

ant-design/components/form/tests/list-noStyle.test.tsx:48

    46 |     }
    47 | 
>   48 |     addItem();
    49 |     addItem();
    50 |

ant-design/components/form/tests/list-noStyle.test.tsx:49

    47 | 
    48 |     addItem();
>   49 |     addItem();
    50 | 
    51 |     // Submit

ant-design/components/notification/index.tsx:161

   159 | 
   160 |   if (cacheInstance) {
>  161 |     Promise.resolve(cacheInstance).then(instance => {
   162 |       callback({ prefixCls: `${prefixCls}-notice`, iconPrefixCls, instance });
   163 |     });

ant-design/components/notification/index.tsx:304

   302 |   destroy() {
   303 |     Object.keys(notificationInstance).forEach(cacheKey => {
>  304 |       Promise.resolve(notificationInstance[cacheKey]).then(instance => {
   305 |         instance.destroy();
   306 |       });

ant-design/components/upload/Upload.tsx:256

   254 |   const handleRemove = (file: UploadFile) => {
   255 |     let currentFile: UploadFile;
>  256 |     Promise.resolve(typeof onRemove === 'function' ? onRemove(file) : onRemove).then(ret => {
   257 |       // Prevent removing file
   258 |       if (ret === false) {

...and 777 more

📋 View full report

New issues flagged (787)

S9383

Stop stripping the "add await"/"add void" suggestions from no-floating-promises. Following the established pattern for decorated wraps of suggestion-producing typescript-eslint rules (e.g. S6572, S6582): pass the upstream suggest payload through unmodified and test the exact fix output via the [[qfN]]/fix@/edit@ comment-based DSL, instead of defensively discarding it. RSPEC now declares quickfix=partial (not all report shapes carry a suggestion - the promise-array case never does), updated on both the in-review RSPEC PR and the local mirror.

…overed The previous rspec.sha pin was a commit based on plain master, but CI's unpinned default (and the rspec-maven-plugin's rule-data generation) resolves against dogfood-automerge, which has diverged from master. Pinning to master-only meant deploy-rule-data couldn't find resources/rule-data/javascript/S7503.json (unrelated rule) and prepare_rspec_rule_data failed with ENOENT. Fix: merge the RSPEC S9383 branch onto the current dogfood-automerge tip (SonarSource/rspec@2d0508b, via a throwaway Martin/dogfood-pin-S9383 branch so PR #8128's own history isn't rewritten) and pin rspec.sha to that commit instead. Verified locally with `npm run rspec:refresh` - resolves cleanly now, no ENOENT. Also mirrors the RSPEC-side quickfix=covered fix (see previous commit) in the local resource file, confirmed by the live refresh pulling the same value from the pinned commit.

✅ Code review updated (blocking issues remain unresolved).

_{Configure merge blocking}

sonarqube-next · 2026-09-08T13:08:17Z

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

martin-strecker-sonarsource · 2026-09-08T13:15:02Z

S9383 ruling validation — 791 findings

	Count	%
True Positive	775	98.0%
Confirmed False Positive	6	0.8%
Borderline / low-value TP (not a defect — see below)	10	1.3%

Confirmed false positives — 2 distinct root causes, both pre-existing upstream limitations (not introduced by our wrapping)

Root cause A — any-typed rejection handler misread as "not a function" (4 instances)
Upstream's isValidRejectionHandler() checks getCallSignatures().length > 0 on the handler's resolved TS type. any structurally has zero call signatures even though it's callable at runtime by definition, so a genuinely-callable handler gets misreported as "A rejection handler that is not a function will be ignored."
Not previously tracked upstream (checked) — filed typescript-eslint/typescript-eslint#12848 with a verified minimal repro and a proposed one-line fix (tsutils.isIntrinsicAnyType short-circuit, using a helper already imported in that file), confirmed locally to resolve all four occurrences without weakening detection of genuinely non-callable handlers.

ace/src/ext/code_lens_test.js:114 and :139 — .catch(next), untyped Mocha callback parameter.
p5.js/src/core/preload.js:93 — .catch(errorCallback), untyped rest-args reassignment.
eigen/scripts/algolia-example-galleries-near-me.ts:82 — .catch(console.error); project's tsconfig.json has no "node"/DOM lib, so console is untyped there.

Root cause B — array-mutation method misread as a floating promise array (2 instances)

vitest/packages/expect/src/utils.ts:36 and vitest/packages/runner/src/artifact.ts:148 — .splice() on an already-.finally()-handled Promise[] tracking array. .splice()'s return type is coincidentally Promise[] too, which trips the array-of-promises heuristic even though nothing new is being launched.

Borderline cases (10) — technically true positives per the rule's literal contract, but low real-world signal

8× vuetify/packages/vuetify/src/components/VOverlay/useActivator.tsx — timer-backed promises whose executor structurally never calls reject (verified). Matches the exception our own rule.adoc already documents ("can be ignored for promises you know will always resolve like timers").
1× oak/http_server_bun.ts:210 — flagged from an interface's declared void | Promise<void> union; this concrete override never returns a promise at runtime.
1× vitest/packages/vitest/src/runtime/moduleRunner/nativeModuleMocker.ts:199 — the derived .finally() chain is technically unhandled even though the original promise is awaited by the caller elsewhere.

Non-issue worth a one-line mention

ant-design/components/form/__tests__/list-noStyle.test.tsx:48-49 — correct TP, but S9383 is scope: Main; a real SonarQube scan excludes test files at the Java-plugin level. The ruling harness doesn't apply that filter, so it surfaces here but wouldn't in production.

Bottom line: zero false positives traceable to our wrapper — both FP categories are pre-existing upstream typescript-eslint limitations (one now filed and fix-proposed), and at 6/791 (0.8%) they're narrow enough that I wouldn't block on them.

🤖 Generated with Claude Code

martin-strecker-sonarsource · 2026-09-08T13:18:49Z

Re: #7896 (comment)

Checked both points against current state rather than the diff snapshot the bot analyzed:

Point 2 (description contradicts implementation) — already stale. The PR description was updated two commits ago to reflect that suggestions are passed through (not stripped) and RSPEC declares quickfix: covered (not partial/unknown). No further action needed there.

Point 1 (no automated guard against rspec.sha reaching master) — real, but the framing is slightly off. A guard already exists: the "Reject root rspec.sha" step inside the promote job (if [ -e rspec.sha ]; then exit 1; fi) — it's currently failing on this PR, exactly as intended. So it's not that no CI step exists.

The actual gap: I checked this repo's branch-protection rulesets via the API (gh api repos/SonarSource/SonarJS/rulesets/<id>), and promote is not a required status check on master — the only required check is StepSecurity Required Checks (a security scan, unrelated). So nothing currently stops a merge while that step is red; the existing guard just isn't wired up as required.

That's a repo-level branch-protection/ruleset setting, not something a code change in this PR's diff can fix — happy to raise it separately with whoever administers branch protection for this repo, since it'd protect every future rspec.sha pin, not just this one. For this PR specifically, the pin removal is already tracked as an explicit checklist item in the description.

Upstream's isValidRejectionHandler() treats any as "not a function" since it structurally has zero call signatures, even though it's callable at runtime. typescript-eslint/typescript-eslint#12848 was closed working-as-intended, so fix it locally: interceptReport() re-checks the handler's type and drops the report when it's exactly any, covering the 4 real-world FPs from ruling.

gitar-bot · 2026-09-10T18:59:49Z

Code Review 👍 Approved with suggestions 4 resolved / 5 findings

Implements S9383 (unhandled promises) as a decorated wrap of typescript-eslint's no-floating-promises, passing through upstream suggestions for "add await" and "add void" with quickfix: covered. Update the PR description to reflect that suggestions are now passed through (not stripped) and verify the RSPEC mirror and root rspec.sha pin are removed before merging to master.

💡 Quality: PR description still claims suggestions are stripped and quickfix unknown

📄 packages/analysis/src/jsts/rules/S9383/rule.ts:26-31 📄 packages/analysis/src/jsts/rules/S9383/cb.fixture.ts:5-9 📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json:25

The PR summary states "Suggestions (Add await operator. / Add void operator to ignore.) are stripped for now — quickfix support isn't implemented yet (RSPEC quickfix: unknown)", but this commit removes the interceptor that stripped them and flips the RSPEC mirror to partial, so suggestions are now passed through and asserted in cb.fixture.ts. Update the description (and the linked RSPEC PR's quickfix value) so reviewers and the RSPEC sync are not driven by the stale claim.

✅ 4 resolved

✅ Bug: Local S9383 RSPEC mirror is wiped by rspec:refresh, breaking CI

📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json:1-15 📄 packages/analysis/src/jsts/rules/S9383/meta.ts:18-22
The hand-written S9383.json mirror does not survive the RSPEC sync path that CI runs, so the build breaks until rspec#8128 is merged and rspec.sha bumped. .github/workflows/build.yml job prepare_rspec_rule_data runs npm run rspec:refresh, which downloads rspec rule data into resources/rule-data/javascript and then calls npm run deploy-rule-data. syncRuleData() derives its rule list from the rule folders (listRulesDir(), which now contains S9383), rmSynces the whole .../l10n/javascript/rules/javascript folder — deleting the committed S9383.json — and then, for every rule name, reads <source>/S9383.json and copies <source>/S9383.html. Both are absent while the RSPEC PR is open, so writeNormalizedManifest/copyFileSync throw ENOENT and every downstream build/test job fails; even a tolerant sync would leave the rule with no S9383.html, which CheckListTest#test asserts exists for every rule key. So the PR description's claim that the local mirror lets the rule build before rspec merges only holds for npm run bbf (which reads the json through getRspecMeta), not for the Maven/CI path — keep this PR blocked on the RSPEC merge, or make deploy-rule-data tolerate rules that have no rspec source by preserving the existing manifest/description instead of throwing.

✅ Bug: Root rspec.sha pin overrides the dogfood-automerge RSPEC branch

📄 rspec.sha:1
The refresh is configured to resolve rule data from RSPEC branch dogfood-automerge (pom.xml:80), and docs/DEV.md:82-84 state that a root rspec.sha wins over the configured branch and that branch selection is ignored while the file is present. So this pin does not just add S9383 — it moves the RSPEC revision for all ~400 rules to the tip of the rspec#8128 PR branch. If that commit does not contain the dogfood-automerge state (a PR branch cut from rspec master will not), prepare_rspec_rule_data regenerates every rule from master-state metadata, and any rule whose RSPEC entry exists only on dogfood-automerge makes deploy-rule-data throw ENOENT in writeNormalizedManifest (tools/deploy-rule-data.ts:120,163) — the exact failure mode the pin was meant to avoid, just for a different rule. Pin a commit that merges the RSPEC S9383 branch into dogfood-automerge (or rebase the RSPEC branch onto it) so the pinned revision is a superset of the branch the repo normally refreshes from.

✅ Bug: quickfix 'partial' + hasSuggestions fails validate-quickfix build step

📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json:25 📄 packages/analysis/src/jsts/rules/S9383/rule.ts:27
tools/validate-quickfix.ts:29-46 treats a rule as declaring quickfix support only when rspecMeta.quickfix === 'covered', and errors with Rule has hasSuggestions but RSPEC doesn't declare quickfix='covered' otherwise. This commit drops hasSuggestions: false from generateMeta(...), so S9383's merged meta now inherits hasSuggestions: true from typescript-eslint's no-floating-promises (node_modules/@typescript-eslint/eslint-plugin/dist/rules/no-floating-promises.js:56), while S9383.json sets quickfix: "partial" — Validation 2 fires and the script exits 1. That script runs as part of the Maven build (docs/BUILD.md lists npm run validate-quickfix under the phases skipped only by -Dskip-nodejs), so the build/CI breaks. Note that CI refreshes RSPEC from the pinned rspec.sha, so the RSPEC PR must also declare quickfix: "covered"; setting only the local mirror to partial cannot pass either.

✅ Quality: Committed temporary pin has no automated guard against reaching master

📄 rspec.sha:1
The root rspec.sha is listed in .gitignore:20, so it had to be force-added and will never show up in git status again — the repo's only friction against the pin reaching master is bypassed, leaving the description checkbox as the sole protection. If it merges, the nightly generated_files_freshness job (build.yml:324-352) consumes the pinned prepare_rspec_rule_data output and opens PRs rewriting every tracked rule JSON at that unmerged RSPEC PR commit, and packaged per-language rspec.sha release markers would attribute releases to a commit that exists only on an RSPEC PR branch (docs/rspec-sync-requirements.md file-roles section). Add a merge-blocking CI step that fails when rspec.sha is tracked and the PR base is master (e.g. git ls-files --error-unmatch rspec.sha && exit 1 in a PR-only job), so removal cannot be forgotten.

🤖 Prompt for agents

Code Review: Implements S9383 (unhandled promises) as a decorated wrap of typescript-eslint's `no-floating-promises`, passing through upstream suggestions for "add await" and "add void" with `quickfix: covered`. Update the PR description to reflect that suggestions are now passed through (not stripped) and verify the RSPEC mirror and root `rspec.sha` pin are removed before merging to master.

1. 💡 Quality: PR description still claims suggestions are stripped and quickfix unknown
   Files: packages/analysis/src/jsts/rules/S9383/rule.ts:26-31, packages/analysis/src/jsts/rules/S9383/cb.fixture.ts:5-9, sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json:25

   The PR summary states "Suggestions (`Add await operator.` / `Add void operator to ignore.`) are stripped for now — quickfix support isn't implemented yet (RSPEC `quickfix: unknown`)", but this commit removes the interceptor that stripped them and flips the RSPEC mirror to `partial`, so suggestions are now passed through and asserted in `cb.fixture.ts`. Update the description (and the linked RSPEC PR's `quickfix` value) so reviewers and the RSPEC sync are not driven by the stale claim.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

martin-strecker-sonarsource · 2026-09-10T19:00:46Z

Update (2026-09-10): typescript-eslint/typescript-eslint#12848 (filed above for root cause A) was closed by the maintainer (bradzacher) as NOT_PLANNED/working-as-intended — treating an any-typed rejection handler as reportable is a deliberate FP/FN tradeoff on their side, not a bug they're planning to fix. Since this is now a permanent upstream characteristic rather than a pending fix, root cause A's 4 false positives are suppressed locally in this PR's decorator instead: S9383/rule.ts intercepts the floatingUselessRejectionHandler(Void) messageIds and drops the report when the rejection handler's resolved type is exactly any. Root cause B (the .splice()-on-tracking-array FPs) is unaffected by this and remains open.

Drop optional-chaining and void-unwrapping - none of the 4 real-world FPs from the ruling review took those shapes, and the decorator shouldn't guess at forms we haven't actually seen.

Removes the 4 false positives fixed in the decorator: ace:114,139, p5.js:93, eigen:82 - the exact any-typed .catch() handlers from the ruling review.

guillemsarda

LGTM. Worth adding the two tests.

guillemsarda · 2026-09-14T07:07:33Z

+}
+
+// `any` has no call signatures though it's callable at runtime; upstream closed this
+// as working-as-intended (typescript-eslint/typescript-eslint#12848), so fix it here.


Thanks for the Issue!

Follows the codebase's majority pattern for decorated typescript-eslint rules (index.ts calling decorate() from decorator.ts) instead of inlining everything in rule.ts, per review feedback.

…cess Locks in that rejection handlers reached only through optional chaining (?.catch/?.then) or computed member access (['catch']/[`then`]) fall outside findRejectionHandler's direct-call match and are still reported, even when any-typed, per review feedback.

alex-meseldzija-sonarsource · 2026-09-16T08:42:26Z

/pvf

…c main Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…-info branches - decorator.ts: replace the forbidden rule.meta! non-null assertion with a safe fallback (S2966). - cb.fixture.ts: add an any-typed rejection handler passed via .then(), which previously left the then-branch of findRejectionHandler uncovered. - unit.test.ts: cover the isRequiredParserServices-false branch by decorating a mock upstream rule and linting without a TypeScript-aware parser. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Spreading rule.meta directly already yields {} when it's undefined, so the ?? {} fallback added in the previous commit was redundant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

gitar-bot · 2026-09-16T13:19:30Z

Code Review ✅ Approved 5 closed / 5 findings

Implements S9383 rule wrapping typescript-eslint's no-floating-promises to detect unhandled promises. Resolves local RSPEC mirror persistence issue, removes temporary rspec.sha pin now that upstream metadata is merged, and fixes quickfix declaration to covered for suggestion pass-through validation. False positive from upstream's any-typed rejection handler misclassification is suppressed locally based on resolved type check.

✅ 5 closed

✅ Bug: Local S9383 RSPEC mirror is wiped by rspec:refresh, breaking CI

📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json:1-15 📄 packages/analysis/src/jsts/rules/S9383/meta.ts:18-22
The hand-written S9383.json mirror does not survive the RSPEC sync path that CI runs, so the build breaks until rspec#8128 is merged and rspec.sha bumped. .github/workflows/build.yml job prepare_rspec_rule_data runs npm run rspec:refresh, which downloads rspec rule data into resources/rule-data/javascript and then calls npm run deploy-rule-data. syncRuleData() derives its rule list from the rule folders (listRulesDir(), which now contains S9383), rmSynces the whole .../l10n/javascript/rules/javascript folder — deleting the committed S9383.json — and then, for every rule name, reads <source>/S9383.json and copies <source>/S9383.html. Both are absent while the RSPEC PR is open, so writeNormalizedManifest/copyFileSync throw ENOENT and every downstream build/test job fails; even a tolerant sync would leave the rule with no S9383.html, which CheckListTest#test asserts exists for every rule key. So the PR description's claim that the local mirror lets the rule build before rspec merges only holds for npm run bbf (which reads the json through getRspecMeta), not for the Maven/CI path — keep this PR blocked on the RSPEC merge, or make deploy-rule-data tolerate rules that have no rspec source by preserving the existing manifest/description instead of throwing.

✅ Bug: Root rspec.sha pin overrides the dogfood-automerge RSPEC branch

📄 rspec.sha:1
The refresh is configured to resolve rule data from RSPEC branch dogfood-automerge (pom.xml:80), and docs/DEV.md:82-84 state that a root rspec.sha wins over the configured branch and that branch selection is ignored while the file is present. So this pin does not just add S9383 — it moves the RSPEC revision for all ~400 rules to the tip of the rspec#8128 PR branch. If that commit does not contain the dogfood-automerge state (a PR branch cut from rspec master will not), prepare_rspec_rule_data regenerates every rule from master-state metadata, and any rule whose RSPEC entry exists only on dogfood-automerge makes deploy-rule-data throw ENOENT in writeNormalizedManifest (tools/deploy-rule-data.ts:120,163) — the exact failure mode the pin was meant to avoid, just for a different rule. Pin a commit that merges the RSPEC S9383 branch into dogfood-automerge (or rebase the RSPEC branch onto it) so the pinned revision is a superset of the branch the repo normally refreshes from.

✅ Bug: quickfix 'partial' + hasSuggestions fails validate-quickfix build step

📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json:25 📄 packages/analysis/src/jsts/rules/S9383/rule.ts:27
tools/validate-quickfix.ts:29-46 treats a rule as declaring quickfix support only when rspecMeta.quickfix === 'covered', and errors with Rule has hasSuggestions but RSPEC doesn't declare quickfix='covered' otherwise. This commit drops hasSuggestions: false from generateMeta(...), so S9383's merged meta now inherits hasSuggestions: true from typescript-eslint's no-floating-promises (node_modules/@typescript-eslint/eslint-plugin/dist/rules/no-floating-promises.js:56), while S9383.json sets quickfix: "partial" — Validation 2 fires and the script exits 1. That script runs as part of the Maven build (docs/BUILD.md lists npm run validate-quickfix under the phases skipped only by -Dskip-nodejs), so the build/CI breaks. Note that CI refreshes RSPEC from the pinned rspec.sha, so the RSPEC PR must also declare quickfix: "covered"; setting only the local mirror to partial cannot pass either.

✅ Quality: Committed temporary pin has no automated guard against reaching master

📄 rspec.sha:1
The root rspec.sha is listed in .gitignore:20, so it had to be force-added and will never show up in git status again — the repo's only friction against the pin reaching master is bypassed, leaving the description checkbox as the sole protection. If it merges, the nightly generated_files_freshness job (build.yml:324-352) consumes the pinned prepare_rspec_rule_data output and opens PRs rewriting every tracked rule JSON at that unmerged RSPEC PR commit, and packaged per-language rspec.sha release markers would attribute releases to a commit that exists only on an RSPEC PR branch (docs/rspec-sync-requirements.md file-roles section). Add a merge-blocking CI step that fails when rspec.sha is tracked and the PR base is master (e.g. git ls-files --error-unmatch rspec.sha && exit 1 in a PR-only job), so removal cannot be forgotten.

✅ Quality: PR description still claims suggestions are stripped and quickfix unknown

📄 packages/analysis/src/jsts/rules/S9383/rule.ts:26-31 📄 packages/analysis/src/jsts/rules/S9383/cb.fixture.ts:5-9 📄 sonar-plugin/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json:25
The PR summary states "Suggestions (Add await operator. / Add void operator to ignore.) are stripped for now — quickfix support isn't implemented yet (RSPEC quickfix: unknown)", but this commit removes the interceptor that stripped them and flips the RSPEC mirror to partial, so suggestions are now passed through and asserted in cb.fixture.ts. Update the description (and the linked RSPEC PR's quickfix value) so reviewers and the RSPEC sync are not driven by the stale claim.

Review coverage

Functional validation No results

Rules No rules evaluated

Auto-approval Not enabled · Set up

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Counting what did not apply, without listing it.

Comment with these commands to change the behavior for this request:

`Auto-apply`	`Compact`
`gitar auto-apply:on`	`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Gitar}

sonarqube-next · 2026-09-16T13:39:24Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

martin-strecker-sonarsource · 2026-09-16T13:48:00Z

/pvf

martin-strecker-sonarsource · 2026-09-16T13:59:27Z

/pvf

hashicorp-vault-sonar-prod · 2026-09-16T14:11:37Z

📊 Performance Validation complete
🆕 929 new · 🗑️ 0 lost issues
View dashboard (SonarSource SSO required)

martin-strecker-sonarsource · 2026-09-16T14:45:36Z

Reviewed the Peach validation results (929 new / 0 lost issues): all 929 are S9383 (673 TypeScript, 256 JavaScript), no other rule regressed. Spot-checked source snippets across several projects (ag-grid, rollup, jest, koel, ngx-admin) - findings range from clear true positives (e.g. unhandled HTMLMediaElement.play(), fetch().then().then() chains with no .catch) to plausible fire-and-forget calls consistent with the rule's intent. Nothing suspicious found - no FP patterns, no decoder/location mismatches.

martin-strecker-sonarsource added 2 commits September 7, 2026 17:13

JS-2407 S9383: implement Promises should not be left unhandled

bb55d9c

Wraps typescript-eslint's no-floating-promises, following the S6544 wrapping pattern. Suggestions (add await/void) are stripped for now since quickfix support isn't implemented yet.

gitar-bot Bot reviewed Sep 7, 2026

View reviewed changes

Comment thread ...n/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json

gitar-bot Bot reviewed Sep 8, 2026

View reviewed changes

Comment thread rspec.sha Outdated

gitar-bot Bot reviewed Sep 8, 2026

View reviewed changes

Comment thread rspec.sha Outdated

This comment has been minimized.

Sign in to view

gitar-bot Bot reviewed Sep 8, 2026

View reviewed changes

Comment thread ...n/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S9383.json Outdated

gitar-bot Bot reviewed Sep 8, 2026

View reviewed changes

Comment thread packages/analysis/src/jsts/rules/S9383/rule.ts Outdated

This comment was marked as resolved.

Sign in to view

martin-strecker-sonarsource marked this pull request as ready for review September 8, 2026 13:19

martin-strecker-sonarsource requested a review from guillemsarda September 8, 2026 13:33

martin-strecker-sonarsource mentioned this pull request Sep 10, 2026

Bug: [no-floating-promises] any-typed rejection handler misclassified as 'not a function' typescript-eslint/typescript-eslint#12848

Closed

4 tasks done

martin-strecker-sonarsource added 2 commits September 10, 2026 21:08

JS-2407 S9383: simplify findRejectionHandler to the evidenced call shape

73d66fc

Drop optional-chaining and void-unwrapping - none of the 4 real-world FPs from the ruling review took those shapes, and the decorator shouldn't guess at forms we haven't actually seen.

JS-2407 S9383: sync ruling baseline for any-typed rejection handler fix

941e80e

Removes the 4 false positives fixed in the decorator: ace:114,139, p5.js:93, eigen:82 - the exact any-typed .catch() handlers from the ruling review.

guillemsarda approved these changes Sep 14, 2026

View reviewed changes

guillemsarda reviewed Sep 14, 2026

View reviewed changes

Comment thread packages/analysis/src/jsts/rules/S9383/decorator.ts

martin-strecker-sonarsource added 2 commits September 15, 2026 10:04

JS-2407 S9383: split decoration logic into decorator.ts

119573c

Follows the codebase's majority pattern for decorated typescript-eslint rules (index.ts calling decorate() from decorator.ts) instead of inlining everything in rule.ts, per review feedback.

martin-strecker-sonarsource requested a review from guillemsarda September 15, 2026 09:42

guillemsarda approved these changes Sep 16, 2026

View reviewed changes

martin-strecker-sonarsource and others added 3 commits September 16, 2026 14:07

Remove rspec.sha pin now that S9383 rule metadata is merged into rspe…

6da0638

…c main Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

JS-2407 S9383: drop useless empty-object fallback (S7744)

5153b5e

Spreading rule.meta directly already yields {} when it's undefined, so the ?? {} fallback added in the previous commit was redundant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

martin-strecker-sonarsource merged commit e9f3d5a into master Sep 16, 2026
41 checks passed

martin-strecker-sonarsource deleted the Martin/JS-2407_ImplementS9383FloatingPromises branch September 16, 2026 14:48

Conversation

martin-strecker-sonarsource commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Ruling

Test plan

Uh oh!

hashicorp-vault-sonar-prod Bot commented Sep 7, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ruling Report

New issues flagged (787 issues)

S9383

New issues flagged (787)

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

sonarqube-next Bot commented Sep 8, 2026

Quality Gate passed

Uh oh!

martin-strecker-sonarsource commented Sep 8, 2026

S9383 ruling validation — 791 findings

Confirmed false positives — 2 distinct root causes, both pre-existing upstream limitations (not introduced by our wrapping)

Borderline cases (10) — technically true positives per the rule's literal contract, but low real-world signal

Non-issue worth a one-line mention

Uh oh!

martin-strecker-sonarsource commented Sep 8, 2026

Uh oh!

gitar-bot Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martin-strecker-sonarsource commented Sep 10, 2026

Uh oh!

guillemsarda left a comment

Choose a reason for hiding this comment

Uh oh!

guillemsarda Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alex-meseldzija-sonarsource commented Sep 16, 2026

Uh oh!

gitar-bot Bot commented Sep 16, 2026

Uh oh!

sonarqube-next Bot commented Sep 16, 2026

Quality Gate passed

Uh oh!

martin-strecker-sonarsource commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martin-strecker-sonarsource commented Sep 16, 2026

Uh oh!

hashicorp-vault-sonar-prod Bot commented Sep 16, 2026

Uh oh!

martin-strecker-sonarsource commented Sep 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martin-strecker-sonarsource commented Sep 7, 2026 •

edited

Loading

hashicorp-vault-sonar-prod Bot commented Sep 7, 2026 •

edited by atlassian Bot

Loading

github-actions Bot commented Sep 8, 2026 •

edited

Loading

gitar-bot Bot commented Sep 10, 2026 •

edited

Loading

martin-strecker-sonarsource commented Sep 16, 2026 •

edited

Loading