Skip to content

fix(cli): complete version protocol follow-ups#45

Merged
tkkhq merged 3 commits into
mainfrom
fix/pr39-review-followups
Jul 14, 2026
Merged

fix(cli): complete version protocol follow-ups#45
tkkhq merged 3 commits into
mainfrom
fix/pr39-review-followups

Conversation

@tkkhq

@tkkhq tkkhq commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #39 that completes the CLI-side version-protocol fixes:

  • Report a neutral dev request version for source, dirty, and detached-revision builds while preserving the full build identifier in User-Agent and preserving release/nightly version forms.
  • Render API instruction notices as soon as a log stream is established, before --follow blocks; consume the one-shot instruction so it is not printed again when command execution returns.
  • Make the CLI wire-contract comment self-contained rather than referring to unavailable implementation details.
  • Add coverage for stream request/response handling, no-latest deprecation notices, source-build version reporting, an actual /auth/* device-flow login, and sticky global-state cleanup.

Audience

  • End users: notices remain visible for long-running log-follow sessions, and locally built binaries report an unambiguous development version.
  • Contributors and self-builders: protocol behavior is covered through hermetic httptest cases, including source-build and login flows.

Test

  • go build ./...
  • go test ./... -count=1
  • go test -race ./internal/api ./internal/cmd/upgrade ./internal/cmd/functions ./internal/cmd/frontends ./cmd/volcano -count=1

@tkkhq tkkhq requested a review from a team as a code owner July 10, 2026 15:25
Copilot AI review requested due to automatic review settings July 10, 2026 15:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Completes CLI-side version protocol handling for development builds and long-running log streams.

Changes:

  • Reports source builds as dev while preserving diagnostic User-Agent data.
  • Displays and consumes API notices when log streams open.
  • Expands protocol, login, stream, and state-cleanup tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/logfollow/follow.go Adds stream-open callbacks.
internal/cmd/upgrade/upgrade.go Consumes one-shot notices.
internal/cmd/upgrade/upgrade_test.go Tests deprecation and cleanup.
internal/cmd/functions/logs.go Prints notices before following logs.
internal/cmd/functions/logs_test.go Tests runtime-follow notices.
internal/cmd/frontends/logs.go Prints notices before following logs.
internal/cmd/frontends/logs_test.go Tests runtime-follow notices.
internal/api/version_protocol.go Adds consumption and development-version reporting.
internal/api/version_protocol_test.go Tests version classification and consumption.
internal/api/log_stream_test.go Tests stream protocol headers.
cmd/volcano/main_test.go Expands end-to-end notice and login coverage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/api/version_protocol.go Outdated
Comment on lines +81 to +82
lastInstructions.CLIInstruction = ""
lastInstructions.LatestVersion = ""
@tkkhq tkkhq force-pushed the fix/pr39-review-followups branch from aabab83 to 44e8be5 Compare July 13, 2026 19:45

@swkeever swkeever left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Reviewed the full change against the source on both branches.

Verified

  • reportedCLIVersion(): git-describe/dirty/bare-SHA builds report dev; release/rc/nightly pass through. No false positives on real releases (v* prefix isn't all-hex; -g<sha> detection requires a numeric distance segment + ≥7-char hex sha, so -graphql/-gamma are correctly rejected). Full build id preserved in User-Agent.
  • ConsumeCLIInstructions() one-shot semantics prevent the log-follow early notice from being reprinted by run() after Execute returns; LastInstructions() still retains LatestVersion/DeviceInstruction for the error paths. No duplicate reauth hint (printError reads DeviceInstruction from LastInstructions; PrintAPIInstructionNotices only switches on CLIInstruction).
  • RuntimeWithStreamOpened fires the opened callback via sync.Once after the first successful open, at which point instructions are already recorded by the doer.
  • Test coverage is thorough and correctly wired (shared thread-safe buffer for the stderr-notice assertions; exempt-auth-route test pins the exactly-once behavior).

No correctness, convention, performance, or security concerns.

@tkkhq tkkhq merged commit a23d1a0 into main Jul 14, 2026
8 checks passed
@tkkhq tkkhq deleted the fix/pr39-review-followups branch July 14, 2026 17:44

@marckong marckong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as a PR (after-merge). Overall: correct — build, full test suite, and -race all pass; no blocking bugs found.

The consume-once mechanism (ConsumeCLIInstructions + consumedCLIInstructions) and the sync.Once stream-opened callback compose correctly across the success, error, 426, and reconnect paths — no double-print or lost-notice cases. The reportedCLIVersion git-describe/SHA/dirty heuristics correctly map source builds to dev while passing through releases, -rc. prereleases, and -nightly.* forms.

Only a P3 nit: the header-constants comment dropped the pointer to volcano-hosting's canonical copy, so a maintainer changing these values loses the "where to sync" hint — intentional per the PR description.

Approve.

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.

4 participants