Add OSC 133/633 shell-integration marks in interactive mode#42
Draft
carldebilly wants to merge 28 commits into
Draft
Add OSC 133/633 shell-integration marks in interactive mode#42carldebilly wants to merge 28 commits into
carldebilly wants to merge 28 commits into
Conversation
ConsoleReplInteractionPresenter carried private env-var sniffing (WT_SESSION, ConEmuANSI, TERM_PROGRAM, TMUX, TERM prefixes) that the upcoming shell-integration marks would have had to duplicate. Move the logic verbatim to an internal TerminalEnvironmentClassifier shared by terminal-specific emitters. - No behavior change: existing advanced-progress tests pass unmodified. - Promote EnvironmentVariableScope to a shared test helper. - Add focused classifier tests (tmux, screen, WT, ConEmu, tmux-wrapped). Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
- New TerminalCapabilities.ShellIntegrationMarks flag (1 << 5). - TerminalCapabilitiesClassifier recognizes vscode identities and infers the marks flag for wezterm, iterm, ghostty, windows terminal, and vscode. ConEmu keeps ProgressReporting but never gets marks (it has no OSC 133 support). - TerminalEnvironmentClassifier gains IsKnownShellIntegrationTerminal (WT_SESSION, TERM_PROGRAM=vscode/WezTerm, multiplexers excluded) and IsVsCodeTerminal. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
- ShellIntegrationMarkEmitter: OSC 133 lifecycle marks (A/B/C/D) with an OSC 633 backend under VS Code, including the 633;E command-line report with spec-compliant escaping (backslash, semicolon, control chars). A phase state machine guarantees one D per prompt cycle; gating mirrors advanced progress (passthrough, ANSI, redirection, Auto/Always/Never). - Public surface: ShellIntegrationMode, TerminalIntegrationOptions, and UseTerminalIntegration extensions on CoreReplApp and ReplApp. - ReplOptions carries the integration options internally; null keeps the feature disabled (opt-in). Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
InteractiveSession now drives a ShellIntegrationMarkEmitter through the prompt cycle: A before the prompt text, B before the line read, 633;E (VS Code backend) plus C after a non-empty commit, and a single D per cycle owned by the loop. - Exit codes now flow to the D mark: DispatchInteractiveCommandAsync, ExecuteWithCancellationAsync, and ExecuteInteractiveInputAsync return the exit code instead of discarding it (help renders 0/1, matched commands surface ExecuteMatchedCommandAsync's computed code, context navigation 0/1, route failures 1, ambient errors 1). - Cancelled commands report 130 (128+SIGINT) alongside the existing Cancelled. message. - Aborted cycles (Escape, EOF, empty commit) emit D without an exit code and skip C, matching the FinalTerm aborted-command form. - Ambient commands, ambiguous prefixes, and failures all run inside the C..D region because C is emitted by the loop before dispatch. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
- New docs/terminal-shell-integration.md: marks, modes, gating, backend selection, exit-code conventions, hosted sessions. - Cross-links from configuration-reference (TerminalIntegrationOptions), interactive-loop (lifecycle positions), and terminal-metadata (capability gating). Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d489825c8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address review feedback on the shell-integration marks: - Auto mode and 633-backend selection now respect the session boundary: hosted sessions rely solely on the client-advertised capability and terminal identity; WT_SESSION/TERM_PROGRAM describe the terminal the server runs in and no longer leak marks (or the VS Code dialect) to remote clients that never advertised support. - A failed interactive 'complete' ambient command now propagates HandledError so the command-end mark reports exit code 1 instead of decorating the failure as success. - Regression tests for both: server-env suppression, backend selection under a vscode server env, and the complete-failure exit code. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6bee0faa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address second-round review feedback: - Protocol-passthrough routes run interactively no longer get output marks: the input is pre-resolved before opening the output region, and the cycle is abandoned silently (no D after the payload; the next prompt-start implicitly closes it on the terminal side). - A dispatch that throws (e.g. history with a non-numeric --limit) now emits D;1 before the exception propagates, so the terminal never keeps an unterminated command segment. - A failed ambient help render (unknown output format) propagates HandledError and reports D;1, matching the non-ambient --help path. - Committed-input handling extracted to ExecuteCommittedInputAsync to keep the session loop within analyzer limits. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
@codex please review |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c066a8366
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Third-round review feedback: - Enablement and backend are re-resolved at each prompt start instead of frozen at session start, so hosted clients that advertise capabilities mid-session (Telnet TTYPE, @@repl:* control messages) get marks from the next cycle. Environment variables are still only consulted when no hosted session is active. - 633;E now escapes spaces too: the VS Code contract requires escaping characters 0x20 and below, so multi-word command lines round-trip. - A passthrough route invoked with --help only renders help; the pre-resolution now treats it as a normal lifecycle (C and D emitted). - Test helper uses a typed catch (code-quality feedback). Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7370c10dac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fourth-round review feedback: - A protocol-passthrough invocation that fails (validation error, unsupported host, or a thrown exception) never streams a payload: it now keeps its command-end mark and exit code instead of abandoning the cycle, preserving failure decorations. Only a successful passthrough dispatch abandons the cycle silently. - Hosted clients advertising ANSI purely through capability flags (no AnsiSupport override) no longer lose marks to the server console's redirection fallback; explicit opt-outs (AnsiSupport=false, AnsiMode.Never) still win. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4d945a3c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…sification Fifth-round review feedback: - Once a passthrough invocation dispatches, no command-end mark is emitted regardless of exit code or exception: handlers may emit raw bytes and then fail (e.g. Results.Exit(7)), and an exit code cannot prove the payload never started. This supersedes the previous validation-error decoration in favor of protocol integrity. - Passthrough classification now rules out ambient commands first: an ambient command sharing its token with a passthrough route (help, history, custom ambient) keeps the normal lifecycle. Token-only mirror helper cross-referenced with the ambient dispatch table. - Red-first regression tests for all three scenarios. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
@codex please review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Review-panel security finding (HIGH): EscapeCommandLine only covered backslash, semicolon, and chars <= 0x20. DEL (0x7f) and the C1 controls 0x80-0x9f passed through verbatim — an unescaped ST (U+009C), OSC (U+009D), or CSI (U+009B) in a pasted command line breaks out of the \x1b]633;E;... sequence and lets attacker-supplied text forge terminal control sequences on xterm.js/VTE (OSC 52 clipboard write, title spoof). - Extend the escape predicate and the SearchValues set to cover DEL and 0x80-0x9f (68 chars total). - Red-first regression test embedding DEL/ST/OSC/CSI and a >0x9f accent. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Review-panel findings (HIGH H2, MEDIUM M1/M2). H2 (flagged by architect/skeptic/security/quality/style): the passthrough pre-check parsed and resolved the input independently of dispatch, each calling ResolveActiveRoutingGraph separately — a concurrent routing-graph invalidation between them could make the pre-check say 'not passthrough' while dispatch matched a passthrough route, emitting a C mark + 633;E payload into a protocol stream. Now a single ResolveCommittedInput captures one graph snapshot + one parse into a CommittedResolution record reused by both the mark decision and dispatch (which no longer re-parses or re-resolves). Removes the double/triple parse besides closing the race. M2 (architect/skeptic/quality/style): IsAmbientCommandInvocation was a hand-maintained token mirror of the dispatch table. It is now the single classification authority — TryHandleAmbientCommandAsync guards on it, so the two can never disagree. M1 (operability/skeptic): the cleanup command-end write on the exception path is now best-effort (TryWriteCommandEndAsync) so a torn-down transport failing the D-mark write can no longer mask the original dispatch exception; host-shutdown OperationCanceledException closes the cycle with an aborted D (no exit code) instead of a failure D;1. - Red-first test: a failing mark write surfaces the original exception. - Characterization test: passthrough route with a leading global option. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Review-panel cleanup findings (style/skeptic/contract, LOW): - Precompute the constant A/B/C and D-without-code mark strings per backend (MarkSet) so the per-prompt path allocates no throw-away interpolated strings; only D-with-exit-code still formats. - Add exact ESC/BEL framing tests (\x1b]133;A\x07 ...) so a dropped introducer or a wrong terminator is caught, not just the ]133;X substring. - Delete the private EnvironmentVariableScope copy in the advanced-progress test; use the shared Repl.Tests.TerminalSupport one. - Replace the two duplicated CountOccurrences helpers with a shared TerminalMarks.Count over MemoryExtensions.Count. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Review-panel docs findings (quality/operability, MEDIUM): - Correct the passthrough section: A/B precede a passthrough command (written around the prompt before it is known), then E/C/D are suppressed and the cycle is abandoned regardless of exit code; the next prompt-start closes it. Note the ambient/--help exceptions. - Add a symptom -> gate troubleshooting table so the black-box enablement decision can be triaged without reading ResolveEnabled. - Document the disable paths: ShellIntegrationMode.Never per app, and NO_COLOR/TERM=dumb as the end-user escape hatch (with the all-ANSI collateral called out). Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
Ran a local multi-agent review panel (architect / security / skeptic / quality / style / operability / contract) and addressed the findings in four commits:
Full suite green (1050/1051, 1 pre-existing MCP skip). All behavioral changes are red-first. Deferred (own follow-up issue), noted for reviewers:
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ef23ee27d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
autocarl
reviewed
Jul 6, 2026
autocarl
left a comment
autocarl
left a comment
Contributor
There was a problem hiding this comment.
Adding the remaining material inline review notes. I skipped duplicate threads already present for Unicode escaping, prefix resolution, interactive --help, and prompt lifecycle closure.
autocarl
reviewed
Jul 6, 2026
autocarl
reviewed
Jul 6, 2026
… help
Round-7 Codex findings (P2).
- Escape (F1): IsForbiddenControl escaped every char >= 0x80 once the
builder path opened, so a legitimate char >= 0xa0 (e.g. 'é') following an
escapable char was wrongly escaped ('echo caf\xe9'). Bound the C1 clause
to <= 0x9f. Red-first test: escapable char before 'é'.
- Single snapshot (F3): ResolveUniquePrefixes re-fetched the active graph
internally, so prefix expansion and the route match used two snapshots —
reopening the TOCTOU the refactor closed. Add a graph-taking overload and
resolve prefixes against the captured graph in ResolveCommittedInput.
- Prefix-before-help (F2): resolve prefixes before deciding help so
'ser --help' scopes help to the resolved 'server', matching the
non-interactive path; carry the PrefixResolutionResult so the ambiguous
branch reuses it instead of re-resolving.
Characterization test: a prefix-abbreviated passthrough route (ser->serve)
is classified passthrough (no C/D) through the shared-graph path.
Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Round-7 Codex finding (P2, F4): once the prompt marks A/B are written, an exception or cancellation from the read path (or the history append) unwinds the loop with no matching D, stranding an open command segment on the terminal. Wrap the loop iteration so any failure emits a best-effort aborted command-end (no exit code) via TryWriteCommandEndAsync before rethrowing; the emitter's phase guard makes it a no-op when the cycle was already closed. Iteration body extracted to RunPromptCycleAsync to keep the loop within analyzer limits. Red-first test: an empty key queue makes the first read throw after A/B; the fix emits an aborted D before the exception propagates. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca132c4bf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Round-7 Codex finding (P2): ResolveCommittedInput resolved the route before DispatchInteractiveCommandAsync ran GlobalOptionsSnapshotInstance .Update, so module-presence predicates that read IGlobalOptionsAccessor during ResolveActiveRoutingGraph saw stale/default globals. Move the Update ahead of route resolution (matching the non-interactive path, verified: the CLI path finds an env-gated command from "--env prod"). Note: a full interactive observable (a per-command global re-gating a module) is additionally bounded by the routing-graph cache, which is populated at banner time with default globals and does not key on per-invocation global values — a pre-existing concern orthogonal to this PR, flagged for a separate follow-up. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
@codex please review |
Code-quality nit: the mask-prevention test helper caught the generic Exception; narrow it to InvalidOperationException (the ambient-failure type it asserts) so any other exception escapes as a genuine test failure instead of being swallowed. Return type narrowed accordingly. Claude-Session: https://claude.ai/code/session_01AopmWBr1VknLrUqerBEWsB
Member
Author
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
autocarl
reviewed
Jul 6, 2026
autocarl
left a comment
autocarl
left a comment
Contributor
There was a problem hiding this comment.
Re-checked current head b39b863.
Validation is green:
- GitHub checks are passing (release job skipped as expected).
- Targeted shell/terminal tests: 59 passed.
npx markdownlint-cli2on the modified docs: 0 errors.dotnet test src/Repl.slnx -c Release: 1053 total, 0 failed, 1052 passed, 1 skipped (expected MCP Inspector external-toolchain skip).
Two issues still need fixes before merge: interactive protocol passthrough does not enter the true passthrough scope, and hosted shell-integration capabilities remain sticky after a client downgrade. See inline comments.
| int exitCode; | ||
| try | ||
| { | ||
| (outcome, exitCode) = await DispatchInteractiveCommandAsync( |
Contributor
There was a problem hiding this comment.
Still reproducible on b39b863. The interactive loop only uses resolution.IsProtocolPassthrough to suppress shell-integration marks, then dispatches through ExecuteInteractiveInputAsync -> ExecuteMatchedCommandAsync instead of the protocol-passthrough path. A command-handler probe sees ReplSessionIO.IsProtocolPassthrough == false when run from interactive mode, while the same route sees true in CLI one-shot mode. This means .AsProtocolPassthrough() commands still do not get the stdout/stderr/session isolation contract in interactive mode. Please route this branch through the same ExecuteProtocolPassthroughCommandAsync / PushProtocolPassthrough contract (or a shared helper) and add the CLI-vs-interactive regression.
Member
Author
There was a problem hiding this comment.
Fixed in cc9a058. ExecuteProtocolPassthroughCommandAsync is now the single execution contract shared by both paths: it owns the hosted-capability guard (moved from the CLI call site, behavior unchanged there), the PushProtocolPassthrough scope, and the stdout/stderr isolation, and the interactive Routed dispatch routes passthrough matches through it instead of calling ExecuteMatchedCommandAsync directly.
Red-first regressions added:
- an interactive handler probing
ReplSessionIO.IsProtocolPassthroughnow observestrue(When_PassthroughCommandRunsInteractively_Then_HandlerObservesPassthroughScope); - a hosted interactive session rejects a passthrough route lacking an
IReplIoContextparameter with the same error as CLI one-shot (When_HostedInteractivePassthroughLacksIoContext_Then_HostedGuardRejectsLikeCli).
The pre-existing interactive passthrough tests were updated to declare hosted-capable handlers (IReplIoContext parameter) so their streamed-payload intent still holds under the guard.
| }; | ||
| } | ||
|
|
||
| private static bool SessionAdvertisesShellIntegration() => |
Contributor
There was a problem hiding this comment.
Still reproducible on b39b863. Auto re-reads ReplSessionIO.TerminalCapabilities per prompt, but TerminalIdentity updates still OR inferred flags into the session metadata. After a hosted client reports Windows Terminal and later dumb, the session still contains ShellIntegrationMarks, so the next prompt emits OSC 133 marks. Please separate explicit capabilities from identity-inferred capabilities, or replace/recalculate the inferred portion on identity change, and add a Windows Terminal -> dumb regression.
Member
Author
There was a problem hiding this comment.
Fixed in 4459be9. SessionMetadata now tracks IdentityInferredCapabilities — the subset of the effective flags earned only by identity inference. A new identity replaces that subset (WithTerminalIdentity), while explicit grants (overrides, control messages, actual resize/ANSI reports, wholesale sets) are demoted out of it (WithExplicitCapabilities / WithoutCapabilities) so a later downgrade cannot revoke them. Both identity write sites (the ReplSessionIO.TerminalIdentity setter and StreamedReplHost.UpdateTerminalIdentity) share the same helpers.
Red-first regressions added:
- Windows Terminal → dumb stops marks on the next prompt cycle (
When_HostedClientDowngradesToDumbMidSession_Then_MarksStopOnNextPromptCycle); - metadata-level inference replacement plus explicit-grant survival across a downgrade, and the hosted-transport identity path (
Given_ReplSessionIO).
…tion PR-review thread: TerminalIdentity updates OR-ed inferred flags into the session metadata, so a hosted client re-identifying from Windows Terminal to dumb kept ShellIntegrationMarks and OSC 133 marks kept flowing to a terminal that no longer claims support. SessionMetadata now tracks IdentityInferredCapabilities - the subset of the effective flags earned only by identity inference. A new identity replaces that subset (WithTerminalIdentity); explicit grants (overrides, control messages, resize/ANSI reports, wholesale sets) demote bits out of the inferred subset (WithExplicitCapabilities/WithoutCapabilities) so a later downgrade cannot revoke them. Both identity write sites (ReplSessionIO setter and StreamedReplHost.UpdateTerminalIdentity) share the same helpers. Red-first regressions: emitter-level WT->dumb (marks stop on the next prompt cycle), metadata-level inference replacement + explicit-grant survival, and the hosted-transport identity path.
…ontract PR-review thread: the interactive loop used its passthrough classification only to suppress shell-integration marks, then dispatched through ExecuteMatchedCommandAsync directly - bypassing PushProtocolPassthrough and the hosted-capability guard. A handler probing ReplSessionIO.IsProtocolPassthrough saw false interactively but true in CLI one-shot for the same route, and everything gated on that flag (advanced progress, styling) could interleave with a protocol payload. ExecuteProtocolPassthroughCommandAsync is now the single execution contract: it owns the hosted-not-supported guard (moved from the CLI call site, unchanged behavior there) plus the passthrough scope and stream isolation, and the interactive Routed dispatch routes passthrough matches through it. Unmatched-input handling extracted to keep the method within analyzer limits. Red-first regressions: an interactive handler observes IsProtocolPassthrough == true, and a hosted interactive session rejects a passthrough route lacking an IReplIoContext parameter with the same error as CLI. Existing interactive passthrough tests now declare hosted-capable handlers (IReplIoContext parameter), preserving their streamed-payload intent under the guard.
…ypass red Review finding: da19857 (apply parsed globals before resolving the committed route) landed without an executable red because the routing graph cached at banner time masks the interactive observable. The regression drives the cache-bypass path: a module gated on a per-command global (--env prod) plus a command calling InvalidateRouting, so the next committed line is the first resolution at the new cache version and the presence predicate reads the just-applied global. Autocomplete is off so nothing re-caches a stale graph between the two. Red was executed and observed by temporarily moving the snapshot Update back after route resolution (module absent, route failure); the committed order keeps it green. StaticWindowSizeProvider promoted from a private lifecycle-test helper to a shared IntegrationTests helper - the streamed host needs it to skip DTTERM VT probing, which never answers in tests.
Review finding (operability): ResolveEnabled folded 7+ inputs into a bool with no way to tell which gate decided, leaving black-box symptom guessing as the only triage path. The resolution now returns ShellIntegrationGate - an internal enum naming the deciding gate in its fixed evaluation order - recorded per cycle as ShellIntegrationMarkEmitter.LastGate. Deliberately internal and knob-free: no new public surface, no env vars; tests pin the tricky decisions (passthrough, hosted not-advertising vs enabled, not-configured vs mode-never) and the troubleshooting doc now states the exact gate order instead of calling the decision a black box. Red-first: the gate tests were written against the missing member.
Review finding (architect/quality/style): the resolution record was a union-by-nullable-fields consumed through five null-forgiving operators in production code, so a new kind or field drift would compile silently and NRE at runtime. Per-kind factories (Ambient/Ambiguous/Help/Routed) now own which fields are populated, and guarded accessors throw a descriptive InvalidOperationException on a kind mismatch instead of relying on `!`. No behavior change; existing interactive-loop tests cover all four kinds.
Review finding (quality): the hosted-ANSI fallback added for marks (a4d945a) was never applied to the OSC 9;4 progress gate, so the two emitters diverged on the exact bug it fixed - a hosted client advertising ANSI purely through capability flags (identity inference, control messages) got marks but no advanced progress. The fallback now lives in a shared TerminalAnsiCapability helper consumed by both emitters, and the presenter evaluates it per event instead of freezing IsAnsiEnabled at construction - aligning progress with the marks per-cycle re-resolution contract for mid-session advertisement. Red-first regression: hosted client, AnsiMode.Auto, capabilities inferred from a Windows Terminal identity, no AnsiSupport override -> OSC 9;4 emitted.
Review finding (contract): "one-shot / MCP stdio emits no marks" was only structural (the interactive loop owns the emitter); a refactor wiring marks into the one-shot path would have regressed protocol streams with no red test. Pins Always mode + capable hosted terminal + matched command: payload rendered, zero ]133;/]633; bytes.
…ures Review-panel LOW batch (style/quality/contract): - Loop-stable state (mark emitter, scope list, history provider, services, cancel handler) now travels as one PromptCycleContext instead of seven positional parameters through every per-cycle method. - Ambient command tokens promoted to shared constants consumed by IsAmbientCommandInvocation, TryHandleAmbientCommandAsync, and the non-interactive ambient path, so classification and dispatch cannot drift by editing one token list and not the other. - Read-only inputTokens parameters accept IReadOnlyList<string>. - TerminalIntegrationOptions documents that ShellIntegration is re-read each prompt cycle (runtime changes take effect on the next prompt) and no longer promises unimplemented future intents. - MarkSet perf comment corrected: 633;E also formats per call, not just D-with-exit-code.
…lings Review-panel LOW batch (style/contract/skeptic): - RunInteractiveSession and CaptureInteractiveRun now share one session-scope core, and the helper no longer sits between test methods. - NeutralTerminalEnvironment moved to shared TerminalTestEnvironments, removing the per-class copies. - MarkFailingWriter observes every write shape through a rolling-window detector and records Threw; the mask-prevention test asserts it fired, so the fault injection cannot silently stop and pass vacuously. - The mask-prevention assertion no longer couples to the exact internal exception message (type + stable "--limit" marker instead). - Magic Substring(index, 8) windows replaced by MarkPayloadAt (slice to the BEL terminator). - New mark test: an ambiguous prefix reports D;1 inside the normal lifecycle (previously untested exit-code branch).
Review-panel INFO findings: document that the VS Code 633;E report transmits the committed command line verbatim to the terminal (secrets typed as arguments included, matching VS Code shell-integration behavior for regular shells), and that any handler OperationCanceledException - not only Ctrl+C - decorates the command as interrupted (130).
| ? session.TerminalCapabilities | TerminalCapabilities.Ansi | ||
| : session.TerminalCapabilities & ~TerminalCapabilities.Ansi; | ||
| return session with { AnsiSupport = ansiSupported.Value, TerminalCapabilities = capabilities }; | ||
| var updated = ansiSupported.Value |
| false => session.TerminalCapabilities & ~TerminalCapabilities.Ansi, | ||
| _ => session.TerminalCapabilities, | ||
| true => session.WithExplicitCapabilities(TerminalCapabilities.Ansi), | ||
| false => session.WithoutCapabilities(TerminalCapabilities.Ansi), |
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
2 participants
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
Implements the shell-lifecycle slice of the terminal-integration layer: in interactive REPL mode, each prompt cycle is bracketed with FinalTerm semantic marks (OSC 133), or the VS Code shell-integration dialect (OSC 633, including the
Ecommand-line report) when the VS Code integrated terminal is detected. This unlocks command navigation, command-aware selection/copy, and success/failure decorations in Windows Terminal, VS Code, WezTerm, and other capable terminals.app.UseTerminalIntegration(options => options.ShellIntegration = ShellIntegrationMode.Auto)on bothCoreReplAppandReplApp. No marks are emitted without the call; existing apps are bit-identical (now test-pinned for one-shot runs too).Abefore the prompt,Bbefore the line read,633;E(VS Code, spec-compliant escaping) +Cafter a non-empty commit, and at most oneDper cycle with the real exit code — the interactive loop now stops discardingExecuteMatchedCommandAsync's computed code. Cancelled commands report 130 (128+SIGINT); aborted cycles (Escape, EOF, empty line) reportDwithout an exit code; dispatched protocol-passthrough cycles are abandoned with noDby design (no byte may trail a protocol payload)..AsProtocolPassthrough()routes now runs through the same execution contract as CLI one-shot (PushProtocolPassthroughscope, hosted-capability guard, stream isolation), so handlers observeIsProtocolPassthroughidentically in both modes.Autouses the newTerminalCapabilities.ShellIntegrationMarksflag (hosted sessions) or environment detection (WT_SESSION, TERM_PROGRAM=vscode/WezTerm), with multiplexers conservatively off; ConEmu is excluded (no OSC 133 support). Identity-inferred capabilities are recalculated on re-identification, so a Windows Terminal → dumb downgrade stops the marks. The deciding gate is recorded per cycle (internalShellIntegrationGate) and the gate order is documented for exact triage.TerminalEnvironmentClassifier(existing progress tests pass unmodified), and the hosted-ANSI capability fallback is now shared between marks and advanced progress (TerminalAnsiCapability).IReplInteractionChannelprompts emit nothing by construction.Public API added (all additive):
ShellIntegrationMode,TerminalIntegrationOptions,TerminalCapabilities.ShellIntegrationMarks, and the twoUseTerminalIntegrationextensions.Verification
dotnet test src/Repl.slnx -c Release— full suite green (1 pre-existing MCP inspector skip, also present on main).TerminalHarness.RawOutput), mark ordering, backend selection, 633;E escaping (\,\x3b, control chars incl. DEL/C1), double-D guard, tmux/passthrough/redirection gates, WT→dumb downgrade, CLI-vs-interactive passthrough contract, gate-reason diagnostics, one-shot no-marks pin, and full interactive-loop scenarios (success, error, unknown command, ambiguous prefix, help, empty line, Escape, exit, cancellation, VS Code).npx markdownlint-cli2on modified docs — 0 errors.Docs
New
docs/terminal-shell-integration.md(incl. troubleshooting gate order, 633;E privacy note, disable paths) plus cross-links fromconfiguration-reference.md,interactive-loop.md, andterminal-metadata.md.