morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 07:47:29 +00:00

[PR #3108] [MERGED] fix(setup): refuse linked skill installation directories #5621

New issue

Closed

opened 2026-10-31 01:08:03 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:08:03 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3108
Author: @geekwolf
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/14/2026
Merged by: @N0zoM1z0

Base: main ← Head: fix/skill-symlink-installation

📝 Commits (8)

5f4f8fb fix(setup): refuse linked skill installation directories
4263051 test(setup): require a concrete doctor failure result
852111d chore(setup): integrate skill-directory refusal with current main
ffad00c fix(setup): admit selected skill directories before changes
ef45d98 chore(setup): synchronize directory verification with latest main
a03e6bf chore(setup): synchronize the upstream PCAP CI repair
f4cca10 fix(setup): retain unreadable skill facts after drift
36b93d5 fix(setup): recognize absent skill files before reading bundled bytes

📊 Changes

12 files changed (+718 additions, -89 deletions)

View changed files

📝 docs/installation.md (+10 -0)
📝 src/application/Doctor.ts (+52 -18)
📝 src/application/DoctorScope.ts (+10 -2)
📝 src/application/Setup.fixture.ts (+8 -1)
📝 src/application/Setup.planning.test.ts (+91 -0)
📝 src/application/Setup.ts (+20 -4)
📝 src/application/SetupHost.ts (+8 -0)
📝 src/application/SetupSkill.ts (+193 -61)
📝 src/application/SetupTypes.ts (+9 -1)
📝 src/application/Uninstall.ts (+6 -0)
📝 tests/acceptance/setup/selectedSkillSetup.test.ts (+181 -2)
📝 tests/boundary/filesystem/setupSkill.test.ts (+130 -0)

📄 Description

Summary

Prevent setup from writing through a linked skill root or references directory. Validate the final selected destinations before preview/approval, again before any client/Hopper changes, and at skill installation. Fixes #3057.

Problem and expected behavior

Setup could write into a symbolic link's target. The initial PR refused the skill write, but combined setup still configured the client first. An already canonical linked copy also appeared installable/aligned. Doctor must distinguish unmanaged directories and unreadable observations from missing files; retained uninstall files must remain reported as retained.

Change and scope

Shared typed directory observations and single-pass canonical/read facts own the policy. Every selected managed file remains examined after a mismatch or expected read failure; observed metadata and all unreadable paths survive stale/absent manifests. Cleanup/resource failures retain their owner. Duplicate manifest reads are removed. Absent installed files are classified before any bundled-byte comparison, so source-only initial snapshots do not require generated artifacts. Final selection is admitted before side effects, preserving interactive choices for unselected clients and registration-only --skill=false. Doctor reports unmanaged/unknown with paths, reasons and alternatives. Update excludes linked roots/references; uninstall propagates retained child outcomes and preserves ownership manifests.

The explicit unmanaged doctor direction also incorporates #3109, credited in the follow-up commit. This is an ordinary correction within existing installation authority; a new capability scope decision is not applicable.

Contract and boundary impact

Owner: shared skill filesystem inspection, consumed by setup/doctor/update; uninstall owns aggregation of retained cleanup.
CLI/MCP: existing setup failure code skill_install_failed; doctor adds explicit unmanaged/unknown skill states and optional issues. Public stdio MCP doctor consumers were tested.
Providers and Evidence: no target analysis or provider protocol changes. Paths, known installed metadata, inspection reasons and cleanup ownership remain visible.
Selection and authority: only final selected destinations block setup; --skill=false remains available. Readiness/freshness does not establish permission to install.
Installation: normal directories retain install/rerun/repair behavior. Linked directories require regular destinations or registration-only setup. Generated skill/catalog outputs are not committed.

Expected effects: reads selected existing files; normal approved setup may write skill/configuration files and use existing Hopper installation behavior. No new provider, target execution, instrumentation or protection recovery is added.

Evidence and regression coverage

Clean main ec67aebed: three original regressions fail; initial PR: 19 focused tests pass. Two additional actual-CLI preflight tests fail on the initial PR because it creates Claude configuration before refusal.
Final selected setup returns needs_human, code: "skill_install_failed", and appliedActions: [] for unsafe roots/references. Doctor reports identity.skill.state: "unmanaged"; unreadable reads remain unknown with reasons.
Tests cover target bytes/link preservation, normal install/rerun/repair, registration-only setup, retained manifests, rollback, permissions, unselected-client choices and directory changes during approval.
No arbitrary concurrent directory-swap containment claim. Native Windows execution of this follow-up, real client-agent sessions, global package update and native Hopper execution remain unverified locally. The original author reported Windows junction coverage for the initial patch.

Validation performed

Final reviewed head 36b93d512, based on main 29d3dcb7e. Linux / Node 24.18.0 / npm 11.16.0:

Original follow-up: 16 files / 205 passes; latest-main lifecycle, actual stdio MCP doctor, update maintenance and CLI: four files / 95 passes, repeated after main synchronization.
Review uncovered early mismatch/first-error/absent-manifest exits hiding later unreadable references. Three real permission regressions fail on a03e6bf9c. Related selection: 17 files / 217 passes.
Follow-up f4cca100d exposed an own source-CI regression: missing installed files unnecessarily requested generated bundled bytes. A clean no-dist/no-skills checkout reproduces exactly 5 passes / one failure in setupEnvironment; the final absence-first correction passes all six. Full no-artifact source suite: 508 files / 7,446 passes / eight existing platform/condition skips. Production source matches final head byte-for-byte. Final focused environment/filesystem/cleanup/actual CLI/MCP selection: five files / 41 passes.
Full npm run check passes for the single-pass follow-up; final absence correction passes check:fast and git diff --check. Earlier docs:check (five consistency tests) passes. The identity-reader complexity warning is resolved. Module length remains one nonblocking warning (585 nonblank lines); related filesystem ownership stays in one module, with no forwarding layers or relaxed rules.
Four equivalent public CLI install/rerun/repair/uninstall tasks succeed on baseline/fixed isolated profiles. Each four-call task emits 9,758 o200k output tokens (9,850 for the explicit CLI transcript proxy). Main cold/warm: 9.60/8.04 s, CPU 10.05/9.01 s, family RSS 375/357 MiB. Fixed: 7.71/7.45 s, CPU 8.88/8.52 s, RSS 350/385 MiB. Individual trials with isolated Node compile caches and warm OS caches; no speedup or billed/whole-agent token reduction claim.
Final 36b93d512 compared against a03e6bf9c: four equivalent complete CLI tasks keep identical 10,668 output tokens (10,760 explicit transcript proxy). Before cold/warm: 10.70/9.88 s, CPU 11.74/10.58 s, RSS 390/385 MiB; fixed: 10.30/10.04 s, CPU 11.08/10.93 s, RSS 386/351 MiB. Individual trials, isolated compile caches, warm OS caches; no speedup, statistical equivalence or billed-token claim.
Two-CPU / 8-GiB sampled-family-RSS guard; peak verification about 4.4 GiB, no guard stop. Probes used owned profiles and minimal child environments with HOME omitted.
Prior a03e6bf9c CI passes all five shards, static/source/docs/build and all four native package platforms. Its review finding is corrected and resolved with regression evidence. Previous main PCAP blocker is repaired upstream in 1341e8c83 (clean main: 20 passes); duplicate repair PR #3257 was closed as already incorporated. Greptile passed f4cca100d with no remaining threads, but its own source-CI failure is fixed above. Final 36b93d512 CI run 38528527903 passes all five shards, source/static/docs/build, four native package platforms and aggregate gates. Greptile passes, with no unresolved review threads. This is current-head validation.

Compatibility and release

No dependency, package/release metadata or target-format change. Existing normal setup calls keep their behavior; linked skill directories now receive actionable refusal. Transport credentials and ambient environments are not persisted. No generated artifacts are committed.

Before merge

Concrete problem, linked issue and resolved existing installation authority.
Focused outcome, Conventional Commit title and reviewed final diff.
Observable CLI/MCP behavior, meaningful failure coverage and owning docs updated.
Evidence and verification limits preserved.
Required CI and review pass for the current head.

_{🔄 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/3108 **Author:** [@geekwolf](https://github.com/geekwolf) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/14/2026 **Merged by:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Base:** `main` ← **Head:** `fix/skill-symlink-installation` --- ### 📝 Commits (8) - [`5f4f8fb`](https://github.com/morluto/rea/commit/5f4f8fbed838563006b60247a2ba565865b722ee) fix(setup): refuse linked skill installation directories - [`4263051`](https://github.com/morluto/rea/commit/42630519cd2b90b759f9d24bc88813741200c6eb) test(setup): require a concrete doctor failure result - [`852111d`](https://github.com/morluto/rea/commit/852111daea9c9a9246529d6a4cfe0359286b2604) chore(setup): integrate skill-directory refusal with current main - [`ffad00c`](https://github.com/morluto/rea/commit/ffad00ca8ee3d51e39d87acc0e62861cc92f9333) fix(setup): admit selected skill directories before changes - [`ef45d98`](https://github.com/morluto/rea/commit/ef45d981ce6f827b42f96d87ddd545e9cc3d0f28) chore(setup): synchronize directory verification with latest main - [`a03e6bf`](https://github.com/morluto/rea/commit/a03e6bf9cde3c9fb375f42cc7faa0e9db40af6c4) chore(setup): synchronize the upstream PCAP CI repair - [`f4cca10`](https://github.com/morluto/rea/commit/f4cca100d5a32ce377b04328e6858ba5184aa0b0) fix(setup): retain unreadable skill facts after drift - [`36b93d5`](https://github.com/morluto/rea/commit/36b93d5128b2f2de0422b09ced62d6cf3f2d1901) fix(setup): recognize absent skill files before reading bundled bytes ### 📊 Changes **12 files changed** (+718 additions, -89 deletions) <details> <summary>View changed files</summary> 📝 `docs/installation.md` (+10 -0) 📝 `src/application/Doctor.ts` (+52 -18) 📝 `src/application/DoctorScope.ts` (+10 -2) 📝 `src/application/Setup.fixture.ts` (+8 -1) 📝 `src/application/Setup.planning.test.ts` (+91 -0) 📝 `src/application/Setup.ts` (+20 -4) 📝 `src/application/SetupHost.ts` (+8 -0) 📝 `src/application/SetupSkill.ts` (+193 -61) 📝 `src/application/SetupTypes.ts` (+9 -1) 📝 `src/application/Uninstall.ts` (+6 -0) 📝 `tests/acceptance/setup/selectedSkillSetup.test.ts` (+181 -2) 📝 `tests/boundary/filesystem/setupSkill.test.ts` (+130 -0) </details> ### 📄 Description ## Summary Prevent setup from writing through a linked skill root or `references` directory. Validate the final selected destinations before preview/approval, again before any client/Hopper changes, and at skill installation. Fixes #3057. ## Problem and expected behavior Setup could write into a symbolic link's target. The initial PR refused the skill write, but combined setup still configured the client first. An already canonical linked copy also appeared installable/aligned. Doctor must distinguish unmanaged directories and unreadable observations from missing files; retained uninstall files must remain reported as retained. ## Change and scope Shared typed directory observations and single-pass canonical/read facts own the policy. Every selected managed file remains examined after a mismatch or expected read failure; observed metadata and all unreadable paths survive stale/absent manifests. Cleanup/resource failures retain their owner. Duplicate manifest reads are removed. Absent installed files are classified before any bundled-byte comparison, so source-only initial snapshots do not require generated artifacts. Final selection is admitted before side effects, preserving interactive choices for unselected clients and registration-only `--skill=false`. Doctor reports `unmanaged`/`unknown` with paths, reasons and alternatives. Update excludes linked roots/references; uninstall propagates retained child outcomes and preserves ownership manifests. The explicit unmanaged doctor direction also incorporates #3109, credited in the follow-up commit. This is an ordinary correction within existing installation authority; a new capability scope decision is not applicable. ## Contract and boundary impact - Owner: shared skill filesystem inspection, consumed by setup/doctor/update; uninstall owns aggregation of retained cleanup. - CLI/MCP: existing setup failure code `skill_install_failed`; doctor adds explicit `unmanaged`/`unknown` skill states and optional issues. Public stdio MCP doctor consumers were tested. - Providers and Evidence: no target analysis or provider protocol changes. Paths, known installed metadata, inspection reasons and cleanup ownership remain visible. - Selection and authority: only final selected destinations block setup; `--skill=false` remains available. Readiness/freshness does not establish permission to install. - Installation: normal directories retain install/rerun/repair behavior. Linked directories require regular destinations or registration-only setup. Generated skill/catalog outputs are not committed. Expected effects: reads selected existing files; normal approved setup may write skill/configuration files and use existing Hopper installation behavior. No new provider, target execution, instrumentation or protection recovery is added. ## Evidence and regression coverage - Clean main `ec67aebed`: three original regressions fail; initial PR: 19 focused tests pass. Two additional actual-CLI preflight tests fail on the initial PR because it creates Claude configuration before refusal. - Final selected setup returns `needs_human`, `code: "skill_install_failed"`, and `appliedActions: []` for unsafe roots/references. Doctor reports `identity.skill.state: "unmanaged"`; unreadable reads remain `unknown` with reasons. - Tests cover target bytes/link preservation, normal install/rerun/repair, registration-only setup, retained manifests, rollback, permissions, unselected-client choices and directory changes during approval. - No arbitrary concurrent directory-swap containment claim. Native Windows execution of this follow-up, real client-agent sessions, global package update and native Hopper execution remain unverified locally. The original author reported Windows junction coverage for the initial patch. ## Validation performed Final reviewed head `36b93d512`, based on main `29d3dcb7e`. Linux / Node 24.18.0 / npm 11.16.0: - Original follow-up: 16 files / 205 passes; latest-main lifecycle, actual stdio MCP doctor, update maintenance and CLI: four files / 95 passes, repeated after main synchronization. - Review uncovered early mismatch/first-error/absent-manifest exits hiding later unreadable references. Three real permission regressions fail on `a03e6bf9c`. Related selection: 17 files / 217 passes. - Follow-up `f4cca100d` exposed an own source-CI regression: missing installed files unnecessarily requested generated bundled bytes. A clean no-`dist`/no-`skills` checkout reproduces exactly 5 passes / one failure in `setupEnvironment`; the final absence-first correction passes all six. Full no-artifact source suite: 508 files / 7,446 passes / eight existing platform/condition skips. Production source matches final head byte-for-byte. Final focused environment/filesystem/cleanup/actual CLI/MCP selection: five files / 41 passes. - Full `npm run check` passes for the single-pass follow-up; final absence correction passes `check:fast` and `git diff --check`. Earlier `docs:check` (five consistency tests) passes. The identity-reader complexity warning is resolved. Module length remains one nonblocking warning (585 nonblank lines); related filesystem ownership stays in one module, with no forwarding layers or relaxed rules. - Four equivalent public CLI install/rerun/repair/uninstall tasks succeed on baseline/fixed isolated profiles. Each four-call task emits 9,758 o200k output tokens (9,850 for the explicit CLI transcript proxy). Main cold/warm: 9.60/8.04 s, CPU 10.05/9.01 s, family RSS 375/357 MiB. Fixed: 7.71/7.45 s, CPU 8.88/8.52 s, RSS 350/385 MiB. Individual trials with isolated Node compile caches and warm OS caches; no speedup or billed/whole-agent token reduction claim. - Final `36b93d512` compared against `a03e6bf9c`: four equivalent complete CLI tasks keep identical 10,668 output tokens (10,760 explicit transcript proxy). Before cold/warm: 10.70/9.88 s, CPU 11.74/10.58 s, RSS 390/385 MiB; fixed: 10.30/10.04 s, CPU 11.08/10.93 s, RSS 386/351 MiB. Individual trials, isolated compile caches, warm OS caches; no speedup, statistical equivalence or billed-token claim. - Two-CPU / 8-GiB sampled-family-RSS guard; peak verification about 4.4 GiB, no guard stop. Probes used owned profiles and minimal child environments with HOME omitted. - Prior `a03e6bf9c` CI passes all five shards, static/source/docs/build and all four native package platforms. Its review finding is corrected and [resolved with regression evidence](https://github.com/morluto/rea/pull/3108#discussion_r4267804498). Previous main PCAP blocker is repaired upstream in `1341e8c83` (clean main: 20 passes); duplicate repair PR #3257 was closed as already incorporated. Greptile passed `f4cca100d` with no remaining threads, but its own source-CI failure is fixed above. Final `36b93d512` [CI run 38528527903](https://github.com/morluto/rea/actions/runs/38528527903) passes all five shards, source/static/docs/build, four native package platforms and aggregate gates. Greptile passes, with no unresolved review threads. This is current-head validation. ## Compatibility and release No dependency, package/release metadata or target-format change. Existing normal setup calls keep their behavior; linked skill directories now receive actionable refusal. Transport credentials and ambient environments are not persisted. No generated artifacts are committed. ## Before merge - [x] Concrete problem, linked issue and resolved existing installation authority. - [x] Focused outcome, Conventional Commit title and reviewed final diff. - [x] Observable CLI/MCP behavior, meaningful failure coverage and owning docs updated. - [x] Evidence and verification limits preserved. - [x] Required CI and review pass for the current head. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:08:03 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:44:37 +00:00

[PR #5621] [MERGED] fix(browser): capture the full visual viewport #7000

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#5621

No description provided.

Rows
Columns