fix: numeric port normalization; make LICENSE machine-detectable - #2
Conversation
Two small fixes, unrelated except that both cost information for a formatting reason. `url.URL.Port` preserves leading zeros, and the default-port check compared the port as text, so "https://host:0443/pkg" kept its port while "https://host/pkg" did not. The two named one location and reconciled as a disagreement, throwing away a valid origin. Ports are now rewritten from their parsed number, which also settles ":08443" against ":8443" -- the same problem one step along, which a test had quietly frozen as expected behaviour. The LICENSE file began with a blank line, leaving it one line longer than the canonical Apache-2.0 text. GitHub's licence detection does not classify it, so `licenseInfo.spdxId` is empty and anything reading GitHub rather than deps.dev sees this module as unlicensed -- which is what a Bomly scan of a consumer reported. Removing the blank line leaves the text byte-identical to the canonical version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 41 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes add an Apache License 2.0 file and update ChangesOrigin port normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR normalizes equivalent numeric ports and restores machine-detectable Apache-2.0 licensing with targeted verification; no actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84b1d598d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Rebuilding the host for a rewritten port restored brackets when the hostname
contained a colon. That is correct for every host url.Parse currently accepts --
a bracketed host that survives parsing is an IPv6 literal, since bracketed IPv4
and IPvFuture are both rejected -- but it infers the answer from the hostname's
contents when the URL already said it plainly.
Keying off whether the parsed host was bracketed says what is meant and stays
correct if host validation ever widens, where an unbracketed literal would name
a different host.
No behaviour change: a review suggested this as a fix for IPvFuture hosts such
as "[v1.foo]", which url.Parse rejects outright ("unexpected character"), so
that case never reaches this code.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Two small fixes. Unrelated except that both lose information for a formatting reason.
Ports are normalized from their number, not their text
url.URL.Portpreserves leading zeros, and the default-port check compared the port as a string. Sohttps://host:0443/pkgkept its port whilehttps://host/pkgdid not — the two name one location, andReconcileOriginmarked them a disagreement and discarded a valid origin.Ports are now rewritten from the parsed number that the 1–65535 range check already computes. That also settles
:08443against:8443, which is the same problem one step along — and which a test in v0.4.0 had frozen as expected behaviour, asserting the leading zeros were "kept as written". Fixing only the default-port case would have left that asymmetry in place.Reported by Codex review on bomly-cli#397 (comment 3809856511), where the rule now lives here rather than in the CLI.
LICENSE is detectable again
The file began with a blank line, leaving it 202 lines against the canonical Apache-2.0 text's 201. GitHub's licence detection does not classify it, so
licenseInfo.spdxIdis empty, and anything reading GitHub rather than deps.dev sees this module as unlicensed. That is exactly what surfaced as a code-scanning alert on a consumer: "Package license is unknown in pkg:golang/github.com/bomly-dev/bomly-sdk@v0.4.0".deps.dev does report
Apache-2.0, so the module's licensing was never in doubt — only its machine-readability. Removing the blank line leaves the text byte-identical to the canonical version (verified with a diff against the previous content minus that line).Verification
go test ./...,go vet ./..., gofmt clean.:0443,:080, and:08443, plus reconciliation asserting one port written two ways is one location while a genuinely different port still disagrees.FuzzPackageOriginclean at 25s.Worth a patch release when convenient — bomly-cli#397 is on v0.4.0 and does not need this to merge, but it carries the same reconciliation behaviour.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation