CodeRabbit findings against the carried skills tree, raised on a downstream carry

[ptr727]

Carrying .github/skills/ into ptr727/NxWitness (ptr727/NxWitness#550) put the whole tree in front of CodeRabbit as new files. It raised 16 findings against hub canonical content.

They are filed here rather than fixed there, per the carried-content review rule: the tree is manifest-owned, carry.py check reports matching source and target digests, and a local edit would fail the next fidelity check. So a substantive defect's fix belongs at its canonical source, which is this repository.

A caveat on confidence. I verified one of these against the upstream action's own action.yml and am reporting the rest as the reviewer stated them, unverified by me. I filed a batch of unverified claims on #1116 earlier and had to retract six of ten, so I would rather label the uncertainty than repeat that.

Verified

operational-vs-release-workflow/references/branch-protection-and-promotion.md:109, the app-token action's version boundary. The guidance names v3.0.0 as the release where client-id arrived and app-id was deprecated. It is v3.1.0.

Reading that action's action.yml at each tag, the v3.0.0 input list contains app-id and no client-id; the v3.1.0 list contains both, client-id first. So a workflow pinned to v3.0.0 that follows this guidance omits the required app-id, and its token step fails.

Reported as stated, not independently verified

Grouped by what they claim:

Factual or logic defects

• git-commit-conventions/references/history-rewrite.md:10 - git filter-repo strips signatures rather than re-signing, so the procedure needs an explicit post-rewrite signing step before pushing to a signature-protected branch.
• merge-and-release/SKILL.md:75 - a missing releaseTrigger makes the jq return null, and step 5 dispatches for every value except none, so it fails open on null and on a typo.
• merge-and-release/SKILL.md:207 - the headRefOid check and the subsequent git branch -D / git push origin --delete are not a compare-and-swap, so the tip can move between them.
• git-commit-conventions/SKILL.md:80 - both identity probes accept any local part ending @users.noreply.github.com, so another account's noreply address passes a rule that means the committing account's own.
• branch-protection-and-promotion.md:38 - the normalized diff proves equality or difference only, so a nonzero result cannot distinguish a develop-only superset from conflicting main-only content before git checkout --theirs.
• standup-a-repo/SKILL.md:83 - configure.sh check returns 1 for both drift and an API failure, so the two are indistinguishable before configuration is applied.

Portability

• shell-codestyle/SKILL.md:48 - the skill covers sh shebang scripts, but set -E, pipefail and BASH_SOURCE are not POSIX sh, and some implementations exit on them. Scope those to Bash or define the covered interpreter.

Coverage gaps

• fleet-conformance-check/SKILL.md:39 - checks AGENTS.md and GOVERNANCE.md but not CLAUDE.md, which standup-a-repo defines as the required @AGENTS.md import. Worth weighting this one: NxWitness carried AGENTS.md for a long time with no CLAUDE.md, so Claude Code loaded no rules at all, and this check would not have caught it.
• code-review/SKILL.md:22 - does not route branch or publishing reviews to operational-vs-release-workflow.
• carried-instruction-file-guard/SKILL.md:17 - permits any existing hub checkout for the probe rather than requiring a fresh main fetch, so a stale canonical can classify content against retired rules. This matches something I hit directly: my probe was scoped to the six Markdown files and missed that most rule text now lives in the skills tree, which is how Promotion candidates and two interface gaps found resyncing NxWitness #1116 went wrong.
• copilot-instructions-keeper/SKILL.md:55 - inspection alone does not prevent deletion; the procedure could require promotion, preservation or relocation before overwriting content not confirmed hub-owned.
• repo-worktree/SKILL.md:177 - a clean tree can still be in use by another session, so switching it away from task-branch can disrupt that task. Require positive ownership rather than cleanliness.

Wording

• python-codestyle/references/profiles.md:8 - "dependencies in [dependency-groups]" should say development dependencies; runtime dependencies belong in [project].dependencies.
• comment-and-doc-style/references/line-endings.md:75 - for a byte-preserve directory, name the .gitattributes rule (<dir>/** -text or binary) alongside the .editorconfig override, since * text=auto eol=lf can classify a text-like binary as text.

Not a defect, for the record

CodeRabbit also flagged upstream-contribution-workflow/SKILL.md:60 as contradicting the never-force-push rule. It does not: the paragraph immediately after states that the rule "has no jurisdiction here" because the branch lives on the maintainer's own fork outside the fleet, and then narrows the allowance further. Declined on the downstream PR rather than filed.

A note on the shape of this

A downstream carry is the first time this tree gets read by a reviewer that has no stake in it. Sixteen findings on first contact suggests that is worth doing deliberately rather than as a side effect, since the hub's own PRs only ever show it these files as diffs.

[ptr727]

Same class, second downstream carry: ptr727/PlexCleaner took the .github/skills/ tree in ptr727/PlexCleaner#940 against hub a378121, and CodeRabbit raised 7 findings against it. Adding them here rather than opening a parallel issue.

Taking your caveat seriously, I verified each against the canonical before reporting rather than passing the reviewer's wording through. Two hold, one you already have, and two I think are wrong and declined on the downstream PR.

Verified, and not already in your list

python-codestyle/SKILL.md, the clean-compile and the CI gate are not the same command, but both passages say they are.

The local loop:

The build-profile Python clean-compile is uv run ruff format + uv run ruff check + the repo's type checker

and four lines later:

CI runs the same clean-compile commands as the authoritative backstop.

The CI gate:

The build-profile CI gate is uv run ruff check, uv run ruff format --check, the repo's type checker ... the same commands as the local loop above

ruff format rewrites files and ruff format --check fails on a diff. Each passage is individually right about what to run and both assert an identity that does not hold, so a reader reconciling them picks whichever they read second. The lint-only sentence inherits the same claim through "its uvx equivalents".

repo-worktree/SKILL.md:167, the continuation procedure's first command fails on a branch that was never pushed.

git -C ~/repos/<Repo> fetch origin <task-branch>
git -C ~/repos/<Repo> worktree add ~/repos/worktrees/<Repo>-<task-slug> <task-branch>

Reproduced against a throwaway local remote, with a branch that exists only in the base clone:

$ git fetch origin local-only-branch
fatal: couldn't find remote ref local-only-branch
fetch exit=128

The second command would have worked on its own, since the local branch is already there. The case is ordinary rather than exotic: a session interrupted before its first push leaves exactly this state, and the mandate's own "a continuation re-isolates" rule is what sends the next session down this path. The paragraph after the snippet covers the mirror case, a base clone holding only the remote-tracking ref, so the local-only direction reads like an oversight rather than a decision.

Already covered above

git-commit-conventions/references/history-rewrite.md, the explicit post-rewrite signing step. Same finding as the one already in your "Factual or logic defects" list.

Declined on the downstream PR, for the record

Listing these so nobody re-files them from a later carry.

fleet-conformance-check/SKILL.md, "pin and verify the hub checkout before running its Python entry point" (raised as Major/Security). The skill fetches mutable main and runs scripts/skills_install.py from it. That is deliberate rather than an oversight: GOVERNANCE.md "Hub-Hosted Tooling" and RESYNC.md section 1 both require reading main fetched immediately before use, precisely so a repo never runs a stale copy, and GOVERNANCE.md "Running the Linters Locally" states the same for the doc gates, calling pinning something Dependabot does not maintain "stale by construction". Trading that for a pinned commit is a fleet-wide policy change, not a skill-text fix. Flagging only in case you read the trade differently.

git-commit-conventions/SKILL.md:80, "use the target repository's effective Git configuration." Distinct from the noreply-local-part claim already listed at that same line. The reviewer wants the probe to read the target repo's effective user.email, commit.gpgsign, gpg.format and user.signingkey rather than --global in a scratch repo. That would invert the rule the probe enforces: the skill already says a repo-local user.email is "redundant where the global is right and a silently-shadowing wrong identity where it is not", and that a mismatch is "a host fault to surface to the maintainer, not to patch per repo". Honoring a repo-local override would make the probe pass on exactly the misconfiguration it exists to catch.

One narrow observation from looking at it, offered as a question rather than a finding: because the probe commits in a scratch repo, a repo-local commit.gpgsign=false in the target would not be exercised, so the probe could pass while that repo's commits are unsigned. Branch protection rejects them at push, so it surfaces either way, just later and less legibly. If the host-level model is the intent, this is by design and worth nothing more than a sentence saying so.

[ptr727]

Two more, from the same downstream carry. These are against GOVERNANCE.md rather than the skills tree, raised on ptr727/NxWitness#551 which carries GOVERNANCE.md for the first time. Both sit inside declared-verbatim sections and byte-match the hub, so the same disposition applies: filed here, not edited there.

Durable Knowledge and Self-Improvement - the destinations for durable knowledge do not include GOVERNANCE.md itself. Since this is the file holding the canonical cross-cutting rules, an agent that surfaces a durable cross-cutting rule and reads this section is not told to record it here. Reviewer's claim, and it reads correct to me on the text, though I have not tried to establish what the section's original intent was.

Repository Details - the section names repo-config/configure.sh apply as the command that writes the About panel, without marking it hub-hosted. That is fine in the hub, where the path resolves. It is misleading in every carrying repo that has retired its local repo-config/ per spec/divergences.json, which is now the intended end state for all of them: a reader runs the command, it is not there, and nothing in the sentence says to reach the hub for it. Hub-Hosted Tooling states the general rule, but this reference does not carry the marker.

The second is the more interesting one, because it is a class rather than an instance. Retiring repo-config/ downstream makes every carried reference to a repo-config/ path a dangling pointer in the carrying repo while remaining correct in the hub. NxWitness#551 hit this in its own prose too and fixed it by naming the behavior and the hub-hosted command rather than the path, six sites in WORKFLOW.md. The carried sections cannot be fixed that way downstream, so it wants doing at the canonical source. Worth a sweep of carried text for repo-config/ references, and possibly a prose_lint.py check, since the existing dead-path check exempts these paths via HUB_HOSTED precisely so the retirement does not fail CI, which also means it cannot flag the pointers the retirement leaves behind.

[ptr727]

Corroboration on the Repository Details / repo-config/ item: raised again independently on the same line, ptr727/NxWitness#551 GOVERNANCE.md:288, by Copilot after CodeRabbit had already flagged it. Two reviewers converging on the same carried line without seeing each other's report.

That is a better signal than either report alone, and it is worth reading as being about the class rather than the line. Any carried reference to a repo-config/ path becomes a dangling pointer in a repo that has retired its local copy, which spec/divergences.json now marks as the end state for every carrying repo. The reviewers see it because they read the carried file in the downstream tree where the path genuinely is not there.

Also worth noting the lint cannot help here by construction: prose_lint.py --check dead-path exempts these paths via HUB_HOSTED so the retirement itself does not fail CI, which is the same exemption that stops it flagging the pointers the retirement leaves behind. So this class is invisible to the mechanical check and visible to a first-time reader, which is roughly the worst combination.

[ptr727]

One more from the same carry, raised on the promotion pull request ptr727/NxWitness#556.

comment-and-doc-style/references/markdown-links.md:15 - the inline-link exception is a closed four-file list (AGENTS.md, GOVERNANCE.md, CODESTYLE.md, WORKFLOW.md). Docker/README.md uses inline links and is not on it.

That file is a real case rather than an oversight in one repo: a Docker-publishing repo keeps a separate Docker/README.md as the Docker Hub overview, and spec/readme-structure.md mandates it for exactly that purpose. Docker Hub renders it, and reference-style definitions parked at the bottom of a Hub overview are worse than inline links there for the same reason OPERATIONS.md takes the inline exception: nobody scrolls to a definition block on that surface.

So the question for the hub is whether the closed list should gain Docker/README.md for repos that publish images, or whether that file is expected to convert to reference style. Either answer is fine downstream; what a carrying repo cannot do is decide it locally, since the list is carried.

That is eighteen findings against this tree from one resync now, which I mention only because the count itself is the argument for reading the tree deliberately once rather than discovering it a repo at a time.

[ptr727]

Correction to my earlier comment on the inline-link exception, and a measurement that came out of getting it right.

I described the exception list as AGENTS.md, GOVERNANCE.md, CODESTYLE.md, WORKFLOW.md. That is wrong. comment-and-doc-style/references/markdown-links.md states it as the four files "read one section at a time rather than end to end": AGENTS.md, GOVERNANCE.md, OPERATIONS.md, and .github/copilot-instructions.md. I inverted two of the four. Caught by Copilot on ptr727/NxWitness#558.

That is my second wrong factual claim in this issue tracker in one session, after the six-of-ten retraction on #1116, and both have the same cause: asserting a carried rule from memory of having read it rather than re-reading the line. Worth saying plainly since I keep filing against this repo.

The measurement

Checking the corrected list against ptr727/NxWitness at the tip of #558:

File	Inline links	Reference links	Status
AGENTS.md	8	0	exempt
GOVERNANCE.md	29	0	exempt
OPERATIONS.md	5	0	exempt
.github/copilot-instructions.md	15	0	exempt
CODESTYLE.md	16	0	violates
WORKFLOW.md	12	0	violates
ARCHITECTURE.md	0	7	conforms
README.md	0	233	conforms
HISTORY.md	0	12	conforms
Docker/README.md	0	3	conforms after #558

CODESTYLE.md and WORKFLOW.md are carried files that violate a rule they themselves carry. This is not a NxWitness defect: the hub's own copies have the same inline links, so a downstream repo converting them would diverge from the canonical rather than fix anything, and the next resync would flag the divergence.

What this suggests

Both files are read section by section in practice, the same property the exemption exists for, and CODESTYLE.md's own preamble says a repo "reads only the section(s) for the languages it ships and ignores the rest", which is the exemption's rationale stated almost word for word. So the likelier answer is that the list is short by two rather than that both files need converting. Either way it wants deciding at the hub, because every fleet repo carries both files and whichever way it goes should be uniform.

Worth noting the list is described as "that closed list of four files, never a category to argue from case by case", which is a good rule and is also why the list being wrong is expensive: there is deliberately no principle to fall back on, so the enumeration has to be right.

The Docker/README.md question from my earlier comment stands unchanged and is separate: that file is off the list under either reading, and the question is whether a Docker Hub overview should join it.

[ptr727]

Second independent carrier, and this one lets me move two of your "reported as stated" entries into verified, because I hit both first-hand rather than reading them.

Raised by CodeRabbit on ptr727/LanguageTags#341, the develop -> main promotion that carries the skills tree to main after ptr727/LanguageTags#334 carried it to develop. Eleven findings, eight against the skills tree. All declined on that pull request under the same carried-content rule you used, with carry.py check reporting matching digests.

Verified first-hand, upgrading two of your entries

branch-protection-and-promotion.md:38, the normalized diff is not a superset check. You have this as reported-not-verified. I can confirm the failure mode is real, because the promotion I was running is exactly the case it breaks on.

main and develop had both moved independently: Dependabot and the codegen bot land on each branch in parallel by design, so main genuinely held three commits develop had never seen. Following that diff step and then git checkout --theirs would have been unsound, since a nonzero result there says only "these differ" and cannot distinguish a develop-only superset from main-only content about to be discarded. I verified the superset property by hand instead, with a review pass briefed specifically to hunt for main-only content the merge dropped, and only then took develop's side on the one conflicted file. It found none, but the documented check would not have told me that.

The concrete stake: one of main's three changes was a Dependabot bump inside a file develop deletes. Taking the deletion was correct only because the hub's own task carries the identical action SHA, which I had to go read to establish.

shell-codestyle/SKILL.md:48, set -E and pipefail are not POSIX. Also reported-not-verified in your list. Confirmed against a real script.

ptr727/LanguageTags's .husky/pre-commit is precisely the tracked extensionless sh-shebang script the skill's own target rule pulls in, and it cannot take the mandated header:

$ python3 -c '
import re
DASH=None
' ; dash -c 'set -Eeuo pipefail' ; echo "dash exit=$?"
dash: 1: set: Illegal option -E
dash exit=2

That hook now carries set -eu with a comment recording why, which is the POSIX subset. Copilot separately raised the absence of a header on the same file, so a downstream repository following the skill literally is caught between two reviewers, and only splitting the rule by interpreter resolves it.

Not in your list

Eight more, six against the skills tree. Reported as the reviewer stated them, with the same caveat you used, except where noted.

• add-host-tool/SKILL.md:7 - directs edits to spec/host-tools.json, the fleet declaration, where a repository's own registry is the root host-tools.json. I would weight this one: LanguageTags added a root host-tools.json in configure.sh model inference on carries; harden ruleset snippets #334, and the two files have different schemas, so following the skill would have a repository editing the hub's declaration.
• drive-pr/SKILL.md:96 - the remote branch delete after an ls-remote check is not a compare-and-swap, so the tip can move between the two. Same shape as your merge-and-release/SKILL.md:207, different file.
• branch-protection-and-promotion.md:24 - the promotion PR-head contract assumes develop is the head, but the EOL-conflict recovery path opens promote/develop-to-main instead, and selection, merge, and cleanup all key off the head. Also verified first-hand: this promotion has exactly that head, for exactly that reason.
• skill-lifecycle/SKILL.md:49 - the moved-content rule at line 46 makes the skill the owner, while line 49 says the doc wins in both shapes, so following line 49 can overwrite the authoritative skill with a stale summary.
• upstream-contribution-workflow/SKILL.md:51 - a bare git push --force-with-lease resolves through push configuration rather than naming a refspec, so it does not guarantee only the presentation branch is rewritten. Distinct from the line 60 finding you declined.
• repo-worktree/SKILL.md:177 and git-commit-conventions/references/history-rewrite.md recurred here too, matching your entries.

Two fell outside the skills tree, on AUDIT.md and GOVERNANCE.md, so I filed those separately rather than widening this issue.

On the shape of this

Your closing note said a downstream carry is the first time this tree is read by a reviewer with no stake in it. Worth adding that the second carry is not redundant: this run surfaced six findings yours did not, on a tree that had not changed in between, because a different reviewer read it against a different repository. Both times the findings arrive on a pull request that cannot act on them.

[ptr727]

Rolled up per #1315 into the unit tracker(s) #1338, #1339, #1340, #1341, #1344, #1345, #1347, #1348, #1350, #1351, #1353, #1354, #1355: every finding this issue enumerates appears there as a numbered line naming this issue. Closing as a duplicate of those trackers so no finding is lost and this bundle stops being a unit of work.