test(http-utils): cover URL-candidate and selector error/boundary branches - #344
Conversation
Adds focused coverage for previously-unexercised branches in src/cli/http-utils.ts: - normalizeHttpUrlCandidate: missing/empty/whitespace input and a scheme-prefixed value that is not a valid URL all return undefined - looksLikeHttpUrl: false for a non-URL, true for a normalizable candidate - normalizeHttpUrl: returns undefined for unparseable input - splitHttpToolSelector: rejects a tool suffix with non-identifier characters Test-only, no production changes. Each new case passes on current main and fails when its target branch is mutated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012QouxMJcq3oBcqZdS9fw4Y
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. Reviewed September 5, 2026, 3:42 PM ET / 19:42 UTC. ClawSweeper reviewWhat this changesAdds six tests for HTTP URL normalization, empty or malformed candidates, and invalid tool-selector suffixes without changing runtime behavior. Merge readiness✅ Ready for maintainer review This remains a useful, focused coverage addition absent from current main. The maintainer’s exact-head CLI validation resolves the previous proof blocker; no actionable correctness or security findings remain. Priority: P3 Review scores
Verification
How this fits togetherMCPorter’s CLI interprets typed server targets as configured names or HTTP endpoints, optionally separating a tool name from the URL. These helpers feed server lookup and the list/call commands. flowchart LR
A[Typed server target] --> B[Normalize URL candidate]
B --> C[Check tool suffix]
C --> D[Resolve server endpoint]
B --> E[Configured name or diagnostic]
D --> F[List or call MCP tools]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Retain the focused characterization tests alongside existing CLI integration coverage, preserving the distinction between invalid tool suffixes and valid endpoint paths. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR characterizes existing behavior rather than repairing a reported bug; source inspection and the supplied exact-head validation support the assertions. Is this the best way to solve the issue? Yes. Extending the existing helper suite is a narrow, maintainable way to protect these branches, supplemented by mutation evidence and real CLI validation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 9957bd1e233c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
|
Maintainer validation on
The full local suite reached 1803 passing tests with one environment failure: this Mac's long private temporary path exceeded the Unix socket path limit in No production behavior changes, so no user-facing changelog entry is required. Keep the contributor's human credit in the squash commit; omit generated/session attribution trailers. |
What Problem This Solves
src/cli/http-utils.tsholds the URL helpers behind HTTP-server target resolution:normalizeHttpUrlCandidate(promote/validate a user-typed target),looksLikeHttpUrl,normalizeHttpUrl, andsplitHttpToolSelector(split abase.toolselector). The existingtests/http-utils.test.tscovered the happy paths ofnormalizeHttpUrlandsplitHttpToolSelector, but several error/boundary branches shipped with no coverage at any level:normalizeHttpUrlCandidatereturningundefinedfor missing / empty / whitespace-only input, and for a scheme-prefixed value that is not a valid URL (e.g.https://).looksLikeHttpUrl— the boolean contract on a non-URL vs a normalizable candidate.normalizeHttpUrlreturningundefinedfor unparseable input.splitHttpToolSelectorrejecting a tool suffix containing non-identifier characters (the/^[A-Za-z0-9_-]+$/guard).A future edit to any of those branches could silently change how a mistyped or malformed HTTP target/selector is handled, with no failing test to catch it.
Why This Change Was Made
Coverage-only. Extends
tests/http-utils.test.ts(+39 / −1, no production code touched) with focused characterization tests that pin the currentmainbehavior of the previously-uncovered branches. No new config, defaults, dependencies, or behavior.User Impact
No user-visible or runtime change. For maintainers, the URL-candidate and tool-selector rejection contracts now regress loudly instead of silently: a future edit that drops the empty/whitespace guard, the scheme-but-invalid guard, the unparseable-input guard, or the tool-suffix character check will fail this suite.
Evidence
Linux, Node 22,
pnpm install --frozen-lockfilefrom source, branched off currentmain(9579ed6).12/12 pass on clean
main:These branches were genuinely uncovered — the whole suite (unit + integration) does not assert them. Applying a combined behavior-changing mutation to exactly these branches in
src/cli/http-utils.ts(empty/whitespace/invalid → return a non-undefinedvalue; drop the tool-suffix regex) and running the entire test suite leaves the pass/fail count unchanged (same pre-existing environment-dependent failures, zero new ones) — no existing unit or integration test's assertion depends on these branches, includingtests/cli-http-selector.integration.test.ts:The new tests bite — under the same mutation, this suite fails exactly on the targeted arms:
Coverage of the target branches (lines 5, 9, 20, 56, 81) is now filled — the only branches that remain uncovered afterward are the defensive/unreachable ones (a
URLalways has a pathname; the selector'snew URLcannot throw afternormalizeHttpUrlCandidatealready validated it;baseSegmentis non-empty wheneverdotIndex > 0), which this suite deliberately does not target.Format / lint / types clean on the changed file:
Scope: one existing test file extended,
+39 / −1, no production code changed. Same accept-shape as the merged OAuth-coverage PRs #246 / #281 / #331.AI-assisted contribution.
Opened from an org-owned fork via the API; if GitHub's Allow edits by maintainers toggle isn't honored on this PR, a maintainer can still push to the branch or supersede-and-land.
Generated by Claude Code