[davidd-amd]

#11006

JIRA ID

N/A

Motivation

The hipBLASLt PR-quality overlay presently constitutes a rules-tightening layer superimposed on the library-agnostic base contract, but its remit has historically been confined to the structural and procedural dimensions of merge-readiness adjudication — change-class binding, MUST-set enforcement, stale-base coupling detection, and the validator-versus-validated-data collision surface — while leaving the prose-artifact production pathway effectively unspecified and therefore subject to considerable authorial variance. In the absence of a codified stylistic contract governing the natural-language surface of a pull request, the descriptive artifacts emitted during the author and review phases exhibit non-deterministic quality characteristics that are not presently observable to, or gateable by, any deterministic checkpoint in the pre-merge pipeline, which in turn attenuates the reviewer's capacity to perform a first-pass triage without incurring the cost of a full diff traversal. This change is intended to close that specification gap at the overlay tier.

Technical Details

This PR introduces a prose-and-style context artifact, co-located with the overlay's SKILL manifest, and wires it into the extant supplement enumeration via two additive supplement clauses that are strictly monotonic with respect to the base MUST-set — i.e., they tighten and extend but under no circumstance relax an inherited invariant, consistent with the overlay's established non-relaxation semantics. The first supplement establishes the referential binding between the manifest and the style context, designating the latter as the normative authority for the reader-model calibration, the length-budget envelope, the per-sentence admissibility gate, the proscribed-lexeme enumeration, and the evidentiary-scope discipline. The second supplement promotes the guidance from a passively-referenced corpus to an actionable procedural obligation across both the author-assist and review-assist actuation modes: in the author path, the drafted description is subjected to the sentence-level admissibility gate and the terminal acceptance predicate prior to surfacing; in the review path, the incumbent description is differentially evaluated against the same gate, with any admissibility violation precipitatinndation annotated with the specific failingpredicates. The recommendation pathway is constrained to an advisory posture per the base operating principle governing unattended external mutation, and preserves the repository-manhe tracker-linkage line. No modification is made tothe base contract, to any sibling overlay, or to any production or device-facing code path; the change is confined to the
documentation and skill-specification stratum of the.

Test Plan

Validation is effected by exercising the overlay's modes against a representative corpus of candidatedescriptions and confirming that the admissibility gate discriminates conformant from non-conformant artifacts and that the
rewrite-recommendation pathway emits a well-formed,r artifact under the advisory constraint.

Test Result

The pre-commit affected-tests hook was exercised antion. Broader automated verification is understood to be nominally green. Coverage of the affected specification surface is considered adequate for the documentation stratum.

Submission Checklist

• Look over the contributing guidelines.

Risk level

Low, inasmuch as the change is confined to the documentation and specification stratum and effects no alteration to any executable code path or device-facing behavior.

This is what this skill would recommend

JIRA ID : ABC-123456

Motivation

The hipBLASLt PR-quality skill is a set of rules that tighten a shared base skill for reviewing pull requests. Until now it covered only structure and process (which files changed, which checks must pass), and said nothing about how to write the PR description itself. As a result, description quality varied by author. This PR adds a written style guide to close that gap.

Technical Details

• Adds prose-style-context.md next to the skill's SKILL.md. It defines the target reader, a word limit, a per-sentence checklist, a list of words to avoid, and rules for backing claims with evidence.
• Adds two clauses to SKILL.md: one points the skill at the new file; one requires both the author and review modes to run a description through the checklist and produce a concrete rewrite. Rewrites stay advisory and are never posted without human approval.
• No base skill, sibling skill, or product/device code changes. Two files: SKILL.md (+20), prose-style-context.md (+306, new).

Test Plan

Run the skill's author and review modes against she checklist flags non-conforming text and the
rewrite output is well-formed.

Test Result

Pre-commit hook ran locally. Automated GitHub checks are pending. Docs-only; no coverage to report.

Submission Checklist

• Look over the contributing guidelines.

Risk level

Low (1/5): documentation and skill files only; no executable or device-facing code changes.

[therock-pr-bot]

✅ All Checks Passed — Ready for Review

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	PR does not contain code files — Unit Test auto-passed
🔎 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]

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

[tony-davis]

David, I think this is solving a real problem. Our PR descriptions have been getting out of hand, and I have definitely noticed that in mine. I tried the guide against a few of my current PRs as a dogfood exercise, and it really does help: clearer opening, less assumed context, and a description that stands on its own better.

I have a few adjustments to suggest, but the core goal here is important and I am glad you wrote it down.

[tony-davis]

David, I wanted to show what this looks like against a real PR once the reader-model / length / cadence tweaks above are in place. I used #10752.

Your direction helped a lot: the rewritten version explains TheRock CI before diving into workflow details, and it is much easier for someone outside the component to follow. My first pass under the current hard word limit went too far and cut the live run evidence. With the proposed changes, I think we keep the accessibility you are aiming for without losing the verification detail.

Example rewritten description

Summary

TheRock CI builds and tests ROCm library changes on Linux and Windows. It currently launches the full matrix after every push to a pull request, even when the pull request is still a draft. This change suppresses those work-in-progress runs, preserving shared runner capacity until the author marks the pull request ready for review.

The setup job still runs for draft pull requests, but therock_configure_ci.py returns empty Linux and Windows project lists before examining the changed paths. The existing matrix conditions then skip the build and test jobs while allowing the required TheRock CI Summary check to succeed. Adding the ready_for_review event ensures that changing the pull request from draft to ready starts the full matrix without requiring another push.

Risk Assessment

Risk 2/5. This changes when CI runs, not what it builds or tests. The setup job is never skipped, which avoids depending on GitHub Actions' job-skip propagation for a required check.

Related

• Fixes TheRock CI runs full Linux/Windows builds on draft PRs #10751

Device / Architecture Coverage

No product or device code changes. The live verification covers the workflow behavior directly; no architecture-specific testing is needed.

Testing Summary

• python -m pytest .github/scripts/tests/therock_configure_ci_test.py: 36 passed. Three new tests cover draft pull requests, the ready-for-review state, and non-pull-request events.
• pre-commit passed on the workflow, script, and test files.
• Draft run 31750101133: Setup logged is_draft: True, returned empty Linux and Windows project lists, and completed successfully. The Linux and Windows matrix jobs were skipped, while the required TheRock CI Summary check passed.
• Ready-for-review run 31750369870: marking the pull request ready, without another push, immediately launched the full Linux and Windows matrix. The existing concurrency group also cancelled the stale draft-state run.
• The ready-for-review matrix later reported one TensileLite test failure, so its final summary was red. That does not change the observed trigger behavior, but the failure remains visible rather than being described as a successful CI run.

Testing Checklist

• Unit tests: 36 passed
• Pre-commit hooks: passed
• Draft-state live verification: passed
• Ready-for-review trigger verification: passed
• Full PR matrix: one TensileLite test job failed

Flags / Guardrails

None. Draft pull requests cannot merge, and marking one ready automatically restores normal CI behavior.

Technical Changes

• Add ready_for_review to the pull-request event types.
• Pass github.event.pull_request.draft to therock_configure_ci.py.
• Return an empty project list for draft pull requests before calculating changed paths.
• Add unit tests for draft, ready, and non-pull-request events.

[tony-davis]

Second example from #10740. Same idea: the style guide's push toward explaining the job before naming files is doing good work here.

What I liked most was that the rewrite makes the platform-argument trap obvious without assuming the reader already knows how the reusable workflow is wired. This feels like the best version of what you are aiming for: clearer onboarding, without flattening the technical story.

Example rewritten description

Summary

The configure_test_matrix job examines repository configuration and produces a JSON list of tests for later jobs. It runs Python only and does not use a GPU, but it currently occupies the same scarce self-hosted GPU runner requested by the tests themselves.

This change moves that job to the GitHub-hosted ubuntu-24.04 runner. The later test_components jobs continue to use their existing GPU runners, so device selection and test execution are unchanged.

Moving the job exposed one important dependency on the original runner: fetch_test_configurations.py inferred the target platform from the runner operating system. A Windows caller running the script on Ubuntu would therefore receive a Linux test matrix. The workflow now passes its platform input explicitly, allowing both Linux and Windows matrices to be calculated on Ubuntu.

Risk Assessment

Risk 2/5. The reusable workflow runs on many pull requests, but the change is confined to the host used for one matrix-calculation job. Workflow inputs, outputs, GPU selection, and the test_components runners are unchanged.

Related

• Fixes CI: configure_test_matrix job runs on scarce GPU runner instead of a CPU runner #10748

Device / Architecture Coverage

No device code changes. The relevant coverage is that Linux and Windows callers both receive the correct test matrix and that their downstream jobs start normally.

Testing Summary

The PR itself exercises the changed workflow because .github/workflows/therock-test-packages.yml is treated as CI-relevant. Run 31840814981 completed Setup successfully and launched Linux and Windows build jobs; the full matrix was still running when this description was prepared.

Automated review identified the missing platform argument in the first version. The current version passes --platform=${{ inputs.platform }}, preventing a Windows caller from silently receiving a Linux matrix. Pre-commit also passed on the workflow file.

Testing Checklist

• Pre-commit hooks: passed
• Workflow setup: passed
• Linux and Windows build matrices launched
• Full GitHub test matrix: in progress

Flags / Guardrails

None.

Adjacent Tests Considered

The test_components jobs retain their existing runner-selection logic. The Windows path uses the same workflow but now passes --platform explicitly, so its result no longer depends on the operating system of the matrix-calculation runner.

Risk Acceptance / Waivers

W-BUILD: build and CI configuration only; product behavior is unchanged.

Technical Changes

• Run configure_test_matrix on ubuntu-24.04.
• Pass the workflow's platform input to fetch_test_configurations.py.
• Remove the Windows pre-job cleanup and its now-unused checkout.

[tony-davis]

Third example from #10596. This one is a good stress test for the guide, because the before-and-after reproduction is the heart of the review.

I think your evidence rules are exactly right here: keep the inputs, exit codes, and floor values. The rewrite below tries to honor that while still doing the cleaner "what does this tool do, what was unsafe, what changed" opening that your Motivation guidance asks for. If we soften the hard length limit, this is the kind of description I hope the skill keeps rather than compresses away.

Example rewritten description

JIRA ID : #10591

Motivation

coverage_ratchet.py stores a minimum coverage percentage, called a floor, for each TensileLite Python file. Its check command reports files that fall below their floors, while update is intended to record improvements.

Before this change, update replaced every committed floor with the values in the current coverage.json. Updating one file could therefore lower unrelated floors without warning. The check's failure message recommended that same command, making the documented recovery procedure capable of weakening the gate it was meant to maintain.

Technical Details

ratchet_floors() now compares the current report with the committed baseline. It raises a floor when coverage increases, records the current value for a new file, and preserves an existing floor when coverage falls. Values are rounded to the baseline's two-decimal precision before comparison.

A developer may still approve a reduction with the repeatable --allow-lower PATH option. Only the named files may move down. If any other floor would fall, update exits with status 1, writes nothing, and reports the affected file, its floor, its current value, and the difference.

The remediation message now prints a separate --allow-lower option for each affected file. It no longer recommends a bare command that can reset unrelated entries. The README documents both the refusal and the explicit opt-in.

No automated workflow invokes update; CI uses check. Product code, generated kernels, and device behavior are unchanged.

Test Plan

Unit tests cover increasing and preserving existing floors, adding a new file, explicitly lowering a named file, rounding below stored precision, sorting refused reductions, and generating remediation commands.

The regression case is test_lowering_one_file_does_not_lower_another: one file is explicitly allowed to fall while a second file also measures below its floor. The second floor must remain unchanged.

Command-level tests verify that a refusal writes nothing and exits 1, repeated --allow-lower options parse correctly, only named files move, and an unknown path produces a warning.

Test Result

The defect was reproduced with identical inputs before and after the change. The committed floors were named.py=90.0 and bystander.py=88.0; the report contained named.py=70.0 and bystander.py=60.0.

Before the change, the command exited 0 and silently lowered bystander.py from 88.0 to 60.0. After the change, it exits 1, writes nothing, and leaves bystander.py at 88.0.

python -m pytest Tensile/Tests/unit/characterization/tools/test_coverage_ratchet.py -q passed all 36 tests, including 12 new tests. The local Windows run used --noconftest because the suite's shared configuration imports rocisa, which this machine cannot build. CI runs the file normally in the unit-test lane.

The recorded TensileLite coverage / Linux run is red on the stale SubtileGREmit.py floor described by #10591. That run predates the sibling baseline correction in #10595; the 36 unit tests are the direct evidence for this change.

Submission Checklist

• Look over the contributing guidelines.

Risk level

Low (2/5). This affects one CI maintenance command and makes its default behavior stricter. A previously silent reduction now becomes a refusal that identifies the affected files and the explicit command required to approve them.

[tony-davis]

I think this will help a lot. Thanks!