[PR #2959] [CLOSED] test(setup): cover byte-exact skill recovery snapshots #5518

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:07:16 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2959
Author: @dakjdakd
Created: 10/13/2026
Status: ❌ Closed

Base: main ← Head: fix/skill-recovery-snapshots

📝 Commits (4)

3f13722 fix(setup): preserve verified current skill recovery snapshots
425bcdf fix(setup): retain original skill bytes through recovery
41fd0ab Merge remote-tracking branch 'origin/main' into HEAD
d5bce77 test(setup): merge current main with skill recovery coverage

📊 Changes

2 files changed (+289 additions, -3 deletions)

View changed files

📝 tests/acceptance/setup/selectedSkillSetup.test.ts (+37 -3)
📝 tests/boundary/filesystem/setupSkill.test.ts (+252 -0)

📄 Description

Summary

Add boundary and public CLI regression coverage for byte-exact managed-skill recovery snapshots. The implementation from #2958 is already on main in ec56c1da0. The remaining PR changes only tests/boundary/filesystem/setupSkill.test.ts and tests/acceptance/setup/selectedSkillSetup.test.ts.

The added cases protect three related guarantees: snapshots preserve the current file's exact bytes, an older first backup is retained without being mistaken for the current recovery copy, and setup verifies a snapshot before publishing any replacement file. The CLI case also checks that repairing a non-UTF-8 reference leaves the selected skill aligned and that the next invocation is idempotent.

Related issue: #2958, already resolved by the implementation on main.

Problem and expected behavior

A managed skill can already have a .rea.backup from its first installation when a later repair starts. If that backup contains V0 and the live file contains V2, a V2-to-current repair needs a verified V2 recovery copy. Reporting V0 as the current transaction's recovery location would lose the relationship between the file that was replaced and the file the user can recover.

Byte identity also matters independently of text validity. A skill file can contain bytes from a legacy editor encoding or damaged content. Decoding those bytes as UTF-8 and re-encoding the result does not preserve the original. Setup must be able to replace the damaged file while keeping its exact previous bytes available, and a successful rollback must write those same bytes back.

The implementation on main handles these guarantees through verified snapshots and original byte buffers. These tests exercise the filesystem and CLI outcomes, including transaction error reporting, instead of introducing another snapshot-selection rule.

Change and scope

The filesystem tests create isolated managed-skill directories and exercise installCanonicalSkill through its production implementation. New cases cover:

Repair of non-UTF-8 files both with and without a historical first backup. The snapshot is read as bytes and compared directly to the original buffer, and the installed file is compared to the authored bundle.
Recovery after a publication rejection, with separate successful-rollback and retained-recovery cases. Successful rollback restores the original bytes; when rollback cannot complete, the reported recovery path still points to an exact copy of the current original rather than the historical backup.
Preservation of the first backup and reuse of a verified, content-addressed current snapshot. An aligned rerun returns unchanged; repairing the same original again reuses the existing snapshot without rewriting it.
A historical first backup alongside a current V2 snapshot. The returned recovery path and message refer to V2, while the V0 first backup remains intact.
Occupied snapshot locations, including directories and an existing recovery file with different bytes. Setup rejects the backup stage before publishing the manifest or reference file.
Readback verification after a backup copy. A fixture that changes the copied bytes is rejected before the writer is called, and the original managed file remains intact.

The publication and copy fault cases are deterministic transaction fixtures around real files. They check the production owner's failure semantics; they do not simulate a successful filesystem result or report a fault fixture as a developer test failure.

The acceptance test extends the existing selected-skill public CLI workflow. It writes a known non-UTF-8 byte sequence into an installed reference while keeping the historical first backup, invokes the actual setup command, and checks status: "ready" with appliedActions: ["installed_skill"]. It verifies complete bundle alignment, the unchanged first backup, and the byte-exact content-addressed recovery file. The next CLI invocation returns status: "ready" with an empty appliedActions array. Existing assertions continue to protect the selected install location and caller configuration.

The updated branch retains main's additional setup tests and imports alongside this coverage. All three production Setup*.ts files remain identical to main. This is test-only coverage for the existing installation and recovery authority.

The existing linked-skill CLI cases now inspect parsed JSON fields for the rejected link's path. On Windows, the serialized JSON representation escapes path separators; comparing an unescaped filesystem path to that representation is not a reliable check of the reported identity. The assertions check remediation and the doctor's issues entry with the exact path and symbolic_link kind. They continue checking the nonzero result, skill_install_failed, no applied actions, the unmanaged state, and preservation of the link.

Contract and boundary impact

Semantic owner and earliest changed stage: regression coverage at managed-skill snapshot preparation, publication, rollback, and public selected-skill setup.
Contribution scope and maintainer decision: tests for the existing setup workflow; no new-capability scope decision is applicable.
CLI and MCP/tool-catalog contract: unchanged. The acceptance case checks existing setup JSON fields, and the boundary cases check existing backup-stage and recovery-path facts.
Provider, bridge, target-format, or platform compatibility: no provider, bridge, or target-format change. Exact-byte comparisons do not depend on a file being valid UTF-8.
Evidence, artifact, provenance, or reconstruction contract: assertions bind each recovery copy to the current original bytes and its actual path; no output schema is added.
Target selection and cleanup: tests use fixture-owned temporary directories and the existing public selected-install workflow. They do not write a real user skill directory or broaden setup authority.
Generated metadata, package, or installation impact: no package dependencies, installer implementation, or generated files are committed. Built CLI and authored-skill artifacts are verification prerequisites only.

Verification launches the local setup CLI and mutates fixture-owned files. It does not install unrelated tools, contact an external application, or execute a reverse-engineering provider.

Evidence and regression coverage

The historical recovery defect is documented in #2958. This PR preserves additional non-UTF-8, historical-backup, pre-publication verification, and idempotence coverage against the implementation that is now on main.

The filesystem cases read the actual snapshot bytes and inspect the production transaction result. The public CLI case additionally proves that the repair completes through the user-facing command and that the selected bundle is aligned afterward. A text-only snapshot comparison, a recovery path pointing at the historical backup, or publication before successful snapshot readback would violate these assertions.

Validation performed

The branch normally merges main f75b1a416ba67b8421e7dbd248160afedb79581f and retains the maintainer's previous merge. Verification used Node.js 24.18.0 and npm 11.16.0 on Windows x64.

npm run test:focused -- tests/boundary/filesystem/setupSkill.test.ts tests/acceptance/setup/selectedSkillSetup.test.ts — 28 tests passed; four POSIX permission-mode cases were skipped on Windows. All six public CLI cases passed.
npm run check — passed typechecking, lint and resolved module boundaries, formatting, unused-code checks, package metadata checks, and architecture guards.
Runtime and authored-skill build prerequisites completed for the acceptance tests.
Normal pre-commit formatting/lint tasks — passed during the merge commit; git diff --check origin/main...HEAD passed and the working tree is clean.

Windows x64 verification includes the production filesystem workflow and the actual local setup CLI. POSIX permission-bit assertions are not claimed as executed on Windows; platform-specific skips are reported with the results. No native provider or other operating-system installation qualification is claimed.

The current-head GitHub CI required check has passed, including the source tests, build, static checks, and portable test suite.

Compatibility, safety, and release

The remaining diff changes tests only. It adds no runtime migration, package dependency, setup effect, or release metadata. Snapshot and rollback fixtures remain confined to their owned temporary directories, and existing main coverage is retained.

Author readiness

Human owner: @dakjdakd

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

The remaining diff has one focused recovery-regression outcome.
The related issue and the implementation already on main are identified above.
Exact bytes, recovery-path identity, rollback, and idempotence are covered.
The final diff is limited to the two setup test files.
Required checks pass for the current PR revision.

_{🔄 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/2959 **Author:** [@dakjdakd](https://github.com/dakjdakd) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/skill-recovery-snapshots` --- ### 📝 Commits (4) - [`3f13722`](https://github.com/morluto/rea/commit/3f13722a302301192b3afe0f4222f06eaf5b9fd3) fix(setup): preserve verified current skill recovery snapshots - [`425bcdf`](https://github.com/morluto/rea/commit/425bcdff57cd81637d9a413611915a91af7f19f5) fix(setup): retain original skill bytes through recovery - [`41fd0ab`](https://github.com/morluto/rea/commit/41fd0abe1b6158e8d82f8ee18333f1398b40c5c7) Merge remote-tracking branch 'origin/main' into HEAD - [`d5bce77`](https://github.com/morluto/rea/commit/d5bce779b91b4efc77056a6c47af2992ab5c8783) test(setup): merge current main with skill recovery coverage ### 📊 Changes **2 files changed** (+289 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `tests/acceptance/setup/selectedSkillSetup.test.ts` (+37 -3) 📝 `tests/boundary/filesystem/setupSkill.test.ts` (+252 -0) </details> ### 📄 Description ## Summary Add boundary and public CLI regression coverage for byte-exact managed-skill recovery snapshots. The implementation from #2958 is already on main in [ec56c1da0](https://github.com/morluto/rea/commit/ec56c1da04c6c25825721a45e596e8cc237a6dd3). The remaining PR changes only `tests/boundary/filesystem/setupSkill.test.ts` and `tests/acceptance/setup/selectedSkillSetup.test.ts`. The added cases protect three related guarantees: snapshots preserve the current file's exact bytes, an older first backup is retained without being mistaken for the current recovery copy, and setup verifies a snapshot before publishing any replacement file. The CLI case also checks that repairing a non-UTF-8 reference leaves the selected skill aligned and that the next invocation is idempotent. Related issue: #2958, already resolved by the implementation on main. ## Problem and expected behavior A managed skill can already have a `.rea.backup` from its first installation when a later repair starts. If that backup contains V0 and the live file contains V2, a V2-to-current repair needs a verified V2 recovery copy. Reporting V0 as the current transaction's recovery location would lose the relationship between the file that was replaced and the file the user can recover. Byte identity also matters independently of text validity. A skill file can contain bytes from a legacy editor encoding or damaged content. Decoding those bytes as UTF-8 and re-encoding the result does not preserve the original. Setup must be able to replace the damaged file while keeping its exact previous bytes available, and a successful rollback must write those same bytes back. The implementation on main handles these guarantees through verified snapshots and original byte buffers. These tests exercise the filesystem and CLI outcomes, including transaction error reporting, instead of introducing another snapshot-selection rule. ## Change and scope The filesystem tests create isolated managed-skill directories and exercise `installCanonicalSkill` through its production implementation. New cases cover: - Repair of non-UTF-8 files both with and without a historical first backup. The snapshot is read as bytes and compared directly to the original buffer, and the installed file is compared to the authored bundle. - Recovery after a publication rejection, with separate successful-rollback and retained-recovery cases. Successful rollback restores the original bytes; when rollback cannot complete, the reported recovery path still points to an exact copy of the current original rather than the historical backup. - Preservation of the first backup and reuse of a verified, content-addressed current snapshot. An aligned rerun returns `unchanged`; repairing the same original again reuses the existing snapshot without rewriting it. - A historical first backup alongside a current V2 snapshot. The returned recovery path and message refer to V2, while the V0 first backup remains intact. - Occupied snapshot locations, including directories and an existing recovery file with different bytes. Setup rejects the backup stage before publishing the manifest or reference file. - Readback verification after a backup copy. A fixture that changes the copied bytes is rejected before the writer is called, and the original managed file remains intact. The publication and copy fault cases are deterministic transaction fixtures around real files. They check the production owner's failure semantics; they do not simulate a successful filesystem result or report a fault fixture as a developer test failure. The acceptance test extends the existing selected-skill public CLI workflow. It writes a known non-UTF-8 byte sequence into an installed reference while keeping the historical first backup, invokes the actual setup command, and checks `status: "ready"` with `appliedActions: ["installed_skill"]`. It verifies complete bundle alignment, the unchanged first backup, and the byte-exact content-addressed recovery file. The next CLI invocation returns `status: "ready"` with an empty `appliedActions` array. Existing assertions continue to protect the selected install location and caller configuration. The updated branch retains main's additional setup tests and imports alongside this coverage. All three production `Setup*.ts` files remain identical to main. This is test-only coverage for the existing installation and recovery authority. The existing linked-skill CLI cases now inspect parsed JSON fields for the rejected link's path. On Windows, the serialized JSON representation escapes path separators; comparing an unescaped filesystem path to that representation is not a reliable check of the reported identity. The assertions check `remediation` and the doctor's `issues` entry with the exact path and `symbolic_link` kind. They continue checking the nonzero result, `skill_install_failed`, no applied actions, the unmanaged state, and preservation of the link. ## Contract and boundary impact - Semantic owner and earliest changed stage: regression coverage at managed-skill snapshot preparation, publication, rollback, and public selected-skill setup. - Contribution scope and maintainer decision: tests for the existing setup workflow; no new-capability scope decision is applicable. - CLI and MCP/tool-catalog contract: unchanged. The acceptance case checks existing setup JSON fields, and the boundary cases check existing backup-stage and recovery-path facts. - Provider, bridge, target-format, or platform compatibility: no provider, bridge, or target-format change. Exact-byte comparisons do not depend on a file being valid UTF-8. - Evidence, artifact, provenance, or reconstruction contract: assertions bind each recovery copy to the current original bytes and its actual path; no output schema is added. - Target selection and cleanup: tests use fixture-owned temporary directories and the existing public selected-install workflow. They do not write a real user skill directory or broaden setup authority. - Generated metadata, package, or installation impact: no package dependencies, installer implementation, or generated files are committed. Built CLI and authored-skill artifacts are verification prerequisites only. Verification launches the local setup CLI and mutates fixture-owned files. It does not install unrelated tools, contact an external application, or execute a reverse-engineering provider. ## Evidence and regression coverage The historical recovery defect is documented in #2958. This PR preserves additional non-UTF-8, historical-backup, pre-publication verification, and idempotence coverage against the implementation that is now on main. The filesystem cases read the actual snapshot bytes and inspect the production transaction result. The public CLI case additionally proves that the repair completes through the user-facing command and that the selected bundle is aligned afterward. A text-only snapshot comparison, a recovery path pointing at the historical backup, or publication before successful snapshot readback would violate these assertions. ## Validation performed The branch normally merges main `f75b1a416ba67b8421e7dbd248160afedb79581f` and retains the maintainer's previous merge. Verification used Node.js 24.18.0 and npm 11.16.0 on Windows x64. - `npm run test:focused -- tests/boundary/filesystem/setupSkill.test.ts tests/acceptance/setup/selectedSkillSetup.test.ts` — 28 tests passed; four POSIX permission-mode cases were skipped on Windows. All six public CLI cases passed. - `npm run check` — passed typechecking, lint and resolved module boundaries, formatting, unused-code checks, package metadata checks, and architecture guards. - Runtime and authored-skill build prerequisites completed for the acceptance tests. - Normal pre-commit formatting/lint tasks — passed during the merge commit; `git diff --check origin/main...HEAD` passed and the working tree is clean. Windows x64 verification includes the production filesystem workflow and the actual local setup CLI. POSIX permission-bit assertions are not claimed as executed on Windows; platform-specific skips are reported with the results. No native provider or other operating-system installation qualification is claimed. The current-head GitHub `CI required` check has passed, including the source tests, build, static checks, and portable test suite. ## Compatibility, safety, and release The remaining diff changes tests only. It adds no runtime migration, package dependency, setup effect, or release metadata. Snapshot and rollback fixtures remain confined to their owned temporary directories, and existing main coverage is retained. ## Author readiness Human owner: @dakjdakd - [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] The remaining diff has one focused recovery-regression outcome. - [x] The related issue and the implementation already on main are identified above. - [x] Exact bytes, recovery-path identity, rollback, and idempotence are covered. - [x] The final diff is limited to the two setup test files. - [ ] Required checks pass for the current PR revision. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:07:16 +00:00