diff --git a/.consiliency/notes/derive_npm_flags.py b/.consiliency/notes/derive_npm_flags.py index 561a23c4..586a6d69 100644 --- a/.consiliency/notes/derive_npm_flags.py +++ b/.consiliency/notes/derive_npm_flags.py @@ -33,6 +33,8 @@ `production`, `workspaces` and `expect-results` are all `null|Boolean`, so without the drop they read as conditional and `--yes`/`-y` -- a real MCP launch form -- would be refused. + The drop is for CLASSIFICATION only; the fourth table below needs + the bit back, so it is read off the RAW type labels. `Array` "may repeat". Measured inert on npm 11.19.0 (no definition changes class either way, and none is `Boolean|Array`); the generator reports it if that ever stops being true. @@ -64,6 +66,26 @@ class either way, and none is `Boolean|Array`); the generator This subsumes the hand-coded `-y` special case `_npm_package_arg` used to carry, and makes `npm --silent exec pkg` -> `pkg` hold by construction. +THE FOURTH TABLE: WHICH BOOLEAN SPELLINGS ALSO CONSUME A LITERAL `null` +----------------------------------------------------------------------- +A boolean flag consumes a literal `true`/`false` as its value. A boolean whose +declared type ALSO carries `null` consumes a literal `null` as well, and `null` +is a real published npm package -- so the wrong answer in either direction +mints a wrong package identity. The rule, applied to the boolean table: + + a spelling is nullable iff its resolution target -- after shorthand + expansion and after stripping a leading `no-` -- is a definition whose RAW + declared type includes `null`. + +On npm 11.19.0 that is five definitions (`yes`, `optional`, `production`, +`workspaces`, `expect-results`) but EIGHTEEN spellings, because `y`, `ws`, +`n` and `no` are shorthands (`n` and `no` both expand to `--no-yes`) and every +short alias/shorthand key is emitted in both its `-x` and `--x` form. A +hand-written version of this table shipped in 2.5.1 with twelve entries and so +got `--y -ws -n --n -no --no` wrong. Hence: generated, and cross-checked at +the SPELLING level against nopt (`--verify`), because the definition-level +probe alone would not have caught a spelling omission. + Deliberately NOT a pytest: npm's schema is version-specific and CI has no npm. Behaviour is pinned by the pure unit tests in tests/test_version_checker.py; this script is what keeps those pins honest against a real npm. @@ -131,7 +153,11 @@ class either way, and none is `Boolean|Array`); the generator for (const [k, v] of Object.entries(m.definitions)) { const t = Array.isArray(v.type) ? v.type : [v.type]; const lit = t.find((x) => typeof x === 'string' || typeof x === 'number'); - const probes = ['zzarbitrary', 'true', 'false']; + // `null` is in the probe list because a nullable boolean takes it as its + // VALUE while every other boolean leaves it as the package name -- and + // `null` is a real published package, so this is the probe that keeps the + // nullable table honest instead of merely internally consistent. + const probes = ['zzarbitrary', 'true', 'false', 'null']; if (lit !== undefined) probes.push(String(lit)); definitions[k] = { type: t.map(label), @@ -139,10 +165,33 @@ class either way, and none is `Boolean|Array`); the generator probes: Object.fromEntries(probes.map((p) => [p, consumes(k, p)])), }; } + +// The same question asked of every SPELLING rather than every definition: +// does `npm exec null zz` resolve to `zz` (null consumed) or to +// `null` (null is the package)? The 2.5.1 defect was a spelling omission with +// a correct definition set, so a definition-level probe would have missed it. +const nullSpellings = {}; +const probeSpelling = (flag) => { + nullSpellings[flag] = !remain(['exec', flag, 'null', 'zz']).includes('null'); +}; +for (const [k, v] of Object.entries(m.definitions)) { + probeSpelling('--' + k); + probeSpelling('--no-' + k); + for (const s of (v.short === undefined ? [] : [].concat(v.short))) { + probeSpelling('-' + s); + probeSpelling('--' + s); + } +} +for (const key of Object.keys(m.shorthands)) { + probeSpelling('-' + key); + probeSpelling('--' + key); +} + console.log(JSON.stringify({ npm_version: require(path.join(root, 'package.json')).version, definitions, shorthands: m.shorthands, + null_spellings: nullSpellings, })); """ @@ -191,15 +240,18 @@ def expected_arity(cls: str, probe_results: dict[str, bool]) -> bool | None: return None -def build(schema: dict) -> tuple[dict[str, str], list[str], list[str], list[str]]: - """Return (flag -> class, conditional names, union-with-Boolean, unreadable).""" +def build( + schema: dict, +) -> tuple[dict[str, str], dict[str, bool], list[str], list[str], list[str]]: + """Return (flag -> class, flag -> nullable, conditional, union, unreadable).""" defs = schema["definitions"] classes: dict[str, str] = {} + nullable: dict[str, bool] = {} conditional: list[str] = [] union_with_boolean: list[str] = [] unreadable: list[str] = [] - def record(flag: str, cls: str) -> None: + def record(flag: str, cls: str, takes_null: bool = False) -> None: """Write one entry, refusing to silently reclassify an existing one. Definitions, `short` aliases and shorthands are written in that order, @@ -209,13 +261,30 @@ def record(flag: str, cls: str) -> None: silently because the generator and the table would agree with each other while both disagreeing with npm. Empty on 11.19.0; this exists so a future npm cannot make it non-empty quietly. + + The same applies to nullability, which is written twice for `-y`/`--y` + (once from `yes`'s `short`, once from the `y` shorthand): agreeing + today is not a reason to let a future npm disagree in silence. """ previous = classes.get(flag) if previous is not None and previous != cls: raise SystemExit( f"conflicting classification for {flag}: {previous} vs {cls}" ) + previous_null = nullable.get(flag) + if previous_null is not None and previous_null != takes_null: + raise SystemExit( + f"conflicting nullability for {flag}: {previous_null} vs {takes_null}" + ) classes[flag] = cls + nullable[flag] = takes_null + + # Nullability is read off the RAW type labels, BEFORE `classify` drops + # `null` as a non-value marker: the drop is what makes `--yes` a boolean + # at all, and this table is precisely the bit the drop throws away. + name_nullable: dict[str, bool] = { + name: "null" in info["type"] for name, info in defs.items() + } name_class: dict[str, str] = {} for name, info in defs.items(): @@ -243,15 +312,19 @@ def record(flag: str, cls: str) -> None: union_with_boolean.append(f"{name}: type={'|'.join(info['type'])}") if name in _POSITIVE: continue + # Only a BOOLEAN can be nullable: a VALUE flag consumes the next token + # whatever it is, so `null` is never a special case for it. + takes_null = cls == BOOLEAN and name_nullable[name] if cls in (BOOLEAN, VALUE): - record(f"--{name}", cls) + record(f"--{name}", cls, takes_null) for short in info["short"]: - record(f"-{short}", cls) - record(f"--{short}", cls) + record(f"-{short}", cls, takes_null) + record(f"--{short}", cls, takes_null) if cls == BOOLEAN: # `--no-X` negates a boolean and, like the boolean itself, - # consumes only a literal true/false. Verified against nopt. - record(f"--no-{name}", cls) + # consumes only a literal true/false -- plus `null` when the base + # definition is nullable. Verified against nopt. + record(f"--no-{name}", cls, takes_null) for key, expansion in schema["shorthands"].items(): if len(expansion) >= 2: @@ -265,19 +338,26 @@ def record(flag: str, cls: str) -> None: # the package when npm's package is `true` -- a wrong identity, # and `--silent true X` / `--silent false X` collapse onto X. cls = SKIP # a value is baked into the expansion + base = None else: target = expansion[0].lstrip("-") if target.startswith("no-"): # `--no-global`, `--no-yes`: boolean-arity iff the base is a # boolean. Otherwise unlisted -- `--no-color` refuses, safely. - cls = BOOLEAN if name_class.get(target[3:]) == BOOLEAN else CONDITIONAL + base = target[3:] + cls = BOOLEAN if name_class.get(base) == BOOLEAN else CONDITIONAL else: + base = target cls = name_class.get(target, CONDITIONAL) + # `n` and `no` both expand to `--no-yes`, so stripping the `no-` is + # what makes `-n`/`--n`/`-no`/`--no` inherit `yes`'s nullability -- + # the four spellings 2.5.1's hand-written table missed outright. + takes_null = cls == BOOLEAN and bool(base) and name_nullable.get(base, False) if cls in (BOOLEAN, VALUE, SKIP): - record(f"-{key}", cls) - record(f"--{key}", cls) + record(f"-{key}", cls, takes_null) + record(f"--{key}", cls, takes_null) - return classes, conditional, union_with_boolean, unreadable + return classes, nullable, conditional, union_with_boolean, unreadable def check_against_nopt(schema: dict) -> list[str]: @@ -285,15 +365,18 @@ def check_against_nopt(schema: dict) -> list[str]: This is the check that makes "verified against npm's parser" true rather than asserted. A BOOLEAN flag is predicted to consume a probe iff the probe - is exactly `true`/`false`; a VALUE flag is predicted to consume every - probe; a CONDITIONAL flag makes no prediction because it is unlisted. + is exactly `true`/`false` -- or `null`, when the declared type carries + `null`; a VALUE flag is predicted to consume every probe; a CONDITIONAL + flag makes no prediction because it is unlisted. """ bad: list[str] = [] for name, info in schema["definitions"].items(): cls = classify(info["type"]) for probe, observed in info["probes"].items(): if cls == BOOLEAN: - predicted = probe in ("true", "false") + predicted = probe in ("true", "false") or ( + probe == "null" and "null" in info["type"] + ) elif cls == VALUE: predicted = True else: @@ -307,8 +390,55 @@ def check_against_nopt(schema: dict) -> list[str]: return bad +_TABLE_NAMES = ( + "_NPM_VALUE_FLAGS", + "_NPM_BOOLEAN_FLAGS", + "_NPM_NULLABLE_BOOLEAN_FLAGS", + "_NPM_SKIP_FLAGS", +) + +# The nullable table is a SUBSET of the boolean table by construction, so it is +# excluded from the pairwise-disjointness check below and gets a containment +# check instead. +_DISJOINT_NAMES = ( + "_NPM_VALUE_FLAGS", + "_NPM_BOOLEAN_FLAGS", + "_NPM_SKIP_FLAGS", +) + + +def check_null_spellings( + schema: dict, classes: dict[str, str], live_nullable: set[str] +) -> list[str]: + """The derived nullable table must equal what nopt does, SPELLING by SPELLING. + + `check_against_nopt` asks the question of each definition; this asks it of + each spelling, which is where 2.5.1 went wrong -- the five nullable + definitions were right and six of their eighteen spellings were missing. + Restricted to the boolean table on purpose: a VALUE flag consumes `null` + like it consumes anything else, and a spelling that is not in any table + makes the scanner refuse rather than predict. + """ + observed = schema["null_spellings"] + booleans = {f for f, c in classes.items() if c == BOOLEAN} + bad: list[str] = [] + for flag in sorted(booleans): + if flag not in observed: + # Every boolean spelling this generator emits is enumerable in the + # node probe; if one is not, the two enumerations have drifted. + bad.append(f"{flag}: boolean table entry never probed against nopt") + continue + predicted = flag in live_nullable + if predicted != observed[flag]: + bad.append( + f"npm exec {flag} null zz: derived nullable={predicted}, " + f"nopt consumes null={observed[flag]}" + ) + return bad + + def read_committed_tables(source: pathlib.Path) -> dict[str, set[str]]: - """Parse the two tables out of version_checker.py with `ast`. + """Parse the four tables out of version_checker.py with `ast`. Deliberately NOT `import pmcp.manifest.version_checker`: that pulls in aiohttp/packaging/semver, so the verifier would only run inside the @@ -330,11 +460,7 @@ def read_committed_tables(source: pathlib.Path) -> dict[str, set[str]]: continue names = [t.id for t in node.targets if isinstance(t, ast.Name)] for name in names: - if name not in ( - "_NPM_VALUE_FLAGS", - "_NPM_BOOLEAN_FLAGS", - "_NPM_SKIP_FLAGS", - ): + if name not in _TABLE_NAMES: continue call = node.value if ( @@ -347,32 +473,38 @@ def read_committed_tables(source: pathlib.Path) -> dict[str, set[str]]: ast.literal_eval(element) for element in call.args[0].elts # type: ignore[attr-defined] } - missing = { - "_NPM_VALUE_FLAGS", - "_NPM_BOOLEAN_FLAGS", - "_NPM_SKIP_FLAGS", - } - found.keys() + missing = set(_TABLE_NAMES) - found.keys() if missing: raise SystemExit(f"not module-level literals in {source}: {sorted(missing)}") return found -def emit(classes: dict[str, str]) -> str: - def block(cls: str) -> str: - entries = sorted(f for f, c in classes.items() if c == cls) - body = "\n".join(f' "{e}",' for e in entries) +def nullable_table(classes: dict[str, str], nullable: dict[str, bool]) -> set[str]: + """The boolean spellings that also consume a literal `null`.""" + return {f for f, c in classes.items() if c == BOOLEAN and nullable.get(f)} + + +def emit(classes: dict[str, str], nullable: dict[str, bool]) -> str: + def render(entries: set[str] | list[str]) -> str: + body = "\n".join(f' "{e}",' for e in sorted(entries)) return "{\n" + body + "\n }" + def block(cls: str) -> str: + return render([f for f, c in classes.items() if c == cls]) + return ( f"_NPM_VALUE_FLAGS = frozenset(\n {block(VALUE)}\n)\n\n\n" f"_NPM_BOOLEAN_FLAGS = frozenset(\n {block(BOOLEAN)}\n)\n\n\n" + "_NPM_NULLABLE_BOOLEAN_FLAGS = frozenset(\n " + f"{render(nullable_table(classes, nullable))}\n)\n\n\n" f"_NPM_SKIP_FLAGS = frozenset(\n {block(SKIP)}\n)\n" ) def main() -> int: schema = read_schema() - classes, conditional, union, unreadable = build(schema) + classes, nullable, conditional, union, unreadable = build(schema) + live_nullable = nullable_table(classes, nullable) print( f"npm {schema['npm_version']}: " @@ -405,14 +537,25 @@ def main() -> int: for entry in mismatches: print(f" {entry}", file=sys.stderr) + null_mismatches = check_null_spellings(schema, classes, live_nullable) + print( + f"\nnullable spellings: {len(live_nullable)} derived, " + f"{len(null_mismatches)} mismatch(es) over " + f"{len(schema['null_spellings'])} spellings probed with a literal `null`", + file=sys.stderr, + ) + for entry in null_mismatches: + print(f" {entry}", file=sys.stderr) + print(f" derived: {' '.join(sorted(live_nullable))}", file=sys.stderr) + if "--verify" not in sys.argv: - print(emit(classes)) + print(emit(classes, nullable)) return 0 source = REPO / "src" / "pmcp" / "manifest" / "version_checker.py" committed_tables = read_committed_tables(source) print(f"\nverifying tables in {source}", file=sys.stderr) - failures = list(mismatches) + failures = list(mismatches) + null_mismatches for label, live, committed in ( ( "value", @@ -424,6 +567,11 @@ def main() -> int: {f for f, c in classes.items() if c == BOOLEAN}, committed_tables["_NPM_BOOLEAN_FLAGS"], ), + ( + "nullable", + live_nullable, + committed_tables["_NPM_NULLABLE_BOOLEAN_FLAGS"], + ), ( "skip", {f for f, c in classes.items() if c == SKIP}, @@ -437,12 +585,23 @@ def main() -> int: # A flag in both tables would let the boolean branch win or lose by table # order rather than by npm's schema; either way it is not a classification. - names = ("_NPM_VALUE_FLAGS", "_NPM_BOOLEAN_FLAGS", "_NPM_SKIP_FLAGS") - for i, left in enumerate(names): - for right in names[i + 1 :]: + for i, left in enumerate(_DISJOINT_NAMES): + for right in _DISJOINT_NAMES[i + 1 :]: for flag in sorted(committed_tables[left] & committed_tables[right]): failures.append(f"{flag} is in BOTH {left} and {right}") + # The nullable table is exempt from the disjointness check because it is a + # SUBSET of the boolean table -- so pin that instead. A nullable entry that + # is not a boolean entry is dead code: `_npm_package_arg` only consults it + # from inside the boolean branch, so it would never be reached. + for flag in sorted( + committed_tables["_NPM_NULLABLE_BOOLEAN_FLAGS"] + - committed_tables["_NPM_BOOLEAN_FLAGS"] + ): + failures.append( + f"{flag} is in _NPM_NULLABLE_BOOLEAN_FLAGS but not _NPM_BOOLEAN_FLAGS" + ) + if failures: print(f"\nFAIL: {len(failures)} discrepanc(ies)", file=sys.stderr) for entry in failures: @@ -450,7 +609,8 @@ def main() -> int: return 1 print( f"\nOK: {len(committed_tables['_NPM_VALUE_FLAGS'])} value + " - f"{len(committed_tables['_NPM_BOOLEAN_FLAGS'])} boolean + " + f"{len(committed_tables['_NPM_BOOLEAN_FLAGS'])} boolean " + f"({len(committed_tables['_NPM_NULLABLE_BOOLEAN_FLAGS'])} of them nullable) + " f"{len(committed_tables['_NPM_SKIP_FLAGS'])} skip entries " f"match live npm {schema['npm_version']}", file=sys.stderr, diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b2fb5cb..355da22a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,47 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [2.5.2] - 2026-08-26 + +### Fixed +- **Six npm flag spellings read a literal `null` as the package name.** 2.5.1 + added a table of the boolean flags that take a literal `null` as their *value* + rather than as the package name — `null` is a real published npm package, so + the distinction decides a server's identity, and `refresher.py`'s freshness + gate treats a matching identity as **positive confirmation** that a cached + tool description still describes the configured package. That table was + written by hand with 12 entries. npm has five nullable boolean definitions + (`yes`, `optional`, `production`, `workspaces`, `expect-results`) but + **eighteen** spellings for them, because `y`, `ws`, `n` and `no` are + shorthands — `n` and `no` both expand to `--no-yes` — and each is legal in + both its `-x` and `--x` form. The six that were missing are **`--y`, `-ws`, + `-n`, `--n`, `-no`, `--no`**: under 2.5.1 each of these read the following + `null` as the package name, so `npm exec -n null server-a` and + `npm exec -n null server-b` both resolved to the package `null` and could be + served each other's cached tool descriptions. + + **Not a regression between releases** — 2.4.1, 2.5.0 and 2.5.1 all resolve + these six to `null`; verified by running each released version's + `detect_package_type` directly. The blanket rule that briefly handled them + correctly existed only on `main` between two unreleased commits, so no shipped + version was ever right about them. 2.5.2 is the first. +- **The set is now generated, not hand-listed.** + `.consiliency/notes/derive_npm_flags.py` derives it from npm's own + `@npmcli/config` definitions: a spelling is nullable iff its resolution + target, after shorthand expansion and after stripping a leading `no-`, is a + definition whose declared type includes `null`. It was the fourth defect in + this parser traceable to hand-transcribing npm's behaviour. +- **`--verify` now covers this table**, which previously had no drift + protection at all. Each definition is probed with a literal `null`, and every + one of npm's 442 enumerable flag spellings is run through npm's own parser as + `npm exec null zz` — a definition-level check alone would not have + caught a spelling omission. The new check rejects the shipped 2.5.1 table + with exactly six mismatches. + + This does **not** close the broader gap tracked in + Consiliency/pmcp#195: attached values (`--global=pkg`), npx's own `-p=` + rewriting, and npx's `-n` removal are still unhandled. + ## [2.5.1] - 2026-08-26 ### Fixed diff --git a/pyproject.toml b/pyproject.toml index d24bbc22..f8d5bb38 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "pmcp" -version = "2.5.1" +version = "2.5.2" description = "PMCP - Progressive MCP: Minimal context bloat with on-demand tool discovery" readme = "README.md" license = "MIT" diff --git a/src/pmcp/__init__.py b/src/pmcp/__init__.py index 1662f21f..861e05f8 100644 --- a/src/pmcp/__init__.py +++ b/src/pmcp/__init__.py @@ -1,3 +1,3 @@ """PMCP - A meta-server for minimal Claude Code tool bloat.""" -__version__ = "2.5.1" +__version__ = "2.5.2" diff --git a/src/pmcp/manifest/version_checker.py b/src/pmcp/manifest/version_checker.py index 17f138d4..a44f4b96 100644 --- a/src/pmcp/manifest/version_checker.py +++ b/src/pmcp/manifest/version_checker.py @@ -761,15 +761,30 @@ def _npm_tag(package: str) -> str | None: # `--yes null B` both resolve to the package `null` (ah board review). _NPM_BOOLEAN_LITERALS = frozenset({"true", "false"}) -# Booleans whose declared type is `null|Boolean` -- these, and ONLY these, also -# consume a literal `null`. Verified against nopt: `npm exec --yes null zz` -# leaves `zz`, but `npm exec --global null zz` leaves `null zz`, so `null` is -# the package for every NON-nullable boolean. A blanket `null` rule therefore -# minted a wrong identity across the other ~180 boolean entries -- and `null` -# is a real published npm package (ah board review, correctness seat). +# GENERATED by .consiliency/notes/derive_npm_flags.py -- do not hand-edit. +# +# The boolean spellings whose declared type is `null|Boolean` -- these, and +# ONLY these, also consume a literal `null`. Verified against nopt: `npm exec +# --yes null zz` leaves `zz`, but `npm exec --global null zz` leaves `null zz`, +# so `null` is the package for every NON-nullable boolean. A blanket `null` +# rule therefore minted a wrong identity across the other ~180 boolean entries +# -- and `null` is a real published npm package (ah board review, correctness +# seat). +# +# Hand-listing it was the other way to get this wrong, and 2.5.1 did: the five +# nullable definitions have EIGHTEEN spellings, because `y`, `ws`, `n` and `no` +# are shorthands (`n` and `no` both expand to `--no-yes`) and each is emitted +# in both its `-x` and `--x` form. The hand-written table carried twelve, so +# `--y -ws -n --n -no --no` each minted the package `null`. The set is now +# derived -- a spelling is nullable iff its resolution target, after shorthand +# expansion and after stripping a leading `no-`, is a definition whose declared +# type includes `null` -- and `derive_npm_flags.py --verify` re-probes every +# spelling against nopt, so a future npm cannot change it silently. _NPM_NULLABLE_BOOLEAN_FLAGS = frozenset( { "--expect-results", + "--n", + "--no", "--no-expect-results", "--no-optional", "--no-production", @@ -779,7 +794,11 @@ def _npm_tag(package: str) -> str | None: "--production", "--workspaces", "--ws", + "--y", "--yes", + "-n", + "-no", + "-ws", "-y", } ) diff --git a/tests/test_version_checker.py b/tests/test_version_checker.py index f5ddc9dc..8f88728b 100644 --- a/tests/test_version_checker.py +++ b/tests/test_version_checker.py @@ -8,6 +8,7 @@ import pytest +from pmcp.manifest import version_checker from pmcp.manifest.version_checker import ( _digest_identity, _docker_image_name, @@ -2352,6 +2353,13 @@ def test_real_boolean_still_consumes_the_literal(self) -> None: "pkg", ) + def test_baked_value_shorthand_still_skips_an_ordinary_token(self) -> None: + """Not consuming a literal must not become not skipping at all.""" + assert detect_package_type("npm", ["--silent", "exec", "pkg"]) == ( + "npm", + "pkg", + ) + class TestNpxBooleanLiterals: """npx parses boolean switches differently from npm, so pmcp refuses. @@ -2379,12 +2387,21 @@ def test_npx_boolean_without_a_literal_still_resolves(self) -> None: class TestOnlyNullableBooleansConsumeNull: - """`null` is a real published npm package, and only 5 flags consume it. + """`null` is a real published npm package: 5 definitions, 18 spellings. Verified against nopt: `npm exec --yes null zz` leaves `zz` (nullable consumes), while `npm exec --global null zz` leaves `null zz` -- so `null` - is the package. A blanket rule applying `null` to all ~198 boolean entries - minted a wrong identity for the other ~193 (ah board review). + is the package. A blanket rule applying `null` to all 198 boolean entries + minted a wrong identity for the other 180 (ah board review). + + The opposite error shipped in 2.5.1: the table was hand-written with 12 of + the 18 spellings, so `--y -ws -n --n -no --no` each read the `null` as the + package. The 5 nullable definitions (`yes`, `optional`, `production`, + `workspaces`, `expect-results`) reach 18 spellings because `y`, `ws`, `n` + and `no` are shorthands -- `n` and `no` BOTH expand to `--no-yes` -- and + every short alias/shorthand key is legal in both `-x` and `--x` form. + The table is generated now; these pins are what keeps a regeneration + honest on a host without npm. """ def test_nullable_boolean_consumes_null(self) -> None: @@ -2393,20 +2410,123 @@ def test_nullable_boolean_consumes_null(self) -> None: "zz", ) + @pytest.mark.parametrize( + "flag", + [ + "-y", + "--y", + "--yes", + "-n", + "--n", + "-no", + "--no", + "-ws", + "--ws", + "--workspaces", + "--no-yes", + "--optional", + "--no-optional", + "--production", + "--no-production", + "--expect-results", + "--no-expect-results", + "--no-workspaces", + ], + ) + def test_every_nullable_spelling_consumes_null(self, flag: str) -> None: + """All 18, one per spelling. 2.5.1 failed six of these. + + Cross-checked against nopt with npm's own definitions and shorthands: + `nopt(types, shorthands, ["exec", flag, "null", "zz"]).argv.remain` is + `["exec", "zz"]` for every flag here, i.e. the `null` is the flag's + value and `zz` is the package. + """ + assert detect_package_type("npm", ["exec", flag, "null", "zz"]) == ( + "npm", + "zz", + ) + def test_non_nullable_boolean_does_not_consume_null(self) -> None: assert detect_package_type("npm", ["exec", "--global", "null", "zz"]) == ( "npm", "null", ) - def test_true_and_false_are_consumed_by_any_boolean(self) -> None: - assert detect_package_type("npm", ["exec", "--global", "false", "a"]) == ( + @pytest.mark.parametrize("flag", ["-g", "--global", "-f", "--force", "--no-global"]) + def test_non_nullable_spellings_leave_null_as_the_package(self, flag: str) -> None: + """The other direction: over-broad is just as wrong as too narrow. + + nopt leaves `["exec", "null", "zz"]` for each of these, so the package + really is `null` and consuming it would collapse two configs. + """ + assert detect_package_type("npm", ["exec", flag, "null", "zz"]) == ( "npm", - "a", + "null", ) - def test_baked_value_shorthand_still_skips_an_ordinary_token(self) -> None: - assert detect_package_type("npm", ["--silent", "exec", "pkg"]) == ( + def test_nullable_spellings_do_not_collapse_two_configs(self) -> None: + """The identity consequence, stated directly. + + Under 2.5.1 both of these resolved to `null`, so the freshness gate + would confirm one server's cached tools against the other's config. + """ + got_a = detect_package_type("npm", ["exec", "-n", "null", "server-a"]) + got_b = detect_package_type("npm", ["exec", "-n", "null", "server-b"]) + assert got_a == ("npm", "server-a") + assert got_b == ("npm", "server-b") + assert got_a != got_b + + def test_nullable_table_is_a_subset_of_the_boolean_table(self) -> None: + """A nullable entry outside the boolean table is unreachable. + + `_npm_package_arg` only consults the nullable table from inside the + boolean branch, so an entry missing from `_NPM_BOOLEAN_FLAGS` would be + dead. `derive_npm_flags.py --verify` pins the same property against a + live npm; this pins it without one. + """ + assert ( + version_checker._NPM_NULLABLE_BOOLEAN_FLAGS + <= version_checker._NPM_BOOLEAN_FLAGS + ) + + def test_nullable_table_is_pinned_to_the_exact_derived_set(self) -> None: + """The table is pinned by VALUE, not just by shape. + + Every other check here is one-directional: the parametrized spellings + prove each of the 18 consumes `null`, and the subset check proves none + is unreachable. An *extra* entry passes both -- and an extra entry is a + wrong identity in the other direction, since a non-nullable flag would + then swallow a `null` that npm treats as the package. + + Pinned as a literal so a table change has to be a deliberate edit here + too. `derive_npm_flags.py --verify` re-derives it against a live npm; + this pins it without one (board review, adversarial seat). + """ + assert version_checker._NPM_NULLABLE_BOOLEAN_FLAGS == frozenset( + { + "--expect-results", + "--n", + "--no", + "--no-expect-results", + "--no-optional", + "--no-production", + "--no-workspaces", + "--no-yes", + "--optional", + "--production", + "--workspaces", + "--ws", + "--y", + "--yes", + "-n", + "-no", + "-ws", + "-y", + } + ) + + def test_true_and_false_are_consumed_by_any_boolean(self) -> None: + assert detect_package_type("npm", ["exec", "--global", "false", "a"]) == ( "npm", - "pkg", + "a", ) diff --git a/uv.lock b/uv.lock index f9630657..d61e51d0 100644 --- a/uv.lock +++ b/uv.lock @@ -1068,7 +1068,7 @@ wheels = [ [[package]] name = "pmcp" -version = "2.5.1" +version = "2.5.2" source = { editable = "." } dependencies = [ { name = "aiohttp" },