[PR #1744] [CLOSED] fix(setup): verify independently selected skill destinations #4794

New issue

Closed

opened 2026-10-31 00:55:07 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:55:07 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1744
Author: @ac1982
Created: 10/11/2026
Status: ❌ Closed

Base: main ← Head: fix/setup-skill-readiness

📝 Commits (1)

bf0a6fd fix(setup): verify independently selected skill destinations

📊 Changes

6 files changed (+160 additions, -2 deletions)

View changed files

📝 docs/installation.md (+6 -0)
📝 src/application/Doctor.ts (+4 -2)
📝 src/application/DoctorScope.ts (+4 -0)
📝 src/application/Setup.recovery.test.ts (+1 -0)
📝 src/application/Setup.ts (+5 -0)
➕ tests/acceptance/setup/selectedSkillSetup.test.ts (+140 -0)

📄 Description

Summary

Fixes #1711. Verify the skill destinations selected by --skill-client when setup performs its final readiness check. A successful client-only refresh and a no-change rerun now return ready with exit 0.

Problem and expected behavior

With a Claude Code registration and no shared REA skill, setup --skill-client claude_code --yes --json installs the correct files under .claude/skills but checks the unselected shared destination. It returns needs_human, exit 1, and misleading optional-Hopper remediation.

Registration and skill selection are independent. For example, --client codex --skill-client claude_code must require the Codex registration and the Claude skill copy, without also requiring a Claude registration or a shared skill.

Change and scope

Carry the selected skill client IDs through the setup readiness scope into Doctor's existing skill-identity check. Add an optional skill_clients field to the reported scope so the checked skill selection remains visible. Doctor callers that omit the new internal selection keep their existing behavior.

No installation-path or registration-writing changes. Related #1352 and #1504 address destination discovery and client configuration; this fixes the final readiness projection after those files are correctly installed.

Contract and boundary impact

Semantic owner and earliest changed stage: setup's selected readiness scope, before Doctor chooses skill identities.
CLI and MCP/tool-catalog contract: existing CLI flags; additive doctor.scope.skill_clients output when supplied. No MCP tool/schema changes.
Provider, bridge, target-format, or platform compatibility: no provider changes; verified on Linux arm64.
Evidence, artifact, provenance, or reconstruction contract: selected registration and skill scopes remain distinct and visible.
Process execution, authorization, cleanup, or containment impact: approval, file writes and cleanup retain existing ownership.
Generated metadata, package, or installation impact: no package metadata or generated-file changes; setup correctly reports the selected installed copy.

Evidence and regression coverage

Both new compiled CLI tests fail on unmodified base 844fa527 with exit 1 and missing shared-skill identity.

The passing workflows verify:

client-only install, a no-change rerun, and every installed bundle file against the packaged source;
a damaged selected reference is reported stale, then repaired;
an unselected shared copy stays absent and the existing registration stays byte-identical;
explicit shared installation still succeeds;
separate Codex registration and Claude skill selection succeeds without adding a Claude registration or shared skill.

Representative corrected output:

{"status":"ready","doctor":{"healthy":true,"scope":{"clients":[],"skill":true,"skill_clients":["claude_code"]},"identity":{"skill":{"state":"aligned"}}}}

Observed, derived, and inferred claims remain distinguishable.
Selected paths and identities remain preserved.
Missing and damaged selected copies retain failure reporting.

Validation performed

Linux arm64, pinned Node 24.18.0 / npm 11.16.0:

npm run test:focused -- tests/acceptance/setup/selectedSkillSetup.test.ts src/application/Doctor.test.ts src/application/Setup.configuration.test.ts src/application/Setup.planning.test.ts src/application/Setup.recovery.test.ts src/application/Setup.initialSnapshot.test.ts — both CLI regressions and 55 other service tests passed. One recovery expectation needed the newly carried skill scope.
npm run test:local -- src/application/Setup.recovery.test.ts — all 11 recovery tests passed after updating that expectation.
npm run check — all six tasks and architecture guards passed; warnings, no lint errors.
npm run verify:test-discovery — all 814 test files discovered exactly once.
npm run docs:check — passed.
npm run verify:package — passed, including installed CLI/MCP, setup/uninstall, idempotence, backups, failure recovery, skill bundle and owned-process cleanup.
npm pack --dry-run --json and git diff --check — passed.

Compatibility, safety, and release

No migration or new installation destination. Tests use disposable profiles and existing approved setup commands. Interactive client applications, native Hopper/Ghidra and non-Linux hosts were not exercised; full repository test-suite execution remains with CI.

Review checklist

Concrete reported bug; one focused outcome and conventional title.
Issue linked; open, closed and merged PR searches checked the issue and related setup work.
Tests cover real compiled CLI behavior, reruns, damaged content and independent selections.
Owning documentation updated; no generated output committed.
Representative CLI output included.
Final diff reviewed for unrelated changes, credentials and unsupported claims.

Prepared with OpenAI Codex assistance.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/1744 **Author:** [@ac1982](https://github.com/ac1982) **Created:** 10/11/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/setup-skill-readiness` --- ### 📝 Commits (1) - [`bf0a6fd`](https://github.com/morluto/rea/commit/bf0a6fd849f907210cf60a3c18df3eb6556f56e9) fix(setup): verify independently selected skill destinations ### 📊 Changes **6 files changed** (+160 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `docs/installation.md` (+6 -0) 📝 `src/application/Doctor.ts` (+4 -2) 📝 `src/application/DoctorScope.ts` (+4 -0) 📝 `src/application/Setup.recovery.test.ts` (+1 -0) 📝 `src/application/Setup.ts` (+5 -0) ➕ `tests/acceptance/setup/selectedSkillSetup.test.ts` (+140 -0) </details> ### 📄 Description ## Summary Fixes #1711. Verify the skill destinations selected by `--skill-client` when setup performs its final readiness check. A successful client-only refresh and a no-change rerun now return `ready` with exit 0. ## Problem and expected behavior With a Claude Code registration and no shared REA skill, `setup --skill-client claude_code --yes --json` installs the correct files under `.claude/skills` but checks the unselected shared destination. It returns `needs_human`, exit 1, and misleading optional-Hopper remediation. Registration and skill selection are independent. For example, `--client codex --skill-client claude_code` must require the Codex registration and the Claude skill copy, without also requiring a Claude registration or a shared skill. ## Change and scope Carry the selected skill client IDs through the setup readiness scope into Doctor's existing skill-identity check. Add an optional `skill_clients` field to the reported scope so the checked skill selection remains visible. Doctor callers that omit the new internal selection keep their existing behavior. No installation-path or registration-writing changes. Related #1352 and #1504 address destination discovery and client configuration; this fixes the final readiness projection after those files are correctly installed. ## Contract and boundary impact - Semantic owner and earliest changed stage: setup's selected readiness scope, before Doctor chooses skill identities. - CLI and MCP/tool-catalog contract: existing CLI flags; additive `doctor.scope.skill_clients` output when supplied. No MCP tool/schema changes. - Provider, bridge, target-format, or platform compatibility: no provider changes; verified on Linux arm64. - Evidence, artifact, provenance, or reconstruction contract: selected registration and skill scopes remain distinct and visible. - Process execution, authorization, cleanup, or containment impact: approval, file writes and cleanup retain existing ownership. - Generated metadata, package, or installation impact: no package metadata or generated-file changes; setup correctly reports the selected installed copy. ## Evidence and regression coverage Both new compiled CLI tests fail on unmodified base `844fa527` with exit 1 and missing shared-skill identity. The passing workflows verify: - client-only install, a no-change rerun, and every installed bundle file against the packaged source; - a damaged selected reference is reported stale, then repaired; - an unselected shared copy stays absent and the existing registration stays byte-identical; - explicit shared installation still succeeds; - separate Codex registration and Claude skill selection succeeds without adding a Claude registration or shared skill. Representative corrected output: ```json {"status":"ready","doctor":{"healthy":true,"scope":{"clients":[],"skill":true,"skill_clients":["claude_code"]},"identity":{"skill":{"state":"aligned"}}}} ``` - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Selected paths and identities remain preserved. - [x] Missing and damaged selected copies retain failure reporting. ## Validation performed Linux arm64, pinned Node 24.18.0 / npm 11.16.0: - `npm run test:focused -- tests/acceptance/setup/selectedSkillSetup.test.ts src/application/Doctor.test.ts src/application/Setup.configuration.test.ts src/application/Setup.planning.test.ts src/application/Setup.recovery.test.ts src/application/Setup.initialSnapshot.test.ts` — both CLI regressions and 55 other service tests passed. One recovery expectation needed the newly carried skill scope. - `npm run test:local -- src/application/Setup.recovery.test.ts` — all 11 recovery tests passed after updating that expectation. - `npm run check` — all six tasks and architecture guards passed; warnings, no lint errors. - `npm run verify:test-discovery` — all 814 test files discovered exactly once. - `npm run docs:check` — passed. - `npm run verify:package` — passed, including installed CLI/MCP, setup/uninstall, idempotence, backups, failure recovery, skill bundle and owned-process cleanup. - `npm pack --dry-run --json` and `git diff --check` — passed. ## Compatibility, safety, and release No migration or new installation destination. Tests use disposable profiles and existing approved setup commands. Interactive client applications, native Hopper/Ghidra and non-Linux hosts were not exercised; full repository test-suite execution remains with CI. ## Review checklist - [x] Concrete reported bug; one focused outcome and conventional title. - [x] Issue linked; open, closed and merged PR searches checked the issue and related setup work. - [x] Tests cover real compiled CLI behavior, reruns, damaged content and independent selections. - [x] Owning documentation updated; no generated output committed. - [x] Representative CLI output included. - [x] Final diff reviewed for unrelated changes, credentials and unsupported claims. Prepared with OpenAI Codex assistance. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 00:55:07 +00:00