fix(cli): honour NO_COLOR in manager warning, add --no-color flag - #848
lakshya-dhariwal wants to merge 2 commits into
Conversation
|
Thanks @lakshya-dhariwal for your contribution to Failproof AI! 🙌 We'd love to discuss your PR and welcome you to our community. Discord: https://discord.befailproof.ai/ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe CLI now removes ChangesColor output controls
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The CLI color-control change appears ready to merge after normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 3 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
__tests__/hooks/no-color.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. bin/failproofai.mjsESLint skipped: the matched ESLint configuration already failed (missing-dependency). src/hooks/manager.tsESLint skipped: the matched ESLint configuration already failed (missing-dependency).
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit hops through terminal light, Comment |
Description
Fixes #688. One note up front: by the time I got here, the
policieslisting itself was already routed throughpaint()/chip()intui.ts, so the 16 hardcoded sites this issue describes had shrunk to exactly one - the duplicate-scope warning insrc/hooks/manager.ts, which still printed\x1B[33m…\x1B[0mregardless ofNO_COLOR. This PR finishes the job rather than pretending the listing was still broken:src/hooks/manager.ts: the warning now usespaint(!process.env.NO_COLOR).warn(...)(the install-prompt.ts pattern), so no hardcoded escapes remain in the file.src/hooks/tui.ts: newapplyNoColorFlag(args)helper - setsNO_COLOR=1and strips--no-colorfrom argv, so no subcommand's flag parser ever sees it.bin/failproofai.mjs: calls it once, right after argv is read; documents--no-colorin the top-level help footer and inpolicies --help.__tests__/hooks/no-color.test.ts: covers the TTY gate (colorsEnabledfalse under NO_COLOR), the painter emitting zero ESC bytes under NO_COLOR, the flag helper's strip-and-set behaviour, and a regression lock assertingmanager.tsnever reintroduces a hardcoded escape.I verified the help screens stay within the layout budget the help-index test enforces: index goes 25 to 26 lines (cap 30), max column stays 78 (cap 80). I measured by rendering
helpScreenwith the real specs, since this environment has no bun to spawn the binary.Type of Change
Checklist
npm run lintpasses (eslint on the touched files - clean)npx tsc --noEmitpassesnpm run test:runpasses - ran the related suites instead:no-color.test.ts(new, 5 tests),tui.test.ts,tui-kit.test.ts,manager.test.ts,manager-cloud-listing.test.ts- 152/152 green. Full test:run + build need bun, which I don't have here - flagging rather than ticking a box I didn't run. The bun-driven suites (help-index,unified-policies-surface) should exercise the help layout and the new flag in CI.npm run buildsucceeds - same bun constraint.Summary by CodeRabbit
--no-coloroption to disable colored output across commands. It can also be enabled withNO_COLOR=1, and is documented in the command-line help.