What happened
PR #197 introduced two classes of dead references that the automated review agent missed across 4 review runs:
annotations.yaml referenced a line_accuracy judge not defined in eval/review/eval.yaml.challenger.md added an original_line output field not present in the strip-list (SKILL.md step 6d.4) or review-result.schema.json (additionalProperties: false), meaning it would cause runtime schema validation failures.
The correctness sub-agent at skills/pr-review/sub-agents/correctness.md has two related checklist items — "Consumer completeness" (new enum value → verify consumers handle it) and "Removal / rename staleness" (removed identifier → grep for remaining references) — but lacks the inverse pattern: when a diff introduces a reference to an identifier, verify the referent exists.
What could go better
The correctness sub-agent's existing guidance covers "new producer, check consumers" and "removed producer, check for stale consumers" but not "new consumer, check that the producer exists." This is the exact pattern that caused both dead-reference bugs in this PR:
annotations.yaml consumed a line_accuracy judge that was never produced (never defined).challenger.md produced an original_line field that no downstream consumer (strip-list, schema) was prepared to handle — and adding it to the output without updating the schema is effectively referencing a schema field that doesn't exist.
Confidence is moderate. I verified that correctness.md has no guidance covering this pattern (lines 40-53 cover the two existing patterns but not the inverse). However, prompt-level changes have inherently uncertain impact — the sub-agent may or may not apply new guidance consistently. The CI validation in the companion proposal provides a mechanical backstop for the eval-specific case.
Proposed change
Add a "Reference validity" checklist item to skills/pr-review/sub-agents/correctness.md, positioned after the existing "Removal / rename staleness" block (after line 53). The new block should follow the same pattern:
**Reference validity:** When the diff introduces a reference to an
identifier not defined within the diff itself (e.g., a judge name in
an eval annotation, a field name expected by a schema, a label name
in a workflow dispatch, a config key consumed by a script), grep the
repository for the identifier's definition. A reference with no
definition is a Medium-severity finding: "undefined reference to
`<identifier>` in `<file>:<line>` — no definition found in the
repository."
This completes the triad: consumer completeness (forward), removal staleness (backward), reference validity (inverse).
Validation criteria
On the next 5 PRs to this repo that introduce new YAML/config references (judge names, schema fields, label names, action enum values), the correctness sub-agent should either (a) verify the reference target exists and note it, or (b) flag undefined references as findings. Track by searching review bot comments for mentions of 'undefined reference' or 'reference validity.' If the sub-agent does not apply the guidance after 5 opportunities, escalate to a structural fix (e.g., a dedicated sub-agent or CI check for cross-file reference consistency).
Generated by retro agent from #197
What happened
PR #197 introduced two classes of dead references that the automated review agent missed across 4 review runs:
annotations.yamlreferenced aline_accuracyjudge not defined ineval/review/eval.yaml.challenger.mdadded anoriginal_lineoutput field not present in the strip-list (SKILL.mdstep 6d.4) orreview-result.schema.json(additionalProperties: false), meaning it would cause runtime schema validation failures.The correctness sub-agent at
skills/pr-review/sub-agents/correctness.mdhas two related checklist items — "Consumer completeness" (new enum value → verify consumers handle it) and "Removal / rename staleness" (removed identifier → grep for remaining references) — but lacks the inverse pattern: when a diff introduces a reference to an identifier, verify the referent exists.What could go better
The correctness sub-agent's existing guidance covers "new producer, check consumers" and "removed producer, check for stale consumers" but not "new consumer, check that the producer exists." This is the exact pattern that caused both dead-reference bugs in this PR:
annotations.yamlconsumed aline_accuracyjudge that was never produced (never defined).challenger.mdproduced anoriginal_linefield that no downstream consumer (strip-list, schema) was prepared to handle — and adding it to the output without updating the schema is effectively referencing a schema field that doesn't exist.Confidence is moderate. I verified that
correctness.mdhas no guidance covering this pattern (lines 40-53 cover the two existing patterns but not the inverse). However, prompt-level changes have inherently uncertain impact — the sub-agent may or may not apply new guidance consistently. The CI validation in the companion proposal provides a mechanical backstop for the eval-specific case.Proposed change
Add a "Reference validity" checklist item to
skills/pr-review/sub-agents/correctness.md, positioned after the existing "Removal / rename staleness" block (after line 53). The new block should follow the same pattern:This completes the triad: consumer completeness (forward), removal staleness (backward), reference validity (inverse).
Validation criteria
On the next 5 PRs to this repo that introduce new YAML/config references (judge names, schema fields, label names, action enum values), the correctness sub-agent should either (a) verify the reference target exists and note it, or (b) flag undefined references as findings. Track by searching review bot comments for mentions of 'undefined reference' or 'reference validity.' If the sub-agent does not apply the guidance after 5 opportunities, escalate to a structural fix (e.g., a dedicated sub-agent or CI check for cross-file reference consistency).
Generated by retro agent from #197