chore(scaffold): Go project skeleton with lint, hooks, and CI#79
chore(scaffold): Go project skeleton with lint, hooks, and CI#79ameeracadam wants to merge 14 commits into
Conversation
|
I'm thinking if we want to shift all the AWS related stuff out to the respective retrieval and LLM orchestration stories (understand the issue might have added but we can descope and shift the concerns) since for the scaffold setup might be premature to do this right now |
|
can we have a CLAUDE.md file as well? easier for referencing when we vibe certain things |
|
Removed the AWS SDK here, can add back the different SDKs in the various tasks
|
| # | ||
| set -euo pipefail | ||
|
|
||
| BRANCH=$(git rev-parse --abbrev-ref HEAD) |
There was a problem hiding this comment.
[Medium] The pre-push gate checks the current branch, not the push target, so git push origin HEAD:main from a feature branch is not blocked
BRANCH=$(git rev-parse --abbrev-ref HEAD) # ← checks where you ARE, not where you're pushing TO
if [[ "$BRANCH" == "main" || "$BRANCH" == "master" ]]; then
echo "ERROR: Direct push to '$BRANCH' is not allowed."Problem: {push_remote} and {push_ref} are not Lefthook template placeholders (Lefthook defines {files}, {staged_files}, {push_files}, {all_files}, {cmd}), so the script receives the literal strings {push_remote} and {push_ref} as arguments and ignores them. Git's pre-push contract delivers the refs being pushed on stdin (<local-ref> <local-sha> <remote-ref> <remote-sha> per line), not as arguments. Two consequences: git push origin HEAD:main from a feature branch reaches main unblocked, and git push origin somebranch while checked out on main is blocked even though nothing is pushed to main.
Suggestion: read the remote ref from stdin, and forward stdin in lefthook.yml:
#!/usr/bin/env bash
set -euo pipefail
while read -r _local_ref _local_sha remote_ref _remote_sha; do
case "$remote_ref" in
refs/heads/main|refs/heads/master)
echo "ERROR: Direct push to '${remote_ref#refs/heads/}' is not allowed." >&2
echo "Please create a feature branch and open a pull request." >&2
exit 1
;;
esac
donecheck-push-target:
runner: bash
use_stdin: true
run: scripts/hooks/check-push-target.sh🤖 aif-code-review · claude-fable-5
| commands: | ||
| golangci-lint: | ||
| glob: "*.go" | ||
| run: ./bin/golangci-lint run --new-from-rev=HEAD {staged_files} |
There was a problem hiding this comment.
[Medium] Passing staged file paths to golangci-lint fails once a commit touches more than one package
golangci-lint:
glob: "*.go"
run: ./bin/golangci-lint run --new-from-rev=HEAD {staged_files} # ←Problem: golangci-lint inherits go build's constraint that named file arguments must belong to a single directory/package. The scaffold has one package today, but #64/#73 add more; any commit staging .go files from two packages will make the pre-commit gate fail with a typecheck error unrelated to the code being committed.
Suggestion: keep the glob as the trigger but lint packages, letting --new-from-rev scope the findings:
run: ./bin/golangci-lint run --new-from-rev=HEAD ./...Tradeoff: lints the whole module per commit, but at this repo's size that is fast, and --new-from-rev=HEAD still limits findings to changed code.
🤖 aif-code-review · claude-fable-5
|
|
||
| func Load() (*Config, error) { | ||
| cfg := &Config{ | ||
| AWSRegion: getEnv("AWS_REGION", "ap-southeast-1"), |
There was a problem hiding this comment.
[Low] AWS_REGION config is leftover from the descoped AWS setup, and the validation guarding it is unreachable
cfg := &Config{
AWSRegion: getEnv("AWS_REGION", "ap-southeast-1"), // ← AWS scope was descoped from this PR
LogLevel: getEnv("LOG_LEVEL", "info"),
}
...
func (c *Config) validate() error {
if c.AWSRegion == "" { // ← always false: getEnv falls back to "ap-southeast-1"
return fmt.Errorf("AWS_REGION is required")
}
return nil
}Problem: the AWS SDKs were removed from this PR and allocated to the downstream stories (see #79 (comment)), but AWS_REGION stayed behind in config.go, .env.example, and the README with nothing consuming it. It should travel with the rest of the AWS scope and arrive alongside the SDK it configures (#64/#73/#23). The check guarding it is also dead code: getEnv substitutes the fallback whenever the variable is empty or unset, so AWSRegion is never "" and Load() cannot fail. Separately, .env.example and the README document LOG_LEVEL as debug|info|warn|error, but LOG_LEVEL=banana loads without error.
Suggestion: drop AWSRegion from Config, .env.example, and the README env table (it comes back with the per-story SDK wiring), and give validate() a check that can actually fail:
type Config struct {
LogLevel string
}
func (c *Config) validate() error {
switch c.LogLevel {
case "debug", "info", "warn", "error":
return nil
default:
return fmt.Errorf("LOG_LEVEL must be one of debug, info, warn, error; got %q", c.LogLevel)
}
}🤖 aif-code-review · claude-fable-5
| make hooks | ||
|
|
||
| # 4. Copy environment template and fill in values | ||
| cp .env.example .env |
There was a problem hiding this comment.
[Low] Setup step 4 (cp .env.example .env) has no effect: nothing loads .env
Problem: config.Load reads the process environment only, and go.mod has zero dependencies, so there is no dotenv/direnv integration. An engineer following setup fills in .env, then make check and every tool silently ignore it and fall back to defaults.
Suggestion: either document how to export it (set -a && source .env && set +a, or direnv), or defer the step until a loader lands with the server (#23).
🤖 aif-code-review · claude-fable-5
|
|
||
| ### Prerequisites | ||
|
|
||
| - **Go 1.24+** — the exact version is pinned in `go.mod` |
There was a problem hiding this comment.
[Low] Prerequisite says "Go 1.24+" but go.mod pins go 1.26.3
Problem: CLAUDE.md says Go 1.26+, go.mod says 1.26.3, the README says 1.24+. A Go 1.24 install only works because toolchains >= 1.21 auto-download the pinned version, which the doc does not mention; the three stated requirements contradict each other.
Suggestion: state one truth: "Go 1.26.3 (pinned in go.mod); any toolchain >= 1.21 will auto-download it."
🤖 aif-code-review · claude-fable-5
| ## tools: Install pinned dev tools into ./bin | ||
| tools: $(GOLANGCI_LINT) $(LEFTHOOK) $(GITLEAKS) | ||
|
|
||
| $(GOLANGCI_LINT): |
There was a problem hiding this comment.
[Low] Tool targets are file targets, so bumping a pinned version never reinstalls the tool
$(GOLANGCI_LINT):
@mkdir -p $(BIN)
GOBIN=$(BIN) go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION)Problem: the target is satisfied by the existing ./bin/golangci-lint file. After a version bump in the Makefile, every existing checkout silently keeps the old binary while CI (which installs fresh) uses the new one, producing "passes locally, fails in CI" lint drift.
Suggestion: version-stamp the artifact so a bump invalidates it:
GOLANGCI_LINT := $(BIN)/golangci-lint-$(GOLANGCI_LINT_VERSION)
$(GOLANGCI_LINT):
@mkdir -p $(BIN)
GOBIN=$(BIN) go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION)
mv $(BIN)/golangci-lint $(GOLANGCI_LINT)(Same pattern for lefthook and gitleaks.)
🤖 aif-code-review · claude-fable-5
Code Review Summary
What Looks Good
🤖 aif-code-review · claude-fable-5 |
Summary
github.com/String-sg/tw-context-intelligence) with pinned toolchain, enforced git hooks, CI workflow, and conventional project layoutmake tools→make hooks→make checkruns the full tidy/build/lint/test pipeline locally and in CIWhat's included
go 1.26.3, AWS SDK for Go v2 deps pinned viatools.go.golangci.yamlv2 with gofmt, goimports,interface{}→anyrewrite.github/workflows/ci.yml— tidy, build, lint, test on PR to maininternal/config/with env loading, validation, and smoke testsPatterns followed
.golangci.yamlversion: "2"with formatters[gofmt, goimports],interface{}→anyrewrite, default linter setlefthook.yml; gitleaks with.gitleaks.toml; no package.json/Husky — Lefthook pinned in Makefile and wired viamake hooks→lefthook installtools,tidy,build,lint,test,hooksgodirective ingo.modmake tools; mandatory pre-commit gateHard constraints addressed
net/http, gin, echo, chi, or any web framework in the modulego.modcontains onlyaws-sdk-go-v2modules for AI/ML./bin.envgitignored; only.env.examplecommitted; gitleaks pre-commit gate mandatorygo 1.26.3directive ingo.modOut of scope
Test plan
go build ./...exits 0go mod tidyproduces no diffmake lintexits 0 (0 issues)go test ./... -racepassesmake hooksinstalls pre-commit + pre-push without "Can't find lefthook" warningAdditional test scenarios
Test runner is wired correctly
Given the configured Go test setup, when the suite runs with only the scaffold present,
internal/config/config_test.gois discovered, executed, and reported as passing:Lint gate catches a violation
Given golangci-lint is configured, when an unformatted/vet-flagged change is introduced,
make lintexits non-zero:Pre-commit secrets gate fires
Given gitleaks protect --staged runs in pre-commit, when a staged file contains a detectable secret (GitHub token), the gate exits non-zero: