Skip to content

fix(hipdnn): close hipdnn-deep-review silent-drop and drafting gaps - #14566

Merged
BrianHarrisonAMD merged 4 commits into
developfrom
users/bharriso/hipdnn-deep-review-fixes
Oct 28, 2026
Merged

BrianHarrisonAMD merged 4 commits into
developfrom
users/bharriso/hipdnn-deep-review-fixes

Conversation

@BrianHarrisonAMD

@BrianHarrisonAMD BrianHarrisonAMD commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes gaps in the hipdnn-deep-review skill that a full review round of #14457 exposed. In that round, findings and coverage rows were dropped without any error, follow-up finding IDs were numbered wrongly, and the drafted comments needed a hand rewrite. The scripts now fail or explain in each of those cases.

JIRA ID : ALMIOPEN-2945

Risk Assessment

Risk 1. Changes are limited to the skill's scripts, docs and tests under projects/hipdnn/tools/ai/. No product code, build or packaging change.

Testing Summary

  • Skill tooling tests: 22 new or extended regression cases, each failing before its fix and passing here, plus 3 guard cases for inputs that must keep passing.
  • Skill validation: validate-skills.py validates all 16 skills.
  • End-to-end smoke run of seed, ids, cluster, coverage, draft and orient-gate on a scratch repository.

Testing Checklist

  • Skill tooling tests - python3 -m pytest projects/hipdnn/tools/ai/tests - Status: Passed (334 passed, 7 skipped)
  • Skill validation - python3 projects/hipdnn/tools/ai/validate-skills.py - Status: Passed (16 skills)
  • PR CI - GitHub PR checks - Status: Pending

Technical Changes

  • Findings no longer vanish. ledger.py ids fails when a report holds a finding block it cannot read: an ID: line in bulleted, numbered, bold, quoted, heading or table form, a partly fenced block, or a block with no ID: line. It also fails when a Findings section cites a provisional ID that no report of the round parsed, which catches findings written as table rows, including a merged agent's other lanes. Lane notes after a ## heading or --- no longer leak into the last field of a finding.
  • Follow-up IDs. Provisional IDs must be <LANE>-p<n>. Follow-up reports had written RUN-f1-p1, which became the final ID RUN-f-1.
  • Coverage. ledger.py coverage explains why an owner lane's row missed its file (short path, grouped or glob row) instead of leaving the file pending with no reason.
  • Clustering. New ledger.py cluster <kept> <duplicate>... replaces hand edits of ID-MAP.md. It keeps chains of superseded findings valid.
  • Orient gate. New partition.py orient-gate replaces the shell pipeline. It accepts a cited file the change adds.
  • Draft comments. Each suggestion or future-work item becomes its own comment, grouped by tier and kind.
  • Brief changes. A new Scope: input records notes such as deprecated areas. Verifiers no longer treat a mutant in code the test binary does not compile as proof of a test gap.

…ALMIOPEN-2945)

Fixes found running the skill on #14457:

- ledger.py ids fails on marked-up ID: lines (bullet, bold, quote, inline
  code), not only on partial fences, and on provisional IDs that are not
  <LANE>-p<n> (follow-ups wrote RUN-f1-p1, which mapped to RUN-f-1).
- A markdown heading or --- ends a finding field, so lane notes no longer
  bleed into Fix and from there into COMMENTS.md.
- ledger.py coverage prints why an owner lane's row missed (short path,
  grouped or globbed row) instead of leaving files pending silently.
- ledger.py cluster supersedes duplicates, repoints chains and refuses a
  superseded keep; it replaces hand edits of ID-MAP.md.
- partition.py orient-gate replaces the grep pipeline and accepts a cited
  file the change adds.
- comments.py drafts one General item per SUGGESTION/FUTURE WORK finding.
- Docs: Scope: input for deprecated areas; follow-up ID numbering; verify
  test-gaps from the link graph when no test binary compiles the file.
@therock-pr-bot

therock-pr-bot Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

✅ All Checks Passed — Ready for Review

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🔎 pre-commit ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —
🤖 therock-pr-bot ✅ Pass —

🎉 All checks passed! This PR is ready for review.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

🎉 All checks passed! This PR is ready for review.

- ids fails on ID: lines in numbered, plus, heading, table, italic or spaced form, and on an own-lane <LANE>-p<n> cited in Findings that no report of the round parsed. The ID-line pattern is linear time.
- A single-# line (shell comment) no longer ends a field; ## headings and --- still do. findings.md says to fence commands and code in fields.
- orient-gate strips a leading ./ from cited paths.
- RUNBOOK and findings.md wording: one General item per lower-tier finding; VER<batch> provisional prefix.
…OPEN-2945)

- ids also counts Tier: lines, so a finding block with no ID: line fails instead of vanishing or overwriting the previous block.
- The cited-ID check covers every report prefix, not only the report's own lane, so a merged agent's other-lane findings and table-row findings are caught. A cited ID passes when any report of the round parses it.
- The report-lane prefix list lives in partition.py and comments.py reuses it.
@BrianHarrisonAMD
BrianHarrisonAMD marked this pull request as ready for review October 28, 2026 18:20
@BrianHarrisonAMD
BrianHarrisonAMD requested a review from a team as a code owner October 28, 2026 18:20

@adickin-amd adickin-amd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran this against my latest PR and it worked as expected

@BrianHarrisonAMD
BrianHarrisonAMD merged commit 882dba4 into develop Oct 28, 2026
33 of 34 checks passed
@BrianHarrisonAMD
BrianHarrisonAMD deleted the users/bharriso/hipdnn-deep-review-fixes branch October 28, 2026 18:56
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants