Improvement/cldsrv 875 integrate shared prettier by benzekrimaha · Pull Request #6124 · scality/cloudserver

benzekrimaha · 2026-03-31T16:19:36Z

bert-e · 2026-03-31T16:19:44Z

Hello benzekrimaha,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

scripts/format-changed.sh

claude · 2026-03-31T16:22:43Z

Bug: scripts/format-changed.sh:33 — the grep regex double-escapes the backslash (\\. instead of \.), so the pattern matches a literal backslash before the dot. No filenames contain a backslash, so zero files will ever match and the Prettier check silently passes without checking anything.
- Use a single backslash before the dot in the grep pattern

Review by Claude Code

scripts/format-changed.sh

claude · 2026-03-31T16:25:42Z

scripts/format-changed.sh:33 — double backslash in grep pattern makes the file-extension filter a no-op, so the Prettier check silently passes on every PR without actually checking any files.

Review by Claude Code

.github/workflows/tests.yaml

scripts/format-changed.sh

claude · 2026-03-31T16:26:37Z

Bug: grep regex never matches any files (scripts/format-changed.sh:29) — Double backslash in single-quoted grep pattern means no file extensions will match, so the Prettier check is a no-op on every PR. Fix: use a single backslash before the dot.
- CI: unnecessary full clone on push builds (.github/workflows/tests.yaml:87) — fetch-depth: 0 applies to all triggers but is only needed for the Prettier step on pull_request events. Consider making it conditional.

Review by Claude Code

bert-e · 2026-03-31T16:26:37Z

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.

scripts/format-changed.sh

claude · 2026-03-31T16:26:42Z

scripts/format-changed.sh:29 — Double-escaped backslash in grep pattern (\\\.) causes file extension matching to fail silently. No files ever match, so the Prettier check is effectively a no-op. Use a single backslash (\.) to match a literal dot.

Review by Claude Code

.github/workflows/tests.yaml

claude · 2026-03-31T16:34:00Z

--depth=0 is not a valid documented git fetch option (.github/workflows/tests.yaml:98)
- Remove the --depth=0 flag or use a valid positive integer

Review by Claude Code

.github/workflows/tests.yaml

scripts/format-changed.sh

claude · 2026-03-31T16:44:54Z

Shallow fetch depth may break three-dot diff (.github/workflows/tests.yaml:98): --depth=1 fetches only the base branch tip. The script uses BASE_SHA...HEAD (three-dot), which requires merge-base computation and may fail with insufficient history.
- Increase fetch depth or switch to two-dot diff in the script
Script injection in workflow (.github/workflows/tests.yaml:98): ${{ github.event.pull_request.base.ref }} used directly in run: — pass via env var instead per GitHub Security Lab best practices.
- Use an env: block to pass base.ref and reference $BASE_REF in the command
Silent skip on missing base ref (scripts/format-changed.sh:26): Exits 0 when base ref is unresolvable, silently skipping the prettier check.
- Use ::warning:: annotation so the skip is visible in CI summary

Review by Claude Code

.github/workflows/tests.yaml

scripts/format-changed.sh

claude · 2026-03-31T16:49:21Z

Adding pull_request trigger alongside existing push trigger causes all CI jobs to run twice for PRs (once per event). Only the Prettier step is gated on event type.
- Restrict push to default branch only, or add a concurrency group to cancel duplicates.
format-changed.sh fetch fallback (line 20) cannot fetch by raw SHA from most remotes; if the base SHA is outside the shallow clone depth, the Prettier check silently skips.
- Use BASE_REF for the fetch fallback instead of stripping origin/ from a SHA.

Review by Claude Code

claude · 2026-03-31T17:09:22Z

LGTM

Review by Claude Code

codecov · 2026-03-31T17:14:49Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.28%. Comparing base (0c0530c) to head (47fedf5).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@               Coverage Diff                @@
##           development/9.3    #6124   +/-   ##
================================================
  Coverage            84.28%   84.28%           
================================================
  Files                  206      206           
  Lines                13256    13256           
================================================
  Hits                 11173    11173           
  Misses                2083     2083

