Two bugs found in the plugin hook test harness that together caused
Linux CI to fail on 'hooks::tests::collects_and_runs_hooks_from_enabled_plugins'
with 'Broken pipe (os error 32)'. Three reproductions plus one rerun
failure on main today: 24120271422, 24120538408, 24121392171.
Root cause 1 (chmod, defense-in-depth): write_hook_plugin writes
pre.sh/post.sh/failure.sh via fs::write without setting the execute
bit. While the runtime hook runner invokes hooks via 'sh <path>' (so
the script file does not strictly need +x), missing exec perms can
cause subtle fork/exec races on Linux in edge cases.
Root cause 2 (the actual CI failure): output_with_stdin unconditionally
propagated write_all errors on the child's stdin pipe, including
BrokenPipe. A hook script that runs to completion in microseconds
(e.g. a one-line printf) can exit and close its stdin before the parent
finishes writing the JSON payload. Linux pipes surface this as EPIPE
immediately; macOS pipes happen to buffer the small payload, so the
race only shows on ubuntu CI runners. The parent's write_all raised
BrokenPipe, which output_with_stdin returned as Err, which run_command
classified as 'failed to start', making the test assertion fail.
Fix: (a) make_executable helper sets mode 0o755 via PermissionsExt on
each generated hook script, with a #[cfg(unix)] gate and a no-op
#[cfg(not(unix))] branch. (b) output_with_stdin now matches the
write_all result and swallows BrokenPipe specifically (the child still
ran; wait_with_output still captures stdout/stderr/exit code), while
propagating all other write errors. (c) New regression guard
generated_hook_scripts_are_executable under #[cfg(unix)] asserts each
generated .sh file has at least one execute bit set.
Surgical scope per gaebal-gajae's direction: chmod + pipe tolerance +
regression guard only. The deeper plugin-test sealing pass for ROADMAP
#25 + #27 stays in gaebal-gajae's OMX lane.
Verification:
- cargo test --release -p plugins → 35 passing, 0 failing
- cargo fmt -p plugins → clean
- cargo clippy -p plugins -- -D warnings → clean
Co-authored-by: gaebal-gajae <gaebal-gajae@layofflabs.com>
The plugin loader already pruned stale registry entries, but stale enabled state
could linger in settings.json after bundled or installed plugin discovery
cleaned up missing installs. This change removes those orphaned enabled flags
when stale registry entries are dropped so loader-managed state stays coherent.
Constraint: Commit only plugin loader/registry code in this pass
Rejected: Leave stale enabled flags in settings.json | state drift would survive loader self-healing
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Any future loader-side pruning should remove matching enabled state in the same code path
Tested: cargo fmt --all; cargo test -p plugins
Not-tested: Interactive CLI /plugins flows against manually edited settings.json
Expanded the plugin manager so installed plugin discovery now falls back across
install-root scans and registry-only paths without breaking on stale entries.
Missing registry install paths are pruned during discovery, while valid
registry-backed installs outside the install root remain loadable.
Constraint: Keep the change isolated to plugin manifest/manager/registry code
Rejected: Fail listing when any registry install path is missing | stale local state should not block plugin discovery
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: Discovery now self-heals missing registry install paths; preserve the registry-fallback path for valid installs outside install_root
Tested: cargo fmt --all; cargo test -p plugins
Not-tested: End-to-end CLI flows with mixed stale and git-backed installed plugins
Expanded the Rust plugin loader coverage around manifest parsing so invalid
permission values, invalid tool permissions, and multi-error manifests are
validated in a structured way. Added scan-path coverage for installed plugin
directories so both root and packaged manifests are discovered from the install
root, independent of registry entries.
Constraint: Keep plugin loader changes isolated to the plugins crate surface
Rejected: Add a new manifest crate for shared schemas | unnecessary scope for this pass
Confidence: high
Scope-risk: narrow
Reversibility: clean
Directive: If manifest permissions or tool permission labels expand, update both the enums and validation tests together
Tested: cargo fmt --all; cargo test -p plugins
Not-tested: Cross-crate runtime consumption of any future expanded manifest permission variants
