consumer-claude-code: mention-only triggers (propagated from template) - #75
Closed
assisted-by-ai wants to merge 6 commits into
Closed
consumer-claude-code: mention-only triggers (propagated from template)#75assisted-by-ai wants to merge 6 commits into
assisted-by-ai wants to merge 6 commits into
Conversation
Byte-identical cp from developer-meta-files/consumer-templates. pull_request and workflow_dispatch resolve to claude-code-action's agent mode, whose trigger is a non-empty 'prompt' input that this workflow does not pass, so those events could only spin up a runner and exit without calling the model - a green "Claude Code Review" check that reviewed nothing. Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Claude workflow now runs only for created comment events and filters for ChangesWorkflow and string helper behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
check_is_not_empty_and_only_one_line echoed the value it had just rejected straight to the terminal with printf. Callers reach that branch holding exactly the input they could not vouch for -- an api reply, a config field, a filename -- so the value may carry ANSI escapes or a U+202E right-to-left override and spoof what the operator reads about why it was rejected. A validator whose failure path is itself an injection vector undoes the reason it was called. This was the only such site in the file. check_is_alpha_numeric already reports through stecho on the same class of failure, so this is a consistency fix as much as a security one; line 175 interpolates with %q and is fine. Verified at byte level rather than by eye, since the sandbox transport sanitises its own output and hides the difference: with printf the stderr contained a literal 0x1B, with stecho it is '_'. Exit status, the accepted single-line case and the empty-value rejection are unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
The gate greps WHOLE changed files, so touching this file exposed three pre-existing findings. All three are now green. R-010 wanted a strict-mode preamble. It is a SOURCED library: setting 'set -o errexit' here would silently impose it on every caller's shell, and the file's own header already documents that the caller owns that choice. Waived with 'style-ok: no-strict' and the reason recorded, rather than adding options a library has no business setting. R-034: echo -> printf in the error trap. R-042 flagged the trailing newline in random_alpha_numeric as a decorative blank-line separator. It is not -- 'head --bytes' emits no trailing newline, so that printf terminated the value. The two obvious rewrites are both dead ends: R-030/R-031 requires the newline be spelled printf '%s\n' "", which is exactly what R-042 rejects, and printf '\n' "" is a shellcheck SC2182 error. Capturing the bytes and printing value and terminator in one statement sidesteps the knot and reads better. Behaviour verified unchanged for n=1/8/43: length, alphanumeric content, trailing newline, and the non-numeric rejection. Co-Authored-By: Claude <noreply@anthropic.com>
stecho is the wrong tool for echoing back a value that failed validation. It passes SGR escape sequences through -- verified on this host, TERM independent: given 'A\033[31mB\033[2J' it strips the clear-screen but keeps the colour code, because stdisplay defaults sgr to get_sgr_support(). SGR alone still spoofs, e.g. foreground matched to background to hide text in the very message explaining the rejection. It also imposes no length limit: a 5000-byte value produced 5001 bytes of output. sanitize-string escapes every ESC unconditionally and takes a maximum length. All three sites that echo a rejected value now use it, capped at 200 characters: check_is_not_empty_and_only_one_line, check_is_alpha_numeric and validate_safe_filename. The remaining stecho call reports "empty!" and interpolates no value. Verified: no ESC byte reaches stderr from any of the three, a 5000-byte multi-line value reports in 297 bytes, and valid inputs are still accepted. Co-Authored-By: Claude <noreply@anthropic.com>
read_integer_file's bounds checks fail open on leading-zero input. The value is validated as a string with ^[0-9]+$ and then compared inside ((...)), which reads a leading zero as octal. With file contents '08' and bounds 0..7 both comparisons abort with "value too great for base", each 'if' is therefore false, and the function returns 0 and emits 08 -- above the upper bound it was asked to enforce. '010' with bounds 9..100 is rejected although the caller means decimal 10. Reproduced, then fixed by forcing base 10 on both operands. validate_safe_filename dumped the rejected value to stdout, raw and uncapped, immediately above the sanitized stderr message -- so the sanitizing was moot. Confirmed on this host: a filename containing U+202E put the raw override bytes on stdout. It is also the wrong stream for a library, corrupting any caller that captures stdout, and od amplifies its input several-fold, defeating the length cap. The %q line is gone (%q runs under the caller's locale and passes multibyte sequences through, which is exactly why string_quote_safe exists) and the hex dump now goes to stderr, capped. random_alpha_numeric returned whatever it got. A failed read left the value empty and the function still exited 0, handing a caller an empty string where it asked for a secret. Length is now checked. Verified: 08/010/5 bound cases, stdout empty and stderr sanitized for a U+202E filename, and a 43-character result. Co-Authored-By: Claude <noreply@anthropic.com>
string_bsh_tests began with 'shellcheck "$0"'. Sourced -- which is how this library is used -- $0 is the CALLER, so it linted whatever invoked the library, or died when that was not a file, and the ERR trap aborted the function before a single assertion. Measured: 0 tests executed. Linting BASH_SOURCE[0] instead, and skipping when shellcheck is absent, brings the suite to 28 passed / 0 failed. With it running, the rest: - br_add_to_file dropped the final line of a file not ending in a newline: 'a\nb' produced only 'a<br />'. br_add, the same transform in memory, kept both. - validate_safe_filename measured length in CHARACTERS against a limit that is 255 BYTES, so 200 multibyte letters (600 bytes) passed a check meant to guarantee the filesystem will accept the name. - check_is_alpha_numeric accepted accented letters under a UTF-8 locale, because 'a-z' is resolved by collation. The test marked "unclear if this is possible" is now enabled and passes. check_valid_linux_user_- account_name got the same treatment: SYS_NAME_REGEX is meant byte-for-byte. - its empty-value branch was unreachable -- the regex rejects "" first -- so an empty value reported "Invalid character!". Checked before the regex now. - check_is_not_empty_and_only_one_line accepted a carriage return as one line, though CR ends a line and rewrites it on a terminal. Also fixes the comment claiming the read trims whitespace; with IFS= it trims nothing. - the six displaytime tests were vacuous: run_test passes a variable NAME, right for the name-based validators but not for a function taking a value, so every case was forced to T=0 and would have passed with the arithmetic entirely wrong. Replaced with output assertions, plus a non-numeric case. - a zero-argument call read $1 before the arity check, killing the caller's shell under nounset instead of returning 1. Also 'stcat --' so a filename beginning with a dash is an operand. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
usr/libexec/helper-scripts/strings.bsh (1)
194-200: 🎯 Functional Correctness | 🔵 TrivialHex-dump diagnostic includes a phantom trailing byte.
od -An -tx1 <<<"$value"uses a here-string, which appends a newline to$valuebefore feeding it tood. The resulting hex dump on stderr will always show a trailing0athat isn't actually part of the rejected value, which can mislead someone diagnosing the exact bytes that triggered rejection.♻️ Avoid the synthetic trailing newline
- LC_ALL=C od -An -tx1 <<<"$value" | head --bytes="$STRINGS_BSH_REPORT_MAX" >&2 + LC_ALL=C printf '%s' "$value" | od -An -tx1 | head --bytes="$STRINGS_BSH_REPORT_MAX" >&2🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usr/libexec/helper-scripts/strings.bsh` around lines 194 - 200, Update the hex-dump diagnostic around the od invocation to feed the exact contents of value without the here-string’s synthetic trailing newline, while preserving LC_ALL=C, the byte cap, and stderr output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@usr/libexec/helper-scripts/strings.bsh`:
- Around line 194-200: Update the hex-dump diagnostic around the od invocation
to feed the exact contents of value without the here-string’s synthetic trailing
newline, while preserving LC_ALL=C, the byte cap, and stderr output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a217de54-bc97-46d5-b1b4-bf66c2271ee4
📒 Files selected for processing (1)
usr/libexec/helper-scripts/strings.bsh
This was referenced Jul 29, 2026
Author
|
Superseded by #78: this branch is merged into Two changes were made to this work while consolidating, both at the maintainer's request:
CodeRabbit's two findings on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Byte-identical propagation of
consumer-claude-code.ymlfromdeveloper-meta-files/consumer-templates, paired with org-ai-assisted/developer-meta-files#165.pull_requestandworkflow_dispatchresolve to claude-code-action's agent mode, whose trigger condition is a non-emptypromptinput. This workflow passes no prompt, so those events could only spin up a runner and exit without calling the model:The result was a green "Claude Code Review" check on every PR that had reviewed nothing. Triggers are now comment-only, which is the only path that ever reached the model.
Merge order does not matter: the reusable's job-level
if:no longer acceptspull_request, so once #165 lands these repos stop producing misleading green checks either way. This PR removes the pointless runs entirely.Generated with assistance from Claude Code.
Summary by CodeRabbit