[PR #5936] test(setup): cover skill aliases and independent cleanup retries #7108

Open
opened 2026-11-01 21:45:26 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5936
Author: @dakjdakd
Created: 10/21/2026
Status: 🔄 Open

Base: main ← Head: fix/setup-skill-rollback-ownership


📝 Commits (5)

  • 111fc5b fix(setup): preserve concurrent skill edits during rollback
  • 0213368 chore: merge upstream main into setup skill fix
  • 97f2495 fix(setup): preserve primary skill failure during cleanup
  • 8ec8484 fix(setup): verify skill publication at its canonical destination
  • e4d312b test(setup): cover skill aliases and independent cleanup retries

📊 Changes

1 file changed (+171 additions, -0 deletions)

View changed files

➕ tests/boundary/filesystem/setupSkillPublicationLifecycle.test.ts (+171 -0)

📄 Description

Summary

Related to #5928. Adds six filesystem regression cases for the current SkillFilePublication owner: installation through a home-directory alias, and independent cleanup retries after either a verified installation or a later publication failure.

Main already addresses the original ownership and recovery problem in 211e3533b. Following the maintainer's scope request, this revision removes the superseded production changes from the PR diff and exercises that implementation directly. These are passing regression controls against main, rather than a claim of another reproduced runtime defect.

Problem and expected behavior

Setup installs a bundle of managed skill files. Its file-publication owner has to reconcile two different concerns: the destination must refer to the intended physical directory, and every acquired cleanup lease must remain reachable until release succeeds.

A home directory can be reached through a directory symlink, including a Windows junction. Installing through that spelling should produce the same complete bundle seen through the canonical home path. An upgrade must preserve the previous manifest in its recovery backup, and a repeat installation should report unchanged without replacing the alias.

Cleanup also needs to remain independent across files. If two publications cannot release their leases, retrying one successfully must not discard the other. This matters both after successful bundle verification and when a subsequent publication fails. In the latter case, the original write error must remain the primary diagnostic while earlier files are restored and the cleanup failures retain their own resource paths. Retrying cleanup must leave those restored files alone. After a verified installation, cleanup retries must likewise preserve the installed canonical bundle.

Change and scope

The final diff adds only tests/boundary/filesystem/setupSkillPublicationLifecycle.test.ts. It uses the real installCanonicalSkill, SkillFilePublication.prepare, publication writes, backups, and filesystem readback.

The six cases cover:

  • Fresh installation through a home-directory alias.
  • An upgrade through the alias with an existing manifest and missing references. Both alias cases check freshness through both path spellings, idempotence, and preservation of the alias; the upgrade also checks the original manifest backup.
  • Two independently blocked cleanup leases after a verified installation, without a resource scope.
  • The same verified-installation case with an ArtifactResourceScope.
  • Two independently blocked cleanup leases when preparation of the third publication throws, without a resource scope.
  • The same later-publication failure with an ArtifactResourceScope.

The cleanup cases seed every managed destination with known original contents. Only the first two real leases' release methods are gated; the test does not replace the publication implementation. It checks both attributed cleanup failures and both retained owners, unblocks one lease, confirms that the other remains pending, then releases the second. Before and after retries it verifies either the restored original contents or the canonical installed bundle. The write-failure cases additionally preserve the original cause object, reason, and write stage.

Contract and boundary impact

  • Semantic owner: the existing SkillFilePublication filesystem owner and its SetupSkill consumer; production behavior is unchanged.
  • Contribution scope: focused regression coverage within the existing setup lifecycle, as discussed in the linked maintainer comment. No additional capability or authority.
  • CLI, MCP/tool catalog, provider, bridge, evidence, and target-format contracts: unchanged.
  • Resource lifecycle: verifies retained ownership across independent release failures and partial retries, using both the returned error's retry method and the shared resource scope.
  • Generated metadata, package contents, installation code, and release metadata: unchanged. Test execution writes only isolated temporary fixtures and generated local build prerequisites.

Evidence and regression coverage

The six new cases pass against main at 79b8219f290ef6304e469d9b8c40dc2af30edba9. They cover directory identity and the final-cleanup/retry lifecycle through the production filesystem boundary. They supplement the owner controls already introduced on main; no new production fix is required by these cases.

Local execution used Windows x64, Node.js 24.22.1, and npm 11.16.0. The alias tests exercised Windows junctions. POSIX directory-symlink execution remains for CI; this local run does not establish macOS or Linux results. No real reverse-engineering provider or target interaction is involved.

Validation performed

  • npm run test:focused -- tests/boundary/filesystem/setupSkillPublicationLifecycle.test.ts — all six tests passed; required runtime and skill build preparation also passed.

  • npm run verify:test-discovery — all 2,641 test files were discovered exactly once.

  • npm run check:fast — typechecking and lint, including module-boundary verification, passed.

  • git diff --cached --check — passed.

  • npm run check — formatting and metadata checks passed, but the complete gate is blocked by main's unused staticFileUrlPath export and ObjcClassImportBindingCoverage type. Running npm run knip with the new test removed reports the same two findings. Full-gate completion remains a validation limitation for this base revision.

  • CI run 40440959853 passed static checks, package build, documentation, the three native package baselines, PR readiness, and title validation. Completion of the full gate is blocked by Apple inventory/provider-binding, Electron, SAZ, and Java-probe test failures also present in the main baseline run at 79b8219f. Several test shards were cancelled, so this run does not establish complete cross-platform execution of the six new cases.

Compatibility, safety, and release

No breaking changes, migration, production permission changes, or package metadata changes. Cleanup errors are deliberately injected into test-owned leases to check retention and retry behavior. Every test unblocks and closes its retained owners before its temporary directory is removed.

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

  • 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.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked above.
  • Tests cover the selected observable behavior and meaningful failure paths.
  • Owning contracts and generated metadata were checked where applicable.
  • The final diff was checked for unrelated cleanup.

🔄 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/5936 **Author:** [@dakjdakd](https://github.com/dakjdakd) **Created:** 10/21/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/setup-skill-rollback-ownership` --- ### 📝 Commits (5) - [`111fc5b`](https://github.com/morluto/rea/commit/111fc5b68b14a96778aa49dc0aeb81c37d05935f) fix(setup): preserve concurrent skill edits during rollback - [`0213368`](https://github.com/morluto/rea/commit/02133688f40116c16d570f411a10183b770b3e3b) chore: merge upstream main into setup skill fix - [`97f2495`](https://github.com/morluto/rea/commit/97f24953471c8ea1801d0eba589ee0f9995de619) fix(setup): preserve primary skill failure during cleanup - [`8ec8484`](https://github.com/morluto/rea/commit/8ec8484c357c301ea5fdcb8ae84f72d1dab66729) fix(setup): verify skill publication at its canonical destination - [`e4d312b`](https://github.com/morluto/rea/commit/e4d312b189d2bbc6f905e86100e12492d4510d33) test(setup): cover skill aliases and independent cleanup retries ### 📊 Changes **1 file changed** (+171 additions, -0 deletions) <details> <summary>View changed files</summary> ➕ `tests/boundary/filesystem/setupSkillPublicationLifecycle.test.ts` (+171 -0) </details> ### 📄 Description ## Summary Related to #5928. Adds six filesystem regression cases for the current `SkillFilePublication` owner: installation through a home-directory alias, and independent cleanup retries after either a verified installation or a later publication failure. Main already addresses the original ownership and recovery problem in [211e3533b](https://github.com/morluto/rea/commit/211e3533b34f81ae0910ad4f53f2a3d3c8587d3e). Following the [maintainer's scope request](https://github.com/morluto/rea/pull/5936#issuecomment-6428415060), this revision removes the superseded production changes from the PR diff and exercises that implementation directly. These are passing regression controls against main, rather than a claim of another reproduced runtime defect. ## Problem and expected behavior Setup installs a bundle of managed skill files. Its file-publication owner has to reconcile two different concerns: the destination must refer to the intended physical directory, and every acquired cleanup lease must remain reachable until release succeeds. A home directory can be reached through a directory symlink, including a Windows junction. Installing through that spelling should produce the same complete bundle seen through the canonical home path. An upgrade must preserve the previous manifest in its recovery backup, and a repeat installation should report `unchanged` without replacing the alias. Cleanup also needs to remain independent across files. If two publications cannot release their leases, retrying one successfully must not discard the other. This matters both after successful bundle verification and when a subsequent publication fails. In the latter case, the original write error must remain the primary diagnostic while earlier files are restored and the cleanup failures retain their own resource paths. Retrying cleanup must leave those restored files alone. After a verified installation, cleanup retries must likewise preserve the installed canonical bundle. ## Change and scope The final diff adds only `tests/boundary/filesystem/setupSkillPublicationLifecycle.test.ts`. It uses the real `installCanonicalSkill`, `SkillFilePublication.prepare`, publication writes, backups, and filesystem readback. The six cases cover: - Fresh installation through a home-directory alias. - An upgrade through the alias with an existing manifest and missing references. Both alias cases check freshness through both path spellings, idempotence, and preservation of the alias; the upgrade also checks the original manifest backup. - Two independently blocked cleanup leases after a verified installation, without a resource scope. - The same verified-installation case with an `ArtifactResourceScope`. - Two independently blocked cleanup leases when preparation of the third publication throws, without a resource scope. - The same later-publication failure with an `ArtifactResourceScope`. The cleanup cases seed every managed destination with known original contents. Only the first two real leases' `release` methods are gated; the test does not replace the publication implementation. It checks both attributed cleanup failures and both retained owners, unblocks one lease, confirms that the other remains pending, then releases the second. Before and after retries it verifies either the restored original contents or the canonical installed bundle. The write-failure cases additionally preserve the original cause object, reason, and write stage. ## Contract and boundary impact - Semantic owner: the existing `SkillFilePublication` filesystem owner and its `SetupSkill` consumer; production behavior is unchanged. - Contribution scope: focused regression coverage within the existing setup lifecycle, as discussed in the linked maintainer comment. No additional capability or authority. - CLI, MCP/tool catalog, provider, bridge, evidence, and target-format contracts: unchanged. - Resource lifecycle: verifies retained ownership across independent release failures and partial retries, using both the returned error's retry method and the shared resource scope. - Generated metadata, package contents, installation code, and release metadata: unchanged. Test execution writes only isolated temporary fixtures and generated local build prerequisites. ## Evidence and regression coverage The six new cases pass against main at `79b8219f290ef6304e469d9b8c40dc2af30edba9`. They cover directory identity and the final-cleanup/retry lifecycle through the production filesystem boundary. They supplement the owner controls already introduced on main; no new production fix is required by these cases. Local execution used Windows x64, Node.js 24.22.1, and npm 11.16.0. The alias tests exercised Windows junctions. POSIX directory-symlink execution remains for CI; this local run does not establish macOS or Linux results. No real reverse-engineering provider or target interaction is involved. ## Validation performed - `npm run test:focused -- tests/boundary/filesystem/setupSkillPublicationLifecycle.test.ts` — all six tests passed; required runtime and skill build preparation also passed. - `npm run verify:test-discovery` — all 2,641 test files were discovered exactly once. - `npm run check:fast` — typechecking and lint, including module-boundary verification, passed. - `git diff --cached --check` — passed. - `npm run check` — formatting and metadata checks passed, but the complete gate is blocked by main's unused `staticFileUrlPath` export and `ObjcClassImportBindingCoverage` type. Running `npm run knip` with the new test removed reports the same two findings. Full-gate completion remains a validation limitation for this base revision. - [CI run 40440959853](https://github.com/morluto/rea/actions/runs/40440959853) passed static checks, package build, documentation, the three native package baselines, PR readiness, and title validation. Completion of the full gate is blocked by Apple inventory/provider-binding, Electron, SAZ, and Java-probe test failures also present in the [main baseline run at `79b8219f`](https://github.com/morluto/rea/actions/runs/40440009919). Several test shards were cancelled, so this run does not establish complete cross-platform execution of the six new cases. ## Compatibility, safety, and release No breaking changes, migration, production permission changes, or package metadata changes. Cleanup errors are deliberately injected into test-owned leases to check retention and retry behavior. Every test unblocks and closes its retained owners before its temporary directory is removed. ## 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] 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. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked above. - [x] Tests cover the selected observable behavior and meaningful failure paths. - [x] Owning contracts and generated metadata were checked where applicable. - [x] The final diff was checked for unrelated cleanup. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
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#7108
No description provided.