👷(CLDSRV-860) Monitor async await migration by DarkIsDude · Pull Request #6088 · scality/cloudserver

DarkIsDude · 2026-02-25T15:47:58Z

Pull request template

Description

Motivation and context

Why is this change required? What problem does it solve?

Related issues

Please use the following link syntaxes #600 to reference issues in the
current repository

Checklist

Add tests to cover the changes

New tests added or existing tests modified to cover all changes

Code conforms with the style guide

Sign your work

In order to contribute to the project, you must sign your work
https://github.com/scality/Guidelines/blob/master/CONTRIBUTING.md#sign-your-work

Thank you again for contributing! We will try to test and integrate the change
as soon as we can.

DarkIsDude · 2026-03-10T15:45:25Z

.github/scripts/check-diff-async.mjs

+
+    const propertyAccesses = sourceFile.getDescendantsOfKind(SyntaxKind.PropertyAccessExpression);
+    for (const access of propertyAccesses) {
+        if (access.getName() !== 'then') continue;


We don't allow then but we ask them in the migration. What do you think ?

do we actually need this extra "check"?

I guess we would only use then() because we have a callback function... which should be reported already

even after the migration, do you thing we may have some cases where it makes sense to use the raw .then() instead of await ?

Issue: CLDSRV-860

claude · 2026-03-10T16:12:32Z

.github/scripts/check-diff-async.mjs

+import { execSync } from 'node:child_process';
+import { Project, SyntaxKind } from 'ts-morph';
+
+const CALLBACK_PARAM_PATTERN = /^(cb|callback|next|done|err)$/i;


err is a common name for error parameters (e.g. (err, result) => {}, .on("error", (err) => {})), not callbacks. Including it here will cause false positives on error handlers and error-first callback signatures where err is the error, not the continuation.

— Claude Code

claude · 2026-03-10T16:12:34Z

.github/workflows/tests.yaml

      run: pip install flake8
    - name: Lint Javascript
-      run: yarn run --silent lint -- --max-warnings 0
+      run: yarn run --silent lint


Removing --max-warnings 0 means the entire eslint run now tolerates unlimited warnings — not just the new promise/prefer-await-to-then and n/callback-return rules but every other warning too. This weakens overall lint enforcement. Consider keeping --max-warnings 0 and either (a) setting a higher threshold, or (b) adding eslint-disable comments to existing violations so new warnings still fail CI.

— Claude Code

kind of agree with Claude here, we don't want to disable all warnings, but only the prefer-await-to-than and callback-return... but we want to get a report on these...

could we instead run eslint multiple times? First without these checks but with strict warning; then only on these "migration" checks (and ignoring warnings).

another option may be to create a custom eslint reporter, to generate a separate report AND filter out (=ignore) irrelevant warnings ? c.f. [1]

maybe we could use a custom reporter to automatically handle the check-diff-async part (i.e. ignore defects not in diff).... and maybe (probably if I am being optimistic...) this is a feature of eslint or an existing plugin that we may leverage?

[1] https://stackoverflow.com/questions/66899779/configure-eslint-to-report-only-plugin-specific-errors

claude · 2026-03-10T16:12:48Z

.github/scripts/count-async-functions.mjs

+console.log(`Migration: ${asyncFunctions}/${totalFunctions} functions (${migrationPercent}%)`);
+
+if (process.env.GITHUB_STEP_SUMMARY) {
+    const { writeFileSync, appendFileSync } = await import('node:fs');


writeFileSync is imported but never used. Only appendFileSync is needed here.

— Claude Code

claude · 2026-03-10T16:13:47Z

.github/scripts/check-diff-async.mjs

+    const base = process.env.GITHUB_BASE_REF
+        ? `origin/${process.env.GITHUB_BASE_REF}`
+        : 'HEAD';
+    const diff = execSync(`git diff ${base} -- ${filePath}`, { encoding: 'utf8' });


filePath is interpolated directly into a shell command via execSync. A filename with shell metacharacters could break or inject commands. Use execFileSync with an array of arguments instead to avoid shell interpretation. Same applies to line 17.

— Claude Code

claude · 2026-03-10T16:13:55Z

.github/workflows/tests.yaml

    - name: Lint Yaml
      run: yamllint -c yamllint.yml $(git ls-files "*.yml")
+    - name: Check async/await compliance in diff
+      run: yarn run check-diff-async


The lint job checkout (line 82) uses default fetch-depth 1 (shallow clone). check-diff-async.mjs needs the base branch ref which will not exist in a shallow clone. This step will fail on every PR. Add fetch-depth: 0 to the checkout step.

— Claude Code

claude · 2026-03-10T16:15:15Z

Lint job checkout uses shallow clone but check-diff-async.mjs needs the base branch ref. Will fail on every PR. Add fetch-depth 0.
execSync interpolates filePath into shell commands (lines 17 and 31), risking injection. Use execFileSync with array args.
CALLBACK_PARAM_PATTERN includes err, causing false positives on error handlers. Remove err.
Removing max-warnings 0 from lint means all eslint warnings are silently ignored in CI.
count-async-functions.mjs imports writeFileSync but never uses it (line 77).

Review by Claude Code

francoisferrand · 2026-03-11T08:10:19Z

.github/scripts/count-async-functions.mjs

+
+if (process.env.GITHUB_STEP_SUMMARY) {
+    const { writeFileSync, appendFileSync } = await import('node:fs');
+    appendFileSync(process.env.GITHUB_STEP_SUMMARY, [


do we really want a summary in every PR?
for monitoring the progress, this may be done in a separate job (eg. nightly on on dev branches) which builds a report giving the status on the "latest branch" (as a badge in the README, as a GitHub page, ....)

francoisferrand · 2026-03-11T08:14:03Z

.github/workflows/tests.yaml

      run: pip install flake8
    - name: Lint Javascript
-      run: yarn run --silent lint -- --max-warnings 0
+      run: yarn run --silent lint


kind of agree with Claude here, we don't want to disable all warnings, but only the prefer-await-to-than and callback-return... but we want to get a report on these...

could we instead run eslint multiple times? First without these checks but with strict warning; then only on these "migration" checks (and ignoring warnings).

another option may be to create a custom eslint reporter, to generate a separate report AND filter out (=ignore) irrelevant warnings ? c.f. [1]

maybe we could use a custom reporter to automatically handle the check-diff-async part (i.e. ignore defects not in diff).... and maybe (probably if I am being optimistic...) this is a feature of eslint or an existing plugin that we may leverage?

[1] https://stackoverflow.com/questions/66899779/configure-eslint-to-report-only-plugin-specific-errors

francoisferrand · 2026-03-11T08:18:56Z

.github/scripts/check-diff-async.mjs

+
+    const propertyAccesses = sourceFile.getDescendantsOfKind(SyntaxKind.PropertyAccessExpression);
+    for (const access of propertyAccesses) {
+        if (access.getName() !== 'then') continue;


do we actually need this extra "check"?

I guess we would only use then() because we have a callback function... which should be reported already

even after the migration, do you thing we may have some cases where it makes sense to use the raw .then() instead of await ?

francoisferrand · 2026-03-11T08:19:37Z

.github/scripts/check-diff-async.mjs

should these scripts not be in the Guidelines repo, so they can be reused?

(and if needed, it may include a github action as well)

francoisferrand · 2026-03-11T08:20:35Z

.github/scripts/check-diff-async.mjs

we should have tests for these scripts, to make it easier to test/debug them (even if they are tools).
(this will be easier to implement if they are in a kind of dedicated repo -like Guidelines- and simply used here)

note: for testing against git/diff, maybe we could use https://github.com/kiegroup/mock-github, as we do for testing CI workflows

francoisferrand · 2026-03-11T08:25:31Z

.github/scripts/check-diff-async.mjs

@@ -0,0 +1,142 @@
+/**
+ * Check that all new/modified functions in the current git diff use async/await.
+ * Fails with exit code 1 if any additions introduce callback-style functions or .then() chains.


can't we use https://github.com/tj-actions/eslint-changed-files for this purpose?
seems quite similar...

or run eslint like this: eslint --fix $(git diff --name-only HEAD | xargs) (c.f. https://stackoverflow.com/questions/54511168/enable-eslint-only-for-edited-files)

francoisferrand · 2026-03-11T08:29:15Z

.github/scripts/count-async-functions.mjs

+    skipAddingFilesFromTsConfig: true,
+});
+
+project.addSourceFilesAtPaths([


shouldn't this be some parameters of the script (or auto-discovered from package.json/...), to allow reusing this script?

francoisferrand · 2026-03-11T08:30:25Z

.github/scripts/count-async-functions.mjs

+
+    const propertyAccesses = sourceFile.getDescendantsOfKind(SyntaxKind.PropertyAccessExpression);
+    for (const access of propertyAccesses) {
+        if (access.getName() === 'then') {


same as above: not sure it makes sense to try to "eradicate" then() (but we can reporting them 😁)

francoisferrand · 2026-03-11T08:33:15Z

.github/scripts/count-async-functions.mjs

+    }
+}
+
+const migrationPercent = totalFunctions > 0


this is not correct : not all functions will be (ever) be async.

the migration progress is really 1 - callbackFunctions / (callbackFunctions_before_migration)....

as a "kind of proxy", we may compute callbackFunctions / (asyncFunctions + callbackFunctions) ; but not sure it is that useful...

(completion is really just when callbackFunctions == 0)

bert-e · 2026-03-18T15:49:27Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

DarkIsDude self-assigned this Feb 25, 2026

This comment was marked as resolved.

Sign in to view

DarkIsDude changed the base branch from development/9.2 to development/9.3 February 25, 2026 15:48

This comment was marked as resolved.

Sign in to view

DarkIsDude commented Mar 10, 2026

View reviewed changes

DarkIsDude marked this pull request as ready for review March 10, 2026 16:09

DarkIsDude requested review from a team, benzekrimaha and delthas March 10, 2026 16:09

DarkIsDude added 2 commits March 10, 2026 17:10

🚨 add eslint configuration to monitor async/await

afefcce

Issue: CLDSRV-860

👷 update CI to add new check on async/await file

8981239

Issue: CLDSRV-860

DarkIsDude force-pushed the feature/CLDSRV-860/monitor-async-await-migration branch from 93020af to 8981239 Compare March 10, 2026 16:10

claude bot reviewed Mar 10, 2026

View reviewed changes

francoisferrand requested changes Mar 11, 2026

View reviewed changes

This comment was marked as resolved.

Sign in to view

Conversation

DarkIsDude commented Feb 25, 2026

Pull request template

Description

Motivation and context

Related issues

Checklist

Add tests to cover the changes

Code conforms with the style guide

Sign your work

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

claude bot commented Mar 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

bert-e commented Mar 18, 2026

Request integration branches

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants