Skip to content

[architect] refactor: extract TerminalSession interface from Manager's eight terminal seam fields (pkg/agent, #5636 phase 1) - #5638

Merged
kubestellar-prow[bot] merged 1 commit into
v4from
arch/refactor-terminal-session
Sep 2, 2026
Merged

[architect] refactor: extract TerminalSession interface from Manager's eight terminal seam fields (pkg/agent, #5636 phase 1)#5638
kubestellar-prow[bot] merged 1 commit into
v4from
arch/refactor-terminal-session

Conversation

@kubestellar-hive

Copy link
Copy Markdown
Contributor

Refactor

Claimed ground: src/pkg/agent/manager.go (Manager struct terminal fields + captureTmuxPaneForAgent, captureVisiblePaneForAgent, tmuxSessionHasAttachedClientForAgent, tmuxSendLiteralForAgent, tmuxSendKeysForAgent, sleepDuringPromptDismiss), src/pkg/agent/kick_logs.go (captureScrollbackForAgent, clearScrollbackForAgent), new src/pkg/agent/terminal.go + terminal_seams_test.go, and mechanical seam-assignment rewrites in 9 pkg/agent test files. Disjoint from all open hold-gated PRs (#5634 scanner escalation, #5628 docs, #5625 pkg/convergence/mutation, #5559 UPGRADE.md, #4032 proxy auth).

Phase 1 of #5636 (Manager god object). Manager carried eight ad-hoc nilable func fieldspaneCapture, visiblePaneCapture, sessionAttached, sendLiteralForAgent, sendKeysForAgent, promptDismissSleep, captureFullLogFn, clearHistoryFn — that together formed an implicit terminal-IO contract, each with its own nil-guard and inline tmux fallback.

This names that contract:

  • TerminalSession interface (terminal.go) with tmuxTerminal as the production impl; every method body is the previous fallback code moved verbatim from manager.go / kick_logs.go.
  • Manager's 8 fields collapse to one terminal TerminalSession; nil ⇒ tmux via Manager.term(), so zero-value &Manager{} (used widely in tests) behaves identically.
  • funcTerminal (test file only) is a per-method override fake with tmux fallback, preserving old unset-seam semantics; test assignments rewritten mechanically m.paneCapture = ftermSeams(m).capturePane = f.

No behavior change. go vet clean; full go test ./pkg/agent/ suite passes (269s).

Follow-up phases per #5636: credential/resolver grouping, nil-safe Hooks struct, file split.

Refs #5636


Filed by architect agent (ACMM L5 — hold-gated mode). Hold-gated: human review required.

— hive: agent=architect backend=copilot model=claude-opus-4-6

…s eight ad-hoc terminal seam fields

Phase 1 of #5636. Manager carried eight nilable func-typed fields
(paneCapture, visiblePaneCapture, sessionAttached, sendLiteralForAgent,
sendKeysForAgent, promptDismissSleep, captureFullLogFn, clearHistoryFn)
that together formed an implicit terminal-IO contract, each guarded by
its own nil-check with an inline tmux fallback.

This names that contract:

- pkg/agent/terminal.go: TerminalSession interface + tmuxTerminal, the
  production implementation holding the exact tmux invocations moved
  verbatim from manager.go and kick_logs.go.
- Manager gains one 'terminal TerminalSession' field; nil means tmux
  (Manager.term()), so a zero-value Manager behaves exactly as before.
- pkg/agent/terminal_seams_test.go: funcTerminal, a per-method override
  fake with tmux fallback, mirroring the old unset-seam semantics; test
  assignments rewritten mechanically to termSeams(m).<method> = ...

No behavior change: every method body is the previous fallback code,
and unset overrides still reach tmux. go vet clean; full pkg/agent
suite passes.

Refs #5636

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: hive-architect <sec-check@hive.kubestellar.io>
@kubestellar-hive kubestellar-hive Bot added the hold label Sep 2, 2026
@kubestellar-prow kubestellar-prow Bot added the dco-signoff: yes Indicates the PR's author has signed the DCO. label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Changelog: this PR changes code but does not touch CHANGELOG.md.

If it is user-visible — a feature, a fix an operator would notice, a
security change, a migration, a deprecation, or anything breaking —
please add a line under ## Unreleased. If it is a refactor, a
test-only change, or dependency churn, no entry is needed and you can
ignore this.

This is a reminder, not a gate; it never blocks a merge.

@clubanderson

Copy link
Copy Markdown
Member

Reviewed with specific attention to pkg/agent Manager's locking history. Approving — OK to merge.

Verified interface extraction only, no behavior or lock-ordering change:

  • Every tmuxTerminal method body is a verbatim move of the previous inline fallback from manager.go / kick_logs.go (compared line by line: CapturePane, CaptureVisiblePane, SessionAttached including its three fail-open returns, SendLiteral, SendKeys, Sleep, CaptureFullLog including the -J/-S/-E flags and error wrap, ClearHistory).
  • No new locking anywhere: Manager.term() reads m.terminal exactly as the eight old nilable func fields were read (no m.mu acquisition), tmuxCmd takes no lock, and no TerminalSession method touches m.mu — so nothing changes for callers that hold the lock when reaching a terminal seam. No re-entrancy risk introduced.
  • Zero-value &Manager{} semantics preserved: nil terminal falls back to tmux, same as nil func fields did; funcTerminal's per-method nil fallback preserves the old unset-seam behavior for tests.
  • Grepped the package for the eight removed field names: no stale references remain outside this diff (the one hit, paneCaptureSleep in manager_test.go, is an unrelated constant). Test rewrites are mechanical field-for-field swaps to termSeams(m).

@clubanderson clubanderson added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed hold labels Sep 2, 2026
@kubestellar-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubestellar-prow
kubestellar-prow Bot merged commit c90c3ea into v4 Sep 2, 2026
55 checks passed
@kubestellar-prow
kubestellar-prow Bot deleted the arch/refactor-terminal-session branch September 2, 2026 13:38
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution! Your PR has been merged.

Check out what's new:

Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. copilot dco-signoff: yes Indicates the PR's author has signed the DCO. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant