Skip to content

fix: make jd license URL deterministic before grouping reports - #3534

Open
IrynaKulakova wants to merge 1 commit into
mainfrom
irynakulakova-pr-3490-feedback
Open

IrynaKulakova wants to merge 1 commit into
mainfrom
irynakulakova-pr-3490-feedback

Conversation

@IrynaKulakova

Copy link
Copy Markdown
Contributor

Summary

Normalize the inconsistent jd/v2 license URL before sorting and hashing architecture reports, preventing spurious license-check failures. Regenerate the Darwin, Linux, and Windows reports with the canonical URL.

Why

Addresses the review feedback on #3490. The failed job split otherwise identical Windows architecture reports because go-licenses emitted different license URLs for the same dependency; making notification failures nonfatal does not fix that mismatch.

What changed

  • Normalize only https://github.com/josephburnett/jd/blob/v2.5.0/v2/LICENSE to the verified repository-root license URL before sorting and hashing. This is a version-specific workaround, not a general rewrite of /v2/ paths.
  • Regenerate all three platform reports. Preserve the final validation failure gate and leave workflow notification behavior unchanged.

MCP impact

  • No tool or API changes - only license-report generation and generated reports change.
  • Tool schema or behavior changed
  • New tool added

Prompts tested (tool changes only)

  • N/A - no tool changes.

Security / limits

  • No security or limits impact - license validation remains enforced; no permissions change.
  • Auth / permissions considered
  • Data exposure, filtering, or token/size limits considered

Tool renaming

  • I am renaming tools as part of this PR (e.g. a part of a consolidation effort)
    • I have added the new tool aliases in deprecated_tool_aliases.go
  • I am not renaming tools as part of this PR - no tool definitions change.

Note: if you're renaming tools, you must add the tool aliases. For more information on how to do so, please refer to the official docs.

Lint & tests

  • Linted locally with ./script/lint - script/lint passed with 0 issues.
  • Tested locally with ./script/test - script/test passed, including race tests.

Additional validation:

  • PATH="$(go env GOPATH)/bin:$PATH" script/licenses - passed twice; SHA-256 comparisons confirmed identical reports and all saved license files across consecutive runs.
  • bash -n script/licenses - passed.
  • Focused normalization assertions passed: the incorrect URL is corrected; the canonical URL, other versions, other modules, and other filenames remain unchanged. The final failure gate remains intact.
  • git diff --check - passed.

Docs

  • Not needed
  • Updated (README / docs / examples) - regenerated the three platform license reports; no MCP documentation changes needed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 66177876-e733-4a86-b7bc-236e5d06cbf7
@IrynaKulakova
IrynaKulakova requested a review from a team as a code owner October 19, 2026 15:23
Copilot AI max review requested due to automatic review settings October 19, 2026 15:23

Copilot AI 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.

🟢 Approval recommended

The narrow rewrite matches the pinned dependency, the reports are consistent, and validation remains enforced.

No open issues 🎉

What changed in this PR

Makes jd/v2 license URLs deterministic so identical architecture reports group consistently.

Changes:

  • Normalize the verified v2.5.0 URL before sorting and hashing.
  • Refresh platform reports without changing license validation or workflow notifications.
File Description
third-party-licenses.windows.md Uses the canonical license URL.
third-party-licenses.linux.md Uses the canonical license URL.
third-party-licenses.darwin.md Uses the canonical license URL.
script/​licenses Normalizes the version-specific URL before grouping.

Review effort: Max

Copilot AI 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.

🟢 Approval recommended

Focused checks confirm URL convergence, unrelated URLs remain unchanged, and the generated reports match the correction.

No open issues 🎉

Review effort: Max

Comment thread script/licenses
Comment on lines +93 to +97
# go-licenses can resolve jd/v2's license to a nonexistent v2/LICENSE.
# At v2.5.0 it lives at the repository root; normalize before hashing.
grep -E '^ - \[' "${report_file}" |
sed 's|https://github\.com/josephburnett/jd/blob/v2\.5\.0/v2/LICENSE)|https://github.com/josephburnett/jd/blob/v2.5.0/LICENSE)|g' |
LC_ALL=C sort > "${packages_file}"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we make this generic instead of pinning jd v2.5.0? go-licenses sends a HEAD to blob/<tag>/v2/go.mod and only keeps the v2/ folder when that request goes through, so it's really a flaky network check that can hit any /vN module living in a subfolder. if we always drop the folder we get the same output as when the check fails (which is also the link that actually works), and it won't quietly stop matching when jd gets bumped

Suggested change
# go-licenses can resolve jd/v2's license to a nonexistent v2/LICENSE.
# At v2.5.0 it lives at the repository root; normalize before hashing.
grep -E '^ - \[' "${report_file}" |
sed 's|https://github\.com/josephburnett/jd/blob/v2\.5\.0/v2/LICENSE)|https://github.com/josephburnett/jd/blob/v2.5.0/LICENSE)|g' |
LC_ALL=C sort > "${packages_file}"
# go-licenses only keeps a /vN/ folder in license URLs when a GitHub check succeeds, so always drop it.
grep -E '^ - \[' "${report_file}" |
sed -E 's#(github\.com/[^/]+/[^/]+/blob/[^/]+)/v[0-9]+/#\1/#' |
LC_ALL=C sort > "${packages_file}"

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants