Initial spec design#1
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds proposal, design, specs, tasks, and OpenSpec metadata for a cross‑harness plugin installer: shared core plus per‑marketplace wrappers, OAuth auth, skill install/uninstall/linking, MCP configuration, uninstall tracking, error‑handling, doctor checks, and a 10‑phase implementation roadmap. ChangesCross-Harness Plugin Installer Specification
Sequence DiagramsequenceDiagram
participant Marketplace
participant Installer
participant Auth
participant Skills
participant MCP
participant Storage
Marketplace->>Installer: postinstall hook
Installer->>Auth: ensureAuthenticated()
alt No stored credentials
Auth->>Storage: loadCredentials() -> null
Auth->>Auth: start local OAuth callback server
Auth->>Marketplace: open browser to OAuth provider
Marketplace->>Auth: OAuth callback (token)
Auth->>Storage: save credentials
else Credentials present & valid
Auth->>Storage: loadCredentials() -> credentials
end
Installer->>Skills: installSkills(skills, source)
Skills->>Storage: copy skills/create harness links
Installer->>MCP: configureMcpServers(harness, servers)
MCP->>Storage: merge & write MCP config
Installer->>Storage: write tracking file
Installer->>Marketplace: return InstallResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 9
🧹 Nitpick comments (4)
openspec/schemas/ns-workflow/schema.yaml (1)
21-21: 💤 Low valueConsider clarifying the template-to-files relationship.
The specs artifact generates
specs/**/*.md(multiple files) but references a singlespecs.mdtemplate. If the template is meant to be replicated or adapted for each spec file, this might be fine, but the singular/plural mismatch could be confusing.🤖 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 `@openspec/schemas/ns-workflow/schema.yaml` at line 21, The YAML currently lists a single template ("template: specs.md") but the artifact produces multiple output files ("specs/**/*.md"); update the schema so the template-to-files relationship is explicit: either change "template: specs.md" to a plural/parameterized value indicating it is applied per-spec (e.g., a templating pattern or template directory) or document that the single template is replicated for each generated file, and ensure the descriptor near "template: specs.md" references the output pattern "specs/**/*.md" or the generator behavior (replicate per file) so readers understand that one template is used to produce multiple specs.openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md (1)
151-156: ⚡ Quick winConsider warning users about modified skills during uninstall.
Line 155 states that modified NodeSource skills are deleted without warning during uninstall. While the rationale ("user chose to uninstall") is reasonable, this could surprise users who have customized skills for their workflow.
Consider adding a pre-uninstall check that:
- Detects modified NodeSource skills (e.g., compare checksums)
- Lists modified files in the uninstall confirmation/output
- Gives users awareness of what they're about to lose
This would improve user experience without complicating the uninstall flow significantly.
🤖 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 `@openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md` around lines 151 - 156, Update the uninstall flow to detect and surface modified NodeSource skills before deleting them: implement a pre-uninstall check (e.g., add a function detectModifiedNodeSourceSkills or listModifiedFiles used by the uninstall handler) that compares stored checksums or repo/file state (computeChecksum or git-status equivalent) against current SKILL.md and related files, collect any modified items, and include that list in the uninstall confirmation/output so users see what will be removed and can confirm or abort the uninstall; ensure the uninstall routine still deletes tracked NodeSource skills if the user confirms.openspec/changes/cross-harness-plugin-installer/tasks.md (2)
68-74: ⚡ Quick winDocument behavior when all OAuth callback ports are occupied.
Task 7 specifies a fallback range (8765-8770), but doesn't address what happens if all 6 ports are occupied. This edge case should be covered in testing or error handling.
🧪 Suggested test case addition
Add to the testing section:
-- **Testing**: Unit tests for server startup, callback handling, timeout, and port conflicts. Use mock HTTP client to simulate callback. +- **Testing**: Unit tests for server startup, callback handling, timeout, and port conflicts. Use mock HTTP client to simulate callback. Test graceful failure when all ports (8765-8770) are occupied.🤖 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 `@openspec/changes/cross-harness-plugin-installer/tasks.md` around lines 68 - 74, Add explicit handling and tests for the case where all OAuth callback ports (8765–8770) are occupied: update the oauth-server.ts startup logic (the server start/port-selection routine and the function that returns credentials) to detect when no ports are available and throw or return a clear, typed error (e.g., PortUnavailableError) after attempting the full range; add unit tests that simulate all ports in use to assert the thrown error and that the 5-minute timeout behavior remains unchanged; also document this behavior in the Task 7 testing section so callers and CI know the expected failure mode when all ports are occupied.
256-293: ⚡ Quick winAdd task for test infrastructure setup.
Phase 8 introduces integration tests (Task 28) but doesn't explicitly set up the test infrastructure. Individual tasks mention unit tests, but there's no task for installing and configuring the test runner, assertion library, mocking framework, or coverage tooling.
🧪 Suggested new task
Insert before Task 28:
### Task 27.5: Set up test infrastructure - **Description**: Install and configure test framework (e.g., Jest, Vitest, or Mocha), assertion library, mocking utilities (e.g., sinon), and code coverage tooling (e.g., c8, nyc). Configure test scripts in root and core package.json. Set coverage thresholds (e.g., 80% line coverage). - **Depends on**: Task 2 - **Files**: - `package.json` (add test scripts) - `packages/core/package.json` (add test dependencies and scripts) - `jest.config.js` or `vitest.config.ts` - `.nycrc` or coverage config - **Testing**: Run `npm test` with a minimal passing test. Verify coverage report generation.🤖 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 `@openspec/changes/cross-harness-plugin-installer/tasks.md` around lines 256 - 293, The plan omits a dedicated test-infrastructure setup task; add a new Task "27.5: Set up test infrastructure" before Task 28 in openspec/changes/cross-harness-plugin-installer/tasks.md that describes installing/configuring the test runner (Jest/Vitest/Mocha), assertion/mocking libs (e.g., chai/sinon or built-ins), coverage tooling (c8/nyc) and adding test scripts and config files (package.json scripts, packages/core/package.json deps, jest.config.js or vitest.config.ts, .nycrc), and require a minimal verification step (npm test) and coverage threshold (e.g., 80%) so Task 28's tests have the necessary infra.
🤖 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 `@openspec/changes/cross-harness-plugin-installer/design.md`:
- Around line 194-268: The design doc lacks specification for variable
substitution, dependency validation, and tracking-file permissions; update the
bundle descriptor section to: describe the variable expansion mechanism for
placeholders (e.g., ${MCP_ROOT}, ${AUTH_TOKEN}, ${AUTH_ORG_ID}) including when
expansion occurs (install time vs runtime), the source precedence (env vars,
agent config, credential store ~/.agents/.nodesource-auth.json), and failure
behavior; document how skill MCP dependencies (the "requiresMcp" array on skills
such as requiresMcp in bundle.json) are validated during install (e.g., check
installed MCP list, call harness adapter hook like validateMcpAvailability or
installMcpServer, and fail/rollback on missing dependencies) and whether
automatic MCP installation is allowed; and specify file permissions for the
tracking file (~/.agents/.nodesource-installed.json) (recommend 0600 and atomic
write/owner-only ownership) so harness adapters implement consistent handling.
In `@openspec/changes/cross-harness-plugin-installer/proposal.md`:
- Around line 64-78: Update the proposal and auth docs to explicitly distinguish
credential behavior: in the Rollback Plan (section mentioning
~/.agents/.nodesource-auth.json and the per-harness uninstall hook that removes
tracking files and skill dirs) state that rollback is an emergency full cleanup
that deletes shared credentials (remove ~/.agents/.nodesource-auth.json),
whereas in installation-and-auth.md (the “credentials preserved (shared across
installs)” text) clarify normal uninstall preserves the shared credentials for
re-use across harnesses; ensure both files reference the same credential
filename (~/.agents/.nodesource-auth.json) and add a short note in the
per-harness uninstall/uninstall hook documentation that only rollback removes
credentials while normal uninstall retains them.
In
`@openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md`:
- Around line 142-149: Update the "Uninstall with missing tracking file"
scenario to explicitly define "best-effort cleanup" and list "known NodeSource
artifacts": enumerate exact file paths/patterns (e.g., /usr/local/bin/node*,
/usr/local/lib/node_modules/nodesource*,
/etc/apt/sources.list.d/nodesource.list, %PROGRAMFILES%/nodejs/* on Windows) and
any associated symlinks and service units; specify the algorithm the installer
uses when no tracking file is present (scan only these predefined patterns,
verify ownership or package manager metadata where available, skip files
modified within X days, and prompt or log each deletion); and state the policy
(conservative by default: remove only exact-match artifacts and leave ambiguous
files untouched unless a --force flag is provided) so implementers have a
deterministic, safe deletion plan tied to the "Uninstall with missing tracking
file" scenario and the terms "best-effort cleanup" and "known NodeSource
artifacts."
In `@openspec/changes/cross-harness-plugin-installer/tasks.md`:
- Around line 138-177: The Phase 5 tasks omit an Antigravity adapter; add a new
Task (insert as Task 20) that scaffolds
packages/core/src/harnesses/antigravity-adapter.ts (exported from
packages/core/src/harnesses/index.ts) with the harness-adapter interface
implemented but methods left as TODO/throw NotImplementedError, include a .TODO
notes file if desired, ensure the adapter type exports and compiles (type-check
only), and renumber subsequent tasks accordingly so all harness adapters are
listed in Phase 5.
- Around line 103-110: Task 11's tracker omits which harness(es) installed a
skill; update the spec and implementation so skill-tracker records harness
association(s). Modify packages/core/src/skills/skill-tracker.ts to include a
harnesss field (e.g., array of harness IDs/names) alongside skill name, path,
and timestamp, add methods to add/remove harness entries when
installing/uninstalling, and update read/write logic to handle missing/corrupted
files with harness-aware semantics; also export the updated API from
packages/core/src/skills/index.ts and add/adjust unit tests to verify
multi-harness installs, targeted uninstall behavior, and corrupted/missing
tracking-file handling.
- Around line 95-101: Clarify and implement an idempotent handling strategy in
packages/core/src/skills/skill-linker.ts (e.g., inside createSkillLinks and any
helper like linkSkillForHarness): when target path is a symlink that points to
the correct source, skip (no-op); when target is a symlink that is broken or
points elsewhere, remove and recreate the symlink; when target is a regular file
or directory, back it up by renaming with a .bak timestamp suffix then create
the new symlink (or copy for Pi harnesses); ensure operations are deterministic
and return clear status values (skipped/created/replaced/backed-up) for unit
tests and logging to satisfy idempotency requirements referenced by Task 33.
- Around line 31-38: Task 3 lacks a concrete path for the canonical bundle.json;
update the task to specify the full location (e.g., packages/core/bundle.json)
and ensure consistency across code and tests by updating the references in
packages/core/src/validate.ts, the unit test that loads the fixture, and any
documentation or file lists so they all point to that exact path (use the symbol
names bundle.json, validate.ts, and the test file name to locate updates).
- Around line 130-136: Add a "harness" association to the MCP tracker: update
the TrackedMcp interface in mcp-tracker.ts to include a harness:string field and
ensure all write/read functions (e.g., addTrackedMcp, removeTrackedMcp,
readMcpTrackingFile, listTrackedMcps) persist and return harness alongside name,
configPath, and timestamp; update any file-format versioning/validation to
handle missing/corrupted harness values (default or error), and export the
updated public API from packages/core/src/mcp/index.ts so callers can record and
query MCPs by harness for targeted uninstalls; add unit tests that verify
writing/reading entries with harness, lookup by harness, and robust handling of
missing or corrupted tracking files.
In `@openspec/schemas/ns-workflow/schema.yaml`:
- Line 57: The `implementation` artifact in the schema has mismatched
metadata—`generates: .` (actual code) but `template: design.md` (a design doc);
update the `implementation` artifact entry so `template` either matches a
code-oriented template or is removed/renamed to a guidance field: locate the
`implementation` artifact in the schema (look for the block containing
`implementation`, `generates: .`, and `template: design.md`) and either set
`template` to a code template name consistent with other artifacts (or to
null/omit it) or rename the key to something like `guidanceTemplate` to make
clear it is not a generated-file template.
---
Nitpick comments:
In
`@openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md`:
- Around line 151-156: Update the uninstall flow to detect and surface modified
NodeSource skills before deleting them: implement a pre-uninstall check (e.g.,
add a function detectModifiedNodeSourceSkills or listModifiedFiles used by the
uninstall handler) that compares stored checksums or repo/file state
(computeChecksum or git-status equivalent) against current SKILL.md and related
files, collect any modified items, and include that list in the uninstall
confirmation/output so users see what will be removed and can confirm or abort
the uninstall; ensure the uninstall routine still deletes tracked NodeSource
skills if the user confirms.
In `@openspec/changes/cross-harness-plugin-installer/tasks.md`:
- Around line 68-74: Add explicit handling and tests for the case where all
OAuth callback ports (8765–8770) are occupied: update the oauth-server.ts
startup logic (the server start/port-selection routine and the function that
returns credentials) to detect when no ports are available and throw or return a
clear, typed error (e.g., PortUnavailableError) after attempting the full range;
add unit tests that simulate all ports in use to assert the thrown error and
that the 5-minute timeout behavior remains unchanged; also document this
behavior in the Task 7 testing section so callers and CI know the expected
failure mode when all ports are occupied.
- Around line 256-293: The plan omits a dedicated test-infrastructure setup
task; add a new Task "27.5: Set up test infrastructure" before Task 28 in
openspec/changes/cross-harness-plugin-installer/tasks.md that describes
installing/configuring the test runner (Jest/Vitest/Mocha), assertion/mocking
libs (e.g., chai/sinon or built-ins), coverage tooling (c8/nyc) and adding test
scripts and config files (package.json scripts, packages/core/package.json deps,
jest.config.js or vitest.config.ts, .nycrc), and require a minimal verification
step (npm test) and coverage threshold (e.g., 80%) so Task 28's tests have the
necessary infra.
In `@openspec/schemas/ns-workflow/schema.yaml`:
- Line 21: The YAML currently lists a single template ("template: specs.md") but
the artifact produces multiple output files ("specs/**/*.md"); update the schema
so the template-to-files relationship is explicit: either change "template:
specs.md" to a plural/parameterized value indicating it is applied per-spec
(e.g., a templating pattern or template directory) or document that the single
template is replicated for each generated file, and ensure the descriptor near
"template: specs.md" references the output pattern "specs/**/*.md" or the
generator behavior (replicate per file) so readers understand that one template
is used to produce multiple specs.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7ac99a6-14ee-4c8a-94cb-954f8bd4ec7c
📒 Files selected for processing (6)
openspec/changes/cross-harness-plugin-installer/.openspec.yamlopenspec/changes/cross-harness-plugin-installer/design.mdopenspec/changes/cross-harness-plugin-installer/proposal.mdopenspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.mdopenspec/changes/cross-harness-plugin-installer/tasks.mdopenspec/schemas/ns-workflow/schema.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md (1)
311-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winComplete or explicitly defer the Antigravity configuration specification.
The Antigravity scenario contains "TBD based on Antigravity docs" which leaves the specification incomplete. While other harnesses (Claude Code, Codex, OpenCode, Pi) have concrete configuration paths and formats, Antigravity lacks implementation guidance.
Suggested actions:
- If Antigravity details are known, add the specific config path and format
- If Antigravity support is deferred to a later phase, document this explicitly (e.g., "Antigravity support planned for Phase X") and reference the implementation roadmap
- If Antigravity is speculative, consider moving this scenario to a separate "Future Harnesses" section
🤖 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 `@openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md` around lines 311 - 316, The "Scenario: Antigravity CLI configuration" is incomplete—replace the "TBD based on Antigravity docs" and clarify next steps: either (A) provide the concrete Antigravity config path and format and update the acceptance criteria so the step "entries are added to Antigravity-specific config location" and "skills are copied to Antigravity skill directory" mirror the concrete examples used for Claude Code/Codex/OpenCode/Pi, or (B) explicitly defer support by changing the scenario to state "Antigravity support deferred to Phase X" and add a reference to the implementation roadmap (or move the scenario into a new "Future Harnesses" section). Ensure the Scenario heading "Scenario: Antigravity CLI configuration" and the two steps mentioning config entries and skill copying are updated accordingly.
🧹 Nitpick comments (1)
openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md (1)
10-10: ⚡ Quick winClarify credential file permissions for cross-platform support.
Line 10 specifies Unix permissions
0600for the credentials file, but doesn't address Windows. Since this is a cross-platform installer, the specification should document the equivalent Windows behavior.📝 Suggested addition
-**And** credentials are stored at `~/.agents/.nodesource-auth.json` with permissions 0600 +**And** credentials are stored at `~/.agents/.nodesource-auth.json` with permissions 0600 (Unix) or restricted to current user (Windows ACL)🤖 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 `@openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md` at line 10, Update the spec sentence that currently states "credentials are stored at `~/.agents/.nodesource-auth.json` with permissions 0600" to be explicit about cross-platform behavior: keep the existing Unix/Nix instruction (create the file in the user home directory and set POSIX mode 0600) and add a Windows equivalent that specifies storing the file in the user profile (e.g., %USERPROFILE%\.agents\.nodesource-auth.json) and restricting NTFS ACLs so only the current user has read/write access (e.g., remove inheritance and grant full control to the user). Reference the file path token `~/.agents/.nodesource-auth.json` and the permission token `0600` in the updated text so reviewers can locate and verify the change.
🤖 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
`@openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md`:
- Around line 152-191: The file contains a duplicated "Best-Effort Cleanup
Algorithm" section repeated twice; remove the second copy so only one instance
of that heading and its bulleted Algorithm and Known NodeSource artifacts list
remain (keep the first occurrence and delete the duplicate). Locate the repeated
"Best-Effort Cleanup Algorithm" heading and the subsequent identical
lists/numbered steps and delete the latter block (the duplicate after the first
occurrence) so the spec contains the algorithm exactly once.
- Around line 156-163: Update the "Known NodeSource artifacts" cleanup list to
include harness-specific skill directory patterns in addition to
`~/.agents/skills/ns-*`: add `~/.claude/skills/`, `~/.codex/skills/`,
`~/.config/opencode/skills/`, and `~/.pi/skills/` and ensure the tracking file
`~/.agents/.nodesource-installed.json` and MCP entries (`ns-benchmark`,
`nsolid-mcp`, `ncm-mcp`) remain listed; modify the section containing the
artifact patterns so the cleanup algorithm will detect and remove
harness-specific skill installs referenced elsewhere in the spec.
---
Outside diff comments:
In
`@openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md`:
- Around line 311-316: The "Scenario: Antigravity CLI configuration" is
incomplete—replace the "TBD based on Antigravity docs" and clarify next steps:
either (A) provide the concrete Antigravity config path and format and update
the acceptance criteria so the step "entries are added to Antigravity-specific
config location" and "skills are copied to Antigravity skill directory" mirror
the concrete examples used for Claude Code/Codex/OpenCode/Pi, or (B) explicitly
defer support by changing the scenario to state "Antigravity support deferred to
Phase X" and add a reference to the implementation roadmap (or move the scenario
into a new "Future Harnesses" section). Ensure the Scenario heading "Scenario:
Antigravity CLI configuration" and the two steps mentioning config entries and
skill copying are updated accordingly.
---
Nitpick comments:
In
`@openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md`:
- Line 10: Update the spec sentence that currently states "credentials are
stored at `~/.agents/.nodesource-auth.json` with permissions 0600" to be
explicit about cross-platform behavior: keep the existing Unix/Nix instruction
(create the file in the user home directory and set POSIX mode 0600) and add a
Windows equivalent that specifies storing the file in the user profile (e.g.,
%USERPROFILE%\.agents\.nodesource-auth.json) and restricting NTFS ACLs so only
the current user has read/write access (e.g., remove inheritance and grant full
control to the user). Reference the file path token
`~/.agents/.nodesource-auth.json` and the permission token `0600` in the updated
text so reviewers can locate and verify the change.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66555777-c871-4e78-b5fd-51572cfb2e37
📒 Files selected for processing (5)
openspec/changes/cross-harness-plugin-installer/design.mdopenspec/changes/cross-harness-plugin-installer/proposal.mdopenspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.mdopenspec/changes/cross-harness-plugin-installer/tasks.mdopenspec/schemas/ns-workflow/schema.yaml
💤 Files with no reviewable changes (1)
- openspec/schemas/ns-workflow/schema.yaml
✅ Files skipped from review due to trivial changes (2)
- openspec/changes/cross-harness-plugin-installer/proposal.md
- openspec/changes/cross-harness-plugin-installer/tasks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/changes/cross-harness-plugin-installer/design.md
Branch Review:
|
| Priority | Action | Where |
|---|---|---|
| HIGH | Add platform path resolution strategy (Windows %USERPROFILE% vs Unix ~) |
design.md, Task 4 |
| HIGH | Replace symlink-first strategy with copy-on-Windows fallback | design.md, Task 10 |
| MEDIUM | Document Windows file permission limitations; consider DPAPI/Credential Manager | design.md, Task 5 |
| MEDIUM | Fix atomic write strategy for Windows (rename fails if target exists) | design.md, Task 4 |
| MEDIUM | Add Windows to the testing matrix or explicitly exclude it | Task 40, proposal.md |
| LOW | Make error messages platform-aware (no sudo/chown on Windows) |
Task 34 |
| LOW | Provide cross-platform test scripts (Node.js or dual .sh/.ps1) |
Task 29 |
| LOW | Normalize path separators in tracking file storage | design.md, Task 11 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
openspec/changes/cross-harness-plugin-installer/tasks.md (2)
359-364: ⚡ Quick winAntigravity research (Task 38) should precede implementation tasks.
Task 38 researches "Antigravity CLI 2.0 plugin format and marketplace" but is scheduled in Phase 10, after Task 19b (Antigravity adapter, Phase 5) and Task 26 (Antigravity plugin package, Phase 7) have already been implemented. Research should inform implementation, not follow it. Consider moving Task 38 to Phase 1 or early Phase 2, and updating Task 19b and Task 26 to depend on Task 38.
🤖 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 `@openspec/changes/cross-harness-plugin-installer/tasks.md` around lines 359 - 364, Task 38 (Document Antigravity CLI requirements) is scheduled after work that depends on its findings; move the research earlier and add dependencies: relocate "Task 38: Document Antigravity CLI requirements" (docs/antigravity-research.md) to Phase 1 or early Phase 2, and update Task 19b (Antigravity adapter, Phase 5) and Task 26 (Antigravity plugin package, Phase 7) to declare a dependency on Task 38 so the adapter and plugin package cannot proceed until the research doc and required manifest format are completed.
305-311: ⚡ Quick winAdd Windows to the CI/CD testing matrix.
Task 32 sets up CI/CD workflows but doesn't mention Windows in the testing matrix. Given that Windows support is a key objective of this PR (per PR objectives and commit message), and multiple tasks (4, 5, 10, 29, 34, 40) include Windows-specific guidance, the CI pipeline should test on Windows alongside macOS and Linux to catch platform-specific issues (path handling, symlinks/junctions, atomic writes, file permissions) early.
📋 Suggested enhancement
### Task 32: Set up CI/CD pipeline -- **Description**: Create GitHub Actions workflow for automated testing. Run unit tests, integration tests, and lint on PR. Build and publish packages on release tag. +- **Description**: Create GitHub Actions workflow for automated testing on macOS, Linux, and Windows. Run unit tests, integration tests, and lint on PR. Build and publish packages on release tag. Use matrix strategy to test across platforms. - **Depends on**: Task 28 - **Files**: - `.github/workflows/test.yml` - `.github/workflows/publish.yml` -- **Testing**: Push to branch, verify CI runs. Create test release, verify publish workflow. +- **Testing**: Push to branch, verify CI runs on all three platforms. Create test release, verify publish workflow.🤖 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 `@openspec/changes/cross-harness-plugin-installer/tasks.md` around lines 305 - 311, Task 32's CI workflows do not include Windows; update the GitHub Actions matrices to add windows-latest so tests run on Windows alongside ubuntu-latest and macos-latest. Specifically, modify the matrix in .github/workflows/test.yml (the job that runs unit/integration/lint) to include windows-latest and ensure Windows-compatible steps (use actions/checkout, actions/setup-node, and any path/permission steps) are applied or conditionalized for runs on the 'windows-latest' OS; also update .github/workflows/publish.yml to include a Windows runner if builds/publish need Windows artifacts, and add any Windows-specific setup (e.g., path normalization, symlink handling, or using robocopy/junction alternatives) so Task 32 validates Windows support.
🤖 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 `@openspec/changes/cross-harness-plugin-installer/tasks.md`:
- Around line 323-332: Update the Windows example path in the platform-aware
permission error messages to match the canonical Claude config location
(~/.claude.json) — replace any occurrences of
"C:\Users\<user>\.claude\.mcp.json" with "C:\Users\<user>\.claude.json" in the
platform-specific message generation (see packages/core/src/errors.ts and any
functions/methods that build the permission-denied message, e.g., the error
class or helper that checks process.platform); ensure the updated string is used
wherever structured errors for permission issues are created so Windows guidance
stays consistent with Task 16.
- Line 42: Update Task 4 wording to explicitly point to the existing "Platform
Path Resolution" section in the design doc instead of implying it must be
created: in tasks.md change the phrasing around Task 4 to state that the
design.md already contains a "### Platform Path Resolution" section and that
implementers should follow that existing section for the platform path mapping
table and path normalization requirements; remove ambiguous language like "needs
to be created" so the note clearly references the current "Platform Path
Resolution" section in design.md.
---
Nitpick comments:
In `@openspec/changes/cross-harness-plugin-installer/tasks.md`:
- Around line 359-364: Task 38 (Document Antigravity CLI requirements) is
scheduled after work that depends on its findings; move the research earlier and
add dependencies: relocate "Task 38: Document Antigravity CLI requirements"
(docs/antigravity-research.md) to Phase 1 or early Phase 2, and update Task 19b
(Antigravity adapter, Phase 5) and Task 26 (Antigravity plugin package, Phase 7)
to declare a dependency on Task 38 so the adapter and plugin package cannot
proceed until the research doc and required manifest format are completed.
- Around line 305-311: Task 32's CI workflows do not include Windows; update the
GitHub Actions matrices to add windows-latest so tests run on Windows alongside
ubuntu-latest and macos-latest. Specifically, modify the matrix in
.github/workflows/test.yml (the job that runs unit/integration/lint) to include
windows-latest and ensure Windows-compatible steps (use actions/checkout,
actions/setup-node, and any path/permission steps) are applied or
conditionalized for runs on the 'windows-latest' OS; also update
.github/workflows/publish.yml to include a Windows runner if builds/publish need
Windows artifacts, and add any Windows-specific setup (e.g., path normalization,
symlink handling, or using robocopy/junction alternatives) so Task 32 validates
Windows support.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41adf82c-fcee-4f22-9ba6-f8ee32281063
📒 Files selected for processing (4)
openspec/changes/cross-harness-plugin-installer/design.mdopenspec/changes/cross-harness-plugin-installer/proposal.mdopenspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.mdopenspec/changes/cross-harness-plugin-installer/tasks.md
✅ Files skipped from review due to trivial changes (2)
- openspec/changes/cross-harness-plugin-installer/proposal.md
- openspec/changes/cross-harness-plugin-installer/specs/installation-and-auth.md
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/changes/cross-harness-plugin-installer/design.md
This was referenced Jun 9, 2026
Closed
Closed
This was referenced Jun 9, 2026
Merged
This was referenced Jun 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Documentation
Chores