feat(cli): one hook execution contract — event × harness drives install, labels and docs (VST-283) - #1401
Conversation
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
ApprovabilityVerdict: Needs human review This PR introduces a new hook execution contract system that changes install behavior (refusing hooks with uncovered events), adds enforcement labeling to CLI output, and modifies Codex command resolution. The scope and behavioral changes warrant human review. You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR formalizes hook behavior across harnesses by introducing a single “hook execution contract” matrix, then uses it to drive installation behavior and user-visible enforcement labels.
Changes:
- Added a centralized event × harness contract (mechanism + enforced/advisory/unsupported) and enforcement-resolution logic.
- Updated hook installation/registration (notably Codex command anchoring) and added advisory banners to advisory artifacts.
- Improved CLI output (
list/check) and added tests to prevent contract/docs drift and validate hook command behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/size-ratchet-baseline.tsv | Updates size baseline to reflect code growth from the contract/enforcement additions. |
| pi-extensions/pi-hooks/README.md | Clarifies Pi hook coverage, explicitly noting hooks without Pi ports. |
| hooks/block-unsafe-rm.sh | Declares harnesses: to exclude Pi where no implementation exists. |
| cli/tests/hook_contract.rs | Adds end-to-end contract tests for registration anchoring and enforcement labeling. |
| cli/src/resolve.rs | Switches Codex prose-fallback detection to the contract helper. |
| cli/src/installer/hooks/tests.rs | Updates ownership predicate usage and adds UTF-8 path refusal test coverage. |
| cli/src/installer/hooks/opencode.rs | Prepends advisory banner to OpenCode advisory instruction artifacts. |
| cli/src/installer/hooks/enforcement.rs | Introduces enforcement resolution/downgrade logic (allowlist, missing artifacts, Pi carrier package). |
| cli/src/installer/hooks/contract.rs | Adds the contract matrix, rendering helpers, and tests that enforce README synchronization. |
| cli/src/installer/hooks.rs | Routes installs via the contract mechanism; updates Codex/Claude commands and advisory banner usage. |
| cli/src/installer.rs | Re-exports new contract/enforcement helpers for CLI and harness generators. |
| cli/src/harness/mod.rs | Adds stable harness indexing for contract table arrays and tests it. |
| cli/src/harness/claude.rs | Ensures Claude agent hook commands match installer registrations (global vs project). |
| cli/src/commands/list.rs | Enhances list output with per-harness enforcement labels. |
| cli/src/commands/check.rs | Enhances check output with per-harness enforcement labels. |
| cli/src/commands/add.rs | Validates selected hook events against the contract pre-write for atomic refusal. |
| README.md | Replaces prose with a generated contract table and explains enforcement levels/banners. |
| CHANGELOG.md | Documents the contract, anchoring fixes, advisory labeling, and breaking refusal of unknown events. |
| AGENTS.md | Updates internal guidance to reference the single contract and anchoring strategy. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73278bd1b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
cli/src/harness/mod.rs:45
Harness::COUNTis hard-coded to5, which can drift fromHarness::ALLif a harness is added/removed/reordered. SinceALLis an array, you can make this self-maintaining by derivingCOUNTfromSelf::ALL.len()(and keep the existing test as a guard).
/// Number of harnesses, so a per-harness array cannot fall out of step
/// with [`Harness::ALL`].
pub const COUNT: usize = 5;
cli/src/installer/hooks.rs:204
unwrap_or_default()can silently produce an emptyscript_pathand therefore an invalid global command (e.g.,bash \"\") ifhooks_dir()ever returnsNone. Elsewhere the code treats this as infallible (e.g.,expect(\"Claude hooks dir\")), so it’s safer to either: (a) make this function returnResult<String>/Option<String>, or (b) mirror theexpect(...)behavior so failures are loud and diagnosable rather than producing a broken command string.
pub(crate) fn claude_installed_hook_command(global: bool, hook_name: &str) -> String {
let script_path = Harness::ClaudeCode
.hooks_dir(global)
.map(|dir| dir.join(format!("{hook_name}.sh")))
.unwrap_or_default();
claude_hook_command(global, hook_name, &script_path)
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f22ae19d58
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
f22ae19 to
1c4108a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
cli/src/pi_extension.rs:422
is_pi_extension_operationalconsiders a dangling symlink as operational (dest.is_symlink()even whendest.exists()is false). That can incorrectly report Pi as enforced when the package target is missing. To match the docstring (“deployed and registered”), require the symlink target to exist (e.g., rely ondest.exists()only, or explicitly verify the symlink resolves).
pub fn is_pi_extension_operational(name: &str, global: bool) -> bool {
let Ok(dest) = checked_pi_package_path(name, global) else {
return false;
};
(dest.exists() || dest.is_symlink())
&& settings_references_package(name, &dest, global).unwrap_or(false)
}
cli/src/installer/hooks/enforcement.rs:91
- For
OpenCodeInstruction,artifact_presentonly checks the instruction file exists, but the module docstring says presence should include the registration that makes the harness load it. Ifopencode.jsonno longer references the instruction, the label could incorrectly remain advisory. Consider also checking that the OpenCode config referencesopencode_hook_instruction_ref(...)(similar to how Codex/Claude validate registrations).
Mechanism::CursorRule => super::cursor_hook_rule_path(global, name).is_file(),
Mechanism::OpenCodeInstruction => {
super::opencode_hook_instruction_path(global, name).is_file()
}
cli/src/installer/hooks.rs:235
claude_installed_hook_commandfalls back to an emptyPathBufwhenhooks_diris unavailable, producing a command that can silently point at an empty/invalid path. Since this is used to generate agent frontmatter, it would be safer to returnResult<String>(orOption<String>) and let callers decide how to handle an unsupported/misconfigured Claude directory.
pub(crate) fn claude_installed_hook_command(global: bool, hook_name: &str) -> String {
let script_path = Harness::ClaudeCode
.hooks_dir(global)
.map(|dir| dir.join(format!("{hook_name}.sh")))
.unwrap_or_default();
claude_hook_command(global, hook_name, &script_path)
}
cli/src/commands/list.rs:86
enforcement::summaryperforms filesystem reads/parsing (e.g., settings/config JSON/TOML, directory scans) per entry. In projects with many installed items,vstack listcould become noticeably I/O-heavy. Consider caching per-scope reads (e.g., load settings/config once per harness per run) and reusing them across entries.
let harnesses = crate::installer::enforcement::summary(entry, global)
.unwrap_or_else(|| entry.harnesses.join(", "));
eprintln!(" {} ({}) [{}]", entry.name, entry.method, harnesses);
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c4108a3f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Merge queue ejected this PR ( Ejecting merge-group run: https://github.com/vanillagreencom/vstack/actions/runs/31996432335 ( Failing job(s):
No usable same-named comparison on the PR head (checks absent, skipped, or still running) — no flake-vs-genuine call is available; inspect the failing run before re-arming. Automated by merge-queue-ejection-alert (VST-196). This alert never re-arms auto-merge. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (3)
cli/src/commands/list.rs:85
- Calling
enforcement::summary(entry, global)per line item can be expensive because it may (transitively) read/parse multiple config files and scan source hooks from disk for each entry. Consider adding a batch API (e.g., compute summaries once per scope) or caching parsed configs / discovered hooks for the duration of thelistrun, so repeated IO/JSON parsing doesn’t scale with the number of installed hooks.
let harnesses = crate::installer::enforcement::summary(entry, global)
.unwrap_or_else(|| entry.harnesses.join(", "));
cli/src/installer/hooks/enforcement.rs:121
- Unknown
harness_ids are silently skipped, which can makelist/checkoutput misleading when a lock contains an unrecognized harness (e.g., forward-compat or corrupted lock). Instead ofcontinue, consider emitting a label for unknown harness IDs (e.g.,\"<id>: unsupported (unknown harness)\") so the summary still accounts for every harness recorded in the lock.
for harness_id in &entry.harnesses {
let Some(harness) = Harness::from_id(harness_id) else {
continue;
};
cli/tests/hook_contract.rs:137
- This integration test suite hard-depends on
bashbeing present. If CI runs on Windows (or other environments without bash), these tests will fail outright. Consider gating this file (or the affected tests) with#[cfg(unix)], or switching the probe execution to a platform-agnostic mechanism where feasible.
let mut child = Command::new("bash");
child
.arg("-c")
.arg(command)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc9b2bd5e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Merge queue ejected this PR ( Ejecting merge-group run: https://github.com/vanillagreencom/vstack/actions/runs/31996432335 ( Failing job(s):
No usable same-named comparison on the PR head (checks absent, skipped, or still running) — no flake-vs-genuine call is available; inspect the failing run before re-arming. Automated by merge-queue-ejection-alert (VST-196). This alert never re-arms auto-merge. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
cli/src/installer/hooks/enforcement.rs:134
summary()is called fromlist/checkper lock entry and, viaresolve()→artifact_present(), can trigger repeated disk reads + JSON parsing for each hook (Claude settings, Codex hooks.json/config.toml, OpenCode opencode.json, directory scans for prose, etc.). On projects with many hooks this becomes O(hooks × harnesses) file IO. Consider loading/parsing each harness config once per command invocation (or per scope) and passing a cached context intosummary()/resolve()(e.g., pre-parsed Claude settings JSON, Codex doc/config, OpenCode config, and a precomputed set of existing artifacts).
pub fn summary(entry: &LockEntry, global: bool) -> Option<String> {
if entry.kind != ItemKind::Hook {
return None;
}
let Some(hook) = hook_definition(entry, global) else {
// Without the definition there is no event, and without an event no
// level can be derived. Saying so beats printing one.
return Some(format!(
"{} — enforcement unknown (hook definition unavailable)",
entry.harnesses.join(", ")
));
};
let pi_hooks_installed =
crate::pi_extension::is_pi_extension_operational(PI_HOOKS_PACKAGE, global);
let mut parts: Vec<String> = Vec::new();
for harness_id in &entry.harnesses {
let Some(harness) = Harness::from_id(harness_id) else {
continue;
};
let label = match resolve(&hook, harness, global, pi_hooks_installed) {
Some(resolved) => resolved.label(),
None => format!("unsupported (event {} not in contract)", hook.event),
};
parts.push(format!("{harness_id}: {label}"));
}
if parts.is_empty() {
return None;
}
Some(parts.join(", "))
}
cli/src/installer/hooks.rs:251
- Falling back to
PathBuf::default()whenhooks_dir(global)isNonecan produce an invalid command string (e.g., global case becomesbash) and silently bakes it into generated Claude agent frontmatter. Prefer returningOption<String>(and omitting hook entries when the harness is unavailable), or using an error/expectwith a clear message ifhooks_diris assumed to always exist for Claude.
pub(crate) fn claude_installed_hook_command(global: bool, hook_name: &str) -> String {
let script_path = Harness::ClaudeCode
.hooks_dir(global)
.map(|dir| dir.join(format!("{hook_name}.sh")))
.unwrap_or_default();
claude_hook_command(global, hook_name, &script_path)
}
cli/src/commands/refresh.rs:276
- This re-implements contract membership checking via
events().all(...). To keep refresh aligned with install/refusal semantics (and avoid drifting error wording), consider reusinginstaller::contract::validate_event(&hook.name, &hook.event)and mapping itsErrintostats.fail(...).
if let Some(hook) = crate::resolve::source_hook_for_lock_entry(&all_hooks, entry)
&& installer::contract::events().all(|event| event != hook.event)
{
stats.fail(
name,
None,
installer::contract::unknown_event_error(&hook.name, &hook.event),
);
return stats;
}
|
Merge queue ejected this PR ( Ejecting merge-group run: https://github.com/vanillagreencom/vstack/actions/runs/31996432335 ( Failing job(s):
No usable same-named comparison on the PR head (checks absent, skipped, or still running) — no flake-vs-genuine call is available; inspect the failing run before re-arming. Automated by merge-queue-ejection-alert (VST-196). This alert never re-arms auto-merge. |
…ll, labels and docs (VST-283) Installing a hook meant something different on every harness, and nothing said which. Enforcement-class guards became prose on Cursor and OpenCode and nothing at all on Pi, while `vstack list` reported them as installed either way — a guard that reads as armed but cannot block manufactures false safety. The project-scope Codex command compounded it: it resolved its script through `$(git rev-parse --show-toplevel)`, so in a project that is not a git repository every registered hook expanded to `/.codex/hooks/<name>.sh` and failed silently. Both are the same missing thing — a contract for what installation means. One matrix of event × harness now holds it, and the installer, the CLI labels and the published table all derive from it, so a cell cannot be true in one place and stale in another. Where a harness genuinely cannot execute, the artifact it writes says so in its first line, and `list`/`check` say so per harness per hook. An event the matrix does not cover is refused rather than registered somewhere nothing runs it. Anchors now hold from any working directory, git repository or not: Codex carries the install-time absolute path, which is the only anchor it can resolve given it sets no project-root variable and runs from the session cwd; Claude keeps `$CLAUDE_PROJECT_DIR` for project scope and takes the absolute path for global, where no project layer exists to resolve against.
…sence backs the Codex fallback label A script no harness invokes enforces nothing: the Claude and Codex levels now require the settings.json / hooks.json handler beside the script, the Codex prose fallback requires its `## Safety:` block in a generated agent file, and an unreadable config reads as unregistered. Stale Codex handlers from a moved project are recognized in shell_quote's single-quoted form too. Docs say advisory artifacts carry the banner rather than open with it, and the false "TaskCompleted is Claude-Code-only" example is gone. Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
…ed AND registered, Codex hooks feature on, live foreign handlers stay Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
…penCode advisory requires the opencode.json reference Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
…ion, matching add's atomicity Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
ab2210e to
22559c4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (5)
cli/src/pi_extension.rs:422
is_pi_extension_operationaltreats a broken symlink as "operational" becausedest.is_symlink()can be true even whendest.exists()(following the link) is false. For “deployed + registered”, a broken symlink should report false; consider removing the|| dest.is_symlink()clause or replacing it with asymlink_metadatacheck that still requires the symlink target to exist.
pub fn is_pi_extension_operational(name: &str, global: bool) -> bool {
let Ok(dest) = checked_pi_package_path(name, global) else {
return false;
};
(dest.exists() || dest.is_symlink())
&& settings_references_package(name, &dest, global).unwrap_or(false)
}
cli/src/installer/hooks.rs:252
- For global installs,
unwrap_or_default()can yield an emptyscript_path, producing an invalid command likebash ''in generated agent frontmatter (and potentially causing drift vs. settings.json). Instead of defaulting, prefer constructing the expected global path deterministically (or returning an error/Option when the hooks dir cannot be resolved).
pub(crate) fn claude_installed_hook_command(global: bool, hook_name: &str) -> String {
let script_path = Harness::ClaudeCode
.hooks_dir(global)
.map(|dir| dir.join(format!("{hook_name}.sh")))
.unwrap_or_default();
claude_hook_command(global, hook_name, &script_path)
}
cli/src/commands/refresh.rs:276
- This re-implements contract membership testing via
events().all(...). Consider using the centralizedinstaller::contract::validate_event(...)(or acell(...)/Rowlookup) to keep refresh’s preflight behavior consistent with the install-time validation logic and reduce duplication.
if let Some(hook) = crate::resolve::source_hook_for_lock_entry(&all_hooks, entry)
&& installer::contract::events().all(|event| event != hook.event)
{
stats.fail(
name,
None,
installer::contract::unknown_event_error(&hook.name, &hook.event),
);
return stats;
}
cli/tests/hook_contract.rs:103
- This new integration test suite shells out to
git(and elsewhere tobash). If CI runs on environments without these binaries (notably Windows runners without Git/Bash in PATH), the tests will fail for reasons unrelated to the CLI logic. Consider#[cfg(unix)]gating, skipping when binaries are missing, or using a Rust git library / more platform-neutral execution for the parts that don’t strictly require Bash.
fn init_git(&self) {
let output = Command::new("git")
.arg("-C")
.arg(&self.project)
.arg("init")
.output()
.expect("git init");
assert!(output.status.success(), "git init failed");
}
cli/src/installer/hooks.rs:520
- The stale-project detection uses a hardcoded
\"/.codex/hooks/...\"suffix and string-based path handling, which can fail on Windows path separators (and potentially other path representations). If the CLI supports Windows (per README), consider usingPathoperations for the suffix/ends-with check (e.g., normalize and compare path components) instead of relying on/in a string.
let script = script_path.to_string_lossy().into_owned();
let project_tail = format!("/.codex/hooks/{hook_name}.sh");
move |command: &str| {
if command == exact {
return true;
}
if global {
return false;
}
let Some(argument) = command.strip_prefix("bash ") else {
return false;
};
let argument = argument.trim();
let unquoted = argument
.strip_prefix('"')
.and_then(|rest| rest.strip_suffix('"'))
.or_else(|| {
argument
.strip_prefix('\'')
.and_then(|rest| rest.strip_suffix('\''))
})
.unwrap_or(argument);
if unquoted == script {
return true;
}
unquoted.ends_with(&project_tail) && !Path::new(unquoted).exists()
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22559c4bd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
…ackage symlink is not deployed; the event preflight runs before the prune pass Claude-Session: https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (5)
cli/src/installer/hooks/enforcement.rs:140
summary()is called fromlist/checkfor each lock entry; it performs filesystem + JSON parsing work repeatedly (e.g.,is_pi_extension_operationaland the per-mechanism registration checks). Consider adding a cached context (e.g., pre-readsettings.json/.codex/hooks.json/opencode.jsonand Pi settings once per run) or changing the API so callers computepi_hooks_installedonce and pass it in. This will avoid O(n) repeated disk reads for large lockfiles.
let pi_hooks_installed =
crate::pi_extension::is_pi_extension_operational(PI_HOOKS_PACKAGE, global);
let mut parts: Vec<String> = Vec::new();
for harness_id in &entry.harnesses {
let Some(harness) = Harness::from_id(harness_id) else {
continue;
};
let label = match resolve(&hook, harness, global, pi_hooks_installed) {
Some(resolved) => resolved.label(),
None => format!("unsupported (event {} not in contract)", hook.event),
};
parts.push(format!("{harness_id}: {label}"));
}
cli/src/installer/hooks/enforcement.rs:127
- Values displayed here come from the lock file (
entry.harnesses), which is user-editable input. Printing raw harness IDs can allow control characters to be emitted to terminals/logs (log injection). Consider rendering harness IDs with debug formatting ({harness_id:?}) or escaping control characters before printing.
return Some(format!(
"{} — enforcement unknown (hook definition unavailable)",
entry.harnesses.join(", ")
));
};
cli/src/installer/hooks/enforcement.rs:139
- Values displayed here come from the lock file (
entry.harnesses), which is user-editable input. Printing raw harness IDs can allow control characters to be emitted to terminals/logs (log injection). Consider rendering harness IDs with debug formatting ({harness_id:?}) or escaping control characters before printing.
parts.push(format!("{harness_id}: {label}"));
cli/src/installer/hooks.rs:266
unwrap_or_default()can silently generate an invalid command whenhooks_dir(global)isNone(e.g., producing an empty script path that becomesbash \"\"for global scope). Since this function is used to generate agent frontmatter, it can cause hooks to be registered/represented with a broken command. Prefer returning aResult<String>(and propagating) or constructing the expected path from known config roots so it can’t default to an empty path.
pub(crate) fn claude_installed_hook_command(global: bool, hook_name: &str) -> String {
let script_path = Harness::ClaudeCode
.hooks_dir(global)
.map(|dir| dir.join(format!("{hook_name}.sh")))
.unwrap_or_default();
claude_hook_command(global, hook_name, &script_path)
}
README.md:250
- This row conflicts with the new hook execution contract table, which states
TaskCompletedis advisory on Codex (agent instructions) and enforced on Pi. Either update the description to match the contract behavior, or explicitly note that this specific shipped hook is scoped viaharnesses:(if that’s the intent) so it’s actually Claude-only.
| `task-completed-check` | `TaskCompleted` | Runs final lint checks before marking work complete. Claude-Code-only — codex has no clean equivalent event. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a7ba3aec1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Merge queue ejected this PR ( Ejecting merge-group run: https://github.com/vanillagreencom/vstack/actions/runs/31998504207 ( Failing job(s):
No usable same-named comparison on the PR head (checks absent, skipped, or still running) — no flake-vs-genuine call is available; inspect the failing run before re-arming. Automated by merge-queue-ejection-alert (VST-196). This alert never re-arms auto-merge. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cli/src/pi_extension.rs:422
is_pi_extension_operationalusesdest.exists(), which will return true for a plain file as well as a directory. Pi packages are expected to be deployed as directories; otherwise enforcement could be reported as active when Pi can’t actually load the package. Consider usingdest.is_dir()(follows symlinks and rejects dangling links) and, if needed, explicitly handling the “exists but not a directory” case.
pub fn is_pi_extension_operational(name: &str, global: bool) -> bool {
let Ok(dest) = checked_pi_package_path(name, global) else {
return false;
};
dest.exists() && settings_references_package(name, &dest, global).unwrap_or(false)
}
cli/src/installer/hooks/enforcement.rs:138
- Unknown harness IDs recorded in the lock are silently skipped in the enforcement summary. This can hide useful information from
vstack list/vstack check(and changes behavior versus printingentry.harnesses.join(\", \")). Instead ofcontinue, consider including the originalharness_idin the output with a label likeunsupported (unknown harness)so users can see and remediate unexpected lock contents.
for harness_id in &entry.harnesses {
let Some(harness) = Harness::from_id(harness_id) else {
continue;
};
cli/src/installer/hooks/enforcement.rs:108
artifact_present(and its helpers likeclaude_hook_registered,codex_hook_registered, andopencode_hook_instruction_registered) can read/parse the same config files repeatedly during a singlelist/checkrun (once per hook, sometimes per harness). This can become noticeable with many installed hooks. Consider caching parsed configs per scope/harness insummary(e.g., loadsettings.json,hooks.json,opencode.json, and Codexconfig.tomlonce) and then doing purely in-memory checks per hook.
match mechanism {
Mechanism::ClaudeSettingsHook => {
Harness::ClaudeCode
.hooks_dir(global)
.is_some_and(|dir| dir.join(format!("{name}.sh")).is_file())
&& super::claude_hook_registered(global, name, event, matcher)
}
Mechanism::CodexHooksJson => {
super::codex_root(global)
.join("hooks")
.join(format!("{name}.sh"))
.is_file()
&& super::codex_hook_registered(global, name, event, matcher)
}
Mechanism::CursorRule => super::cursor_hook_rule_path(global, name).is_file(),
Mechanism::OpenCodeInstruction => {
super::opencode_hook_instruction_path(global, name).is_file()
&& super::opencode_hook_instruction_registered(global, name)
}
Mechanism::CodexInstructions => super::codex_hook_prose_present(global, name),
Mechanism::PiHooksExtension => true,
}
cli/src/commands/refresh.rs:260
- The uncovered-event check re-implements contract membership via
events().all(...). Since the contract already exposescell(...)/validate_event(...), consider using a direct contract predicate (e.g.,contract::cell(&hook.event, Harness::ClaudeCode).is_none()or a dedicatedcontract::covers_event(&hook.event)) to reduce duplication and keep all membership logic in one place.
if let Some(hook) = crate::resolve::source_hook_for_lock_entry(source_hooks, entry)
&& installer::contract::events().all(|event| event != hook.event)
{
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97ef54547d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
Summary
Installing a hook meant something different on every harness and nothing said which. Enforcement-class guards (
block-bare-cd,pre-commit-check) became prose on Cursor and OpenCode and nothing at all on Pi, whilevstack listreported them as installed either way. The project-scope Codex command compounded it: it resolved its script through$(git rev-parse --show-toplevel), so in a project that is not a git repository every registered hook expanded to/.codex/hooks/<name>.shand failed silently.Both are the same missing thing — a contract for what installation means. This adds one, and derives everything from it.
What changed
The matrix is the source.
cli/src/installer/hooks/contract.rsholds oneevent × harnesstable —enforced(mechanism)/advisory(mechanism)/unsupported, one cell per harness inHarness::ALLorder.install_hookno longer chooses a mechanism: it looks up the cell and writes what the cell names. Codex's native-vs-prose routing, which used to live in a secondmatchincodex_event_for, now derives from the same table, as doesinstalled_codex_fallback_hooks.The published copy is generated, not maintained.
README.mdcarries the table between<!-- generated: hook-contract -->markers;contract::render_markdown_table()produces it and a unit test fails when the two drift. The hand-maintained per-harness bullet list and the duplicated hook claims in the Supported Tools table are gone.Anchors hold in non-git projects. Codex sets no project-root variable and runs hook commands from the session cwd (
learn.chatgpt.com/docs/hooks), so the registered command now carries the install-time absolute path — the one anchor that resolves from any directory whether or not the project is a git repository.codex_owned_hook_commandslists both shapes vstack has registered, so a reinstall replaces its own entry instead of adding a second handler beside it. Claude Code agent frontmatter had a second copy of the project anchor and used it for global installs too, pointing at a project path that does not exist; it now calls the installer's own command builder.The level is stated, in three places.
vstack listandvstack checkprintharness: enforced/advisory/unsupportedper installed hook, derived from the table. Every advisory artifact (Cursor rule, OpenCode instruction, Codex prose block) opens withadvisory — this harness cannot execute hooks. Pi reportsunsupported (pi-hooks not installed)until@vanillagreen/pi-hooks— which carries all Pi hook behavior — is actually present in that scope, and a hook the allowlist excludes reportsunsupported (excluded by harnesses:).One known lie removed.
block-unsafe-rmnow declaresharnesses:withoutpi:pi-hookshas no port of it, and without the exclusion the new labels would have reported Pi enforcement that does not exist.Breaking: a hook whose
event:is not a row of the contract is refused at install, naming the event and the supported set, instead of registering something no harness runs. All shipped hooks are covered (pinned by a test).Deliverable 4 —
.git/hooksshim extension: evaluated, not extendedThe VST-216 shim is owned by the
growth-guardsskill:install-git-hookswrites avstack-guardshelper plus one marked delegating line into.git/hooks/pre-commitandcommit-msg, and those run growth-guards' own checks — not vstack'shooks/*.sh, which are harness-shaped (they read a harness JSON payload on stdin and key offtool_input.command).pre-push— no vstack hook maps to a push. The contract's rows are harness session/tool events; there is no row for the shim to carry, so this would be new behavior rather than a mechanism the table already names.post-checkout— already owned by the worktree skill's auto-repair hooks; double-installing there was explicitly out of scope.pre-commit-check(PreToolUse/Bash, matchinggit commit), and that path is already guarded for every tool by the growth-guards shims. A second shim feeding a synthetic harness payload into the same script would double-run the same checks with two different failure texts.So the shim stays where it is and does not enter the matrix: it fires on a git event for every tool, harness or none, which is neither axis of an
event × harnesstable. What the contract adds is that this is now stated next to the table, so a reader who seespre-commit-checkasadvisoryon Cursor also reads that the commit path itself is guarded repo-wide whengrowth-guardsis installed.Cross-model review, applied before the first push
second-opinion review(codex,gpt-5.6-sol, xhigh) returned three blockers against the first commit. Two were real defects this change introduced and one was a real gap in what the labels claimed; all three are fixed here, each with its own pin (red evidence below)./.codex/hooks/<name>.sh— not by the literal string an earlier location wrote. A moved project no longer accumulates a second handler, and removal takes the old one. A handler naming a script outside that directory is still left alone (pinned both ways). The non-UTF-8 path case is refused rather than written lossily, because the JSON config cannot carry those bytes at all.add, which is where the atomicity claim lives: every selected hook's event is checked beside the existing reserved-name preflight, before the first write, and a refused install leaves no lock, agent, settings, config or script behind (pinned).refreshkeeps its per-item failure semantics — a hook that cannot install is reported as a failed item there by design, exactly as a disk error already is.unsupported (artifact missing)when the artifact behind it is gone, alongside the existingexcluded by harnesses:andpi-hooks not installeddowngrades. It is deliberately not a probe of harness runtime state: whether Codex has been told to trust a project's.codex/layer, and which hooks are toggled on in pi-extension-manager, are the harness's to answer and not observable from here. The README now says exactly this and points atvstack verify.Proof
Lanes run locally:
cd cli && cargo test(all suites green),cli/scripts/integration-check.shviatools/validate-changed(every derived lane green),skills/size-ratchet/scripts/size-ratchet(baseline row forcli/src/installer.rsraised to 3821 in this diff),skills/preflight/scripts/preflight --base origin/main. Cross-modelsecond-opinion reviewrun before the first push.Red-once
New pins in
cli/tests/hook_contract.rsrun against the pre-change source (git stash push -- cli/src README.md): 9 failed, 3 passed. The three that passed are the paired must-pass controls — the same assertions in the shapes that already worked — so the file cannot pass vacuously.codex_project_hook_command_fires_in_a_non_git_projectfatal: not a git repository→bash: /.codex/hooks/probe.sh: No such file or directory(exit 127)a_codex_reinstall_replaces_a_git_anchored_registrationclaude_global_agent_frontmatter_command_firesbash: /.claude/hooks/probe.sh: No such file or directory(exit 127)cursor_and_opencode_artifacts_open_with_the_advisory_banner# Safety: probe, no bannerthe_codex_prose_fallback_carries_the_advisory_bannerlist_and_check_label_every_harness_with_its_enforcement_levelprobe (copy) [claude-code, cursor]— no levelspi_reports_unsupported_until_its_carrier_package_is_installeda_harness_dropped_from_the_allowlist_reports_as_excludedcursor: advisoryfor a hook that no longer applies therean_event_outside_the_contract_is_refused_at_install.claude/hooks/notify.shcodex_project_hook_command_fires_in_a_git_project,codex_global_hook_command_fires,claude_project_hook_command_fires_in_a_non_git_projectSecond round, for the review findings — same file, run before those fixes: 3 failed, 13 passed.
a_moved_project_does_not_accumulate_stale_codex_registrationsPreToolUseheld bothbash /somewhere/else/.codex/hooks/probe.shand the current path (left: 2, right: 1)an_uncovered_event_is_refused_before_anything_is_writtenrefused install left .claude/agents/rust.md behinda_deleted_hook_script_stops_reading_as_enforcedprobe (copy) [claude-code: enforced]with the script deleteda_user_authored_codex_handler_elsewhere_is_left_alone—bash /opt/mine/probe.shsurvives the prunerequire_utf8_script_pathis a new refusal rather than a behavior regression, so it is pinned at its seam with its paired control: the invalid path errors naming the reason, the valid path still installs.contract::tests::the_readme_table_is_the_rendered_matrixwas red in the same state (README carries the generated hook-contract block).Every anchor pin runs the exact command string the config carries, from
<project>/nested/deeper, and asserts the hook body ran — a marker file the script appends to — so a command that resolves to nothing cannot pass.Closes VST-283
https://claude.ai/code/session_01RqdGH7w3Qz8EoYU2yS8z8X