feat(check): add check for URL correctness + double slashes by twitchyliquid64 · Pull Request #944 · gominimal/minimal

twitchyliquid64 · 2026-07-23T20:44:57Z

Makes the issue from gominimal/pkgs#516 a check error.

Note

Add URL correctness check for web sources in `check_package`

Adds a new SourceUrlsValid graph-based checker in sources.rs that validates URLs on web Source build dependencies. The checker fails on unparseable URLs, unsupported schemes (only http, https, and gs are allowed), or double slashes in the URL path, and respects skip_checkers.

^{Macroscope summarized 81b4cbc.}

Summary by CodeRabbit

New Features
- Added validation for web source URLs used by build inputs.
- URLs are checked for valid formatting, supported schemes (http, https, and gs), and accidental double slashes in paths.
- Reports clear validation results, including passed, failed, or skipped checks.
Tests
- Added coverage for valid URLs, invalid formats, unsupported schemes, double-slash paths, missing sources, and skipped checks.

coderabbitai · 2026-07-23T20:45:17Z

📝 Walkthrough

Walkthrough

The check crate adds a graph-based checker for web source URLs, validates URL syntax, schemes, and path slashes, integrates its result into package checks, and adds tests covering pass, fail, and skip outcomes.

Changes

Source URL validation

Layer / File(s)	Summary
URL validation implementation `crates/check/Cargo.toml`, `crates/check/src/sources.rs`	Adds the URL dependency and validates web source URLs for syntax, supported schemes, and double-slashes in paths.
Checker harness integration `crates/check/src/lib.rs`	Registers `SourceUrlsValid` and aggregates its result with the existing graph-based checkers.
URL validation test coverage `crates/check/src/sources.rs`	Tests valid URLs, missing sources, malformed URLs, unsupported schemes, double-slashes, and configured skips.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PackageChecker
  participant SourceUrlsValid
  participant BuildGraph
  participant CheckResult
  PackageChecker->>SourceUrlsValid: run checker
  SourceUrlsValid->>BuildGraph: find package and build dependencies
  BuildGraph-->>SourceUrlsValid: return web sources
  SourceUrlsValid->>SourceUrlsValid: parse and validate URLs
  SourceUrlsValid-->>CheckResult: produce pass, fail, or skip
  CheckResult-->>PackageChecker: aggregate checker outcome

Suggested reviewers: bryan-minimal

Poem

A rabbit checks each source with care,
For tidy paths and schemes so fair.
HTTPS hops, while errors flee,
Eleven checks now guard the tree.
Hop hop—clean URLs everywhere!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description lacks the required Summary, Testing, and Checklist sections from the template.	Add the template sections with a clear summary, testing evidence, and checklist items such as docs and breaking-change status.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is a clear conventional-commit summary that matches the new URL validation check.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

_{Comment @coderabbitai help to get the list of available commands.}

bryan-minimal

LGTM

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/check/src/sources.rs`:
- Around line 141-156: Remove the CARGO_MANIFEST_DIR mutation and restoration
around Layer::new_for_test in the affected test. Update the parsing flow to
receive the stdlib resolver root explicitly, or isolate it in a child process,
so it does not rely on ENV_LOCK or process-wide environment state.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 32089a5b-b970-46a2-9d5b-96e781ab54bf

📥 Commits

Reviewing files that changed from the base of the PR and between 07a7750 and 81b4cbc.

⛔ Files ignored due to path filters (1)

Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

crates/check/Cargo.toml
crates/check/src/lib.rs
crates/check/src/sources.rs

coderabbitai · 2026-07-23T20:47:59Z

+            let _guard = ENV_LOCK.lock().expect("env lock");
+            let prev_manifest_dir = std::env::var_os("CARGO_MANIFEST_DIR");
+            // SAFETY: every test that mutates CARGO_MANIFEST_DIR does so while
+            // holding ENV_LOCK, and no other code in this test binary reads the
+            // variable, so this set never races with another thread's access.
+            unsafe {
+                std::env::set_var("CARGO_MANIFEST_DIR", &stdlib);
+            }
+            let parsed = Layer::new_for_test(nickel.to_string());
+            // SAFETY: same invariant as the set above — still under ENV_LOCK.
+            unsafe {
+                match prev_manifest_dir {
+                    Some(v) => std::env::set_var("CARGO_MANIFEST_DIR", v),
+                    None => std::env::remove_var("CARGO_MANIFEST_DIR"),
+                }
+            }


🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash set -euo pipefail fd -t f -e rs '^outputs\.rs$' crates --exec ast-grep outline {} --items all rg -n -C2 'CARGO_MANIFEST_DIR|std::env::(set_var|remove_var|var_os)' crates --type rust

Repository: gominimal/minimal

Length of output: 29919

🏁 Script executed:

#!/bin/bash set -euo pipefail sed -n '1,240p' crates/check/src/sources.rs echo '---' sed -n '1,220p' crates/check/src/outputs.rs echo '---' sed -n '1,220p' crates/mip/src/cmd_dep.rs

Repository: gominimal/minimal

Length of output: 25842

🏁 Script executed:

#!/bin/bash set -euo pipefail printf '\n== check Cargo.toml for test/runtime features ==\n' sed -n '1,220p' crates/check/Cargo.toml printf '\n== sources.rs test annotations and nearby helpers ==\n' rg -n -C2 '#\[tokio::test\]|serial_test|ENV_LOCK|graph_with_pkg|Layer::new_for_test' crates/check/src/sources.rs printf '\n== outputs.rs test annotations and nearby helpers ==\n' rg -n -C2 '#\[tokio::test\]|serial_test|ENV_LOCK|graph_with_pkg|Layer::new_for_test' crates/check/src/outputs.rs printf '\n== any crate-local test serialization or env-lock patterns ==\n' rg -n -C2 'static ENV_LOCK|serial_test|#[[:space:]]*test|tokio::test' crates/check/src crates/check/tests crates/check/Cargo.toml

Repository: gominimal/minimal

Length of output: 15270

Remove the process-wide env mutation here. ENV_LOCK only serializes callers in this module, while crates/check/src/outputs.rs mutates CARGO_MANIFEST_DIR under a separate lock. In async tests this still leaves a process-wide race; pass the resolver root explicitly or isolate the parse in a child process.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/check/src/sources.rs` around lines 141 - 156, Remove the CARGO_MANIFEST_DIR mutation and restoration around Layer::new_for_test in the affected test. Update the parsing flow to receive the stdlib resolver root explicitly, or isolate it in a child process, so it does not rely on ENV_LOCK or process-wide environment state.

feat(check): add check for URL correctness + double slashes

81b4cbc

twitchyliquid64 requested a review from bryan-minimal July 23, 2026 20:44

bryan-minimal approved these changes Jul 23, 2026

View reviewed changes

coderabbitai Bot reviewed Jul 23, 2026

View reviewed changes

twitchyliquid64 enabled auto-merge (squash) July 23, 2026 20:53

twitchyliquid64 merged commit 9375eff into main Jul 23, 2026
29 checks passed

twitchyliquid64 deleted the tom/check branch July 23, 2026 21:12

Navigation Menu

Uh oh!

feat(check): add check for URL correctness + double slashes - #944

feat(check): add check for URL correctness + double slashes#944
twitchyliquid64 merged 1 commit into
mainfrom
tom/check

twitchyliquid64 commented Jul 23, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 23, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

bryan-minimal left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

twitchyliquid64 commented Jul 23, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add URL correctness check for web sources in check_package

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

bryan-minimal left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

twitchyliquid64 commented Jul 23, 2026 •

edited by coderabbitai Bot

Loading

Add URL correctness check for web sources in `check_package`

coderabbitai Bot commented Jul 23, 2026 •

edited

Loading