From d0d1dcef15c55f9726b4c9ceb834c9d02e125dc5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 15 Jun 2026 02:39:11 -0400 Subject: [PATCH] fix(client/go): correct tag-go-module dirty-tree guard; GOPRIVATE docs (Client.Go-028,029) --- clients/go/README.md | 7 ++- code-reviews/Client.Go/findings.md | 85 +++++++++++++++++++++++++++++- scripts/tag-go-module.ps1 | 12 +++-- 3 files changed, 96 insertions(+), 8 deletions(-) diff --git a/clients/go/README.md b/clients/go/README.md index b6ab95c..b8943a6 100644 --- a/clients/go/README.md +++ b/clients/go/README.md @@ -299,8 +299,11 @@ import "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/mxgateway" If your build environment cannot reach `gitea.dohertylan.com` directly, configure `GOPROXY` to point at an internal proxy that fronts the Gitea -repo, or use `GONOSUMCHECK` + `GOPRIVATE` to bypass the checksum database -for the internal module path. +repo, or set `GOPRIVATE=gitea.dohertylan.com/*` to fetch the module +straight from the VCS — this both bypasses the public module proxy and +disables checksum-database (`sum.golang.org`) verification for that path. +Add `GOINSECURE=gitea.dohertylan.com/*` if the host serves the module over +plain HTTP rather than HTTPS. ## Releasing a new version diff --git a/code-reviews/Client.Go/findings.md b/code-reviews/Client.Go/findings.md index e06c2d0..aa33cce 100644 --- a/code-reviews/Client.Go/findings.md +++ b/code-reviews/Client.Go/findings.md @@ -4,8 +4,8 @@ |---|---| | Module | `clients/go` | | Reviewer | Claude Code | -| Review date | 2026-05-24 | -| Commit reviewed | `42b0037` | +| Review date | 2026-06-15 | +| Commit reviewed | `410acc9` | | Status | Re-reviewed | | Open findings | 0 | @@ -83,6 +83,39 @@ that earlier commit. | 9 | Testing coverage | New issue: the five new bulk SDK methods and `Client.StreamAlarms` have no unit tests in `mxgateway/` (Client.Go-024). | | 10 | Documentation & comments | No issues found in this diff. README documents the new `StreamAlarms`/`AcknowledgeAlarm` SDK calls; `Session.ReadBulk` documents the cached-vs-snapshot semantics and `timeout=0` default; `WriteSecuredBulk` flags credential sensitivity. | +### 2026-06-15 re-review (commit 410acc9) + +Re-review pass at `410acc9`. The diff is larger than the brief suggested: +`82996aa` resolved Client.Go-022..027 (already closed). On top of that, +`fd2a0ac`/`4a19854`/`da3aa7b`/`92cc468`/`75610e3` added a `LazyBrowseNode` +lazy-hierarchy walker (`Browse`/`Expand`/`BrowseChildrenRaw`) over the new +`BrowseChildren` RPC and paginated `DiscoverHierarchy`; `c463b49`/`2eb8137`/ +`9bdb899` made the TLS path lenient-by-default (accept the gateway's +self-signed cert unless `RequireCertificateValidation` or `CACertFile` is set); +`6df373a` added the release docs + `scripts/tag-go-module.ps1`. `gofmt -l .`, +`go vet ./...`, `go build ./...`, and `go test ./...` are all clean at HEAD. + +Two new low/medium issues in the release-helper and install docs. The +lenient-TLS default is an intentional, documented project posture +(`docs/GatewayConfiguration.md` "clients are lenient" to pair with the +auto-generated self-signed cert) and the `//nolint:gosec` is correctly +justified — not a finding. The `LazyBrowseNode` concurrency model +(coalesced in-flight Expand, non-sticky failures, snapshot copies under +`RWMutex`) is sound and well-tested, including a 10-goroutine race test. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | New issue: `tag-go-module.ps1`'s clean-tree guard is order-dependent and silently permits tagging with uncommitted tracked changes when an untracked path sorts first (Client.Go-028). `DiscoverHierarchy`/`browseChildrenInner` pagination, the `child_has_children` hint mapping, and the duplicate-page-token guard are all correct. | +| 2 | mxaccessgw conventions | No issues found. `gofmt -l .` / `go vet ./...` clean; the `//nolint:gosec` on `InsecureSkipVerify` carries a narrow justified reason per the suppression convention. | +| 3 | Concurrency & thread safety | No issues found — `LazyBrowseNode.Expand` runs the RPC outside both locks, coalesces concurrent callers onto one in-flight RPC, publishes the result before `close(done)`, and leaves failures retryable; verified by `TestGalaxyBrowseExpandConcurrentCallersOnlyFireOneRpc` (`-race`-shaped). | +| 4 | Error handling & resilience | No issues found — `BrowseChildrenRaw` wraps transport failures in `*GatewayError`; both paginating loops guard against a repeated page token. | +| 5 | Security | No issues found — no committed secrets (only `"test"` / `"test-api-key"` fixtures); the lenient-TLS default is the documented project posture with an opt-in strict mode (`RequireCertificateValidation`). | +| 6 | Performance & resource management | No issues found — `DiscoverHierarchy` cancels each page's call context promptly inside the loop; `Children()` returns a defensive copy. | +| 7 | Design-document adherence | No issues found — lazy browse matches `docs/GalaxyRepository.md#browsechildren`; lenient TLS matches `docs/GatewayConfiguration.md`. | +| 8 | Code organization & conventions | No issues found — additive API (`Browse`/`BrowseChildrenOptions`/`RequireCertificateValidation`); `tlsConfigForOptions` cleanly extracted for testability. | +| 9 | Testing coverage | No issues found — new walker, pagination, dup-token, filter-forwarding, and TLS-posture paths are all covered. | +| 10 | Documentation & comments | New issue: README "Installing the Go client" recommends the `GONOSUMCHECK` env var, which was removed from the Go toolchain in 1.13 and is a no-op on Go 1.26 (Client.Go-029). | + ## Findings ### Client.Go-001 @@ -625,3 +658,51 @@ The two cases the empty-line check seems to cover — (a) operator pressing Ente **Recommendation:** Change `if line == "" { break }` to `if line == "" { continue }` (alongside the existing `len(args) == 0` continue, which is then redundant — keep one, drop the other for clarity). Update the `runBatch` doc-comment to read "only stdin EOF ends the session" and drop the "or an empty line" clause. If the interactive ergonomic is genuinely wanted, gate it on `isatty(stdin)` so the batch-from-pipe case isn't affected. **Resolution:** 2026-05-24 — `runBatch` no longer treats a blank line as end-of-session. The `if line == "" { break }` early-exit was removed; blank or whitespace-only lines now fall through the existing `if len(args) == 0 { continue }` guard (kept as the single blank-line skip rule for clarity), so only stdin EOF ends the session. The doc-comment was updated to read "Blank lines are skipped; only stdin EOF ends the session." Regression test `TestRunBatchSkipsBlankLinesAndContinuesUntilEOF` in `cmd/mxgw-go/main_test.go` feeds `version --json\n\nversion --json\n` (a stray blank line between two commands) and asserts two EOR sentinels are emitted — pre-fix the test failed with "EOR sentinel count = 1, want 2" because the blank line broke the loop and the second command never ran; post-fix both commands run. + +### Client.Go-028 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `scripts/tag-go-module.ps1:42-46` | +| Status | Resolved | + +**Description:** The release helper's clean-working-tree guard is order-dependent and can silently let a release tag be created on top of uncommitted tracked changes — the exact thing it advertises it prevents (the README at `clients/go/README.md` says "The script ... refuses to tag with uncommitted tracked changes"). The check is: + +```powershell +$status = (git status --porcelain) -join "`n" +if ($status -and -not ($status -match '^\?\?')) { + throw "Working tree has tracked changes. Commit or stash before tagging." +} +``` + +`git status --porcelain` emits one line per path (`XY path`), with untracked entries prefixed `??`. The lines are joined into a single string and matched against `'^\?\?'` with PowerShell `-match`, which by default is single-line (no `(?m)` multiline flag), so `^` anchors to the start of the *whole* joined string. The guard therefore inspects only the **first** status line: if that first line is an untracked file (`??`), the `-not (... -match '^\?\?')` clause is false and the throw is skipped — even when later lines are tracked modifications (` M file.go`, `A file.go`, etc.). Because `git status --porcelain` orders entries by pathname, an untracked file whose name sorts ahead of a modified tracked file (e.g. an untracked `AAA-notes.md` alongside a modified `mxgateway/session.go`) puts the `??` line first and the tag is created from a dirty tree. This was confirmed empirically: with `"?? untracked.md\n M tracked.go"` the script allows the tag; with the tracked line first it correctly throws. The whole point of the guard — reproducible release tags that match a committed state — is defeated in this ordering. + +**Recommendation:** Test each status entry individually rather than the first line of a joined blob. For example, iterate the porcelain lines and throw if any line does **not** start with `??`: + +```powershell +$dirty = (git status --porcelain) | Where-Object { $_ -and ($_ -notmatch '^\?\?') } +if ($dirty) { + throw "Working tree has tracked changes. Commit or stash before tagging.`n$($dirty -join "`n")" +} +``` + +(Equivalently, keep the joined string but use the multiline flag and negate per-line: `($status -split "`n") | ? { $_ -notmatch '^\?\?' }`.) Including the offending lines in the thrown message also helps the operator see what is dirty. + +**Resolution:** 2026-06-15 — Replaced the order-dependent joined-blob check in `tag-go-module.ps1` with a per-line filter (`git status --porcelain | Where-Object { $_ -and ($_ -notmatch '^\?\?') }`) that throws on any tracked change regardless of ordering, listing the offending lines. Verified under pwsh 7.5.4 that an untracked path sorting ahead of a modified tracked file is now correctly rejected, while untracked-only and clean trees are still allowed. + +### Client.Go-029 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `clients/go/README.md:300-303` | +| Status | Resolved | + +**Description:** The "Installing the Go client" section advises, for build environments that cannot reach `gitea.dohertylan.com` directly, to "use `GONOSUMCHECK` + `GOPRIVATE` to bypass the checksum database for the internal module path." `GONOSUMCHECK` is a dead environment variable — it was removed from the Go toolchain in Go 1.13 (its short-lived successor `GONOSUMDB` was also removed), and on the Go 1.26 toolchain this client targets (`go.mod` says `go 1.26`) setting it has no effect. The actual mechanism is `GOPRIVATE` (or the finer-grained `GONOSUMCHECK`-replacement `GONOSUMDB`→now `GONOSUMCHECK` is gone) — `GOPRIVATE=gitea.dohertylan.com/*` alone already both skips the checksum database and bypasses the public proxy for matching module paths, so the `GONOSUMCHECK` half of the recommendation is inert and misleading. A reader who copies the advice and finds checksum-db verification still failing has no working escape hatch from this doc. + +**Recommendation:** Drop `GONOSUMCHECK` and document the current knobs: set `GOPRIVATE=gitea.dohertylan.com/*` (covers both sum-db bypass and direct VCS fetch), or for the checksum database specifically `GONOSUMCHECK`'s modern equivalent `GONOSUMDB` is also gone — use `GONOSUMCHECK`→`GOFLAGS=-insecure` only for plaintext, and `GONOSUMCHECK`. Concretely: "set `GOPRIVATE=gitea.dohertylan.com/*` (this disables both the checksum database and the public module proxy for that path); add `GOINSECURE=gitea.dohertylan.com/*` if the host serves the module over plain HTTP." + +**Resolution:** 2026-06-15 — Dropped the dead `GONOSUMCHECK` advice from the "Installing the Go client" section of `clients/go/README.md`; it now documents `GOPRIVATE=gitea.dohertylan.com/*` (which bypasses both the public module proxy and checksum-database verification for that path) plus `GOINSECURE=gitea.dohertylan.com/*` for plain-HTTP hosts. diff --git a/scripts/tag-go-module.ps1 b/scripts/tag-go-module.ps1 index 13b1ccd..55a0db4 100644 --- a/scripts/tag-go-module.ps1 +++ b/scripts/tag-go-module.ps1 @@ -39,10 +39,14 @@ if ($Version -notmatch '^v\d+\.\d+\.\d+(-[A-Za-z0-9.-]+)?$') { $tag = "clients/go/$Version" Write-Host "Creating Go-module tag: $tag" -ForegroundColor Cyan -# Verify we're on a clean checkout — refuse to tag with uncommitted changes. -$status = (git status --porcelain) -join "`n" -if ($status -and -not ($status -match '^\?\?')) { - throw "Working tree has tracked changes. Commit or stash before tagging." +# Verify we're on a clean checkout — refuse to tag with uncommitted tracked +# changes. Test each porcelain line individually: any entry that is not an +# untracked file (`??`) is a tracked change, regardless of how the entries +# happen to sort. Joining and anchoring `^\?\?` against the whole blob would +# only inspect the first line and miss tracked changes behind an untracked one. +$dirty = (git status --porcelain) | Where-Object { $_ -and ($_ -notmatch '^\?\?') } +if ($dirty) { + throw "Working tree has tracked changes. Commit or stash before tagging.`n$($dirty -join "`n")" } # Verify the tag doesn't already exist.