dohertj2/mxaccessgw
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(client/go): correct tag-go-module dirty-tree guard; GOPRIVATE docs (Client.Go-028,029)

Browse Source
This commit is contained in:
Joseph Doherty
2026-06-15 02:39:11 -04:00
parent fb2b1a4a52
commit d0d1dcef15
3 changed files with 96 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Client.Go/findings.md
+83 -2
View File
@@ -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.