Skip to content

fix(disk-hygiene): define "suspicious" for unhinted home-root entries - #869

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/287-disk-hygiene-home-droppings
Jul 21, 2026
Merged

fix(disk-hygiene): define "suspicious" for unhinted home-root entries#869
kyle-sexton merged 1 commit into
mainfrom
fix/287-disk-hygiene-home-droppings

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

Closes the two gaps #286 left open from issue #287: the clean skill's step 2 said to inspect
"hinted or suspicious" entries but never defined "suspicious" for an entry with no hint, so
unhinted agent-session droppings at a home-directory root (a status snapshot, a one-off export)
were never triaged even though the scan snapshot already records every walked entry.

Fix

  • plugins/disk-hygiene/skills/clean/SKILL.md §2: added a paragraph defining "suspicious" for
    a user-home target — any loose root-level entry not in protected_exact_names and not
    matching a recognizable app/config convention. This single guidance addition realizes both of
    fix(disk-hygiene): unblock guarded engine lane and bounded large-root audits #286's open findings (the undefined "suspicious" criteria, and the agent-session status-file
    heuristic) since they're the same positional gap: the engine already snapshots every walked
    entry with a possibly-empty hints list, so the data was always available, just never acted
    on.
  • plugins/disk-hygiene/skills/clean/reference/baseline-policy.json is intentionally
    untouched. The issue's tmp_* hint-glob gap already shipped in fix(disk-hygiene): unblock guarded engine lane and bounded large-root audits #286/v0.3.0. The remaining
    class of entries (melo-lap-001-*.json, org_usage.json) shares no common name shape — the
    issue itself calls a generic glob "admittedly hard" — so adding a baseline-policy.json pattern
    here would mean fabricating a naming convention the evidence doesn't support. The positional
    SKILL.md guidance is the mechanism the issue's own notes suggest as the alternative.
  • Bumped plugins/disk-hygiene/.claude-plugin/plugin.json 0.4.1 → 0.4.2 and added a matching
    CHANGELOG entry.

Verification

  • Confirmed the linchpin empirically before writing the guidance: hygiene.py's snapshot walk
    (scripts/hygiene.py:608-615) appends every walked entry with "hints": matching_hints(...)
    (possibly empty), not just hinted ones — so the new guidance has real data to act on.
  • Ran the repo's skill-quality gate: bash scripts/check-changed-skills.sh origin/main — PASS,
    0 errors (SKILL.md 235/500 lines, well under the hard cap; only a pre-existing soft
    line-count WARN, unrelated to this change).

Closes #287

Related

The clean skill's step 2 referenced "hinted or suspicious" entries but
never said what made an unhinted entry suspicious, so agent-session
droppings with no shared name shape (a status snapshot, a one-off
export) were never triaged even though the scan snapshot already
records every walked entry. Step 2 now instructs the model to treat
any loose root-level entry at a user-home target that isn't in
protected_exact_names and doesn't match a recognizable app/config
convention as suspicious.

baseline-policy.json is untouched: the tmp_* hint-glob gap this issue
also reported already shipped in #286, and the remaining class has no
common name shape to glob without fabricating a pattern the evidence
doesn't support.

Closes #287

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1V3gkrfSf75isB8MiDy3o
@kyle-sexton kyle-sexton added the automated Opened by automation. label Jul 21, 2026
@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 13s —— View job


PR Review

  • Gather context (PR body, changed files)
  • Read changed files
  • Review SKILL.md guidance addition
  • Review plugin.json version bump
  • Review CHANGELOG entry
  • Post review findings

Summary

This is a guidance-only patch that closes the remaining open findings from #286/#287. No code executes differently — the change is entirely to the clean skill's LLM instructions. The blocking criteria in REVIEW.md (injection, secrets, authorization, atomicity) are not applicable here. No blocking issues found.

Two nits below; the rest is clean.


Verified Claims

The PR's key empirical assertion holds: hygiene.py:608-615 appends every walked entry with "hints": matching_hints(...), and matching_hints() returns [] when no policy pattern matches — so the snapshot already carries unhinted entries and the new triage instruction has real data to act on.

The decision to leave baseline-policy.json untouched is the right call: melo-lap-001-*.json and org_usage.json share no extractable name shape, and fabricating a glob from two instances would over-fit.


