Skip to content

fix: remove version checks on all commands#840

Merged
mbevc1 merged 1 commit intomainfrom
20260501_revert_ver_checks
May 1, 2026
Merged

fix: remove version checks on all commands#840
mbevc1 merged 1 commit intomainfrom
20260501_revert_ver_checks

Conversation

@mbevc1
Copy link
Copy Markdown
Contributor

@mbevc1 mbevc1 commented May 1, 2026

Some CI systems (like GHA) were multiplexing stdErr/stdOut and polluting output when piping or using output from CLI. This removes checking for version on all commands and only does it when explicitly running --version

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 1, 2026

Claude finished @mbevc1's task in 2m 7s —— View job


PR Review: Remove version checks on all commands

  • Run static analysis (build, vet, mod tidy)
  • Read and analyze changed files
  • Review code quality and correctness
  • Post review feedback

Static Analysis

All green — go build, go vet, and go mod tidy pass with no issues.


Summary

Clean, well-scoped change. The PR removes the background version-check goroutine from PersistentPreRunE and the PersistentPostRun hook that consumed it. After this PR, update checks only fire for kosli version (in version.go:52) and kosli --version (in main.go:61). This directly addresses the CI stdout/stderr pollution problem described in the PR body.

What's Good

  • Correct diagnosis and fix — the background goroutine wrote to stderr in PersistentPostRun, which can interleave with stdout in CI environments that mux the two streams. Removing it from every command is the right fix.
  • Existing guardrails preserved — the kosli version subcommand and --version flag paths are untouched, so interactive users still get the upgrade notice when they explicitly ask for the version.
  • Tests updated correctly — the renamed TestVersionNoticeNotShownOnRegularCommands test now asserts both json and table output formats produce no update notice, which matches the new behavior. The TestVersionFlagPrintsNotice test still validates the --version path.
  • No dead code left behind — the updateNoticeCh channel, the goroutine, and the PersistentPostRun hook are all removed together. All imports in root.go are still used.

Potential Issues

  1. Loss of proactive upgrade notices for interactive users — Previously, a user running e.g. kosli list flows at a terminal would see an unobtrusive "new version available" notice on stderr. That's now gone for all commands except explicit version queries. This is a deliberate trade-off (CI correctness > interactive convenience), but worth noting for the team. If you want to bring back the notice for TTY-attached sessions in the future, a isatty(stderr) guard would be a targeted approach.

  2. No KOSLI_DISABLE_UPDATE_CHECK opt-out cleanup — The existing KOSLI_DISABLE_UPDATE_CHECK env var (checked in internal/version/update_check.go) is still referenced and tested. Since the check no longer runs on regular commands, the env var now only affects kosli version / --version. That's fine, but the env var documentation (if any) may need updating to reflect the narrower scope.

Verdict

LGTM — the change is minimal, correct, and well-tested. The CI pollution fix is a clear improvement.

@mbevc1 mbevc1 enabled auto-merge (squash) May 1, 2026 10:19
@mbevc1 mbevc1 merged commit eb93aaa into main May 1, 2026
20 checks passed
@mbevc1 mbevc1 deleted the 20260501_revert_ver_checks branch May 1, 2026 10:22
AlexKantor87 added a commit that referenced this pull request May 1, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants