fix(cli): hook enforcement follow-up — every preflight entrance, both-scope Pi carrier, string-safe Codex feature flag

[Brad (bmethod)]

Follow-up to #1401, carrying the review findings accepted there plus what the pre-push cross-model review surfaced. Every fix has a pin that ran red first.

• The uncovered-event preflight guards every entrance. It already ran before refresh's prune pass; now it also fails add before the first mutation (an installed hook whose source left the contract previously let the add write its items, then die in reconciliation) and the TUI inline update before ITS prune. When a filter admits any agent, the hook preflight ignores the filter — regenerating an agent consumes every locked hook, selected or not.
• The Pi carrier counts from both scopes. Pi loads global and project packages; a globally deployed @vanillagreen/pi-hooks now backs a project hook instead of reading unsupported.
• The Codex hooks feature flag is honest on both sides. The reader is a real TOML parse (an example [features] block inside a multiline string decides nothing; an unparsable file enables nothing), and the line-oriented feature writer now tracks multiline-string state, so a reinstall can no longer rewrite or delete text inside a user's developer_instructions.

Declined from the same review round, consistent with #1401: capability-resolved Pi support (the harnesses: allowlist is the documented per-port contract).

Proof: tools/validate-changed all lanes green, preflight --base origin/main clean, size-ratchet OK, full cargo suite green; second-opinion review ran pre-push and its three actionable blockers are the fixes above.

https://claude.ai/code/session_012epxJEzGqT7q3qcFhdZUt5

[linear-code]

VST-283

[qodo-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[macroscopeapp]

Approvability

Verdict: Needs human review

This PR introduces behavioral changes beyond simple fixes: enabling global Pi carriers to back project hooks, changing Codex config parsing from line-based to proper TOML, and adding new preflight validation that can refuse operations. While well-tested, these runtime behavior changes warrant human review.

^{You can customize Macroscope's approvability policy. Learn more.}

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR tightens hook contract enforcement and update atomicity across add, refresh, and TUI inline updates, while improving Codex config feature detection to avoid false positives from embedded examples.

Changes:

• Add preflight checks to refuse uncovered hook events before any mutation (including prune/reconcile paths).
• Switch Codex hooks feature detection to a real TOML parse and harden line-based merges against multiline-string decoys.
• Expand test coverage for the above regressions (Pi carrier scope, Codex feature decoys, atomic filtered add/inline update).

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tools/size-ratchet-baseline.tsv	Updates size baselines for the touched CLI modules/tests.
cli/tests/hook_contract.rs	Adds regression tests for Pi carrier enforcement and contract/atomicity edge cases.
cli/src/tui/disk_mutations/tests.rs	Adds inline-update regression tests around uncovered events and agent regeneration behavior.
cli/src/tui/disk_mutations.rs	Refuses uncovered hook events before the prune pass during inline updates.
cli/src/installer/hooks/enforcement.rs	Treats globally installed pi-hooks as backing project-scope hooks for enforcement summaries.
cli/src/installer/hooks.rs	Uses TOML parsing for feature detection and adds multiline-string skipping in line merges.
cli/src/commands/refresh.rs	Exposes uncovered_hook_event and ignores name filters when regenerating agents (since they consume all hooks).
cli/src/commands/add.rs	Adds uncovered-event preflight before writing any new items and before reconciliation touches agents.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3018386002

ℹ️ 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".

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

cli/src/installer/hooks.rs:802

• When state is None, this scanner will treat """ / ''' appearing inside TOML comments (after #) as starting a multiline string, which can cause subsequent real TOML structure lines to be skipped and prevent merge_codex_hooks_feature / codex_features_state from applying correct updates. Consider ignoring/comment-stripping the portion after # when outside a multiline string, so delimiters in comments can’t toggle string_state.

fn advance_toml_string_state(line: &str, mut state: Option<&'static str>) -> Option<&'static str> {
    let mut rest = line;
    loop {
        match state {
            Some(delimiter) => match rest.find(delimiter) {
                Some(idx) => {
                    rest = &rest[idx + delimiter.len()..];
                    state = None;
                }
                None => return state,
            },
            None => {
                let double = rest.find("\"\"\"");
                let single = rest.find("'''");
                let (idx, delimiter) = match (double, single) {
                    (None, None) => return None,
                    (Some(d), None) => (d, "\"\"\""),
                    (None, Some(s)) => (s, "'''"),
                    (Some(d), Some(s)) if d < s => (d, "\"\"\""),
                    (_, Some(s)) => (s, "'''"),
                };
                rest = &rest[idx + delimiter.len()..];
                state = Some(delimiter);
            }
        }
    }
}

cli/src/installer/hooks/enforcement.rs:133

• This can call is_pi_extension_operational twice for the project-scope case (global == false). If that function hits the filesystem (likely), consider caching the result(s) in locals (e.g., compute project_operational and global_operational once) to avoid duplicate IO during summary generation.

        crate::pi_extension::is_pi_extension_operational(PI_HOOKS_PACKAGE, global)
            || (!global
                && crate::pi_extension::is_pi_extension_operational(PI_HOOKS_PACKAGE, true));

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

cli/src/commands/add.rs:2382

• The new preflight silently treats a lock-file load failure as “empty lock” via unwrap_or_default(), which can skip the uncovered-hook-event refusal and allow the add to proceed in a potentially inconsistent state. Since this preflight is intended to enforce atomic safety, it should not ignore lock parse/IO errors; instead, propagate the error (or fail the add with a clear message) so the user can repair the lock rather than bypassing this safety check.

        let lock = LockFile::load(&config::lock_file_path(global)).unwrap_or_default();
        let records = crate::refresh_sources::resolve_source_records_without_update(&lock);
        let all_hooks = crate::refresh_sources::all_source_hooks(
            &crate::refresh_sources::load_refresh_sources(&records.sources),
        );
        if let Some((name, error)) =
            crate::commands::refresh::uncovered_hook_event(&lock, &all_hooks, None)
        {
            anyhow::bail!("hook {name}: {error}");
        }

cli/src/installer/hooks.rs:776

• The new multiline-string tracking is covered for full-line comment and multiline-string “decoy” cases, but there isn’t coverage for delimiters appearing in inline comments (e.g., key = 1 # """) or inside regular string literals (e.g., key = "\"\"\""). Adding targeted tests for these cases would prevent regressions where advance_toml_string_state incorrectly flips state and causes the feature writer/state detection to ignore real TOML structure.

fn advance_toml_string_state(line: &str, mut state: Option<&'static str>) -> Option<&'static str> {

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a17a586f3

ℹ️ 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".

[github-actions]

Merge queue ejected this PR (removed_from_merge_queue, reason: MANUAL). The auto-merge arm is now DROPPED — nothing will merge this PR until someone re-arms it.

Ejecting merge-group run: https://github.com/vanillagreencom/vstack/actions/runs/32002645006 (Skill Tests)

Failing job(s):
(run 32002645006 has no failure-shaped jobs recorded — it may be the attempt's still-running or non-ejecting sibling; inspect the run link above)

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.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

Suppressed comments (3)

cli/src/installer/hooks.rs:828

• The multiline-string scanner will treat any occurrence of """ as a closing delimiter while inside a basic multiline string. In TOML, it’s valid to include sequences of quotes where the first quote is escaped (e.g. \"""), which should not terminate the multiline string; this implementation can incorrectly exit “string mode” early and then misinterpret subsequent lines as TOML structure. Consider either (a) tracking escapes while in Some("\"\"\"") and only treating """ as a terminator when it’s not escaped (accounting for an odd/even run of backslashes before the first quote), or (b) using a TOML-aware editor/parser for this transformation to avoid implementing string lexing rules manually.

/// Multiline-string state for a line-oriented TOML walk: `None` outside,
/// `Some(delimiter)` inside. Escapes are not tracked — TOML forbids an
/// unescaped `"""` inside a basic multiline string, and the configs this
/// walks are ones this tool and its users write.
fn advance_toml_string_state(line: &str, mut state: Option<&'static str>) -> Option<&'static str> {
    fn find_bytes(haystack: &[u8], needle: &[u8]) -> Option<usize> {
        haystack
            .windows(needle.len())
            .position(|window| window == needle)
    }
    let bytes = line.as_bytes();
    let mut i = 0;
    // A single-line string or a comment is content: a delimiter inside one
    // opens nothing. Escapes count only in basic (double-quoted) strings.
    let mut single_line: Option<u8> = None;
    while i < bytes.len() {
        match state {
            Some(delimiter) => match find_bytes(&bytes[i..], delimiter.as_bytes()) {
                Some(idx) => {
                    i += idx + delimiter.len();
                    state = None;
                }
                None => return state,
            },
            None => match single_line {
                Some(quote) => {
                    if quote == b'"' && bytes[i] == b'\\' {
                        i += 2;
                        continue;
                    }
                    if bytes[i] == quote {
                        single_line = None;
                    }
                    i += 1;
                }
                None => {
                    if bytes[i..].starts_with(b"\"\"\"") {
                        state = Some("\"\"\"");
                        i += 3;
                    } else if bytes[i..].starts_with(b"'''") {
                        state = Some("'''");
                        i += 3;
                    } else {
                        match bytes[i] {
                            b'#' => return None,
                            b'"' => single_line = Some(b'"'),
                            b'\'' => single_line = Some(b'\''),
                            _ => {}
                        }
                        i += 1;
                    }
                }
            },
        }
    }
    state
}

cli/src/commands/add.rs:2383

• The uncovered-hook-event preflight is duplicated (same uncovered_hook_event call pattern and error wrapping) in two places. It would be more maintainable to extract this into a small helper (e.g., “validate_no_uncovered_hook_events(lock, global, sources…)”) so future adjustments to preflight behavior or error formatting don’t risk diverging between add-time checking and reconcile-time checking.

    // Reconciliation later renders every agent with every locked hook, so an
    // already-installed hook whose source left the contract fails the add
    // here, before this add's own items are written.
    {
        let lock = LockFile::load(&config::lock_file_path(global)).unwrap_or_default();
        let records = crate::refresh_sources::resolve_source_records_without_update(&lock);
        let all_hooks = crate::refresh_sources::all_source_hooks(
            &crate::refresh_sources::load_refresh_sources(&records.sources),
        );
        if let Some((name, error)) =
            crate::commands::refresh::uncovered_hook_event(&lock, &all_hooks, None)
        {
            anyhow::bail!("hook {name}: {error}");
        }
    }

cli/src/commands/add.rs:3059

• The uncovered-hook-event preflight is duplicated (same uncovered_hook_event call pattern and error wrapping) in two places. It would be more maintainable to extract this into a small helper (e.g., “validate_no_uncovered_hook_events(lock, global, sources…)”) so future adjustments to preflight behavior or error formatting don’t risk diverging between add-time checking and reconcile-time checking.

    // Reconciliation regenerates every agent from every locked hook, so a
    // definition install would refuse fails it before any agent is touched.
    if let Some((name, error)) =
        crate::commands::refresh::uncovered_hook_event(&lock, &all_hooks, None)
    {
        anyhow::bail!("hook {name}: {error}");
    }

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fa8f6b1487

ℹ️ 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".

[github-actions]

Merge queue ejected this PR (removed_from_merge_queue, reason: MANUAL). The auto-merge arm is now DROPPED — nothing will merge this PR until someone re-arms it.

Ejecting merge-group run: https://github.com/vanillagreencom/vstack/actions/runs/32003095181 (Skill Tests)

Failing job(s):
(run 32003095181 has no failure-shaped jobs recorded — it may be the attempt's still-running or non-ejecting sibling; inspect the run link above)

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.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

cli/src/commands/add.rs:2386

• unwrap_or_default() suppresses lockfile read/parse errors, which can cause this new uncovered-event preflight to be silently skipped and allow add to proceed in situations where the lock is present but unreadable/corrupt. Prefer propagating the error (or failing the add) so behavior is deterministic and the safety check can’t be bypassed due to an I/O/formatting problem.

        let lock = LockFile::load(&config::lock_file_path(global)).unwrap_or_default();
        let reconciles_agents = !selected_agents.is_empty()
            || lock
                .entries
                .values()
                .any(|entry| entry.kind == config::ItemKind::Agent);
        if reconciles_agents {
            let records = crate::refresh_sources::resolve_source_records_without_update(&lock);
            let all_hooks = crate::refresh_sources::all_source_hooks(
                &crate::refresh_sources::load_refresh_sources(&records.sources),
            );
            if let Some((name, error)) =

cli/src/installer/hooks.rs:776

• The new multiline-string tracking is a correctness boundary for merge_codex_hooks_feature/codex_features_state and currently lacks a regression test for escaped delimiter sequences inside basic multiline strings (e.g., content that includes \\\"\"\"). Adding a focused test case for this scenario would help ensure the feature-writer doesn’t mis-detect string termination and accidentally rewrite real structure below.

fn advance_toml_string_state(line: &str, mut state: Option<&'static str>) -> Option<&'static str> {

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c7602de6b0

ℹ️ 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".