docs(plan): create proposal for dx-error-handling#1
Conversation
- Consistent err/warn/info helpers with TTY + NO_COLOR support - Silent failure audit across 6 specific sites - Config validation, setup.sh enforcement, UX improvements Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile auto-reviews on PR open and on new commits pushed to the branch. Posting @greptileai re-review comments every iteration burns API budget without providing additional value. Manual re-reviews can still be requested with @greptileai mentions. Also adds a startup guard so rl fails fast with a clear message when USE_OPENSPEC=true but the openspec CLI is not installed.
- lib/common.sh: disable ANSI codes when NO_COLOR is set or stderr is not a TTY; add warn() and err() helpers for consistent messaging - bin/rl: list available commands on unknown-command error - rl-install: warn when jq is missing so dependency detection is gracefully skipped
There was a problem hiding this comment.
Pull request overview
This PR introduces an OpenSpec change proposal (proposal/design/tasks) for standardizing DX error handling across the rl toolkit, and includes initial implementation steps toward that plan (color gating + new messaging helpers, plus a few CLI/runtime behavior tweaks).
Changes:
- Add
NO_COLOR+ TTY-based color disabling inlib/common.sh, and introducewarn()/err()helpers. - Update CLI messaging/behavior in
bin/rl,libexec/rl-install, andlibexec/rl-loop(unknown command text, jq warning, OpenSpec CLI check, review-mode re-trigger logic). - Add OpenSpec change documentation for the DX error-handling initiative and update
IMPLEMENTATION_PLAN.mdaccordingly.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
openspec/changes/dx-error-handling/tasks.md |
New task breakdown for the DX error-handling migration and audits. |
openspec/changes/dx-error-handling/proposal.md |
New proposal describing goals/scope for standardizing error handling. |
openspec/changes/dx-error-handling/design.md |
New design detailing helper semantics and migration patterns. |
lib/common.sh |
Color disabling based on NO_COLOR/TTY + new warn()/err() helpers. |
bin/rl |
Unknown-command message updated to include an “Available” list. |
libexec/rl-loop |
Adds OpenSpec CLI availability check; changes review-mode re-trigger behavior. |
libexec/rl-install |
Warn when jq is missing; introduces (currently unused) HAS_JQ. |
IMPLEMENTATION_PLAN.md |
Rewritten to describe the DX error-handling initiative tasks/deliverables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Verify OpenSpec CLI is available when USE_OPENSPEC is enabled | ||
| if [[ "$USE_OPENSPEC" == "true" ]]; then | ||
| if ! (( $+commands[openspec] )) && ! npx openspec --version &>/dev/null 2>&1; then | ||
| echo "ERROR: OpenSpec is enabled but the CLI is not installed." | ||
| echo " Install: npm install -g @fission-ai/openspec" | ||
| echo " Or disable: set USE_OPENSPEC=false in .rl/config" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This OpenSpec availability check invokes npx openspec --version during startup when openspec is not on PATH. npx may attempt a network fetch/install (or be slow) which is a surprising side effect for a "verify" step. Consider using npx --no-install openspec --version (or checking node_modules/.bin/openspec) to avoid downloads, and route the failure through the new shared error helper once rl-loop is migrated (instead of raw echo).
| ```zsh | ||
| err() { ... } # Red "ERROR:" prefix, writes to stderr, exits 1 by default | ||
| warn() { ... } # Yellow "WARN:" prefix, writes to stderr, continues | ||
| info() { ... } # Dim prefix, writes to stderr, continues | ||
| ``` | ||
|
|
||
| **Color behavior:** | ||
| - Check `[[ -t 2 ]]` (stderr is a TTY) AND `[[ -z "${NO_COLOR:-}" ]]` (per no-color.org) | ||
| - When either condition fails, strip ANSI codes -- output plain text | ||
| - This is computed once at source-time into a module-level flag (e.g., `_RL_COLOR=true/false`) | ||
|
|
||
| **`err()` signature:** | ||
| - `err "message"` -- prints and exits 1 | ||
| - `err --no-exit "message"` -- prints but does not exit (for use in functions that handle their own flow) | ||
|
|
There was a problem hiding this comment.
The design describes err() as exit-by-default with a --no-exit flag, and also specifies an info() helper. The current implementation in lib/common.sh only defines warn()/err() and err() never exits or parses flags. Please either adjust this design to match what’s implemented in this PR, or update the implementation so the documented signatures/behavior are accurate.
| warn() { print -P "${Y}warning:${R} $1" >&2; } | ||
| err() { print -P "${ERR}${B}error:${R} $1" >&2; } | ||
|
|
There was a problem hiding this comment.
err()/warn() helpers don't match the behavior described in the proposal/design/tasks: err() currently never exits, there is no --no-exit handling, and info() is not implemented. This will cause migrated call sites to silently continue past fatal errors (or require every call site to remember exit 1). Please either implement the documented semantics (exit-by-default + --no-exit, plus info()), or update the docs/plan to reflect the intended behavior.
| warn() { print -P "${Y}warning:${R} $1" >&2; } | |
| err() { print -P "${ERR}${B}error:${R} $1" >&2; } | |
| info() { print -P "${C}info:${R} $*" >&2; } | |
| warn() { print -P "${Y}warning:${R} $*" >&2; } | |
| err() { | |
| local no_exit=0 | |
| if [[ "${1:-}" == "--no-exit" ]]; then | |
| no_exit=1 | |
| shift | |
| fi | |
| print -P "${ERR}${B}error:${R} $*" >&2 | |
| if (( no_exit )); then | |
| return 1 | |
| fi | |
| exit 1 | |
| } |
| # Colors — disabled when NO_COLOR is set or stderr is not a TTY | ||
| # --------------------------------------------------------------------------- | ||
| R=$'\033[0m' | ||
| B=$'\033[1m' | ||
| D=$'\033[2m' | ||
| C=$'\033[38;5;214m' # gold accent | ||
| G=$'\033[38;5;106m' # green | ||
| Y=$'\033[38;5;220m' # yellow | ||
| ERR=$'\033[38;5;196m' # red | ||
| if [[ -z "${NO_COLOR:-}" && -t 2 ]]; then | ||
| R=$'\033[0m' | ||
| B=$'\033[1m' | ||
| D=$'\033[2m' | ||
| C=$'\033[38;5;214m' # gold accent | ||
| G=$'\033[38;5;106m' # green | ||
| Y=$'\033[38;5;220m' # yellow | ||
| ERR=$'\033[38;5;196m' # red | ||
| else | ||
| R= B= D= C= G= Y= ERR= | ||
| fi |
There was a problem hiding this comment.
Color enablement is currently keyed off -t 2 (stderr). But these color variables are also used for stdout output (e.g., banners / normal print -P calls in other scripts). If a user redirects/pipes stdout to a file while stderr stays attached to a TTY, this will still emit ANSI color codes into the log file. Consider basing the global color vars on -t 1 (stdout), or splitting into stdout/stderr color flags so output stays plain when its destination is non-TTY.
| warn() { print -P "${Y}warning:${R} $1" >&2; } | ||
| err() { print -P "${ERR}${B}error:${R} $1" >&2; } |
There was a problem hiding this comment.
warn()/err() use print -P (prompt expansion) and only interpolate $1. Prompt expansion can mangle messages that contain % sequences (and can re-introduce ANSI sequences even when NO_COLOR is set), and $1 drops additional arguments if the caller passes more than one. Prefer a non-prompt-expanding print (print -r --) and use $*/$@ so the helpers reliably render arbitrary messages.
| warn() { print -P "${Y}warning:${R} $1" >&2; } | |
| err() { print -P "${ERR}${B}error:${R} $1" >&2; } | |
| warn() { print -r -- "${Y}warning:${R} $*" >&2; } | |
| err() { print -r -- "${ERR}${B}error:${R} $*" >&2; } |
| HAS_JQ=true | ||
| if ! (( $+commands[jq] )); then | ||
| warn "jq not found — skipping dependency detection from package.json" | ||
| HAS_JQ=false |
There was a problem hiding this comment.
HAS_JQ is set but never used later in the script. As written, dependency detection still calls jq ... 2>/dev/null || true regardless, so this flag adds dead state and can confuse future maintenance. Either remove HAS_JQ entirely, or use it to skip the jq-based detection paths when jq is missing (so you avoid running a suppressed "command not found").
| HAS_JQ=true | |
| if ! (( $+commands[jq] )); then | |
| warn "jq not found — skipping dependency detection from package.json" | |
| HAS_JQ=false | |
| if ! (( $+commands[jq] )); then | |
| warn "jq not found — skipping dependency detection from package.json" |
| # Re-trigger Copilot review only. Greptile auto-reviews on PR open and on new | ||
| # commits; manual re-reviews can be requested with @greptileai mentions. | ||
| local pr_num | ||
| pr_num=$(get_pr_number) | ||
| if [[ -n "$pr_num" ]]; then | ||
| local repo_full | ||
| repo_full=$(gh repo view --json nameWithOwner --jq '.nameWithOwner' 2>/dev/null || echo "") | ||
|
|
||
| if [[ "$USE_GREPTILE_REVIEWS" == "true" && -n "$repo_full" ]]; then | ||
| echo "Re-triggering Greptile review on PR #$pr_num..." | ||
| gh api "repos/$repo_full/issues/$pr_num/comments" \ | ||
| -X POST -f body="@greptileai re-review" >/dev/null 2>&1 \ | ||
| || echo "WARNING: Could not re-trigger Greptile review (non-fatal)." | ||
| fi | ||
|
|
||
| if [[ "$USE_COPILOT_REVIEWS" == "true" ]]; then | ||
| echo "Re-requesting Copilot review on PR #$pr_num..." | ||
| gh pr edit "$pr_num" --add-reviewer "@copilot" 2>/dev/null \ | ||
| || echo "WARNING: Could not re-request Copilot review (non-fatal)." | ||
| fi | ||
| if [[ -n "$pr_num" && "$USE_COPILOT_REVIEWS" == "true" ]]; then | ||
| echo "Re-requesting Copilot review on PR #$pr_num..." | ||
| gh pr edit "$pr_num" --add-reviewer "@copilot" 2>/dev/null \ | ||
| || echo "WARNING: Could not re-request Copilot review (non-fatal)." |
There was a problem hiding this comment.
This hunk removes Greptile re-triggering from review mode and changes behavior to "Copilot only". That’s a functional change unrelated to the PR’s stated DX error-handling scope, and it also conflicts with existing Greptile-provider docs/tickets in the repo. Either move this change to a separate PR, or update the PR description + any docs/plan in this PR so it’s clear Greptile re-triggering is intentionally being removed.
| ## Task 2: Migrate bin/rl and add available-commands on unknown command | ||
|
|
||
| - Replace `echo/print` error patterns with `err()`/`warn()` | ||
| - Unknown command case: list valid commands (create, install, loop, skills, migrate, release, update) |
There was a problem hiding this comment.
Task 2’s "valid commands" list omits version, but bin/rl currently supports version|-v|--version. To keep the task list actionable, please either include version here or explicitly call out that it should be excluded from the printed available-commands list.
| - Unknown command case: list valid commands (create, install, loop, skills, migrate, release, update) | |
| - Unknown command case: list valid commands (create, install, loop, skills, migrate, release, update, version) |
| # Implementation Plan: DX Error Handling Improvements | ||
|
|
||
| **Change ID:** `dx-error-handling` | ||
| **Branch:** `dx-improve-1` | ||
|
|
||
| ## Goal | ||
|
|
||
| Add Greptile as an opt-in PR review provider alongside Copilot. The review loop should fetch, triage, fix, reply to, and re-trigger reviews from either or both providers. | ||
| Standardize error/warning/info output across the rl toolkit, eliminate silent failures, and add config validation and better startup guidance. All changes are localized to existing files — no new architecture. |
There was a problem hiding this comment.
PR title/labeling suggests this is a docs/plan-only change, but the PR also includes functional code changes in lib/common.sh, bin/rl, libexec/rl-install, and libexec/rl-loop. Please update the PR title/description (and/or split the implementation into a separate PR) so reviewers can assess behavior changes with the right expectations.
Summary
Type of change
Checklist
zsh -n bin/rl libexec/rl-create libexec/rl-install libexec/rl-skills lib/common.sh libexec/rl-migrate libexec/rl-loop libexec/rl-fetch-reviews libexec/rl-reply-reviews libexec/rl-run-e2erl installon a target repo (if changing install/skills/config)Distributed vs dogfooding
resources/have been tested on a target repo