Skip to content

fix(guardrails): skill-reference-verify reports an untouched reference under replace_all: true #2129

Description

@kyle-sexton

Filed by AI while independently verifying PR #2100's discharge of PRRT_kwDOTCGFQM6TxHZU (#1319).
Recorded as its own item rather than left buried in a thread that is otherwise correctly resolved.

The residual

plugins/guardrails/hooks/skill-reference-verify.sh reports an untouched, pre-existing
unresolved reference when an Edit carries replace_all: true.

Fixture: a file containing /alpha:ghost and a pre-existing /alpha:ghost-old, edited with
new_string: "ghost" and replace_all: true.

revision result
527dcd85^ (pre-#1466) reports both refs
1cbb4bc9^ (pre-#2100) reports both refs
origin/main reports both refs

Unchanged across all three, so nothing regressed — it has simply always been there.

Why it was not held against #1319's thread

That thread's scenario is inserting ghost into one command. replace_all: true edits every
occurrence by definition, so it is a different call shape, and replace_all is absent or false on
an ordinary Edit. The thread's own complaint is fully discharged for the shape it describes —
verified with a same-line discriminator that separates origin/main from both earlier revisions.

Why it may not be fixable as stated

It looks payload-inherent. After a replace_all edit, nothing in the payload distinguishes a
ghost this call wrote from the ghost inside a pre-existing ghost-old — the hook is
reconstructing intent from a post-edit file plus a replacement string, and that information is
simply gone. The hook already says as much in its own "What this does NOT claim" block.

So the plausible dispositions are:

  • Accept and document more prominently. The current note is inside the hook; a consumer hitting
    this sees an advisory naming a reference they did not touch, with no pointer to why.
  • Suppress under replace_all. Narrow the reconstruction path when replace_all is true,
    trading a false positive for a missed real finding. Needs a judgement about which direction an
    advisory guard should fail in — the hook's existing reasoning prefers the quiet direction
    elsewhere.
  • Close as won't-fix with the reasoning recorded, so it is not rediscovered a third time.

Acceptance, if it is fixed

  • The replace_all: true fixture above stops reporting /alpha:ghost-old while still reporting
    /alpha:ghost.
  • A control proves the case discriminates: it must behave differently before and after. A fixture
    answering the same on both trees proves nothing — that trap has produced six separate instances on
    this sweep, including tests that passed while never executing the branch they claimed to cover.
  • A liveness proof that the reconstruction path was actually reached, not merely that the hook
    exited 0. The technique used on the parent thread works: run the identical payload against a
    truncated, empty target — if the hook is silent there, every finding in the real fixture
    demonstrably came from reading the file rather than from the payload.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions