Repository navigation
🐛 fix: unblock review backlog findings in untouched files - #14486
thatguywob wants to merge 4 commits into
Conversation
Cache a bounded changed-files lookup per filing pass and park only findings tied to the reviewed PR, retaining conservative blocking on lookup failure and PR provenance for all new items. Co-authored-by: clubanderson <407614+clubanderson@users.noreply.github.com> Hive-Run: hivecommons#14473 Hive-Plan: review-backlog-file-scope Hive-Spec: issue-14473#changed-files-routing Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@thatguywob is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this repository have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
|
@thatguywob this PR is back in your court ( Failing checks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Your relay session received this as a |
Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
Signed-off-by: thatguywob <thatguywob@users.noreply.github.com>
|
@thatguywob this PR is back in your court ( Failing checks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Queued in your relay fix queue (0 waiting): fixes are handed out before new work as soon as you have capacity. |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
There was a problem hiding this comment.
Correctness — reviewed at ad8fe8a.
Changed-file match is exact-string, so a touched file can be released as "untouched" (src/pkg/github/review_backlog_blocks.go:410-414). The probe keys files on GitHub's filename (always repo-root relative, no ./) and looks up strings.TrimSpace(file). The rest of this pipeline treats finding paths as needing normalization — normaliseFingerprintPath (review_backlog_routing.go:411-417) strips \, ./, / and cleans the path before fingerprinting (routing.go:206,401,495). A finding citing ./src/pkg/x.go or /src/pkg/x.go on a PR that modifies src/pkg/x.go therefore returns (false, true) → filed unblocked with help wanted, which is the #13833 regression this PR otherwise guards against (the base probe stays conservative because PathExistsAtRef on such a path errors or 404s, so missing only ever over-blocks). Before this PR every item was blocked, so the diff introduces the exposure.
--- a/src/pkg/github/review_backlog_blocks.go
+++ b/src/pkg/github/review_backlog_blocks.go
@@ func (c *Client) newReviewBacklogChangedFilesProbe(
for _, f := range changed {
- files[f.GetFilename()] = true
+ files[normaliseFingerprintPath(f.GetFilename())] = true
if old := f.GetPreviousFilename(); old != "" {
- files[old] = true
+ files[normaliseFingerprintPath(old)] = true
}
}
@@
- if files[strings.TrimSpace(file)] {
+ file = normaliseFingerprintPath(file)
+ if files[file] {
return true, true
}
- return false, complete && strings.TrimSpace(file) != ""
+ return false, complete && file != ""
}Worth adding a "./pkg/a.go" / "/pkg/a.go" case to TestReviewBacklogBlocksOnlyRelatedFiles (review_backlog_scope_test.go:20-26) expecting blocked: true.
Everything else checks out: ReconcileReviewBacklogBlocks skips records with BlockedOnPR set but no blocked state (review_backlog_blocks.go:170), so provenance-only records are inert; the 40-page budget × 100 covers the 3000-file cap; lookup failure and exhausted budget both yield complete=false → blocked.
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1
|
@thatguywob this PR is back in your court ( Review asks:
Reply here or push a new head to this branch (same sign-off); CI and the review swarm re-run automatically and the PR returns to the normal path. Your relay session received this as a |
|
@thatguywob the hive has adopted this PR ( |
newReviewBacklogChangedFilesProbe compared raw strings, so a finding citing ./pkg/a.go or /pkg/a.go on a PR that modifies pkg/a.go was reported as untouched and filed unblocked. Normalise both the GitHub filenames and the lookup with normaliseFingerprintPath, matching the rest of the pipeline. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
1 of 4 commits on |
There was a problem hiding this comment.
Read the diff at b8740e3 against v5 (663f43a) plus the consumer. Looks correct to me.
- The one thing I wanted to be sure of: unparked items still get
fpRecord.BlockedOnPR = req.Number(review_backlog.go:398) but noBlockState.ReconcileReviewBacklogBlocksgates onrec.BlockState != reviewBacklogBlockBlocked || rec.BlockedOnPR <= 0(review_backlog_blocks.go:170), so provenance-only records are skipped by the sweep, as the body says. - The
strings.Replacerewording for a failed lookup (review_backlog.go:337) matches the literal inblockedOnReviewedPRNote(review_backlog_blocks.go:67), so it is not a silent no-op;review_backlog_scope_test.go:78asserts the original wording is gone in that case. newReviewBacklogChangedFilesProbereports unknown (→ stays blocked) on anyListFileserror, on exhaustingreviewBacklogBlockLookups(40 pages) withoutNextPage == 0, and at the 3000-file cap, and indexesprevious_filenameso renames stay parked. Existing tests whose fake server has no/pulls/N/filesroute therefore 404 into the conservative branch rather than silently unparking, which is the right failure direction.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
@thatguywob the hive has adopted this PR ( |
|
stage 095-pre-merge-soak → 074-adopted-by-hive: required-check-pending:Sign-off survives the squash |
|
Closing: #14516 (the hive's adoption of this PR, with @thatguywob's commits) has merged. Thank you! |
Review backlog findings about untouched files were always parked on the reviewed PR, keeping unrelated work out of the queue until that PR settled.
Check the PR's changed files once per filing pass, with pagination and a bounded lookup budget. Findings in untouched files now retain normal labels and have no blocked-on body marker; changed files, renamed paths, and files missing on the base remain parked. The ledger retains source PR provenance without scheduling untouched findings for block reconciliation. Failed or incomplete file lookups conservatively retain blocking with wording that does not assert the file changed. Update the review follow-up explanation and add a changelog fragment.
Validation: added regression coverage for untouched/changed/renamed/base-missing files, lookup failure, lookup caching, ledger state, and pagination.
gofmt -lis clean. Tests are reserved for CI under AGENTS.md and were not run locally.Fixes #14473
— hive: backend=codex model=gpt-6.1-sol
🐝 Hive Agent:
contributor| SHA:d21833061