Uh oh!
There was an error while loading. Please reload this page.
feat: improve version checking - #799
Conversation
Claude finished @mbevc1's task in 2m 27s —— View job PR Review: feat: improve version checking
Static AnalysisAll checks pass: SummaryThis is a solid, well-structured PR. The changes address real pain points (JSON output pollution in CI, version check on What's good:
Minor nits (non-blocking):
No bugs, no security issues, no missing edge cases. The previous review rounds have been thorough and the feedback has been addressed well. LGTM. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mbevc1
left a comment
There was a problem hiding this comment.
Goroutine-safety nit on SetCheckForUpdateOverride — inline with a sync.RWMutex suggestion and a race test. Current call site is fine by happens-before; the mutex is cheap insurance against future callers / flaky timings.
Generated by Claude Code
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
mbevc1
left a comment
There was a problem hiding this comment.
Coverage gap: executeCommandC bypasses innerMain, so the new --version sync-notice branch in main.go has no test coverage. Inline with a sketch calling innerMain directly via the existing SetCheckForUpdateOverride helper.
Generated by Claude Code
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
future prof machine formatting Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
update the comment as well Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
d37685b to
8ff25e7CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
* feat: improve version checking * Update internal/version/update_check.go Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * feat: improve version checking - fix tests * feat: improve version checking - review improvements * feat: improve version checking - fix testing * feat: improve version checking - add version test * feat: improve version checking - fix version test * chore: Update internal/version/update_check.go * feat: improve version checking - fix missing import * chore: Update cmd/kosli/root.go future prof machine formatting Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * chore: Update cmd/kosli/root.go update the comment as well Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * feat: improve version checking - add race test * fix: AI broken suggestion * feat: improve version checking - improve fragile testing * feat: improve version checking - remove duplicated and add empty version test * feat: improve version checking - disable when in debug mode * fix: Update cmd/kosli/version.go typo Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * chore: Update cmd/kosli/root.go comment Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> * feat: improve version checking - parse DEBUG ENV same as Viper --------- Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
* test: pin fingerprint capture cleanliness contract Adds three tests that defend the customer-facing contract for `kosli fingerprint`: stdout is exactly the fingerprint, stderr is empty on the success path, and stderr remains a functional channel for opt-in debug output. The version-notice-on-stderr bug has been re-introduced three times in two weeks (PR #781 → PR #799 → PR #840) because no test pinned the actual contract customers depend on — that the output of `kosli fingerprint` is shell-capturable. Each round added a test narrowly aimed at the symptom the author was thinking about; none asserted the contract. These tests assert the contract directly via: TestFingerprintFile_CaptureCleanliness stdout == "<sha256>\n", stderr == "", combined == stdout — matches the customer pattern FP=$(kosli fingerprint ... 2>&1). TestFingerprintDir_CaptureCleanliness Same contract for --artifact-type=dir, the slow path that triggered the cyber-dojo failure (the goroutine had time to complete and pollute stderr). TestFingerprintFile_DebugModeIsAllowedToWriteStderr Pins the inverse: --debug=true MUST produce stderr output containing "calculated fingerprint", catching anyone who over-corrects a CaptureCleanliness regression by silencing the logger inside the fingerprint code path. Closes #5564 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: align dir test, add docker variant for capture cleanliness Address review feedback on PR #846: - @mbevc1 (and the claude bot): align TestFingerprintDir_CaptureCleanliness with the file variant. The dir fingerprint of testdata/folder1 is already pinned in fingerprint_test.go, so use Equal with that exact value plus the combined-stream assertion. Both tests now have the same shape and the same three contracts. - @JonJagger: add TestFingerprintDocker_CaptureCleanliness covering --artifact-type=docker, which goes through internal/docker.GetImageFingerprint and is a separate code path from file/dir hashing. Mirrors the existing docker test pattern (alpine pinned by digest, pulled in SetupSuite). OCI variant and broader attest-command coverage tracked as follow-ups in #848 and #849 — both are real engineering work that warrants separate PRs (OCI needs registry scaffolding; attest needs auth/server + a contract surface audit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
--versiondevas version