Skip to content

detect_package_type collapses distinct packages for some command forms, so the identity gate can falsely confirm #180

Closed

Description

@ViperJuice

Found by the red-team seat of the pre-merge board on #179 (v12 UPDPATH) and confirmed empirically.

detect_package_type returns a package name that is not unique for several legal command forms, so two genuinely different packages resolve to the same identity:

docker ['run', 'registry:5000/old-image'] -> ('docker', 'registry')
docker ['run', 'registry:5000/new-image'] -> ('docker', 'registry')
npm   ['exec', 'old-pkg']                 -> ('npm', 'exec')
npm   ['exec', 'new-pkg']                 -> ('npm', 'exec')

The docker case comes from raw.split(":")[0] (src/pmcp/manifest/version_checker.py:316), which is parsing a registry host:port as an image tag separator. The npm case takes the first argument as the package name without accounting for exec.

Why it matters now

This is pre-existing — the parser has always behaved this way, and before UPDPATH nothing compared package names, so a collapsed name only ever produced a wrong version lookup. UPDPATH makes it consequential: _same_package compares these names to decide whether a cached description still describes the configured package. For the forms above it confirms identity across a real package swap, so the freshness short-circuit fires and the wrong package's tool descriptions keep being served — exactly the defect UPDPATH exists to close, unmet for those command forms.

So EC-UPDPATH-1, -3 and -6 hold for the ordinary forms (npx -y <pkg>, uvx <pkg>, docker run <image>) and do not hold for these.

Why it was not fixed in UPDPATH

detect_package_type backs every version lookup in the codebase, not just the identity gate. Changing its parsing has blast radius well beyond that phase and deserves its own RED tests and review round rather than being appended to a closeout. Deliberately deferred by the maintainer with the phase's limitation recorded in specs/phase-plans-v12.md.

Repro

from pmcp.manifest.version_checker import detect_package_type
assert detect_package_type("docker", ["run", "registry:5000/old-image"]) == ("docker", "registry")
assert detect_package_type("docker", ["run", "registry:5000/new-image"]) == ("docker", "registry")

Suggested scope

  • Parse a docker reference properly: a : only separates a tag when it appears after the last /, so registry:5000/img has no tag while img:1.2 does.
  • Skip an exec / run subcommand before taking the npm package name.
  • Decide what an unparseable form should return. Returning "unknown" is safe for the identity gate (unknown forces a refresh) but changes version-lookup behaviour, so the two callers may want different handling.
  • Whatever the fix, pin it with cases that would have failed before it — a collision test that passes today proves nothing.

Activity

  1. ViperJuice commented on Aug 24, 2026

    @ViperJuice
    CollaboratorAuthor

    Filed from the v12 UPDPATH closeout. Recorded as a known limitation in specs/phase-plans-v12.md's UPDPATH post-execution amendments, so the next phase inherits the boundary rather than assuming the identity gate is total.

  2. ViperJuice commented on Aug 25, 2026

    @ViperJuice
    CollaboratorAuthor

    Narrowed by #184 (5043d11), not closed — deliberately left open.

    Fixed

    Both measured collisions in this issue:

    docker run registry:5000/old-image  vs  new-image   -- were both ('docker','registry')
    npm exec old-pkg                    vs  new-pkg     -- were both ('npm','exec')
    

    Docker split on the first :, so a registry host:port read as the image name. The correct rule already existed in the module as _docker_image_tag, so the fix adds its paired complement _docker_image_name rather than a second, divergent implementation. npm had no subcommand skip; one now fires once, leading-only, for npm only (exec, x, run, install, i, add, create, dlx), so npm install i still finds the real package i and npx -y exec still finds a package named exec.

    Two live defects surfaced along the way and were also fixed: img@sha256:abc reported a digest fragment as a version pin, and npm exec pkg@1.2 reported a real pin as unpinned.

    Still open here

    The board reviewing that PR falsified the plan's premise — that only tag-stripping was broken. Identity is still collapsed wherever a flag's value is taken as the package name, and these are ordinary forms:

    docker run --env-file .env old-image / new-image      -> both ('docker', '.env')
    docker run --mount type=bind,src=/a A / B             -> both ('docker', 'type=bind,...')
    npm exec --package=old-pkg / new-pkg -- shared-bin    -> both ('npm', 'exec')
    

    _docker_image_arg's _value_flags table is incomplete, and _npm_package_arg discards option tokens wholesale. Closing those needs either an exhaustive per-ecosystem flag table or a fail-closed contract — a parser redesign rather than a bounded fix, so it is tracked on #182 (broadened to cover them) alongside the uvx/pip/cargo cases.

    #183 tracks the more serious consequence of a misparse: update_server builds npx -y {parsed}@latest --help, and npx -y installs without prompting.

    This issue stays open until the identity gate actually holds for ordinary commands. Marking it fixed now would put a closed label on a live hole.

  3. ViperJuice commented on Aug 26, 2026

    @ViperJuice
    CollaboratorAuthor

    Re-narrowed by #190 (6a5d84e, which closed #182). Still open, deliberately.

    Now closed

    All three forms named in this issue's earlier narrowing comment:

    docker run --env-file .env old/new    -> ('docker','old-image') / ('docker','new-image')
    docker run --mount type=bind,… A/B    -> ('docker','A') / ('docker','B')
    npm exec --package=old/new -- bin     -> ('npm','old-pkg') / ('npm','new-pkg')
    

    …plus the two original forms, which stay fixed. Verified independently on the merged tree, not taken from the PR.

    Still open — the npm residual

    _npm_package_arg remains the one ecosystem that fails open on unlisted flags, and these are ordinary forms:

    npm exec --loglevel silly a / b       -> both ('npm','silly')
    npm exec --registry https://r a / b   -> both ('npm','https://r')
    npx -y --registry https://r a / b     -> both ('npm','https://r')
    

    By this issue's own closing sentence — "until the identity gate actually holds for ordinary commands" — that is not closed.

    Why it wasn't converted in #190. Making npm fail-closed needs an enumeration of npm's boolean globals, or the pinned ordering npm --silent exec pkg → pkg breaks. That is a new design surface deserving its own board round. The middle ground — a fail-open npm value table — is exactly the shape #190's plan rejected after #183's board, so it was not introduced into a fifth ecosystem to close an issue faster.

    Mitigating: once --package is seen the scan stops, so the residual is confined to bare value flags appearing before the package.

    Correction to this issue's own text

    The earlier narrowing comment says npm exec --package=old-pkg -- shared-bin yielded ('npm','exec'). Post-#184 main actually yielded ('npm','bin') — stale, and worth correcting so the next reader measures rather than trusts it.

    Scope if picked up

    • Enumerate npm's boolean globals from npm --help / npm exec --help mechanically, not from memory. fix(parser): classify flag values so a flag's argument is never the package name (#182) #190 found ten entries across the other four ecosystems that had been drafted from recall and could not be verified; all ten were removed rather than kept, because an unverified entry fails open.
    • Keep npm --silent exec pkg → pkg passing; it is pinned at tests/test_version_checker.py.
    • Pin each pair with an inequality assertion proved RED first, and mutation-prove per node with an import-provenance guard — without one, every mutant falsely survives while the unmutated tree serves the imports.
  4. added 7 commits that reference this issue on Aug 26, 2026
    bfce366
    6b56e6d
    ea8333c
    ffd90c1
    7aa7d4e
    dd39aa8
    d3ddbd4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions