Repository navigation
Conversation
AI-assisted implementation with Codex. Co-authored-by: BlueX888 <[email protected]>
|
This pull request is awaiting maintainer assignment. Linked PRs stay open while maintainers triage the issue; this check remains failing until assignment. Per CONTRIBUTING.md, an external PR must reference an issue that's assigned to its author. To get there:
Once you're assigned and the link is present, this check is re-run automatically. Previously gate-closed PRs also reopen — no further action needed. Maintainers: reopen this PR or remove the |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
marvin-context-protocol
Bot
added
the
bug
BrickerP
left a comment
BrickerP
left a comment
There was a problem hiding this comment.
Looks correct for #5819.
Advertising quote(skill.name, safe='') in list/template URIs, then unquote on the skill segment in _get_resource / _get_resource_template, matches how AnyUrl already percent-encodes non-ASCII hosts and rejects spaces — so list→read round-trips for café / my skill / # names without double-encoding the pre-quoted form. Keeping resource name / manifest skill id as the raw directory name is the right split.
The skill+one legacy read path is a nice touch (unquote leaves + alone; template uri_template keeps parts[0] spelling). Parametrized tests cover the interesting hosts, including a literal %20 directory.
Process only: check-issue-link is red because HootRock is not assigned on #5819 — not a code defect.
![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: 66f92bda64
ℹ️ 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".
AI-assisted implementation with Codex. Co-authored-by: BlueX888 <[email protected]>
AI-assisted conflict resolution with Codex. Co-authored-by: BlueX888 <[email protected]>
BrickerP
left a comment
BrickerP
left a comment
There was a problem hiding this comment.
Re-reviewed on head 02229fd55573fa874cf0d24871f85256bb5ef0f4.
URI host encoding with quote(..., safe=\"\" ) (and safe=\"/\" only under preserve_bytes), lookup via unquote of the skill segment, and matching client utilities (list_skills / get_skill_manifest / download_skill) form a coherent round-trip. The new parametrized cases (spaces, non-ASCII, literal %20, #) plus the legacy skill+one read cover the failure mode well.
![fastmcp-triage-reviewer[bot]](https://avatars.githubusercontent.com/u/31014960?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review by the FastMCP triage reviewer bot.
The focused run passes all 22 cases, including the reported URI round trips. The namespaced-download test nevertheless asserts an outdated contract: current downloads verify frontmatter and digests and install mounted skills under their final name. Its invalid fixture and nested output expectation cannot establish compatibility with current main.
When updating onto current main, preserve the verified export path. Give the namespaced fixture valid frontmatter and digest/size, expect the final-name directory, and retain the existing mounted-skill tests.
Checks
Retained base/head run: 19 failures + 3 passes → 22 passes. Source/history inspection confirms the export and mounted-directory contracts; no additional tests run.
|
thanks for this. closing along with #5819; the comment there explains why. |
Skills named
caféormy skillcan be listed with unreadable resources, or vanish from the list. Encode directory names in resource URIs and decode them once during lookup and discovery. The public manifest, download and sync helpers use the same encoding, preserving original local directory and file names, including literal%20. Existing namespaced skills and unescaped reads such asskill://skill+one/guide.mdremain readable.Fixes #5819.
AI-assisted implementation with Codex. reviewed and tested locally.
Co-authored-by: BlueX888 [email protected]