fix: Bosun table rendering under policy show#90
Conversation
feat(run): fun confirmation glyphs (🚢🪵 + ⚓️ when anchor, else ✅); honor SHIPLOG_CONFIRM_TEXT override refactor(run): simplify dry-run branch to single cmd_write call; dry-run still prints 'Would execute' and 'Would sign & append' previews
Summary by CodeRabbit
WalkthroughThis PR refactors shell command handling with improved error isolation, enhanced remote fetch error handling, environment variable containment via subshells, dynamic glyph-based confirmations, and new utility functions for error summarization and confirmation text generation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Poem
From: Linus Torvalds torvalds@linux-foundation.org Look, I'll be blunt. This is not a trivial refactor. You're messing with command isolation, error handling paths, and confirmation logic — the places where everything goes sideways when something breaks. The good:
The problems I see:
Bottom line: The architecture is sound, but the devil is in the details with shell scripts. This needs careful testing on the error paths — not just the happy path. I want to see:
This is good work, but don't let shell script hubris bite you. Shell is unforgiving. BunBun Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/common.sh (1)
160-162: Regex in environment variable validation is too restrictive.The character class
[A_Z_]only matches identifiers starting with the literal characters {A, _, Z}, rejecting valid names likeB_VAR,D_PATH,STAGING_ENV, etc. Change to[A-Z_]to match uppercase letters or underscore:- if [[ ! "$name" =~ ^[A_Z_][A-Z0-9_]*$ ]]; then + if [[ ! "$name" =~ ^[A-Z_][A-Z0-9_]*$ ]]; then
♻️ Duplicate comments (1)
lib/commands.sh (1)
20-58: Duplicate logic: sanitize_remote_error mirrors shiplog_summarize_error.
sanitize_remote_error()(lines 20-58) is nearly identical to the newly-addedshiplog_summarize_error()in lib/common.sh (lines 11-35), with the only differences being variable names and return values stored in globals (SANITIZE_REMOTE_EXTRA_LINES,SANITIZE_REMOTE_TOTAL_LINES).This violates DRY. Refactor to reuse
shiplog_summarize_error()instead of duplicating logic. If global state tracking is needed, wrap the helper.-sanitize_remote_error() { - local raw_input normalized_input first_line="" second_line="" trimmed_line="" current_line total_lines=0 extra_lines=0 - - raw_input="${1:-}" - - SANITIZE_REMOTE_EXTRA_LINES=0 - SANITIZE_REMOTE_TOTAL_LINES=0 - - if [ -z "$raw_input" ]; then - printf '' - return 0 - fi - - normalized_input=$(printf '%s' "$raw_input" | tr -d '\r') - - while IFS= read -r current_line; do - case "$current_line" in - *[![:space:]]*) - total_lines=$((total_lines + 1)) - trimmed_line=$(printf '%s' "$current_line" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//') - case "$total_lines" in - 1) first_line="$trimmed_line" ;; - 2) second_line="$trimmed_line" ;; - *) extra_lines=$((extra_lines + 1)) ;; - esac - ;; - esac - done <<<"$normalized_input" - - SANITIZE_REMOTE_EXTRA_LINES=$extra_lines - SANITIZE_REMOTE_TOTAL_LINES=$total_lines - - case $total_lines in - 0) printf '' ;; - 1) printf '%s' "$first_line" ;; - 2) printf '%s; %s' "$first_line" "$second_line" ;; - *) printf '%s; %s; (+%d more lines)' "$first_line" "$second_line" "$extra_lines" ;; - esac -} +sanitize_remote_error() { + # Reuse shiplog_summarize_error and track line counts for backwards compatibility + local summary output + summary=$(shiplog_summarize_error "$1") + printf '%s' "$summary" + + # Count lines for global state (legacy support) + # TODO: Deprecate SANITIZE_REMOTE_* globals; use shiplog_summarize_error directly + local line_count=$(printf '%s\n' "$1" | sed '/^[[:space:]]*$/d' | wc -l) + SANITIZE_REMOTE_TOTAL_LINES=$line_count + SANITIZE_REMOTE_EXTRA_LINES=$(( line_count > 2 ? line_count - 2 : 0 )) +}Actually, this may already be a known pattern or used elsewhere. Can you confirm if
sanitize_remote_error()is intentionally separate or if it should be consolidated withshiplog_summarize_error()?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
lib/commands.sh(34 hunks)lib/common.sh(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{scripts/bosun,lib/**/*.sh}
📄 CodeRabbit inference engine (AGENTS.md)
Construct Bosun table output column-by-column using $'\t' concatenation; avoid embedding literal tabs in a single string
Files:
lib/common.shlib/commands.sh
{scripts/**/*.sh,lib/**/*.sh,test/**/*.bats}
📄 CodeRabbit inference engine (AGENTS.md)
Keep runtime and test dependencies to stock POSIX tools plus jq; avoid introducing other external binaries
Files:
lib/common.shlib/commands.sh
**/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
Shell scripts should pass ShellCheck in CI (fix issues rather than suppress unless documented)
Files:
lib/common.shlib/commands.sh
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to scripts/{**/*.sh,bosun} : For Bosun table output, build columns using $'\t' concatenation; do not embed literal tab characters in single strings
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:24:29.383Z
Learning: Applies to {scripts/bosun,lib/**/*.sh} : Construct Bosun table output column-by-column using $'\t' concatenation; avoid embedding literal tabs in a single string
📚 Learning: 2025-10-22T16:24:29.383Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:24:29.383Z
Learning: Applies to .shiplog/policy.json : Policy must be valid JSON and normalized (e.g., jq -S) to avoid churn; validate with `git shiplog policy validate`
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Default tests to SHIPLOG_USE_LOCAL_SANDBOX=1; set to 0 only when remote sandbox access is explicitly required
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-22T16:24:29.383Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:24:29.383Z
Learning: Set SHIPLOG_USE_LOCAL_SANDBOX=1 for tests (default) to avoid network clones; set to 0 only when explicitly needing the remote sandbox
Applied to files:
lib/commands.sh
📚 Learning: 2025-09-21T09:22:04.779Z
Learnt from: flyingrobots
PR: flyingrobots/shiplog#1
File: PLAN.md:3-8
Timestamp: 2025-09-21T09:22:04.779Z
Learning: In the Shiplog project, the plan is to migrate policy parsing from yq (currently implemented in the pre-receive hook) to jq for JSON-only policy format, because jq has better availability across systems compared to yq.
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-22T16:24:29.383Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:24:29.383Z
Learning: Applies to {scripts/bosun,lib/**/*.sh} : Construct Bosun table output column-by-column using $'\t' concatenation; avoid embedding literal tabs in a single string
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to scripts/{**/*.sh,bosun} : For Bosun table output, build columns using $'\t' concatenation; do not embed literal tab characters in single strings
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-22T16:24:29.383Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:24:29.383Z
Learning: Applies to {scripts/**/*.sh,lib/**/*.sh,test/**/*.bats} : Keep runtime and test dependencies to stock POSIX tools plus jq; avoid introducing other external binaries
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to {scripts,lib,test}/**/*.{sh,bash,bats} : Keep runtime and test dependencies limited to stock POSIX tools plus jq; avoid introducing other external binaries
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-22T16:24:29.383Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:24:29.383Z
Learning: Applies to test/**/*.bats : Never run Shiplog tests locally or directly; run the full suite via Docker using `make test` from the repo root
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-22T16:24:29.383Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:24:29.383Z
Learning: Always use `git push --force-with-lease` instead of `--force` when force-pushing (and only with explicit approval)
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-22T16:24:29.383Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T16:24:29.383Z
Learning: Never amend commits or force-push to shared branches without explicit user approval; amending local commits is encouraged
Applied to files:
lib/commands.sh
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Applied to files:
lib/commands.sh
🧬 Code graph analysis (1)
lib/commands.sh (2)
lib/common.sh (7)
die(55-58)shiplog_confirm_glyphs(40-53)shiplog_remote_name(108-120)shiplog_can_use_bosun(101-106)shiplog_bosun_bin(89-91)need(60-62)shiplog_summarize_error(13-35)lib/git.sh (2)
ref_anchor(14-14)current_tip(18-18)
🔇 Additional comments (9)
lib/common.sh (4)
11-35: New error summarization utility is well-designed.
shiplog_summarize_error()properly normalizes CRLF, filters whitespace, and outputs a compact summary. The fallback to empty string on zero lines is sensible. Error messages will now be properly condensed for display.
37-53: Glyph-based confirmation text is clean.
shiplog_confirm_glyphs()uses case-based dispatch to select emoji based on anchor status and completion status. Well-structured, no issues.
196-212: Improved bosun error isolation in input prompt.Captures bosun stderr to temp file, preserves exit code, summarizes on failure, and cleans up. Proper error handling with fallback to text input. One warning per session via
SHIPLOG_PROMPT_UI_WARNEDto avoid spam. Good.
241-257: Same solid error handling pattern in choice prompt.Consistent with input prompt changes: temp file capture, error summarization, one-time warning, cleanup. Both prompt functions now degrade gracefully when bosun fails.
lib/commands.sh (5)
1147-1182: ✅ Policy table rendering now uses real tabs/newlines — PR objective achieved.Lines 1158-1178 construct table rows using
$'\t'(ANSI-C quoting for actual tab bytes) and$'\n'(actual newlines), avoiding shell quoting pitfalls:rows+='Source'$'\t'"${POLICY_SOURCE:-default}"$'\n'This produces byte-for-byte what bosun expects: real tabs and newlines, not escaped strings. Passed correctly to
printf '%s'and piped tobosun table. Aligns perfectly with coding guidelines and learnings on Bosun table output.
713-763: Environment isolation via subshell is robust.Lines 717-735 execute
cmd_writein a subshell, preventing env mutations (SHIPLOG_BORING, SHIPLOG_ASSUME_YES, etc.) from leaking to caller scope. Output and error code properly captured to temp file and variable. Cleanup happens in both success (line 740) and error (line 759) paths. Good defensive pattern.
741-754: Glyph-based confirmation text integrates cleanly.Lines 744-754 derive anchor status, call
shiplog_confirm_glyphs(), and output custom confirmation. RespectsSHIPLOG_CONFIRM_TEXTenv override. Logic is straightforward: check if anchor ref exists, set flag, call helper, print result. No issues.
1399-1421: Trust show table rendering uses jq @TSV correctly.Line 1399 leverages jq's
@tsvfilter to generate properly tab-separated output, then pipes tobosun table. Signer rows built similarly with awk pattern. Consistent with corrected table rendering approach.
1028-1037: Help text formatting is consistent with changes.Heredoc uses
<<-'EOF'pattern and preserves readability. No quoting issues with embedded special characters.
| jq_output=$(printf '%s\n' "$json" | jq -r \ | ||
| '\n def _validate($msg; $value; $expected; $minlen):\n (try $value catch null) as $v\n | if $expected == "string" then\n if ($v | type) == "string" and ($v | length) >= $minlen then empty else $msg end\n elif $expected == "number" then\n if ($v | type) == "number" then empty else $msg end\n else\n empty\n end;\n def req_str($key):\n _validate("missing_or_invalid:" + $key; .[$key]?; "string"; 1);\n def req_enum_str($key; $allowed):\n (try .[$key] catch null) as $v\n | if (($v | type) == "string")\n and ($v | length) >= 1\n and (($allowed | index($v)) != null)\n then empty else "missing_or_invalid:" + $key end;\n def req_nested_str($msg; $path):\n _validate($msg; (try getpath($path) catch null); "string"; 1);\n def req_nested_int_ge($msg; $path; $min):\n (try getpath($path) catch null) as $v\n | if (($v | type) == "number")\n and ((($v | floor) == $v))\n and ($v >= $min)\n then empty else $msg end;\n [\n req_str("env"),\n req_str("ts"),\n req_enum_str("status"; ["success","failed","in_progress","skipped","override","revert","finalize"]),\n req_nested_str("missing_or_invalid:what.service"; ["what", "service"]),\n req_nested_int_ge("missing_or_invalid:when.dur_s"; ["when", "dur_s"]; 0)\n ]\n | map(select(length > 0))\n | .[]' 2>&1) | ||
| jq_status=$? |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Complex jq validation filter is harder to review but appears structurally sound.
Lines 2155-2157 embed a large jq program with nested function definitions for field validation. The multi-line format (with line continuations) preserves readability. Defines _validate() and specific checkers (req_str, req_enum_str, req_nested_str, req_nested_int_ge), then validates required trailer fields. Logic appears correct but would benefit from extraction to a separate .jq file for testability and maintainability.
Consider extracting the large jq validator to scripts/lib/trailer_validate.jq and referencing it via -f for better maintenance and testing, similar to scripts/lib/policy_validate.jq.
![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.
ℹ️ 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".
| cmd_write --env "$env" | ||
| ) >"$write_log" 2>&1 | ||
| write_status=$? | ||
|
|
||
| if [ $write_status -eq 0 ]; then |
There was a problem hiding this comment.
The refactor now runs cmd_write inside a subshell with >"$write_log" 2>&1 and deletes that file on success, so git shiplog run --dry-run no longer prints the preview entry that cmd_write emits. Prior to this change the dry‑run branch left stdout/stderr connected so the user could see exactly what would be written. With all output swallowed here, a dry run only prints the confirmation glyph and gives no visibility into the entry that would be recorded, defeating the purpose of the flag.
Useful? React with 👍 / 👎.
1 participant
Render table with real tabs/newlines to avoid quoting issues.