fix(disk-hygiene): setup check reports the kill switch deterministically, never from an unexpanded body token#1012
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c57a36911
ℹ️ 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".
1c57a36 to
4fa7bd4
Compare
|
Claude finished @kyle-sexton's task in 2m 52s —— View job PR Review: fix(disk-hygiene): setup check reports kill switch deterministicallyTodo list:
SummaryThis PR correctly diagnoses and fixes a real safety-reporting flaw: Findings1. {k: e['value'] for k, e in zip(interpreted, entries)}
{entry["key"]: entry["value"] for entry in entries}2. The logic is sound: by line 148, effective = values.pop()
if effective is None:
return _report(True, "indeterminate", True, "Internal probe error; assuming default.", settings_path, entries)Low risk given the logical guarantee, but worth noting for a safety-critical path. 3. Codex P2 finding — managed settings scope limitation (design decision, not a bug)
The Codex reviewer flagged that when no user-settings entry exists, the probe reports This limitation is intentional and explicitly documented: the probe docstring says "managed settings and a One thing worth verifying: the 4.
Path(__file__).resolve().parents[2] / "setup" / "scripts" / "kill_switch_probe.py"
5. Consistent-duplicate entries are accepted as
Two entries What's well done
VerdictReady to merge. The one actionable style fix (item 1) is optional but would make the code easier to follow. Everything else is correct. |
|
Claude finished @kyle-sexton's task in 3m 54s —— View job Security Review — PR #1012
Scope: No CRITICAL or IMPORTANT findings.The overall design is sound: the probe is stdlib-only and read-only (exit 0 always), the guard uses SUGGESTION —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fa7bd40f9
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 3m 9s —— View job PR Review: fix(disk-hygiene): setup check reports kill switch deterministicallyTodo list:
ContextThis PR has two commits:
The latest push only changed file modes; no code changed since the prior two Claude reviews. None of the findings from those reviews have been addressed yet. Unaddressed Finding That Needs a Fix
try:
settings = json.loads(settings_path.read_text(encoding="utf-8"))
except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc:
return _report(...) # catches unparsable JSON
plugin_configs = settings.get("pluginConfigs") # ← AttributeError if settings is a list, str, int, or NoneWhen Codex flagged this at the commit Fix — add a type guard immediately after parsing: try:
raw = json.loads(settings_path.read_text(encoding="utf-8"))
except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc:
return _report(True, "indeterminate", True,
f"Could not read the configured toggle from {settings_path} ({exc}); "
"assuming the default (enabled). This is an assumption, not the configured value.",
settings_path, [])
if not isinstance(raw, dict):
return _report(True, "indeterminate", True,
f"{settings_path} root is not a JSON object ({type(raw).__name__}); "
"assuming the default (enabled). This is an assumption, not the configured value.",
settings_path, [])
settings = raw
plugin_configs = settings.get("pluginConfigs")A test case for this path ( Findings Carried From Prior Reviews (Still Open)These were identified in the prior two Claude review comments on this PR and remain unaddressed: 1. The assert is logically guaranteed to never fire (all effective = values.pop()
if effective is None:
return _report(True, "indeterminate", True,
"Internal probe error; assuming default.", settings_path, entries)2.
{entry["key"]: entry["value"] for entry in entries}3. If the probe file is absent from the plugin (partial install), candidate_key = _script_path_key(tokens[1])
expected_key = _script_path_key(expected_script)
return candidate_key is not None and candidate_key == expected_key4.
Additional Note: Test Wrapper Python Resolution Order
The script tries bare What's Well DoneThe core design is sound and the fix addresses a real safety reporting flaw:
VerdictOne fix is needed before merge: the |
|
Claude finished @kyle-sexton's task in 3m 36s —— View job Security Review — PR #1012Todo list:
No CRITICAL or IMPORTANT findings.The design is sound: the probe is stdlib-only and strictly read-only (no subprocess, no writes); the guard uses SUGGESTION —
|
|
🔒 merge-drive lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, worker tier). Will fix clear branch-owned CI/bot findings, defer non-DEFER-FORBIDDEN items with reply+resolve, push, then re-check the merge gate. |
…om an unexpanded body token
setup check step 4 emitted `${user_config.disk_hygiene_enabled}` in the
skill body with the rule "unexpanded or empty means default true", so a
configured false (audit-only mode) whose token failed to expand was
misreported as enabled — a false-negative on the one safety-critical
setting the check exists to verify. The clean skill's audit-only
instruction carried the same "unset = enabled" assumption.
Add a report-only, stdlib-only probe
(skills/setup/scripts/kill_switch_probe.py) that reads
pluginConfigs[<plugin-id>].options.disk_hygiene_enabled from the user
settings.json (CLAUDE_CONFIG_DIR-aware) and emits one JSON line with the
effective boolean, its provenance (configured/default/indeterminate), a
degraded flag, and the matched entries. Unparsable settings, invalid
value types, and conflicting entries degrade honestly — assumed default
true is always labeled an assumption, never the configured value.
setup check now reports the probe result with provenance and treats the
body token as at most a cross-check whose contradiction is reported.
clean resolves an unexpanded token through the same probe; the guard's
Bash allowlist permits exactly the argument-free bundled probe shape
(any argument, bare python, or another path stays denied). Enforcement
remains the guard's runtime-substituted --disk-hygiene-enabled hook
argument (0.4.4).
Docs verified this session: plugins-reference documents skill-content
substitution for non-sensitive user_config values, but a live run
observed the token unexpanded, so body expansion cannot be load-bearing
for a safety report.
Closes #1007
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… index CI's shebang-executable gate requires 100755 for files with a shebang. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d53a11e to
7cc6d57
Compare
|
Claude finished @kyle-sexton's task in 2m 24s —— View job PR Review: fix(disk-hygiene): setup check reports kill switch deterministicallyTodo list:
ContextThe latest push ( One Finding Still Needs a Fix Before Merge
try:
settings = json.loads(settings_path.read_text(encoding="utf-8"))
except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc:
return _report(...) # only catches parse-level errors
plugin_configs = settings.get("pluginConfigs") # ← AttributeError if root is [], "str", 42, nullWhen The Fix — add an if not isinstance(settings, dict):
return _report(
True,
"indeterminate",
True,
f"{settings_path} is valid JSON but not an object "
f"({type(settings).__name__}); assuming the default (enabled). "
"This is an assumption, not the configured value.",
settings_path,
[],
)
plugin_configs = settings.get("pluginConfigs")A test for this path is also missing — Open Suggestions (Carried From Prior Reviews)These are non-blocking but remain open. 1. The assert is logically guaranteed not to fire (all effective = values.pop()
if effective is None:
return _report(True, "indeterminate", True,
"Internal probe error; assuming default.", settings_path, entries)2. {k: e['value'] for k, e in zip(interpreted, entries)}
{entry["key"]: entry["value"] for entry in entries}3. return _script_path_key(tokens[1]) == _script_path_key(expected_script)If the probe file is absent from the installed plugin (partial install, accidental deletion), candidate_key = _script_path_key(tokens[1])
expected_key = _script_path_key(expected_script)
return candidate_key is not None and candidate_key == expected_key4.
What's Well DoneThe overall design is sound and addresses a real safety-reporting flaw:
VerdictOne fix is needed before merge: the |
|
Claude finished @kyle-sexton's task in 3m 28s —— View job Security Review — PR #1012Todo list:
No CRITICAL findings.The design posture is sound: the probe is stdlib-only and strictly read-only (no subprocess, no writes), the guard uses SUGGESTION —
|
… its user-settings-only scope A settings.json whose root is valid JSON but not an object (e.g. []) hit .get() and crashed the probe with AttributeError, breaking the promised single-line-JSON contract; it now degrades to indeterminate like any other unreadable settings document. Default-source reports also state in the report itself that managed settings or a --settings flag could carry a value the probe cannot see, instead of leaving that caveat to the skill prose alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 57s —— View job PR Review: fix(disk-hygiene): setup check reports kill switch deterministicallyTodo list:
ContextLatest commit Blocking Finding — Now Fixed
The fix chosen wraps the isinstance check inside the existing try:
settings = json.loads(settings_path.read_text(encoding="utf-8"))
if not isinstance(settings, dict):
raise ValueError("settings root is not a JSON object")
except (OSError, UnicodeDecodeError, ValueError) as exc:
return _report(True, "indeterminate", True, ...)Two notes on the approach:
A dedicated test ( Scope Documentation — Also AddressedBoth the "no settings file" and "no entry" default-report paths now append to their
A corresponding test ( Open Suggestions (Carried From Prior Reviews — Non-Blocking)These were raised in prior review cycles and remain unaddressed. None block merge independently. 1. The assert is logically guaranteed not to fire (all effective = values.pop()
if effective is None:
return _report(True, "indeterminate", True,
"Internal probe error; assuming default.", settings_path, entries)2. {k: e['value'] for k, e in zip(interpreted, entries)}
{entry["key"]: entry["value"] for entry in entries}3. return _script_path_key(tokens[1]) == _script_path_key(expected_script)
candidate_key = _script_path_key(tokens[1])
expected_key = _script_path_key(expected_script)
return candidate_key is not None and candidate_key == expected_key4.
VerdictReady to merge. The blocking finding — |
|
Claude finished @kyle-sexton's task in 3m 53s —— View job Security Review — PR #1012Todo list:
No CRITICAL or IMPORTANT findings.The latest commit ( SUGGESTION —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0cab40f53
ℹ️ 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".
…spected Path.is_file() swallows OSError and returns False for existing non-files, so an inaccessible settings.json (unsearchable ancestor) or a directory at the settings path was reported as "no settings file, default, not degraded" — a certainty the probe did not have. The probe now stats the path itself: only FileNotFoundError keeps the clean default report; any other OSError or a non-regular file degrades to indeterminate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 7s —— View job PR Review: fix(disk-hygiene): setup check reports kill switch deterministicallyTodo list:
SummaryThis PR correctly diagnoses and resolves a real safety-reporting flaw: the The PR has gone through five review rounds. Each round addressed the blocking finding of that cycle. At HEAD (3eb21d7), all blocking findings across all rounds are resolved. Latest Commit (3eb21d7) — Verified FixedProbe degrades when the settings path cannot be inspected
The new
Two regression tests added: The uninspectable test uses Prior Findings — All Resolved
Remaining Suggestions (Non-Blocking — All Carried From Prior Reviews)These have been raised in prior review cycles without being addressed. None block merge; the prior four review rounds all reached "ready to merge" without them. 1. The assert is logically unreachable (all effective = values.pop()
if effective is None:
return _report(True, "indeterminate", True,
"Internal probe error; assuming default.", settings_path, entries)2. {k: e['value'] for k, e in zip(interpreted, entries)}
{entry["key"]: entry["value"] for entry in entries}3.
candidate_key = _script_path_key(tokens[1])
expected_key = _script_path_key(expected_script)
return candidate_key is not None and candidate_key == expected_key4.
What's Well Done
VerdictReady to merge. All blocking findings from all five review rounds are resolved. Items 1–4 above are non-blocking suggestions that have been open since the first review round; the author may defer them. |
|
Claude finished @kyle-sexton's task in 2m 36s —— View job Security Review — PR #1012Todo list:
No CRITICAL or IMPORTANT findings.The latest commit ( The enforcement kill-switch channel — runtime-substituted SUGGESTION — TOCTOU between
|
Reconcile disk-hygiene conflicts from concurrent main advances (#1010, #1012, #1028, #1031) that landed while this branch was open. - plugin.json / CHANGELOG: restack this branch's fix onto main's version line, 0.4.8 -> 0.6.3 (next patch above main's landed 0.6.2); preserve every landed CHANGELOG entry (0.6.2 / 0.6.1 / 0.6.0 / 0.5.0) with this branch's entry on top. - clean/SKILL.md: compose #983's corrected kill-switch framing (a skill-scoped hook receives neither ${user_config.*} nor CLAUDE_PLUGIN_OPTION_*, so the guard cannot enforce audit-only) with #1012's deterministic kill_switch_probe read, dropping main's stale "guard enforces via --disk-hygiene-enabled" claim that #983's hook-arg change (only --plugin-root) invalidated. - clean/reference/safety-model.md: keep #983's --data-root derivation-from- plugin-root rewrite and append #1010's --confirmed-large-scan grammar note. destructive_guard.py and test_hygiene.py auto-merged (data-root derivation + --confirmed-large-scan + kill_switch_probe allowlist + MIN_PYTHON floor); full suite 98 passed, 4 platform-skipped.
Summary
Fixes the HIGH finding from handoff-inbox item
20260722-004536-disk-hygiene-setup-audit:/disk-hygiene:setup checkreported the safety-criticaldisk_hygiene_enabledkill switch by emitting${user_config.disk_hygiene_enabled}in the skill body with the rule "unexpanded or empty means defaulttrue" — so a configuredfalse(audit-only mode) whose token failed to expand was misreported as enabled.skills/setup/scripts/kill_switch_probe.py(stdlib-only, report-only, exit 0 always): readspluginConfigs[<plugin-id>].options.disk_hygiene_enabledfrom usersettings.json(CLAUDE_CONFIG_DIR-aware), emits one JSON line:effective,source(configured/default/indeterminate),degraded, matchedentries. Unparsable settings, invalid types, and conflicting entries degrade honestly — an assumed default is always labeled an assumption.python, other paths stay denied — fail-closed posture unchanged).--disk-hygiene-enabledhook argument (0.4.4) remains the authority.Fresh-docs verification (fetched 2026-07-22): https://code.claude.com/docs/en/plugins-reference states non-sensitive
${user_config.*}values "can also be substituted in skill and agent content", while https://code.claude.com/docs/en/skills omits${user_config.*}from its substitution table and the producer's live run observed the token unexpanded — docs/field conflict, so body expansion cannot be load-bearing for a safety report.Testing
test_kill_switch_probe.py, wrapperkill_switch_probe.test.shauto-discovered byscripts/run-plugin-tests.sh), 4 new guard tests (probe shape allowed incl. audit-only mode; arguments / bare python denied).test_hygiene.py85 tests OK (4 platform skips);check-changed-skills.shPASS;check-changelog-parity.sh --check-bumpPASS; shellcheck clean.Related
20260722-004536-disk-hygiene-setup-audit(F1, HIGH)🤖 Generated with Claude Code