feat(code-tidying): dissolve-comments empty-arg scope fallback ladder (0.14.0) - #3156
Conversation
… (0.14.0) (#3117) A clean tree no longer dead-ends the empty-argument invocation: scope now resolves down a ladder (uncommitted diff -> branch diff vs. base -> whole repository), with interactive confirmation before repo-wide widening and a deterministic safe-mode default for non-interactive runs. Zero-in-scope resolutions report a per-reason exclusion tally instead of exiting silently. Reconciles the .claude/ exclusion wording (safety.md prose vs. exclusions.md globs) by widening the GLOBAL HARD entry to .claude/** plus any settings-wired hook script wherever it lives, and promotes the SSOT/materialized-copy header check from Gotchas into Workflow step 1 (Scope). Closes #3117 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BGyDNAyN1Ld6KRrrEZvrJK
|
Claude finished @kyle-sexton's task in 38s —— View job Security review —
|
|
Last security-reviewed head: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c06e756ab
ℹ️ 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".
… both settings scopes (#3117) Two review findings on the scope-fallback change: the ladder now advances only when a rung does not exist (clean tree, no branch diff) — a rung that exists but yields no code files ends the run with the exclusion tally, so a docs-only branch never silently escalates to repo-wide scope; and the settings-wired hook-script exclusion now names both project settings scopes (.claude/settings.json and .claude/settings.local.json) across exclusions.md, safety.md, and tidy's orientation summary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BGyDNAyN1Ld6KRrrEZvrJK
|
Claude finished @kyle-sexton's task in 52s —— View job Security review —
|
…3117) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BGyDNAyN1Ld6KRrrEZvrJK
…egration-w4pxl1 # Conflicts: # plugins/code-tidying/.claude-plugin/plugin.json # plugins/code-tidying/CHANGELOG.md
|
Claude finished @kyle-sexton's task in 58s —— View job Security review —
|
|
Claude finished @kyle-sexton's task in 3m 11s —— View job Code review —
|
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
#3156 landed the dissolve-comments scope-fallback ladder on main and took 0.14.0, so this branch's 0.13.4 no longer sits above the published version and both the manifest and the changelog collided. * plugin.json takes main's description verbatim — #3156 rewrote the dissolve-comments clause — with the version moved to 0.14.1. * CHANGELOG keeps main's 0.14.0 entry intact and re-headings this branch's entry as 0.14.1 above it. The entry text is unchanged: it documents the audit-comment-residue escape-decode fix, which is untouched by #3156. No code conflict this time; #3156 does not touch audit-comment-residue. Refs #3126, #3140, #3156 Co-Authored-By: Claude <noreply@anthropic.com>
#3156 landed the dissolve-comments scope-fallback ladder on main and took 0.14.0, so this branch's 0.13.4 no longer sits above the published version and both the manifest and the changelog collided. * plugin.json takes main's description verbatim — #3156 rewrote the dissolve-comments clause — with the version moved to 0.14.1. * CHANGELOG keeps main's 0.14.0 entry intact and re-headings this branch's entry as 0.14.1 above it. The entry text is unchanged: it documents the audit-comment-residue escape-decode fix, which is untouched by #3156. No code conflict this time; #3156 does not touch audit-comment-residue. Refs #3126, #3140, #3156 Co-Authored-By: Claude <noreply@anthropic.com>
…#3151) No related issue: follow-on to the limitation #3140 recorded at its own parse site. #3126 is already closed by that PR, and no issue tracks the residual escape-decode gap. ## Summary #3140 fixed the #3126 false negative by slicing the v1 porcelain record, and recorded the remaining limitation at the parse site: > Git's octal escapes for control and non-ASCII bytes are still not decoded by either, so such a path continues to miss. This PR closes that gap, in both parsers that carry it. **It is a follow-on to #3140, not a competing fix.** | Path | v1 renders as | v1 slice yields | |---|---|---| | `café.py` | `?? "caf\303\251.py"` | literal escape sequence — names nothing | | `tab<TAB>here.py` | `?? "tab\there.py"` | literal `\t` — names nothing | Both confirmed against git 2.55. The file is silently dropped, so the run reports a clean tree — the same false-negative class #3126 described. ## Fix Both porcelain parsers in this skill move to the NUL-delimited `--porcelain -z` form, which git documents as performing no quoting or backslash-escaping, so there is nothing left to decode. - **`detect.sh`** — the audit's target router. - **`SKILL.md`'s `Uncommitted code files:` preview** — the pre-computed context the model reads. #3140 deliberately brought this to parity and added a test that extracts and runs it, so leaving it behind would have reopened the divergence that test exists to prevent: the audit would find `café.py` while the preview listed nothing. Under `-z` a rename emits the **new** path first and the original as a following record — the reverse of v1's display order — and that second record is consumed and dropped. Rename handling is therefore structural, with no arrow matching, which also resolves the intent-to-add rename #3140 gated on the worktree status letter (` R dst\0src\0` → `dst`, verified against git 2.55). ## Verification - **53/53 pass**, including every one of #3140's checks. - Nothing vacuous, checked by reverting each piece: - `main`'s `detect.sh` → fails 2 of case 11's 5 assertions (`non-ASCII`, `tab-bearing`). The arrow assertion passes on `main` — #3140's `[RC]` gating already handles it, **not** claimed here. - `main`'s `SKILL.md` → fails 3 assertions, including #3140's own `SKILL.md preview covers every file detect.sh audits`. - The parity harness was itself masking the defect and is fixed here. It extracted only the awk program and hardcoded the porcelain invocation; feeding `-z` to a v1 program makes the v1 program look correct, because `-z` output carries no quoting for it to fail at decoding. It now reads the invocation from `SKILL.md` too, and `REPO13` gains a non-ASCII fixture. - `mawk 1.3.4` (the runner's default `awk`) confirmed to support `RS = "\0"` before relying on it. - Local gates green: typos, shell-portability (scripts and `SKILL.md`), skill-portability, skill-precompute-compose, changed-skills, leaf-names, count-claims, fixture-git-isolation, orphaned-fixtures, manifest-duplicate-keys, `bash -n`, changelog-parity in all three modes. - `ci-status` (the required aggregate check) reported **success** on head `65a7b2c`; head `d7b2b67` adds only a `main` merge plus the version rebase. ## Current state — read this before continuing Head is **`d7b2b67`**, version **`0.14.2`**. `main` published its own `0.14.0` (#3156) and `0.14.1` mid-review, so the version was rebased twice; the changelog keeps each published entry intact with this PR's entry above them. **One blocker remains, and it is not about the code.** The branch's commits are signed with a key that is not registered on the committing GitHub account, so they report `verified=false, reason=unknown_key`. That trips two ruleset rules: - `required_signatures` - `require_extra_approval_for_unattributed_changes` Everything else is satisfied: all four required checks green, all review threads resolved, no merge conflicts, and `required_approving_review_count` is **0** — so once the signature question is settled this PR needs no human approval. Two ways to settle it: 1. **Register the signing key** on the account (Settings → SSH and GPG keys → New SSH key, type *Signing Key*). No push needed; the existing commits become verified, and the commit history is preserved. 2. **Rebuild the branch through the GitHub API.** Commits created via the API are signed by GitHub and verify automatically — that is why every commit on `main` shows `committer=noreply@github.com, verified=true`. Since the repo is squash-merge only, collapsing this branch's commits loses nothing that would survive the merge anyway. ## Related - Refs #3140 — landed the v1 slice this builds on; its `0.13.3` entry and full test suite are preserved here. - Refs #3156 — bumped `code-tidying` to `0.14.0` mid-review. - Refs #3126 — the original bug, closed by #3140. Not reopened here. - Refs #3164 — `audit-noise` carries the same defect class. #3171 fixed its rename-split half; the octal-escape half is still open and tracked there. **Not fixed here** — separate plugin, separate version bump.
Closes #3117
Summary
/code-tidying:dissolve-commentsinvoked with an empty argument dead-ended on a clean working tree (friendly no-op exit, no fallback). The empty argument now resolves down a scope ladder, and the three related findings from the issue — the.claude/exclusion-wording divergence, the SSOT header check living only in Gotchas, and silent zero-in-scope exits — are fixed in the same pass. Ships ascode-tidying0.14.0.Fix
skills/dissolve-comments/SKILL.md): empty argument resolves uncommitted diff → current branch's diff vs. the base/default branch → whole repository. Repo-wide widening is confirmed with the user in interactive sessions; non-interactive runs proceed deterministically but take any widened rung in safe mode (class-A deletions only, class-B as proposals), never full default mode..claude/exclusion wording reconciled:tidy/reference/exclusions.md's GLOBAL HARD entry widened from enumerated globs (.claude/hooks/**et al.) to.claude/**in full, plus any script wired as a hook command in.claude/settings.jsonwherever it lives;dissolve-comments/reference/safety.mdandtidy/SKILL.md's orientation summary now state the same list, so a literal reader of any surface reaches the same answer.Verification
scripts/allowed-tools-pairing.test.sh(all checks),skills/audit-comment-residue/scripts/detect.test.sh(27/27),skills/tidy/scripts/open-pr-count.test.sh(9/9).markdownlint-cli2on all six changed markdown files: 0 issues;typosandeditorconfig-checkeron all seven changed files: clean;check-jsonschemavalidatesplugin.jsonagainst the manifest schema..claude/glob list.main; the diff is confined toplugins/code-tidying/.Related
N/A
🤖 Generated with Claude Code
https://claude.ai/code/session_01BGyDNAyN1Ld6KRrrEZvrJK
Generated by Claude Code