ci: add lint and security jobs, update AGENTS.md with best practices; enhance Makefile and docs#119
Conversation
📝 WalkthroughWalkthroughAdds comprehensive CI/CD infrastructure with new linting and security jobs, expands golangci-lint configuration with tiered linter coverage, introduces Makefile targets for build operations, and adds developer guidance documentation. Also includes minor formatting corrections across multiple files. Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Removed CGO_ENABLED environment variable from CI workflow.
Removed CGO_ENABLED variable and export from Makefile
Removed 'needs' dependency for the build job.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
metaphorics
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.golangci.yml:
- Around line 111-112: The linter config currently excludes context.WithTimeout
and context.WithCancel from errcheck; remove those exclusions so errcheck will
flag discarded cancel funcs, then audit usages of context.WithTimeout and
context.WithCancel across the repo (look for calls to context.WithTimeout and
context.WithCancel) and update each call site to capture the returned cancel
function and ensure it is called (typically via defer cancel() or explicit
cancel where appropriate) to avoid goroutine/resource leaks; after changes
re-run golangci-lint to confirm no discarded-cancel warnings remain.
- Line 126: Remove the exception for crypto/rand.Read from the linter config
(undo the "- crypto/rand.Read" exclusion) so errcheck will enforce handling,
then audit all call sites that invoke crypto/rand.Read and update them to check
and handle the returned error (propagate the error, return it to callers, or
fail-fast with a clear error message), ensuring any tests are updated to cover
error paths; reference the crypto/rand.Read symbol to locate call sites and the
linter exclusion entry to remove.
In `@Makefile`:
- Line 1: The .PHONY declaration is missing the build-protoc target; update the
.PHONY line (which currently lists help fmt vet lint test vuln tidy all run
build build-wasm compress-wasm build-frontend build-tunnel build-server clean)
to include build-protoc so the Makefile treats build-protoc as a phony target
and avoids conflicts with any file named build-protoc.
🧹 Nitpick comments (4)
Makefile (3)
5-14: Help text is incomplete—new targets undocumented.The
helptarget doesn't listfmt,vet,lint,test,vuln,tidy, orall. Developers won't discover these viamake help.Proposed fix
help: `@echo` "Available targets:" + `@echo` " make fmt - Format code (gofmt + goimports)" + `@echo` " make vet - Run go vet" + `@echo` " make lint - Run golangci-lint" + `@echo` " make test - Run tests with race detector" + `@echo` " make vuln - Run govulncheck" + `@echo` " make tidy - Tidy and verify go modules" + `@echo` " make all - Run fmt, vet, lint, test, vuln, build" `@echo` " make build - Build everything (protoc, wasm, frontend, server)"
15-17:goimportsrequires separate installation.
goimportsisn't part of the standard Go toolchain. Consider documenting this dependency or adding an install step, otherwisemake fmtwill fail for new contributors.
35-35:alltarget runsfmtwhich mutates files—problematic in CI.Running
fmtas part ofallwill modify source files. If used in CI, this could cause unexpected behavior or mask formatting issues. Consider a separatecheck-fmttarget or removingfmtfromall..github/workflows/ci.yml (1)
63-64: Consider pinning golangci-lint version for reproducibility.Using
@latestcan cause unexpected CI failures when new linter versions introduce stricter checks. Pin to a specific version (e.g.,@v2.1.0) for consistent behavior.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
cmd/relay-server/frontend/public/favicon.svgis excluded by!**/*.svggo.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.github/workflows/ci.yml.golangci.yml.pre-commit-config.yamlAGENTS.mdCLAUDE.mdMakefilebuf.gen.yamlcmd/portal-tunnel/README.mdcmd/vanity-id/README.mdcmd/webclient/wasm_exec.jsdocs/architecture.mddocs/development.mdportal/core/cryptoops/README.md
🧰 Additional context used
🪛 LanguageTool
AGENTS.md
[uncategorized] ~125-~125: The official name of this software platform is spelled with a capital “H”.
Context: ...runs test -> lint -> security -> build (.github/workflows/ci.yml). Verbalized samplin...
(GITHUB)
🔍 Remote MCP
Perfect! I now have sufficient context. Let me compile the findings:
Additional Context for PR #119 Review
Project Architecture and Scope
The Portal project consists of three main components: a relay-server, a webclient, and an SDK, designed to expose local applications publicly through a secure relay layer. The project is organized into cmd/, sdk/, portal/, and docs/ directories, with the Makefile orchestrating build processes for WASM client, React frontend, and Go relay server.
Current CI/CD and Testing Practices
The CI workflow currently consists of two main jobs: test and build, where the build job depends on the test job, ensuring code is only built if tests pass. The test job runs Go tests with race detection and generates coverage profiles using go test -v -race -coverprofile=coverage.out ./....
Important finding: Although a .golangci.yml file exists, the CI workflow does not currently include a linting step. This PR addresses that gap by adding explicit lint and security jobs.
Pre-commit hooks are already configured in pre-commit-config.yaml to enforce code quality locally, including golangci-lint, golangci-lint-full, golangci-lint-fmt, and golangci-lint-config-verify hooks.
Development Guidelines and Code Standards
The docs/development.md file outlines five key principles: (1) Usability Invariance—new features must not alter existing usability; (2) Prior Agreement for Changes—features impacting usability require written rationale and team approval; (3) Testing and Quality Assurance—all new features must be fully tested in personal branches before merging; (4) Project Philosophy and Scope—Portal functions as a relay layer for end-to-end encryption; and (5) Principles for Resolving Disagreements—through constructive consensus.
Error Handling and Security Expectations
The project emphasizes robust error handling with centralized mechanisms for displaying errors to users, retry logic for recoverable issues, and specific error types for different failure scenarios. Security practices include end-to-end encryption for WebSocket traffic, cryptographic key generation and validation, timestamp validation with a maxTimestampSkew of 30 seconds to prevent replay attacks, and memory wiping for sensitive data.
Linting Configuration Alignment
The golangci.yml configuration enables various linters such as govet, errcheck, staticcheck, and others for performance, concurrency, style, and cognitive load analysis. The PR's expansion of linting rules (Tier 1-4 structure) aligns with these security and quality expectations, particularly the emphasis on errcheck for unchecked errors and noctx for preventing goroutine leaks.
Key PR Change Impact Assessment
- The addition of
go vet,lint(golangci-lint), andsecurity(govulncheck) jobs strengthens CI/CD without changing runtime behavior, consistent with the "no usability impact" principle - The Makefile additions (
fmt,vet,lint,test,vuln,tidy) provide developer convenience targets aligned with existing pre-commit configuration - The comprehensive
.golangci.ymlexpansion operationalizes existing linting rules that were only applied locally via pre-commit hooks - The
AGENTS.mddocumentation was added to assist automated tools in understanding the repository—this aligns with the project's need for clear developer guidance
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (17)
cmd/vanity-id/README.md (1)
117-117: LGTM—standard EOF newline added.This formatting change aligns with POSIX standards and the PR's linting enforcement objectives.
CLAUDE.md (1)
1-1: Clarify the purpose of this redirect file.Why create a separate file that just points to
AGENTS.md? Consider whether this indirection is necessary or if developers should referenceAGENTS.mddirectly.[scratchpad_end] -->
buf.gen.yaml (1)
12-12: LGTM!Correct YAML list syntax fix - removes the erroneous extra dash that was improperly nesting the list item.
cmd/portal-tunnel/README.md (1)
23-23: LGTM!Trailing whitespace cleanup aligns with the PR's formatting improvements.
docs/architecture.md (1)
1-99: LGTM!Adding the top-level heading improves document structure, and the code fence formatting normalization is consistent with the PR's documentation polish goals.
.pre-commit-config.yaml (1)
24-30: LGTM!Adding the
golangci-lint-config-verifyhook is a good safeguard to catch configuration errors before commit. This aligns well with the PR's goal of strengthening CI/tooling.Note: The AI summary mentions this is a "temporary addition and subsequent removal," but it's present in the code. If this was truly temporary, ensure it serves its intended purpose.
portal/core/cryptoops/README.md (1)
143-587: LGTM!Whitespace normalization in code blocks. Pure formatting improvements with no semantic changes.
AGENTS.md (3)
124-125: Capitalize "GitHub" properly.Per standard naming, "GitHub" should have a capital H.
Proposed fix
CI/CD: -- CI runs test -> lint -> security -> build (`.github/workflows/ci.yml`). +- CI runs test -> lint -> security -> build (`.GitHub/workflows/ci.yml` or keep path lowercase but capitalize the word in prose).Actually, the file path should stay lowercase, but if referencing GitHub as a product name in prose, capitalize it. Here the path is fine.
1-128: Solid agent guidance document.This provides clear, actionable instructions for automated agents—commands, architecture overview, coding standards, and CI expectations are well-organized. The tiered linter explanation and explicit security/performance guidance are particularly useful.
76-76: Go version 1.25.3 is correctly documented. Thego.modfile confirms this specification.docs/development.md (1)
1-45: Documentation cleanup looks good.Formatting improvements and the new "Principles for Resolving Disagreements" section are straightforward additions. No issues.
Makefile (1)
19-34: New development workflow targets are well-structured.
vet,lint,test,vuln, andtidytargets align with CI jobs and AGENTS.md documentation. Good consistency..golangci.yml (2)
174-177: Verify G104 and G304 exclusions are intentional.
- G104 (unhandled errors): Already covered by
errcheck, so this may be acceptable to avoid duplicate noise.- G304 (file path from tainted input): Excluding this reduces protection against path traversal. Confirm this is acceptable for the relay server's threat model.
7-13: Well-organized tiered linter configuration.The four-tier structure (Bugs/Correctness → Code Quality → Concurrency/Safety → Performance/Modernization) is logical and comprehensive. Formatter configuration with local prefixes for import grouping is a nice touch.
Also applies to: 15-66
.github/workflows/ci.yml (3)
91-93: Build job runs in parallel with test—intentional?Removing
needs: testmeans build starts immediately, potentially wasting CI minutes if tests fail. If this is intentional for faster feedback on build issues, that's fine. Otherwise, consider restoring the dependency or using a workflow-levelconcurrencysetting.
41-43: Good additions: vet step and security job.Adding
go vetto the test job and a dedicatedgovulnchecksecurity job strengthens the CI pipeline without adding complexity.Also applies to: 69-89
47-67: Lint job does not need protobuf generation—.pb.go files are committed.The
.pb.gofiles (portal/core/proto/rdsec/rdsec.pb.go,rdsec_vtproto.pb.go,rdverb.pb.go,rdverb_vtproto.pb.go) are already committed to the repository and not gitignored. The lint job will analyze the committed files without needing to regenerate them. No action required.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Updated local-prefixes for goimports and removed unused context functions from the linter settings.
metaphorics
changed the title
metaphorics
requested review from
changchanghwang,
rabbitson87,
snowmerak,
splkm97 and
yoonhyunwoo
1 participant
Note
Introduces CI/CD and developer tooling across the repo.
test(incl.go vetand protobuf generation),lint(golangci-lint),security(govulncheck), andbuild(multi-component build + Docker image).golangci.ymlwith expanded linters, rules, and exclusions; pre-commit hooks for config verification, lint, and fmtfmt,vet,lint,test,vuln,tidy,build-protoc, and WASM/frontend/tunnel/server build targets (incl. brotli + wasm-opt)AGENTS.md,docs/architecture.md,docs/development.md, and READMEs; minor housekeeping (buf.gen.yaml, go.sum, assets)Written by Cursor Bugbot for commit 3606ba6. This will update automatically on new commits. Configure here.