Flag	Coverage Δ
file-ft-tests	`67.93% <ø> (ø)`
kmip-ft-tests	`28.10% <ø> (ø)`
mongo-v0-ft-tests	`69.19% <ø> (+0.02%)`	⬆️
mongo-v1-ft-tests	`69.13% <ø> (-0.06%)`	⬇️
multiple-backend	`36.30% <ø> (ø)`
sur-tests	`35.62% <ø> (ø)`
sur-tests-inflights	`37.48% <ø> (ø)`
unit	`69.88% <ø> (ø)`
utapi-v2-tests	`34.36% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

package.json

claude · 2026-04-01T09:59:01Z

package.json reformatted to 4-space indentation will conflict with yarn's native 2-space output when dependencies are added/removed, requiring developers to re-run prettier after every yarn add/remove
- Exclude package.json from prettier via .prettierignore, or document this step in contributing guidelines

Review by Claude Code

claude · 2026-04-01T10:42:42Z

.github/workflows/tests.yaml

@@ -74,85 +78,100 @@ permissions:
  contents: read
  packages: write


The concurrency group uses pull_request.head.sha, which changes with every new commit pushed to the PR. This means each push creates a different concurrency group, so previous runs won't be canceled — defeating the purpose of cancel-in-progress: true.

Use the PR number (or head_ref) instead so that successive pushes to the same PR cancel prior runs:

```suggestion
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }}

claude · 2026-04-01T10:42:48Z

Concurrency group uses pull_request.head.sha which is unique per commit, so cancel-in-progress won't actually cancel previous runs for the same PR. Use pull_request.number instead.

Review by Claude Code

.github/workflows/tests.yaml

claude · 2026-04-02T07:16:59Z

Duplicate CI runs: adding pull_request trigger alongside the unfiltered push trigger will run all jobs twice for every PR push, since the concurrency group keys differ between the two events.
- Restrict push to main/release branches, or add a condition to skip push-triggered runs when a PR is open.

Review by Claude Code

.github/workflows/tests.yaml

claude · 2026-04-02T07:58:33Z

Most issues from previous reviews have been addressed (grep pattern fix, depth=50, env var for BASE_REF, ::warning:: annotation). Two items remain:

- Dual push+pull_request trigger still causes double runs: The concurrency group key diverges between push (uses SHA, always unique) and pull_request (uses PR number) events, so both run independently for each push to a PR branch. Use github.head_ref || github.sha as the group key, or restrict push to the default branch only.
- package.json 4-space indent vs yarn 2-space default: As previously noted, yarn add/yarn remove will rewrite package.json with 2-space indent, requiring a manual yarn format pass before CI passes.

Review by Claude Code

.github/workflows/tests.yaml

claude · 2026-04-02T08:08:49Z

The bare pull_request: trigger on the workflow will run all CI jobs (build, integration tests, SSE migration, etc.) on every PR event, not just the lint job with the Prettier check. Consider scoping it or adding path filters if the intent was only to enforce formatting on PRs.

Review by Claude Code

scripts/format-changed.sh

package.json

Issue: CLDSRV-875

.github/workflows/lint.yaml

claude · 2026-04-02T11:33:52Z

The lint job in tests.yaml was not removed after being duplicated into the new lint.yaml workflow. Both trigger on pull_request, so linting runs twice per PR. Remove the lint job from tests.yaml.

Review by Claude Code

claude · 2026-04-02T11:57:15Z

LGTM

Review by Claude Code

claude · 2026-04-02T12:01:31Z

LGTM

Review by Claude Code

DarkIsDude · 2026-04-02T12:34:43Z

CONTRIBUTING.md

 This will start the developement server, watching for changes and restarting
 as needed.
+
+### Formatting


Then I think we should ignore package.json ?

either add it to the ignore or we inforce the linting in it , it's a choice to make we can check with others what they prefer

DarkIsDude · 2026-04-02T12:39:20Z

package.json

+        "ft_sse_arn": "cd tests/functional/sse-kms-migration && mocha --reporter mocha-multi-reporters --reporter-options configFile=$INIT_CWD/tests/reporter-config.json -t 10000 cleanup.js arnPrefix.js --exit",
+        "lint": "eslint $(git ls-files '*.js')",
+        "lint_md": "mdlint $(git ls-files '*.md')",
+        "format:changed": "scripts/format-changed.sh",


Suggested change

"format:changed": "scripts/format-changed.sh",

"format:check:diff": "scripts/format-changed.sh",

Maybe ?

tried to keep the same name as the sh , can indeed be changed

DarkIsDude · 2026-04-02T12:40:00Z

scripts/format-changed.sh

Maybe a quick comment to explain the goal of the script and how it's working ?

claude · 2026-04-02T14:36:50Z

LGTM

Review by Claude Code