Skip to content

Four Defects in the local-strict-review Skill's "What It Does" Section #1150

Description

@ptr727

Four defects in the local-strict-review Skill's "What It Does" section, found by a full-content pass over that unit during #1148 and deliberately not fixed there. That PR changed one clause in this section, and its own findings were fixed in it. These four predate it and are about how the Skill briefs a reviewing subagent, which is a different subject from the PR that surfaced them.

1. Excluding "Publish Every Finding" wholesale drops two rules that are not publication mechanics

code-review's separate "Publish Every Finding" section does not apply here: this skill has no PR to post a comment on and no coverage marker to close a review with, so its own report contract below replaces that section rather than extending it.

That section also carries "Never suppress or hide a finding because confidence is low" and "Do not report a clean review until every changed file has been read." Neither depends on having a pull request to post to, and the replacing contract, the Return: line, carries neither. A subagent briefed exactly as this section specifies is never told not to suppress a low-confidence finding, which is the failure mode pr-review-conduct treats as significant enough to count suppressed findings separately in scripts/pr_review.py.

2. code-review's "Establish the Contract" section is neither adopted nor excluded

The section names "Review the Change" as the criteria and explicitly excludes "Publish Every Finding", leaving the third section unmentioned, so a reader cannot tell whether it applies. Following the text literally, the pass never loads the language and documentation skills that section routes to, comment-and-doc-style, python-codestyle, shell-codestyle and workflow-ci-contract, and never gets its step 4, "Treat a missing executable on PATH as no evidence that its check is unavailable."

The omission is consequential in a specific way: findings from this pass concentrate in prose rather than code, so comment-and-doc-style is the skill most likely to matter and the one most reliably dropped.

3. Two absolutes about the same command contradict each other

That merge-base diff covers every commit already on the branch plus whatever is currently staged or unstaged

Two sentences later:

it reports nothing for content a commit carries that the working tree has since put back

Both are unqualified claims about the same git diff invocation. The second falsifies the first rather than qualifying it, and a reader has no way to tell which bound is the real one.

4. "the engine" is used before anything names it

and why the engine reads HEAD rather than this diff, its change set coming from the merge base against HEAD, the index and the working tree

The claim is accurate against local_review.fingerprints, but scripts/local_review.py is introduced only in a later section, and this repository now has two review engines. A reader holding only this unit cannot tell which is meant or check the claim.

Note on how these were found

A subagent read the unit whole, as a repository carrying it for the first time would, which is the pass #1138 exists to move into this repository. This is the second batch that pass produced against content #1148 did not introduce; the first is #1149.

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

    documentationImprovements or additions to documentationproseA defect in rule or procedure textskillsAgent skill

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions