Skip to content

orb(content-lane): canonicalize issue-body path tokens in `checkContentLaneDeliverable - #9930

Closed
phamngocquy wants to merge 1 commit into
JSONbored:mainfrom
phamngocquy:miner/issue-9667
Closed

orb(content-lane): canonicalize issue-body path tokens in `checkContentLaneDeliverable#9930
phamngocquy wants to merge 1 commit into
JSONbored:mainfrom
phamngocquy:miner/issue-9667

Conversation

@phamngocquy

Copy link
Copy Markdown
Contributor

Summary

checkContentLaneDeliverable (src/review/content-lane/registry-logic.ts:919-936) is the
deterministic "did this PR actually deliver the content file its linked issue names?" check — the guard
against a test-only PR closing a content issue via a bare Closes #N. It compares two things against
the same predicate:

const matchesSpec = (candidate: string): boolean =>
  spec.entryFilePattern.test(candidate) || (spec.providerFilePattern?.test(candidate) ?? false);
const mentionedPath = extractPathTokens(issueText).find(matchesSpec);              // :929  RAW token

const delivered = changedFiles.some((file) => matchesSpec(canonicalize(file)));    // :932  canonicalized

entryFilePattern/providerFilePattern are compiled by globToRegExp
(packages/loopover-engine/src/signals/change-guardrail.ts:82-103), which canonicalizes the glob
first (canonicalize lowercases, packages/loopover-engine/src/signals/change-guardrail.ts:11-13) and
emits new RegExp("^…$") with no i flag. So the compiled pattern only ever matches lowercase
input.

Result: an issue body that writes the path with any capitalization — Registry/subnets/foo.json,
registry/Subnets/Foo.json — produces no mentionedPath. With no issueTitleImpliesEntryPattern
configured (or a title that doesn't match it), checkContentLaneDeliverable returns
{ verdict: "not-applicable" }, and the deliverable gate silently does not run for that issue —
the exact "test-only PR closes a content issue without ever delivering the content" gap the function's
own header (src/review/content-lane/registry-logic.ts:875-882) says it exists to close.

classifyRegistryPrScope in the same file already documents why this matters
(src/review/content-lane/registry-logic.ts:778-781): "globToRegExp compiles the spec patterns against
a CANONICALIZED path … so match on canonicalize(f), or an uppercase / ./-prefixed / backslash-separated
changed path silently fails to classify as a registry submission." The issue-body side never got that
treatment.

Deliverables

  • checkContentLaneDeliverable in src/review/content-lane/registry-logic.ts returns
    { verdict: "missing", mentionedPath: "Registry/Subnets/Foo.json" } for an issue body naming
    Registry/Subnets/Foo.json against a PR that changed no matching file — asserted by a new named
    case in test/unit/content-lane-registry-logic.test.ts.
  • The same test asserts the "delivered" verdict for that mixed-case issue body when the PR DOES
    change registry/subnets/foo.json.
  • A test asserts the existing all-lowercase path behaviour (both delivered and missing) is
    unchanged.
  • A test asserts an issue body naming a path that matches NO spec pattern still returns
    not-applicable (the change does not over-broaden the match).

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for
example canonicalizing the token (Deliverable 1) but also lowercasing the reported mentionedPath,
breaking the public-comment text — does not resolve this issue.

Test plan

This repo enforces 99%+ Codecov patch coverage, branch-counted, and vitest.config.ts's
coverage.include covers src/**/*.ts — this file is measured. Both arms of the
find(matchesSpec) outcome (a token that matches, and one that does not) need a test, as do both arms
of the mentionedPath ?? … fallback in the missing return.

Fixes #9667

@phamngocquy
phamngocquy requested a review from JSONbored as a code owner July 30, 2026 16:15
@loopover-orb

loopover-orb Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-30 16:25:13 UTC

2 files · 1 AI reviewer · 1 blocker · CI green · clean

🛑 Suggested Action - Reject/Close

Review summary
This PR fixes a real symmetry bug in checkContentLaneDeliverable: mentionedPath already ran through canonicalize (matching classifyRegistryPrScope's established convention at :778-781), but the delivered check at line 932 was double-canonicalizing changedFiles (canonicalize applied both inline and inside matchesSpec), while the raw issue-body token passed to matchesSpec was never canonicalized at all — so a mixed-case issue-body path token failed to match even when a correctly-cased file was delivered. The fix correctly moves canonicalize inside matchesSpec uniformly for both call sites, matching the documented pattern used elsewhere in the file. Three new tests cover mixed-case issue text, all-lowercase behavior preservation, and the not-applicable case, giving good branch coverage of the change.

Nits — 3 non-blocking
  • The comment in registry-logic.ts:930-932 says 'while keeping the original string for anything user-visible (mentionedPath in the public PR comment)' — worth double-checking that mentionedPath (still derived from the raw, non-canonicalized extractPathTokens output) is indeed what gets surfaced, since the new tests confirm this via toEqual but it's not obvious from the diff alone without reading the surrounding function.
  • No test exercises the double-canonicalization redundancy removal itself (i.e., verifying canonicalize is idempotent) but given canonicalize is a pure lowercase+strip function this is low-risk and not worth a dedicated test.
  • Consider a one-line note near matchesSpec's definition cross-referencing the identical matchesPattern helper in classifyRegistryPrScope (registry-logic.ts:~810) to make the shared canonicalization convention explicit for future readers, similar to the existing comment already added in this diff.

Why this is blocked

  • Linked issue's expected content was never delivered: This PR's linked issue names registry/subnets/foo.json, but the PR's changed files never touch it -- the issue's actual content deliverable does not appear to have been added. — Edit registry/subnets/foo.json to deliver the issue's actual ask, or link the correct issue.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. Linked issue's expected content was never delivered: This PR's linked issue names `registry/subnets/foo.json`, but the PR's changed files never touch it -- the issue's actual content deliverable does not appear to have been added. — Edit registry/subnets/foo.json to deliver the issue's actual ask, or link the correct issue.

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #9667
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 63 registered-repo PR(s), 17 merged, 0 issue(s).
Contributor context ✅ Confirmed Gittensor contributor phamngocquy; Gittensor profile; 63 PR(s), 0 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: minor
Linked issue satisfaction

Addressed
The diff canonicalizes candidates inside matchesSpec (mirroring classifyRegistryPrScope's approach) while preserving the original mentionedPath token, and adds tests for mixed-case delivered/missing, unchanged lowercase behavior, and not-applicable for non-matching paths, covering all four required deliverables.

Review context
  • Author: phamngocquy
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Java, Python, Lua, Jupyter Notebook, C, Dockerfile, JavaScript, Shell
  • Official Gittensor activity: 63 PR(s), 0 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 30, 2026
@codecov

codecov Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.62%. Comparing base (479af01) to head (7886a22).

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9930       +/-   ##
===========================================
- Coverage   91.86%   79.62%   -12.25%     
===========================================
  Files         921      282      -639     
  Lines      113296    58867    -54429     
  Branches    27313     8759    -18554     
===========================================
- Hits       104079    46871    -57208     
- Misses       7929    11706     +3777     
+ Partials     1288      290      -998     
Flag Coverage Δ
backend 100.00% <100.00%> (+4.31%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/review/content-lane/registry-logic.ts 100.00% <100.00%> (ø)

... and 772 files with indirect coverage changes

@loopover-orb

loopover-orb Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (Linked issue's expected content was never delivered). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

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

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

orb(content-lane): canonicalize issue-body path tokens in `checkContentLaneDeliverable

1 participant