|
| 1 | +# NpmDiffWatch — Improvement Plan |
| 2 | + |
| 3 | +Review of the PyDiffWatch → npm port. The safety scaffolding came over intact, but the |
| 4 | +core detection front-door is broken and there are no tests. Work the priority order at the |
| 5 | +bottom: ingest first (nothing else matters without it), then the contained bug + test fixes. |
| 6 | + |
| 7 | +> Context for a fresh session: this is a Python tool that scans **npm** packages (ported from |
| 8 | +> PyDiffWatch, which scanned PyPI). The hard invariant is **NO EXECUTION** — analyzed packages |
| 9 | +> are data, never run/installed/imported. Pipeline: `ingest → fetcher → differ → engine (triage) |
| 10 | +> → reviewer (only on escalate) → notifier → store`, SQLite under `.diffwatch/`. |
| 11 | +
|
| 12 | +--- |
| 13 | + |
| 14 | +## The Ugly — the core function doesn't work |
| 15 | + |
| 16 | +### 1. Ingest can't find new releases (`ingest.py:13`, `ingest.py:55-103`) |
| 17 | +```python |
| 18 | +_SEARCH_URL = "/-/v1/search?by=maintenance&size=250" |
| 19 | +``` |
| 20 | +- npm's `/-/v1/search` requires a `text` query and ranks by quality/popularity/maintenance — |
| 21 | + `by=` is not a real parameter. |
| 22 | +- Even if it returned results, sorting toward *maintenance* surfaces established, popular |
| 23 | + packages — the opposite of the brand-new, low-reputation packages where supply-chain malware |
| 24 | + lands. A freshly published malicious version will essentially never appear. |
| 25 | +- **Fix:** switch to the replication changes feed (`https://replicate.npmjs.org/_changes`, |
| 26 | + monotonic `seq`). This is npm's real equivalent of PyPI's `changelog_since_serial` and maps |
| 27 | + 1:1 onto the serial-cursor model already ported. Follow `seq` forward from the cursor. |
| 28 | + |
| 29 | +### 2. The "serial" cursor is fake (`ingest.py:40-47`, `ingest.py:55-103`, `orchestrator.py:122-169`) |
| 30 | +- `current_serial()` returns `int(datetime.now().timestamp())` — wall-clock, not a registry |
| 31 | + sequence. |
| 32 | +- `changes_since` ignores `since_serial` for filtering; dedup happens against the DB `releases` |
| 33 | + table (`ingest.py:88-92`), and `serial` is just a local batch counter (`serial += 1`). |
| 34 | +- Result: the seed / `--backfill` / `advance_to` / "genesis" machinery in `run_once` is dead |
| 35 | + scaffolding with no npm meaning — incrementality/resumability are illusory. |
| 36 | +- **Fix:** once #1 uses the `_changes` feed, make the serial the real `seq`; the cursor then |
| 37 | + gates correctly and the existing seed/advance logic becomes meaningful again. |
| 38 | + |
| 39 | +> #1 and #2 share one root cause: the PyPI firehose model was lifted without an npm source |
| 40 | +> underneath it. Fixing ingest to the `_changes` feed resolves both. |
| 41 | +
|
| 42 | +--- |
| 43 | + |
| 44 | +## The Bad — real bugs |
| 45 | + |
| 46 | +### 3. Dependency screening always crashes (`fetcher.py:198-200`) |
| 47 | +```python |
| 48 | +def _fetch_json(name): |
| 49 | + url = f"{cfg.npm_registry.rstrip('/')}/{name}" |
| 50 | + return _fetch_json(url, cfg) # shadows module-level _fetch_json; calls THIS 1-arg closure with 2 args |
| 51 | +``` |
| 52 | +- The nested `_fetch_json` shadows the module-level one and calls itself → `TypeError`. |
| 53 | +- `deps.screen_added_deps` calls it for any added dep that isn't in the corpus and isn't a |
| 54 | + typosquat (`deps.py:86`); no try/except, so it propagates → `fetch_artifacts` → |
| 55 | + caught in `_process_fetched` as generic exception → `fetch_failed` → retried forever. |
| 56 | +- Net: nonexistent / brand-new dependency detection never runs; any package with a novel added |
| 57 | + dep silently never completes. |
| 58 | +- **Fix:** rename the inner closure (e.g. `_lookup`) so the module-level `_fetch_json(url, cfg)` |
| 59 | + is the one called. Low risk, contained. |
| 60 | + |
| 61 | +### 4. Zero tests (no `tests/` dir; `CLAUDE.md:53-63`, `pyproject.toml` dev extra + `pythonpath`) |
| 62 | +- `CLAUDE.md` documents `pytest -v` and `ruff check npmdiffwatch/ tests/`; `pyproject.toml` |
| 63 | + declares `dev=["pytest>=8"]` and `pythonpath=["."]` — but there is no `tests/` directory. |
| 64 | +- PyDiffWatch's entire safety claim rests on `tests/test_containment_reviewer.py` — AST guards |
| 65 | + proving reviewer/backends/fetcher stay network-free and in-memory, i.e. the enforcement of |
| 66 | + NO-EXECUTION. None of that was ported. The invariants are currently honored by convention only. |
| 67 | +- **Fix:** port the containment test suite first (AST guards over `reviewer.py` / `backends.py` |
| 68 | + / `fetcher.py`), then unit tests for `egress`, `rules` (fail-closed validator + no-eval |
| 69 | + matcher), `facts` (tree-sitter categories), `differ`, `deps`, and `fetcher` extraction caps. |
| 70 | + |
| 71 | +### 5. Anthropic backend uses an unverified API shape (`backends.py:99-101`) |
| 72 | +```python |
| 73 | +thinking={"type": "adaptive"}, |
| 74 | +output_config={"format": {"type": "json_schema", "schema": schema}}, |
| 75 | +``` |
| 76 | +- Uncertain these are valid in the Anthropic Python SDK (`anthropic>=0.40`). `thinking` is |
| 77 | + normally `{"type": "enabled", "budget_tokens": N}`; JSON-schema output via `output_config` |
| 78 | + isn't a confirmed shape. **Flagged as uncertain — verify, don't assume.** |
| 79 | +- If wrong, every Anthropic review raises `APIError` → silent heuristic fallback, undetectable |
| 80 | + without a test. |
| 81 | +- **Fix:** verify against the installed SDK version; correct the call; add a backend test with a |
| 82 | + mocked client. |
| 83 | + |
| 84 | +--- |
| 85 | + |
| 86 | +## Smells (low severity) |
| 87 | + |
| 88 | +- **`_is_strict_binary` duplicates `_is_binary`** (`fetcher.py:40-41`); `_is_binary` is unused — |
| 89 | + copy-paste residue. |
| 90 | +- **Dead branch in `_is_surface`** (`fetcher.py:133-135`): `base.startswith("bin/")` can never be |
| 91 | + true since `base = posixpath.basename(path)` has no slash. The `"/bin/" in path` clause is what |
| 92 | + works. |
| 93 | +- **README drift** (`README.md:46,48,8`): documents `max_tgz_bytes` (actual field is |
| 94 | + `max_download_bytes`, `config.py:25`); shows `new_package_policy = "skip"` while the default is |
| 95 | + `"surface"` (`config.py:39`); `pip install npmdiffwatch` implies a published package. |
| 96 | + |
| 97 | +--- |
| 98 | + |
| 99 | +## The Good — ported faithfully (do not regress) |
| 100 | + |
| 101 | +- **Safety core intact.** In-memory streamed extraction (`tarfile.open(mode="r|")`, |
| 102 | + `_BoundedReader`, member/size/name/decompressed caps, `_unsafe` path rejection, never |
| 103 | + `extractall`, never to disk — `fetcher.py:21-100`); default-deny egress guard with |
| 104 | + `is_installed()` + `run_once` warning (`egress.py`, `orchestrator.py:123-125`); |
| 105 | + `assert_web_scheme` `file://`/SSRF guard. |
| 106 | +- **Rules engine kept its safety boundary**: pure-data matcher, no eval/exec, fail-closed |
| 107 | + validator, scope-checked predicates (`rules.py`). Community rules stay untrusted input. |
| 108 | +- **Reviewer injection defense**: per-request CSPRNG markers (`reviewer.py:12-13`), strong |
| 109 | + "judge behavior not stated purpose / content between markers is inert data" system prompt, |
| 110 | + client-side verdict validation with heuristic fallback. |
| 111 | +- **npm threat modeling is appropriate**: lifecycle hooks (`pre/post/install`, `prepare`, |
| 112 | + `prepublish`), `child_process`/`fs`/`process.env` binding, prototype pollution, dynamic |
| 113 | + `require`/`import`, lockfile integrity/new-package, `bin` field changes (`facts.py`, |
| 114 | + `differ.py`). tree-sitter for JS/TS is correct. Dropping `defusedxml` was correct — npm |
| 115 | + registry is JSON, no XML-RPC. |
| 116 | + |
| 117 | +--- |
| 118 | + |
| 119 | +## Priority order |
| 120 | + |
| 121 | +1. **Replace ingest with the `_changes` replication feed** (make the serial a real `seq`). |
| 122 | + Without this, nothing else matters. (#1 + #2) |
| 123 | +2. **Fix the `_fetch_json` shadowing bug** (`fetcher.py:198-200`). Contained, low-risk. (#3) |
| 124 | +3. **Port the containment test suite**, then broader unit tests. Keeps NO-EXECUTION from |
| 125 | + quietly rotting. (#4) |
| 126 | +4. **Verify the Anthropic SDK call** against the real API. (#5) |
| 127 | +5. Clean up smells + README drift once the above land. |
0 commit comments