chore(#1859): gofmt sweep + gofmt/go vet CI gate (rebase of #1881) - #1941
Merged
Conversation
Regenerated on current master. The sweep in Kpa-clawbot#1881 was cut on 2026-09-02 07:13 and roughly forty PRs landed after it, so its 72-file diff no longer applied; re-running gofmt is cheaper and safer than resolving 72 formatting conflicts. Same change, same intent, different base. 66 files across the 14 modules. Verified formatting-only, twice, rather than asserted: 1. Every changed file is byte-identical to gofmt(previous content): 0 of 66 deviate. 2. With line comments and all whitespace stripped, 0 of 66 files differ. So no code outside comments changed. 14 of the 66 also show doc-comment reflow: since Go 1.19 gofmt re-indents indented comment blocks to tabs and inserts a blank comment line before them (e.g. the behavior matrix above resolveHopWithContext in cmd/ingestor/path_resolver.go). That is gofmt's normal output, not an edit. Excluded: the misnamed Dockerfile.go, which is a Dockerfile that gofmt cannot parse, and docs/DEPLOYMENT.md, which a case-insensitive filesystem surfaces as a spurious modification against docs/deployment.md and has nothing to do with this sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQS3XLoPD98yu9pxdRujqg
…st_test `go vet ./...` flags the range loop in TestNodeDaysOrDefault: the table element is copied per iteration and contains a Config, which embeds sync.Once fields — copying a lock. This is the only vet finding across all 14 modules and would otherwise block the vet gate added next. Range by index and take a pointer to the element instead of copying it. NodeDaysOrDefault already has a pointer receiver, so no behaviour changes; the same cases run. Co-Authored-By: Claude <noreply@anthropic.com> (cherry picked from commit b80e981)
…blame Add a CI step that fails the build when Go code is not gofmt-clean or when `go vet` reports findings, closing the two gaps from Kpa-clawbot#1859: - gofmt -l over every tracked *.go file (excluding the misnamed Dockerfile.go, which gofmt cannot parse). - go vet per module — the repo is multi-module, so vet runs in each of the 14 module directories in turn. Also record the preceding gofmt sweep commit in .git-blame-ignore-revs so `git blame` skips the mechanical reformatting. Verified locally: gofmt clean and go vet passes across all 14 modules. Co-Authored-By: Claude <noreply@anthropic.com> (cherry picked from commit fdadaa7) Rebase note (not by the original author): .git-blame-ignore-revs pointed at 04bc80e, the sweep commit on the Kpa-clawbot#1881 branch. That commit does not exist on this base, so git blame --ignore-revs-file would error on it. Repointed at d3a0259, the regenerated sweep in this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQS3XLoPD98yu9pxdRujqg
This was referenced Sep 2, 2026
Closed
efiten
added a commit
that referenced
this pull request
Sep 3, 2026
…done (#1945) Closes #1943. The colour picker's keyboard navigation is broken, and the E2E flake that has been failing unrelated PRs (#1940, #1941, and master pushes `589fa987` and `859173f1`) was reporting it correctly. ## Cause `showPopover` deferred focusing the first swatch with an uncancellable `setTimeout(..., 0)` at `channel-color-picker.js:146`, and nothing cleared it on hide. The file contained **zero** `clearTimeout` calls. Reopen the popover while a swatch still holds focus and that timer lands after the user has already pressed an arrow key, pulling focus back to the first swatch. Proven, not argued. Instrumenting `HTMLElement.prototype.focus` with a stack trace, on one open: ``` focus(#f97316) @10205ms <- the keydown handler focus(#ef4444) @10208ms <- channel-color-picker.js:146:58 ``` Three milliseconds apart. ## The user-visible bug Worse than a flaky test. **Open the picker, arrow to a colour, press Enter, and the first colour is assigned instead of the one you chose.** Holding the timing still made the existing suite say so directly: ``` ✗ Enter should assign focused color (#f97316), got #ef4444 ``` ## Why the test looked flaky The revert happens on **every** open. Only whether the assertion reads before or after it varies, which is why an idle machine passes and a loaded runner does not. #1939 (mine) assumed the opposite: a race in which the handler had not yet moved focus, cured by waiting for it. #1943 has the measurement that disproves it. The failing step took **16 ms** while that wait has a **3 second** budget, so the wait was resolving successfully and then the value was reverted underneath it. It never helped. Its comment is corrected in this PR rather than left to mislead the next reader. ## Fix Keep a handle for the timer, cancel a pending one on both show and hide, and inside it do nothing when the popover has since been hidden or when focus already sits inside it. A fresh open still focuses the first swatch, which is what the accessibility behaviour is for. An open that inherits focus, or a user who has already navigated, is left alone. ## Verification - The **regression test added here fails on unmodified master** with `a late focus timer must not move focus after the user did` and passes with the fix. - It is **deterministic, not load-dependent**: it reproduces the exact sequence the stack trace identified (open, Escape, reopen, ArrowRight before the timer lands) rather than waiting for contention. It also asserts the Enter path, so the user-visible half is covered and not just focus position. - Full suite: 10 of 10, three consecutive runs. ## Note on the other flake This is one of two E2E failures blocking the queue. The other, #1925, is a different mechanism in a different file and is fixed separately in #1944. Together they should leave the E2E suite deterministic again. Same shape as @TeTeHacko's finding in #1940: something is operable before its setup has finished. That is now three instances in this codebase, so it may be worth a look as a pattern rather than three separate fixes. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Recycling this (close + reopen) before merging it. Its green run is from 2026-09-02 21:37 and master has moved six times since, including two E2E fixes (#1944, #1945) and two of @TeTeHacko's PRs. A 66-file sweep is not something to merge on a verdict measured against a different tree, and A plain re-run would reuse the old merge commit, so close and reopen is what forces a fresh merge ref. No change to the content. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebase of #1881 by @SaarMesh-Bot onto current master. Their three commits are preserved, two of them cherry-picked with authorship intact; the sweep itself had to be regenerated. Opened as a new PR rather than force-pushing their branch.
Closes #1881 once merged. Addresses parts 1 and 3 of #1859; part 2 landed as #1937.
Why regenerated rather than merged
The sweep in #1881 was cut on 2026-09-02 07:13 and roughly forty PRs landed after it, so it went
CONFLICTING/DIRTY. Re-runninggofmton current master is cheaper and less error-prone than resolving 72 conflicts that are all whitespace. The drift it fixes also grew in the meantime: 66 files now, against 72 then, but spread differently.The three commits
style(#1859)—gofmt -wacross the 14 modules. 66 files.test(#1859)— @SaarMesh-Bot's fix for the onego vetcopylocks finding,cmd/ingestor/coverage_boost_test.go: the range variable copied aConfigembeddingsync.Once. Cherry-picked unchanged.ci(#1859)— @SaarMesh-Bot's CI step that fails on gofmt drift or vet findings, plus.git-blame-ignore-revs. Cherry-picked with one change, noted in the commit message: the ignore file pointed at04bc80ee, the sweep commit on their branch, which does not exist on this base and would makegit blame --ignore-revs-fileerror. Repointed atd3a02599, the sweep here.Verification
The claim "formatting only" is checked twice rather than asserted:
gofmt(previous content). 0 of 66 deviate.14 of the 66 also show doc-comment reflow. Since Go 1.19
gofmtre-indents indented comment blocks to tabs and inserts a blank comment line before them; the behavior matrix aboveresolveHopWithContextincmd/ingestor/path_resolver.gois a clear example. That is gofmt's own output, not an edit, but it is worth naming because it makes the diff look larger than "whitespace" suggests.The gate was run locally exactly as the workflow runs it:
gofmtclean, andgo vetclean in all 14 modules, includingcmd/ingestorwhich is what commit 2 fixes.Suites:
cmd/serverok (80.7s),internal/packetpathok (2.3s),cmd/ingestorpasses exceptTestWriteStatsAtomic_SymlinkAtDestIsReplaced, which fails identically on bare master with "A required privilege is not held by the client" (Windows symlink privilege on my host, not code).Sequencing
This should go last in the queue. The sweep touches 66 files, so merging it before the remaining open Go PRs gives each of them a conflict about nothing but formatting. After it lands the gate is active, and any PR with drift fails CI until it runs
gofmt -w.Excluded from the sweep: the misnamed
Dockerfile.go, which is a Dockerfile that gofmt cannot parse (the workflow excludes it too), anddocs/DEPLOYMENT.md, which a case-insensitive filesystem surfaces as a spurious modification againstdocs/deployment.mdand is unrelated.