fix: data races in tests#440
Open
alnr wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughThis PR refactors all Cobra command definitions from package-level variables with init()-based registration to factory functions, eliminating global state races during concurrent command instantiation. Monorepo subcommands use scratch variables populated per-invocation to safely bind flags. Testing and CI now include race-condition detection. ChangesCobra command factory refactoring for concurrent safety
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/dev/ci/monorepo/run.go (1)
94-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
modeparameter instead of package-levelrunModein error message.The error message references the package-level scratch variable
runModeinstead of the function parametermode. This is inconsistent with the rest of the function and could produce misleading error messages in tests that callrunWrapperdirectly.Proposed fix
default: - return fmt.Errorf("unknown runMode: %s", runMode) + return fmt.Errorf("unknown runMode: %s", mode) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/dev/ci/monorepo/run.go` around lines 94 - 96, The default case in runWrapper currently uses the package-level variable runMode in the error string; update the error to reference the function parameter mode instead. Locate the switch/default inside runWrapper and change the fmt.Errorf("unknown runMode: %s", runMode) call to fmt.Errorf("unknown runMode: %s", mode) so the error message reflects the actual argument passed to runWrapper rather than the package-level scratch variable.
🧹 Nitpick comments (1)
cmd/dev/ci/monorepo/components.go (1)
17-19: ⚡ Quick winRemove the
componentModepackage-level scratch variable and switch onlocalComponentModedirectly.
componentModeis only used insidenewComponentsCmd()’sRunclosure incmd/dev/ci/monorepo/components.go, so the package-level mutable state is unnecessary. Eliminating it avoids any residual shared-state risk while keeping the behavior the same.♻️ Proposed change
-// Package-level scratch variable for the `components` subcommand, populated -// from a per-command local in Run so concurrent construction does not race. -var componentMode string - func newComponentsCmd() *cobra.Command { var localComponentMode string c := &cobra.Command{ Use: "components", Short: "List components based on mode.", Long: `List components based on mode by reading dependency configs and displaying the dependency graph.`, Run: func(cmd *cobra.Command, args []string) { - componentMode = localComponentMode - var graph ComponentGraph _, _ = graph.getComponentGraph(rootDirectory) - switch componentMode { + switch localComponentMode { case "affected": affectedComponents := getAffectedComponents(&graph) displayComponents(affectedComponents) case "all": allComponents := graph.components displayComponents(allComponents) case "changed": changedComponents := getChangedComponents(&graph) displayComponents(changedComponents) case "involved": involvedComponents := getInvolvedComponents(&graph) displayComponents(involvedComponents) default: - log.Fatalf("Unknown ListMode '%s'", componentMode) + log.Fatalf("Unknown ListMode '%s'", localComponentMode) } }, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/dev/ci/monorepo/components.go` around lines 17 - 19, Remove the package-level variable componentMode and use the local variable localComponentMode directly inside the Run closure of newComponentsCmd; update any switch/case or conditional that references componentMode to instead reference localComponentMode (inside newComponentsCmd and its Run), and delete the now-unused componentMode declaration to eliminate unnecessary package-level mutable state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/dev/ci/github/env.go`:
- Around line 54-59: The guard incorrectly treats an unset/empty
SWAGGER_SPEC_IGNORE_PKGS as non-empty because strings.Split("", ",") yields
[""], causing export of "-x " entries; update the logic around reading
SWAGGER_SPEC_IGNORE_PKGS (the os.Getenv call and ignorePkgs handling) to first
check if the env var is non-empty (or filter out empty strings after
strings.Split) before building the "-x " list and calling fmt.Printf, ensuring
ignorePkgs contains only non-empty package names before joining and printing.
In `@cmd/dev/ci/monorepo/main.go`:
- Line 18: Remove the unused scratch variable "branch" and its flag binding from
the monorepo command or wire it into downstream logic; specifically, delete the
"branch string" declaration and the corresponding "--branch" PersistentFlag
setup in main.go (the PersistentPreRun populates it) OR propagate the variable
into the existing flow (e.g., pass branch into the functions used by
run.go/changes.go or use it inside isPR()/change-detection logic) so the value
is consumed rather than unused.
---
Outside diff comments:
In `@cmd/dev/ci/monorepo/run.go`:
- Around line 94-96: The default case in runWrapper currently uses the
package-level variable runMode in the error string; update the error to
reference the function parameter mode instead. Locate the switch/default inside
runWrapper and change the fmt.Errorf("unknown runMode: %s", runMode) call to
fmt.Errorf("unknown runMode: %s", mode) so the error message reflects the actual
argument passed to runWrapper rather than the package-level scratch variable.
---
Nitpick comments:
In `@cmd/dev/ci/monorepo/components.go`:
- Around line 17-19: Remove the package-level variable componentMode and use the
local variable localComponentMode directly inside the Run closure of
newComponentsCmd; update any switch/case or conditional that references
componentMode to instead reference localComponentMode (inside newComponentsCmd
and its Run), and delete the now-unused componentMode declaration to eliminate
unnecessary package-level mutable state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9f04a2a-9a62-468a-86d5-2ec0fa88be67
📒 Files selected for processing (39)
.github/workflows/ci.yamlMakefilecmd/dev.gocmd/dev/ci/deps/main.gocmd/dev/ci/deps/url.gocmd/dev/ci/github/env.gocmd/dev/ci/github/main.gocmd/dev/ci/main.gocmd/dev/ci/monorepo/changes.gocmd/dev/ci/monorepo/components.gocmd/dev/ci/monorepo/main.gocmd/dev/ci/monorepo/run.gocmd/dev/ci/orbs/bump.gocmd/dev/ci/orbs/main.gocmd/dev/headers/copyright.gocmd/dev/headers/cp.gocmd/dev/headers/main.gocmd/dev/main.gocmd/dev/markdown/main.gocmd/dev/markdown/render.gocmd/dev/newsletter/draft.gocmd/dev/newsletter/main.gocmd/dev/newsletter/send.gocmd/dev/openapi/main.gocmd/dev/openapi/migrate.gocmd/dev/release/compile.gocmd/dev/release/main.gocmd/dev/release/notify/draft.gocmd/dev/release/notify/main.gocmd/dev/release/notify/send.gocmd/dev/release/publish.gocmd/dev/schema/main.gocmd/dev/schema/render_version.gocmd/dev/swagger/main.gocmd/dev/swagger/sanitize.gocmd/nodev.gocmd/root.gocmd/root_race_test.gocmd/version.go
💤 Files with no reviewable changes (1)
- Makefile
Comment on lines
+54
to
+59
| if ignorePkgs := strings.Split(os.Getenv("SWAGGER_SPEC_IGNORE_PKGS"), ","); len(ignorePkgs) > 0 { | ||
| for k, p := range ignorePkgs { | ||
| ignorePkgs[k] = "-x " + p | ||
| } | ||
| fmt.Printf(`export SWAGGER_SPEC_IGNORE_PKGS='%s'`, strings.Join(ignorePkgs, " ")) | ||
| } |
There was a problem hiding this comment.
Guard is always true when SWAGGER_SPEC_IGNORE_PKGS is unset.
strings.Split("", ",") returns [""] (length 1), so len(ignorePkgs) > 0 never short-circuits. When the env var is empty/unset, this emits export SWAGGER_SPEC_IGNORE_PKGS='-x ', which is not the intended no-op. This is pre-existing logic carried over by the refactor, but worth fixing while it's being touched.
🐛 Proposed fix
- if ignorePkgs := strings.Split(os.Getenv("SWAGGER_SPEC_IGNORE_PKGS"), ","); len(ignorePkgs) > 0 {
+ if raw := os.Getenv("SWAGGER_SPEC_IGNORE_PKGS"); raw != "" {
+ ignorePkgs := strings.Split(raw, ",")
for k, p := range ignorePkgs {
ignorePkgs[k] = "-x " + p
}
fmt.Printf(`export SWAGGER_SPEC_IGNORE_PKGS='%s'`, strings.Join(ignorePkgs, " "))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ignorePkgs := strings.Split(os.Getenv("SWAGGER_SPEC_IGNORE_PKGS"), ","); len(ignorePkgs) > 0 { | |
| for k, p := range ignorePkgs { | |
| ignorePkgs[k] = "-x " + p | |
| } | |
| fmt.Printf(`export SWAGGER_SPEC_IGNORE_PKGS='%s'`, strings.Join(ignorePkgs, " ")) | |
| } | |
| if raw := os.Getenv("SWAGGER_SPEC_IGNORE_PKGS"); raw != "" { | |
| ignorePkgs := strings.Split(raw, ",") | |
| for k, p := range ignorePkgs { | |
| ignorePkgs[k] = "-x " + p | |
| } | |
| fmt.Printf(`export SWAGGER_SPEC_IGNORE_PKGS='%s'`, strings.Join(ignorePkgs, " ")) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/dev/ci/github/env.go` around lines 54 - 59, The guard incorrectly treats
an unset/empty SWAGGER_SPEC_IGNORE_PKGS as non-empty because strings.Split("",
",") yields [""], causing export of "-x " entries; update the logic around
reading SWAGGER_SPEC_IGNORE_PKGS (the os.Getenv call and ignorePkgs handling) to
first check if the env var is non-empty (or filter out empty strings after
strings.Split) before building the "-x " list and calling fmt.Printf, ensuring
ignorePkgs contains only non-empty package names before joining and printing.
| debug bool | ||
| pr string | ||
| revisionRange string | ||
| branch string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify if `branch` is used anywhere in the monorepo package
rg -n '\bbranch\b' cmd/dev/ci/monorepo/Repository: ory/cli
Length of output: 259
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files in cmd/dev/ci/monorepo:"
fd -t f "" cmd/dev/ci/monorepo/ | sort
echo
echo "Occurrences of branch/localBranch in package:"
rg -n --hidden --no-ignore-vcs '\b(branch|localBranch)\b' cmd/dev/ci/monorepo/
echo
echo "Occurrences of the \"branch\" flag string in package:"
rg -n --hidden --no-ignore-vcs '"branch"' cmd/dev/ci/monorepo/Repository: ory/cli
Length of output: 1356
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for concurrent execution patterns in the package (goroutines/channels):"
rg -n --hidden --no-ignore-vcs '\bgo\s+\w|make\(|chan\s' cmd/dev/ci/monorepo/Repository: ory/cli
Length of output: 469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' cmd/dev/ci/monorepo/main.goRepository: ory/cli
Length of output: 1770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Also search for how "localRoot/localPR/localRev" are used vs "localBranch"
rg -n --hidden --no-ignore-vcs '\blocal(PR|Root|Rev|Branch)\b' cmd/dev/ci/monorepo/main.goRepository: ory/cli
Length of output: 614
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore-vcs '\b(rootDirectory|verbose|debug|pr|revisionRange)\b' cmd/dev/ci/monorepo/Repository: ory/cli
Length of output: 4697
Unused scratch variable branch in monorepo command
branch is declared and set from the --branch persistent flag (PersistentPreRun) but is never read anywhere in cmd/dev/ci/monorepo/—only pr is consumed (via isPR()) while other scratch vars (rootDirectory, verbose, debug, revisionRange) are used in run.go/changes.go. Remove branch (and the --branch flag binding) or wire it into the downstream logic.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 18-18: var branch is unused
(unused)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/dev/ci/monorepo/main.go` at line 18, Remove the unused scratch variable
"branch" and its flag binding from the monorepo command or wire it into
downstream logic; specifically, delete the "branch string" declaration and the
corresponding "--branch" PersistentFlag setup in main.go (the PersistentPreRun
populates it) OR propagate the variable into the existing flow (e.g., pass
branch into the functions used by run.go/changes.go or use it inside
isPR()/change-detection logic) so the value is consumed rather than unused.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Calling
cmdA.AddCommand(cmdB)modifies bothcmdAandcmdBand hence is not goroutine safe.Since we extensively test most commands and subcommands, we cannot have global
var ... = &cobra.Command{...}declarations.This diff is a lot more palatable when ignoring whitespace.
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores