Answer Four Promotion-Review Findings Against Content Already on Develop - #1168
Merged
Conversation
The refusal table said no count of its rows is kept here so a row cannot go stale against it, then referred to two rows by position. An inserted or reordered row breaks both, which is the failure that sentence exists to prevent. Both now name the row by its own wording. OPERATIONS.md explained why CI carries !cancelled() on report --check and left the local block beside it reading as though it behaved the same. It runs under set -Eeuo pipefail, so a failing check stops it and every gate below never runs. The symlink guard's own test created a symlink unconditionally, which Windows refuses without the privilege or Developer Mode, so the case would have failed with an environment error rather than reporting the guard it tests. It skips with the reason instead, which is the execution-boundary distinction rather than a verdict. The unknown-marker floor covered headings and metadata labels while the positive case beside it covered all three vetted lists, so turning off summary vetting would have left both green. Verified by vetting an unknown summary and watching exactly the new arm fail.
PR Summary by QodoResolve promotion-review documentation and test findings
AI Description
Diagram
High-Level Assessment
Files changed (8) |
Code Review by Qodo🐞 Bugs (0)📘 Rule violations (0)📎 Requirement gaps (0)Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The updates are consistent with the stated findings, improve test robustness across platforms, and do not introduce behavioral or contract regressions.
Pull request overview
Addresses four promotion-review findings that were raised on the develop -> main promotion PR (#1163) against content already merged into develop, by tightening marker-vetting coverage, clarifying canonical-review behavior in local vs CI runs, and making a Windows-specific symlink test report an execution boundary instead of a false negative.
Changes:
- Extend
pr_reviewmarker-shape floor coverage by adding the missing<summary>/summary-arm case to the “unknown marker” test. - Make the canonical-review symlink test skip (with an execution-boundary reason) on hosts that cannot create symlinks (notably Windows without privilege/Developer Mode).
- Update local-strict-review refusal-table wording to avoid referencing rows by position; refresh derived skill distributions and the canonical-review ledger entry.
File summaries
| File | Description |
|---|---|
| scripts/tests/test_pr_review.py | Adds the missing summary unknown-marker case so summary vetting can’t silently regress. |
| scripts/tests/test_canonical_review.py | Skips the symlink-guard test with an execution-boundary explanation when symlink creation is not permitted. |
| reports/canonical-review.json | Updates the recorded digest/stamp for the changed canonical unit. |
| OPERATIONS.md | Clarifies why the local gate sequence can’t mirror CI’s !cancelled() behavior for report --check. |
| .github/skills/local-strict-review/SKILL.md | Removes row-number-based references in the refusal table by pointing to row wording/relative placement instead. |
| .claude-plugin/fleet-skills/skills/local-strict-review/SKILL.md | Propagates the same refusal-table wording fix into the plugin distribution. |
| .claude-plugin/fleet-skills/.source-digest | Updates the distribution source digest to reflect the regenerated skill content. |
| .agents/skills/local-strict-review/SKILL.md | Updates the canonical skill source with the same refusal-table wording fix. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Answers four review findings raised on the
develop -> mainpromotion #1163 against content already merged todevelop. The promotion's diff cannot carry a fix, so they land here.local-strict-review's refusal table says "no count of the rows is kept here to go stale against the table", then refers to two rows by positionOPERATIONS.mdexplains why CI carries!cancelled()onreport --checkand leaves the local block beside it reading as though it behaved the sameset -Eeuo pipefail, so a failingcheckstops it and the gates below never runtest_canonical_review.pycreates a symlink unconditionally, which Windows refuses without the privilege or Developer Modetest_pr_review.py's unknown-marker floor covers headings and metadata labels, while the positive case beside it covers all three vetted listssummaryarm, so turning off summary vetting can no longer leave both greenVerification
The two test changes were checked by mutation rather than by reading. Vetting an unknown summary in
pr_review.VETTED_SUMMARIESmakes exactly one test fail, the new arm, and nothing else. The symlink guard still fails as before when a symlink can be created, which is the case on this host.prose_lint,ruff check,ruff format --check,spec/validate.py,build_dist --checkand the fullscripts/testssuite all pass, and the one canonical unit this moves is recorded.Not fixed here
One finding on #1163 is declined rather than carried:
docs/reusable-workflows.mdnamingNUGET_USERNAME. That is the NuGet mechanism's credential, declared fleet-wide inspec/secrets.jsonundernuget-oidcand identical for every NuGet adopter, so it is the mechanism the hub guide is supposed to describe rather than an adopter's own specifics. The reasoning is in that thread.