Conversation
7e100f2 to
0b05b27
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is non-blocking, failure-isolated, and comprehensively tested across its key behaviors.
Review effort: Balanced
Findings: None
What changed in this PR
Adds a background release notifier that preserves command output and exit behavior.
Changes:
- Checks official releases and caches daily check/reminder state.
- Integrates non-blocking notifications into successful commands.
- Adds comprehensive tests and user documentation.
| File | Description |
|---|---|
README.md |
Documents updates and notifier behavior. |
cmd/root.go |
Integrates background checks into command lifecycle. |
cmd/root_test.go |
Tests notifier integration and exclusions. |
internal/update/update.go |
Implements eligibility, release checks, caching, and notices. |
internal/update/update_test.go |
Covers update-check behavior and failure modes. |
go.mod |
Adds direct notifier dependencies. |
go.sum |
Records module checksums. |
docs/src/content/docs/reference/cli.md |
Adds detailed notifier reference documentation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
francisfuzz
left a comment
francisfuzz
left a comment
There was a problem hiding this comment.
✨ This is thoughtful work. The install validation, pre-request reservation, state recovery, and test coverage are all much stronger than the spike I put together.
❓ Asking to learn: are we intentionally choosing successful commands as the delivery point for the gh-stack notice?
I spiked the inverse on spike/update-notifier: let core gh own successful invocations, then have gh-stack cover failures because core gh skips its extension PostRun notice when the extension returns an error.
With the current approach, a modern gh installation can potentially show both notices after a successful command:
A new release of gh-stack is available: 0.1.0 -> 0.1.1
To upgrade, run: gh extension upgrade stack
A new release of stack is available: 0.1.0 → 0.1.1
To upgrade, run: gh extension upgrade stack
Meanwhile, a failed command, which is when someone is most likely to file a bug, does not show either notice.
I am not suggesting we replace this implementation with the spike. The implementation here is stronger. I mainly want to confirm that accepting possible duplication on success, while leaving failures untouched, is the intended product tradeoff. If so, could we capture that rationale in the Boundary section?
Not blocking my approval. I want to make sure this behavior is deliberate rather than incidental to PersistentPostRun. ✅
Adds a lightweight upgrade nudge for users running an older gh-stack release, pointing them to
gh extension upgrade stackwithout upgrading automatically.Functionality and user impact
/releases/latestendpoint—the same release source used by extension upgrades—and require a newer stable version with a matching platform binary.StateDir()/gh-stack/state.yml, shared across repositories for the user.GH_STACK_NO_UPDATE_NOTIFIER=1disables the notifier; failures are diagnostic-only underGH_DEBUG.Boundary: commands never wait for the network check. Short commands may miss a notice, and failed or canceled attempts still count toward the daily check limit; completed results can be shown later. Local/development, prerelease, and pinned installations are excluded, as are help, version, and completion commands. Concurrent processes may occasionally duplicate a check or reminder.
Key areas to review
internal/update/update.gocheck,noticeDue,readState, andwriteStateseparate discovery from delivery, throttle failed attempts, recover invalid state, and replace the YAML file atomically.eligibleandfetchLatestrestrict notices to official, unpinned installs and avoid downgrade or unsupported-platform suggestions.cmd/root.go: pre/post-run hooks, buffered result delivery, and execution-context cancellation keep checks out of command output and error handling until a successful command finishes.Related issues