feat: thin Pi adapter mounting ECC's canonical skills and commands - #2759
Conversation
Adds first-class Pi (@earendil-works/pi-coding-agent) support as a thin adapter layer, following the maintainer review on affaan-m#2352. ECC's canonical assets stay the single source of truth: nothing is copied or generated under .pi/. The `pi` manifest in package.json points Pi directly at `skills/` and `commands/`. No transformation is needed — ECC's SKILL.md files already follow the Agent Skills standard Pi implements, and ECC's command frontmatter is already Pi's prompt-template format. .pi/extensions/index.ts is the only adapter logic. It: - uses Pi's documented `pi.on(...)` lifecycle, not an undocumented event bus - resolves hook scripts from the installed package via `__dirname`, never `process.cwd()`, so global installs work from any project directory - runs hooks with `execFile(process.execPath, [...])` and no shell, so paths containing spaces or shell metacharacters are safe - invokes hooks through ECC's own `run-with-flags.js`, so `ECC_HOOK_PROFILE` and `ECC_DISABLED_HOOKS` keep gating hooks under Pi - runs hooks in the user's project directory so project detection stays correct, while resolving the scripts themselves package-relative - injects the SessionStart hook's `additionalContext` into the system prompt on the next `before_agent_start` - isolates hook failures behind a timeout and an output limit - registers `/ecc-doctor` for install diagnostics Registers `.pi` in the platform-configs install module and adds a Pi row to the harness adapter compliance matrix. Verified against Pi 0.84.1: a global `pi install` exposes 285 skills and 94 commands resolved from `skills/` and `commands/`, plus `/ecc-doctor`, with no generated copies. Scope deliberately excludes subagents, chains, approval gates, todos, profiles, and MCP; ECC works in Pi without any companion package.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a Pi extension adapter for ECC resources, session hooks, context injection, diagnostics, packaging, compliance tracking, and compatibility tests. ChangesPi adapter integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Pi
participant PiExtension
participant ECCHookRunner
participant AgentSystemPrompt
Pi->>PiExtension: start session
PiExtension->>ECCHookRunner: run SessionStart hook
ECCHookRunner-->>PiExtension: return hook output or warning
PiExtension->>AgentSystemPrompt: inject additionalContext and portable rules
Pi->>PiExtension: end session
PiExtension->>ECCHookRunner: run SessionEnd hook
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 @.pi/extensions/index.ts:
- Around line 190-199: Attach an error handler to child.stdin in the hook
execution flow, alongside the existing child error handler, and resolve the
operation with the same failure result when asynchronous stdin writes emit EPIPE
or another error. Keep the synchronous try/catch for immediate failures and
ensure stdin errors are handled without allowing an unhandled stream event to
terminate the session.
- Around line 289-303: Update isCompanionInstalled to resolve packageName from
Pi’s package roots, including the global ~/.pi/agent/npm and project .pi/npm
module roots, rather than relying only on the ECC extension context’s
require.resolve. Reuse Pi’s resolved package-path/module-resolution mechanism
and preserve the existing package.json-first fallback to the package entry
point, returning false only when neither Pi root resolves the companion.
In `@scripts/lib/harness-adapter-compliance.js`:
- Around line 136-149: Update the affected entries in the compliance data:
remove the trailing period from the first risk_notes string and replace the
internal semicolon in the second unsupported_surfaces string with punctuation
that does not conflict with the matrix renderer’s "; " join. Preserve the entry
meanings and all other records unchanged.
In `@tests/pi/pi-extension-adapter.test.js`:
- Around line 89-106: Update the tests around extractAdditionalContext and its
assertions to explicitly acknowledge the local implementation is a copy, then
add source-level assertions against the shipped adapter’s actual guard behavior,
including the startsWith("{") check and non-empty trimmed string validation.
Ensure the tests fail when .pi/extensions/index.ts diverges, rather than
validating only the copied function or merely checking field-name presence.
- Around line 309-340: Update the profile-gating test around disabledResult and
minimalResult to create and use an isolated buildEccSkeleton root for both
runHookRunner calls instead of repoRoot. Wrap the test setup and assertions in
try/finally, and remove the temporary skeletonRoot recursively in the finally
block.
In `@tests/pi/pi-package-manifest.test.js`:
- Around line 110-126: Harden both `.pi/` regression guards: update the
file-count test around “REGRESSION GUARD: git tracks fewer than 10 files under
.pi/” to recursively walk `.pi/` on disk using a Node 18-compatible manual
traversal, counting files regardless of Git tracking status. Also revise the
README heuristic in the nearby regression tests to recognize copy instructions
while allowing valid negated phrasing such as “no generated copies.”
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4638f045-10da-401c-bbf1-abbc31ea4610
📒 Files selected for processing (8)
.pi/README.md.pi/extensions/index.tsdocs/architecture/harness-adapter-compliance.mdmanifests/install-modules.jsonpackage.jsonscripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (17)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
manifests/install-modules.jsonpackage.jsonscripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
manifests/install-modules.jsonpackage.jsonscripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Delegate complex features to a planner agent, architectural decisions to an architect agent, modified code to a code-reviewer agent, and security-sensitive work to a security-reviewer agent; use parallel agents for independent operations.
Never compromise security; validate all inputs and prevent hardcoded secrets, injection, XSS, CSRF, authentication or authorization failures, sensitive error leakage, and missing rate limits.
Never hardcode secrets; use environment variables or a secret manager, validate required secrets at startup, and rotate exposed secrets immediately.
Always create new objects and never mutate existing ones.
Plan complex features before implementation, identifying dependencies, risks, and phases.
Prefer many small, focused files; keep functions under 50 lines, files under 800 lines where practical, avoid nesting deeper than four levels, and use readable, well-named identifiers.
Handle errors at every level, provide user-friendly UI messages, log detailed server-side context, and never silently swallow errors.
Validate all user input at system boundaries using schema-based validation; fail fast with clear messages and never trust external data.
Required tests include unit tests, integration tests for APIs and database operations, and end-to-end tests for critical user flows.
Follow the mandatory TDD cycle: write a failing test, implement the minimum passing solution, then refactor and verify coverage.
Use a consistent API response envelope containing a success indicator, data payload, error message, and pagination metadata.
Encapsulate data access behind a repository interface with operations such asfindAll,findById,create,update, anddelete; business logic must depend on the abstraction rather than storage details.
Files:
manifests/install-modules.jsonpackage.jsonscripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jsdocs/architecture/harness-adapter-compliance.mdtests/pi/pi-package-manifest.test.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
package.jsonscripts/lib/harness-adapter-compliance.js
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/lib/harness-adapter-compliance.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/lib/harness-adapter-compliance.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
🪛 ast-grep (0.45.0)
tests/pi/pi-extension-adapter.test.js
[warning] 28-28: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: require("child_process")
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
[warning] 115-115: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(extensionPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
tests/pi/pi-package-manifest.test.js
[warning] 14-14: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: require("child_process")
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
[warning] 41-41: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(repoRoot, "package.json"), "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 153-153: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(planCommandPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 179-179: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(sampleSkillPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 201-201: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(readmePath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
.pi/extensions/index.ts
[warning] 322-322: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(ECC_ROOT, "package.json"), "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 23-23: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🔇 Additional comments (9)
docs/architecture/harness-adapter-compliance.md (1)
42-42: The rendered.;and duplicated-semicolon artifacts in this row come from the source record. I raised it onscripts/lib/harness-adapter-compliance.jslines 136-149. Regenerate this row withnpm run harness:adaptersafter that fix..pi/README.md (2)
22-23: LGTM!Also applies to: 87-88, 102-107, 143-148
53-61: 📐 Maintainability & Code QualityKeep the Pi settings example unchanged.
Pi supports the
skillsandpromptskeys and expands~in paths.> Likely an incorrect or invalid review comment.package.json (1)
53-53: LGTM!Also applies to: 475-485
manifests/install-modules.json (1)
116-116: LGTM!scripts/lib/harness-adapter-compliance.js (1)
125-135: LGTM!Also applies to: 150-157
.pi/extensions/index.ts (2)
103-142: LGTM!Also applies to: 207-287, 332-368, 435-447
377-413: 🎯 Functional CorrectnessNo change needed. Pi awaits
session_startduring runtime setup before it accepts the first prompt, sopendingContextis populated beforebefore_agent_start.> Likely an incorrect or invalid review comment.tests/pi/pi-extension-adapter.test.js (1)
43-52: LGTM!Also applies to: 59-87, 118-236, 240-307
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
daltino
left a comment
daltino
left a comment
There was a problem hiding this comment.
Thanks for working on this! The changes look neat and well-structured.
Bot review on affaan-m#2759 surfaced two real runtime defects and several hardening gaps. Runtime fixes: - Attach an `error` listener to the hook child's stdin. `stdin.end()` writes asynchronously, so a hook that exits, short-circuits, or is killed by the timeout before reading the payload raises EPIPE as an `error` event that the surrounding try/catch cannot see. Unhandled, that event would terminate the Pi session and break the isolation guarantee the adapter documents. - Clear `pendingContext` at the top of the `session_start` handler. Pi can start a new session (/new, /resume, /fork) before `before_agent_start` consumes the previous value; if the newer hook then failed, the next agent start received context describing a different session's project state. - Replace `require.resolve` companion detection with a read of Pi's own `packages` list, honoring `PI_CODING_AGENT_DIR`. Pi installs packages under its config directory, which is not on Node's module resolution path from the extension, so the previous check reported every companion as missing no matter what was installed. Compliance matrix: remove internal semicolons and a trailing period from the Pi record's list entries. The renderer joins entries with "; ", so those characters split one entry into several in the rendered cell. Tests: run profile gating against the temp skeleton instead of the real checkout so it cannot leave marker artifacts behind; count files under .pi/ by walking disk rather than git, so untracked copies cannot bypass the regression guard; allow negated phrasing in the README heuristic; pin the adapter's real parser guards with source assertions so the local mirrors cannot silently diverge; add coverage for EPIPE isolation, stale context clearing, and companion detection.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
All review findings addressed in Real defects (both confirmed, both fixed)
Found while verifying the review CodeRabbit's question about companion-package resolution led to a third real bug: Also fixed
Adapter tests went from 11 to 17. Full suite: 3,750 passed, 0 failed. Marked as draft while the remaining scope questions in the PR description are settled. |
…ng them Every capability listed as out of scope is already provided by a maintained community Pi package: pi-subagents, @juicesharp/rpiv-ask-user-question, @juicesharp/rpiv-todo, and pi-mcp-adapter for MCP. Pi supports pulling other pi packages in via dependencies plus bundledDependencies, but this adapter deliberately does not. Bundling would ship third-party code that executes with full user permissions in every ECC install, turn optional capabilities into mandatory ones, and add four fast-moving pins to maintain. Instead /ecc-doctor now prints the exact `pi install npm:<name>` command for each companion it does not find, so adopting one stays a deliberate user choice. Also corrects the MCP claim: Pi core has no MCP surface by design, but the community pi-mcp-adapter package adds one. This adapter neither installs nor verifies it, and ECC's MCP reference configs are not known to be compatible.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Tested rather than assumed. The community pi-mcp-adapter package reads the standard mcpServers format from .mcp.json and ~/.config/mcp/mcp.json, which is exactly the format ECC already uses in .mcp.json and mcp-configs/mcp-servers.json. Verified against pi-mcp-adapter 2.21.2 in an isolated PI_CODING_AGENT_DIR: copying mcp-configs/mcp-servers.json to a project's .mcp.json registers Pi's `mcp` tool and `/mcp` command with all 35 ECC servers discovered, coexisting with this adapter's /ecc-doctor. No translation layer and no ECC change are needed, so this stops being a limitation and becomes documentation. Recorded caveats: the adapter's first run against a new config performs initialization that blocks in non-interactive mode, and only discovery was verified, not live tool invocation. ECC still neither installs nor depends on the package.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
Maintainer routing note: #2270 is prior groundwork for the structured tool-result envelope proposed around ECC’s Pi integration. I closed that older adapter to keep one active integration path here because its runtime and policy layer had drifted substantially. As this thin adapter stabilizes, please preserve credit to #2270 for that envelope idea and keep any generic tool port as a separate, focused follow-up. |
ECC's rules were the one durable asset the adapter did not deliver: skills and commands reached Pi in full, but the 122 rule files that carry ECC's coding style, testing, security, git workflow, and code-review standards did not, so ECC in Pi was a library of skills rather than a set of enforced standards. Rules are read at runtime from the canonical rules/common/ directory of the installed package and appended to the system prompt inside an <ecc-engineering-rules> block. Nothing is copied or generated under .pi/, which keeps the single-source-of-truth constraint this PR exists to satisfy. Injection reuses the before_agent_start path already built for session context, so no new lifecycle mapping is introduced. Rules are re-applied every turn because they are standing policy, while the session context stays one-shot and is consumed on first use. agents.md, hooks.md, and performance.md are excluded: they describe Claude Code primitives Pi does not have (Task/TodoWrite delegation, Claude hook event types, thinking-budget toggles), so injecting them would point the model at tools that are not there. A test asserts they stay excluded, and a leakage test asserts none of those primitives appear in the injected text. Language-specific rules under rules/<language>/ are out of scope for this first adapter. Injection is bounded by MAX_RULES_BYTES and can be disabled with ECC_PI_RULES, following ECC's existing off-switch convention. /ecc-doctor reports the state and injected size. Measured on this repo: 7 files, 12,361 characters, roughly 3k tokens. Also replaces a Function() call in the test helper with direct arithmetic, and repins a stale assertion that pinned one spelling of the context handoff rather than the guarantee (read before clear, clear before return).
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
Understood on all three points, and thank you for the routing note. No tool port here. Confirming concretely, since that is the part most at risk of scope creep: this adapter registers zero tools (no Because nothing here produces a tool result, there is no surface in this PR for the structured envelope to attach to — which is why keeping it out is natural rather than a concession. Credit preserved. The description now records #2270 by @SiaoZeng as prior groundwork for the structured tool-result envelope, and states that if a Pi tool port lands later it will be a separate PR crediting #2270 for the envelope design. Scope held. Following the same rule already applied elsewhere in this PR — agent conversion and chains stay out because they would mean generating 67 files under For visibility, the one capability added since your July review beyond the minimal set is rule injection: ECC's portable |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/pi/pi-package-manifest.test.js (1)
215-218: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate all published skill manifests.
The packed artifact omits
skills/skill-comply/SKILL.md. Assert that every checkoutskills/**/SKILL.mdis present innpm pack --dry-run --json.🤖 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 `@tests/pi/pi-package-manifest.test.js` around lines 215 - 218, Extend the package manifest assertion around the existing files.some check to enumerate every checkout skills/**/SKILL.md file and assert each appears in the npm pack dry-run file list, including skills/skill-comply/SKILL.md. Preserve the existing requirement that the package contains at least one skills/ entry.
🤖 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 @.pi/extensions/index.ts:
- Around line 471-482: Update the rule-loading status flow around
loadPortableRules and describeRulesStatus to track the number of rule files
successfully loaded, excluding read failures, empty files, and files skipped
after MAX_RULES_BYTES is reached. Report that tracked count instead of
PORTABLE_RULE_FILES.length, and reset cachedRuleFileCount to zero when
ECC_PI_RULES disables the rules.
- Around line 378-425: Update normalizePiPackageName and its use in
listInstalledPiPackages to support object-form package entries, extracting and
normalizing the package name from the entry’s source field (including
npm-prefixed sources). Preserve existing handling for string entries and ensure
these packages are included in the installed-name set.
In @.pi/README.md:
- Around line 121-142: Update the Notes bullet that says MCP servers are out of
scope so it references the new MCP section and its pi-mcp-adapter guidance
instead. Remove the conflicting direction to use ECC MCP configs only with other
harnesses, while preserving the remaining Notes content.
---
Outside diff comments:
In `@tests/pi/pi-package-manifest.test.js`:
- Around line 215-218: Extend the package manifest assertion around the existing
files.some check to enumerate every checkout skills/**/SKILL.md file and assert
each appears in the npm pack dry-run file list, including
skills/skill-comply/SKILL.md. Preserve the existing requirement that the package
contains at least one skills/ entry.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d0a0da7f-4492-412f-a0da-60061df15c99
📒 Files selected for processing (6)
.pi/README.md.pi/extensions/index.tsdocs/architecture/harness-adapter-compliance.mdscripts/lib/harness-adapter-compliance.jstests/pi/pi-extension-adapter.test.jstests/pi/pi-package-manifest.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (17)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Delegate complex, domain-specific, architectural, security-sensitive, review, build, and testing tasks to the appropriate specialized agent; use parallel execution for independent operations.
For new features and bug fixes, follow TDD: write a failing test first, implement the minimum solution, then refactor and verify coverage.
Maintain at least 80% test coverage and provide unit, integration, and end-to-end tests, including critical user flows.
Never compromise security: validate all inputs, prevent SQL injection with parameterized queries, sanitize HTML against XSS, enable CSRF protection, verify authentication and authorization, rate-limit endpoints, and avoid leaking sensitive data in errors.
Never hardcode API keys, passwords, tokens, or other secrets; use environment variables or a secret manager, validate required secrets at startup, and rotate exposed secrets immediately.
If a security issue is found, stop, use the security-reviewer agent, fix critical issues, rotate exposed secrets, and review the codebase for similar issues.
Always create new objects and return new copies with changes applied; never mutate existing objects.
Organize code into many small, focused files by feature or domain rather than by type; target 200–400 lines and keep files below 800 lines where practical.
Handle errors at every level, show user-friendly messages in UI code, log detailed context server-side, and never silently swallow errors.
Validate all external and user input at system boundaries using schema-based validation; fail fast with clear messages and never trust external data.
Keep functions under 50 lines, files focused and under 800 lines, avoid nesting deeper than four levels, avoid hardcoded values, and use readable, well-named identifiers.
Plan complex features before implementation, identifying dependencies and risks and breaking work into phases.
After modifying code, run code review immediately and address critical and high-severity issues.
Store personal ...
Files:
docs/architecture/harness-adapter-compliance.mdscripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
{package.json,*.config.js,scripts/**/*.js}
📄 CodeRabbit inference engine (CLAUDE.md)
Package manager detection should support npm, pnpm, yarn, and bun, with configuration via CLAUDE_PACKAGE_MANAGER environment variable or project config.
Files:
scripts/lib/harness-adapter-compliance.js
scripts/**/*.js
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure cross-platform support for Windows, macOS, and Linux via Node.js scripts in the scripts/ directory.
Files:
scripts/lib/harness-adapter-compliance.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
scripts/lib/harness-adapter-compliance.jstests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
{scripts,bin}/**
⚙️ CodeRabbit configuration file
{scripts,bin}/**: Focus on command injection, unsafe subprocess usage, path traversal, SSRF, secret exposure, and missing tests for new CLI behavior.
Files:
scripts/lib/harness-adapter-compliance.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/pi/pi-package-manifest.test.jstests/pi/pi-extension-adapter.test.js
🪛 ast-grep (0.45.1)
tests/pi/pi-package-manifest.test.js
[error] 42-42: An archive entry path (e.g. entry.path / entry.fileName / header.name) is joined to an output directory without validating that the resolved path stays inside that directory. A malicious archive can use "../" sequences to escape the extraction directory and overwrite arbitrary files (Zip Slip). Resolve the path and verify it starts with the normalized output directory, or strip traversal with path.basename, before writing the entry.
Context: path.join(dir, entry.name)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(zip-slip-archive-extraction-javascript)
.pi/extensions/index.ts
[warning] 356-356: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(ECC_ROOT, "rules", "common", file), "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 408-408: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(file, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
tests/pi/pi-extension-adapter.test.js
[warning] 46-46: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: require("child_process")
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
[warning] 169-169: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(settingsFile, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 255-255: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(rootDir, "rules", "common", file), "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 937-947: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(
settingsFile,
JSON.stringify({
packages: [
"npm:pi-subagents@2.0.0",
"npm:@juicesharp/rpiv-todo@1.4.2",
"/Users/example/local-pi-plugin",
"git:https://github.com/example/pi-plugin.git",
],
})
)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 981-981: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(malformedFile, "{ this is not valid json")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🪛 LanguageTool
.pi/README.md
[style] ~84-~84: Try using a descriptive adverb here.
Context: ....md, and performance.md` are excluded on purpose: they describe Claude Code primitive...
(ON_PURPOSE_DELIBERATELY)
[style] ~125-~125: Consider an alternative for the overused word “exactly”.
Context: ...and ~/.config/mcp/mcp.json — which is exactly the format ECC already uses in `.mcp.js...
(EXACTLY_PRECISELY)
🔇 Additional comments (21)
docs/architecture/harness-adapter-compliance.md (1)
42-42: LGTM!scripts/lib/harness-adapter-compliance.js (2)
133-140: LGTM!
147-150: LGTM!.pi/README.md (3)
16-26: LGTM!
81-97: LGTM!
102-119: LGTM!.pi/extensions/index.ts (6)
112-136: LGTM!
220-236: LGTM!
327-349: LGTM!
350-376: LGTM!
427-442: LGTM!
549-553: LGTM!Also applies to: 571-589
tests/pi/pi-extension-adapter.test.js (9)
23-59: LGTM!
108-184: LGTM!
186-234: LGTM!
236-284: LGTM!
486-540: LGTM!
542-628: LGTM!
632-746: LGTM!
748-991: LGTM!
995-1253: LGTM!
Two reporting defects in /ecc-doctor, the command whose whole job is telling
a user what is actually installed.
Pi's settings accept a `packages` entry in two shapes: the bare source string
("npm:pi-subagents") and an object carrying that source alongside resource
filters ({ source: "npm:pi-subagents", skills: [] }). normalizePiPackageName
only recognized the string, so a user who narrowed which resources a companion
contributes was told the companion was not installed, along with an install
command for something already present. The source type still decides whether a
name is comparable, so an object wrapping a git source or a path stays
unrecognized exactly as before.
loadPortableRules drops rule files it cannot read, drops empty ones, and stops
at MAX_RULES_BYTES, but describeRulesStatus reported PORTABLE_RULE_FILES.length
regardless. A partial install that loaded 3 of 7 files reported "7 rule file(s)"
to the one command a user runs to find a partial install. The loaded count is
now tracked next to the cache and reported as a ratio, with the shortfall named.
Also reconciles the Notes bullet in .pi/README.md, which still called MCP out of
scope after the MCP section landed documenting that ECC's configs load in Pi
through pi-mcp-adapter.
Both defects were reported by CodeRabbit and verified against Pi's own
packages.md before fixing. Adapter tests go from 24 to 26; the two source
contracts that pinned the previous spellings now pin the new guards, so the
object-form unwrapping and the loaded-count reporting cannot be silently
reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
All three findings from the latest review addressed in
Tests Adapter tests go from 24 to 26. Both new ones are behavioral, and the two source contracts that pinned the previous spellings now pin the new guards — including the Full suite: 3,759 passed, 0 failed. No other actionable items in the review; the remaining entries are the inference-engine notes. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/pi/pi-extension-adapter.test.js`:
- Around line 1186-1230: Split tests/pi/pi-extension-adapter.test.js into
focused test files by moving the companion-package and rule-diagnostics groups
out of its oversized main function. Extract shared setup and assertion helpers
for reuse, ensuring each resulting file is under 800 lines and every function is
under 50 lines while preserving the existing test behavior.
- Around line 946-983: Refactor the new normalizePiPackageName test cases into
Arrange/Act/Assert structure: define each input in an Arrange step, assign the
normalizePiPackageName result in a separate Act step, and assert that result
afterward. Preserve the existing descriptive test name and expected values for
all object, invalid-source, missing-source, and null cases.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dfce9ad7-b179-42ff-ac7f-13840dc623b1
📒 Files selected for processing (3)
.pi/README.md.pi/extensions/index.tstests/pi/pi-extension-adapter.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (14)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}
📄 CodeRabbit inference engine (.cursor/rules/common-coding-style.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php,scala,kt}: Always create new objects, never mutate existing ones. Use immutable patterns to prevent hidden side effects and enable safe concurrency
Organize code into many small files (200-400 lines typical, 800 lines max) organized by feature/domain rather than by type
Always handle errors explicitly at every level and never silently swallow errors
Always validate all user input before processing at system boundaries
Use schema-based validation where available
Fail fast with clear error messages when validation fails
Never trust external data (API responses, user input, file content)
Ensure code is readable and well-named
Keep functions small (less than 50 lines)
Keep files focused (less than 800 lines)
Avoid deep nesting (more than 4 levels)
Do not use hardcoded values; use constants or configuration instead
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
No hardcoded secrets (API keys, passwords, tokens) - validate before any commit
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php}: All user inputs must be validated
Enable CSRF protection on all state-changing endpoints
Verify authentication and authorization for all protected endpoints
Implement rate limiting on all endpoints to prevent abuse
Ensure error messages do not leak sensitive data in responses
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,sql}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Use parameterized queries to prevent SQL injection
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,html,php,java,cs,rb,go}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
Implement XSS prevention by sanitizing HTML output
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,py,java,cs,rb,go,php,swift,kt,rs,c,cpp,h,hpp,properties,yml,yaml,json,env,config}
📄 CodeRabbit inference engine (.cursor/rules/common-security.md)
NEVER hardcode secrets in source code - ALWAYS use environment variables or a secret manager
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript-coding-style.md)
**/*.{ts,tsx,js,jsx}: Use spread operator for immutable updates in TypeScript/JavaScript instead of direct mutation
Use async/await with try-catch for error handling in TypeScript/JavaScript
Use Zod for schema-based input validation in TypeScript/JavaScript
No console.log statements in production code; use proper logging libraries instead
**/*.{ts,tsx,js,jsx}: Auto-format JavaScript/TypeScript files using Prettier after edit
Warn aboutconsole.logstatements in edited files
Check all modified files forconsole.logstatements before session ends
**/*.{ts,tsx,js,jsx}: Use the ApiResponse interface pattern with generic type parameter:interface ApiResponse<T> { success: boolean; data?: T; error?: string; meta?: { total: number; page: number; limit: number; } }
Implement custom React hooks following the pattern: export a named function with use prefix, generic type parameters, and proper useEffect cleanup for side effects
**/*.{ts,tsx,js,jsx}: Never hardcode secrets; always use environment variables for sensitive credentials like API keys
Throw an error when required environment variables are not configured to fail fast and ensure security prerequisites are metUse Playwright as the E2E testing framework for critical user flows in TypeScript/JavaScript
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{test,spec}.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,ts,jsx,tsx}: Write tests before implementation (test-driven development); target 80%+ coverage
Achieve minimum 80% test coverage across all three layers: Unit, Integration, and E2E
Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts,jsx,tsx}: Always create new objects and never mutate in place; return new copies instead
Keep files between 200–400 lines typical, with a maximum of 800 lines
Extract helpers when a file exceeds 200 lines
Handle errors explicitly at every level; never swallow errors silently
Validate all user input before processing; use schema-based validation where available
Never trust external data (API responses, file content, query params); always validate
All user inputs must be validated and sanitized
Error messages must be scrubbed of sensitive internals
Use readable, well-named identifiers in all code
Keep functions under 50 lines
Keep files under 800 lines
Avoid nesting deeper than 4 levels
Implement comprehensive error handling in all code
Do not hardcode values; use constants or environment configuration instead
Do not use in-place mutation; always return new objects or state
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{js,ts,jsx,tsx,json,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not hardcode secrets, API keys, passwords, or tokens
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use parameterized queries for all database writes (no string interpolation)
Auth/authz must be checked server-side for every sensitive path
Rate limiting must be applied to all public endpoints
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
HTML output must be sanitized where applicable
Files:
tests/pi/pi-extension-adapter.test.js
**/*.{js,ts,env*}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Required environment variables must be validated at startup
Files:
tests/pi/pi-extension-adapter.test.js
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Delegate complex, domain-specific, architectural, security-sensitive, review, build, and testing tasks to the appropriate specialized agent; use parallel execution for independent operations.
For new features and bug fixes, follow TDD: write a failing test first, implement the minimum solution, then refactor and verify coverage.
Maintain at least 80% test coverage and provide unit, integration, and end-to-end tests, including critical user flows.
Never compromise security: validate all inputs, prevent SQL injection with parameterized queries, sanitize HTML against XSS, enable CSRF protection, verify authentication and authorization, rate-limit endpoints, and avoid leaking sensitive data in errors.
Never hardcode API keys, passwords, tokens, or other secrets; use environment variables or a secret manager, validate required secrets at startup, and rotate exposed secrets immediately.
If a security issue is found, stop, use the security-reviewer agent, fix critical issues, rotate exposed secrets, and review the codebase for similar issues.
Always create new objects and return new copies with changes applied; never mutate existing objects.
Organize code into many small, focused files by feature or domain rather than by type; target 200–400 lines and keep files below 800 lines where practical.
Handle errors at every level, show user-friendly messages in UI code, log detailed context server-side, and never silently swallow errors.
Validate all external and user input at system boundaries using schema-based validation; fail fast with clear messages and never trust external data.
Keep functions under 50 lines, files focused and under 800 lines, avoid nesting deeper than four levels, avoid hardcoded values, and use readable, well-named identifiers.
Plan complex features before implementation, identifying dependencies and risks and breaking work into phases.
After modifying code, run code review immediately and address critical and high-severity issues.
Store personal ...
Files:
tests/pi/pi-extension-adapter.test.js
🔇 Additional comments (2)
.pi/README.md (1)
189-189: LGTM!.pi/extensions/index.ts (1)
334-342: LGTM!Also applies to: 358-358, 386-386, 442-460, 500-502
| ["companion package name normalization (behavioral mirror): an object entry with resource filters resolves to the same name as the bare source string", () => { | ||
| assert.strictEqual( | ||
| normalizePiPackageName({ source: "npm:pi-subagents", skills: [] }), | ||
| "pi-subagents", | ||
| "expected the object form Pi documents for filtered packages to resolve to the " + | ||
| "same name as the bare string; a user who narrows which resources pi-subagents " + | ||
| "contributes still has it installed, and /ecc-doctor exists to report exactly that" | ||
| ) | ||
| assert.strictEqual( | ||
| normalizePiPackageName({ source: "npm:@juicesharp/rpiv-todo@1.4.2", prompts: ["prompts/review.md"] }), | ||
| "@juicesharp/rpiv-todo", | ||
| "expected an object entry to go through the same version-stripping path as a " + | ||
| "string entry, scope intact" | ||
| ) | ||
| assert.strictEqual( | ||
| normalizePiPackageName({ source: "git:github.com/example/pi-plugin@v1" }), | ||
| undefined, | ||
| "expected an object entry wrapping a git source to stay unrecognized; the source " + | ||
| "type decides, not the entry shape" | ||
| ) | ||
| assert.strictEqual( | ||
| normalizePiPackageName({ extensions: ["extensions/*.ts"] }), | ||
| undefined, | ||
| "expected an object entry with no source field to normalize to undefined instead " + | ||
| "of throwing" | ||
| ) | ||
| assert.strictEqual( | ||
| normalizePiPackageName({ source: 42 }), | ||
| undefined, | ||
| "expected a non-string source to normalize to undefined instead of throwing" | ||
| ) | ||
| assert.strictEqual( | ||
| normalizePiPackageName(null), | ||
| undefined, | ||
| "expected a null entry to normalize to undefined; typeof null is \"object\", so " + | ||
| "this is the case an unguarded object branch would throw on" | ||
| ) | ||
| }], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use Arrange / Act / Assert for these new test cases.
Assign each normalizePiPackageName(...) result in an Act step. Assert that value in a separate Assert step. This makes each expected behavior clear when an assertion fails.
As per coding guidelines, "Use AAA structure (Arrange / Act / Assert) in tests with descriptive test names that explain behavior under test."
🤖 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 `@tests/pi/pi-extension-adapter.test.js` around lines 946 - 983, Refactor the
new normalizePiPackageName test cases into Arrange/Act/Assert structure: define
each input in an Arrange step, assign the normalizePiPackageName result in a
separate Act step, and assert that result afterward. Preserve the existing
descriptive test name and expected values for all object, invalid-source,
missing-source, and null cases.
Source: Coding guidelines
| ["/ecc-doctor reports rule files actually loaded, not the allowlist length (source contract)", () => { | ||
| assert.ok( | ||
| /let\s+cachedRuleFileCount\s*=\s*0/.test(extensionSource), | ||
| "expected .pi/extensions/index.ts to track how many rule files actually loaded in a " + | ||
| "cachedRuleFileCount counter alongside cachedRules" | ||
| ) | ||
| assert.ok( | ||
| /cachedRuleFileCount\s*=\s*sections\.length/.test(extensionSource), | ||
| "expected loadPortableRules in .pi/extensions/index.ts to set cachedRuleFileCount " + | ||
| "from sections.length, which is what survived the read failures, the empty-file " + | ||
| "skip, and the MAX_RULES_BYTES break" | ||
| ) | ||
|
|
||
| const disabledBranch = extensionSource.slice( | ||
| extensionSource.indexOf("isDisabledByEnv(process.env.ECC_PI_RULES)"), | ||
| extensionSource.indexOf("const sections: string[] = []") | ||
| ) | ||
| assert.ok( | ||
| /cachedRuleFileCount\s*=\s*0/.test(disabledBranch), | ||
| "expected the ECC_PI_RULES disable branch of loadPortableRules in " + | ||
| ".pi/extensions/index.ts to reset cachedRuleFileCount to 0, so the counter can " + | ||
| "never survive from a prior load into a disabled session" | ||
| ) | ||
|
|
||
| const statusStart = extensionSource.indexOf("function describeRulesStatus") | ||
| assert.ok( | ||
| statusStart !== -1, | ||
| "expected .pi/extensions/index.ts to define a function named describeRulesStatus" | ||
| ) | ||
| const nextFunctionStart = extensionSource.indexOf("\nfunction ", statusStart + 1) | ||
| const statusSource = | ||
| nextFunctionStart === -1 | ||
| ? extensionSource.slice(statusStart) | ||
| : extensionSource.slice(statusStart, nextFunctionStart) | ||
|
|
||
| assert.ok( | ||
| /\$\{cachedRuleFileCount\}\/\$\{PORTABLE_RULE_FILES\.length\}\s+rule file/.test(statusSource), | ||
| "expected describeRulesStatus in .pi/extensions/index.ts to report the loaded count " + | ||
| "over the allowlist length (`${cachedRuleFileCount}/${PORTABLE_RULE_FILES.length} " + | ||
| "rule file(s)`); loadPortableRules silently skips unreadable and empty files and " + | ||
| "breaks out of the loop at MAX_RULES_BYTES, so reporting the allowlist length " + | ||
| "alone makes an install that loaded 3 of 7 report 7 -- and /ecc-doctor is the one " + | ||
| "place a user looks to find a partial install" | ||
| ) | ||
| }], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Split this test script before adding more test groups.
tests/pi/pi-extension-adapter.test.js is 1,350 lines. Its main function spans more than 1,000 lines. Move the companion-package and rule-diagnostics groups into focused test files. Extract shared helpers so each resulting file stays below 800 lines and each function stays below 50 lines.
As per coding guidelines, "Keep functions small (less than 50 lines)" and "Keep files focused (less than 800 lines)."
🤖 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 `@tests/pi/pi-extension-adapter.test.js` around lines 1186 - 1230, Split
tests/pi/pi-extension-adapter.test.js into focused test files by moving the
companion-package and rule-diagnostics groups out of its oversized main
function. Extract shared setup and assertion helpers for reuse, ensuring each
resulting file is under 800 lines and every function is under 50 lines while
preserving the existing test behavior.
Source: Coding guidelines
|
think this is good to merge in. nice work! |
4 participants
What Changed
Adds first-class Pi (
@earendil-works/pi-coding-agent) support to ECC as a thin adapter, following the maintainer review on #2352.8 files, +1,289 lines, 0 deletions. For comparison, #2352 is 440 files, +99,628 / −2,785, which exceeded both Greptile's and CodeRabbit's review limits.
pimanifest points Pi at./skillsand./commands. Nothing is copied or generated under.pi/. Two regression tests fail the build if.pi/skills,.pi/agents,.pi/prompts,.pi/chainsreappear, or if more than 10 files are tracked under.pi/export default function (pi: ExtensionAPI)withpi.on("session_start" | "session_shutdown" | "before_agent_start"). Noapp.eventspath.resolve(__dirname, "..", "..")for resolution;execFile(process.execPath, [...])for execution.process.cwd()appears nowhere in the fileextensions,skills,prompts. Noagents/chainskeys — those belong topi-subagentsand would be silently ignored by Pi's core manifest. A test asserts they stay absent/ecc-doctor, and two lifecycle mappings. Profiles, approval UI, todos, agent conversion, and chains are explicitly out of scopeNo transformation needed
ECC's assets are already Pi-compatible, which is what makes the copy-free approach work:
skills/*/SKILL.mdalready follows the Agent Skills standard that Pi implementscommands/*.mdfrontmatter (description,argument-hint) is already Pi's prompt-template formatRules are the exception — Pi has no rules concept — so the adapter reads them from the canonical
rules/common/directory at runtime and appends them to the system prompt inside an<ecc-engineering-rules>block, reusing the injection path already built for session context. Still no copies: nothing is generated under.pi/.The adapter
.pi/extensions/index.tsis the only logic added. Beyond the review points above it:scripts/hooks/run-with-flags.js, soECC_HOOK_PROFILEandECC_DISABLED_HOOKSkeep gating hooks under Pi rather than being bypassedhookSpecificOutput.additionalContextfrom stdout, injecting it into the system prompt on the nextbefore_agent_startinside an<ecc-session-context>block. Non-JSON output (a profile-disabled hook passing stdin through) is tolerated, not treated as an errorAlso registers
.piin theplatform-configsinstall module and adds a Pi row to the harness adapter compliance matrix.Testing Done
Tested against Pi 0.84.1, Node 24.18.1, macOS 15 (Darwin 25.5.0). No companion packages installed — the integration was exercised without
pi-subagents,@juicesharp/rpiv-ask-user-question, or@juicesharp/rpiv-todopresent.Global install, isolated config directory
Installed into a clean
PI_CODING_AGENT_DIRso the results could not be contaminated by pre-existing user settings, then inspected the loaded resources throughpi.getCommands(), filtering to entries whosesourceInfo.pathis inside the ECC package:Repo ground truth:
ls skills | wc -l= 285,ls commands/*.md | wc -l= 94. Exact match, and the resolved paths are the canonical directories — not.pi/./ecc-doctorresolved to.pi/extensions/index.tsfrom the package.Lifecycle and context injection, from a path containing a space
Ran Pi from a working directory whose path contains a space, with a probe extension asserting on the assembled system prompt:
projectDiris the user's project, confirming hooks execute there while resolving package-relative. An earlier iteration passedcwdas the ECC root and reported ECC's own directory as the project; that was caught by this test and fixed.Automated
All 8 CI validators pass, including
validate-install-manifests.jsfor theinstall-modules.jsonchange.Test coverage includes: package manifest shape, the
.pi/copy regression guards, canonical-asset Pi compatibility,__dirnamevsprocess.cwd()resolution, absence of shell invocation, documented-lifecycle usage, a global-install simulation from a package root whose path contains a space, hook resolution whencwdpoints elsewhere, profile/disable gating, andadditionalContextparsing including malformed and non-JSON input.Full suite:
node tests/run-all.jsis green on this branch — 3,757 passed, 0 failed.npm run lint(eslint + markdownlint) passes clean.One transient failure appeared mid-work and is fixed in this PR rather than worked around: adding
.pi/topackage.jsonfilesbroketests/scripts/npm-publish-surface.test.js, which validates the published surface against the install module graph. The fix was to register.piin theplatform-configsmodule inmanifests/install-modules.json, which is where a shipped harness directory belongs.Type of Change
feat:New featuredocs:Documentationtest:Testschore:Maintenance/toolingSecurity & Quality Checklist
Security notes specific to this adapter:
execFilewith an argv array — no shell, so paths with spaces or shell metacharacters cannot be reinterpreted as commands.scripts/hooks/*.js./ecc-doctorprints paths, counts, and profile names only — no credentials or environment secrets.Known Limitations
session_shutdownmaps to ECC'ssession:end:markerhook rather thansession-end.js: the latter expects a Claude Code transcript, and Pi's session JSONL is a different format. Feeding it one would fabricate a compatibility that does not exist.What Reaches Pi
skills/, via thepimanifestcommands/, via thepimanifestrules/common/)pi-mcp-adapter— see belowsession:startandsession:end:marker, through ECC's ownrun-with-flags.jspi-subagentscompanion packageThe last two rows are deliberate. Converting 67 agents would mean generating 67 files under
.pi/, which is the pattern this PR exists to avoid, and the review asked for agent conversion and chains to follow independently. The remaining 20 hooks are ClaudePreToolUse/PostToolUse/Stopevents; Pi has equivalents (tool_call,tool_result,agent_settled), so they are mappable, but each carries its own semantics and belongs in a separate change.agents.md,hooks.md, andperformance.mdare excluded from rule injection on purpose: they describe Claude Code primitives Pi does not have, so injecting them would point the model at tools that are not there. Tests assert both that they stay excluded and that none of those primitives appear in the injected text.MCP needs nothing from ECC
pi-mcp-adapterreads the standardmcpServersformat from.mcp.json— exactly what ECC already uses. Verified against version 2.21.2 in an isolatedPI_CODING_AGENT_DIR: copyingmcp-configs/mcp-servers.jsonto a project's.mcp.jsonregisters Pi'smcptool and/mcpcommand with all 35 ECC servers discovered, coexisting with/ecc-doctor. No translation layer, no ECC change. ECC neither installs nor depends on that package;/ecc-doctorreports which optional companions are present and prints the exactpi installcommand for the ones that are not.Review Response
Automated review flagged two real runtime defects, both fixed in
2597544f:stdin.end()writes asynchronously, so a hook that exits, short-circuits, or is killed by the timeout before reading its payload raisesEPIPEas anerrorevent that the surroundingtry/catchcannot observe. Unhandled, it would take the Pi session down — exactly the guarantee the adapter claims to provide. Now handled, with both a source contract test and a behavioral test that writes 2 MB to a child which exits without reading.pendingContextlives betweensession_startandbefore_agent_start. Pi can begin a new session (/new,/resume,/fork) before that value is consumed; if the newer hook then failed, the next agent start received context describing a different session's project state. Greptile reproduced this against a real failing hook.pendingContextis now cleared at the top of the handler, before the hook runs.A third issue was found while verifying the review: companion-package detection used
require.resolve, which cannot see packages Pi installs under its own config directory (~/.pi/agent/npm, overridable viaPI_CODING_AGENT_DIR) because that path is not on Node's module resolution path from the extension./ecc-doctorwould have reported every companion as missing regardless of what was installed. It now reads Pi's ownpackageslist, correctly handling scoped names with versions.Also addressed: the compliance matrix renderer joins list entries with
"; ", so internal semicolons and trailing periods in the Pi record split one entry into several in the rendered cell; the profile-gating test ran against the real checkout and could leave marker artifacts behind, and now uses the isolated skeleton; the.pi/file-count guard counted tracked files only, so untracked copies could bypass it, and now walks disk; the README heuristic rejected valid negated phrasing; and the local parser mirrors in the tests are now pinned by source assertions so they cannot silently diverge from the adapter.Credit
This builds on the groundwork and problem framing in #2352 by @juicesharp. The Pi integration surface, the companion-package landscape, and the installation model were mapped there first; this PR reimplements that scope as an adapter along the lines the maintainer review requested.
#2270 by @SiaoZeng is the prior groundwork for the structured tool-result envelope discussed around ECC's harness integrations. This adapter registers no tools — it exposes one command,
/ecc-doctor, and noregisterToolcall — so there is no tool-result surface here for that envelope to apply to. If a Pi tool port happens later it will be a separate, focused PR, and it will credit #2270 for the envelope design.