feat(VST-258): surface vstack drift to the agent at session start - #1327
Conversation
ApprovabilityVerdict: Needs human review Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. 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.
Adds a session-start “drift check” feature (shell hook + Pi extension) backed by an expanded vstack check command/reporting contract, including quiet/JSON/offline modes and safer remote-cache refresh behavior.
Changes:
- Introduces session-start drift reporting in
@vanillagreen/pi-hooks(configurable, non-blocking) and a matching shell hook + tests. - Expands the
vstack checkCLI into a stable contract (exit codes 0/1/2,--quiet,--json,--offline,--no-available) with structured reporting and new test coverage. - Refactors/process-hardens remote cache refresh (bounded/background refresh, unified fetch outcomes) and improves path/name safety + verification logic.
Reviewed changes
Copilot reviewed 29 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pi-extensions/pi-hooks/tests/drift-check.test.ts | Adds Bun tests for drift-check classification and session_start wiring. |
| pi-extensions/pi-hooks/package.json | Bumps extension version and documents new session drift settings. |
| pi-extensions/pi-hooks/extensions/process.ts | Adds shared bounded child-process runner used by drift check and cargo. |
| pi-extensions/pi-hooks/extensions/hooks.ts | Wires session_start to run drift check asynchronously and send a custom message. |
| pi-extensions/pi-hooks/extensions/drift-check.ts | Adds drift-check implementation + message formatting for Pi hooks. |
| pi-extensions/pi-hooks/extensions/config.ts | Adds defaults + config access for drift settings and timeout. |
| pi-extensions/pi-hooks/extensions/cargo.ts | Refactors runCargoAsync to reuse the shared process runner. |
| pi-extensions/pi-hooks/README.md | Documents the new session-start drift report hook behavior/settings. |
| pi-extensions/pi-hooks/CHANGELOG.md | Records consumer-impacting changes for 0.4.0. |
| hooks/tests/session-drift-check.test.sh | Adds shell-level tests for the new session drift hook contract. |
| hooks/session-drift-check.sh | Adds the session-start drift hook that runs vstack check --quiet. |
| cli/tests/check_contract.rs | Adds integration tests pinning the vstack check process contract. |
| cli/tests/add_noninteractive_tty.rs | Minor formatting change in an existing test assertion. |
| cli/src/tui/install_flow.rs | Makes cache refresh messaging conditional and reports refresh problems. |
| cli/src/tui/disk_mutations.rs | Minor formatting simplification of a function signature. |
| cli/src/refresh_sources.rs | Unifies remote-cache resolution and fetch outcome reporting; adds tests. |
| cli/src/pi_extension.rs | Shares rename table visibility and centralizes safe-component validation. |
| cli/src/path_safety.rs | Adds shared safe-component/item-name predicates + tests. |
| cli/src/mapping.rs | Adds strict vs forgiving mapping config loads + tests. |
| cli/src/main.rs | Extends check CLI flags and adds hidden cache-refresh plumbing command. |
| cli/src/installer/hooks.rs | Tracks whether hook install produced an artifact; fixes Codex prose marker logic. |
| cli/src/installer.rs | Mostly formatting/structure changes in install/remove logic. |
| cli/src/commands/verify.rs | Shares missing-artifact detection across kinds; improves Codex hook verification. |
| cli/src/commands/check.rs | Major rewrite: structured report, JSON/quiet/offline/no-available, cache failure reporting, safety scrubbing, and extensive tests. |
| cli/src/commands/add.rs | Threads an interactive mode into remote source resolution/fetch behavior. |
| cli/src/catalog.rs | Adds inventory APIs that distinguish missing roots vs empty roots vs errors; adds tests. |
| cli/src/agent.rs | Minor formatting changes. |
| cli/Cargo.toml | Adds libc dependency on unix for advisory file locking. |
| README.md | Documents vstack check semantics and adds session-drift-check to hooks table. |
| AGENTS.md | Updates developer guidance to reference the new check contract and Pi events. |
Suppressed comments (1)
pi-extensions/pi-hooks/tests/drift-check.test.ts:1
- The
resolvesassertion isn’t awaited/returned, so the test can pass without actually validating the promise result. Change this toawait expect(Bun.file(argsLog).text()).resolves.toBe(...)(or return that expectation) so failures are properly observed.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d2e11264f
ℹ️ 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".
1d2e112 to
6e69044
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 28 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
pi-extensions/pi-hooks/tests/drift-check.test.ts:56
- This assertion is not awaited/returned. In async tests, an un-awaited
expect(...).resolvescan be skipped (the test may finish before the promise settles), producing false positives. Await the expectation (or return it) so the args log check is actually enforced.
expect(Bun.file(argsLog).text()).resolves.toBe("check --quiet\n");
cli/src/installer/hooks.rs:312
- The Codex safety marker string is now centralized here, but other presence checks (e.g., functions that scan agent TOMLs for the marker) still appear to build the marker string independently. To prevent future drift between install-time markers and verify/check-time detection, consider reusing
codex_hook_safety_markereverywhere the marker is constructed.
pub(crate) fn codex_hook_safety_marker(hook_name: &str) -> String {
format!("## Safety: {hook_name}")
There was a problem hiding this comment.
💡 Codex Review
vstack/cli/src/commands/add.rs
Lines 2903 to 2905 in 6e69044
After a fresh remote clone succeeds, this path never creates the fetch stamp used by remote_cache_fetch_due. Therefore the first non-offline vstack check immediately considers the brand-new cache overdue and launches another git fetch and reset, rather than waiting for the six-hour TTL; every newly added remote source incurs this redundant network operation. Record the successful clone as fresh before returning.
AGENTS.md reference: AGENTS.md:L197-L197
ℹ️ 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 26 out of 29 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
pi-extensions/pi-hooks/tests/drift-check.test.ts:25
- These tests create a fake
vstackas a bash script, which won’t run on Windows (and may fail in minimal CI images withoutbash). To keep the test suite portable, either skip these tests onwin32(and any environment lacking bash), or generate a tiny Node-based executable instead of a bash script.
`#!/usr/bin/env bash
printf '%s\\n' "$*" >>"${argsLog}"
if [ -n "\${FAKE_OUT:-}" ]; then printf '%s\\n' "$FAKE_OUT" >&2; fi
exit "\${FAKE_RC:-0}"
`,
pi-extensions/pi-hooks/tests/drift-check.test.ts:187
- This hard-codes
:as the PATH delimiter, which breaks on Windows where the delimiter is;. Usenode:path’sdelimiter(or an equivalent helper) when prepending toPATHso the test behaves consistently across platforms.
const oldPath = process.env.PATH;
process.env.PATH = `${root}:${oldPath ?? ""}`;
try {
pi-extensions/pi-hooks/extensions/drift-check.ts:50
- The comment and tests describe “relay the report verbatim,” but
.trim()can remove leading whitespace (if any) and always removes trailing newlines/spaces, which is not strictly verbatim. Using.trimEnd()(or no trimming) better preserves formatting while still avoiding accidental extra blank lines at the end.
const report = `${result.stderr}${result.stdout}`.trim();
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7372911392
ℹ️ 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".
7372911 to
289f1ab
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 43 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
pi-extensions/pi-hooks/extensions/drift-check.ts:1
- The module/docs (and tests) describe “relays the report verbatim”, but the implementation trims whitespace and also concatenates stderr+stdout (which can change formatting vs the shell hook’s
2>&1capture). Either (a) adjust the wording to explicitly state the output is normalized (trimmed, stderr-first), or (b) preserve the report bytes as closely as possible to the underlying command’s output (including trailing newlines) so “verbatim” is accurate.
cli/src/commands/add.rs:2867 - The comment claims the cache directory is shaped like
<host>_<owner>_<repo>, but the new remote-cache implementation uses an encoded, host-aware single-component key (e.g.,github.com%2Fowner%2Frepoper tests). Please update this comment to match the actual on-disk layout to avoid misleading future changes/debugging.
/// Clone or update a remote repo into `~/.vstack/cache/<host>_<owner>_<repo>`
fn clone_or_update(source: &str, interactive: bool) -> Result<PathBuf> {
There was a problem hiding this comment.
💡 Codex Review
vstack/cli/src/commands/verify.rs
Line 286 in 289f1ab
When a Claude hook script remains on disk but its entry is deleted from settings.json (or moved under the wrong event), this branch reports the install as present even though Claude never executes it; for session-drift-check, that makes the new session-start path silently stop reporting drift. Fresh evidence beyond the resolved Codex case is that this Claude branch still checks only the script, while the installer writes both the script and its settings.json registration. Validate the expected event and command entry as well.
AGENTS.md reference: AGENTS.md:L58-L58
vstack/cli/src/commands/verify.rs
Line 298 in 289f1ab
When the generated instruction file remains but its path is removed from the instructions array in opencode.json, this branch reports the hook as installed even though OpenCode does not load that instruction. Because check now shares this presence predicate, both check and verify return a false-clean result for affected OpenCode hooks; parse the config and require the expected instruction reference alongside the file.
AGENTS.md reference: AGENTS.md:L58-L58
ℹ️ 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".
289f1ab to
c9832ba
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated no new comments.
Suppressed comments (5)
cli/src/commands/add.rs:2942
repo_dir.to_str().unwrap()can panic on non-UTF8 paths (e.g., a non-UTF8 HOME on unix).std::process::CommandacceptsOsStr, so passrepo_dir.as_os_str()(or&repo_dir) instead of forcing UTF-8.
let status = crate::config::git_command_for_cache()
.args([
"clone",
"--depth",
"1",
&git_url,
repo_dir.to_str().unwrap(),
])
pi-extensions/pi-hooks/extensions/hooks.ts:1
- This is described as a port of
hooks/session-drift-check.sh, but the shell hook skipsresume|compactwhile this skipsreload|resume. Ifcompact(or other non-fresh reasons) can occur in Pi, this will run the drift check more often than intended. Consider switching to an allowlist of “fresh start” reasons (e.g.,startup|new|fork) or matching the shell hook’s skip set so the two adapters stay behaviorally identical.
pi-extensions/pi-hooks/extensions/drift-check.ts:1 - Concatenating stderr+stdout without a delimiter can merge the last line of stderr with the first line of stdout (e.g.,
...error+{json...}→...error{json...}) and makes diagnostics harder to read. Consider joining with a newline when both are non-empty (and preserving original line breaks) before trimming.
pi-extensions/pi-hooks/extensions/process.ts:1 - The timeout diagnostic embeds the full
commandand joinedargsverbatim. If any caller ever passes sensitive values (tokens, credentials) or control characters in arguments, they’ll be copied into logs/agent context. Consider redacting/scrubbing (control-char replacement + length cap, similar todisplay_text) and/or omitting args from this message unless explicitly safe.
pi-extensions/pi-hooks/tests/process.test.ts:1 - These tests assume external binaries (
bash,sleep) are present and behave consistently. If CI or contributors run tests on environments without these tools (notably Windows), this will fail unrelated torunCommandAsync. Consider guarding tests by platform / availability (skip whenbash/sleepare missing) or using a Node/Bun-based fixture process to make the suite portable.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9832ba0b6
ℹ️ 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/31968078178 ( 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. |
|
Merge queue ejected this PR ( Ejecting merge-group run: https://github.com/vanillagreencom/vstack/actions/runs/32031674585 ( 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. |
…uation, one OpenCode registration predicate Claude-Session: https://claude.ai/code/session_01MmBDMLngdEbAcgEuPYcBmi
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee63956fdc
ℹ️ 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/32031674585 ( 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. |
… when two co-occur
A presence check that returned on the first fault it found prescribed a
remedy the second fault refuses. Every per-harness reader now asks all of
its questions and collects the answers; a fault is suppressed only when the
SAME command would fix it anyway, never when that command would refuse.
- claude/opencode read their registration whatever the artifact says, and
claude's disableAllHooks is read beside a missing script (no reinstall
flips it); codex surfaces an unparseable hooks.json/config.toml beside a
missing script.
- install_gap reads Pi settings whatever the package directory says, and
names an unusable .agents ahead of the project skill whose reinstall it
blocks.
- pi_carrier_state carries the parse failure blocking the carrier install.
- remote_cache puts an unwritable cache ahead of the fetch failure its
frozen stamp can no longer age past the drift threshold.
Verified as must-fail controls: each new test fails against the pre-fix
source and passes after.
Split verify.rs -> verify/hooks.rs, remote_cache.rs -> remote_cache/problems.rs,
and check/tests.rs -> tests/{skills,hooks/codex}.rs at their concept seams.
Claude-Session: https://claude.ai/code/session_01MmBDMLngdEbAcgEuPYcBmi
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c111cfd90
ℹ️ 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".
…he lease's whole lifetime The portable fetch lock recorded its liveness only while a fetch child was being waited on, but the lease it belongs to outlives that fetch by the whole read: discovery, hashing, copying, and any interactive selection. Where the platform cannot answer whether a pid is alive, a lock whose mtime aged past STALE_LOCK_AFTER was taken over and `reset --hard` ran under a live reader. The heartbeat now belongs to the lock, not to the fetch wait: it starts when the lock is taken, beats every LOCK_HEARTBEAT, and Drop signals and JOINS it before the unlink, so no beat can land after the lease ends. It is a plain thread, so it cannot keep the process alive at exit — an exit that skips Drop leaves a lock whose pid is dead, still takeable once it goes stale. The wait-only heartbeat in wait_for_fetch is gone; the lifetime one subsumes it. The flock path is untouched: the kernel releases it, so it has no staleness heuristic to race. Splits guard.rs at the liveness seam (guard/liveness.rs) to stay under the size ratchet.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcd6239958
ℹ️ 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".
`matcher` was left to the schema's `Any` fallback, so a `"matcher": 42` neither claude nor codex can deserialize read back as `None` — "no matcher" — which is exactly the slot a matcherless hook accepts. list, check and verify all reported session-drift-check registered against a registration the harness drops the whole file over. Declare it as an optional string, then close the general gap: every field any reader interprets is now declared with its real type, and each remaining `Any` carries the per-field reason it is a value vstack only preserves — hook handler `type`/`timeout` (written, never read back), opencode `instructions[]` and `permission.<tool>`, Pi `packages[]`, and each document's foreign root keys. None of those can turn a malformed value into a claim that something is installed, which is what separated them from `matcher`. The deviation walker moves to json_config/deviation.rs at its own seam. Claude-Session: https://claude.ai/code/session_01MmBDMLngdEbAcgEuPYcBmi
Summary
vstack checkbecomes the full drift contract:outdated/removed/available(suggestion-only, never drift) / source issues (unresolvable, unreadable, discovery failures) / cache-refresh failures, with--json(stdout),--quiet(silent when clean),--offline, and exit codes0clean /1drift /2check failed.~/.vstack/cache, outcome stamped for the next session). Measured 0.00s session-start cost with unreachable remotes.hooks/session-drift-check.sh(SessionStart; Claude Code + Codex native) and pi-hooks 0.4.0sessionDriftCheckare thin adapters overcheck --quiet: silent when clean, silent on resume/compact, one bounded report block when drift exists.GIT_CEILING_DIRECTORIESpinning (a cache mutation can no longer touch any enclosing repository), symlink/containment refusal, credential-free reports in every output mode, per-kind install presence (a deleted agent/hook/Pi package is drift), event-derived Codex nativeness, live-owner lock takeover guard.Completed Issues
Created Issues
QA Metrics
Live multi-harness QA (release binary, isolated HOME, throwaway projects) — verdict pass, all seven cells:
Perf (reviewer-perf, release builds): local check path 38-45ms/run, no regression across rounds; detached refresher dies at its 60s deadline with a stamped cause.
Test Plan
cd cli && cargo test(683 tests),cargo clippy --all-targets -- -D warnings,cargo fmt --checkcli/scripts/integration-check.sh(29 checks)bash hooks/tests/suites (session-drift-check: 25 assertions) andbun testinpi-extensions/pi-hooks(85)