Findings

🟡 Nit — "Recognizable app/config convention" is undefined and has no examples

SKILL.md:90-92

does not belong to a recognizable app/config convention as suspicious too

protected_exact_names in baseline-policy.json covers shell folders (Desktop, Documents, etc.) but does NOT include .ssh, .config, .bashrc, .profile, .zshrc, .local, or any dotfile/dotdir. The second condition — "does not belong to a recognizable app/config convention" — is the only safety valve that keeps those from being flagged, but that phrase carries no examples or anchors. A model following the protected_exact_names check literally would find .ssh unprotected and rely entirely on training knowledge to exclude it via the convention clause.

For a skill that runs at a home-directory root (high-blast-radius target), a brief inline example set in the new paragraph — e.g., "dotfiles, dotdirs, XDG base-dir subdirectories (~/.config, ~/.local), shell rc files, and vendor-documented locations" — would reduce inter-run variance without changing the design intent.

Fix this →


🟡 Nit — "User-home target" is not defined or anchored in SKILL.md

SKILL.md:89

at a user-home target, treat any loose root-level entry…

The phrase "user-home target" appears only here. The model must infer from the invocation path whether it qualifies (e.g., /home/<user>, /Users/<user>, ~, C:\Users\<user>). A short parenthetical — "(a target whose path resolves to the current user's home directory)" — would remove that ambiguity and prevent the heuristic being applied to other large roots (e.g., a project root that happens to contain unrecognized files).


What's Right

  • Insertion point is optimal: the new paragraph sits between the scan-output description and "For each hinted or suspicious entry…", extending the trigger condition precisely where the processing step reads it. (SKILL.md:89-96)
  • Version bump is correct: guidance-only patch → 0.4.1 → 0.4.2 is the right semver increment.
  • CHANGELOG entry is accurate: clearly states both closed findings, explains the shared root cause, and ties to disk-hygiene:clean misses underscore-prefixed temp dirs and unowned agent-session files at a home-directory root #287.
  • "Loose root-level entry" scoping: correctly limits to depth=1, consistent with the bounded --max-depth 1 scan pass described for large roots.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 5s —— View job


Security Review — PR #869

  • Gather PR diff and changed files
  • Review SKILL.md guidance additions for injection/trust surfaces
  • Review plugin.json version bump
  • Review CHANGELOG.md entry
  • Post findings

No security issues found in this PR.

Scope reviewed: all three changed files — SKILL.md, plugin.json, CHANGELOG.md.

SKILL.md (primary change): The added paragraph is model-instruction prose only — it introduces no new shell commands, no new code execution paths, no new tool invocations, and no new data flows. The definition of "suspicious" it adds (root-level home entry not in protected_exact_names and not matching a recognizable app/config convention) broadens what entries reach triage, but does not touch or short-circuit any of the safety mechanisms that govern what happens after triage: the evidence-gathering step, the three-tier classification, the deterministic preview gate with approval-token binding, and the explicit per-tier AskUserQuestion confirm gate all remain fully in place. A wrongly-flagged legitimate file still cannot be deleted without the user approving it at the exact-path tier level.

The hook frontmatter in SKILL.md (destructive_guard.py blocking all Bash/PowerShell other than the exact scan/preview/apply shapes) is unchanged.

plugin.json: Semver patch bump 0.4.1 → 0.4.2, no other field altered. No injection surface; no permission or capability change.

CHANGELOG.md: Documentation text only.

@kyle-sexton
kyle-sexton marked this pull request as ready for review July 21, 2026 16:52
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Post-green review classification (work lane):

Finding Source Classification Disposition
Everything substantive PR review + security review "No blocking issues found" / "No security issues found in this PR" No action needed
"recognizable app/config convention" undefined, no examples (dotfiles/.ssh/.config not in protected_exact_names) PR review (nit) Non-blocking — reviewer notes the underlying safety gates (preview + per-tier approval) remain intact regardless; a wrong flag still can't delete anything without user approval Left alone
"user-home target" undefined/unanchored PR review (nit) Non-blocking doc-polish suggestion Left alone

No DEFER-FORBIDDEN class applies — this is guidance-only prose; the destructive-action gates (preview, approval-token binding, per-tier AskUserQuestion) are unchanged and confirmed intact by the security review. No inline review threads to resolve (PR-level comments only).

