chore(main): release 1.38.0#156
Merged
Merged
Conversation
github-actions
Bot
force-pushed
the
release-please--branches--main
branch
2 times, most recently
from
July 17, 2026 02:25
900a196 to
a2d3c3c
Compare
github-actions
Bot
force-pushed
the
release-please--branches--main
branch
from
July 17, 2026 03:37
a2d3c3c to
d10a4f2
Compare
Contributor
Author
|
🤖 Created releases: 🌻 |
tstapler
added a commit
that referenced
this pull request
Jul 26, 2026
…tor gap (#163) * fix(terminal): correctly scan OSC/DCS escape sequences to stop render artifacts stripANSIBytes and sanitizeUTF8Bytes treated any ASCII letter as the end of an escape sequence. That's only true for CSI (ESC[...letter); OSC (ESC]...BEL or ESC\) and DCS/PM/APC/SOS (ESC{P,^,_,X}...ESC\ or 0x9C) terminate differently, and their payloads (window titles, hyperlink URLs, shell-integration marks) almost always contain a letter before the real terminator. Claude Code's newer renderer emits more of these OSC sequences, so their payload tails were leaking through as literal text in the web terminal and throwing off cursor-column math. Add a shared scanEscapeSequence helper (mirrors the correct boundary logic already used by pkg/analytics/escape_code_parser.go) and rewire both duplicated stripANSIBytes definitions plus sanitizeUTF8Bytes to consume whole sequences atomically instead of stopping at the first letter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(terminal): widen CSI final-byte range to 0x40-0x7E, cap OSC/DCS scan size Code review on PR #156 found that the new scanCSI only accepted A-Z/a-z as CSI terminators, missing real ECMA-48-valid final bytes like '@' (0x40, Insert Character) and '~' (0x7E, used by many real xterm sequences e.g. function/navigation keys). Confirmed empirically: stripANSIBytes("\x1b[5@Hello") leaked "@hello" instead of "Hello" — the exact bug class this PR exists to eliminate, just for a different final byte. Widened to the full 0x40-0x7E range and aligned the malformed-CSI fallback with pkg/analytics' semantics (give up and consume only the ESC, rather than swallowing partially-scanned params). Also: - Widened pkg/analytics/escape_code_parser.go's parseCSI terminator range to match (it was cited as the reference implementation for this fix but had the same narrower gap for '~' and other non-letter finals in 0x5B-0x60/0x7B-0x7E). - Added a size cap (mirroring escape_code_parser.go's existing 65536 bound) to scanUntilTerminator so an unterminated/adversarial OSC or DCS payload can't force an unbounded scan. - Pre-size the bytes.Buffer in stripANSIBytes/sanitizeUTF8Bytes with Grow(len(b)) to avoid reallocation growth in this hot path. - Removed the now-vestigial (*StateGenerator).stripANSIBytes wrapper method now that its only caller can use the shared free function directly. - Added regression tests for all of the above, including a mid-buffer (start > 0) case and the new size-cap behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * refactor: remove dead MOSH-style terminal state-sync infrastructure Investigating the terminal-artifacts report (PR #156) surfaced that server/terminal (the package that PR fixed) has zero callers anywhere in the repo — it's pre-integration code for a "state" streaming mode that was designed but never wired into the live rendering path. Tracing further found this same abandoned effort spans three separate, never- fully-connected implementations, plus the client-side code built to consume them: - server/terminal (StateGenerator/DeltaGenerator) — zero callers. - server/ssp (Mosh-inspired Coordinator) + session/framebuffer (its diff engine) — zero callers. - session/terminal_state.go (regex-based TerminalState) — only called from SessionService.StreamTerminal, which browsers never reach: the WebSocket bridge (connectrpc_websocket.go) intercepts the same RPC procedure and reimplements it with plain raw-byte passthrough. - server/compression (LZMA) — only exercised by transport_e2e_test.go, itself a spec-as-test for streaming modes the server never actually implements (its own docstring says so). - Frontend: StateApplicator/DeltaApplicator/EchoOverlay/lzma.ts and the predictive-echo/SSP-negotiation logic in useTerminalFlowControl.ts — all gated behind `sspNegotiated`, which never becomes true since the server never sends an SSPNegotiation message. Confirmed sendInput (the working path) is always used in practice, not sendInputWithEcho. - The streaming-mode selector (config field, proto field, dropdown UI) that drove all of the above: the value was accepted and logged but never branched on by the code that actually sends output. Simplified SessionService.StreamTerminal to plain raw output (removing its session.TerminalState dependency) rather than deleting it outright, since the RPC interface still requires an implementation for any future non-browser Connect client. Also widened the same CSI final-byte gap (missing '@'/'~' etc.) in pkg/analytics/escape_code_parser.go that PR #156 fixed in server/terminal, since it was cited as that fix's reference implementation and had the identical bug. Left ADR-002 (which documented the abandoned "state" mode decision for external sessions) in place with its status marked Superseded, rather than deleting it, to preserve the historical record. Verification: go build/vet, full Go test suite, npx tsc --noEmit, and the full Jest suite (2776 tests) all pass. go mod tidy dropped the now- unused github.com/ulikunitz/xz dependency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(terminal): resolve CSI terminator gap and code-review follow-ups BUG-025: widen the CSI final-byte character class from [a-zA-Z] to [@-~] (0x40-0x7E per ECMA-48) everywhere it was copy-pasted narrow, so sequences terminating on '@' or '~' no longer leak raw escape bytes past rate-limit/activity detection and tmux banner filtering. Consolidates the fix into one shared, tested module per language (pkg/ansi for Go, web-app/src/lib/terminal/stripAnsi.ts for TypeScript) instead of five independently duplicated regexes, per code-review findings. Also reserves and deletes the orphaned MOSH oneof message types left behind in events.proto by the earlier dead-code removal. Adding real end-to-end test coverage for StreamTerminal's simplified output path surfaced two pre-existing concurrency bugs under -race, now fixed: - the output goroutine could race Connect's own stream-close write since nothing joined it before the handler returned - session.Instance.Started() read the started field without its lock while start() wrote it outside the lock Also adds a test pinning the WebSocket-vs-general-handler routing precedence for the StreamTerminal path, previously only documented in a comment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(terminal): close residual races flagged by PR #163 code review A /github:pr-ship code-review gate on PR #163 (4 parallel agents: testing, code quality, architecture, security) found that both concurrency fixes from the previous commit were real but incomplete: - StreamTerminal's output goroutine was bounded-and-abandoned on timeout rather than actually interrupted, so it could still call stream.Send() after the handler returned if the PTY read blocked past the 2s bound. Fixed by dup'ing the PTY fd so this goroutine gets its own independent *os.File/poll.FD, safe to set a real 250ms read deadline on without racing the instance's other PTY consumers (response stream, command executor) that share the original fd. - session.Instance.started was fixed at exactly the two sites this session's new test exercised, leaving ~30 other unguarded read/write sites across the package equally racy. Converted the field to atomic.Bool everywhere instead, which is race-free by construction rather than by per-site lock-discipline audit. Also: added a deterministic unit test for waitWithTimeout's timeout branch (previously only indirectly exercised), and exported ansi.IsCSIFinalByte as a byte-range twin to CSIFinalByteClass so pkg/analytics/escape_code_parser.go's manual byte-scanning CSI check (a 6th independent reimplementation the prior consolidation missed) now derives from the same shared source of truth. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(session): close remaining actor/i.mu synchronization gaps post-merge Follow-up to the main-merge commit: these edits were made while chasing -race failures during conflict resolution but landed after that commit's git add, so they weren't included in it. - Instance.Preview, InstanceStatusManager.GetStatus: read Status via Snapshot() instead of a bare/RLock-guarded field read, matching the pattern already used by GetStatus/IsActive/etc. Both were caught by go test ./... -race (transitionToLocked writes i.Status outside i.mu). - pkg/ansi: tighten csiRegex/csiFinalByteMin/csiFinalByteMax to unexported now that no caller outside the package needs them directly. - instance_fork_test.go, instance_workspace_test.go: fix two `started:` struct-literal fields / bare assignments that go vet flagged after the bool -> atomic.Bool conversion. - instance_timestamp_test.go: wrap the test instance in a LiveInstance so TestInstance_TimestampConcurrency's concurrent UpdateTerminalTimestamps calls are actually actor-serialized, instead of silently taking the no-actor synchronous fallback (send()'s documented behavior when liveInstance is nil) with zero cross-goroutine synchronization. - instance_test.go: TestFromInstanceDataWithMissingWorktree now uses ForceStatus instead of a bare `instance.Status = Paused` write, so the Paused() check after it doesn't read a stale cached Snapshot(). - instance_serialization.go: ToInstanceData now builds its snapshot via sendSyncErr instead of reading the cached Snapshot(). Storage.UpdateInstance and SaveInstancesSync mutate Instance fields (Tags, Category) directly and expect the next ToInstanceData() call to see it; Snapshot()'s cache only refreshes when an actor command republishes it, so those callers were reading stale data (TestStorage_UpdateInstance, TestStorage_SaveInstancesSync). sendSyncErr gets a fresh buildSnapshot(s.inst) while still serializing against any concurrently-running actor commands. - instance_approval.go: simplify snap.ReviewState.TimeSince* to snap.TimeSince* (staticcheck QF1008 — ReviewState is embedded in InstanceSnapshot). - session_service_stream_terminal_test.go: wire session.NewRegistry + SetRegistry, matching server/dependencies.go's production wiring, so CreateSession actually wraps the instance in a LiveInstance during the test. go test ./... -race: 0 data races (verified across multiple full-suite runs). * fix(server): make PTY fd duplication cross-platform for windows/amd64 build syscall.Dup (used by StreamTerminal to give its reader goroutine an independent PTY fd) doesn't exist on Windows, breaking the release build matrix (windows/amd64) added by .github/workflows/build.yml — the failure first surfaced on PR #163's post-merge CI run, caused fail-fast cancellation of the whole Build matrix. Move the dup behind a dupPTYFile helper, split by build tag like the existing execSyscall precedent (exec_unix.go/exec_windows.go): syscall.Dup on Unix, windows.DuplicateHandle (golang.org/x/sys/windows, already a direct dependency) on Windows. Verified go build ./... for darwin/{amd64,arm64}, linux/{amd64,arm64}, and windows/amd64. Also investigated two other CI failures from the same run: - TestActorNoLeak/TestActorStopIdempotent (goleak "unexpected goroutines" from TestMain's own background helpers) — could not reproduce locally (isolated run or full `go test ./session -race`), consistent with a CI-timing-sensitive flake in this newly-merged upstream test, not something introduced here. - A TmuxSession.ptmx data race (GetPTY's unguarded read racing closePTYAndAttachCmd's unguarded close/reconnect) reproduced locally under -race -count=15 on TestStreamTerminal_SendsRawOutput. Pre-existing in session/tmux (t.ptmx is read/written without synchronization in ~15 call sites in tmux.go); fixing it properly needs a TmuxSession-level mutex refactor, out of scope for this merge-conflict PR. Flagged as a known follow-up, not fixed here. * fix(session): stop TestActorNoLeak/TestActorStopIdempotent flaking on unrelated leaks Reproduced the CI-only failure (2/2 remote runs) locally by constraining GOMAXPROCS=2, which changes goroutine scheduling enough to surface it — a plain -race/-count run on this fast dev machine never hit it. Two contributing issues, both fixed: 1. TestApprove_FromHibernated_Succeeds and TestApprove_AllSourceStatuses's Hibernated->Active case call Approve(), which (via transitionToLocked) dispatches resumeFromHibernationLocked's real Start()/StartController()/ StartSessionDriver() work in a background goroutine. StartSessionDriver's ticker loop has no external cancellation and runs for driverTotalTimeout (tens of minutes) — these tests never stop it, so it leaks for the rest of the test binary's life. Fixed by pre-setting driverRunning (the same CAS guard TestStartSessionDriver_Idempotent already relies on) so StartSessionDriver is a no-op; the tests aren't exercising driver dispatch, just the state transition. 2. The deeper issue: TestActorNoLeak/TestActorStopIdempotent used a bare process-wide goleak.VerifyNone(), which asserts "no goroutine is alive anywhere in the process" rather than these tests' actual claim ("Stop() joins the goroutine THIS test started"). In the large session package's shared test binary, ANY earlier, unrelated test that leaves its own background goroutine running (a ResponseStream poller, an exec.Cmd wait, etc.) trips this on unrelated grounds, nondeterministically depending on run order and scheduling — confirmed locally: after fixing #1, a -count=8 stress run still surfaced two more pre-existing unrelated leaks (MangleCorrelator.StartEviction, os.Process.pidWait) before either test assertion even reached the actor-specific logic. Auditing and fixing every leak source across this large, pre-existing test package is a separate, much larger effort - fixed the actual fragility instead: goleak.IgnoreCurrent() baselines the goroutine set before the actor work, so pre-existing goroutines from other tests are excluded and only a genuine actor-goroutine leak fails the test. Verified: GOMAXPROCS=2 go test ./session -race -v -count=8 — 0 failures (was reproducing on every run before this fix). * fix(scanner): register 3 missing backlog GitHub RPCs in methodToID tools/scanner's TestMethodToIDCompleteness failed in CI (Run scanner unit tests step of PR #163's Test job): SearchGitHubRepos, ListGitHubIssues, and ImportGitHubIssue exist in backlog.proto (and have real, if currently unimplemented/stubbed, server handlers - see the separate TestSearchGitHubRepos_*/TestListGitHubIssues_* skips) but were never added to proto_scanner.go's methodToID feature-registry map. Landed via the same upstream fork-sync merge as the actor-pattern refactor; nothing in this PR added these RPCs, just the first CI run to reach this check since the merge. Ran make registry-generate to regenerate the per-feature JSON files this map drives (docs/registry/features/backend/backlog/{search-github-repos, list-github-issues,import-github-issue}.json), which also cleaned up a stale duplicate (top-level DeleteBacklogItem.json, superseded by the already-committed backlog/delete-item.json from PR #159's merge - unrelated to this fix, just picked up by the same regenerate pass). registry-diff divergence back to 0.0%. * fix(scanner): keep GitHub RPC feature IDs stable to avoid a registry-diff false positive Follow-up to 94ce8333f: mapping SearchGitHubRepos/ListGitHubIssues/ ImportGitHubIssue to new kebab-case backlog:* IDs relocated their registry files (docs/registry/features/backend/backlog/*.json), which two things then treated as brand-new, untested RPCs: - The "Check new RPCs have tests" CI gate diffs registry files against origin/main by path; a relocated file is indistinguishable from a genuinely new one there. - SearchGitHubRepos and ListGitHubIssues already have real, committed tests (server/services/backlog_github_rpc_test.go) and tested:true/testIds on origin/main under their old top-level path/id — relocating them would have discarded that (loadExisting only preserves tested/testIds/ lastModified when the output *path* is unchanged). Fixed by mapping these three to their own method name (matching ScanProto's pre-existing fallback when methodToID has no entry), so the generated id/path is byte-identical to what's already committed. Restored SearchGitHubRepos.json/ListGitHubIssues.json/ImportGitHubIssue.json from origin/main and re-ran make registry-generate: markerFound correctly recalculates to false (no // +api: marker exists — these RPCs are still unimplemented stubs, that part hasn't changed), while tested/testIds/ lastModified are preserved via loadExisting's existing-file logic. ImportGitHubIssue.json (tested:false on both branches, no marker either side) comes out byte-identical to origin/main; the other two only change markerFound (true, which was stale, to false) and lastModified, with tested:true and their real testIds untouched - so neither trips the "new RPC has no test" per-file check (tested is true, and ImportGitHubIssue doesn't show up as changed in the origin/main diff at all). registry-diff: 0.0% divergence. cd tools/scanner && go test ./...: pass. * chore(tests): untrack tests/e2e/node_modules and fix flaky accessibility waits node_modules under tests/e2e was accidentally committed since it wasn't covered by .gitignore. Also swap networkidle waits in accessibility.spec.ts for element-based waits, since this app polls continuously and network activity never settles for the required window (matches alias.spec.ts). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: include tests/e2e/node_modules/ in .gitignore (missed in prior commit) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: bump otel past CVE-2026-41178, fix PreviewFullHistory race, refresh stale docs - security: bump go.opentelemetry.io/otel, /sdk, /trace from v1.43.0 to v1.44.0. v1.43.0 falls inside CVE-2026-41178's affected range (baggage-header-parsing DoS); the app registers the baggage propagator globally and otelhttp wraps the network- exposed HTTP server as the outermost layer, ahead of auth middleware. go mod tidy confirmed otelhttp v0.67.0 and otlptracegrpc v1.39.0 remain compatible with the bumped otel core, so those were left untouched; golang.org/x/sys picked up a transitive bump to v0.45.0. go mod tidy also dropped an unused gvisor.dev/gvisor indirect require (checklocks is installed via `go install ...@latest` in the Makefile, not consumed as a module dependency). - fix(session): PreviewFullHistory() read i.Status directly, unsynchronized with transitionToLocked's writes (which happen inside the actor's own serialization, not under i.mu) - the same hazard already fixed on Preview() a few lines above. Extracted the shared check into a new previewBlocked() helper used by both Preview() and PreviewFullHistory() so the two can't diverge again. - docs(session): updated ReviewState's file-header and UpdateTimestamps doc comments, which still described the old "protected by Instance.mu" discipline. UpdateTerminalTimestamps (the sole call path) now runs inside the actor's i.send() closure instead of holding i.mu; the comments now describe that. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Tyler Stapler <tystapler@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
🤖 I have created a release beep boop
1.38.0 (2026-07-17)
Features
Bug Fixes
This PR was generated with Release Please. See documentation.