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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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