feat(plugins): enforce reviewed bundle activation

[Hmbown]

What changed

• add one deterministic, Codewhale-owned plugin inventory over user and workspace bundle roots
• keep trust separate from enablement and bind both to exact content, capabilities, workspace, and monotonic generation receipts
• activate only namespaced Skills and reviewed MCP servers through the existing engines across launch, resume, fork, exec, nested worker, and serve paths
• stage reviewed content into owner-only snapshots and fail closed on traversal, symlink, hardlink, Windows reparse/ACL, malformed-state, capability-escalation, and source/stage-drift boundaries
• cancel reviewed MCP authority on disable/revoke/generation changes, scrub child environments, disable plugin OAuth, and isolate reviewed remote transports from ambient proxies
• expose structural, secret-safe review and lifecycle commands while keeping discovery, list, show, validate, and doctor non-mutating
• document the deliberately bounded v0.9.1 surface and its explicit non-goals

Release scope

v0.9.1 activates declarative Skills and MCP bundles only. Hooks, commands, agents, LSP, native extensions, marketplaces, installers/updaters, automatic trust, and ambient compatibility imports remain inactive or deferred. Plugin trust does not claim to be an OS sandbox; local stdio servers run with the host user authority and still pass through normal MCP approval.

Security review

Independent review found five pre-merge blockers. This head now:

• sets and verifies the current Windows owner with a protected owner-only DACL before publishing state
• hardens the exact temporary state object before atomic replacement and preserves old stable state on failure
• hides plugin Skills when shared activation state is unreadable or malformed without destroying native recovery
• revalidates both source and staged hashes before exposing cached MCP catalogues
• requires complete content and capability SHA-256 values in explicit trust confirmations

Validation

• cargo fmt --all -- --check
• git diff --check
• all-target cargo clippy with warnings denied
• MCP: 174 passed, 1 pre-existing ignored
• plugin registry: 46 passed
• Skill state: 11 passed
• Runtime API plugin Skills: 2 passed
• sealed plugin E2E: 4 passed
• Windows GNU codewhale-tui --tests cross-compilation via Zig
• exact-range gitleaks: zero findings

Native Windows junction/owner/ACL tests remain an exact-head CI gate on this PR. The original plugin foundation from @pkeging in #3708, #3709, and #3710 is preserved with co-authorship; @nightt5879's startup-registry commit remains in main ancestry; @codepgq's compatibility request shaped the explicit migration boundary.

Fixes #3917
Fixes #4399

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @Hmbown's task in 1m 45s —— View job

---

Claude is reviewing this PR

Todo list

• Gather context (PR body, diff stat, commit list)
• Review correctness (panics/unwraps, error handling, exhaustive matches) — 4 parallel sub-reviews in progress across plugin core, MCP subsystem, skills/runtime wiring, and TUI/localization
• Review provider/model/route safety invariant
• Review reuse/simplification/dead code
• Review test coverage for new behavior
• Review security (secrets, exec policy, input validation, path/symlink handling)
• Post final summary

This is a large PR (~11.9k additions across 61 files), so I've split the review into 4 parallel passes: plugin core/security (manifest, registry, discovery, path_identity), MCP subsystem (mcp.rs, oauth, sse, stdio), skills/runtime wiring (skill_state, engine, main.rs entry points), and TUI/localization. Consolidating findings shortly.

[gitguardian]

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

---

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude is reviewing this PR

Todo list

• Gather context (PR body, diff stat, commit list)
• Review correctness (panics/unwraps, error handling, exhaustive matches)
• Review provider/model/route safety invariant
• Review reuse/simplification/dead code
• Review test coverage for new behavior
• Review security (secrets, exec policy, input validation, path/symlink handling)
• Post final summary

Starting with repo setup and diff analysis.

View job run

[claude]

Claude finished @Hmbown's task in 7m 37s —— View job

---

Claude finished reviewing this PR

Todo list

• Gather context (PR body, diff stat, commit list)
• Review correctness (panics/unwraps, error handling, exhaustive matches)
• Review provider/model/route safety invariant
• Review reuse/simplification/dead code
• Review test coverage for new behavior
• Review security (secrets, exec policy, input validation, path/symlink handling)
• Post final summary

Reviewed this in 4 parallel passes (plugin core/security, MCP subsystem, skills/runtime wiring, TUI/localization) against origin/main. This is a genuinely rigorous piece of security engineering — trust/enablement separation, content+capability hash binding, monotonic generation receipts, and fail-closed behavior throughout. No blockers found. Two majors worth addressing before/soon after merge, plus a few minors.

Findings

1. Major (TOCTOU) — crates/tui/src/plugins/manifest.rs:1161 (hash_path)
The directory branch of the bundle content-hashing walk uses pathname-based fs::symlink_metadata + fs::read_dir rather than an fd-anchored/O_NOFOLLOW traversal, unlike the fd-based copy_bundle_directory_fd used for staging in registry.rs. A directory swapped to a symlink between the check and the read could let content_hash — the value trust receipts are bound to — reflect bytes from outside the plugin root.
Fix this →

2. Major — crates/tui/src/skills/mod.rs:1011-1019
render_available_skills_context_for_workspace_with_mode_and_plugins never calls .into_enabled(), unlike its sibling render_available_skills_context_for_workspace_and_dir_with_mode_and_plugins (line 1064-1075), which does — so the enable/disable + fail-closed filter (hiding plugin skills when skills_state.toml is malformed/unreadable) is skipped on this path, reachable from prompts.rs:1279 when skills_dir is None. Every current production caller (core/engine.rs:1060,3741, tui/ui.rs:1745) passes Some(...), and LoadSkillTool separately re-applies .into_enabled() before actually loading a skill, so this isn't an active bypass today — but it's a real asymmetry in a function whose sibling was clearly patched for this exact purpose, and a future caller could trip over it.
Fix this →

3. Efficiency (plausible, major-ish) — crates/tui/src/mcp.rs:2530 (McpConnection::catalog_authorized / verify_plugin_authority)
Re-hashes the entire plugin bundle from disk (source + staged manifest, two full walks) synchronously on every is_ready(), all_tools(), all_resources(), all_resource_templates(), all_prompts(), and tool-name resolution call — effectively every model turn. This is blocking I/O inside async pool methods and could add latency proportional to bundle size, worse with multiple reviewed-plugin servers connected. Worth caching the verification result and invalidating on the same generation/revocation signal already used elsewhere, rather than re-hashing every call.

4. Minor — crates/tui/src/plugins/registry.rs:661 (harden_plugin_state_directory)
No-op on non-Windows, so the directory holding state.json, the lock file, and .runtime/ staging trees relies on process umask rather than an explicit 0o700, unlike every other object this file hardens (and unlike its own Windows ACL counterpart).

5. Minor (race) — crates/tui/src/mcp/stdio.rs:355 (StdioTransport::drop)
Aborts authority_cancel_watch then does a non-blocking try_lock on the child mutex; if the watcher is mid-terminate and still holds the lock, try_lock fails silently and Drop's own SIGTERM path is skipped, deferring cleanup until the aborted task's future is actually dropped (kill_on_drop is a backstop). Narrow window, not a permanent leak, but it's precisely the revoke-teardown path this PR is meant to harden — worth a comment or a blocking-lock-with-timeout guard.

6. Minor (dup logic) — crates/tui/src/main.rs:1486
Commands::Exec reimplements resolve_workspace(&cli)'s fallback inline instead of calling the shared helper. No active bug today (identical logic), but a reuse risk given Engine::new silently falls back to an empty plugin registry (fail-closed but functionality-breaking) on any workspace mismatch.

7. Test-coverage gap — major — crates/tui/tests/plugin_e2e_acceptance.rs:689-799
The only new e2e test (plugin_toml_binary_lifecycle_skill_and_stdio_mcp_acceptance) is a pure happy path: show → trust → enable → review → MCP call → revoke. Nothing exercises $demo:review, /plugin enable demo, or an MCP tool call before trust/enable to verify the UI actually denies an unreviewed/untrusted plugin — the exact failure path this PR is meant to guard. The .feature file wasn't extended with denial scenarios either.

8. UX correctness gap (minor) — crates/tui/src/tui/ui.rs:3775-3786
When a queued message fails queued_message_content_for_app (e.g. authority revoked mid-flight), it's pushed back onto the FIFO queue (app.queue_message, app.rs:6318-6324). Since the authority check is permanent, this message re-fails on every subsequent turn indefinitely, spamming "Dispatch failed" toasts with no automatic way to drop it. Consider dropping-with-notice on permanent-denial errors vs. transient ones.

