fix: address review feedback for AGENTS.md and tooling

Avoid double race suite, stdlib-only import checks, recursive package
scan, cmd/ coverage filter, hygiene log.Print, design path, gofmt CI.
This commit is contained in:
Reese Norris
2026-07-12 19:58:51 -04:00
parent 514562055a
commit 2ea1fd587c
7 changed files with 262 additions and 124 deletions

View File

@@ -12,28 +12,46 @@ jobs:
with:
go-version-file: go.mod
# Race gate (single race suite — coverage step does not re-run with -race).
- name: Test
run: go test -race ./...
# Soft coverage without -race to avoid doubling race-test wall time.
- name: Coverage report (soft)
if: always()
run: |
chmod +x scripts/coverage.sh
./scripts/coverage.sh
# Soft only: floors are phased (AGENTS.md §8). Failures here are test failures only.
# Soft only: floors are phased (AGENTS.md §8). Fails only if tests fail.
- name: Hygiene (panic/reflect/fmt.Print)
- name: gofmt
if: always()
run: |
unformatted="$(gofmt -l .)"
if [ -n "$unformatted" ]; then
echo "The following files need gofmt:"
echo "$unformatted"
exit 1
fi
echo "gofmt OK"
- name: Hygiene (panic/reflect/fmt.Print/log.Print)
if: always()
run: |
chmod +x scripts/check-hygiene.sh
./scripts/check-hygiene.sh
- name: Import graph (forbidden edges)
if: always()
run: |
chmod +x scripts/check-import-graph.sh
./scripts/check-import-graph.sh
# Advisory until a dedicated cleanup PR lands; then drop continue-on-error.
- name: golangci-lint
if: always()
uses: golangci/golangci-lint-action@v6
continue-on-error: true # advisory until package split; hard fail later
continue-on-error: true
with:
version: v1.64.5
args: --timeout=5m

View File

@@ -1,6 +1,12 @@
# golangci-lint configuration for openfsd (Go 1.24+).
# Start with a passable baseline on current code; tighten later.
# See AGENTS.md for operational criteria.
#
# CI runs this with continue-on-error: true (advisory) because the legacy
# fsd/db/web tree still has dozens of findings. Before flipping the hard gate:
# 1) dedicated cleanup PR (errcheck / revive / staticcheck / ineffassign)
# 2) optionally exclude or fix SA1019 (deprecated rand.Seed) in tests
# 3) then drop continue-on-error in .github/workflows/ci.yml
run:
timeout: 5m

View File

