Route remaining compile stderr styling through console helpers#43586
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR finishes migrating remaining compile-related stderr styling call sites to the shared pkg/console formatting layer, avoiding direct styles.*.Render() usage while keeping TTY-aware behavior for stderr output.
Changes:
- Added stderr-focused console helpers (
FormatTableHeaderStderr,FormatErrorTextStderr) that route throughapplyStyleWithTTY(..., isStderrTTY). - Updated compile CLI outputs to use these helpers instead of direct style rendering in
compile_schedule_calendar.goandcompile_stats.go. - Added unit tests covering TTY vs non-TTY formatting behavior, plus WASM no-op stubs for the new helpers.
Show a summary per file
| File | Description |
|---|---|
| pkg/console/console.go | Adds stderr-specific formatting helpers for table headers and error-styled text. |
| pkg/console/console_wasm.go | Adds WASM stubs for the new stderr helpers. |
| pkg/console/console_formatting_test.go | Adds focused tests for the new TTY-gated formatting helpers. |
| pkg/cli/compile_stats.go | Routes oversized-workflow error styling through the new console helper. |
| pkg/cli/compile_schedule_calendar.go | Routes header/label styling through the new stderr table header helper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Low
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43586 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (60 additions across 5 files, threshold is 100). |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics (4 tests)
Verdict
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Route remaining compile stderr styling through console helpers
Clean refactor that correctly consolidates the two remaining direct styles.*.Render() call sites into the shared console layer.
Correctness ✅ — Both new helpers (FormatTableHeaderStderr, FormatErrorTextStderr) delegate through applyStyleWithTTY(..., isStderrTTY), exactly mirroring the pattern used by all other *Stderr helpers in this file. Behavior is identical to the old inline if isTerminal branches.
Behavioral edge ✅ — compile_stats.go: the previous non-TTY branch left fileSize unstyled entirely. The new path passes the already-formatted file-size string through FormatErrorTextStderr, which returns it unchanged on non-TTY — correct.
WASM stubs ✅ — Both new public functions have matching no-op stubs in console_wasm.go, consistent with every other stub in that file.
Tests ✅ — TestFormatTableHeaderWithTTY and TestFormatErrorTextWithTTY cover both TTY and non-TTY paths against the internal helpers, following the existing test structure.
Minor observation (non-blocking): compile_schedule_calendar.go still calls console.FormatInfoMessage (stdout-TTY variant) at line 237 for output sent to stderr. This pre-existing inconsistency is outside the diff scope but worth a follow-up to align with FormatInfoMessageStderr.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 51.8 AIC · ⌖ 5.81 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting only; no blocking issues, but a few naming and test-coverage items worth addressing.
📋 Key Themes & Highlights
Key Themes
- Naming inconsistency:
FormatErrorTextStderrusesTextwhile all other paired helpers useMessage, and it silently omits the✗prefix thatFormatErrorMessageincludes. This is the most likely source of future misuse. - Public API untested: New tests exercise private helpers (
formatTableHeaderWithTTY,formatErrorTextWithTTY) but the exported functions themselves have no coverage, leaving the TTY-check wiring unverified. - Single-render vs. dual-render behaviour: The old code called
styles.Error.Render()twice (prefix + name separately); the new code renders them in a single call. Worth a targeted TTY-mode assertion to confirm no visual difference. - WASM stub ordering: Two stubs inserted out-of-sequence in the otherwise ordered block — minor but adds diff noise.
Positive Highlights
- ✅ Clean elimination of duplicated inline TTY branching — the call sites are now clearly simpler.
- ✅
applyStyleWithTTYreuse is correct and consistent with every other helper in the module. - ✅ WASM stubs are present and correct (return identity / no-op).
- ✅ Behaviour preserved: non-TTY output stays plain text,
✗prefix still appears at the call site.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 81.9 AIC · ⌖ 8.48 AIC · ⊞ 6.7K
Comment /matt to run again
| t.Fatalf("formatTableHeaderWithTTY() = %q, want %q", result, "Header") | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
[/tdd] The tests directly exercise the unexported formatTableHeaderWithTTY and formatErrorTextWithTTY helpers — but the public surface (FormatTableHeaderStderr, FormatErrorTextStderr) is not tested at all. If a future change swaps the isStderrTTY function for a different TTY check, the public function will silently break while these tests still pass.
💡 Suggestion
Add smoke tests for the exported functions to lock in the wiring between public function → private helper → TTY check. They do not need to simulate a real TTY; even a non-TTY assertion is enough:
func TestFormatTableHeaderStderr_NonTTY(t *testing.T) {
// Stdout/stderr are not terminals in test, so plain text is expected
got := FormatTableHeaderStderr("Mon")
if got != "Mon" {
t.Errorf("FormatTableHeaderStderr() = %q, want %q", got, "Mon")
}
}This protects the public API contract separately from the internal implementation.
@copilot please address this.
|
|
||
| func FormatSuccessMessage(message string) string { return "✓ " + message } | ||
| func FormatInfoMessage(message string) string { return "i " + message } | ||
| func FormatTableHeaderStderr(text string) string { return text } |
There was a problem hiding this comment.
[/codebase-design] The two new WASM stubs are inserted in the middle of the alphabetically-ordered block, breaking the ordering. FormatTableHeaderStderr is at line 70 (between FormatInfoMessage and FormatWarningMessage) and FormatErrorTextStderr is at line 73 (between FormatErrorMessage and FormatLocationMessage). This is a minor but recurring source of diff noise and makes it harder to verify completeness at a glance.
💡 Suggestion
Group the two stderr-specific stubs together or keep them adjacent to their non-stderr counterparts. If the block is intended to be alphabetical, place them in alpha order; otherwise add a short comment separating stderr-specific stubs from the prefix-bearing ones.
@copilot please address this.
There was a problem hiding this comment.
Three issues to address before merging
The refactor direction is correct — removing direct styles.*.Render() calls from call sites is the right call. But the new console API has two structural deficiencies, and one test file only covers internal helpers while leaving public entrypoint wiring untested.
🔍 Blocking findings (3)
1. Incomplete API pairs (console.go) — FormatTableHeaderStderr and FormatErrorTextStderr are Stderr-only helpers with no stdout counterpart, violating the FormatXxx/FormatXxxStderr pairing that every other function in this file follows. Future callers writing to stdout will be forced to reach into styles directly.
2. Untested public entrypoints (console_formatting_test.go) — The new tests only inject fake TTY checkers into the unexported formatXxxWithTTY helpers. The exported FormatTableHeaderStderr/FormatErrorTextStderr functions are never called from any test, so incorrect TTY function wiring (e.g., isTTY instead of isStderrTTY) would be invisible.
3. Fragile single-render substitution (compile_stats.go) — Joining "✗ " + workflow name before a single Render() call is not semantically identical to the original two-call approach when styles.Error acquires layout properties (width, padding, margin) in the future. Needs a clarifying comment documenting the assumption.
🔎 Code quality review by PR Code Quality Reviewer · 192.9 AIC · ⌖ 5.68 AIC · ⊞ 5.4K
Comment /review to run again
| } | ||
| } | ||
|
|
||
| func TestFormatTableHeaderWithTTY(t *testing.T) { |
There was a problem hiding this comment.
Test gap: the public FormatTableHeaderStderr / FormatErrorTextStderr entrypoints are never called, so a wrong TTY function wired inside them would pass all tests silently.
💡 Details and suggested fix
Both new tests (TestFormatTableHeaderWithTTY, TestFormatErrorTextWithTTY) inject a fake ttyCheck directly into the unexported formatXxxWithTTY helpers. They do not call the exported FormatTableHeaderStderr or FormatErrorTextStderr even once.
The only thing the tests verify is that applyStyleWithTTY works when given any arbitrary TTY function. They do not verify that the public API delegates to isStderrTTY rather than isTTY. If someone accidentally wrote:
func FormatTableHeaderStderr(text string) string {
return formatTableHeaderWithTTY(text, isTTY) // wrong TTY function
}...the tests would still pass, and stderr output would silently use stdout TTY detection.
Suggested fix — add lightweight entrypoint tests that exercise the real functions, for example:
func TestFormatTableHeaderStderr_NonTTY(t *testing.T) {
// isStderrTTY returns false in a CI environment
got := FormatTableHeaderStderr("Header")
if got != "Header" {
t.Fatalf("FormatTableHeaderStderr() = %q; want plain text in non-TTY env", got)
}
}
func TestFormatErrorTextStderr_NonTTY(t *testing.T) {
got := FormatErrorTextStderr("boom")
if got != "boom" {
t.Fatalf("FormatErrorTextStderr() = %q; want plain text in non-TTY env", got)
}
}These work because CI never has a real TTY on stderr, guaranteeing the non-TTY path without mocking.
|
@copilot please run the skill, address unresolved review comments, and rerun checks once the branch is up to date. Unresolved review feedback to address:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed |
|
🎉 This pull request is included in a new release. Release: |
Two compile CLI paths were still calling
styles.*.Render()directly instead of using the shared console styling layer. This change removes those last direct render calls incompile_schedule_calendar.goandcompile_stats.gowhile preserving existing stderr/TTY behavior.What changed
console.FormatTableHeaderStderrconsole.FormatErrorTextStderrapplyStyleWithTTYpath rather than rendering styles directly at call sites.CLI call site cleanup
pkg/cli/compile_schedule_calendar.gostyles.TableHeader.Render(...)usage for the hour header row and day labels withconsole.FormatTableHeaderStderr(...).pkg/cli/compile_stats.gostyles.Error.Render(...)usage for oversized workflow rows withconsole.FormatErrorTextStderr(...).Behavior preserved
✗prefix behavior for large workflows.Compatibility