Repository navigation
style: apply ruff's safe mechanical fixes - #12
Merged
Merged
Conversation
Import blocks reordered (I001) across 5 files, re.I/re.S replaced with their non-deprecated-looking spelled-out names re.IGNORECASE/re.DOTALL (FURB167, verbatim aliases per the re module docs), a redundant quoted type annotation dropped now that `from __future__ import annotations` already makes it unnecessary (UP037), and one of five `with a: with b:` nestings collapsed where ruff judged it safe (SIM117). None change behavior. The remaining SIM117 sites, the two B023 loop-variable-closure warnings in test_backup_restore.py, the three PLW1510 missing-check warnings in test_container_permissions.py, and the SIM115 file-handle warning in collect-support-bundle.py were reviewed individually and are not real issues: the B023 closures are consumed synchronously within the same loop iteration they're defined in, never stored past it; the SIM115 temp file is explicitly closed two lines below where ruff flags it, keeping only its path alive on purpose. Left as-is rather than force-fixed or suppressed. Verified: full pytest suite's failing-test set is byte-identical before and after (diffed the two FAILED lists), both files touched still compile, and no CRLF was introduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`CDPATH= cd -- ...` (no quotes) trips shellcheck's SC1007: an unquoted empty assignment before a command is ambiguous to parse and easy to misread as a mistake. Every other script in this workspace that neutralizes CDPATH before resolving its own directory already uses the quoted form -- `CDPATH='' cd` -- across 14 files in mochad-redux, mochad-mqtt-bridge, and this repo. This was the one holdout. Purely a quoting fix: `CDPATH=` and `CDPATH=''` both set the variable to an empty string. Verified with `bash -n`, a clean shellcheck run, and running the script with no arguments -- identical usage-message output before and after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… credential AUDIT.md P1#6: SECRET_RE is defined but was only ever applied to environment *key names* inside the optional --environment-file (_environment_snapshot). _check_content(), which runs against the two files always backed up (docker-compose.yml, release/versions.env), checked only for PEM key blocks and scheme://user:pass@ URLs. A line like `MQTT_PASSWORD: hunter2` in a compose `environment:` block -- a very common operator pattern -- copied into the archive unmodified, in a tool whose error messages claim to guard against secret material. Add CREDENTIAL_ASSIGNMENT_RE to _check_content(), covering the keyword-followed-by-assigned-value shape (bare, quoted, or YAML `key: value`) that a bare keyword search over-flags on its own -- "primary key: id" or a "password policy" comment would trip a naive password|secret|token match. The pattern is copied verbatim from scripts/support/collect-support-bundle.py's SECRET_PATTERNS rather than re-derived, since that exact balance (catches real secrets, leaves ordinary mentions alone) is already proven against 18 secret cases and 10 benign cases in the workspace's redaction-conformance.py. Verified: - the pattern against this repo's own docker-compose.yml and docker-compose-debug.yml: no match, so the real files still back up - four new secret cases (YAML and env-style, quoted and bare) now raise BackupError; three new benign cases (a password-policy comment, a "primary key" comment, an ordinary port assignment) do not - full pytest suite's failing-test set is byte-identical before and after this change (diffed the two FAILED lists) -- the two pre-existing restore-path failures are unrelated to this fix - redaction-conformance.py, which exercises the source pattern this was copied from, still passes across all three repos - no CRLF introduced, both changed files still compile Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…on a decode failure
Caught by an adversarial local-model review of the CREDENTIAL_ASSIGNMENT_RE
change before this branch merged: `_check_content()` decoded content as
UTF-8 to run the new keyword-plus-value scan, and on UnicodeDecodeError
just returned -- silently skipping that scan for the rest of the file.
Verified the gap is real: `_check_content("x", b"MQTT_PASSWORD=hunter2\n\xff\n")`
raised nothing before this fix. One stray non-UTF-8 byte anywhere in
docker-compose.yml or versions.env -- a bad copy-paste, a stray binary
byte, anything -- would have silently disabled the credential-assignment
check for a plaintext secret sitting in valid UTF-8 right next to it,
while PRIVATE_KEY_RE and URL_CREDENTIAL_RE (which run on raw bytes) kept
working. Fail-open in a function whose entire point is failing closed.
Now raises BackupError on the decode failure instead. These files are
already required to be plain text (the b"\0" check just above), so failing
to decode as UTF-8 is itself anomalous for a docker-compose.yml or .env
file and should refuse the backup, not narrow what gets checked.
A second finding from the same review -- that a bare "password:" or
"token=" inside a comment can also match and refuse a benign file -- was
verified but not changed: it's the same keyword-plus-value pattern already
proven in collect-support-bundle.py's SECRET_PATTERNS, and an occasional
false-positive refusal in a tool whose whole design is "refuse rather than
leak" is a nuisance with a clear, actionable error, not a security
regression -- unlike the fail-open decode path, which degraded the
guarantee silently.
Verified: the exact stray-byte case now raises with the new test; full
pytest suite's failing-test set is byte-identical before and after (the
two pre-existing restore-path failures are unrelated); redaction-
conformance.py still passes across all three repos; no CRLF introduced.
Co-Authored-By: Claude Opus 5 <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
1 participant
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.
Import reordering (I001),
re.I/re.Sspelled out asre.IGNORECASE/re.DOTALL(FURB167, verbatim aliases), a redundant quoted type annotation dropped now thatfrom __future__ import annotationsalready makes it unnecessary (UP037), and onewith-nesting merge ruff judged safe (SIM117). None change behavior.Verified: full pytest suite's failing-test set is byte-identical before and after (diffed), both touched Python files still compile, no CRLF introduced.
The remaining SIM117/B023/PLW1510/SIM115 findings were reviewed individually and are not real issues (closures consumed synchronously within their own loop iteration; the flagged temp file is explicitly closed two lines below where ruff flags it) -- see the commit message. Left as-is.