Repository navigation
Conversation
The lint job's complexity check and the Makefile's complexity and complexity-report targets run `gocyclo ... .`, which walks the whole checkout. npm packages can ship Go source (frontend/node_modules/flatted ships flatted.go with Stringify at complexity 32), so any run after `npm ci` reports third-party functions as findings. Add `(^|/)(node_modules|vendor)/` to gocyclo's -ignore regex in all three places. gocyclo v0.6.0 already skips vendor/, testdata/ and dot/underscore directories, so the vendor/ part only makes that explicit; on a tree without node_modules the set of scanned files is unchanged. The pre-commit gocyclo hook is unaffected: it lists files with `git ls-files`, and frontend/node_modules/ is gitignored. Refs LeanerCloud#177 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
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
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.
Root cause
ci.yml's lint job runsgocyclo -over 10 -ignore "_test\.go" ., and the Makefile'scomplexityandcomplexity-reporttargets rungocyclo ... .. All three walk the whole checkout. gocyclo v0.6.0 skipsvendor/,testdata/and directories starting with.or_, but notnode_modules/. npm packages can ship Go source (frontend/node_modules/flatted/golang/pkg/flatted/flatted.go, withStringifyat complexity 32), so any run afternpm cireports third-party functions as findings. CI is not hit today only because the lint job doesn't install npm deps. Local runs and agents that copy the documented command are hit.Fix
Add
(^|/)(node_modules|vendor)/to gocyclo's-ignoreregex in:.github/workflows/ci.yml(lint job, "Check cyclomatic complexity"):-ignore "_test\.go|(^|/)(node_modules|vendor)/"Makefilecomplexityandcomplexity-report:-ignore '(^|/)(node_modules|vendor)/'. These targets keep their current behavior of including_test.gofiles.gocyclo checks
-ignorebefore it parses a file, so ignored files are never opened. Thevendor/part does nothing with gocyclo v0.6.0, which already skips vendor directories. It is there so the exclusion doesn't depend on that built-in behavior. The pattern is anchored to a whole path segment, so first-party directories such asinternal/vendorutil/orinternal/my_node_modules/are still checked (tested below).I kept the
.walk instead of switching to the issue'sgit ls-filesform so the set of checked files stays exactly the same. Agit ls-fileslist would also pick up tracked.gofiles undertestdata/,.-prefixed or_-prefixed directories, which the walk skips today.The pre-commit
gocyclohook needs no change. It lists files withgit ls-files, andfrontend/node_modules/is gitignored. I confirmed it passes with a fakenode_modulesGo file present (below).Refs #177
Verification
All of this is fixture evidence. I made a
mktemp -dcopy of the tree (git archive HEAD) and added a fakefrontend/node_modules/flatted/golang/pkg/flatted/flatted.gowith a complexity-13 function, plus a fakeinternal/fakevendor/vendor/x/x.go. Nonpm installwas run. Go 1.26.9, gocyclo v0.6.0.Files scanned, old command vs new. I traced each command with
strace -f -e trace=openatand listed every.gofile it opened:gocyclo -over 10 -ignore "_test\.go" .13 flatted Stringify frontend/node_modules/.../flatted.go:3:1)... -ignore "_test\.go|(^|/)(node_modules|vendor)/" .gocyclo -over 10 ./-top 20 .-ignore)node_modulespresent,diff old newshows exactly one line in each case:< frontend/node_modules/flatted/golang/pkg/flatted/flatted.go.cmp).git ls-files '*.go' | grep -v _test.goon this branch (258 files), and to afindthat prunesvendor,testdata,node_modules, and./_-prefixed directories.vendor/file was skipped by both the old and new commands, as expected from gocyclo's built-in skip list.The workflow step itself, before and after. I extracted the "Check cyclomatic complexity"
run:block fromorigin/mainand from this branch with a YAML parser, then ran each withbash -ein the fixture tree:node_modulespresent:❌ Found functions ... flatted Stringify ..., exit 1node_modulespresent:✅ All functions have acceptable cyclomatic complexity (≤10), exit 0The new regex still catches first-party code. I put the same complexity-13 function in
internal/vendorutil/f.goandinternal/my_node_modules/f.go. The new CI command reported both (exit 1) and still skipped thenode_modules/copy.Make targets. In the fixture, the old
make complexitylisted theflattedfinding and the new one did not (61 vs 60 findings; the other 60 are explained under "Out of scope").make -n complexity complexity-reportexpands to the expected-ignore '(^|/)(node_modules|vendor)/'command lines.Pre-commit hook.
gocyclo -over 10 $(git ls-files "*.go" | grep -v _test.go | grep -v vendor/)exits 0 with the fakenode_modulesfile present, so it is not affected.Lint.
actionlintv1.7.12 on.github/workflows/ci.yml: no findings.shellcheck0.11.0 on the extracted step script: no findings.Merge check.
git merge-tree --write-treeagainst the heads of #1222, #1227 and #1119 and the fork branchesfix/issue-1233andfix/issue-353reports no conflicts. Theirci.ymlhunks are at lines 743+ and 1299+; #1227's Makefile hunks are at lines 11 and 159.There is no committed regression test. The fix is a regex argument inside a workflow
run:block and two Makefile recipes, and a test would have to parse it out of the YAML and Makefile. That is brittle, and it would need another CI step to run it. The evidence above is the fail-before/pass-after proof.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope (left for follow-up)
./...picks upnode_modules). I reproduced it in the fixture:go list ./...emitsgithub.com/LeanerCloud/cloud-commitments-platform/frontend/node_modules/flatted/golang/pkg/flatted. It reaches CI through the pre-commitgo-vethook (go vet ./...), whichpre-commit.ymlruns afternpm ci. The same applies to the pre-pushgo-testhook andmake tidy-check. This PR does not change it, which is why it saysRefs #177rather thanCloses. One candidate fix: the Go 1.25+ignore ./frontend/node_modulesdirective ingo.mod. In the fixture it dropped the package fromgo list ./...(43 to 42 packages) and would cover every Go-tool./...walk at once. I didn't make that change here becausego.modis parsed by many tools (Dependabot, gosec, govulncheck, golangci-lint), and checking all of them is more than this xs chore covers. The narrower alternative isgo vet $(go list ./... | grep -v /node_modules/)in the hook. If a maintainer prefers either one here, I can add it and switch toCloses.make complexitydoesn't exclude_test.go, unlike CI. It fails onmaintoday with 60 test-file findings. I left that alone because changing it would change which files the target checks.git ls-fileshook also includes tracked.gofiles undertestdata/,.-prefixed or_-prefixed directories, which CI's walk skips. Onmaintoday the two sets are the same.Labels
Could a maintainer mirror the issue's triage labels onto this PR:
effort/xs,impact/internal,priority/p3,severity/low,triaged,type/chore,urgency/eventually.🤖 Generated with Claude Code