9. Nit — crates/tui/assets/skills/plugin-creator/SKILL.md step 7
Describes /plugin enable <name> as opening the content/capability review, but the actual e2e test drives the review token via /plugin trust demo directly. Both entry points may legitimately surface the token, but worth a maintainer sanity check that documented workflow order matches actual command behavior.

Provider/model/route safety (EPIC #2608 invariant)

Checked the diff for any inference of provider/model selection from provider-prefixed strings (deepseek-ai/, anthropic/, openai/, qwen/) or freeform text. Found none — the only prefix-adjacent matches are unrelated log-redaction comments (~/.deepseek/logs/). No violation of this invariant in this PR.

What's done well

• Trust is cleanly separated from enablement and both are bound to content+capability hashes and a monotonic generation, with activation always re-deriving trusted()/staged_root from live comparisons rather than a cached bit (verified by stale_enable_cannot_resurrect_revoked_trust and related tests).
• No .unwrap()/.expect() on fallible production paths across the ~5,500 reviewed lines — every instance found is confined to #[cfg(test)].
• Proxy isolation (client_builder.no_proxy()) and OAuth-disable are applied consistently at every entry point checked, not just UI-hidden, and are covered by dedicated tests (reviewed_plugin_remote_proxy_policy_never_reads_ambient_environment).
• skill_state.rs's cross-process fd_lock-guarded read/write with fail-closed malformed-state handling is solid and has a real multi-process test.
• main.rs threads a single pre-dotenv plugin registry snapshot consistently through launch/resume/fork/exec/serve/doctor/nested-worker paths; skill namespacing (plugin_name:skill_name, collision → shadow-and-warn-and-drop) is enforced and tested end-to-end.
• i18n: all 10 new message keys verified present across all 7 locale files; localization.rs falls back to English on any miss rather than panicking.
• hotbar/actions.rs's replace_skills fixes a real pre-existing staleness bug where hotbar entries for revoked plugin skills would linger.
• ui/tests.rs::persisted_queued_plugin_skill_is_denied_after_cross_process_revocation is a solid TOCTOU regression test for exactly the property the PR claims to guarantee.

[claude]

Claude finished @Hmbown's task in 3m 45s —— View job

---

Claude finished reviewing this PR

Todo list

• Gather context (PR body, prior review comments, commit history)
• Identify commits new since the last completed review (ba8e389, c371f46, 27ffa55)
• Review ba8e389 — length-frame bundle receipts (manifest.rs)
• Review c371f46 — harden durable authority state (registry.rs)
• Review 27ffa55 — keep Windows revalidation nonblocking (manifest.rs, registry.rs)
• Check whether these commits address prior findings
• Post final summary

This is a delta review: three commits landed after the previous full review round completed, triggering a fresh synchronize event. I reviewed just that delta against the prior findings rather than re-reviewing the whole PR.

What the new commits do

• ba8e389 — Fixes a real content-hash framing collision: the old v1 domain hashed directory/file records with only delimiter bytes, so two structurally different trees (e.g. one file containing an embedded fake "file record" vs. two real files) could hash identically. v2 now length-prefixes the manifest and every file body and adds an explicit directory terminator (E\0), and ships a regression fixture (content_hash_v2_distinguishes_a_file_body_from_an_embedded_file_record) that reproduces the old collision and proves it's fixed. The added expected_len/actual_len check is TOCTOU-safe (length is taken via file.metadata() on the already-open fd, and any growth/shrink during the read loop is caught by the final actual_len != expected_len comparison). No issues found.
• 27ffa55 — Fixes a real self-contention bug: the old Windows revalidation reopened the bundle path with the same deny-write sharing mode as the retained authority handle, which could fail/block against its own retained handle. The new open_bundle_identity_probe uses a zero-access, fully-shared handle purely for identity comparison while the retained handle keeps denying mutation — tested in retained_bundle_handle_allows_identity_revalidation_but_denies_writes. looks_windows_absolute now also rejects a bare leading / or \, closing a gap where rooted-but-driveless MCP args weren't caught (existing callers already OR this with Path::is_absolute(), so this is additive, not a regression). No issues found.
• c371f46 — Directly addresses prior finding Remove Normal mode and consolidate to Agent #4 (harden_plugin_state_directory was a no-op on non-Windows): it now opens the state directory via an O_NOFOLLOW|O_DIRECTORY fd, verifies current-uid ownership, and chmods to 0o700, with a matching validate_plugin_state_directory_for_read gate applied before both state loads and verify_plugin_state_authority. Well covered by new tests (discovery_rejects_an_insecure_state_parent_without_mutating_it, trust_hardens_an_existing_owned_state_parent).

New finding from this delta

Correctness (moderate) — crates/tui/src/plugins/registry.rs, persist_plugin_state (unix, ~line 537) + commit_state_change (~line 437)
persist_plugin_state now does two fallible steps where it used to do one: temporary.persist(path) (the atomic rename that actually commits the mutation) followed by a separate parent-directory.sync_all() for durability. If the rename succeeds but the directory sync_all() fails, the function still returns Err(...) (the message even says "state was published but its directory durability could not be confirmed"). The only caller, commit_state_change (save_state(path, &next)?; at registry.rs:437), propagates that Err immediately via ? and never reaches self.state = next;.

Net effect on that rare I/O-error path: a user-facing command like /plugin disable reports failure even though the mutation is already live on disk, and the in-process registry cache (self.state, and everything apply_state() derives from it — plugin.enabled, trust_status, etc.) silently drifts from the persisted truth until the next full reload/discovery. I checked verify_plugin_state_authority (registry.rs:1659) — the actual per-dispatch enforcement gate — and it re-reads state fresh from disk under lock every time, so this is not a security bypass. But it's a real split-outcome bug worth tightening: either reload self.state from disk before surfacing the error, or distinguish "rename failed" (nothing changed) from "rename succeeded, sync unconfirmed" (state did change) in what's reported/cached.
Fix this →

Status of the previous review round's findings

None of these three commits touch skills/mod.rs, mcp.rs, mcp/stdio.rs, main.rs, tui/ui.rs, or the test/E2E files, so findings #2 (missing .into_enabled()), #3 (catalog re-hash on every dispatch call), #5 (stdio Drop try_lock race), #6 (main.rs dup fallback logic), #7 (missing denial-path E2E test), #8 (queued-message re-fail spam), and #9 (SKILL.md wording) remain as previously reported and are unaffected by this delta.

Prior finding #1 (TOCTOU in hash_path's directory branch using pathname-based fs::symlink_metadata/fs::read_dir instead of fd-anchored traversal, manifest.rs:1154/1174) is still open — ba8e389 changed the hashing domain/framing but not the directory-walk mechanism itself; the walk is still pathname-based.

[claude]

Claude finished @Hmbown's task in 2m 32s —— View job

---

Claude finished reviewing this PR

Todo list

• Gather context (PR body, prior review comments, commit history)
• Identify commit(s) new since the last completed review round
• Review the new commit's diff
• Check whether it addresses prior open findings
• Post final summary

This is another delta review: one commit (5bd2749, "fix(plugins): close authority integrity gaps") landed since the previous review round, which covered up through the commits matching "length-frame bundle receipts" / "harden durable authority state" / "keep Windows revalidation nonblocking". I reviewed just this delta (crates/tui/src/plugins/registry.rs, manifest.rs tests, tests.rs) against origin/main and the prior findings.

What 5bd2749 does

• Replaces auto-repair with fail-closed rejection for pre-existing Unix state directories. The old harden_plugin_state_directory would silently chmod 0o700 any existing state-parent directory, even one an attacker (or a stale install) had left group/world-accessible. It's now split into ensure_private_plugin_state_directory (only sets 0o700 at creation time, via DirBuilder::mode, which mkdir cannot widen via umask) and validate_plugin_state_directory_for_read/validate_unix_plugin_state_directory_fields (a pure, directly-unit-testable ownership+mode check) — a genuinely stronger posture: a pre-existing insecure directory is now rejected outright rather than silently corrected and trusted. Covered by the renamed test trust_rejects_an_existing_group_accessible_state_parent_without_repairing_it and the new discovery_rejects_a_symlinked_state_parent_without_touching_its_target.
• Closes a validation gap in load_state: previously the state-parent permission check only ran if lock_exists || state_exists, so a pre-existing-but-empty insecure parent directory (no lock/state file written yet) wasn't validated at all. validate_existing_plugin_state_parent now validates any existing parent unconditionally, folding NotFound into Ok(()) so first-run/fresh-install behavior is unaffected.
• Bumps runtime staging from .runtime/v1 to .runtime/v2, consistent with the tightened directory-trust model, and updates the Windows junction test's expected path accordingly.
• manifest.rs test simplification: drops the legacy_v1_hash_path/legacy_v1_bundle_hash reproduction helpers and replaces content_hash_v2_distinguishes_a_file_body_from_an_embedded_file_record with a leaner bundle_hash_frames_adversarial_binary_records, per the commit message's stated intent ("keep the adversarial hash regression focused on the current framed format without retaining a legacy collision implementation"). Test-only, no production-code change in this file.

I traced the TOCTOU angle on the new ensure_private_plugin_state_directory: between the path_entry_exists check and DirBuilder::create, an attacker planting a symlink at that exact path would make mkdir fail with EEXIST (mkdir doesn't follow an existing symlink to create through it), so the call still fails closed rather than silently proceeding — no gap there. I also confirmed harden_plugin_state_directory has no remaining references anywhere in crates/tui/src/ (clean rename, no dead code left behind).

No new issues found in this delta — it's a solid tightening of the authority-directory trust model with matching regression tests.

Status of previously reported findings

None of this commit's files (registry.rs, manifest.rs, tests.rs) touch skills/mod.rs, mcp.rs, mcp/stdio.rs, main.rs, or tui/ui.rs, so the previously reported items are unaffected and remain as last stated:

• Fix cargo fmt formatting issues #1 (TOCTOU in hash_path's directory branch, manifest.rs) — still open.
• Simplify UI and improve footer display (v0.3.14) #2 (missing .into_enabled() in skills/mod.rs) — still open.
• fix: UTF-8 safe truncation, deduplicate url_encode, Display impls, pricing fix, model simplification #3 (MCP catalogue re-hash on every dispatch call, mcp.rs) — still open.
• fix: address PR #4 follow-ups #5 (stdio Drop try_lock race) — still open.
• Roadmap: surface "coherence" as a plain-language session health signal #6 (main.rs duplicated fallback logic) — still open.
• Roadmap: build long-session evals for coherence and context handling #7 (missing denial-path E2E test) — still open.
• Roadmap: finish workspace extraction and remove runtime integration seams #8 (queued-message re-fail spam in tui/ui.rs) — still open.
• Roadmap: stabilize thread/turn/item protocol across runtime surfaces #9 (SKILL.md wording nit) — still open.
• The commit_state_change durability-sync split-outcome finding flagged in the previous delta round appears to still apply structurally (persist_plugin_state_with_directory_sync still returns Err after a successful rename if the directory sync_all fails, and commit_state_change still propagates via ? before updating self.state), unaffected by this commit.

[Hmbown]

Security follow-up is resolved on exact head 5bd2749c6187fd59a6c73627d2f1fa0b034fceff. The reviewed-bundle authority now uses versioned length framing, rejects unsafe pre-existing state directories without mutating them, holds no-follow current-owner directory authority for state publication, syncs the containing namespace after atomic replacement, fails closed on symlink/junction state paths, and keeps Windows identity revalidation nonblocking while preserving the mutation barrier.

Local receipts on this head:

• cargo fmt --all and git diff --check: clean
• focused plugin suite: 54/54 passed
• workspace all-target Clippy with warnings denied: passed
• co-author/DCO range checks: passed; all four follow-up commits carry Signed-off-by trailers
• exact-range gitleaks scan: zero findings

Native Windows remains the decisive exact-head CI gate; no merge is requested until it is green.

[claude]

Claude finished @Hmbown's task in 1m 37s —— View job

---

Claude finished reviewing this PR

Todo list

• Gather context (PR body, prior review comments, commit history)
• Identify commit(s) new since the last completed review round (5bd2749)
• Review the new commit's diff
• Check whether it addresses prior open findings
• Post final summary

This is another delta review: one commit (ac7ee54, "fix(plugins): preserve hardened state checks in native CI") landed since the last completed review round. Reviewed it against origin/main and the prior findings.

What ac7ee54 does

All production-code changes are confined to crates/tui/src/plugins/registry.rs and are Windows-only (#[cfg(windows)]); the other six touched files (mcp/tests.rs, runtime_api/tests.rs, subagent/tests.rs, command_palette.rs, ui/tests.rs, skills/mod.rs) are test-only fixture updates that move state_path from a bare plugin-state.json sitting directly under a shared temp root into its own plugin-state/state.json subdirectory — needed so the state-parent hardening checks landed in 5bd2749 don't collide with sibling fixtures sharing the same temp root under native (non cross-compiled) Windows CI.

The real fix closes self-conflicting file-handle/share-mode issues on native Windows that the prior Zig-cross-compiled --tests build apparently didn't exercise the same way:

• save_state_with_hardener: on Windows, converts the NamedTempFile into a TempPath (closing its writer handle) before calling harden_temporary, since the ACL hardener opens its target with a restrictive share mode that would otherwise conflict with the still-open writer handle. persist_plugin_state's Windows signature changes from NamedTempFile to TempPath accordingly, and each platform (unix, windows, fallback) now has its own consistent signature — verified all three compile-shape correctly by inspection (NamedTempFile/TempPath/NamedTempFile).
• open_state_lock: on Windows with create=true, now opens the lock file with an explicit access_mode (FILE_GENERIC_READ | FILE_GENERIC_WRITE | WRITE_DAC | WRITE_OWNER) and hardens that same already-open handle via the new harden_opened_plugin_state_file, instead of the old code path that reopened the path fresh via harden_plugin_state_file(path). This is actually a mild security improvement, not just a portability fix — hardening the exact opened fd instead of reopening by pathname closes a (narrow) reopen-by-path TOCTOU window and is consistent with the fd-anchored pattern used elsewhere in this PR.
• apply_windows_owner_only_acl is extracted from set_windows_owner_only_acl_with_mask to operate directly on an already-open File handle, reused by both the reopen-by-path path and the new opened-handle path. I confirmed the access mask stays consistent (0x001f_01ff) between harden_opened_plugin_state_file and set_windows_owner_only_acl, matching the existing owner entry must grant full access test assertion.
• copy_bundle_tree_bounded: on Windows, explicitly drops the staged file's writer handle before preserve_owner_only_file_mode reopens it for ACL hardening, for the same share-mode reason. Checked destination_file isn't referenced after the drop — it isn't.
• New regression test state_lock_hardening_keeps_its_writer_handle directly exercises the fixed scenario (ACL hardening immediately after lock creation must not conflict with the lock's own writer handle).

This is a narrow, well-tested, Windows-only bugfix that closes handle-sharing conflicts rather than changing any cross-platform trust/authority semantics. No new issues found in this delta.

Status of previously reported findings

None of this commit's production changes touch skills/mod.rs's enablement logic (only a test fixture path there), manifest.rs, mcp.rs, mcp/stdio.rs, main.rs, or tui/ui.rs's dispatch logic, so all previously reported items remain as last stated:

• Fix cargo fmt formatting issues #1 (TOCTOU in hash_path's directory branch, manifest.rs) — still open.
• Simplify UI and improve footer display (v0.3.14) #2 (missing .into_enabled() in skills/mod.rs) — still open.
• fix: UTF-8 safe truncation, deduplicate url_encode, Display impls, pricing fix, model simplification #3 (MCP catalogue re-hash on every dispatch call, mcp.rs) — still open.
• fix: address PR #4 follow-ups #5 (stdio Drop try_lock race) — still open.
• Roadmap: surface "coherence" as a plain-language session health signal #6 (main.rs duplicated fallback logic) — still open.
• Roadmap: build long-session evals for coherence and context handling #7 (missing denial-path E2E test) — still open.
• Roadmap: finish workspace extraction and remove runtime integration seams #8 (queued-message re-fail spam in tui/ui.rs) — still open.
• Roadmap: stabilize thread/turn/item protocol across runtime surfaces #9 (SKILL.md wording nit) — still open.
• commit_state_change durability-sync split-outcome finding — still applies structurally, unaffected by this commit.

Note: I was not able to run cargo check/cargo test in this environment (the sandbox required approval for cargo invocations that I didn't have), so this review is by inspection only for ac7ee54. The platform-specific persist_plugin_state signatures were manually verified to line up per-cfg branch, but a native Windows build/test run (as the PR author notes is the decisive gate) is still the authoritative check.

[claude]

Claude finished @Hmbown's task in 2m 29s —— View job

---

Claude finished reviewing this PR

Todo list

• Gather context (PR body, prior review comments, commit history)
• Identify commit(s) new since the last completed review round (ac7ee54)
• Review the new commit's diff
• Check whether it addresses prior open findings
• Post final summary

This is another delta review: one commit (6441506, "fix(plugins): retry Windows ACL hardening safely") landed since the last completed review round. Reviewed it against origin/main and the prior findings.

What 6441506 does

All changes are confined to crates/tui/src/plugins/registry.rs and are Windows-only (#[cfg(windows)]), with open_state_lock now split into separate #[cfg(not(windows))]/#[cfg(windows)] implementations rather than branching internally.

The change fixes a real bug in the prior Windows ACL hardening: it always requested WRITE_OWNER when opening a target for re-hardening. But WRITE_OWNER isn't implicitly granted to an object's owner in Windows — it must be explicit in the DACL or backed by a privilege — so any object previously hardened with a mask that didn't grant WRITE_OWNER (e.g. the read-only staged-content mask 0xa000_0000 used at registry.rs:1525/1873) would permanently fail to be re-hardened, since the owner (who does implicitly get READ_CONTROL/WRITE_DAC) still couldn't reopen with WRITE_OWNER.

The fix:

• open_windows_acl_target/set_windows_owner_only_acl_with_mask: first tries opening with READ_CONTROL | WRITE_DAC | WRITE_OWNER; only on ERROR_ACCESS_DENIED (is_windows_access_denied, raw OS error 5 specifically — not any other failure) retries with READ_CONTROL | WRITE_DAC only, tagged WindowsAclOwnerMode::VerifyCurrentUser.
• apply_windows_owner_only_acl: in NormalizeCurrentUser mode, sets owner+DACL atomically as before. In VerifyCurrentUser mode, it never touches the owner field — instead it calls ensure_windows_plugin_target_owner (via GetSecurityInfo/EqualSid on the already-open, non-reparse-validated handle) both before the SetSecurityInfo DACL-only call and after it, refusing to proceed or report success if the object isn't (or is no longer) owned by the current user.
• open_state_lock (Windows): applies the identical with/without-WRITE_OWNER retry when creating a lock, and correctly does not retry with create=true on the fallback path — the comment explains this is deliberate, so a lock that disappeared between the two attempts fails closed rather than silently being recreated with an unchecked owner.

I traced all call sites of set_windows_owner_only_acl/set_windows_owner_only_acl_with_mask/harden_opened_plugin_state_file in the file — every one now routes through this fallback consistently, and owner_mode is threaded correctly through the one direct caller of harden_opened_plugin_state_file (inside open_state_lock). No path bypasses the pre/post ownership check. The three new tests (owner_only_acl_rehardens_without_write_owner_access, state_lock_rehardens_without_write_owner_access, owner_only_acl_rejects_an_owner_identity_mismatch) directly exercise the fixed scenario and the negative case (foreign-owned object must be rejected), and match the described fallback path (opening with reduced access, verifying ERROR_ACCESS_DENIED occurs on the full-owner path first).

No new issues found in this delta — it's a narrow, well-reasoned fix with matching regression coverage. As with the previous Windows-only commit in this PR, I could not compile/run this code in this sandbox (it's #[cfg(windows)]-gated and cargo invocations require approval I don't have here), so this is a review by inspection only; native Windows CI remains the decisive gate, as the PR author notes.

Status of previously reported findings

None of this commit's changes touch skills/mod.rs, manifest.rs, mcp.rs, mcp/stdio.rs, main.rs, or tui/ui.rs, so all previously reported items remain as last stated:

• Fix cargo fmt formatting issues #1 (TOCTOU in hash_path's directory branch, manifest.rs) — still open.
• Simplify UI and improve footer display (v0.3.14) #2 (missing .into_enabled() in skills/mod.rs) — still open.
• fix: UTF-8 safe truncation, deduplicate url_encode, Display impls, pricing fix, model simplification #3 (MCP catalogue re-hash on every dispatch call, mcp.rs) — still open.
• fix: address PR #4 follow-ups #5 (stdio Drop try_lock race) — still open.
• Roadmap: surface "coherence" as a plain-language session health signal #6 (main.rs duplicated fallback logic) — still open.
• Roadmap: build long-session evals for coherence and context handling #7 (missing denial-path E2E test) — still open.
• Roadmap: finish workspace extraction and remove runtime integration seams #8 (queued-message re-fail spam in tui/ui.rs) — still open.
• Roadmap: stabilize thread/turn/item protocol across runtime surfaces #9 (SKILL.md wording nit) — still open.
• commit_state_change durability-sync split-outcome finding — still applies structurally, unaffected by this commit.