Add CLI, MCP, and skill surfaces#157
Conversation
📝 WalkthroughWalkthroughThis PR introduces Innerscape, a new agent skill for reflective project organization. It adds a Node.js CLI tool and MCP server that expose three capabilities: project brief generation, module map listing, and planning prompts. These are registered as package executables and documented in the skill definition, README, and agent surfaces guide. ChangesInnerscape Agent Implementation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: a857d053f3
ℹ️ 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".
| process.exitCode = 2; | ||
| } | ||
|
|
||
| const invokedPath = process.argv[1] ? fileURLToPath(import.meta.url) === process.argv[1] : false; |
There was a problem hiding this comment.
When the package bin entry is used via npm link, npx, or a workspace .bin shim, Node starts this file through a symlink path, so process.argv[1] is the symlink while import.meta.url resolves to the real file. In that installed-command scenario innerscape brief exits successfully with no output, even though the skill and projectBrief() advertise innerscape as the CLI surface; use a symlink-aware check or invoke main() for this CLI entry point.
Useful? React with 👍 / 👎.
| "mcpServers": { | ||
| "innerscape": { | ||
| "command": "npm", | ||
| "args": ["run", "mcp"] |
There was a problem hiding this comment.
With this .mcp.json command, an MCP host starts the server through npm run mcp, and npm writes its lifecycle banner to stdout before the JSON-RPC response. The MCP stdio transport requires stdout to contain only valid protocol messages; I confirmed printf '{"jsonrpc":"2.0","id":1,"method":"tools/list"}\n' | npm run mcp emits the > innerscape@0.1.0 mcp banner on stdout first, so clients using the checked-in config will fail to parse the stream. Point the config at node tools/innerscape-mcp.mjs or add npm's silent flag.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
skills/innerscape/SKILL.md (1)
33-43: ⚡ Quick winConsider clarifying when to use this MCP config vs the README example.
This example assumes
innerscape-mcpis in PATH (e.g., afternpm install -gornpm link). The README.md (lines 58-67) shows a different approach usingnpm run mcpwith--prefix, which works for local development without installation.Both are valid, but users might benefit from a note explaining which context each applies to.
📝 Optional clarification
## MCP Setup +For installed packages (after `npm link` or global install): + ```json { "mcpServers": { "innerscape": { "command": "innerscape-mcp" } } }
+For local development, see the README.md example using
npm run mcp --prefix.</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@skills/innerscape/SKILL.mdaround lines 33 - 43, Add a short clarifying note
under the "MCP Setup" example explaining that the shown mcpServers -> innerscape
-> command "innerscape-mcp" assumes the binary is in PATH (installed globally
via npm install -g or npm link), and reference the README.md example (thenpm run mcp --prefixapproach) as the recommended option for local development;
update the SKILL.md section near the "MCP Setup" block to include this
distinction so users know when to use the PATH-based "innerscape-mcp" vs the
localnpm run mcpworkflow.</details> </blockquote></details> <details> <summary>tools/innerscape-cli.mjs (2)</summary><blockquote> `72-75`: _💤 Low value_ **Consider validating flag values.** The `getValue` helper silently falls back to the default when a flag is present but has no value (e.g., `--focus` at the end of arguments), and it doesn't prevent capturing another flag as a value (e.g., `--focus --energy low` would set `focus` to `"--energy"`). While the current fallback behavior may be acceptable for this CLI, consider adding validation to improve the user experience. <details> <summary>🛡️ Optional enhancement for flag value validation</summary> ```diff const getValue = (flag, fallback) => { const index = rest.indexOf(flag); - return index >= 0 && rest[index + 1] ? rest[index + 1] : fallback; + if (index < 0) return fallback; + const value = rest[index + 1]; + if (!value || value.startsWith('--')) return fallback; + return value; };🤖 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 `@tools/innerscape-cli.mjs` around lines 72 - 75, The getValue helper currently returns a fallback when a flag is present but the next token is missing or may be another flag; update getValue to validate the next token by checking that rest[index + 1] exists and does not start with '-' (or is not another recognized flag), and if invalid either return the fallback or surface a user-friendly error message; reference the getValue function and the rest array to locate the change, and consider also supporting "--flag=value" parsing if desired.
100-103: 💤 Low valueConsider simplifying the direct invocation check.
The current pattern with the ternary is defensive but slightly verbose. The standard ESM pattern is more direct.
♻️ Optional simplification
-const invokedPath = process.argv[1] ? fileURLToPath(import.meta.url) === process.argv[1] : false; -if (invokedPath) { +if (process.argv[1] && fileURLToPath(import.meta.url) === process.argv[1]) { main(); }🤖 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 `@tools/innerscape-cli.mjs` around lines 100 - 103, The direct-invocation check is overly verbose; replace the ternary expression around invokedPath with a simple equality comparison using fileURLToPath(import.meta.url) === process.argv[1] and call main() when that evaluates true. Locate the invokedPath variable and the subsequent if (invokedPath) { main(); } and simplify by computing the boolean directly from fileURLToPath(import.meta.url) === process.argv[1] (or by inlining that comparison into the if) so the script uses the standard ESM direct-invocation pattern with main().
🤖 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.
Nitpick comments:
In `@skills/innerscape/SKILL.md`:
- Around line 33-43: Add a short clarifying note under the "MCP Setup" example
explaining that the shown mcpServers -> innerscape -> command "innerscape-mcp"
assumes the binary is in PATH (installed globally via npm install -g or npm
link), and reference the README.md example (the `npm run mcp --prefix` approach)
as the recommended option for local development; update the SKILL.md section
near the "MCP Setup" block to include this distinction so users know when to use
the PATH-based "innerscape-mcp" vs the local `npm run mcp` workflow.
In `@tools/innerscape-cli.mjs`:
- Around line 72-75: The getValue helper currently returns a fallback when a
flag is present but the next token is missing or may be another flag; update
getValue to validate the next token by checking that rest[index + 1] exists and
does not start with '-' (or is not another recognized flag), and if invalid
either return the fallback or surface a user-friendly error message; reference
the getValue function and the rest array to locate the change, and consider also
supporting "--flag=value" parsing if desired.
- Around line 100-103: The direct-invocation check is overly verbose; replace
the ternary expression around invokedPath with a simple equality comparison
using fileURLToPath(import.meta.url) === process.argv[1] and call main() when
that evaluates true. Locate the invokedPath variable and the subsequent if
(invokedPath) { main(); } and simplify by computing the boolean directly from
fileURLToPath(import.meta.url) === process.argv[1] (or by inlining that
comparison into the if) so the script uses the standard ESM direct-invocation
pattern with main().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 868084eb-f9d6-4da3-8007-71b21c967bab
📒 Files selected for processing (8)
.mcp.jsonAGENTS.mdREADME.mdpackage.jsonskills/innerscape/SKILL.mdskills/innerscape/agents/openai.yamltools/innerscape-cli.mjstools/innerscape-mcp.mjs
1 participant
Summary
Verification
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit
Release Notes
New Features
Documentation