helper-scripts: add default_if_empty to strings.bsh - #78
Open
assisted-by-ai wants to merge 53 commits into
Open
Conversation
|
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:
📝 WalkthroughWalkthroughThe pull request updates comment-driven Claude review triggering, adds sanitization CLIs and streaming stdin handling, hardens shell helpers, introduces deterministic VirtualBox OVA normalization, expands CLI regression tests, and adjusts review and CI tooling. ChangesComment-triggered Claude review
Sanitization
Shell helpers
Reproducible OVA normalization
Additional CLI regression coverage
Review and CI tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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 |
This was referenced Jul 29, 2026
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
usr/libexec/helper-scripts/strings.bsh (2)
383-397: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueVerify the entropy source choice for a hard-fail path.
head --byteson atr -dc-filtered/dev/randomstream now hard-fails on a short read. On modern Linux/dev/randomno longer blocks indefinitely after initialization, so this is fine at runtime, but very early boot (pre-seed) can still block, which turns a helper used for secrets into a hang rather than the loud failure the comment promises. Consider/dev/urandom(identical quality post-init) or documenting the boot-time expectation.🤖 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 383 - 397, Update the entropy source used by random_alpha_numeric from /dev/random to /dev/urandom so the hard-fail short-read behavior cannot become an early-boot hang, while preserving the existing length validation and error handling.
82-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLine 125 is a no-op; drop it or make it conditional.
printf ... >/dev/nullcomputes a message and discards it. If it is meant as a debug hook, gate it on a verbosity variable; otherwise remove it.♻️ Proposed cleanup
- printf '%s\n' "$0: INFO: Target file '${target_file}' file_contents: '${file_contents}'" >/dev/null printf '%s\n' "${file_contents}"🤖 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 82 - 127, Remove the unconditional informational printf that writes to /dev/null at the end of the validation flow. If this message is required as a debug hook, make it conditional on the script’s existing verbosity mechanism; otherwise leave the final output as the file_contents printf.usr/lib/python3/dist-packages/sanitize_string/tests/sanitize_echo.py (1)
138-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a stdin-path case.
Every case passes operands, so the documented "read from standard input" branch (
sanitize_echo.pylines 94-98, including itsreconfigurecall) is entirely uncovered. A single test feeding stdin would pin the behavior most likely to break.🤖 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/lib/python3/dist-packages/sanitize_string/tests/sanitize_echo.py` around lines 138 - 154, Extend test_bad_max_length_is_rejected with a case that supplies invalid --max-length input while reading from standard input instead of passing an operand. Feed representative stdin data and assert the same empty stdout, help stderr, and exit code 1, covering the sanitize_echo.py stdin branch and its reconfigure call.
🤖 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.
Inline comments:
In @.github/workflows/consumer-claude-code.yml:
- Line 65: Update the job-level condition for the Claude comment workflow to
require both an `@claude` mention and the GitHub pull-request discriminator on
issue_comment events. Preserve the existing comment-body check while adding
github.event.issue.pull_request so ordinary issue comments cannot trigger the PR
workflow.
- Around line 55-56: Update the concurrency group expression in the workflow to
include a distinct segment based on whether github.event.comment.body contains
“@claude”, so ordinary comments cannot cancel an active human `@claude` review
while preserving cancel-in-progress behavior for matching requests.
In `@usr/libexec/helper-scripts/strings.bsh`:
- Around line 247-253: Update the diagnostic flow around the od/head pipeline so
the human-readable sanitize-echo reason is emitted first and the hex dump
remains the final value detail. Prevent SIGPIPE or pipefail from propagating to
the caller when truncating output at STRINGS_BSH_REPORT_MAX, while preserving
the existing byte-oriented, capped hex representation.
- Around line 211-216: Add an argument-presence guard at the start of
validate_safe_filename before expanding "$1", matching the sibling validators’
behavior: emit the established missing-name error and return 1 when no variable
name is supplied, while preserving the existing validation flow for provided
arguments.
---
Nitpick comments:
In `@usr/lib/python3/dist-packages/sanitize_string/tests/sanitize_echo.py`:
- Around line 138-154: Extend test_bad_max_length_is_rejected with a case that
supplies invalid --max-length input while reading from standard input instead of
passing an operand. Feed representative stdin data and assert the same empty
stdout, help stderr, and exit code 1, covering the sanitize_echo.py stdin branch
and its reconfigure call.
In `@usr/libexec/helper-scripts/strings.bsh`:
- Around line 383-397: Update the entropy source used by random_alpha_numeric
from /dev/random to /dev/urandom so the hard-fail short-read behavior cannot
become an early-boot hang, while preserving the existing length validation and
error handling.
- Around line 82-127: Remove the unconditional informational printf that writes
to /dev/null at the end of the validation flow. If this message is required as a
debug hook, make it conditional on the script’s existing verbosity mechanism;
otherwise leave the final output as the file_contents printf.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63afc91d-57d5-4fb2-aef0-8499fbe07629
📒 Files selected for processing (5)
.github/workflows/consumer-claude-code.ymlusr/bin/sanitize-echousr/lib/python3/dist-packages/sanitize_string/sanitize_echo.pyusr/lib/python3/dist-packages/sanitize_string/tests/sanitize_echo.pyusr/libexec/helper-scripts/strings.bsh
assisted-by-ai
pushed a commit
that referenced
this pull request
Jul 29, 2026
…llback CodeQL on PR #78: 1 high (py/tarslip) + 1 note (py/unused-import), both in this file, which arrived via the merge of #69. extractall(filter="data") is safe, but the 'except TypeError' fallback for interpreters predating that argument extracted with NO validation at all, so a member named '../x' or an absolute path escaped the temporary directory. The fallback now validates every member: the resolved path must stay inside the destination, and a link member is refused outright -- an OVA is a flat archive of .ovf/.vmdk/.mf files, so a link is never legitimate. Verified each hostile member type is refused ('../escaped.txt', '/abs.txt', and a symlink to /etc/passwd) and that a benign archive still yields its members. Also drops the unused 'sys' import. Co-Authored-By: Claude <noreply@anthropic.com>
assisted-by-ai
pushed a commit
that referenced
this pull request
Jul 29, 2026
Three of four taken. validate_safe_filename now rejects a missing argument, like its two siblings. It expanded "$1" unguarded, so a no-argument call aborted the caller with '$1: unbound variable' under nounset instead of returning 1. Reproduced. The hex dump moves AFTER the human-readable reason. It was printed first, which contradicts the reason-FIRST/value-LAST contract documented at the top of the file and buried the explanation under hex digits. consumer-claude-code.yml: - the concurrency group gains an '@claude' term. Concurrency is evaluated BEFORE the job-level 'if:', so an unrelated comment on the same PR joined the group of an in-flight review and cancelled it, while its own job was then skipped. - the job now requires a pull-request context. 'issue_comment' fires for ISSUES too, and an issue carries no PR for the reviewer to check out. Written as an alternation because github.event.issue.pull_request is absent for pull_request_review_comment, which is always a PR. NOT taken: the SIGPIPE half of the od|head finding. Not reproducible -- with a 200 KB value the pipeline still reports PIPESTATUS=(0 0) under pipefail, because GNU head --bytes drains its input rather than closing the pipe early. The ordering half of that same finding is fixed above. Co-Authored-By: Claude <noreply@anthropic.com>
Author
|
Both CodeQL alerts are fixed. py/tarslip (high) -- A deeper instance of the same class, which CodeQL did not flag, was found by another reviewer and fixed in cc09c0d: Verified refused: py/unused-import -- the unused |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@usr/lib/python3/dist-packages/sanitize_string/sanitize_string.py`:
- Around line 60-87: Update sanitize_stdin_loop to create and retain one
incremental markup parser for the loop, feeding each newly read untrusted_line
to it instead of repeatedly passing the entire pending_string to
markup_incomplete. Preserve the existing pending-buffer cap, sanitized output,
and remaining-count behavior, while allowing the parser’s internal state to
track incomplete constructs across lines.
In `@usr/lib/python3/dist-packages/sanitize_string/tests/sanitize_string.py`:
- Around line 131-155: The _run_stdin helper currently patches only sys.stdout
with a buffer, so the closed_stdout stderr scenario is not exercised. Update the
relevant test setup to patch sys.stderr with a working file-like object such as
io.StringIO(), and avoid replacing sys.stdout with the broken closed_stdout
MagicMock; preserve stdout capture for normal output assertions.
In `@usr/libexec/helper-scripts/vbox-ova-reproducible-normalize`:
- Around line 252-275: The repack function must reject any OVA member whose size
exceeds the USTAR limit before writing it, while retaining tarfile.USTAR_FORMAT.
Validate each member using the metadata from tar.gettarinfo (or an equivalent
size check), raise a clear error identifying the oversized member and limit, and
avoid producing or replacing the output archive when validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6854ff9e-183b-45b7-8730-44a2dccb0cf2
📒 Files selected for processing (6)
.github/workflows/consumer-claude-code.ymlusr/lib/python3/dist-packages/sanitize_string/sanitize_string.pyusr/lib/python3/dist-packages/sanitize_string/tests/sanitize_string.pyusr/lib/python3/dist-packages/strip_markup/strip_markup_lib.pyusr/libexec/helper-scripts/strings.bshusr/libexec/helper-scripts/vbox-ova-reproducible-normalize
🚧 Files skipped from review as they are similar to previous changes (1)
- usr/libexec/helper-scripts/strings.bsh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@usr/lib/python3/dist-packages/sanitize_string/tests/sanitize_string.py`:
- Around line 223-242: Strengthen test_stdin_probe_is_throttled in
usr/lib/python3/dist-packages/sanitize_string/tests/sanitize_string.py:223-242
to observe probe activity and assert probes remain throttled while the construct
is open, deriving payload sizes from STDIN_PROBE_INTERVAL_CHARS rather than
relying only on final output. Update the sibling test in
usr/lib/python3/dist-packages/sanitize_string/tests/sanitize_string.py:244-261
to observe sanitization writes and assert output is written after the closing
construct but before EOF; instrument probe/write calls or use a guarded stdin
iterator, with no direct changes required beyond these assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72fb2517-d930-482a-871a-afb0b03a1663
📒 Files selected for processing (6)
usr/lib/python3/dist-packages/sanitize_string/tests/sanitize_echo.pyusr/lib/python3/dist-packages/sanitize_string/tests/sanitize_string.pyusr/lib/python3/dist-packages/stdisplay/tests/stdisplay.pyusr/lib/python3/dist-packages/strip_markup/tests/strip_markup.pyusr/lib/python3/dist-packages/unicode_show/tests/unicode_show.pyusr/libexec/helper-scripts/git-review-driver.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- usr/lib/python3/dist-packages/sanitize_string/tests/sanitize_echo.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In @.github/workflows/consumer-claude-code.yml:
- Line 69: Update the workflow condition using github.event_name so
review-comment events proceed directly, while issue_comment events require
github.event.issue.pull_request to be present in addition to the existing
`@claude` check. Keep ordinary issue comments from invoking the reusable PR-review
workflow.
- Line 59: Update the workflow concurrency group expression to distinguish
events containing an `@claude` request from ordinary human comments, while
retaining the existing bot/human separation. Ensure non-request comments cannot
share a cancellation group with active `@claude` jobs, so cancel-in-progress does
not cancel an active review before the job-level condition runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0770722d-69f9-44be-a0a7-2afa7143ce61
📒 Files selected for processing (2)
.github/dm-consumer.yml.github/workflows/consumer-claude-code.yml
Author
|
Stacked an unrelated commit onto this branch, per the one- Context: sys-whonix burns ~60% CPU with no network (https://forums.whonix.org/t/sys-whonix-has-extreme-cpu-usage-before-an-internet-connection/23450 , QubesOS/qubes-issues#11013). Also adds This is only one amplifier. The root cause is sdwdate's |
Author
|
Stacked an unrelated CI fix onto this branch (one developer-meta-files commit The other master failure here, Same stale path fixed in pyte (#6) and sdwdate (#4). Generated with assistance from Claude Code. |
Author
|
The step-summary-emit fix from this branch was cherry-picked directly to master as The fix is confirmed working: on this branch the Generated with assistance from Claude Code. |
Collaborator
|
Some notes on the more major I made when merging this:
This has now been merged into my arraybolt3/trixie branch and fixed up. |
- hashlib.new(algos[algo_name]) crashed bandit 1.6.2-3, the apt build the
reusable bandit workflow installs on ubuntu-24.04: its hashlib_new plugin
calls .lower() on an argument it cannot resolve to a literal, so a dynamic
name raises AttributeError. xargs turned bandit's non-zero into exit 123
and the lane went red.
- Bind the four manifest constructors directly instead. The algorithm set is
closed and known at import time, so the name-string indirection bought
nothing, and the crash goes away because no hashlib.new() call remains.
- derive_mac / derive_cid: add '# nosec B303'. usedforsecurity=False already
states the intent, but bandit 1.6.2 predates that keyword and blacklists
sha1 unconditionally, so the lane failed on those two lines even once the
crash was gone. nosec is the per-repo suppression the reusable workflow
documents.
Verified in the sandbox against ubuntu:24.04 with the same apt bandit CI
uses, invoked exactly as CI invokes it ('bandit -ll -ii --'):
before: 'Bandit internal error running: hashlib_new ... NoneType', rc 1
after : no issues, no internal error, rc 0
Also rc 0 under bandit 1.7.10. SHA1/SHA256/SHA512/MD5 digests verified
byte-identical between the old and new construction.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- onion-time-pre-script (R-010): the file is dual-mode -- its testscript SOURCES it and relies on unset variables expanding empty -- so six directives at column 0 would leak errexit/nounset into the sourcing shell. Use the repo's guarded idiom (systemd-notify.bsh, get_writable_fs_lists.sh), which is the gate's documented escape for this shape, so R-010 auto-skips with no waiver. Fixes a latent bug in passing: 'set -e errtrace' enabled errexit and assigned 'errtrace' to $1 -- the option was never on. nounset/inherit_errexit deliberately NOT added: they change runtime semantics of a 500-line script. - git-review-driver.sh: printf '(none)' -> printf '%s' '(none)'; the four-%s symlink line precomposed into a variable per R-033. - strings.bsh duration formatting: %d over $(( )) results, already decimal strings, so '%s' is byte-identical. - strings.bsh is_integer: NOT rewritten. R-141 names it the mandated injection guard before an untrusted value reaches an arithmetic context, and its printf FAILING is the check -- '%s' would make it return 0 for every input, silently retiring the guard. A per-file printf-format waiver was rejected as well: it would blind all 46 printfs in a universally-sourced library. The gate now carves out a single-quoted %d format whose own command discards BOTH streams -- a validator, not output. Verified: gate exit 0; strings.bsh suite 28 passed / 0 failed / 0 skipped; onion-time-pre-script-testscript 11 passed / 0 failed / 0 skipped; pre-push-static style rules 144 passed. A/B of onion-time-pre-script's executed path is byte-identical with the same exit code. Canary: mutating is_integer's printf to '%s' fails the suite with "is_integer: 'abc' should be invalid". Each of the four carve-out guards has an assertion that fails when that guard is removed. Known, pre-existing, left alone: is_integer "" returns true (bash printf '%d' "" prints 0 and succeeds). Not exploitable for R-141, but it contradicts the doc comment; tightening it is a product decision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Behaviour deliberately unchanged (product decision): is_integer '' stays TRUE. The doc was the defect -- "same results as is_whole_number, but also allows negative numbers" pointed at a list that says empty input and leading spaces FAIL, and both actually pass. A guard R-141 mandates must not be described by a comment that contradicts it. Accept-set verified by running the function, not inferred from the source: - TRUE : 42 -7 +5 0 '' ' 42' 0x1F 0b101 010 99999999999999999999 - false: '42 ' ' ' '-' '1 2' 5.5 5,500 1.23e10 abc So it is strtoimax syntax, which is the point: a regex cannot reproduce that accept-set, and the printf FAILING is the check. Notable for callers: - '' is 0 in an arithmetic context and a syntax error as a subscript, so it cannot smuggle anything past R-141; a caller that needs a VALUE must reject it itself. - leading whitespace passes, trailing does not. - hex/binary/octal literals pass and the arithmetic context then evaluates them in that base. - out-of-range still passes: this is a syntax check, not a range check. Callers wanting "digits only" are pointed at is_whole_number. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Take ArrayBolt3's trixie rework wholesale (his versions win on every conflict): strings.bsh, onion-time-pre-script, vbox-ova-reproducible-normalize, plus the git-review tooling and keymap/lock-screen helpers. Conflict resolution: - strings.bsh, onion-time-pre-script, vbox-ova-reproducible-normalize: --theirs. - onion-time-pre-script-testscript: git rm (removed upstream). Style pass over the merged shell (gate-green): - R-030 displaytime printfs collapsed to single '%s' (output identical). - R-042 blank-line printf separators removed. - R-090 'command -v' path probe -> 'type -P'. - R-010 strict preamble waived where the upstream idiom omits it. - shellcheck optional checks (SC2250 braces, SC2248 quoting) auto-applied. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ng bugs
Implement every TODO/FIXME ArrayBolt3 introduced in the trixie merge and
remove the markers once resolved.
vbox-ova-reproducible-normalize:
- NAMESPACE: replace the well-known published DCE/MS example GUID with a
freshly generated random UUID so derived UUIDs cannot be precomputed from a
public constant.
- Keep latin-1 at all three sites (OVF, VMDK descriptor, .mf) as a lossless
byte<->codepoint codec; document why UTF-8 would gain nothing and risk a
decode abort. Only ASCII tokens are rewritten, so bytes round-trip.
- CID rewrite preserves leading indentation.
- Rename the 'match' variable ('match' shadows the soft keyword).
- --source-date-epoch falls back to the SOURCE_DATE_EPOCH environment
variable; an explicit flag still wins.
- Keep hashlib for manifest digests (no per-member subprocess).
onion-time-pre-script: anondate-set is forward-only by design, so it cannot
cause the negative stamp age this guard handles; correct the comment (sdwdate
and manual/NTP/VM-snapshot changes still can), keep the guard.
git-review-driver.sh: a 'git diff --stat' rc > 1 is a real error on two
materialized blobs, so fail loud instead of warning and pressing on.
tor_bootstrap_check.bsh: record where the privleap authorization lives rather
than leaving an open "consider moving it" question (relocating security config
across packages is risk for no functional gain).
Test edge cases (strip_markup, sanitize_string): add verified malicious cases
(entity-smuggled brackets, attribute-bearing tags, spec-compliant non-tags,
raw ANSI escapes, bidi overrides) with golden outputs confirmed against the
real sanitizers.
Fix two bugs agy flagged in the merged sanitize_string.py (both confirmed
empirically, both regression-tested):
- A zero max-length took a fast path that swallowed the '--newline' newline,
inconsistent with every other limit. Only fast-path when no newline is due.
- Removing the devnull redirect reintroduced a BrokenPipeError traceback from
the interpreter's shutdown flush ('... | head'). Redirect stdout to
/dev/null after a BrokenPipeError so the shutdown flush is a no-op, keeping
the immediate-exit behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The earlier '--newline' fix let a zero limit fall through the fast path, so '--no-block --newline 0' (and the blocking path) consumed a line of stdin before emitting the newline. A zero limit reads nothing: move the zero-limit handling after the stdout reconfigure and emit just the newline via sanitize_block(), returning without touching stdin. Add a stdin regression test asserting the newline is emitted and stdin stays unread; it fails on the old fall-through. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- File-not-closed: os.open(os.devnull) was never closed. Close the fd in a finally after dup2 duplicates it. - Empty-except: replace 'except OSError: pass' with contextlib.suppress(OSError). - Test import-and-import-from: import the module once and alias main from it, instead of also using 'from sanitize_string.sanitize_string import main'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
Resolved the TODO/FIXME markers left in the trixie reworkEach TODO/FIXME introduced by this rework was implemented and the marker removed. Summary of the dispositions: vbox-ova-reproducible-normalize
onion-time-pre-script
git-review-driver.sh
tor_bootstrap_check.bsh
Test edge cases (strip_markup, sanitize_string)
Two additional correctness bugs surfaced by review in the reworked |
…ed path
strings.bsh sourced its sibling wc-test.sh from a hardcoded absolute
/usr/libexec/helper-scripts/wc-test.sh. That path exists only once the package
is installed; every source-tree build runs the checkout, where it does not
exist, so the very first library load aborted with "No such file or directory"
and exit 1 -- breaking the image build (found via the VirtualBox reproducibility
build; it would break qcow2/iso too). Every sibling (benchmark.bsh,
get_colors.sh, git-review-*) already uses "${HELPER_SCRIPTS_PATH:-}", and
benchmark.bsh sources strings.bsh with that variable set, so strings.bsh must do
the same.
run-tests: add a regression guard that fails if any helper-scripts library
sources a sibling via a hardcoded absolute /usr/libexec/helper-scripts/ path.
Fails on the old code, passes now.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous "${HELPER_SCRIPTS_PATH:-}" fix was insufficient: strings.bsh is
sourced by tools that do NOT set HELPER_SCRIPTS_PATH (dm-upload-images, seen
failing late in an image build) and run from the checkout, so the empty variable
still expanded to the installed /usr path and aborted with "No such file or
directory". Resolve wc-test.sh relative to ${BASH_SOURCE[0]} so it works
installed AND in every source-tree build regardless of the caller's environment.
run-tests: strengthen the regression guard -- besides the static bare-absolute
check, load strings.bsh with HELPER_SCRIPTS_PATH set to a nonexistent path and
assert it still sources wc-test.sh (only a self-relative source survives). Fails
on both prior forms.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
'maxmem' is an optional external override, never set locally, so
'is_whole_number "${maxmem}"' aborted under nounset when unset. Default
it empty ('${maxmem:-}'): unset falls through to the meminfo path, and
the '$(( maxmem * 1000 ))' branch is only reached when maxmem is a
number. Sandbox-tested both paths (unset and maxmem=100).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rewrite for correctness: full strict preamble; source strings.bsh (the script calls str_replace but never sourced it); require >=2 args with a usage message instead of a nounset abort on bare $1; collect the file list as an array and iterate it quoted (SC2124/SC2086 -- also handles paths with spaces). Functionally tested: replaces text, usage on too-few args. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Complete apt-get-update, apt-get-update-kill-helper, get-user-list, pre-bsh-test to the full strict directive set. Fix surfaced debt: source has.sh + 'has' over 'command -v' (apt-get-update pair, R-090); echo -> printf '%s\n' (get-user-list, R-034); SC2317 disable on apt-get-update's SIGTERM/SIGINT trap handler; SC2250 braces. Sandbox-tested (has resolves, get-user-list lists users, no strict regression). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-test temp.sh: dual-mode self-test -- add inherit_errexit + shift_verbose inside the 'if was_executed' guard; document the design with a no-strict waiver (strict not at column 0) and a no-tmp-hardcode waiver (the '/tmp/user/<uid>' path is the EXPECTED TMPDIR under test). Brace SC2250 references. apt-get-update-sanity-test: convert 'set -e' to the full strict preamble (keeping the intentional 'set -x'). Sandbox-tested; no strict regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the full strict preamble to restart-tor, debug-kicksecure-enabled,
check-network-access, system-ready-check, system-ready-check-user,
try-wait-for-tor-service-running, terminal-wrapper-helper. Fix surfaced
debt: expand try-wait's case arms (R-070); SC2250 braces; guard
terminal-wrapper-helper's optional TERMINAL_WRAPPER_NO_COMMAND_ECHO /
TERMINAL_WRAPPER_NO_FINISH_MSG env vars ("${VAR:-}") against a nounset
abort. Sandbox-tested (terminal-wrapper-helper: no abort, runs the
command then 'sleep infinity' to hold the terminal).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…afe fragment
Strict preamble on settings_echo, settings_environment_file_update,
first-boot-skel (+${1:-} guard, rm -f -> safe-rm), sudo-tools-enable
(command -v full-path -> test -x), has_netvm_on_qubes,
detect-ipv6-enabled-in-kernel, apt-get-update-simulate (init lastpid
before the trap; SC2181; SC2317), get-password-status-list. SC2250 braces.
Root fix: tor_bootstrap_check.bsh guarded 11 bare '[ "${GATEWAY_IP...}" =
"" ]' defaults with ':-' -- they aborted under nounset, exposed when
settings_echo (now strict) sources the fragment. Fixes every nounset
caller. Sandbox-tested (settings_echo now emits the defaulted values).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cgroup
Add the full strict preamble (both self-invoke via '"$@"'). curl_exit_codes:
guard the empty-arg check ("${1:-}") so a no-arg call exits 1 instead of a
nounset abort; SC2250 braces. run-in-cgroup already used '${1:-}'.
Sandbox-tested: 'curl_exit_codes 7' -> the code-7 message; no-arg -> exit 1;
run-in-cgroup no-arg -> exit 1, no nounset abort.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convert bare 'set -e' to the full strict preamble (keeping 'set -x').
Make the tor_user / tor_group / tor_dir / client_onion_auth_dir defaults
nounset-safe ('VAR="${VAR:-x}"' instead of '[ -n "$VAR" ] || VAR=x').
SC2250 braces. Sandbox-tested: no nounset abort.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add the full strict preamble. Sourcing shim-signed-mok-setup, has.sh and secure_boot_enabled_check.bsh under nounset is safe (function definitions only, no bare top-level variable references). Sandbox-tested: no nounset abort. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convert bare 'set -e' to the full strict preamble. Source has.sh and use
'has' for the tput/curl/safe-rm/mktemp dependency checks (R-090). Replace
the 5 inline signal traps with named wrapper functions (R-051) --
'shutdown_sigint() { shutdown sigint; }' etc.; behaviour-equivalent ($?
and the signal argument are identical, $BASH_COMMAND is read inside
shutdown either way). SC2004 / SC2250 cleanup; SC2317 disable on the
trap-only handlers. A/B tested against the prior version: behaviour
identical (a file:// URL exits 110 in both).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convert bare 'set -e' to the full strict preamble. Source has.sh and use 'has' instead of 'command -v' (R-090). Build terminal_emulator_extra_args as an ARRAY throughout -- it was a string in some branches and an array in others -- and collapse the three-way final dispatch into one quoted call. Two behaviour bugs this exposed: - '$@' was unquoted in the final dispatch, so an argument containing whitespace was word-split and one containing glob characters was expanded against the caller's working directory. - The 'no supported terminal emulator' branch printed the error but did not exit; it then ran an empty command name, so the first argument was executed as a command (exit 127). nounset-safe initialisation for the base_name / app / extra_args variables and for the optional terminal_emulator_window_title input. Sandbox stub-tested, 13 cases with mocked emulators on a private PATH: qterminal, xfce4-terminal (title set / unset / empty), gnome-terminal (title set / unset), xterm, konsole (Qubes and non-Qubes), both x-terminal-emulator alternative paths, and the error path. 13 pass, 0 fail. The same suite against the pre-change file: 11 pass, 2 fail -- exactly the two bugs above. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Completes the standalone set. benchmark.bsh and source_folder.bsh are NOT
included: both are libraries -- pure function definitions with no
top-level statements -- that simply have no in-tree sourcer yet because
they are new. A strict-mode block in either would rewrite the shell
options of every future caller.
Real nounset traps fixed, each read on a path where only some branches
assign the variable:
- anondate-set: 'indent_style' is read by output_cmd() but assigned much
further down, so any earlier call aborted. Unset previously took the
non-indented branch, which 'false' reproduces.
- anondate-set: 'current_message' / 'old_message' are each assigned only
when their state file exists, then compared unconditionally. Unset
compared "" against "" and skipped systemd-cat; the defaults keep that.
- anondate-get / anondate-set: 'exit_code' is read by the 'trap ... EXIT'
handler but only assigned inside branches. Initialised defensively --
the handler already has an explicit "no exit code set yet" branch, and
an unset read inside an EXIT trap would mask the real status. Note this
one is defence, not a demonstrated abort on any path tested here.
Pre-existing array/string defects the gate surfaced:
- '$@' inside a string in anondate, anondate-get and anondate-set message
lines expands to one word per parameter; '$*' joins them, which is what
those messages mean (SC2145 / SC2124).
- anondate-tester iterated an unquoted "${cmd_item_list[@]}" and passed an
unquoted $@ through to anondate (SC2068).
policy-rc.d moves from '#!/bin/sh' to bash. It is EXECUTED by dpkg via
POLICYRCD, and bash is Essential in any Debian chroot it runs in. Its
'exit 101' is the policy decision and is unchanged -- verified.
Also: echo -> printf (R-034), safe-rm (R-120), 'has' instead of
'command -v' (R-090), SC2250 braces, and a file-wide SC2317 waiver on the
three anondate scripts, whose functions are reached only through the EXIT
trap and the option-parser dispatch.
Sandbox differential vs the pre-change scripts, comparing output AND exit
code with both versions staged at an identical path: 17 cases, all
identical -- including policy-rc.d still exiting 101.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bandit (B303, two sites in vbox-ova-reproducible-normalize): both hash calls already pass 'usedforsecurity=False' and carry comments explaining that collision resistance is irrelevant -- they derive a stable MAC suffix and content id for reproducible builds. bandit blacklists the sha1 call by NAME regardless, so each site now carries an inline '# nosec B303' with the reason, which the consumer-bandit workflow documents as the intended suppression mechanism. Verified: 'bandit -ll -r .' over the whole repo now reports no issues. sanitize-string: with no argument and 'sys.stdin' None -- pythonw, or a detached service -- the '--newline' flag was ignored and nothing at all was printed. The zero-limit branch immediately above goes out of its way to emit just the newline "as it does for every other limit", and echo with no arguments prints a newline too, so the stdin-None branch was inconsistent with both. It now emits the newline as well. That was a real behaviour bug, not a stale expectation: the dist-ai test 'test_no_argument_and_no_stdin_still_prints_a_newline' asserts exactly this and was failing in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes a lint regression I introduced in ec7efbb. Adding the stdin-None newline emission pushed main() to 51 statements, one over pylint's cap, and broke the lint job on all four distro images -- it had been passing. The zero-limit branch and the stdin-None branch now share one helper instead of repeating the same two lines, which removes the duplication and brings main() back under the limit. Verified locally: pylint 10.00/10, black clean, the in-repo pytest suite and the dist-ai sanitize-string suite both 15/15. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mypy caught that the new helper over-narrowed its parameter. The zero-limit call site is narrowed to int by the '== 0' test, but the stdin-None site still holds 'int | None', which is what sanitize_block itself accepts. Match that signature. Verified locally with the full lint stack this time -- mypy clean, pylint 10.00/10, black clean, pytest 15/15 -- rather than pushing and reading the result off CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… unicode
Two distinct false positives, both of which flagged an INNOCENT commit with the
same warning as a real attack. A scanner that cries wolf trains reviewers to
ignore it, so a false positive here costs more than a missed feature.
1. Hunk-header function context
-------------------------------
A hunk header ends with a funcname suffix -- '@@ -3,0 +4 @@ text' -- where
'text' is the nearest preceding line, which the commit did NOT touch. Once a
file contains hostile unicode, every later commit anywhere near it is flagged
for a character it neither added nor removed. '--unified=0' does not suppress
this; it only removes context LINES, not the header decoration.
The ranges themselves are pure ASCII, so they are kept and the decoration is
dropped.
2. Removal lines
----------------
The commit that DELETES a hostile character was flagged, because the diff line
'-dirty <U+202E>line' still contains it. The fix got the same warning as the
attack. Nothing a commit removes can appear in the resulting tree, so a removal
cannot be the attack.
Both file headers ('---' and '+++') are KEPT: a hostile FILENAME must stay
detectable.
Identity and message are now scanned SEPARATELY and unfiltered
--------------------------------------------------------------
The removal filter drops diff lines starting with '-', and a commit message
line may legitimately start with '-'. Feeding the message through that filter
would let hostile unicode hide behind a leading dash -- so the metadata comes
from its own '--no-patch' git show and is never filtered.
Testing
-------
dist-ai's check-ref-commits-for-unicode suite: 44 passed, 0 failed.
Against the pre-change tool the same suite reports 3 failures --
M:clean-not-flagged, M:only-dirty-flagged and M:removal-not-flagged -- so each
regression test genuinely exercises the fix.
The message-behind-a-dash guard is canaried too: routing the message through
the removal filter makes the tool exit 0 with "No unicode detected" on a commit
whose message carries U+202E, and M:dash-message-flagged catches exactly that.
This script was ALREADY bash with a strict preamble -- errexit, nounset,
errtrace, pipefail. It was missing the other two R-010 directives, which this
adds:
shopt -s inherit_errexit
shopt -s shift_verbose
What they do NOT add here, stated because the first version of this commit
claimed otherwise: a failing grub-mkpasswd-pbkdf2 was ALREADY caught. pipefail
makes the pipeline fail, and that is the command substitution's status, so the
assignment fails and errexit aborts. inherit_errexit governs errexit INSIDE a
subshell, which is a different case. Measured, not assumed -- behaviour is
identical with and without it.
Touching the file brought it into the gate's changed-file scope, which surfaced
pre-existing issues, all fixed:
- R-034: six 'echo' uses, including 'echo -n'. The blank-line ones take the
R-030 form 'printf "%s\n" ""', not 'printf "\n"'.
- SC2250 braces on user, grub_config_file, first_input, second_input,
password_hash.
- SC2154 on 'password', which get_password sets from the sourced
get_password.sh that shellcheck cannot follow. Documented at the use site.
- SC1091 on both sourced helpers, for the same reason.
Testing
-------
grub-pwchange is clean in an ISOLATED checkout. NOTE: the helper-scripts package
as a whole does NOT pass -- 27 other files fail shellcheck, and that is entirely
pre-existing. Measured by stashing this change and re-running: the same 27 fail
without it. Those are not touched here and are recorded separately.
The harness compares the GENERATED /etc/grub.d/44_password, not just exit codes,
since the file is the thing this script exists to produce:
3 pass, 0 fail
passwords match -> hash written same, DEADBEEF in the config
passwords differ -> refused same, no config
grub-mkpasswd FAILS -> no config same, no config
Canary: adding '|| true' to the hash pipeline -- simulating an unguarded
failure -- makes an empty hash reach the config, and the third case fails.
Four harness defects on the way, each one a case that would have proved nothing:
- 'timeout' was missing from the stub PATH, so both variants died before exec
and compared equal. A run carrying an 'env:'/'bwrap:'/root-guard line now
fails outright.
- the tool-symlink loop used '[ -n "${real}" ] && ln ...' as the last command in
the loop body. For a missing tool that returns non-zero, and under errexit it
aborted the whole setup -- so every tool AFTER the first missing one was
silently never linked.
- /usr/bin/awk is a symlink through /etc/alternatives/awk, and the harness
replaces /etc with a tmpfs, so the link dangled inside the sandbox. The links
are realpath-resolved now.
- the generated config embeds "$0", and the script runs from a per-variant copy,
so every case differed on the fixture path until it was normalised.
With the gate's --source-path fix (dist-ai 2c48caa) the 27 failures here dropped
to 7. These are those 7, and they were all genuine.
Two live nounset aborts
-----------------------
Both files carry the strict preamble, so these were real:
- lock-screen: XDG_SESSION_TYPE comes from the session manager and is simply
absent in a bare shell, a cron job or a broken session. Reading it bare
aborted before the '*' arm could report the unknown session type.
- anondate-set: 'debugging' is assigned nowhere in the tree.
A latent one
------------
curl-prgrs's curl_download reads '${header_download}'. Both in-file call sites
pass it as a per-call env prefix, but this is a SOURCED library and an external
caller need not. Defaulted to "false", which keeps the truncation check ENABLED
for such a caller -- the safe direction, since skipping it would let a short
download pass as complete.
A diagnostic that captured the failure and threw it away
---------------------------------------------------------
curl-prgrs's trap handler assigned 'last_err="${BASH_COMMAND}"' and never used
it, so the error said a signal had arrived but not what had failed. It is in the
message now.
Genuinely dead code, removed
-----------------------------
- anondate-get: anondate_tor_certificate_lifetime_set_file, a pure string
assignment read nowhere.
- kernel-file-detect: the initrd derivation. The identical two lines live in
initrd-file-detect, which is the script that actually outputs it; this copy
computed a value the script never prints.
Library outputs, documented not silenced
-----------------------------------------
tor_bootstrap_check.bsh sets 13 variables that ARE its interface --
tor_bootstrap_percent, tor_circuit_established_word, the consensus timestamps.
systemcheck's check_tor_bootstrap.bsh reads them and documents each with
'## sets: ...'. shellcheck sees one file, so an output looks unused. One
file-level disable, with that reasoning written down.
Same for TOR_ENABLED in onion-time-pre-script: check_tor_enabled_do assigns it
on its FIRST line as a fallback, so it is always set.
Reported, NOT fixed
-------------------
onion-time-pre-script guards tor_consensus_time_sanity_check with
if [ "${timesanitycheck_static_timestamp_based_failed}" = "true" ]
and NOTHING in this package assigns that variable. The comparison is always
false, so the early return never fires and the duplicate-output optimisation the
comment describes has never taken effect. Defaulted with ':-' so the comparison
is well defined, but not made to fire: that needs someone to decide WHICH check
should set the flag, which is a product decision.
Testing
-------
27 shellcheck failures -> 0. Gate exit 0 in an ISOLATED checkout.
The 20 that vanished were the gate's own path-resolution bug, fixed separately
in dist-ai and measured there against six other packages before landing.
The previous commit defaulted it at the 'case' and left the '*' arm's message
interpolating '${XDG_SESSION_TYPE}' bare. Under nounset the script therefore
still aborted on exactly the path the fix was for -- a bare shell, a cron job or
a broken session -- and never printed the "must be set to 'x11' or 'wayland'"
guidance that arm exists to give.
Declared ONCE before the case now, so both uses are covered and a third cannot
reintroduce it. That is the same reasoning as msgdispatcher's gui/cli: patching
the one site you noticed leaves the next one.
Found by codex, on my own fix.
Testing
-------
Gate exit 0 in an ISOLATED checkout.
4 pass, 0 fail
XDG_SESSION_TYPE UNSET reaches the message old ABORTS, new prints it
unknown value reaches the message unchanged
x11 still dispatches unchanged
wayland still dispatches (no error path) unchanged
The first case asserts the MESSAGE is produced, not that the case statement was
reached -- reaching the case was never the problem.
Canary: removing the declaration fails exactly that case.
Four fixture defects on the way, each one a case that would have proved nothing:
- 'env PATH=... --unset=X' takes '--unset=X' as the COMMAND. env parses options
only BEFORE the first assignment, so the UNSET case ran WITH the variable set
and cheerfully locked x11.
- the sed anchor required '${XDG_SESSION_TYPE}'; the old side spells it
'$XDG_SESSION_TYPE' unbraced, so the old driver extracted NOTHING, printed its
trailing marker and read as a pass. The anchor takes both spellings now, and
an empty extraction fails loudly as EXTRACTION_EMPTY.
- the stubs invented function names the script does not have; it calls
xscreensaver-command and swaylock directly and reads ${title} set earlier.
- the wayland arm CAPTURES swaylock's output into a variable rather than
printing it, so asserting on the stub's text was wrong. A successful lock is
the absence of the failure message.
timesanitycheck_static discarded timesanitycheck's exit code with '|| true', so timesanitycheck_static_timestamp_based_failed was never assigned by anything. Two things are gated on that flag, and NEITHER has ever happened: - tor_consensus_time_sanity_check returns early when it is "true", to avoid repeating a complaint the static check already made. That duplicate output has always been printed. - further down, a "true" flag routes to anondate_use -- the clock-recovery path for a system whose time is wrong by a lot. That path has never run. The second is the reason this is worth fixing rather than deleting: it is not a cosmetic optimisation, it is error recovery that was silently unreachable. timesanitycheck exits 0 when the clock is inside the shipped min/expiry window and 2 or 3 when it is not. The exit code is captured now and the flag set on non-zero. The variable is also declared at file scope so both read sites are defined even when the static check does not run. BEHAVIOUR CHANGE, stated plainly: on a system whose static time check fails, the consensus check is now skipped and anondate_use runs. That is what the code always said it would do. Testing ------- Gate exit 0 in an ISOLATED checkout. 3 pass, 0 fail clock OK (exit 0) same, FLAG_NOT_SET clock too EARLY (exit 2) differ, FLAG_SET clock EXPIRED (exit 3) differ, FLAG_SET Both non-zero codes are covered, not just one -- 2 and 3 are distinct failure modes in timesanitycheck and it would be easy to wire up only the first. The 'clock OK' case asserts the two versions AGREE: the fix must not start setting the flag when the clock is fine.
- 'command -v' prints a bare word for builtins, functions, aliases and keywords rather than a path. Testing that word with '-x' resolved it as a RELATIVE path in the current directory, so 'has printf' returned non-zero even though printf is always available, and the result depended on what happened to be in the cwd. - Only test '-x' when 'command -v' actually produced a path. - 'has' is the R-090 replacement for 'command -v', so a conversion that passed a builtin turned a working check into one that could never succeed. Found while converting tb-updater's version-validator, where 'has printf' aborted the unit test suite at its first case. Regression test asserts builtins, externals, a mix, and absent commands. Verified by canary: with the unconditional '-x' test restored, 4 of the 7 cases fail. ## AI-Assisted Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- The test sourced has.sh by a path relative to itself, so it validated the checkout copy and passed 7/7 while the installed copy was still the broken version. It could not detect a stale deployment, which is the one thing a deployment check has to catch. - Now sources /usr/libexec/helper-scripts/has.sh and fails loudly when it is absent, rather than silently falling back to a copy that always passes. Verified against a real deployment: before installing, the probe reported INSTALLED-STALE; after, INSTALLED-OK with the path guard present and 7/7 pass. ## AI-Assisted Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- mypy writes its cache into the CURRENT directory, and the loops cd into usr/lib/python3/dist-packages/<pkg>/ to lint. That left a .mypy_cache beside the sources, which genmkfile swept into the .deb; lintian then warned with package-contains-python-dot-directory and the build failed closed. - Gitignoring it does not help: genmkfile packages the WORKING TREE, not git's view, so an ignored directory is still shipped. - Anchor the cache at the repo root via --cache-dir, outside anything packaged. Verified: after linting a package, 0 .mypy_cache directories exist under usr/ and the cache is at the repo root instead. ## AI-Assisted Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stcat takes EVERY argument as a path, so 'stcat -- "${target_file}"' made it
read a file literally named '--' and die with FileNotFoundError.
read_integer_file then reported
ERROR: Cannot stcat target file '<path>'!
for a file it had just confirmed exists and is readable. Four of tb-updater's
e2e scenarios failed on this -- all of them the ones that read a cached
signature timestamp back: second_run_cache_hit_tb, cross_context_fallback,
downgrade_attack_warning, second_run_mullvad_cache_hit.
Verified against the real binary: 'stcat -- file' exits 1, 'stcat file' exits 0.
R-062 is why the separator was added, and it is right for tools that accept
one; this is the rule's negative half. pre-push-static now denylists 'stcat --'
so it cannot come back, and the style guide records stcat as a verified
rejecter.
Test: usr/share/helper-scripts/tests/test_read_integer_file.sh, which asserts
the INSTALLED library rather than the checkout -- a self-relative source would
pass against a stale install, which is exactly the failure mode this suite
exists to catch. Canaried: it FAILS against an install carrying the '--', and
passes once the fixed library is installed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Heads-up on a regression this branch now fixes, because it is currently breaking another repo's CI.
for a file it has just confirmed exists and is readable. Upstream Impact: Fixed in 31ff576 on this branch, with Assisted by AI. |
- both cases now live in dist-ai as helper-scripts-lib-tests (has_builtin_test.sh, read_integer_file_test.sh), which resolves the library from the INSTALLED path by default and takes a checkout only via an explicit HELPER_SCRIPTS_REPO the runner wires - the copies here sourced /usr/libexec/helper-scripts directly, so nothing ran them: no runner in this repo globs usr/share/helper-scripts/tests - debian/*.install ships 'usr/*', so there is no entry to drop; removing the files removes them from the package - coverage is unchanged and canaried in its new home: reverting the '-x' on a bare word fails 4 cases, reverting 'stcat --' fails 3
- Add shopt -s inherit_errexit / shift_verbose to the source-able guarded strict blocks that enabled errexit but omitted them (systemd-notify, live-mode, get_writable_fs_lists, pre.bsh, onion-time-pre-script, shim-signed-mok-setup). - Brace all variable refs (R-020/SC2250) across trace, root_cmd, package_installed_check, get_os, xtrace, parse_opt. - pre.bsh: rm -> safe-rm; mark the error-handler 'set +e' with the allow-errexit-toggle waiver (recursion guard). - systemd-notify: 'command -v' -> 'has' (source has.sh). - live-mode: printf keeps the name='value' contract via a prebuilt string (R-030). - no-strict waivers on the pure sourced libs (trace, xtrace, parse_opt, package_installed_check); SC3043 disable where 'local' is used in a deliberately-POSIX file.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
assisted-by-ai has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
A test exercising a caller of light_sleep can set light_sleep_skip=true to skip the real wait; unset in production, where it sleeps normally. Add the no-strict waiver + -r on the two timeout reads to satisfy the static gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
4 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.
Now the single consolidated PR for this repo (git skill: one branch named
ai, one PR).Contents
default_if_emptyinstrings.bsh, using the existingcheck_variable_namesanitize-echo: sanitize-string's sanitizer with echo's shapestrings.bshdynamic-scope shadowing fix (a silent validation bypass)stecho/sanitize-echoinstead ofprintf+ inlinesanitize-stringstrings.bsh; shebangs dropped from import-only python modulesThe bypass, for reviewers
check_is_alpha_numeric,validate_safe_filenameandcheck_is_not_empty_and_only_one_lineread the caller's variable with${!name}, but bash scoping is dynamic, so their own locals shadowed a caller variable of the same name. Measured before the fix:Locals are now prefixed. Regression tests are in
string_bsh_tests, and were verified to FAIL against the pre-fix code rather than pass vacuously.One resolution to review
The merge of the wayland branch conflicted in
use_sudo.sh. Resolved to master'ssudo_error_exit_if_unavailable(which gained an optional graphical mode in fe11d7b) rather than the branch's older auto-detecting variant, which also callsgeneric_gui_message.py-- a path that does not exist here, unlike the non-.pyhelper master calls. The branch's approach to "visible sudo error" is therefore superseded, not merged. Worth a second opinion if that was not intended.Verification
Gate green against master.
strings.bshself-tests pass;sanitize_string24 tests with black/pylint/mypy clean.Generated with assistance from Claude Code.
Summary by CodeRabbit
sanitize-echocommand for sanitizing command-line input or standard input, with optional output-length limits.@claude.