t3748: Fix quality-debt from PR #128 review feedback#4052
Conversation
- Replace GNU-only sort -V with portable version_leq() for cross-platform version comparison (macOS/BSD compatibility) - Add missing-file guard in cmd_diff() to fail fast when subagent file is missing instead of silently proceeding - Strip YAML frontmatter before diffing to match the comment's stated behaviour (was comparing full files including frontmatter) - Add explicit return 0 to cmd_version, cmd_diff, cmd_help functions - Fix MD022: add blank lines around blockquote headings in humanise.md - Fix curly quotes example: Before now shows actual curly quotes - Add Humanise step to content.md pipeline stages (was missing) Closes #3748
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a new public subagent Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as humanise-update-helper.sh
participant Local as Local FS/Cache
participant Upstream as Remote Repo
User->>Script: cmd_check / cmd_version / cmd_diff
Script->>Local: read local subagent file & local version
Script->>Upstream: fetch upstream (use cache or git archive)
Upstream-->>Script: upstream content & version
Script->>Local: write cache + cache-version file
Script->>Script: strip_frontmatter(local, upstream)
Script->>Script: version_leq(local, upstream) -> compare
alt local == upstream
Script-->>User: "up-to-date"
else local < upstream
Script-->>User: "update available"
else local > upstream
Script-->>User: "local newer/anomalous"
end
opt cmd_diff
Script->>User: show diff (frontmatter removed) + upstream commit URL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 05:44:13 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/content.md:
- Around line 79-87: The manifest currently treats Humanise inconsistently—it's
listed as a stage (`Humanise`, `content/humanise.md`, `/humanise`) in the
pipeline section but remains omitted from the Quick Reference pipeline
(`Production → Distribution`) and is still declared under legacy tools in the
frontmatter `subagents:` block; update the Quick Reference to include the
Humanise stage in the correct order (e.g., Production → Humanise →
Distribution), remove or relocate `humanise` from the legacy `subagents:`
frontmatter so it appears under the active pipeline stages, and ensure the path
`content/humanise.md` and route `/humanise` are referenced consistently across
the manifest.
In @.agents/scripts/humanise-update-helper.sh:
- Around line 176-190: The compare_versions function returns 0 (same), 1 (local
older/upstream newer) and 2 (local newer/upstream behind), but the existing if
collapses 1 and 2; change the block that calls compare_versions with
upstream_tracked and upstream_latest to capture its exit status (e.g., call
compare_versions ...; rc=$?) and branch explicitly: if rc==0 print "Up to date"
and return 0; if rc==1 print the existing "UPDATE AVAILABLE" message and return
1; if rc==2 print a distinct warning that the tracked version is ahead of
upstream (mention upstream_tracked/upstream_latest) and return 2 so callers can
detect this state. Ensure you reference compare_versions, upstream_tracked and
upstream_latest when making the conditional branches.
- Around line 33-39: The version extraction pipelines in
.agents/scripts/humanise-update-helper.sh (the blocks that read the subagent
file and run grep/sed/tr to extract version: and upstream_version:) can fail
under set -euo pipefail when the YAML key is missing; update both extraction
blocks to follow the safe pattern used elsewhere in the script (i.e., make the
grep pipeline non-failing and provide a fallback), for example by appending "||
true" to the grep pipeline and then defaulting to "not found" if the pipeline
produced no output so the function (the version/upstream_version extraction)
always returns a string and does not cause the script to exit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d031987-0ef5-4fd8-8c13-ecbcf9a680fb
📒 Files selected for processing (3)
.agents/content.md.agents/content/humanise.md.agents/scripts/humanise-update-helper.sh
|
Dispatching fix worker to address CHANGES_REQUESTED review feedback.
|
- content.md: Promote Humanise consistently across manifest — moved from legacy subagents to pipeline stages in frontmatter, added to Quick Reference pipeline description, added to diamond pipeline diagram - humanise-update-helper.sh: Add fallback for missing YAML keys in get_local_version() and get_upstream_version() to prevent script exit under set -euo pipefail when version/upstream_version keys are absent - humanise-update-helper.sh: Handle compare_versions return code 2 explicitly via case statement — previously collapsed codes 1 and 2 into a single 'UPDATE AVAILABLE' branch, now distinguishes 'tracked version ahead of upstream' as a distinct warning
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 05:58:20 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
@coderabbitai review All 3 inline review comments from the initial review have been addressed in commit e2ba26f:
The previous re-review attempt was rate-limited. Requesting re-review to clear the CHANGES_REQUESTED state. |
|
Triggering a fresh review now to clear the 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.agents/content.md (1)
83-91:⚠️ Potential issue | 🟡 MinorHumanise is still described as a legacy tool later in this file.
These stage changes make
content/humanise.mda first-class post-production step, but the later legacy lists still include it at Line 358 and Line 402. That leaves the manifest describing Humanise both as a core pipeline stage and as legacy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/content.md around lines 83 - 91, Remove the duplicate legacy mentions of "Humanise" so it appears only once as the first-class post-production stage (referenced as content/humanise.md or "Humanise") in the manifest; search for and delete or replace the legacy entries that list Humanise in the later legacy lists (the trailing legacy section) so that only the stage list defines it, and ensure any references or cross-links point to content/humanise.md rather than a legacy label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/humanise-update-helper.sh:
- Around line 245-246: The sed expressions currently used (sed '1{/^---$/d};
1,/^---$/d') delete from line 1 through the first `---` anywhere, which removes
preamble when there is no frontmatter; change both invocations that operate on
"$subagent_file" and "$CACHE_FILE" (producing "$tmp_local" and "$tmp_upstream")
to only strip a YAML frontmatter when the file actually starts with `---` by
using a conditional range like sed '1{/^---$/,/^---$/d}' so deletion runs only
if line 1 matches `^---$`.
- Around line 248-255: The diff-display block currently masks real errors by
appending "|| true" to the diff/delta/colordiff pipelines; update the block that
checks command -v delta / colordiff and runs diff -u "$tmp_local"
"$tmp_upstream" so it temporarily disables errexit (set +e), captures the
command's exit code, re-enables errexit (set -e), and then treat exit code 1 as
expected ("files differ") but propagate any exit code >1 as an error (exit with
that code or return it). Apply this change to the three branches that call
delta, colordiff, or plain diff in the humanise-update-helper.sh diff-display
section so you only suppress the expected diff-exit-1 and not real failures.
- Around line 169-205: cmd_check() currently calls compare_versions() with
possibly "unknown" values from get_upstream_version() / fetch_upstream(), which
causes arithmetic errors in version_leq(); add a guard in cmd_check() before
calling compare_versions() to check if either ${upstream_tracked} or
${upstream_latest} == "unknown" and handle it explicitly (e.g., print a clear
warning/error message and return a non-zero exit status such as 3) instead of
invoking compare_versions(); reference the variables upstream_tracked and
upstream_latest and the functions compare_versions() / version_leq() when adding
the conditional so the script avoids passing "unknown" into version_leq().
---
Duplicate comments:
In @.agents/content.md:
- Around line 83-91: Remove the duplicate legacy mentions of "Humanise" so it
appears only once as the first-class post-production stage (referenced as
content/humanise.md or "Humanise") in the manifest; search for and delete or
replace the legacy entries that list Humanise in the later legacy lists (the
trailing legacy section) so that only the stage list defines it, and ensure any
references or cross-links point to content/humanise.md rather than a legacy
label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57cd38eb-41b5-43f7-9677-87f1fa07d760
📒 Files selected for processing (2)
.agents/content.md.agents/scripts/humanise-update-helper.sh
- Guard against 'unknown' version strings before compare_versions() to prevent arithmetic errors in version_leq() (return 3 with clear message) - Replace fragile sed frontmatter stripping with awk-based strip_frontmatter() that only strips when file starts with '---' - Distinguish diff exit 1 (files differ) from exit >1 (real error) instead of blanket '|| true' that masks failures - Remove duplicate Humanise entries from legacy tool lists (lines 358, 402) — now only listed as first-class pipeline stage at line 83
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 06:29:27 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
All 4 items from the round-2 CodeRabbit review have been addressed in commit 8f7650b:
ShellCheck passes clean (only SC1091 info for external source, expected). @coderabbitai review |
|
Good to hear ShellCheck is clean (SC1091 for external 🧠 Learnings used✅ Actions performedReview triggered.
|
|
|
@coderabbitai review All review feedback from both rounds has been addressed and confirmed in inline comment replies. Requesting fresh review to clear the CHANGES_REQUESTED state. Round 1 (3 items) - addressed in commit e2ba26f:
Round 2 (4 items) - addressed in commit 8f7650b:
All CI checks pass. SonarCloud Quality Gate passed (0 new issues). |
marcusquinn
dismissed
coderabbitai[bot]’s stale review
All 3 review items addressed in commit e2ba26f. CodeRabbit confirmed fixes in inline replies.
|
🧠 Learnings used✅ Actions performedReview triggered.
|
marcusquinn
dismissed
coderabbitai[bot]’s stale review
All 4 review items addressed in commit 8f7650b. CodeRabbit confirmed fixes in inline replies.
marcusquinn
added
code-reviews-actioned
1 participant
Summary
Addresses all unactioned review feedback from PR #128 (humanise subagent introduction), filed as quality-debt issue #3748.
Changes
Shell script (
humanise-update-helper.sh):sort -Vwith portableversion_leq()function using bash arithmetic — fixes cross-platform compatibility on macOS/BSD wheresort -Vis unavailablecmd_diff()— previously could proceed silently when the subagent file was missing, masking errorsdiff -uwas comparing full files including frontmatterreturn 0tocmd_version,cmd_diff,cmd_helpper project shell conventionsMarkdown (
humanise.md):>lines around blockquote headings in "Title Case in Headings" section"...") to demonstrate the pattern, "After" shows straight quotesDocumentation (
content.md):Review feedback sources
Closes #3748
Summary by CodeRabbit
New Features
Documentation
Chores
Cleanup