chore(deps): replace dgageot/ultraviolet fork with upstream - #4006
chore(deps): replace dgageot/ultraviolet fork with upstream#4006Sayt-0 wants to merge 2 commits into
Conversation
aheritier
left a comment
There was a problem hiding this comment.
CI is green across all checks (lint, build-and-test, windows-tests, CodeQL, license-check, build-image). No blocking correctness defect found in this change.
Two should-fix points for follow-up (non-blocking):
- [should-fix]
pkg/tui/renderer_wide_emoji_test.go:70-83: the test constructsuv.NewTerminalRendererdirectly with hardcoded env (TERM=xterm-256color,COLORTERM=truecolor,CLICOLOR_FORCE=1) and forces truecolor on abytes.Buffer. That setup doesn't reflect docker-agent's actual production rendering path, so it can pass even if the real integration diverges. - [should-fix] The test suite covers ultraviolet's
TerminalRenderer/ScreenBufferdirectly rather than docker-agent's TUI integration (the bubbletea layer that actually negotiates DEC mode 2027 and drives rendering at runtime). It pins ultraviolet's contract, not docker-agent's use of it.
Optional: ANSI scanner robustness in the same file could use a bit more hardening, but that's a minor/optional nit.
Given this PR changes rendering behavior around wide-emoji/cursor handling, please complete the mandated manual visual validation on macOS + iTerm2 before merging — the automated coverage above doesn't substitute for it.
aheritier
left a comment
There was a problem hiding this comment.
Correction to my earlier review here: my statement that "CI is green across all checks (lint, build-and-test, windows-tests, CodeQL, license-check, build-image)" was only accurate for the earlier head 0603228. It is no longer true — please disregard it.
windows-tests is currently red on head e87b70d (run 32370277571 / job 96429008797):
--- FAIL: TestBackgroundAgent_InspectorShowsWorkerContext (2.23s)
testing.go:1464: TempDir RemoveAll cleanup: unlinkat C:\Users\RUNNER~1\AppData\Local\Temp\TestBackgroundAgent_InspectorShowsWorkerContext975820743\001\data\tui_state.db: The process cannot access the file because it is being used by another process.
FAIL github.com/docker/docker-agent/e2e/tui 42.183s
One-line summary: the e2e TUI test leaves the tui_state.db SQLite handle open, so t.TempDir cleanup cannot unlink it on Windows.
Attribution — this does not look caused by your diff: newBackgroundAgentTUI (e2e/tui/background_agent_test.go:125-172) calls tui.New(...), which opens the state store at pkg/tui/service/tuistate/store.go:21 under the isolated t.TempDir() data dir, and never closes it. pkg/tui/session_load_click_test.go:100-105 already registers t.Cleanup(m.cleanupManagedResources) for exactly this reason (added in 24fa4d7, "fix: close Windows test resources"); the e2e helper never got the same treatment, and that file predates this PR (18637507, present in base ed11732). Your diff only touches .golangci.yml, go.mod, go.sum and the new pkg/tui/renderer_wide_emoji_test.go, and the same code passed windows-tests on 0603228 — so this reads as a pre-existing latent flake, not a regression from this change.
That said, merge must still wait for a green windows-tests on the head SHA: please rerun the job, and if it reproduces, the missing store close should be fixed first (a separate PR is fine).
Also still outstanding from my earlier review: the manual visual validation on macOS + iTerm2 is required before merge — the automated coverage in this PR does not substitute for it.
|
👋 Some commits in this PR are not signed and verified by GitHub. Please sign your commits with a GPG or SSH key registered in your GitHub account, then force-push. Commits that are not verified: See GitHub's guide on signing commits for setup instructions. I've added |
Summary
dgageot/ultravioletfork with the already-required official upstream revisiongomoddirectivesreplacement exception and refresh module checksumsIssue expectations
TestRendererWideEmojiBackgroundpasses against both the former fork and the upstream revisionv0.0.0-20260811164956-006e29f97886go mod tidy; fork checksums were replaced with official upstream checksumsgomoddirectives.replace-allow-listgo-licenses checkcommand passes in a clean Linux containerValidation
task linttask testtask buildtask check-plan-crossGOOS=windows GOARCH=amd64 go test -exec=true ./pkg/tui/...go-licenses check . --allowed_licenses=Apache-2.0,MIT,BSD-3-Clause,BSD-2-Clause --ignore modernc.org/mathutil --ignore github.com/hashicorp/hcl/v2Closes #3984