docs: address extend-section review findings#26
Conversation
- auth storage: replace "OS keyring" with `~/.mogplex/auth.json` in extend/index.mdx and extend/skills.mdx so the claim matches the canonical CLI docs (cli/guides/authentication, cli/commands/login). - install snippet: prepend a fetch step (git clone or npx degit) in both cli/skills/index.mdx and extend/skills.mdx so the `cp -R skills/*` command works for readers who landed on the rendered docs outside a mogplex-docs checkout. - MCP endpoint URL: add a "placeholder, subject to change" Callout in extend/mcp-server.mdx and replace hard-coded `https://mogplex.com/mcp` in the install snippet with `<URL>` so readers are not led to try an endpoint that may shift before GA. - MCP stdio shim token storage: align with CLI canon (store in `~/.mogplex/auth.json`, not "OS keyring"). - hub card order: lead with the available-today path (Skills) before the preview path (MCP) in extend/index.mdx so the card order matches the recommendation in the surrounding prose. False positives (no change): - "missing terminal period" — byte-check confirms the period is present. - "exec-mode dead link" — /cli/guides/exec-mode exists.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![mogplex[bot]](https://avatars.githubusercontent.com/in/3107927?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mogplex PR Review
Status: Attention needed
This is a focused documentation follow-up that correctly applies five of six prior review findings. Three of the four files are clean. The main concern is a pair of inconsistent and potentially confusing install snippets across extend/skills.mdx and cli/skills/index.mdx, plus a shell command in extend/skills.mdx that is logically broken.
3 findings were added inline.
|
|
||
| ```bash | ||
| # Option A — clone and cd | ||
| git clone https://github.com/webrenew/mogplex-docs.git |
There was a problem hiding this comment.
Critical: Broken shell command: cd mogplex-skills && cd .. undoes the directory change
In extend/skills.mdx, Option B of the new fetch step reads:
npx -y degit webrenew/mogplex-docs/skills mogplex-skills
cd mogplex-skills && cd .. # or rename the dir as you likeThe cd .. immediately reverts the cd mogplex-skills, leaving the user in the original working directory. The next code block then tells readers to run cp -R skills/* ~/.claude/skills/, but after cd .. there is no skills/ subdirectory — degit downloads the contents of skills/ into mogplex-skills/, not a nested skills/ directory. The user will get cp: skills/*: No such file or directory.
The Option A path (git clone … && cd mogplex-docs) leaves the user inside the repo root, so skills/* glob is correct there. For Option B the correct flow should end inside mogplex-skills/ (or the second block should reference mogplex-skills/* from the parent). Suggested fix for Option B:
# Option B — sparse download of just the skills/ tree
npx -y degit webrenew/mogplex-docs/skills mogplex-skills
# The directory is now ready; see copy commands below — run them from the parent, not inside mogplex-skillsThen change the copy block to reference mogplex-skills/* explicitly, or note that the user must cd mogplex-skills first.
|
|
||
| ```bash | ||
| # Fetch just the skills/ tree (no full clone needed) | ||
| npx -y degit webrenew/mogplex-docs/skills mogplex-skills |
There was a problem hiding this comment.
Warning: Install instructions are inconsistent between extend/skills.mdx and cli/skills/index.mdx
The two pages address the same install task but diverge in a way that will confuse readers who visit both:
extend/skills.mdx(Option B) downloads tomogplex-skills/, then the subsequent copy block usescp -R skills/* ~/.claude/skills/— which requires the user to be inside a directory that contains askills/subdirectory. Afterdegit, no such subdirectory exists;mogplex-skills/is the skills tree.cli/skills/index.mdx(Claude Code block) correctly usescp -R . ~/.claude/skills/aftercd mogplex-skills, acknowledging that the current directory is the skills tree.
For the per-project case in cli/skills/index.mdx, the snippet uses cp -R /path/to/mogplex-skills/* .claude/skills/ with a hardcoded placeholder path, which is less helpful than a relative path or an explicit note to replace it. Consider aligning both pages to the same download-then-copy pattern and making sure the glob targets are consistent.
| @@ -44,8 +44,22 @@ When the MCP server ships, both paths will stay supported. Skills will continue | |||
|
|
|||
| ## Install | |||
There was a problem hiding this comment.
Suggestion: Option A (git clone) leaves the user inside mogplex-docs/ — the copy command needs skills/*, not skills/
In extend/skills.mdx, Option A clones the whole repo and does cd mogplex-docs. The next code block then runs cp -R skills/* ~/.claude/skills/, which works correctly from that location. This is fine, but a quick inline comment — # run from repo root — would make it unambiguous for users who might cd skills out of habit before running the copy.
![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: 38aea9b4bd
ℹ️ 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".
| npx -y degit webrenew/mogplex-docs/skills mogplex-skills | ||
| cd mogplex-skills && cd .. # or rename the dir as you like |
There was a problem hiding this comment.
If a reader follows Option B (npx -y degit webrenew/mogplex-docs/skills mogplex-skills), the downloaded directory contains the skill folders at its root, not under a skills/ subdirectory. The next install step still tells them to copy skills/*, which will fail (cannot stat 'skills/*') in the documented flow, so the “sparse download” path is currently unusable unless they manually rewrite commands.
Useful? React with 👍 / 👎.
1 participant
Follow-up to #25. Addresses the six findings from the Mogplex PR Review.
Fixes applied
extend/index.mdx,extend/skills.mdx,extend/mcp-server.mdxstdio-shim note). "OS keyring" replaced with~/.mogplex/auth.jsonto match the canon incli/guides/authentication.mdxandcli/commands/login.mdx. Added a link to the Authentication guide where it makes sense.extend/skills.mdx,cli/skills/index.mdx). Thecp -R skills/*command assumed readers had a localskills/dir. Added a fetch step (git cloneornpx degit webrenew/mogplex-docs/skills) so the snippet works from any starting point.extend/mcp-server.mdx). Added a placeholder Callout and replaced the hard-codedhttps://mogplex.com/mcpin the install code block with<URL>, so early readers aren't led to wire an unstable URL into production clients.extend/index.mdx). Swapped to lead with Skills (available today, recommended) before MCP (preview), matching the recommendation text below the cards.False positives (no change)
od -c) confirms.\nis present; reviewer's diff view clipped it./cli/guides/exec-modepossibly dead — file exists atcontent/docs/cli/guides/exec-mode.mdx; build was rendering it fine.Test plan
pnpm lintpnpm types:checkpnpm build/extend/mcp-server— new Callout renders, install snippet uses<URL>/extend/skillsand/cli/skills— install snippet shows fetch step abovecp/extend— Skills card appears first