Repository navigation
terminal: honour FORCE_COLOR and keep control sequences off non-terminals - #198
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: dbb9622dbc
ℹ️ 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
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Changes requested at exact head dbb9622: confirmed the existing inline P2 and added public-API/PTY evidence in that same thread, rather than duplicating it. The colour-only override still activates clear-screen control output and rich arrow-key prompts. See the existing inline discussion for both consumers and controls.
No additional independent blocker found in the 14-file delta. Focused Release builds are warning/error-free; 1361 unit tests and 27 Spectre integration tests pass. CI is green but lacks the two reproduced consumer cases. Neutral, old-runtime-source and real-terminal positive controls are recorded. No product-code edits or merge.
…nals FORCE_COLOR (any non-empty value, even 0) now forces colour ahead of NO_COLOR and TERM=dumb, the rule System.Console and the console logger apply on .NET 11. It asks for colour only: shell-integration marks and advanced progress are no longer written to a redirected stream because colour was forced there, and NO_COLOR and TERM=dumb still veto them. The ANSI gate split in two. IsAnsiCapableForColor keeps the old answer, plus FORCE_COLOR, and is what Spectre styling follows. IsAnsiCapableForTerminalSequences adds the requirement of a terminal, or an explicit decision by the application (AnsiMode.Always) or the hosted session.
4f963ee to
bfbc572
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bfbc572f00
ℹ️ 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".
Forced colour opened every ANSI consumer, not only styling. With FORCE_COLOR (and, since 0.12.0, CLICOLOR_FORCE) a redirected stream got the screen-clear sequence, and FORCE_COLOR=1 TERM=dumb on a terminal got arrow-key menus. The two answers now have their own names. IsAnsiCapableForColor decides styling; IsAnsiCapableForControlSequences decides whether the terminal may be driven: colour on, plus a terminal behind the stream without NO_COLOR or TERM=dumb, or an explicit AnsiMode.Always, or a hosted session. Screen clearing, advanced progress, rich prompts, the pager's Auto mode, ITerminalInfo.IsAnsiSupported and shell-integration marks use the second; palettes use the first, with the hosted capability fallback so that a handler allowed to draw also gets colours. OutputOptions.IsAnsiEnabled becomes IsColorEnabled, and the environment is read in one place (TerminalEnvironmentClassifier.ResolveColorOverride). IsAnsiCapableForTerminalSequences(OutputOptions) stays, with the colour answer it gave in 0.12.0, because a shipped Repl.Spectre 0.12.0 calls it; a test pins its signature by reflection. Shell-integration marks check redirection before ANSI, so forced colour on redirected output reports "off (OutputRedirected)" again.
Spectre animates Progress and Status when its console is both interactive and ANSI. The console's ANSI follows the colour answer, so forced colour turned the animation on in a redirected output, and on a terminal under NO_COLOR or TERM=dumb. A console that is coloured but may not drive the terminal is now not interactive: live displays print static lines there, and SpectreInteractionHandler, which reads the same rule, leaves prompts to Repl's text prompts. An uncoloured console stays interactive, so prompts on an injected IAnsiConsole keep working under "app | tee log" or NO_COLOR.
docs/output-system.md#ansi-detection is now the reference: one table for the colour precedence, the conditions for driving the terminal, and how hosted clients and Spectre follow them. The other pages (runtime channels, progress, shell integration, terminal metadata, the Spectre README) link to it instead of restating an order that had drifted. "Changed in 0.12.1" callouts in output-system.md and interaction.md list what changes for existing users, including ITerminalInfo.IsAnsiSupported, and ask to upgrade Repl.Spectre together with Repl.Core.
|
Addressing the changes requested on dbb9622: both reproduced consumers (screen clear on redirected streams, rich arrow-key prompts under A local review panel found the same colour-versus-control mismatch in more consumers; all are on the control gate now:
The behaviour changes for the release notes are listed in the PR description. Full suite on net10.0 and net11.0: 5571 total, 0 failed, 74 skipped; |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Approved at exact head c292411 against base 2f9f539. This supersedes my earlier request-changes review 5852093001: both manifestations of the colour/control mismatch are fixed and confirmed in the existing thread (4534439504).
Independent read-only review of the actual 30-file triple-dot diff, excluding inherited #196/#197 changes, found no additional concrete introduced P0-P2. Parent inspected the changed consumers/docs and executed the load-bearing public-API and real-PTY reproductions on both TFMs. Forced colour no longer clears captured streams or enables cursor-driven choice menus under TERM=dumb/NO_COLOR; capable-terminal controls still render rich menus. Core-only and properly registered Spectre services were both exercised. Legacy published Repl.Spectre 0.12.0 loads against the new Core and preserves the colour gate signature. The uncoloured TERM=dumb Spectre selection refusal was separately reproduced on the published baseline, not attributed to this patch.
Local validation: warning-clean Release builds of the affected test projects and external probe; 1373 unit + 54 Spectre + 47 selected integration tests on each TFM = 2948 passed, 0 failed/skipped. This is not a full-solution test run. The probe initially timed out its capable-terminal control because it waited for a label that core rich menus do not print; the external harness was corrected to recognize the actual ready prompt. Product sources were not changed.
Current-head CI is complete and green, including Linux/Windows/macOS builds/tests, Build/Test/Pack, container administration, documentation, shell/MCP/signal checks and analysis. Refreshed the completed packaging/container/Windows/macOS jobs and verified their test steps actually ran. No further rebuild was necessary after the independent result because head/base are unchanged. No merge or package publication performed.
Closes #180 and #181.
Behaviour
FORCE_COLOR(any non-empty value, including0) forces colour, ahead ofNO_COLOR, asSystem.Consoledoes on .NET 11 (ConsoleUtils.GetColorOverrideFromEnvironmentonrelease/11.0). The colour order isFORCE_COLOR>NO_COLOR>CLICOLOR_FORCE=1>TERM=dumb> redirection;AnsiModeand a hosted session's ownAnsiSupportstay above every variable.DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTIONis not honoured.TerminalAnsiCapability.IsAnsiCapableForColordecides styling;IsAnsiCapableForControlSequencesdecides whether the terminal may be driven: colour on, plus a terminal withoutNO_COLOR/TERM=dumb, orAnsiMode.Always, or a hosted session (stated support or capability flags).ITerminalInfo.IsAnsiSupported, shell-integration marks, and Spectre's interactivity (soProgress/Statusstop animating and Spectre prompts give way to Repl's text prompts where the terminal may not be driven).OutputOptions.IsAnsiEnabledis renamedIsColorEnabled; the environment is read in one place (TerminalEnvironmentClassifier.ResolveColorOverride).IsAnsiCapableForTerminalSequences(OutputOptions)stays with its 0.12.0 colour answer, because a shipped Repl.Spectre 0.12.0 calls it; a test pins its signature by reflection.Behaviour changes for the release notes
Also written as "Changed in 0.12.1" callouts in
docs/output-system.md#ansi-detectionanddocs/interaction.md#iterminalinfo.FORCE_COLORoutranksNO_COLORfor colour;NO_COLORkeeps its veto on terminal control.CLICOLOR_FORCE=1on redirected output no longer clears the screen or enables arrow-key menus there (0.12.0 did); it still colours.AnsiMode.Always, the screen clear now needs a terminal, as advanced progress already did.ITerminalInfo.IsAnsiSupported(public) now answers "may drive the terminal":falseon redirected output underCLICOLOR_FORCE=1,truewith a palette for the hosted client above. Handlers deciding on colour should testPalette is not null.IAnsiConsolefails there (ask throughIReplInteractionChannelinstead). Uncoloured consoles stay interactive (app | tee log,NO_COLOR).off (OutputRedirected): redirection is now checked before ANSI.Tests
Consumer tests written red first (
Given_ForceColorConsumers: 4 of 5 failed before the fix, including aCLICOLOR_FORCEscreen-clear leak present since 0.12.0; the hosted palette test failed before its fix).Given_ForceColorcovers the gates;Given_SessionAnsiConsoleandGiven_SpectreTerminalDetectioncover Spectre's interactivity rule, including theOutputOptionswiring underFORCE_COLOR,CLICOLOR_FORCE,NO_COLORandTERM=dumb. Rich prompts and the pager read the same gate but have no behavioural test: the runner's stdin cannot be a terminal.dotnet test --solution src/Repl.slnx -c Releaseon net10.0 and net11.0: 5571 total, 0 failed, 74 skipped (Windows);-warnaserrorclean; markdownlint clean.Docs
docs/output-system.md#ansi-detectionis the single reference (colour table, terminal-control rule, hosted clients and Spectre);runtime-channels.md,progress.md,terminal-shell-integration.md,terminal-metadata.md,interaction.md,comparison.mdandRepl.Spectre/README.mdlink to it.