Skip to content

refactor(sessiontitle): simplify Generator without changing behavior#2575

Merged
dgageot merged 2 commits intodocker:mainfrom
dgageot:board/simplify-sessiontitle-package-without-br-6a252ec0
Apr 28, 2026
Merged

refactor(sessiontitle): simplify Generator without changing behavior#2575
dgageot merged 2 commits intodocker:mainfrom
dgageot:board/simplify-sessiontitle-package-without-br-6a252ec0

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented Apr 28, 2026

Simplifies pkg/sessiontitle to make it easier to read and maintain. No public API changes, no behavior changes — same fallback chain, same timeout, same prompts, same observable outputs to callers.

Changes

Two refactor-only commits, kept separate so the diff stays easy to follow.

1. refactor(sessiontitle): tidy Generator without changing behavior

  • Consolidated the three early-return guards (g == nil, no models, no user messages) at the top of Generate, before allocating the timeout context.
  • Replaced the manual two-pass nil-filter in New with a single slices.DeleteFunc.
  • Extracted generateOnce so each attempt closes its stream via defer (no more streamErr plumbing).
  • Extracted buildPrompt to keep prompt construction out of the main loop.
  • Removed the dead if baseModel == nil { continue } (New already filters nils).
  • Tightened sanitizeTitle to a single-line loop body.

2. refactor(sessiontitle): unify the per-attempt failure path

  • Pushed the "empty model output" check into generateOnce: it now returns a normal error like any other attempt failure, so the main loop has a single failure path.
  • Extracted drainStream as a small helper that owns reading and closing the message stream.
  • Unified per-attempt log level at Debug (these aren't fatal while we have fallbacks; callers already log/wrap the final error).
  • Dropped the post-loop if lastErr != nil guard — the loop's invariants now make lastErr always non-nil at that point.

Final shape

The package is now a clear linear pipeline of small, single-purpose functions:

  • New — build the model list (filtering nils)
  • Generate — iterate models, log per-attempt outcomes, return the first success or wrap the last failure
  • generateOnce — clone provider, open stream, drain, sanitize, validate non-empty
  • drainStream — read + close
  • buildPrompt — format messages
  • sanitizeTitle — first non-empty line, strip \r

Validation

  • mise lint — clean (golangci-lint: 0 issues; custom analyzer: no offenses; go mod tidy clean)
  • mise test — full suite passes, including pkg/sessiontitle, pkg/app, pkg/server, pkg/runtime, cmd/root
  • Existing tests (TestGenerator_Generate_FallsBackOnStreamCreateError, TestGenerator_Generate_FallsBackOnRecvError, TestGenerator_Generate_FallsBackOnEmptyOutput) all still pass unchanged.

dgageot added 2 commits April 28, 2026 15:00
- Consolidate the three early-return guards (nil generator, no models, no\n  user messages) at the top of Generate, before allocating the timeout\n  context.\n- Use slices.DeleteFunc to filter nil providers in New, replacing the\n  manual two-pass append loop.\n- Extract generateOnce so each attempt closes its stream via defer and\n  returns plainly, removing the streamErr plumbing.\n- Extract buildPrompt to keep message construction out of the main loop.\n- Drop the dead 'if baseModel == nil' check in the loop; New already\n  filters nils.\n- Tighten sanitizeTitle into a single loop body.\n- Unify per-attempt failure logging into a single message; the wrapped\n  error already distinguishes create-stream vs receive-stream failures.

Assisted-By: docker-agent
- Move the 'empty model output' check into generateOnce so it returns\n  a regular error like any other attempt failure. The main loop now has\n  a single failure path: log at Debug and continue to the next model.\n- Extract drainStream as a small helper that owns reading and closing\n  the message stream, removing the streamErr plumbing from generateOnce.\n- Unify per-attempt logging at Debug level. Per-attempt failures are not\n  fatal as long as we have fallbacks, and callers already log/wrap the\n  final returned error.\n- Drop the post-loop 'if lastErr != nil' guard: every non-success path\n  in the loop now sets lastErr, so the wrap can be unconditional.

Assisted-By: docker-agent
@dgageot dgageot requested a review from a team as a code owner April 28, 2026 13:27
@dgageot dgageot merged commit 1bb5266 into docker:main Apr 28, 2026
9 checks passed
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