Skip to content

colleague_inventory is a drift detector, not an enforcement gate: 30 evasions and 6 false positives found by adversarial live test #7

Description

@OriNachum

An adversarial live test of scripts/colleague_inventory.py (shipped in #5,
hardened in #6) produced 30 executed evasions at exit 0 and 6 false
positives
. Every result below was run and observed — the evasion fixtures each
created a real process.

The gate is a drift detector, not an enforcement boundary. It reliably
reproduces a pinned inventory and catches the naive forms it was written
against. It does not deliver the property t73 and the #1 §17 comment claim for
it: "a NEW unclassified path fails CI immediately." That claim is retracted.

Why this matters more than a normal bug list

The gate's stated purpose is that colleague cannot grow a new unmediated
subprocess path unnoticed while the known 13 migrate. Three of the findings
defeat that purpose outright, and one of them is architectural rather than a
missing case.

1. _SPAWN_CALLS is a 12-entry literal set (15 evasions)

Anything outside the set is invisible. subprocess.getoutput("cmd") alone
defeats the gate in one line.

Also evading: subprocess.getstatusoutput, os.posix_spawn, os.posix_spawnp,
os.execve, os.execlp, os.spawnl, os.spawnvp, os.fork+exec, pty.spawn,
multiprocessing.Process, os.popen2, runpy.run_module,
ProcessPoolExecutor.

Sharpest instance: import asyncio.subprocess then
asyncio.subprocess.create_subprocess_exec(...) evades, because the set holds
asyncio.create_subprocess_exec — a call the author explicitly tried to cover
slips through on its fully-qualified submodule path.

2. The allow-list is keyed per MODULE, not per SITE (architectural)

This is the most serious finding. Three brand-new spawns were added to
tools.py — two of them shell=True, running rm -rf and cat /etc/shadow
and the gate returned exit 0 with no signal whatsoever.

15 modules are already allow-listed, including tools.py, which is the single
most important one. Any new unmediated path added to an existing allow-listed
file is invisible by design. The gate protects only against spawns appearing
in the ~1 module that is not yet allow-listed.

No amount of widening _SPAWN_CALLS fixes this. It needs per-site pinning:
record the expected spawn-site count (or a hash of the site set) per module at
the pinned SHA, and fail when any module's count changes.

3. One level of indirection defeats resolution (7 evasions)

_resolve_call() understands ast.Name and ast.Attribute against import
bindings only. Assignment aliasing is not tracked at all, so the alias hardening
added in #6 covers only the import ... as form:

sp = subprocess;            sp.run([...])          # exit 0
runner = subprocess.run;    runner([...])          # exit 0
getattr(subprocess, "run")([...])                  # exit 0
importlib.import_module("subprocess").run([...])   # exit 0

Plus a dict of callables, functools.partial, and a class attribute.

4. Structural evasions (6)

Most realistic — sibling re-export, entirely inside colleague/:

# colleague/shim.py
run = subprocess.run          # a reference, never a Call node -> 0 findings
# colleague/evil.py
from .shim import run
run([...])                    # relative imports are skipped -> never binds

Both files sit in colleague/ and the gate reports 0 spawn sites. The
absolute form from colleague.shim import run evades identically.

Also: exec() of a source string; a spawn in a file outside colleague/ that
colleague imports (rglob only walks the package); and a symlinked
subdirectory
inside colleague/rglob does not follow it, yet
import colleague.sub.mod works and spawns.

5. shell=True detection is a literal-spelling artifact

Detection requires a literal ast.Constant. All of these shell out while
reporting shell_true_sites: []:

subprocess.run(cmd, shell=SH)
subprocess.run(cmd, **{"shell": True})
subprocess.run(cmd, shell=bool(1))

So "exactly two shell=True sites" in CLAUDE.md is a statement about how
colleague spells its arguments, not a measurement of its behaviour.
It happens
to be true at the pin; it is not a property the tool can guarantee.

6. False positives — the gate fails CI on ordinary code (6)

Two root causes.

Unbound-base fall-through. _resolve_call fails closed, so any unresolved
os.* / subprocess.* chain counts. A parameter named os calling
os.system() on a plain object was flagged though it returns "linux" and
creates no process. Likewise os = self.os, for os in hosts, and
from mypkg.compat import subprocess (a vendored shim — ImportFrom populates
only direct_names, never module_aliases).

Scope-blind bindings. _import_bindings walks the module flat, so one
legitimate from subprocess import run makes every later local variable named
run a finding. A fixture reported 2 sites where only 1 was real — inflating
counts, which also corrupts debt_remaining.

A gate that cries wolf on a variable named os gets disabled, which is its own
failure mode.

What still works

Worth stating, because the tool is not worthless:

Case Exit Correct
Allow-listed-only fixture 0 yes
Unclassified plain subprocess.run 1 yes
Unparseable file 2 yes
Real colleague checkout 0 yes — 21 sites / 15 modules / 13 debt, SHA matches

The three import forms hardened in #6 (import subprocess as sp,
from subprocess import run as r, function-nested imports) are caught. That
work was real; it just addressed one evasion class out of five.

Proposed resolution

  1. Relabel honestly, now. Every description — the module docstring, the CI
    job comment, CLAUDE.md, the delivery summary, the Build brief: shell-cli — the guarded local operations plane for AI agents #1 §17 comment — says
    drift detector against a pinned baseline, never enforcement gate. Done in
    docs: delivery summary for the M0 opening slice #6.
  2. Per-site pinning (fixes finding 2, the only architectural one): pin
    per-module spawn-site counts at the pinned SHA; fail on any delta. This
    restores a defensible version of the original claim.
  3. Widen _SPAWN_CALLS and match on module prefix so submodule paths
    resolve.
  4. Scope-aware bindings and drop unbound-base fall-through, or downgrade it
    to a warning — fixes the false positives.
  5. Accept the residual. A static AST scanner cannot stop a determined author
    (exec, dynamic import, indirection). The honest ceiling is "detects
    accidental and careless drift from a pinned baseline"
    — which is, notably,
    the same posture the rest of this repo already commits to for the execution
    guard. The gate should make the same promise and no more.

Items 2–4 deserve their own reviewed slice; they change the pinned baseline and
the tool's contract.

Acceptance criteria

  • No description of the tool anywhere claims it prevents new unmediated spawn
    paths.

  • A new spawn added to an already-allow-listed module fails --check.

  • subprocess.getoutput and the other 14 missing APIs are detected.

  • A parameter or local variable named os or run does not produce a finding.

  • The evasion fixtures from this report become regression tests.

  • shell-cli (Claude)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions