Fix: parse_source_ref rejecting refs containing a slash - #501
Conversation
The credential-URL guard in parse_source_ref refused to split any trailing @ref that contained a `/`, to avoid misparsing `https://user:pass@host/path` as a ref split. That guard overshot: it also rejected legitimate branch names using the common type/description convention (chore/..., feature/..., docs/...), leaving the whole string - ref included - globbed into the clone URL and producing a malformed install source. Rewritten to detect credential URLs by position (the @ falls inside the URL's authority, before any path separator following the scheme) instead of by whether the ref contains a slash, so slash-containing refs split correctly everywhere, including full clone URLs with embedded credentials plus a trailing ref. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016F4jsbbHAZtUrKf6MmYDM2
![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: 1f10740793
ℹ️ 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".
| if let Some(scheme_end) = before.find("://") { | ||
| let authority_start = scheme_end + 3; | ||
| if !before[authority_start..].contains('/') { |
There was a problem hiding this comment.
@ characters in generic file URLs
When a supported generic URL contains @ in its path followed by another path segment, such as file:///tmp/repo@group/plugin.git, this condition treats group/plugin.git as a ref and causes plugin installation to clone file:///tmp/repo instead of the requested repository. The previous slash guard kept this input intact, and the plugin contract explicitly says generic file:// URLs are cloned as-is (specs/plugin/plugin.spec.md lines 178–182), so ref detection must distinguish URL path characters from the explicit ref syntax.
AGENTS.md reference: AGENTS.md:L356-L356
Useful? React with 👍 / 👎.
| #[test] | ||
| fn parse_source_ref_branch_with_slash() { |
There was a problem hiding this comment.
These three new regression tests are documented only in the accepted change artifact, while the canonical specs/trust/testing.md unit-test inventory still ends with the old credential test and omits all slash-ref coverage. Because canonical module companions remain the maintained test strategy after a change artifact is accepted, add the new cases there as well so the module spec stays synchronized with the implementation.
AGENTS.md reference: AGENTS.md:L377-L377
Useful? React with 👍 / 👎.
| --- | ||
| module: trust | ||
| version: 5 | ||
| version: 7 |
There was a problem hiding this comment.
The parent spec is version 5, but this change advances the frontmatter directly to 7 and adds both version 6 and version 7 changelog entries for the same CHG-0005 behavior. This makes the source-of-truth history report two revisions where only one occurred and contradicts the commit's stated v6 bump; retain one next-version entry and keep the frontmatter aligned with it.
AGENTS.md reference: AGENTS.md:L356-L356
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Corvin says...
_
<(^\ .oO(Caw! ^v^)
|/(\
\(\\
" "\\
"That's a nice looking export you've got there."
CI Summary
| Check | Status |
|---|---|
| Dependency Audit | ✅ Passed |
| Integration (3 OS) | ✅ Passed |
| Lint (fmt + clippy) | ✅ Passed |
| Spec Validation | ✅ Passed |
| Tests (3 OS) | ✅ Passed |
Powered by corvid-pet
1 participant
Summary
parse_source_ref's credential-URL guard rejected any trailing@refcontaining/, breaking installs from branches using the commontype/descriptionnaming convention (e.g.chore/0.2.0-launch-prep) — the whole string got globbed into the clone URL instead of being split into repo + ref@sits inside the URL's authority, before any path separator following the scheme) instead of by whether the ref contains a slashspecs/trust/trust.spec.mdto v6Test plan
cargo test(966 passed),cargo fmt --check,cargo clippy -- -D warningsspecsync change check --strict --require-coverage 100passes🤖 Generated with Claude Code
https://claude.ai/code/session_016F4jsbbHAZtUrKf6MmYDM2