@@ -4,7 +4,10 @@ This file is **operational and checkable**. Agents and humans must follow it whe
**Primary track:** Track A (core FSD) is primary. Full completion is through **PR17**. **PR11 is an e2e checkpoint only** — not permission to stop FSD finish work (db move, `cmd/openfsd`, stress, coverage floor) or Track B (web PE).
Design reference: agentic refactor design (package layout, phased coverage, PR sequence).
**In-repo contract:** this file is the operational source of truth for agents working from the git tree.
**Design reference (may be local-only / not in this repo):**
`~/.grok/docs/designs/openfsd-agentic-refactor-3bd159cb.md` — package layout, phased coverage, PR sequence. If that path is unavailable, follow this `AGENTS.md` alone.
---
@@ -14,9 +17,9 @@ Target layout (packages may not all exist yet; do not invent imports that violat
| Package | Owns | Notes |
|---------|------|-------|
| `pkg/protocol` | Pure wire format (parse/serialize/validate) | No I/O; stdlib only |
| `pkg/protocol` | Pure wire format (parse/serialize/validate) | No I/O; **stdlib only** (no third-party, no module-internal) |
| `pkg/fsdclient` | Public mock/real FSD client | Imports `protocol` only (+ stdlib) |
| `internal/geo` | Pure haversine / bounding box | Stdlib only |
| `internal/geo` | Pure haversine / bounding box | **stdlib only** (no third-party, no module-internal) |
| `internal/auth` | JWT + VATSIM auth state | No TCP |
| `internal/session` | Per-connection state + send worker | Does not import postoffice/server |
| `internal/postoffice` | Registry (map/tree of participants) | Depends on session ports, not server |
@@ -55,14 +58,16 @@ Enforce with `scripts/check-import-graph.sh` (and later `TestImportGraph` when p
| From | Must not import |
|------|-----------------|
| `pkg/protocol` | Anything in this module except stdlib |
| `pkg/protocol` | **Any non-stdlib import** (no third-party; no module packages) |
| `pkg/fsdclient` | `internal/*` |
| `internal/session` | `postoffice`, `server`, `web`, `metar` |
| `internal/geo` | Anything openfsd except stdlib |
| `internal/geo` | **Any non-stdlib import** (no third-party; no module packages) |
| `internal/web` | `server`, `session`, `postoffice`, `metar` |
| `internal/db` | `server`, `session`, `web`, `fsdclient` |
| `internal/auth` | `server`, `session`, `web` |
Enforcement (`scripts/check-import-graph.sh`): walks **every package** under each root (`…/...`), checks **direct** imports. Stdlib heuristic: first path element contains no `.` (e.g. `fmt`, `net/http`). Third-party is **never** allowed in `pkg/protocol` or `internal/geo`.
**Cycle prevention:** `session` never imports `postoffice`. Postoffice depends on a narrow participant/send port. Shared errors like `ErrCallsignInUse` live next to the registry, not in `pkg/protocol`.
---
@@ -169,12 +174,19 @@ Checklist:
**Measurement:**
```bash
# Race gate (CI Test step) — run separately:
go test -race ./...
# Soft coverage (no -race; avoids double race suite in CI):
./scripts/coverage.sh
# equivalent:
go test -race -coverprofile=cover.out ./...
go test -coverprofile=cover.out ./...
go tool cover -func=cover.out
# script also prints an in-scope total with cmd/* filtered out
```
`cmd/*` is soft-excluded from floor measurement. Hard package floors (PR2+) will be enforced later via env/milestone gates; this PR only reports.
---
## 9. How to run tests
@@ -194,7 +206,7 @@ go test -race ./pkg/... ./internal/...
./scripts/coverage.sh
```
Exits non-zero only if tests fail, not if coverage is below a floor (until floors are enforced in CI).
Does **not** pass `-race` (race is a separate gate). Exits non-zero only if tests fail, not if coverage is below a floor (until floors are enforced in CI). Prints full-tree total plus an in-scope summary excluding `cmd/*`.
### Hygiene / import graph
@@ -203,12 +215,20 @@ Exits non-zero only if tests fail, not if coverage is below a floor (until floor
./scripts/check-import-graph.sh
```
### Format
```bash
gofmt -l .
# CI fails if any file is listed
```
### Lint
```bash
golangci-lint run
```
Lint is **advisory** in CI (`continue-on-error: true`) until a dedicated cleanup PR removes legacy findings and hard-fails the gate.
### E2E (after PR11)
```bash
@@ -251,16 +271,19 @@ Before adding a new frontend framework, client router, global store, or hydratio
|------|-------|-------------|
| No `panic(` | non-test `.go` under `pkg/`, `internal/` | `scripts/check-hygiene.sh` |
| No `reflect` | `pkg/protocol` | `scripts/check-hygiene.sh` |
| No `fmt.Print` | `pkg/`, `internal/` (non-test) | `scripts/check-hygiene.sh` |
| No `fmt.Print` / `log.Print` / `log.Fatal` / `log.Panic` | `pkg/`, `internal/` (non-test) | `scripts/check-hygiene.sh` |
| Forbidden imports | See §2 | `scripts/check-import-graph.sh` |
| gofmt | all `.go` | CI `gofmt -l .` |
Legacy `fsd/` will move into `internal/` / `pkg/`. Hygiene greps target the **target** trees so early PRs stay green while the split lands; do not add new panics/prints in new packages.
**False positives:** greps skip pure `//` and `*` comment lines but may still match string literals. Prefer restructuring strings over weakening the script.
---
## Logging and errors
- Libraries: `log/slog` only (no `fmt.Print` / `log.Print` in `pkg/` / `internal/`)
- Libraries: `log/slog` only (no `fmt.Print` / `log.Print` / `log.Fatal` / `log.Panic` in `pkg/` / `internal/`)
- Wrap errors with `%w`; sentinels + `errors.Is` / `errors.As`
- Panic forbidden in `pkg/*` and `internal/*` (tests exempt)
- Reflection forbidden in `pkg/protocol`

View File

@@ -3,6 +3,11 @@
# Targets target-layout trees pkg/ and internal/ only.
# No-op success if those directories are missing or empty of .go files.
# Legacy fsd/ will move into these trees; do not add panics/prints in new packages.
#
# Limitation: greps can still match string literals (false positives).
# Pure // and * block-comment lines are skipped. Prefer fixing real call sites;
# if a string false positive is unavoidable, restructure the string — do not
# weaken patterns casually.
set -euo pipefail
ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
@@ -16,59 +21,78 @@ list_non_test_go() {
if [[ ! -d "$dir" ]]; then
return 0
fi
# find may return nothing; that is OK
find "$dir" -name '*.go' ! -name '*_test.go' -print 2>/dev/null || true
}
echo "==> Hygiene: panic( in non-test Go under pkg/ and internal/"
panic_hits=""
while IFS= read -r f; do
[[ -z "$f" ]] && continue
if grep -nE '\bpanic\s*\(' "$f" 2>/dev/null; then
panic_hits+="$f"$'\n'
failed=1
# Read file paths from stdin; grep pattern; skip pure // and * comment lines.
# Prints hits; returns 0 if clean, 1 if any match (also sets failed=1).
# Note: pipelines must tolerate zero matches under set -o pipefail.
scan_files() {
local pattern="$1"
local any=0
local f hits
while IFS= read -r f; do
[[ -z "$f" || ! -f "$f" ]] && continue
hits="$(grep -nE "$pattern" "$f" 2>/dev/null | grep -vE '^[0-9]+:[[:space:]]*//' | grep -vE '^[0-9]+:[[:space:]]*\*' || true)"
if [[ -n "$hits" ]]; then
while IFS= read -r line; do
[[ -z "$line" ]] && continue
echo "${f}:${line}"
done <<<"$hits"
any=1
failed=1
fi
done
if [[ "$any" -ne 0 ]]; then
return 1
fi
done < <(list_non_test_go pkg; list_non_test_go internal)
return 0
}
if [[ -z "${panic_hits}" ]]; then
files="$( { list_non_test_go pkg; list_non_test_go internal; } | grep -v '^$' || true)"
echo "==> Hygiene: panic( in non-test Go under pkg/ and internal/"
if [[ -z "$files" ]]; then
echo " OK (no hits, or dirs absent)"
elif printf '%s\n' "$files" | scan_files '\bpanic\s*\('; then
echo " OK"
fi
echo "==> Hygiene: reflect usage in pkg/protocol (when present)"
if [[ -d pkg/protocol ]]; then
reflect_hit=0
while IFS= read -r f; do
[[ -z "$f" ]] && continue
# Match import "reflect" or reflect. usage
if grep -nE '"reflect"|\breflect\.' "$f" 2>/dev/null; then
reflect_hit=1
failed=1
fi
done < <(list_non_test_go pkg/protocol)
if [[ "$reflect_hit" -eq 0 ]]; then
if [[ ! -d pkg/protocol ]]; then
echo " skip (pkg/protocol not present yet)"
else
proto_files="$(list_non_test_go pkg/protocol | grep -v '^$' || true)"
if [[ -z "$proto_files" ]]; then
echo " OK"
elif printf '%s\n' "$proto_files" | scan_files '"reflect"|\breflect\.'; then
echo " OK"
fi
else
echo " skip (pkg/protocol not present yet)"
fi
echo "==> Hygiene: fmt.Print* in non-test Go under pkg/ and internal/"
print_hits=0
while IFS= read -r f; do
[[ -z "$f" ]] && continue
if grep -nE '\bfmt\.Print(f|ln)?\s*\(' "$f" 2>/dev/null; then
print_hits=1
failed=1
fi
done < <(list_non_test_go pkg; list_non_test_go internal)
if [[ "$print_hits" -eq 0 ]]; then
echo "==> Hygiene: fmt.Print* / log.Print* / log.Fatal* / log.Panic* under pkg/ and internal/"
if [[ -z "$files" ]]; then
echo " OK (no hits, or dirs absent)"
else
print_clean=1
if ! printf '%s\n' "$files" | scan_files '\bfmt\.Print(f|ln)?\s*\('; then
print_clean=0
fi
if ! printf '%s\n' "$files" | scan_files '\blog\.Print(f|ln)?\s*\('; then
print_clean=0
fi
if ! printf '%s\n' "$files" | scan_files '\blog\.(Fatal|Fatalf|Fatalln|Panic|Panicf|Panicln)\s*\('; then
print_clean=0
fi
if [[ "$print_clean" -eq 1 ]]; then
echo " OK"
fi
fi
if [[ "$failed" -ne 0 ]]; then
echo
echo "Hygiene check FAILED. See AGENTS.md (no panic/reflect/fmt.Print in library code)."
echo "Hygiene check FAILED. See AGENTS.md (no panic/reflect/fmt.Print/log.Print in library code)."
echo "Note: matches in string literals can be false positives; restructure if needed."
exit 1
fi

View File

@@ -2,6 +2,10 @@
# Forbidden import-edge checks for openfsd (AGENTS.md §2).
# Documents and enforces edges when packages exist; no-op success if absent.
# A Go TestImportGraph may replace or supplement this once packages land.
#
# Checks direct imports of each package under a pattern (./pkg/... style).
# Pure packages (pkg/protocol, internal/geo) must be stdlib-only:
# stdlib import paths have no '.' in the first path element (e.g. fmt, net/http).
set -euo pipefail
ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
@@ -10,117 +14,126 @@ cd "$ROOT"
MODULE="$(go list -m -f '{{.Path}}' 2>/dev/null || echo "github.com/renorris/openfsd")"
failed=0
# Return 0 if package path exists in the module build list.
pkg_exists() {
local import_path="$1"
go list "$import_path" >/dev/null 2>&1
# True if at least one package matches the pattern (e.g. $MODULE/pkg/protocol/...).
pkgs_exist() {
local pattern="$1"
local list
list="$(go list "$pattern" 2>/dev/null || true)"
[[ -n "$list" ]]
}
# Fail if importer's transitive/direct imports include any forbidden path prefix.
# Uses go list -f '{{.Imports}}' on the package.
# Stdlib heuristic: first path element contains no '.' (fmt, encoding/json, net/http).
# Rejects module paths (github.com/...), domain-qualified modules, and local module imports.
is_stdlib_import() {
local imp="$1"
local first="${imp%%/*}"
[[ "$first" != *.* ]]
}
# For each package matching pattern, fail if any direct import matches a forbidden prefix.
# "from_label" is only for messages; pattern is a go list pattern (may end in /...).
check_no_imports() {
local from="$1"
shift
local from_label="$1"
local pattern="$2"
shift 2
local forbidden=("$@")
if ! pkg_exists "$from"; then
echo " skip $from (not present yet)"
if ! pkgs_exist "$pattern"; then
echo " skip $from_label (not present yet)"
return 0
fi
local imports
imports="$(go list -f '{{range .Imports}}{{.}}{{"\n"}}{{end}}' "$from" 2>/dev/null || true)"
# Also check test imports lightly via deps of the package only (direct Imports).
local hit=0
local imp
for imp in $imports; do
for bad in "${forbidden[@]}"; do
case "$imp" in
"$bad"|"$bad"/*)
echo " FAIL: $from imports $imp (forbidden: $bad)"
hit=1
failed=1
;;
esac
local pkg hit=0
while IFS= read -r pkg; do
[[ -z "$pkg" ]] && continue
local imports
imports="$(go list -f '{{range .Imports}}{{.}}{{"\n"}}{{end}}' "$pkg" 2>/dev/null || true)"
local imp
for imp in $imports; do
for bad in "${forbidden[@]}"; do
case "$imp" in
"$bad"|"$bad"/*)
echo " FAIL: $pkg imports $imp (forbidden: $bad)"
hit=1
failed=1
;;
esac
done
done
done
done < <(go list "$pattern" 2>/dev/null || true)
if [[ "$hit" -eq 0 ]]; then
echo " OK $from"
echo " OK $from_label (direct imports under $pattern)"
fi
}
# Fail if any package under pattern has a non-stdlib direct import.
check_stdlib_only() {
local from_label="$1"
local pattern="$2"
if ! pkgs_exist "$pattern"; then
echo " skip $from_label (not present yet)"
return 0
fi
local pkg hit=0
while IFS= read -r pkg; do
[[ -z "$pkg" ]] && continue
local imports
imports="$(go list -f '{{range .Imports}}{{.}}{{"\n"}}{{end}}' "$pkg" 2>/dev/null || true)"
local imp
for imp in $imports; do
if ! is_stdlib_import "$imp"; then
echo " FAIL: $pkg imports $imp (must be stdlib only; third-party and module imports forbidden)"
hit=1
failed=1
fi
done
done < <(go list "$pattern" 2>/dev/null || true)
if [[ "$hit" -eq 0 ]]; then
echo " OK $from_label (stdlib-only under $pattern)"
fi
}
echo "==> Import graph: forbidden edges (AGENTS.md §2)"
echo " module: $MODULE"
echo " note: checks direct imports of every package matched by each pattern"
# pkg/protocol — anything in module except stdlib
if pkg_exists "${MODULE}/pkg/protocol"; then
echo " checking pkg/protocol is free of module-internal imports..."
proto_imports="$(go list -f '{{range .Imports}}{{.}}{{"\n"}}{{end}}' "${MODULE}/pkg/protocol" 2>/dev/null || true)"
hit=0
for imp in $proto_imports; do
case "$imp" in
"${MODULE}"|"${MODULE}"/*)
echo " FAIL: pkg/protocol imports $imp (must be stdlib only within module)"
hit=1
failed=1
;;
esac
done
if [[ "$hit" -eq 0 ]]; then
echo " OK ${MODULE}/pkg/protocol"
fi
else
echo " skip ${MODULE}/pkg/protocol (not present yet)"
fi
# pkg/protocol — stdlib only (no third-party, no module-internal)
check_stdlib_only "pkg/protocol" "${MODULE}/pkg/protocol/..."
# pkg/fsdclient — must not import internal/*
check_no_imports "${MODULE}/pkg/fsdclient" \
check_no_imports "pkg/fsdclient" "${MODULE}/pkg/fsdclient/..." \
"${MODULE}/internal"
# internal/session — must not import postoffice, server, web, metar
check_no_imports "${MODULE}/internal/session" \
check_no_imports "internal/session" "${MODULE}/internal/session/..." \
"${MODULE}/internal/postoffice" \
"${MODULE}/internal/server" \
"${MODULE}/internal/web" \
"${MODULE}/internal/metar"
# internal/geo — anything openfsd except stdlib
if pkg_exists "${MODULE}/internal/geo"; then
echo " checking internal/geo is free of module-internal imports..."
geo_imports="$(go list -f '{{range .Imports}}{{.}}{{"\n"}}{{end}}' "${MODULE}/internal/geo" 2>/dev/null || true)"
hit=0
for imp in $geo_imports; do
case "$imp" in
"${MODULE}"|"${MODULE}"/*)
echo " FAIL: internal/geo imports $imp (must be stdlib only within module)"
hit=1
failed=1
;;
esac
done
if [[ "$hit" -eq 0 ]]; then
echo " OK ${MODULE}/internal/geo"
fi
else
echo " skip ${MODULE}/internal/geo (not present yet)"
fi
# internal/geo — stdlib only
check_stdlib_only "internal/geo" "${MODULE}/internal/geo/..."
# internal/web — must not import server, session, postoffice, metar
check_no_imports "${MODULE}/internal/web" \
check_no_imports "internal/web" "${MODULE}/internal/web/..." \
"${MODULE}/internal/server" \
"${MODULE}/internal/session" \
"${MODULE}/internal/postoffice" \
"${MODULE}/internal/metar"
# internal/db — must not import server, session, web, fsdclient
check_no_imports "${MODULE}/internal/db" \
check_no_imports "internal/db" "${MODULE}/internal/db/..." \
"${MODULE}/internal/server" \
"${MODULE}/internal/session" \
"${MODULE}/internal/web" \
"${MODULE}/pkg/fsdclient"
# internal/auth — must not import server, session, web
check_no_imports "${MODULE}/internal/auth" \
check_no_imports "internal/auth" "${MODULE}/internal/auth/..." \
"${MODULE}/internal/server" \
"${MODULE}/internal/session" \
"${MODULE}/internal/web"

View File

@@ -1,20 +1,74 @@
#!/usr/bin/env bash
# Soft coverage report for openfsd.
# Fails only if tests fail — never fails on coverage floor (floors are phased; see AGENTS.md).
# Fails only if tests fail — never fails on coverage floor (floors are phased; see AGENTS.md §8).
#
# Does NOT use -race: CI already runs `go test -race ./...` separately.
# Locally run `go test -race ./...` for the race gate; use this for cover profile only.
set -euo pipefail
ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
cd "$ROOT"
OUT="${COVER_OUT:-cover.out}"
MODULE="$(go list -m -f '{{.Path}}' 2>/dev/null || echo "github.com/renorris/openfsd")"
echo "==> go test -race -coverprofile=${OUT} ./..."
go test -race -coverprofile="${OUT}" ./...
echo "==> go test -coverprofile=${OUT} ./..."
go test -coverprofile="${OUT}" ./...
echo
echo "==> go tool cover -func=${OUT}"
echo "==> go tool cover -func=${OUT} (full tree)"
go tool cover -func="${OUT}"
# Soft summary: overall total, and in-scope total excluding cmd/ (soft-excluded from floors).
echo
echo "Coverage report complete (soft — no floor enforced yet). See AGENTS.md §8."
echo "==> In-scope summary (excluding cmd/* from floor measurement)"
if [[ -f "${OUT}" ]]; then
# Total line from cover -func
total_line="$(go tool cover -func="${OUT}" | grep -E '^total:' || true)"
echo " full tree: ${total_line:-n/a}"
# Filter profile lines that belong to cmd/ packages, recompute func coverage if possible.
# cover.out mode:set lines look like: path:start.col,end.col num count
# Package paths in profile use module-relative or full paths depending on Go version.
filtered="$(mktemp)"
# Keep header (mode: ...) and all non-cmd body lines
if head -n1 "${OUT}" | grep -q '^mode:'; then
head -n1 "${OUT}" >"${filtered}"
# Drop lines whose file path is under /cmd/ or starts with cmd/
tail -n +2 "${OUT}" | grep -vE '(^|/)cmd/' >>"${filtered}" || true
in_scope_line="$(go tool cover -func="${filtered}" 2>/dev/null | grep -E '^total:' || true)"
echo " excluding cmd/*: ${in_scope_line:-n/a (no statements or empty)}"
rm -f "${filtered}"
else
echo " (unexpected coverprofile format; skipped filter)"
fi
# List packages that exist under pkg/ and internal/ for floor tracking later
echo " target packages present:"
found_target=0
for p in \
"${MODULE}/pkg/protocol" \
"${MODULE}/pkg/fsdclient" \
"${MODULE}/internal/geo" \
"${MODULE}/internal/auth" \
"${MODULE}/internal/postoffice" \
"${MODULE}/internal/session" \
"${MODULE}/internal/metar" \
"${MODULE}/internal/server" \
"${MODULE}/internal/db" \
"${MODULE}/internal/web"
do
if go list "$p" >/dev/null 2>&1; then
echo " - $p"
found_target=1
fi
done
if [[ "$found_target" -eq 0 ]]; then
echo " (none yet — legacy fsd/db/web; floors soft until package split)"
fi
fi
echo
echo "Coverage report complete (soft — no floor enforced yet; cmd/* soft-excluded from summary)."
echo "See AGENTS.md §8. Race detection: run go test -race ./... separately (CI Test step)."
exit 0

View File

@@ -69,6 +69,6 @@ func (s *Server) Run(ctx context.Context) (err error) {
}()
<-ctx.Done()
return
}