chore(wren-launcher): add CI flow and apply lint tools#1877
Conversation
WalkthroughAdds a new GitHub Actions workflow for the wren-launcher subdirectory, a golangci config, expanded Makefile build/quality targets, README CI/docs updates, Go toolchain bump to 1.24, tightened filesystem permissions and security annotations, refactored DBT DataSource interface and tests, and Docker API/type updates in utils. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant CI as Wren Launcher CI
participant Lint as golangci / fmt-and-test
participant Scan as security-scan
Dev->>GH: Open PR (synchronize / labeled) or update workflow
GH->>CI: Trigger workflow (scoped to wren-launcher/**)
CI->>Lint: Run golangci-lint job (golangci) and fmt+test job (fmt-and-test)
Lint-->>CI: Report results
CI->>Scan: On fmt-and-test success, run security-scan (gosec, go mod verify)
Scan-->>CI: Report findings
CI-->>GH: Post statuses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wren-launcher/README.md (1)
18-45: Well-documented CI workflow with minor formatting issue.The Continuous Integration section provides excellent documentation of the GitHub Actions workflow, including trigger conditions and job descriptions. However, there's a minor formatting issue with heading punctuation.
Apply this diff to fix the trailing punctuation in headings (flagged by markdownlint):
-### CI Jobs: +### CI Jobs -### Triggering CI on Pull Requests: +### Triggering CI on Pull Requestswren-launcher/Makefile (1)
23-68: LGTM! Comprehensive code quality targets with minor suggestion.The new Makefile targets provide excellent support for code quality automation and CI integration. The structure is clean and follows good practices with proper
.PHONYdeclarations.Consider adding
.PHONYdeclaration for the existingcleantarget to address static analysis feedback:+.PHONY: clean clean: go clean rm -rf dist
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/wren-launcher-ci.yaml(1 hunks)wren-launcher/.golangci.yml(1 hunks)wren-launcher/Makefile(2 hunks)wren-launcher/README.md(1 hunks)wren-launcher/commands/dbt/converter.go(5 hunks)wren-launcher/commands/dbt/data_source.go(2 hunks)wren-launcher/commands/dbt/data_source_test.go(7 hunks)wren-launcher/commands/dbt/profiles_analyzer.go(1 hunks)wren-launcher/commands/launch.go(5 hunks)wren-launcher/config/config.go(2 hunks)wren-launcher/utils/docker.go(12 hunks)wren-launcher/utils/rc.go(4 hunks)wren-launcher/utils/rc_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-09T02:43:20.433Z
Learnt from: goldmedal
PR: Canner/WrenAI#1827
File: wren-launcher/commands/dbt/data_source.go:50-53
Timestamp: 2025-07-09T02:43:20.433Z
Learning: In wren-launcher/commands/dbt/data_source.go, when encountering unsupported database types in convertConnectionToDataSource function, the user prefers to log a warning and continue processing instead of failing. The function should return nil, nil to allow processing to continue.
Applied to files:
wren-launcher/commands/dbt/converter.gowren-launcher/commands/launch.gowren-launcher/commands/dbt/data_source.gowren-launcher/commands/dbt/data_source_test.go
📚 Learning: 2025-07-09T02:43:07.560Z
Learnt from: goldmedal
PR: Canner/WrenAI#1827
File: wren-launcher/commands/dbt/data_source.go:154-156
Timestamp: 2025-07-09T02:43:07.560Z
Learning: In the WrenAI codebase, the MapType methods for data sources should return the original type name for unknown types, rather than a default fallback like "string". This maintains consistency across all data source implementations.
Applied to files:
wren-launcher/commands/dbt/data_source.go
📚 Learning: 2025-01-15T03:50:47.868Z
Learnt from: andreashimin
PR: Canner/WrenAI#1117
File: wren-ui/src/apollo/server/backgrounds/chart.ts:86-86
Timestamp: 2025-01-15T03:50:47.868Z
Learning: In the WrenAI codebase, ChartType is maintained as a string in the web database, while the ChartType enum is specifically used for AI adaptor input/output interfaces. Therefore, string case conversions like toUpperCase() are intentional for maintaining data consistency in the web DB.
Applied to files:
wren-launcher/commands/dbt/data_source.go
🧬 Code Graph Analysis (1)
wren-launcher/commands/launch.go (1)
wren-launcher/utils/os.go (1)
Openbrowser(35-49)
🪛 markdownlint-cli2 (0.17.2)
wren-launcher/README.md
26-26: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
43-43: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 checkmake (0.2.2)
wren-launcher/Makefile
[warning] 55-55: Target body for "help" exceeds allowed length of 5 (13).
(maxbodylength)
[warning] 54-54: Missing required phony target "all"
(minphony)
[warning] 54-54: Missing required phony target "clean"
(minphony)
🪛 YAMLlint (1.37.1)
.github/workflows/wren-launcher-ci.yaml
[error] 33-33: trailing spaces
(trailing-spaces)
[error] 82-82: trailing spaces
(trailing-spaces)
[error] 100-100: trailing spaces
(trailing-spaces)
[error] 103-103: trailing spaces
(trailing-spaces)
[error] 106-106: trailing spaces
(trailing-spaces)
[error] 122-122: trailing spaces
(trailing-spaces)
[error] 137-137: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (31)
wren-launcher/commands/dbt/profiles_analyzer.go (1)
15-15: LGTM! Appropriate security annotation.The
#nosec G304comment properly documents that the file path is application-controlled, which is a valid justification for suppressing path traversal warnings in this context.wren-launcher/commands/dbt/data_source.go (2)
11-38: Excellent refactoring with constants for type mapping.The introduction of separate constant groups for normalized internal types and SQL data types significantly improves code maintainability and reduces the risk of typos in type mappings.
172-183: LGTM! Proper use of constants in MapType.The switch statement now uses the defined constants instead of hardcoded strings, which improves maintainability. The default case behavior correctly returns the lowercase original type, maintaining consistency with previous learnings.
wren-launcher/utils/docker.go (5)
92-92: Appropriate security annotation for trusted URL.The
#nosec G107comment correctly documents that the URL comes from trusted source constants, justifying the suppression of the HTTP client warning.
96-96: Improved resource cleanup with error-ignoring deferred functions.The anonymous deferred functions that explicitly ignore errors provide consistent resource cleanup handling across the codebase.
Also applies to: 103-103, 497-497, 511-511
168-168: Enhanced security with tightened file permissions.Changing file permissions from
0644to0600improves security by restricting access to the owner only, which is appropriate for configuration files containing potentially sensitive data.Also applies to: 292-292
406-406: Docker API compatibility update.The transition from
types.Containertocontainer.Summaryreflects proper Docker API usage and maintains type consistency across container-related functions.Also applies to: 430-430, 433-433, 446-446, 449-449
468-468: Enhanced port validation logic.Adding port range validation (
port >= 0 && port <= 65535) before comparison improves robustness and prevents potential issues with invalid port numbers.Also applies to: 483-483
wren-launcher/config/config.go (2)
8-12: Good introduction of platform constants.The platform constants improve code maintainability and reduce the risk of typos in platform string usage.
42-42: Consistent use of platform constants in GetPlatform().Replacing hardcoded strings with the defined constants improves maintainability while preserving the existing platform detection logic.
Also applies to: 44-44, 47-47, 49-49, 51-51, 53-53
wren-launcher/utils/rc_test.go (2)
13-13: Improved test cleanup with error-ignoring deferred functions.The anonymous deferred functions provide consistent resource cleanup handling in tests, aligning with the pattern used throughout the codebase.
Also applies to: 40-40
34-34: Better test function naming.Renaming
TestReadNonExistentKeytoTestSetbetter reflects the test's actual purpose and improves test clarity.wren-launcher/README.md (1)
9-16: LGTM! Clear documentation for code quality commands.The Code Quality section provides comprehensive and well-structured documentation for the available Makefile targets, making it easy for developers to understand the available commands for maintaining code quality.
wren-launcher/commands/dbt/converter.go (3)
40-40: LGTM! Security improvement through tightened directory permissions.Changing directory permissions from
0755to0750removes read/execute access for "other" users, which is a good security practice that aligns with similar improvements throughout the codebase.
172-172: LGTM! Enhanced file security with tightened permissions.Changing file write permissions from
0644to0600for configuration files ensures that sensitive data inwren-datasource.jsonandwren-mdl.jsonis only accessible by the file owner, improving the security posture.Also applies to: 201-201
247-247: LGTM! Appropriate security annotations for controlled file paths.The
// #nosec G304annotations correctly mark these file operations as safe since the paths are controlled by the application logic, not user input. This helps reduce false positives in security scanning while maintaining actual security.Also applies to: 261-261
wren-launcher/utils/rc.go (3)
29-29: LGTM! Critical security improvement for directory permissions.Changing from
os.ModePerm(0777) to0750significantly improves security by following the principle of least privilege. The previous setting allowed full access to all users, which was unnecessarily permissive.
38-38: LGTM! Proper security annotations for application-controlled file paths.The
// #nosec G304annotations are correctly applied to file operations where paths are controlled by the application's internal logic (getWrenRcFilePath()), not external input, making them safe from path traversal vulnerabilities.Also applies to: 53-53, 126-126
42-42: LGTM! Improved resource management with deferred cleanup.The use of deferred anonymous functions for file closing ensures proper resource cleanup regardless of function exit paths. Ignoring close errors in cleanup contexts is an acceptable practice when the primary operations have completed successfully.
Also applies to: 57-57, 130-130
wren-launcher/commands/launch.go (3)
32-34: LGTM! Enhanced security and error handling for directory operations.The permission tightening to
0750improves security by removing "other" access, and the improved error handling inprepareProjectDir(returning empty string on failure) provides better error propagation compared to potentially ignoring the error.Also applies to: 511-511
195-196: LGTM! Improved robustness of input handling.Changing from
fmt.Scanf("h")to reading into dummy variables with ignored errors prevents potential blocking, panics, or format mismatches while maintaining the intended pause behavior for user interaction.Also applies to: 379-380, 468-468
376-376: LGTM! Appropriate error handling for browser opening.Explicitly ignoring the browser opening error is correct here - failing to open a browser shouldn't prevent the service from running, and users can manually navigate to the URL if needed.
wren-launcher/.golangci.yml (1)
1-46: LGTM! Comprehensive and well-configured linting setup.This golangci-lint configuration provides excellent coverage across code quality, security, and style concerns. The linter selection is appropriate, custom settings are reasonable (gocyclo complexity at 20, dupl threshold at 150), and the relaxed rules for test files are a good practice.
.github/workflows/wren-launcher-ci.yaml (3)
21-21: Complex concurrency group logic - verify correctness.The concurrency group expression is quite complex and may not behave as expected in all scenarios.
The current logic tries to avoid canceling CI runs when labels other than "launcher" are added, but the expression might not handle all edge cases correctly. Consider simplifying to:
- group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.event.label.name == 'launcher' && github.event.number || github.sha }} + group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.event.number || github.sha }}This would still provide PR-based grouping without the complex label condition.
96-109: LGTM! Comprehensive cross-platform build testing.The build matrix appropriately covers key platforms (Linux AMD64/ARM64, Darwin AMD64) with proper environment variables and build verification.
41-41: Confirmed Go 1.24 availability – no change neededAs of July 8, 2025, the latest Go 1.24 patch release is 1.24.5. GitHub Actions’
go-version: '1.24'will automatically resolve to that patch, so the workflow is valid as-is.Also applies to lines 90 and 130.
wren-launcher/commands/dbt/data_source_test.go (5)
7-14: LGTM! Good use of constants for test maintainability.Extracting common test values into constants reduces duplication and makes tests easier to maintain.
16-45: Excellent helper function design.The
validatePostgresDataSourcehelper consolidates repeated assertions and usest.Helper()correctly to ensure proper test failure reporting. This significantly improves test readability and maintainability.
84-119: Good additional test coverage.The new
TestFromDbtProfiles_PostgresWithDbNametest provides valuable coverage for the database name scenario, and properly reuses the validation helper.
204-229: Excellent abstraction with Validator interface and generic helper.The
Validatorinterface andtestDataSourceValidationhelper function create a reusable pattern for validation testing. This is a great example of the DRY principle and makes adding validation tests for new data source types much easier.
231-271: Well-executed refactoring of validation tests.The conversion to use the new
testDataSourceValidationhelper while maintaining the same test coverage demonstrates good refactoring practices. The test cases remain comprehensive and the code is now more maintainable.
goldmedal
force-pushed
the
chore/launcher-ci
branch
from
c5c4517 to
c43d10d
Compare
goldmedal
force-pushed
the
chore/launcher-ci
branch
from
1eb30c4 to
67cdaee
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (5)
wren-launcher/commands/dbt/data_source.go (2)
323-339: The default case should return the unmodifiedsourceType, not its lowercase form, to align with how other data sources preserve unknown types.• File: wren-launcher/commands/dbt/data_source.go
Lines: 323–339
Update thedefaultbranch inDefaultDataSource.MapType:func (d *DefaultDataSource) MapType(sourceType string) string { // Default type mapping switch strings.ToLower(sourceType) { case "integer", "int", "bigint", "int64": return "integer" case "varchar", "text", "string", "char": return "varchar" case "timestamp", "datetime", "date": return "timestamp" case "double", "float", "decimal", "numeric": return "double" case "boolean", "bool": return "boolean" default: - return strings.ToLower(sourceType) + return sourceType } }This ensures that unknown types emerge exactly as provided, matching the MapType contract used elsewhere.
171-188: Preserve the true original type for unknown mappingsThe default branch should return the caller’s exact input (including its original casing), not a lower- or uppercased version. Capture the incoming
sourceTypebefore normalization and fall back to that value.File:
wren-launcher/commands/dbt/data_source.gofunc (ds *WrenLocalFileDataSource) MapType(sourceType string) string { - // Convert to uppercase for consistent mapping - sourceType = strings.ToUpper(sourceType) + // Preserve the user’s exact input for the fallback case + originalType := sourceType + // Convert to uppercase for consistent switch matching + sourceType = strings.ToUpper(sourceType) switch sourceType { case integerSQL, intSQL, bigintSQL: return integerType case varcharSQL, textSQL, stringSQL: return varcharType case dateSQL: return dateType case timestampSQL, datetimeSQL: return timestampType case doubleSQL, floatSQL, numericSQL, decimalSQL: return doubleType case booleanSQL, boolSQL: return booleanType default: - // Return the original type if no mapping is found - return strings.ToLower(sourceType) + // Return the exact original type if no mapping is found + return originalType } }wren-launcher/Makefile (1)
5-15: Ensure dist exists and avoid CGO for Darwin cross-builds on Linux runners
- Create dist/ before building to avoid failures.
- CGO=1 for darwin targets will fail on Linux CI runners (no cross toolchain). Use CGO_ENABLED=0 unless building on macOS.
build: - env GOARCH=amd64 GOOS=darwin CGO_ENABLED=1 go build -o dist/${BINARY_NAME}-darwin main.go - env GOARCH=arm64 GOOS=darwin CGO_ENABLED=1 go build -o dist/${BINARY_NAME}-darwin-arm64 main.go + mkdir -p dist + env GOARCH=amd64 GOOS=darwin CGO_ENABLED=0 go build -o dist/${BINARY_NAME}-darwin main.go + env GOARCH=arm64 GOOS=darwin CGO_ENABLED=0 go build -o dist/${BINARY_NAME}-darwin-arm64 main.go env GOARCH=amd64 GOOS=linux CGO_ENABLED=0 go build -o dist/${BINARY_NAME}-linux main.go env GOARCH=arm64 GOOS=linux CGO_ENABLED=0 go build -o dist/${BINARY_NAME}-linux-arm64 main.go env GOARCH=amd64 GOOS=windows CGO_ENABLED=0 go build -o dist/${BINARY_NAME}-windows.exe main.gowren-launcher/utils/docker.go (2)
165-168: Guard against unknown generation modelsIf generationModel isn’t in generationModelToModelName, the replacement becomes “litellm_llm.” which is invalid. Add a presence check and fallback.
- if generationModel != "gpt-4.1-nano" { - config = strings.ReplaceAll(config, "litellm_llm.default", "litellm_llm."+generationModelToModelName[generationModel]) - } + if generationModel != "gpt-4.1-nano" { + if mapped, ok := generationModelToModelName[generationModel]; ok && mapped != "" { + config = strings.ReplaceAll(config, "litellm_llm.default", "litellm_llm."+mapped) + } // else: keep default + }
181-189: Differentiate non-existence from other read errors in mergeEnvContentThe comment says “does not exist” but the code treats any error as “not exist”. This masks permission or I/O errors.
- if _, err := os.Stat(newEnvFile); err != nil { - return envFileContent, nil - } + if _, err := os.Stat(newEnvFile); err != nil { + if os.IsNotExist(err) { + return envFileContent, nil + } + return "", err + }
🧹 Nitpick comments (8)
wren-launcher/README.md (3)
26-26: Fix markdownlint MD026: remove trailing punctuation in headingRemove the trailing colon in the “CI Jobs” heading to satisfy MD026.
-### CI Jobs: +### CI Jobs
20-25: Clarify CI triggers to match the workflow (paths and label behavior)The workflow triggers on:
- push to main only when files under wren-launcher/** or the workflow file change,
- pull_request events of types synchronize and labeled, but only when the PR changes those paths.
Also, jobs are not actually gated to run only when the label is present; they run on synchronize too. Consider clarifying text or adding job-level gating (see workflow comment).
Proposed doc tweak:
-This project uses GitHub Actions for CI/CD. The workflow runs automatically on: +This project uses GitHub Actions for CI/CD. The workflow runs automatically on changes under `wren-launcher/**` (or this workflow file) via: -- **Push to main branch**: Runs all checks and tests -- **Pull Request with label `launcher`**: Runs all checks and tests when PR is labeled +- **Push to main branch**: Runs all checks and tests +- **Pull Request**: Runs on synchronize (new commits) or when labeled. To explicitly trigger CI via labeling, use `ci/launcher`. - **Manual trigger**: Can be triggered manually via GitHub Actions UI
9-16: Optional: Document all Makefile targets for completenessMakefile provides more helpful targets (imports, lint-fix, fix, help). Consider listing them here for discoverability.
.github/workflows/wren-launcher-ci.yaml (2)
55-56: goimports is installed but never usedEither run the Makefile’s imports target to apply import formatting or remove this installation to keep the pipeline lean.
- - name: Install goimports - run: go install golang.org/x/tools/cmd/goimports@latest + # Optional: run imports fix if you want to enforce import formatting + - name: Fix imports + run: make imports
19-23: Optional: Gate jobs on label presence to match README behaviorCurrently, jobs run on synchronize even without the label. If you want to truly require the label, add job-level conditions.
Example job-level if:
jobs: golangci: if: > github.event_name != 'pull_request' || contains(join(github.event.pull_request.labels.*.name, ','), 'ci/launcher')wren-launcher/commands/dbt/data_source.go (1)
111-131: Confirm whether Url should retain the filename instead of directoryresolvePath returns only the directory (filepath.Dir), discarding the filename. If Url is expected to point to a file path, this could be incorrect and break file resolution.
If Url should be the full path, change resolvePath to return the full path (with optional dbtHome prefix for relative paths). I can propose a patch once confirmed.
wren-launcher/Makefile (2)
24-69: Add missing PHONYs to satisfy checkmake and avoid accidental file/dir target collisionsDeclare common targets as phony (including clean, build, rebuild, all, help). Optionally add an all target.
-.PHONY: fmt +.PHONY: all build clean rebuild fmt imports vet lint lint-fix check fix test help + +# Optional default target +all: check test build
29-31: Use goimports directly without hardcoded GOPATH pathRelying on $(shell go env GOPATH)/bin path can fail if the bin is not on PATH in all environments. Call goimports directly and let CI ensure it’s installed.
-imports: - $(shell go env GOPATH)/bin/goimports -w . +imports: + goimports -w .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/workflows/wren-launcher-ci.yaml(1 hunks)wren-launcher/.golangci.yml(1 hunks)wren-launcher/Makefile(2 hunks)wren-launcher/README.md(1 hunks)wren-launcher/commands/dbt/converter.go(5 hunks)wren-launcher/commands/dbt/data_source.go(2 hunks)wren-launcher/commands/dbt/data_source_test.go(7 hunks)wren-launcher/commands/dbt/profiles_analyzer.go(1 hunks)wren-launcher/commands/launch.go(5 hunks)wren-launcher/config/config.go(2 hunks)wren-launcher/go.mod(1 hunks)wren-launcher/utils/docker.go(12 hunks)wren-launcher/utils/rc.go(4 hunks)wren-launcher/utils/rc_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- wren-launcher/commands/dbt/converter.go
- wren-launcher/utils/rc_test.go
- wren-launcher/commands/dbt/profiles_analyzer.go
- wren-launcher/.golangci.yml
- wren-launcher/go.mod
- wren-launcher/utils/rc.go
- wren-launcher/commands/launch.go
- wren-launcher/config/config.go
- wren-launcher/commands/dbt/data_source_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-09T02:43:20.433Z
Learnt from: goldmedal
PR: Canner/WrenAI#1827
File: wren-launcher/commands/dbt/data_source.go:50-53
Timestamp: 2025-07-09T02:43:20.433Z
Learning: In wren-launcher/commands/dbt/data_source.go, when encountering unsupported database types in convertConnectionToDataSource function, the user prefers to log a warning and continue processing instead of failing. The function should return nil, nil to allow processing to continue.
Applied to files:
wren-launcher/commands/dbt/data_source.go
📚 Learning: 2025-07-09T02:43:07.560Z
Learnt from: goldmedal
PR: Canner/WrenAI#1827
File: wren-launcher/commands/dbt/data_source.go:154-156
Timestamp: 2025-07-09T02:43:07.560Z
Learning: In the WrenAI codebase, the MapType methods for data sources should return the original type name for unknown types, rather than a default fallback like "string". This maintains consistency across all data source implementations.
Applied to files:
wren-launcher/commands/dbt/data_source.go
🪛 checkmake (0.2.2)
wren-launcher/Makefile
[warning] 55-55: Target body for "help" exceeds allowed length of 5 (13).
(maxbodylength)
[warning] 54-54: Missing required phony target "all"
(minphony)
[warning] 54-54: Missing required phony target "clean"
(minphony)
🪛 markdownlint-cli2 (0.17.2)
wren-launcher/README.md
26-26: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
43-43: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (4)
.github/workflows/wren-launcher-ci.yaml (1)
39-43: Ignore version pin suggestion
Theversion: v2.3.1setting is valid—golangci-lint v2.3.1 is the official latest stable release. No changes are required here.Likely an incorrect or invalid review comment.
wren-launcher/commands/dbt/data_source.go (1)
81-85: LGTM: Unsupported DB types are warned and skipped (per team preference)Logging a warning and returning nil for unsupported types matches the documented team preference from prior PRs.
wren-launcher/utils/docker.go (2)
409-447: Container listing and filters look correct with container.SummaryThe migration from types.Container to container.Summary appears correct, including label checks and return semantics.
470-487: Port checks include bounds validation; LGTMIncluding 0–65535 guard before comparing PublicPort reduces false matches from invalid inputs.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/wren-launcher-ci.yaml (1)
69-70: Run the full test suite, not just ./commands/dbtUse go test ./... (or your Makefile target) to avoid missing regressions in other packages.
- - name: Run tests - run: go test ./commands/dbt + - name: Run tests + run: go test ./... + # Alternatively: + # run: make testwren-launcher/README.md (1)
22-24: Align label name with workflow: useci/launcherBullet still references label
launcher. The workflow and later section useci/launcher. Fix for consistency.-- **Pull Request with label `launcher`**: Runs all checks and tests when PR is labeled +- **Pull Request with label `ci/launcher`**: Runs all checks and tests when PR is labeled
🧹 Nitpick comments (8)
.github/workflows/wren-launcher-ci.yaml (6)
59-66: Simplify and harden the format checkUse git diff’s exit code instead of string tests; it’s simpler and avoids false positives.
- name: Run format check run: | make fmt - # Check if there are any formatting changes - if [ -n "$(git diff --name-only)" ]; then - echo "Code is not formatted properly. Please run 'make fmt' and commit the changes." - git diff - exit 1 - fi + # Fail if formatting changed any tracked file + git diff --name-only --exit-code || { + echo "Code is not formatted properly. Please run 'make fmt' and commit the changes."; + git diff; + exit 1; + }
33-35: Pin GitHub Actions to commit SHAs for supply-chain hardeningactions/checkout and actions/setup-go are referenced by mutable tags. Pin to verified commit SHAs to mitigate hijack risks.
Example:
- uses: actions/checkout@v4 + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4 - uses: actions/setup-go@v5 + uses: actions/setup-go@cdcb36043654635271a94b9a6d1392de5bb323a7 # v5Repeat in all jobs.
Also applies to: 48-53, 77-82
38-42: Pin golangci-lint action and tool version deterministicallyUse a commit SHA for the action, and consider pinning golangci-lint to a fixed version to ensure reproducible lint results.
Example:
-uses: golangci/golangci-lint-action@v8 +uses: golangci/golangci-lint-action@d8099e1d2a3a538b1e2fa58dfb51d4cc49e5f3d0 # v6/v7/v8 commit -with: - version: v2.3.1 +with: + version: v1.60.3 # example fixed linter version; pick the one you standardize on
55-55: Avoid @latest for goimports; pin for reproducibilityInstalling latest can change behavior across runs. Pin a known-good version.
- run: go install golang.org/x/tools/cmd/goimports@latest + run: go install golang.org/x/tools/cmd/goimports@v0.21.0 # pick your vetted version
83-86: Avoid @latest for gosec; pin, or use a maintained actionPin gosec to a specific version to keep findings stable. Optionally, use a dedicated action.
- go install github.com/securego/gosec/v2/cmd/gosec@latest + go install github.com/securego/gosec/v2/cmd/gosec@v2.20.0 # example; choose your baseline
88-90: jq may not be present on all runnersUbuntu images usually have jq, but it isn’t guaranteed. Install it explicitly or guard usage.
Option A (install):
- - name: Run Go mod audit + - name: Run Go mod audit run: | + sudo apt-get update && sudo apt-get install -y jq go mod verify go list -json -deps ./... | jq -r '.Module | select(.Version) | "\(.Path) \(.Version)"' | sort -uOption B (skip if missing):
command -v jq >/dev/null || { echo "jq not found; skipping module dump"; exit 0; }wren-launcher/README.md (2)
26-28: Fix markdownlint MD026: remove trailing colon from headingRemove the colon at the end of “CI Jobs:” to satisfy MD026.
-### CI Jobs: +### CI Jobs
39-41: Confirm doc matches actual gating behaviorThe doc instructs adding label
ci/launcherto run CI. Current workflow also triggers on synchronize. Either add job-level gating in the workflow or clarify here that CI also runs on synchronize when code paths match.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/wren-launcher-ci.yaml(1 hunks)wren-launcher/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
wren-launcher/README.md
26-26: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
.github/workflows/wren-launcher-ci.yaml (1)
80-82: No action needed: Go 1.24 is fully supported and alignedVerified that:
- Go 1.24 was officially released on February 11, 2025.
- actions/setup-go@v5 accepts
go-version: '1.24'and resolves to the latest 1.24.x.- Your
wren-launcher/go.modspecifiesgo 1.24.The CI workflow snippet is correct as written.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wren-launcher/README.md (1)
22-24: Use the exact PR label used by the workflow (avoid drift)README uses label
launcher, but previous review and workflow conventions indicateci/launcher. Align this or confirm the workflow useslauncher.-- **Pull Request with label `launcher`**: Runs all checks and tests when PR is labeled +- **Pull Request with label `ci/launcher`**: Runs all checks and tests when PR is labeledIf the workflow indeed uses
launcher, keep as-is and consider adding a note stating “label must match workflow configuration”.
🧹 Nitpick comments (3)
wren-launcher/README.md (3)
26-26: Fix markdownlint MD026: remove trailing colon from headingRemove the colon to satisfy no-trailing-punctuation.
-### CI Jobs: +### CI Jobs
20-21: Document Go version used in CIAdd a short note that CI runs with Go 1.24 (per workflow), to make local expectations clear.
-This project uses GitHub Actions for CI/CD. The workflow runs automatically on: +This project uses GitHub Actions for CI/CD (Go 1.24 with module caching). The workflow runs automatically on:
11-16: Optionally document additional Makefile quality targetsMakefile includes more targets (imports, lint-fix, fix, help). Listing them improves developer ergonomics.
make check # Run all checks (fmt, vet, lint) make test # Run tests make fmt # Format code make vet # Run go vet make lint # Run golangci-lint +make imports # Organize imports (goimports) +make lint-fix # Auto-fix lint issues where possible +make fix # Run fmt + imports + lint-fix +make help # List available targets
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/wren-launcher-ci.yaml(1 hunks)wren-launcher/README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/wren-launcher-ci.yaml
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
wren-launcher/README.md
26-26: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
wren-launcher/README.md (2)
18-21: CI section addition is clear and usefulGood overview of CI/CD and when it runs. Once labels and job list are aligned with the workflow, this section will be solid.
28-38: Update README to reflect all three CI jobsThe workflow defines three jobs—
golangci,fmt-and-test, andsecurity-scan—but the README only lists two. Please updatewren-launcher/README.md(lines 28–38) to match:-1. **Lint and Test**: - - Code formatting check - - Go vet analysis - - golangci-lint checks - - Unit tests - - All quality checks - -2. **Security Scan**: - - Gosec security analysis - - Go module verification +1. **Lint (golangci-lint)** + - Runs golangci-lint as configured in .golangci.yml + +2. **Format and Test** + - Code formatting check (`make fmt`) + - Go vet analysis (`make vet`) + - Unit tests (`go test ./commands/dbt`) + +3. **Security Scan** + - Gosec security analysis (`gosec ./…`) + - Go module verification (`go mod verify` + audit)[fіx_required]
2 participants
Description
Summary by CodeRabbit
New Features
Documentation
Bug Fixes / Security
Tests