Red-teaming the guard from 3c317470 found two more ways to get a repository's own
code executed. Both are demonstrated by a planted binary that appends to a witness
file, counted before and after.
Case folding. git matches index pathspecs case-sensitively even where
core.ignorecase is set, but APFS does not -- so a repository that commits
`.venv/bin/Python` gets `$venv/bin/python` opening and running that file while the
guard's lowercase query finds nothing in the index and reports it untracked. The
witness logged two invocations. It applies to `venv` and `env` as well, and to any
folding of the name. The query now uses a `:(icase)` pathspec; all nine
directory-by-spelling combinations are refused, and an untracked venv still runs.
Module shadowing. `python -c "import pytest"` puts the working directory first on
sys.path, so a repository that commits a `pytest.py` in its root has that file
imported, and executed, by a check whose only job is to answer whether pytest is
installed. The probe is now `python -I -c "import pytest"` on the virtualenv, uv
and poetry paths alike. Isolation does not hide a real pytest -- it lives in the
interpreter's own site-packages, confirmed against a venv holding pytest 9.1.1.
Still true, and not something this hook can fix: running the repository's declared
suite runs the repository's code. `pytest` imports conftest.py, and the Node arm
runs package.json scripts. That is what a pre-push verification hook is for. The
line this guard draws is narrower and worth keeping -- a capability probe, and the
choice of which interpreter to trust, should not be things the pushed repository
gets to decide.
The guard added in 9cdc40e6 was incomplete. `git ls-files` reports paths as they
are indexed and does not follow symlinks, so a repository that commits `.venv` as
a symlink to its own root alongside a tracked `bin/python` gets asked about
`.venv/bin/python` -- a path git has never heard of -- and the answer is
"untracked". The interpreter then runs. Measured on that shape: the planted
executable logged two invocations against 9cdc40e6 and none against this commit.
`repo_ships_interpreter` now resolves the bin directory with `cd -P`/`pwd -P`,
resolves the worktree root the same way, and asks git about the resolved path
relative to it. The three cases that matter all hold: a plainly committed venv is
still refused, the symlink shape is now refused, and a developer's own untracked
venv still resolves and runs.
`cd -P`/`pwd -P` rather than `realpath` or `readlink -f`, because neither is
portable to a stock macOS.
This branch taught the hook to run `.venv/bin/python`, and that is a binary the
repository can supply. On main the Python arm only ever ran `pytest` from PATH --
the developer's own -- and on a machine without one it ran nothing at all, which
is exactly the machine this branch was written for. So the exposure is new, and
it arrived with the fix.
The hook is installed globally through core.hooksPath. Cloning a hostile
repository, committing nothing, and pushing it to your own fork is enough: the
pre-push hook finds the committed `.venv/bin/python`, runs it once to probe for
pytest and again to run the suite. Reproduced -- the planted executable logged
two invocations under the previous commit and none under this one.
A virtualenv is never committed. It is platform-specific binaries and every
Python project gitignores it, so `git ls-files --error-unmatch` separates the
two cases exactly: a developer's own venv is untracked and still resolves, a
tracked one is skipped with the reason printed. An absolute $VIRTUAL_ENV outside
the worktree reads as untracked, as it should.
Not addressed here, and worth a maintainer's view: `uv run` and `poetry run`
resolve from the repository's own lockfile, so they carry the same shape of
trust in a form this check cannot see. They are gated behind a lockfile being
present, and changing their semantics is a larger decision than this fix.
`[[ -n "${ECC_PYTEST_CMD:-}" ]]` asked whether the variable had a value, not
whether it was set, so `ECC_PYTEST_CMD=` fell through to virtualenv discovery
while `ECC_PYTEST_CMD=" "` failed the push. Two spellings of the same mistake,
two behaviours. Falling through is the wrong one: an override that evaluated to
nothing -- a command substitution that found no pytest, say -- then silently ran
a different runner than the operator named, which is exactly the substitution
this resolver refuses to make anywhere else. Both now fail closed.
`${ECC_PYTEST_CMD+set}` rather than `[[ -v ECC_PYTEST_CMD ]]`, because `-v` is
bash 4.2 and a stock macOS /bin/bash is 3.2, where it is not a false but a
syntax error. The hook runs under whatever `env bash` resolves to.
The override is still not probed -- probing runs the operator's command, and a
wrapper that ignores `--version` executes the whole suite and is then rejected
for not printing a version. What the gate can honestly do about a stale override
is refuse to be quiet about it, so a push that uses one now says so, every time,
and says the hook has not checked that it is pytest. A bypass that announces
itself is not the silent gate this resolver exists to prevent.
The fixture env is built from nothing instead of inheriting process.env with two
keys blanked. Blanking is no longer neutral: a blanked ECC_PYTEST_CMD is now an
override, and every one of these tests would have taken that branch.
Three defects, found by reviewing this branch against a running pytest rather
than by reading it.
Exit 5 is not a failure. pytest reserves it for NO_TESTS_COLLECTED, and
`|| fail "pytest failed"` collapsed it into a blocked push. The `|| fail`
predates this branch, but this branch is what makes it reachable: a repository
whose pyproject.toml only configures ruff or black, with pytest in its venv and
no test files, used to hit the "pytest is not installed" skip and now gets
gated. $VIRTUAL_ENV is the first candidate, so merely having a venv activated in
the pushing shell drags any requirements.txt repository into this path, and the
hook is installed globally. Reproduced with pytest 9.1.1. Exit 5 is now
non-blocking but loud -- a bad rootdir, testpaths or an unimportable conftest
also collects nothing, and swallowing that silently would reopen the hole this
resolver exists to close. Other non-zero codes now carry the code, because 1
(tests failed) and 4 (usage error) call for different responses.
The ECC_PYTEST_CMD probe ran the operator's command. Validating the override
with `--version` assumed it would answer like pytest. A wrapper that sets an
environment variable and execs pytest ignores the flag and runs the whole suite,
so the probe executed the tests, then rejected the command for not printing a
version, then blocked the push -- with the suite green. That is worse than the
silent gate the probe was added to close, so the override is taken as given
again: it is a deliberate setting, the hook cannot inspect it without running
it, and pointing it at something that is not pytest is the operator's call.
`is_pytest` still guards the PATH candidate, which this script composes itself,
where `pytest --version` is harmless. An empty override still fails closed.
The tests inherited the ambient environment. `runHermeticPythonPrePush` passed
process.env through, so an exported ECC_PYTEST_CMD or an activated virtualenv
resolved a pytest the fixture never created and the venv test failed for anyone
who runs the suite that way. Both variables are now neutralised in the base env.
Coverage: the gate had no test proving it blocks. Changing the run line to
`|| true` left all three previous tests green. Seven now cover a spaced venv
path, a red suite, exit 5, an override invoked exactly once with no probe, an
empty override, and the PATH candidate in both directions.
AGENTS.md makes immutability mandatory and the helper built `env` by assigning
into it. Rather than reassigning a `let` through spreads, the two stub paths are
now resolved before the object exists, so `env` is a single `const` built in one
expression with the conditional keys spread in. Nothing to mutate and nothing to
rebind.
Two holes in the resolver this branch added, both found in review.
A virtualenv path may contain spaces. `resolve_pytest` returned one string and
the caller expanded it unquoted, so `/home/me/my env/bin/python -m pytest` split
into `/home/me/my` and `env/bin/python`. The probe that accepted the candidate
was correctly quoted, so the hook reported the venv as usable and then failed to
run anything in it -- rejecting the push for a reason with nothing to do with
the code being pushed. It now builds an argv array and runs `"${PYTEST_CMD[@]}"`.
The resolver's contract is that every candidate is confirmed to be pytest, and
two of them were not. `ECC_PYTEST_CMD` was returned unchecked, so
`ECC_PYTEST_CMD=true` made the hook run `true -q`, exit 0 and report a Python
project verified by nothing. The PATH branch used `command -v pytest`, which
proves only that a file of that name exists. Both now go through `is_pytest`,
which runs `--version` and requires the output to name pytest -- `--version`
alone is not evidence, since `true --version` also exits 0.
A bad `ECC_PYTEST_CMD` fails the push rather than falling through to the next
candidate. An operator who set it asked for that command, and silently running
a different one hides the misconfiguration -- which is the same silent-gate
failure this branch exists to remove, one level along.
Three regression tests cover the three paths: a venv whose directory name
contains a space, an override that is not pytest, and an override that is.
Exact-head independent local Codex review PASS with no P0/P1. CI run 34680860653 attempt 2 passed at 0707cd431c. Includes HTTP/schema failure handling, coalesced sampling cache and regression tests. Disclosed P2 follow-ups remain in the merge-queue receipt. Rollback: revert this squash commit. No deployment or publication claim.
github-coordination sync listed every repo issue and pushed the epic label onto all of them (#3084). Scope the listing to issues carrying the policy's epic label plus issues whose body still holds the coordination marker (label-drift recovery), deduped by number, and reject an empty labels.epic in loadPolicy. Tests cover the filtered path and the recovery path with exact gh argv. Independent exact-head review passed with no P0/P1; CI 44/44 at the head.
tools/list and tools/call on main already admit `_meta` — MCP reserves it
for request metadata and a client may attach it to any request. ping still
refused every parameter, so a client that sends `_meta` on everything
(Codex does) got -32602 on its keepalive.
Rebased onto main and narrowed: when this branch was first written the same
gap existed on tools/list, which has since been fixed upstream. Only the
ping handler is left, so only the ping handler is touched.
Refs #2810
Generalize PR #2981 ownership protection to every managed target. Reject mismatched target state and preserve files that appear during writes or failed-install checkpoints. Keep prior hashes for managed files a failed attempt never writes.
Integrate PR #2980 preview wording and global dry-run propagation, with PR #2956 fail-closed environment validation and CLI/legacy regression coverage.
Fixes#2964. Fixes#2952.
Co-authored-by: ilkmajans-cpu <ilkmajans-cpu@users.noreply.github.com>
Co-authored-by: wellkilo <wellkilo@foxmail.com>
Addresses Greptile's review on #2994:
- missing-dependency.js no longer hardcodes a second copy of the four
runtime dependency versions; it reads them from package.json's
dependencies field instead, so the two can't silently drift apart.
describeMissingDependencyError() still recognizes a tracked
dependency even if package.json can't be read for some reason,
just without a version-pinned install command in that case.
- The regression test now asserts no ancestor directory of its
temp fixture has a node_modules, so a stray one wouldn't let
Node resolve ajv/sql.js from there and mask what the test is
actually meant to exercise. Also copies package.json into the
fixture, matching a real plugin-marketplace git clone and what
the version-lookup above now needs.
install-plan.js and install-apply.js both require ./lib/install/config at
load time, and that module required ajv unconditionally at the top of the
file even though ajv is only actually used when validating an
ecc-install.json. When ECC is installed via the Claude Code plugin
marketplace, the marketplace directory is a bare git clone with no
node_modules, so requiring ajv crashes commands like --list-profiles that
never touch install-config validation at all.
Same root cause in scripts/lib/control-pane/state.js: sql.js and
@iarna/toml were required at module scope even though they are only used
inside openSqlDatabase() and readTomlConfig(), so control-pane.js --help
crashed too.
Make both requires lazy so they only load when the feature that actually
needs them runs. For the case where ajv/sql.js/js-yaml/@iarna-toml is
genuinely needed and still missing, add a small helper that turns the raw
MODULE_NOT_FOUND into an actionable message naming the package and the
install command, instead of a stack trace (install-apply.js) or, worse, an
unhandled crash with a usage banner tacked on that reads like a bad
argument (install-plan.js, control-pane.js). Applied the same helper to
memory-mcp.mjs, where ajv is genuinely load-bearing (it compiles every MCP
tool's JSON schema up front) so it can't be made lazy the same way.
Added a regression test that copies just scripts/, schemas/, and
manifests/ into a directory with no node_modules anywhere above it in the
filesystem, which reproduces the plugin-marketplace install exactly, and
asserts install-plan.js and control-pane.js still work.
The uninstall cases run two full CLI passes (install, then uninstall)
over several hundred files under a flat 30s timeout, which is tight
enough on Windows CI to fail intermittently with spawnSync ETIMEDOUT.
install-apply.test.js already scales its timeout by platform for the
same reason; match that precedent rather than re-running past the flake.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The guided installer asks how ECC hooks should run, but that consent
lived only in the wizard path. Running install-apply directly with a
profile that includes hooks-runtime still materialized the hook runtime
with no disclosure and no decision.
Gate the apply layer instead, so every entry point is covered:
- disclose the six hook capability groups when a plan would materialize
the hook runtime, and refuse to apply until the caller decides
- --enable-hooks confirms the hook runtime; --no-hooks installs the rest
of the selection without it and records the reduced module closure in
install-state
- surface the pending decision as a dry-run warning
- show the same capability disclosure in the guided installer's plan
preview, so the wizard's hook question states what it is asking about
Plans that never materialize hooks (Kimi, --profile minimal,
--without baseline:hooks) are unaffected and need no flag. Repair and
uninstall operate on already-recorded state and stay unchanged.
The capability taxonomy and the held-materialization behavior come from
Samarjeet Singh Tomar's PR #2634, reworked to fit the single-decision
consent model that shipped with the guided installer in #2649.
Co-Authored-By: Samarjeet Singh Tomar <samar_tomar@hotmail.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Artifacts written by `evolve --generate` are inert: Claude Code (and every
spec-compliant Agent Skills client) injects only `name` + `description` at
startup and will not load an artifact missing them.
Today the generator writes:
- skills: `# {name}` with no frontmatter block at all
- commands: `# {cmd_name}` with no frontmatter block at all
- agents: `model`/`tools` only, no `name`, no `description`
So the whole evolve pipeline terminates in files that can never load. I hit
this on a real install: 12 generated artifacts across two projects, none of
which Claude Code had ever seen.
This adds a `_evolved_description()` helper and emits proper frontmatter for
all three artifact kinds. The description is sanitised for the two things that
break loaders: `: ` in an unquoted scalar (rejected by strict YAML parsers)
and `<`/`>` (system-prompt injection risk).
Adds two tests to tests/scripts/instinct-cli-evolve-generate.test.js. Both
fail against current main and pass with this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Juan Garibay