Skip to content

feat(disk-hygiene): catalog scope, ownership investigation and cross-target answer reuse - #5630

Merged
kyle-sexton merged 10 commits into
mainfrom
feat/4008-catalog-scope-procedure
Oct 1, 2026
Merged

kyle-sexton merged 10 commits into
mainfrom
feat/4008-catalog-scope-procedure

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Refs: #4008

Summary

Completes the remaining unmet parts of the disk-hygiene investigated catalog after slice 1 (#5544): catalog scope with an uncataloged report and owner-level markers, cross-target reuse of operator answers by identity, and the required local ownership investigation with presence-gated /discovery:research escalation.

Fix

  • investigated_catalog.py: catalog_scope (immediate children, hinted or empty at any depth, out-of-place at a home or --root-children target), owner_level records covering descendants while identity holds, uncataloged in the report, source: human answers reused across scan targets by identity.
  • hygiene.py catalog: passes the positional flag; scan stdout and the engine grammar/guard are unchanged.
  • reference/ownership-investigation.md and SKILL.md: required per-entry procedure with nine enumerated local sources, conclusion in provenance and each evidence source in the finding's evidence list, research only when no owner is found and only when present, one closing question per unknown owner (stays keep).

Verification

Acceptance criteria (all in plugins/disk-hygiene/skills/clean/):

  1. catalog.json + CATALOG.md with the record shape: test_record_carries_the_issue_shape, test_catalog_writes_both_files_and_scan_annotates.
  2. prior_disposition, changes first, one line for unchanged: test_prior_disposition_requires_the_same_identity, test_rendered_markdown_leads_with_changes_and_ends_with_questions, test_an_answer_reused_from_another_target_is_reported_unchanged (an entry answered under another scan target gets one unchanged line, answered under <target>).
  3. Identity or descendant change invalidates: test_identity_or_descendant_change_invalidates.
  4. Disposition never skips approval, preview or revalidation: test_catalogued_remove_does_not_bypass_preview_approval_or_revalidation.
  5. Scope and owner level: CatalogScopeTest (test_scope_names_each_entry_shape, test_an_owner_level_record_covers_its_descendants, test_a_home_target_marks_loose_root_entries_out_of_place).
  6. Procedure ships and is enumerated: reference/ownership-investigation.md, SKILL.md:267.
  7. Research only when no owner and only when present: reference/ownership-investigation.md:45, with /discovery:explore gated the same way.
  8. Closing questions, source: human, not re-asked: test_unanswered_unknown_owner_stays_keep_and_is_asked, test_human_keep_answer_is_not_reasked_while_identity_holds, test_operator_answer_is_reused_by_identity_from_another_target, test_an_open_question_yields_to_an_answer_from_another_target (a record here that still has an open question yields to an answer recorded under another target), test_an_open_question_stays_when_no_other_target_has_an_answer.
  9. Unanswered stays keep: test_unanswered_unknown_owner_stays_keep_and_is_asked.
  10. scripts/affected-tests.sh --run: NOT fully met. It selected 293 shell suites and ran all of them sequentially (no OOM this time). 290 pass; 3 fail for host reasons unrelated to this diff: scripts/check-html-assets.test.sh and scripts/check-script-contract.test.sh (htmlhint absent from node_modules in the worktree), scripts/hook-census.test.sh (strace missing). Of the 18 selected Python suites run individually, all disk-hygiene ones pass; test_save_point.py (no pytest) and test_overlap.py (host inventory degraded) fail for host reasons. python3 -m unittest discover -s plugins/disk-hygiene/skills/clean/scripts -p 'test_*.py': 840 tests OK.

Remaining: a green affected-tests.sh --run on a host with htmlhint and strace (CI covers this).

Related

🤖 Generated with Claude Code

kyle-sexton and others added 5 commits September 30, 2026 16:31
…ets by identity

A record with source human whose identity and descendant set still hold now
matches the same entry when another scan target reaches it, so the answer is
annotated and not asked again. The scan target itself gets
target_prior_disposition. Engine records stay per target and path. A changed
identity or descendant set still invalidates the answer, and preview and apply
still do not read the catalog.

Refs #4008

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…scope entries

The catalog command now names the entries a catalog must account for: every
immediate child, every hinted or genuinely empty entry at any depth, and at a
user-home or root-children target every loose unprotected unhinted root entry.
Any in-scope entry with no record and no owner-level ancestor is listed under
uncatalogued, so nothing that looks out of place is skipped. A finding or
answer with an owner and owner_level covers its whole subtree, one record per
owning tool instead of one per file.

Refs #4008

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ence-gated research escalation

Refs #4008

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bump disk-hygiene to 0.41.0 above main's 0.40.0 and keep both changelog entries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

PR body contract — issue linkage

This PR body conforms to the issue-linkage contract. Nothing to do.

kyle-sexton and others added 3 commits September 30, 2026 17:23
- The catalog report lists an entry matched to an operator answer recorded
  under another scan target as one `unchanged` line, `answered under <target>`,
  so it no longer drops out of every list.
- The ownership procedure names the record field `provenance` and puts each
  evidence item's source into the finding's `evidence` list, which is what the
  catalog reads.
- `/discovery:explore` is used only when it resolves, with a fallback to the
  local sources.

Refs #4008

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…en question

An engine record with an open question under the scanned target hid a
source: human answer recorded under another target for the same identity and
descendant set, so the question was asked again and the entry annotated
prior_unresolved. sync_catalog and annotate_entries now let a holding
operator answer from another target replace a local record that still has a
question; a local record without one still decides.

Rename the report key and heading uncatalogued to uncataloged so the typos
gate passes, take SKILL.md back under the 500-line cap, and rephrase the
%APPDATA% path in ownership-investigation.md so the shell-portability gate
does not read it as a GNU-only construct.

Refs: #4008

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve the disk-hygiene plugin.json and CHANGELOG conflicts: keep the
0.41.0 to 0.41.2 entries from main and release this branch as 0.42.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 1, 2026 01:18
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-01T01:24:04.782182Z 01ff8c4 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

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


Review complete ✅

  • Gather context (PR body, diff, comments)
  • Understand request (invoke /review:code-review)
  • Run code review per repo convention
  • Post inline findings
  • Final summary

Ran the review:code-review CI lane over the diff (git diff origin/main...HEAD), scoped to the 8 changed files. Focused on investigated_catalog.py (new catalog_scope, _human_by_identity/_other_target_answer cross-target reuse, owner_level) and the hygiene.py wiring, since that's where this PR's new logic lives; SKILL.md, the two reference docs, plugin.json, and CHANGELOG.md are documentation/metadata and didn't turn up findings at this bar. Security-shaped concerns are out of scope for this lane (separate security lane).

Two findings posted as inline review comments:

  1. hygiene.py:353 — catalog_target_is_positional compares the home directory with Path equality (case-sensitive string compare), reintroducing the exact case-insensitive-filesystem pitfall that large_scan_reasons in the same file (hygiene.py:2176-2196) explicitly documents and avoids via os.path.samefile. On a case-insensitive macOS volume this can silently drop the home-target out-of-place scoping. The one test for this path mocks user_home() to return the identical Path object already used to build the target, so it can't catch the mismatch.

  2. investigated_catalog.py:114-135 — _other_target_answer picks the first matching cross-target human answer in an order driven by alphabetical (target, path) sort, not recency. Since nothing reconciles or supersedes an older target's human record when the same identity is re-answered under a different target, a catalog can end up holding two conflicting human records for one identity — and whichever target's name sorts first wins forever, even when the other is the operator's more recent decision. Walked through a concrete repro in the comment. No test exercises two targets independently holding conflicting human answers for the same identity.

Both are plausible-but-unverified-by-execution: I traced them through the code and existing tests rather than running a failing repro script. Author-claimed verification in the PR body (affected-tests.sh, the 840-test unittest run, etc.) is noted but not independently re-run here.

Branch: feat/4008-catalog-scope-procedure

@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review — /review:security-review

  • Gather context (PR diff, changed files)
  • Check skip-gate conditions
  • Review changed files for security issues
  • Check instruction-surface deletions (safety-model.md, SKILL.md changes)
  • Post findings

Scope: plugins/disk-hygiene/skills/clean/scripts/investigated_catalog.py, hygiene.py, SKILL.md, reference/safety-model.md, reference/ownership-investigation.md (new), test_investigated_catalog.py, CHANGELOG.md, plugin.json. Diffed against origin/main.

Findings

No security issues found that meet this lane's bar (logic/trust-boundary flaws with a concrete exploit path). Details below.

Deletion-authority invariant holds. The whole feature (catalog_scope, owner_level, cross-target answer reuse via _other_target_answer/_human_by_identity) only ever writes prior_disposition / target_prior_disposition hints and the catalog/uncataloged report. I traced every write site in hygiene.py (annotate_investigated_catalog, the catalog subcommand) and confirmed preview/apply code paths never read catalog records — the module's own invariant ("a record cannot grant deletion authority or shorten any approval step") is preserved, consistent with the existing test_catalogued_remove_does_not_bypass_preview_approval_or_revalidation test. investigated_catalog.py#L1-L6

Instruction-surface check. reference/safety-model.md narrows a standing rule — "a record belongs to one scan target: the same entry reached from another target is not annotated and is asked again" is replaced by automatic cross-target reuse of source: human answers by filesystem identity (safety-model.md#L862-L895). I checked this against the six Gate 0 classes in docs/conventions/instruction-exception-register/README.md (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority). It doesn't match: the rule being loosened governs only report/question de-duplication, not the actual irreversible-action gate (preview + approval token + revalidation), which is a separate, untouched mechanism confirmed above. No finding.

Cross-target identity matching (_other_target_answer, identity_holds) keys on (device, inode, kind) and falls back to no-match when either is falsy, so it fails safe (re-asks) rather than open on ambiguous identity. The _uncataloged/covered() ancestor-prefix check splits on / and compares full path components, avoiding a naive-prefix ("foo" matching "foobar/baz") bug. Owner-level coverage is computed from post-invalidation records in the same scan, so a changed descendant set under an owned directory correctly re-exposes its children instead of silently staying covered.

No GitHub Actions changes in this diff (zizmor's lane).

Comment thread plugins/disk-hygiene/skills/clean/scripts/hygiene.py Outdated
Comment thread plugins/disk-hygiene/skills/clean/scripts/investigated_catalog.py

@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: 01ff8c4284

ℹ️ 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/scripts/investigated_catalog.py
Comment thread plugins/disk-hygiene/skills/clean/scripts/investigated_catalog.py
Comment thread plugins/disk-hygiene/skills/clean/scripts/investigated_catalog.py
kyle-sexton and others added 2 commits September 30, 2026 21:31
Newest operator answer supersedes older ones for the same entry under other
targets, reused owner-level answers cover descendants, the home target match
uses filesystem identity, sizes-only snapshots are refused, and identity values
must be scalars.

Refs: #4008

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review (/review:code-review) ✅

  • Gather context (PR body, diff, prior comments)
  • Invoke /review:code-review and follow it exactly
  • Review changed files for correctness/maintainability findings
  • Check whether this head's new commits address the prior review round
  • Final summary

Summary

This invocation runs on HEAD 476c442 (git diff origin/main...HEAD, 8 files). Since the
last review round (on 01ff8c4), one new commit landed: 2171b84 — "harden catalog
cross-target reuse and scope"
. I diffed it directly and traced its effect through
investigated_catalog.py and hygiene.py.

That commit fixes all five previously open findings, each with a corresponding new test:

  1. Case-insensitive home-target match (prior finding) — catalog_target_is_positional now uses os.path.samefile(snapshot["target"], home) (guarded for OSError) instead of Path equality, matching the existing large_scan_reasons pattern. hygiene.py:344-357. Covered by test_a_home_target_spelled_through_a_link_is_still_the_home.
  2. Stale cross-target answer could permanently shadow a newer one (prior finding) — a new _supersede helper deletes every other target's human record for the same filesystem identity whenever a fresh human answer is recorded, so there's never more than one live human record per identity. investigated_catalog.py:114-133. Covered by test_the_latest_answer_for_an_object_supersedes_older_ones.
  3. Reused owner-level answers didn't cover descendants (Codex P2) — _uncataloged's owners set now also includes reused paths with owner_level, plus a target_owned short-circuit for a reused answer on the scan target itself. investigated_catalog.py:199-205. Covered by test_a_reused_owner_level_answer_covers_its_descendants / ..._for_the_target_covers_every_entry.
  4. --sizes-only snapshot silently produced an empty, misleading report (Codex P2) — catalog now raises HygieneError on inventory_mode == "sizes-only" before writing anything. hygiene.py:5774-5778. Covered by test_catalog_refuses_a_sizes_only_snapshot.
  5. Non-scalar identity values could crash indexing with TypeError (Codex P2) — _well_formed now rejects a loaded record whose device/inode/kind isn't None/str/int. investigated_catalog.py:329-332. Covered by test_an_identity_with_a_non_scalar_value_is_ignored.

I traced each fix against its call sites (sync_catalog, annotate_entries, _other_target_answer,
_human_by_identity) looking for a new edge case the fix might have introduced — e.g. whether
_supersede could delete a record under the same target (it can't, it's gated on key[0] != target), and whether a reused answer that gets superseded by a fresh same-target answer could
double-report in _report (it can't, the reused path is also in state once re-answered, so
the reused-only reporting branch skips it). No new issue found.

Not independently executed: the sandbox here denies running python3 -m unittest/pytest, so
I verified by reading the code and the new tests, not by running them. The PR body's own
affected-tests.sh / unittest discover results are author-claimed and not re-verified by this
pass.

No new findings on this round. No inline comments posted — static tracing found nothing new to
flag at this lane's bar (security-shaped concerns are out of scope here per the separate security
lane, which already reviewed this diff with no findings).
· branch feat/4008-catalog-scope-procedure

@claude

claude Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review — /review:security-review

  • Gather context (PR diff, comments, prior review history)
  • Check skip-gate conditions
  • Review changed files for security issues (including the latest commit since the last security pass)
  • Check instruction-surface deletions (safety-model.md)
  • Post findings

Scope: full PR diff origin/main...HEAD at 476c442 — investigated_catalog.py, hygiene.py, SKILL.md, reference/safety-model.md, reference/ownership-investigation.md (new), test_investigated_catalog.py, CHANGELOG.md, plugin.json.

Findings

No security issues found that meet this lane's bar (logic/trust-boundary flaws with a concrete exploit path).

This head includes a new commit (2171b84, "harden catalog cross-target reuse and scope") since the prior security pass on 01ff8c4. I re-reviewed the full diff rather than relying on that prior pass, since the new commit touches the same functions. It addresses, as correctness/robustness fixes rather than security fixes, several points raised in code review on the earlier commit:

  • catalog_target_is_positional (hygiene.py#L344-L358) now compares via os.path.samefile instead of Path equality, matching the pattern already used by large_scan_reasons.
  • _supersede (investigated_catalog.py#L117-L135) now drops older cross-target human records for the same filesystem identity whenever a new human answer is recorded, so a stale answer can no longer outrank a newer one by alphabetical target-name sort.
  • _well_formed (investigated_catalog.py#L326-L330) now validates that identity field values are hashable scalars before they're used as dict-index keys.
  • catalog now rejects a --sizes-only snapshot (hygiene.py#L5774-L5778) instead of silently reporting an empty scope.

Deletion-authority invariant holds. catalog_scope, owner_level, _other_target_answer/_supersede only ever write prior_disposition / target_prior_disposition hints and the catalog/uncataloged report. Preview and apply code paths still never read catalog records — confirmed via the unchanged test_catalogued_remove_does_not_bypass_preview_approval_or_revalidation test and by re-tracing every write site in hygiene.py's catalog subcommand and annotate_investigated_catalog.

Instruction-surface check (re-verified against this head). reference/safety-model.md narrows "a record belongs to one scan target" to allow cross-target reuse of source: human answers by filesystem identity (safety-model.md#L876-L897). Checked against the six Gate 0 classes in docs/conventions/instruction-exception-register/README.md: this governs only report/question de-duplication, not the irreversible-action gate (preview + approval token + revalidation), which remains untouched and enforced elsewhere. No finding.

Cross-target identity matching keys on (device, inode, kind) with a falsy-value fallback to no-match, so ambiguous identity fails closed (re-asks) rather than open. _supersede only deletes records matching the just-answered identity, scoped to source: human records, not engine records or unrelated entries — no broader deletion surface introduced.

No GitHub Actions changes in this diff (zizmor's lane).

@kyle-sexton
kyle-sexton merged commit b906919 into main Oct 1, 2026
20 checks passed
@kyle-sexton
kyle-sexton deleted the feat/4008-catalog-scope-procedure branch October 1, 2026 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant