refactor: rename --clear-screen flags to symmetric --clear-on-attach/--clear-on-detach#432
Merged
Merged
Conversation
…tach/--clear-on-detach
Owner
Author
|
PR #432 Review — LGTM — complete, symmetric hard-rename across every layer (CLI flags, env/viper keys, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--clear-screen/--clear-screen-on-detachpair (introduced days ago in fix: stop clearing terminal on attach/detach by default, add opt-in clear flags #426) to the symmetric, lifecycle-explicit--clear-on-attach/--clear-on-detach. "screen" is dropped — at the attach/detach boundary there is nothing else to clear, so the pair now reads as an obvious matched set.0a171ffis in no tagged release (git tag --contains 0a171ffis empty; it lands afterv0.12.5), so AC feat(session): add tty device to status spec #4's hard-rename branch applies.sb attachand thesbshclient root),SB_ATTACH_*/SBSH_CLIENT_*env vars + viper keys,ClientSpec/pkg/attach.Options/api.AttachRequeststruct fields, and the JSON wire tags (clearScreen→clearOnAttach,clearScreenOnDetach→clearOnDetach).Scope notes (calls the reviewer can push back on)
pkg/builder.WithClientClearScreen→WithClientClearOnAttach,WithClientClearScreenOnDetach→WithClientClearOnDetach(+ their internalclientConfigfields), andpkg/attach.Options.ClearScreen/ClearScreenOnDetach→ClearOnAttach/ClearOnDetach.pkg/spawn.buildClientAttachArgs(the producer of thesb attachargv) was updated in lockstep with the renamed flag strings. Leaving these on the old names would have re-created the exact asymmetry the issue removes. All are fix: stop clearing terminal on attach/detach by default, add opt-in clear flags #426-era, pre-entrenchment surfaces.clearScreenparams/vars/struct fields ininternal/terminal/terminalrunner/*andinternal/client/clientrunner/terminal.go, plus theescClearScreen/escClearScreenHomeconsts (which literally name the\x1b[2JANSI sequence, not the flag). Doc-comment / test-error references to the removed--clear-screenflag literal were updated to--clear-on-attachfor accuracy.AC #3 — on-disk metadata / migration (dev to confirm whether on-disk metadata is affected)
No migration path needed. Findings:
clearScreen/clearScreenOnDetachlive only on the ephemeralapi.ClientSpec(pkg/api/client.go). The persistent server-sideapi.TerminalSpeccarries no such field, so persisted terminal metadata is unaffected.sb attach; the live session's behavior is driven by the in-memorySpecvalue (io.go,lifecycle.go), never read back from a persisted client doc to drive the clear. The on-disk client doc is written for live-client discovery only. A pre-rename client doc belongs to a still-running pre-rename client whose behavior is already in memory — so a hard tag rename cannot make an existing session "silently lose the setting." Clients are short-lived (one per interactive attach).AC #6 — docs/README references
git grepover the whole tree for the old flag/env/viper literals on non-.gofiles returns none, so nodocumentationfollow-up issue is warranted.Test plan
make sbsh-sb— canonical build; binary is ELF (7f 45 4c 46),sb/sbshhardlinked../sb attach --helpand./sbsh --helpshow--clear-on-attach/--clear-on-detach;--clear-screen*no longer present.go build ./...clean.go vet ./...clean.make test(full CI target, incl. e2e) — all green.go test ./pkg/...— all green (builder/spawn/attach/api tests updated for the new names).git grepconfirms no remaining--clear-screen/CLEAR_SCREEN/ old viper-key / exportedClearScreen*identifiers (only the permitted internal lowercase locals + ANSI-escape consts remain).Closes #430