[PR #4492] [CLOSED] fix(setup): retain partial skill identity without a manifest #6273

Closed
opened 2026-10-31 01:13:11 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4492
Author: @jayinvers
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/skill-partial-identity


📝 Commits (1)

  • fdcf51b fix(setup): report partial skill identity when the manifest is missing

📊 Changes

4 files changed (+64 additions, -7 deletions)

View changed files

📝 docs/installation.md (+4 -1)
📝 src/application/Doctor.ts (+9 -6)
📝 src/application/SetupSkill.ts (+3 -0)
📝 tests/boundary/filesystem/setupSkill.test.ts (+48 -0)

📄 Description

Summary

A skill destination that retains managed files (for example references/connection-and-recovery.md) but has no readable SKILL.md was reported by rea doctor as entirely missing, hiding a retained partial installation. The identity reader now keeps that observation as a noncanonical identity with unavailable metadata, and doctor renders it as a new partial skill state. Fixes #4384.

Problem and expected behavior

Smallest trigger: a selected skill destination contains retained managed reference bytes, no readable SKILL.md, and no path issues.

  • Violated invariant (AGENTS.md, identity invariants): unresolved identity must remain unknown rather than be treated as absent. readInstalledSkillIdentity returned undefined for this destination — its second early return fired when manifest content was absent and no issues existed, discarding the inspection's installedDestination: true / canonical: false / retained-files observation — and Doctor.ts maps undefined to state missing.
  • Expected: a retained partial installation is distinguishable from an absent skill; the empty-destination control keeps returning absence (undefined); canonicalSkillNeedsInstall stays true for the partial case; retained file bytes are unchanged.

Change and scope

  • src/application/SetupSkill.ts (owning boundary): inspectCanonicalSkill reports whether any destination retained managed file bytes (retainedManagedFiles); readInstalledSkillIdentity now keeps the installed-destination observation as a noncanonical identity when manifest content is absent and no issues exist, marked with a new optional manifestMissing: true and null version/toolCount. undefined remains reserved for a destination with nothing retained.
  • src/application/Doctor.ts: DoctorSkillState gains partial; an identity with manifestMissing maps to partial, with an explicit skill:identity check detail. Existing mappings are unchanged: empty destination stays missing, foreign or unparseable manifest bytes stay stale, path issues stay unmanaged/unknown.
  • docs/installation.md: one sentence naming the new state where the other skill states are described.
  • Not included: no change to existingSkillDestinations (update maintenance still requires a readable managed manifest to establish ownership) or uninstall; the partial copy is repaired by the existing install flow, which already reports needs-install.

Follow the contribution scope.
Not applicable: ordinary read-only fix within existing boundaries.

Contract and boundary impact

  • Semantic owner and earliest changed stage: application setup skill inspection (SetupSkill.ts); doctor projection consumption.
  • Contribution scope fit and linked maintainer decision: not applicable (routine fix, no new capability).
  • CLI and MCP/tool-catalog contract: rea doctor --json identity.skill.state gains one new value, partial (additive). identity.skill also gains manifestMissing: true only in that new case. No tool-catalog schema change; the doctor report identity is not catalog-generated.
  • Provider, bridge, target-format, or platform compatibility: none.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable.
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none; no generated outputs touched.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

  • Read existing files or captured data.
  • Launch or attach to a process.
  • Inject instrumentation or run scripts/code.
  • Change process memory, registers, files, or configuration.
  • Contact network services.
  • Recover protected code or work around protections.
  • Documentation or usability only; no target interaction.
  • Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

  • Tests added or updated: tests/boundary/filesystem/setupSkill.test.ts, "preserves a retained partial skill instead of treating it as absent" — covers (a) retained managed reference + missing manifest reads as exactly {canonical: false, version: null, toolCount: null, manifestMissing: true}, (b) the same destination reads undefined before any file is retained, (c) canonicalSkillNeedsInstall stays true, (d) retained bytes are unchanged, and (e) the doctor projection reports state: "partial" with the new check detail rather than missing.
  • Base reproduction (executed on the base revision, Windows 11, Node 24.18.0, tsx probe with a synthetic home and skillDestinations(home, ["shared"], {}, "darwin")): absent destination → identity undefined; partial destination → identity undefined (the bug); canonicalSkillNeedsInstall → true; retained bytes unchanged. After the fix, the partial read returns {canonical: false, manifestMissing: true, version: null, toolCount: null} and the other three results are unchanged.
  • Red-green (executed): temporarily reverting only the readInstalledSkillIdentity early-return change makes the new test fail on the identity assertion (expected undefined to deeply equal {canonical: false, manifestMissing: true, toolCount: null, version: null}); restoring the fix makes it pass. The pre-existing tests that anchor foreign-manifest bytes as stale caught an earlier too-broad draft of the doctor mapping (any version: null → partial); the final mapping keys on manifestMissing only.
  • User-visible CLI/MCP output: the new projection strings are asserted through runDoctor with the production systemDoctorHost skill identity reader: identity.skill.state = "partial", check detail "Installed REA skill files are present without a readable skill manifest.", remediation "Run rea setup to update the installed REA skill.". A full doctor CLI/MCP run was not executed locally; source-level host assertions only.
  • Remaining proof gaps: local validation is Windows-only; cross-platform behavior (including the permission-dependent skipped tests) relies on CI.

For evidence-bearing changes:

  • Observed, derived, and inferred claims remain distinguishable.
  • Artifact identity, source provenance, and failed attempts remain preserved.
  • Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

  • npm run test:focused -- tests/boundary/filesystem/setupSkill.test.ts src/application/Doctor.test.ts — setupSkill: 13 passed, 4 skipped (Windows-skipped permission cases). Doctor.test.ts: 31 passed, 1 failed: "doctor installed Hopper discovery > checks the canonical user install after the system launcher" also fails on a clean origin/main checkout on this host (verified with git stash; POSIX path-join expectation), pre-existing and unrelated to this change.
  • npm run check:fast — pass (typecheck + lint).
  • npx oxfmt --check src/application/SetupSkill.ts src/application/Doctor.ts tests/boundary/filesystem/setupSkill.test.ts (and docs/installation.md) — all files formatted.
  • npm run verify:test-discovery — "Discovered all 1417 test files exactly once."
  • npm run docs:check — pass (generated-doc freshness, doc facts, licenses, README consistency).
  • Fork CI (jayinvers/rea, branch fix/skill-partial-identity, PR #3): full "CI" workflow.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. The doctor JSON contract change is additive: identity.skill.state can now be partial (previously rendered as missing for this case), and identity.skill.manifestMissing: true appears only in that new case. Consumers switching exhaustively on the state union should admit partial; its remediation is the existing "Run rea setup to update the installed REA skill."
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable (no provider change). Local runs were Windows-only; CI covers the Linux/macOS lanes.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: read-only inspection of already-managed skill locations; no new file access, no target interaction.

Author readiness

Human owner: @jayinvers

  • I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
  • I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
  • I declare this PR ready for initial maintainer review.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. (No scope decision required: read-only fix.)
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🔄 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/4492 **Author:** [@jayinvers](https://github.com/jayinvers) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/skill-partial-identity` --- ### 📝 Commits (1) - [`fdcf51b`](https://github.com/morluto/rea/commit/fdcf51b16316f47a9f4e979a2eb0aa4a3edf4809) fix(setup): report partial skill identity when the manifest is missing ### 📊 Changes **4 files changed** (+64 additions, -7 deletions) <details> <summary>View changed files</summary> 📝 `docs/installation.md` (+4 -1) 📝 `src/application/Doctor.ts` (+9 -6) 📝 `src/application/SetupSkill.ts` (+3 -0) 📝 `tests/boundary/filesystem/setupSkill.test.ts` (+48 -0) </details> ### 📄 Description <!-- PR title: type(optional-scope): imperative outcome The title becomes the squash-merge commit subject and generated changelog entry. --> ## Summary A skill destination that retains managed files (for example `references/connection-and-recovery.md`) but has no readable `SKILL.md` was reported by `rea doctor` as entirely `missing`, hiding a retained partial installation. The identity reader now keeps that observation as a noncanonical identity with unavailable metadata, and doctor renders it as a new `partial` skill state. Fixes #4384. ## Problem and expected behavior Smallest trigger: a selected skill destination contains retained managed reference bytes, no readable `SKILL.md`, and no path issues. - Violated invariant (AGENTS.md, identity invariants): unresolved identity must remain unknown rather than be treated as absent. `readInstalledSkillIdentity` returned `undefined` for this destination — its second early return fired when manifest content was absent and no issues existed, discarding the inspection's `installedDestination: true` / `canonical: false` / retained-files observation — and `Doctor.ts` maps `undefined` to state `missing`. - Expected: a retained partial installation is distinguishable from an absent skill; the empty-destination control keeps returning absence (`undefined`); `canonicalSkillNeedsInstall` stays `true` for the partial case; retained file bytes are unchanged. ## Change and scope - `src/application/SetupSkill.ts` (owning boundary): `inspectCanonicalSkill` reports whether any destination retained managed file bytes (`retainedManagedFiles`); `readInstalledSkillIdentity` now keeps the installed-destination observation as a noncanonical identity when manifest content is absent and no issues exist, marked with a new optional `manifestMissing: true` and `null` version/toolCount. `undefined` remains reserved for a destination with nothing retained. - `src/application/Doctor.ts`: `DoctorSkillState` gains `partial`; an identity with `manifestMissing` maps to `partial`, with an explicit `skill:identity` check detail. Existing mappings are unchanged: empty destination stays `missing`, foreign or unparseable manifest bytes stay `stale`, path issues stay `unmanaged`/`unknown`. - `docs/installation.md`: one sentence naming the new state where the other skill states are described. - Not included: no change to `existingSkillDestinations` (update maintenance still requires a readable managed manifest to establish ownership) or uninstall; the partial copy is repaired by the existing install flow, which already reports needs-install. Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Not applicable: ordinary read-only fix within existing boundaries. ## Contract and boundary impact - Semantic owner and earliest changed stage: application setup skill inspection (`SetupSkill.ts`); doctor projection consumption. - Contribution scope fit and linked maintainer decision: not applicable (routine fix, no new capability). - CLI and MCP/tool-catalog contract: `rea doctor --json` `identity.skill.state` gains one new value, `partial` (additive). `identity.skill` also gains `manifestMissing: true` only in that new case. No tool-catalog schema change; the doctor report identity is not catalog-generated. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details (if applicable): not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none; no generated outputs touched. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/filesystem/setupSkill.test.ts`, "preserves a retained partial skill instead of treating it as absent" — covers (a) retained managed reference + missing manifest reads as exactly `{canonical: false, version: null, toolCount: null, manifestMissing: true}`, (b) the same destination reads `undefined` before any file is retained, (c) `canonicalSkillNeedsInstall` stays `true`, (d) retained bytes are unchanged, and (e) the doctor projection reports `state: "partial"` with the new check detail rather than `missing`. - Base reproduction (executed on the base revision, Windows 11, Node 24.18.0, tsx probe with a synthetic home and `skillDestinations(home, ["shared"], {}, "darwin")`): absent destination → identity `undefined`; partial destination → identity `undefined` (the bug); `canonicalSkillNeedsInstall` → `true`; retained bytes unchanged. After the fix, the partial read returns `{canonical: false, manifestMissing: true, version: null, toolCount: null}` and the other three results are unchanged. - Red-green (executed): temporarily reverting only the `readInstalledSkillIdentity` early-return change makes the new test fail on the identity assertion (`expected undefined to deeply equal {canonical: false, manifestMissing: true, toolCount: null, version: null}`); restoring the fix makes it pass. The pre-existing tests that anchor foreign-manifest bytes as `stale` caught an earlier too-broad draft of the doctor mapping (any `version: null` → `partial`); the final mapping keys on `manifestMissing` only. - User-visible CLI/MCP output: the new projection strings are asserted through `runDoctor` with the production `systemDoctorHost` skill identity reader: `identity.skill.state = "partial"`, check detail "Installed REA skill files are present without a readable skill manifest.", remediation "Run rea setup to update the installed REA skill.". A full doctor CLI/MCP run was not executed locally; source-level host assertions only. - Remaining proof gaps: local validation is Windows-only; cross-platform behavior (including the permission-dependent skipped tests) relies on CI. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:focused -- tests/boundary/filesystem/setupSkill.test.ts src/application/Doctor.test.ts` — setupSkill: 13 passed, 4 skipped (Windows-skipped permission cases). Doctor.test.ts: 31 passed, 1 failed: "doctor installed Hopper discovery > checks the canonical user install after the system launcher" also fails on a clean `origin/main` checkout on this host (verified with `git stash`; POSIX path-join expectation), pre-existing and unrelated to this change. - `npm run check:fast` — pass (typecheck + lint). - `npx oxfmt --check src/application/SetupSkill.ts src/application/Doctor.ts tests/boundary/filesystem/setupSkill.test.ts` (and `docs/installation.md`) — all files formatted. - `npm run verify:test-discovery` — "Discovered all 1417 test files exactly once." - `npm run docs:check` — pass (generated-doc freshness, doc facts, licenses, README consistency). - Fork CI (jayinvers/rea, branch `fix/skill-partial-identity`, PR #3): full "CI" workflow. - CI run: https://github.com/jayinvers/rea/actions/runs/38909096789 — conclusion: success. ## Compatibility, safety, and release - Breaking changes or migration steps: none. The doctor JSON contract change is additive: `identity.skill.state` can now be `partial` (previously rendered as `missing` for this case), and `identity.skill.manifestMissing: true` appears only in that new case. Consumers switching exhaustively on the state union should admit `partial`; its remediation is the existing "Run rea setup to update the installed REA skill." - Real Hopper/Ghidra, browser, or OS coverage: not applicable (no provider change). Local runs were Windows-only; CI covers the Linux/macOS lanes. - Package or release metadata impact: none. - Security, privacy, process, or containment review: read-only inspection of already-managed skill locations; no new file access, no target interaction. ## Author readiness Human owner: @jayinvers - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. (No scope decision required: read-only fix.) - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
bsakaguchi 2026-10-31 01:13:11 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
morluto/rea#6273
No description provided.