Note: verified the actual comment content (not just the green CI check) before this classification — a sibling PR this cycle (#867) had a review check show "pass" while the underlying review job silently failed at the infra level. Confirmed #869's review is a real verdict, not a placeholder.

Flipping to ready, handing off to the babysit lane for ongoing watch.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 51s —— View job


PR Review — #869

  • Gather context (PR body, changed files, prior review comments)
  • Read changed files
  • Review SKILL.md guidance addition
  • Review plugin.json version bump and CHANGELOG entry
  • Post review findings

Summary

Guidance-only patch that closes the remaining open gap from #286/#287. No code execution changes — the entire delta is new prose in the clean skill's step 2, a semver patch bump, and a CHANGELOG entry. No blocking issues found.


Verified Claims

The PR's linchpin assertion holds: baseline-policy.json has a protected_exact_names list covering VCS roots (.git, .hg, .svn), Windows profile files, and standard shell folders (Desktop, Documents, Downloads, Music, OneDrive, Pictures, Videos). Dotfiles (.ssh, .config, .bashrc, .zshrc, .local, etc.) are not in that list, which means the second condition of the new guard — "does not belong to a recognizable app/config convention" — is the only thing keeping those from being flagged as suspicious. The prior review already caught this; see nit below.

The decision to leave baseline-policy.json untouched is correct. melo-lap-001-*.json and org_usage.json share no extractable name shape, and both fall naturally into the "suspicious" bucket the new paragraph creates: not in protected_exact_names, no recognizable app convention.


Observations

Paragraph insertion point is optimal. The new text sits between the os_autoclean advisory paragraph and the "For each hinted or suspicious entry…" step, which is the precise location where the trigger condition is read. The word "too" in "treat … as suspicious too" correctly extends — not replaces — the hinted-entry path. (SKILL.md:89-94)

"Loose root-level entry" scoping is consistent. Step 1 already establishes that large roots like home directories start with --max-depth 1 (SKILL.md:63-65), so the new paragraph's depth-1 constraint is coherent with the scan shape.

"User-home target" has implicit context. SKILL.md:63 uses "a home directory" as the primary exemplar for large-root bounded scanning, so a model reading the full document connects the two. Still, the phrase is not explicitly anchored in the new paragraph itself — the prior review flagged this nit and the author decided to leave it, which is a reasonable call.

"Recognizable app/config convention" is the main variance point. Standard dotfiles (.ssh, .config, .bashrc, .zshrc, .local) are the canonical examples of this convention but are absent from protected_exact_names. For a home-root target, an inline parenthetical — e.g. "(dotfiles, dotdirs, shell rc files, XDG base-dir paths such as ~/.config and ~/.local, and any location with documented app ownership)" — would narrow inter-run variance without changing intent. The prior review surfaced this; the author left it, noting the downstream safety gates (exact-path preview, per-tier AskUserQuestion) remain intact. That reasoning holds.

Version bump and CHANGELOG are correct. Guidance-only change → 0.4.1 → 0.4.2 is the right semver increment. The CHANGELOG entry accurately names both closed findings, explains the shared root cause, and ties to #287.


Bottom Line

No new issues beyond the two nits the prior review already surfaced. Both nits are bounded in practice by the downstream approval gates — a wrongly-flagged .ssh still cannot be deleted without the user approving it at the exact-path tier. The author made a considered, explicit decision to leave them; the trade-off is defensible. Clean to merge.

@claude

claude Bot commented Jul 21, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 3s —— View job


Security Review — PR #869

  • Gather PR diff and changed files
  • Read SKILL.md guidance additions for injection/trust surfaces
  • Review plugin.json version bump
  • Review CHANGELOG.md entry
  • Post security findings

No security issues found in this PR.

Scope reviewed: all three changed files — SKILL.md lines 89–94, plugin.json, CHANGELOG.md.

SKILL.md (primary change): The added paragraph is model-instruction prose only — it introduces no new shell commands, no new code execution paths, no new tool invocations, and no new data flows. The definition of "suspicious" it adds (root-level home entry not in protected_exact_names and not matching a recognizable app/config convention) broadens what entries reach triage, but does not touch or short-circuit any of the safety mechanisms that govern what happens after triage: the evidence-gathering step, the three-tier classification, the deterministic preview gate with approval-token binding, and the explicit per-tier AskUserQuestion confirm gate all remain fully in place. A wrongly-flagged legitimate file still cannot be deleted without the user approving it at the exact-path tier level.

The hook frontmatter in SKILL.md (destructive_guard.py blocking all Bash/PowerShell other than the exact scan/preview/apply shapes) is unchanged.

plugin.json: Semver patch bump 0.4.1 → 0.4.2, no other field altered. No injection surface; no permission or capability change.

CHANGELOG.md: Documentation text only.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f63569e879

ℹ️ 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".

Comment thread plugins/disk-hygiene/skills/clean/SKILL.md
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

This was generated by AI (decide lane — decision-brief pass; parked PR-thread decision surfaced via periodic scan).

Decision brief — how to define the app/config exclusion in the clean skill
Parked thread: #869 (comment) discussion_r3624458682 (Codex P2, VALID-deferred to author).

Problem. This PR adds a home-target exclusion — an entry is not triaged if it "does not belong to a recognizable app/config convention" (plugins/disk-hygiene/skills/clean/SKILL.md:89-94). "Recognizable convention" is undefined, so the model can suppress an unhinted export.json or runner-controller-status.json on name appearance alone — the very artifacts this change means to surface.

Today. The exclusion is a free-text heuristic judged by the model at runtime. The only concrete exclusion in the skill is protected_exact_names — an explicit exact-name allowlist in reference/baseline-policy.json (.git, Downloads, NTUSER.DAT, …). Hints are name_glob patterns (*.tmp, tmp-*). Neither backs "recognizable app/config convention"; that phrase appears once (SKILL.md:90) with no list and no positive-evidence test behind it. The sibling plugins/repo-hygiene/skills/clean/SKILL.md has no analogous name-based exclusion, so this is a disk-hygiene-local decision.

Why the risk is asymmetric (the spine). This is a destructive-adjacent flow, but deletion is gated: §3 tiers (SKILL.md:109-115) keep every Low-confidence / uncertain entry "unless the human separately reviews and approves exact paths," and execution binds per-path approval tokens. So the two failure modes are not symmetric:

Options.

  1. RECOMMENDED — Require documented ownership (positive-provenance test). Reframe the exclusion so an entry is skipped only when it has positive ownership evidence — a manifest, documented naming contract, owning-tool state, or sibling structure — i.e. the §2 evidence ladder already written at SKILL.md:98-105. Absence of that evidence → surface for triage. This targets Codex's hole precisely (kills name-appearance-only exclusion), enumerates nothing (no app catalog to maintain or let drift), reuses machinery the skill already has, and moves the exclusion in the safe (surfacing) direction. Cost, named honestly: more entries surface each run (obviously-owned dotfiles with no manifest included) → triage noise — but the §3 tiers absorb it and nothing extra can be deleted. Reversibility: high (single-paragraph edit).

  2. Explicit exclusion allowlist. Extend the concrete protected_exact_names model to a curated list of known-good app/config conventions. Auditable and version-controlled. Tradeoffs: the app/config space is unbounded and cross-platform, so the list perpetually lags reality (though an incomplete allowlist errs toward surfacing — the safe direction); adds a maintenance surface that drifts; and the PR body already argues these artifacts "share no common name shape," so a positive convention list is thin on evidence. Reversibility: high, but accretes data over time.

  3. Keep heuristic (status quo of this PR). Lowest effort; leans on model judgment. Tradeoff: this is exactly Codex's concern — silent, unbounded under-surfacing with no operational definition. Bounded against deletion by the tier gates, but that does not fix the surfacing regression the change was written to close. Reversibility: high.

A hybrid of 1+2 (provenance test as the rule, protected_exact_names as the fast-path exact allowlist) is viable and mostly already the shape of the code; Option 1 is the minimal delta that resolves the thread.

Cross-links (same-family prior decisions).

No ruling — deferred to the PR author. RECOMMENDED marked per brief convention.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated Opened by automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

disk-hygiene:clean misses underscore-prefixed temp dirs and unowned agent-session files at a home-directory root

1 participant