fix(code-tidying): decompose docs-prose lane to per-section merge (#701) - #726
Conversation
The docs-prose lane declared undeclared wholesale replacement ("a project
lane overrides this file entirely"), forbidden by the consumer-config
layering contract. It froze bundled watch-for pattern improvements (P-1..P-6)
out of any repo that customized Scope.
Decompose per the #647 precedent: a project lane now merges per-section via a
declared `## Merge semantics` block — Scope is a per-section override,
watch-for patterns are additive (concatenate onto the bundled set). The tidy
resolution engine reads both the project and bundled layers and merges per the
lane's declaration; lanes without the declaration resolve project-only
(legacy path), so unmigrated lanes are unchanged.
Sibling deviations tracked, not folded in: the .local.* single-layer gap
(#723) and shell-tooling's identical wholesale language (#724).
Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 28s —— View job PR Review
Overall AssessmentThe approach is sound: using the presence of a One documentation clarity issue and one minor gap below. Finding 1 — Ambiguous "this section" in bundled lane (medium)File: "This section" here refers to the
The presence of this section is what tells `/code-tidying:tidy` to read **both** the project
lane and this bundled lane and merge them, rather than resolving the project lane alone.
When a project lane at `.claude/tidy-lanes/docs-prose.md` includes a `## Merge semantics`
section, `/code-tidying:tidy` reads **both** that project lane and this bundled lane and merges
them per that declaration. A project lane without this section resolves project-only.Finding 2 — No documented escape hatch for opting out of merge mode (low)
Given the intent is that merge mode is the better path and project-only is the legacy path, this is low priority. But a single parenthetical in the scaffolding step 4 description would close the gap:
Other observations (no action required)
Finding 1 should be fixed before merge (misreads as "bundled section triggers merge" when the engine actually checks the project layer). Finding 2 is optional cleanup. |
…ing contract Replace two hardcoded raw.githubusercontent.com URLs with repo-relative markdown links to docs/conventions/consumer-config-layering/README.md, which lives natively in this repo. The raw URL fetched external content for a same-repo file — the anti-pattern issue #432 flags. Relative links render natively on GitHub, need no network fetch, and survive branch renames.
|
Claude finished @kyle-sexton's task in 3m 6s —— View job PR Review
Overall AssessmentThe core change is sound and the second commit (fb51db4) correctly switches raw.githubusercontent.com URLs to repo-relative links — but introduces a new low-severity concern. Finding 1 from the prior review remains open and should be resolved before merge. Finding 1 — Ambiguous "this section" in bundled lane (medium — unresolved from prior review)File: This text lives inside the bundled lane's own Suggested replacement: When a project lane at `.claude/tidy-lanes/docs-prose.md` includes a `## Merge semantics`
section, `/code-tidying:tidy` reads **both** that project lane and this bundled lane and merges
them per that declaration. A project lane without this section resolves project-only.Finding 3 — Relative links exit the plugin directory (low — new, from fb51db4)Files: The fb51db4 commit replaces
The CLAUDE.md design rule says "No Both links are informational-only (the merge behavior is described inline, so an LLM running the skill doesn't need to fetch the linked file), which keeps severity low. But if the LLM ever does attempt to resolve them from an installed plugin context, the paths won't exist. The commit message's stated rationale ("anti-pattern issue #432 flags") is valid for in-repo links that render on GitHub — but these links target a file outside the plugin bundle, which is the exact case raw URLs are correct for. Options:
Finding 2 — No documented escape hatch for merge-mode opt-out (low — unresolved from prior review)Still unaddressed, still low priority. A parenthetical in
Optional — does not block merge. Other observations (no action required)
Finding 1 should be fixed before merge. Finding 3 warrants a conscious decision (revert or accept). Finding 2 is optional cleanup. |
…isambiguate merge-semantics prose Revert fb51db4's relative-link change. The consumer-config layering contract README lives at the marketplace-repo root, outside the code-tidying plugin's own bundle, so it does not ship when the plugin is installed elsewhere — a relative `../` link resolves to nothing in an installed context, and this repo's CLAUDE.md forbids `../` reach-outs from plugin files. The raw.githubusercontent URL is the correct tool for a cross-plugin-bundle-boundary reference (not the #432 anti-pattern, which is about content that lives natively in-bundle). A prior review had already ratified these raw URLs as intentional. Also disambiguate the bundled lane's "presence of this section" wording: it referred to the PROJECT lane's own `## Merge semantics` section, but could be misread as the bundled lane's (always-present) section. Co-Authored-By: Claude <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 38s —— View job PR Review (3rd pass — commit 23716ce)
Overall AssessmentAll medium-severity findings from the prior two reviews are now resolved. The PR is clean and ready to merge. Finding 1 — Ambiguous "this section" ✅ ResolvedFile: The ambiguous When a project lane at `.claude/tidy-lanes/docs-prose.md` includes a `## Merge semantics`
section, `/code-tidying:tidy` reads **both** that project lane and this bundled lane and merges
them per that declaration. A project lane without this section resolves project-only.This now correctly attributes the merge trigger to the project layer, consistent with Finding 3 — Relative links exiting plugin directory ✅ ResolvedFiles: Both relative Finding 2 — No documented escape hatch for merge-mode opt-out (low — unresolved, non-blocking)
Other observations (no action required)
This PR is ready to merge. Findings 1 and 3 (medium and low) resolved; Finding 2 (low) is explicitly non-blocking. |
|
Post-green review pass complete (rule 8). Bot review classification (3 rounds total on this PR):
No inline threads, CI fully green (18/18). Flipping to ready for review; handing off to the babysit lane for merge. |
|
Claude finished @kyle-sexton's task in 1m 40s —— View job PR Review (final pass — ready_for_review trigger)
Overall AssessmentThis is a clean, well-scoped conformance fix. All medium-severity findings from the prior three review rounds are confirmed resolved at HEAD (23716ce). No new issues found. Prior Findings — Status at HEADFinding 1 — Ambiguous "this section" wording ✅ Resolved
This correctly attributes the merge trigger to the project layer and is consistent with Finding 3 — Relative links exiting plugin directory ✅ Resolved Both links in Finding 2 — No documented escape hatch for merge-mode opt-out (low, non-blocking)
Additional Verification
This PR is ready to merge. Findings 1 and 3 (medium and low) resolved; Finding 2 (low) is explicitly non-blocking and left for follow-up. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23716ce355
ℹ️ 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".
…docs-prose-decompose # Conflicts: # plugins/code-tidying/.claude-plugin/plugin.json # plugins/code-tidying/CHANGELOG.md
|
Claude finished @kyle-sexton's task in 2m 18s —— View job Security Review
ScopeChanged files reviewed: Finding 1 — Mutable
|
|
Claude finished @kyle-sexton's task in 1m 12s —— View job PR Review (post-sync pass — merge with origin/main)
Overall AssessmentThe sync merge ( Prior Findings — Status at HEAD (0baa62a)Finding 1 — Ambiguous "this section" wording ✅ Resolved (unchanged from prior pass)
Finding 3 — Relative links exiting plugin directory ✅ Resolved (unchanged from prior pass) Both Finding 2 — No documented escape hatch for merge-mode opt-out (low, non-blocking) Unchanged and explicitly non-blocking. Open Review Thread (Codex)The unresolved Codex thread on No new technical issues were introduced by the sync merge. This PR remains ready to merge. The only merge blocker is the unresolved Codex review thread — the finding itself describes intentional documented behavior and warrants a maintainer close as "by design." |
There was a problem hiding this comment.
💡 Codex Review
claude-code-plugins/plugins/code-tidying/skills/setup/SKILL.md
Lines 91 to 93 in 0baa62a
When setup apply creates a docs-prose override, this instruction copies the entire bundled lane, including ## Merge semantics and P-1..P-6, then only retargets its scope/exclusions. The new resolver sees that declaration and concatenates the project's copied watch-for list with the bundled list, so every generic pattern is duplicated for newly scaffolded docs-prose lanes (and copied non-pattern sections also mask future bundled updates). Scaffold this lane as a sparse merge-layer containing only its declaration and the sections the project changes, rather than copying the complete bundled file.
ℹ️ 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".
Summary
The
code-tidyingdocs-proselane declared undeclared wholesale replacement —docs-prose.md:14said a project lane at.claude/tidy-lanes/docs-prose.md"overrides this file entirely". This is forbidden by the consumer-config layering contract ("Merge semantics"): it froze the generic watch-for patterns (P-1..P-6) out of any repo that customizedScope, so bundled pattern improvements could never reach it. Ruling #649 dispositioned this surface CORRECT (decompose, don't ratify).Fix
Mirrors the sanctioned #647 per-key precedent (
source-control):docs-prose.mddecomposed with a declared## Merge semanticssection.Scopeis a per-section override (retargets doc globs); the generic watch-for patterns are additive — a project's entries append to the bundled P-1..P-6 rather than replacing them, so bundled improvements keep flowing. Every other section is per-section override with fall-through to the bundled value when omitted.tidy/SKILL.mdLane resolution + Phase A step 2): when a project lane declares## Merge semantics, the engine reads both the project and bundled layers and merges per that declaration — no longer "read one and stop". A lane with no declaration resolves project-only (legacy path), so unmigrated lanes are byte-identically unchanged (opt-in migration).setup/SKILL.mdprecedence wording aligned to the per-lane declared semantics.code-tidyingupdated out of the single-layer/undeclared framing to the honest post-merge shape..local.*single-layer gap → code-tidying: setup refuses .local.* overlay — single-layer gap (split from #701) #723;shell-tooling's identical wholesale language → code-tidying: shell-tooling lane carries same undeclared wholesale replacement as #701 #724.Verification
Branch fast-forwarded onto current
origin/mainbefore commit; diff is the 6 files below only.Relative-link integrity: the new
[Merge semantics](#merge-semantics)anchor resolves to the added## Merge semanticsheading in the same file.Closes #701
Related
source-controlper-key precedent this mirrors.local.*single-layer gap (tracked, not folded in)shell-toolingidentical wholesale language (tracked, not folded in)