Skip to content

Several substantive findings in newly-packaged Skills, surfaced reviewing them in a downstream carry (Blog #97) #928

Description

@ptr727

Two third-party review bots (CodeRabbit, qodo-code-review) on ptr727/Blog's own PR review flow surfaced these while reviewing the .github/skills/ tree carried whole into Blog for the first time. None of these are Blog's to fix locally (verbatim-tree fidelity), so filing here per the "Reviewing Carried Fleet Content" convention. I verified each against the actual carried content before including it; skipped several stylistic nitpicks and two clearly-false-positive automated-security-scanner flags (a "malware/YARA match" and an "anti-refusal" flag on ordinary git-signing and shell-command governance prose in git-commit-conventions/SKILL.md and dotnet-codestyle/SKILL.md - neither file contains anything resembling what was flagged).

Real bugs

  1. comment-and-doc-style/references/line-endings.md: the byte-preserve directory example uses [<dir>/*], which only matches one path component (EditorConfig's * excludes /). Nested files under the directory keep inherited normalization instead of the intended byte-preserve treatment. Should be [<dir>/**].
  2. operational-vs-release-workflow/references/branch-protection-and-promotion.md (around line 38): the stage-comparison example diffs git show :2:f / :3:f against a literal file named f rather than the <file> the preceding command selects. Should read :2:<file> / :3:<file>.
  3. resync-a-repo/SKILL.md (~L69-71) and standup-a-repo/SKILL.md (~L76-79): both give repo-config/configure.sh check <owner>/<repo> release|operational as a literal shell command. | is a pipe in a shell, so as written this does not pass a valid mode argument - needs a concrete release or operational in the example, with a note that the reader picks one.
  4. dotnet-codestyle/references/conventions.md (~L105-125): the GetQuoteOfTheDayAsync example declares async Task<string> with an empty body - doesn't compile. Needs a representative return/await.

Internal inconsistencies

  1. resync-a-repo/SKILL.md (~L78-84) and skill-lifecycle/SKILL.md (~L31-33): both instruct committing/pushing during their normal procedure (an audit report, a generated skill tree) unconditionally, which conflicts with git-commit-conventions/SKILL.md's own "default to staging, commit only when explicitly authorized" rule. Worth stating explicitly that the commit step in each needs the user's go-ahead like any other.
  2. dotnet-codestyle/references/project-config.md: presented as the full required-properties list but omits GenerateDocumentationFile/Nullable, both required elsewhere in dotnet-codestyle/SKILL.md. Either complete the list or state it's partial.
  3. comment-and-doc-style/SKILL.md (~L227-244): the PR-title examples mix title case and sentence case where the rule text requires title case throughout.
  4. python-codestyle/SKILL.md (~L73-94, ~L141-145) and python-codestyle/references/testing.md (~L3-14): the shared testing guidance requires uv run pytest/fixtures/uv sync unconditionally, but the lint-only profile (stdlib-only, uvx-run, no lockfile) uses unittest instead. Needs qualifying by profile, or the lint-only path added to testing.md.

Technical accuracy

  1. dotnet-codestyle/references/conventions.md (~L16-20): "types loosely match" isn't a real C# collection-expression rule. Collection expressions are target-typed with no type of their own; each element needs an implicit conversion to the target element type. Worth rephrasing to the actual rule.
  2. git-commit-conventions/SKILL.md (~L104-112): the signing probe accepts sig=U (good signature, unknown validity) as passing, but U only proves Git accepted the signature locally - it doesn't prove GitHub associates the signing key with the committing account. A commit could pass this probe and still be rejected by a branch requiring GitHub-verified signatures. Worth a note on what U does and doesn't prove, even if the probe's practical conclusion (proceed) stays the same.

Documentation precision (lower confidence, worth a look)

  1. workflow-ci-contract/SKILL.md (~L19), workflow-ci-contract/references/d-guarantees.md (~L52), and WORKFLOW.md (~L73, ~L192, ~L288): these describe a repository adding/removing build-<target> jobs and github-release needs: entries directly, which reads as caller-editable when the reusable-task contract makes that job graph hub-owned (a caller only selects among existing targets via enable_*). Worth a look at whether the wording should distinguish "hub adds a target" from "caller enables a target."

CODESTYLE.md/GOVERNANCE.md drift (separate, not a Skill content issue)

  1. Not a Skill finding, but surfaced by the same review: CODESTYLE.md's Shell section still said set -euo pipefail while GOVERNANCE.md's "Workflow YAML Conventions" (re-vendored in the same PR) requires set -Eeuo pipefail. Fixed locally in Blog's CODESTYLE.md since that's Blog's own prose, not carried - flagging here only because every repo with a similar Shell section carried before this GOVERNANCE.md change likely has the same stale -euo wording and won't self-correct without a resync touching that file.

Not filing separately for the two clearly-false-positive scanner flags (malware/YARA on git-commit-conventions/SKILL.md, anti-refusal on dotnet-codestyle/SKILL.md's bare-link style note) since there's nothing there to fix - noting only so it isn't rediscovered as a mystery later.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions