Skip to content

Fix: Explicit adapterConfig attach allow-lists and did-you-mean suggestions - #475

Closed
abhijeetnardele24-hash wants to merge 3 commits into
debugmcp:mainfrom
abhijeetnardele24-hash:fix/issue-466-unknown-attach-keys
Closed

Fix: Explicit adapterConfig attach allow-lists and did-you-mean suggestions#475
abhijeetnardele24-hash wants to merge 3 commits into
debugmcp:mainfrom
abhijeetnardele24-hash:fix/issue-466-unknown-attach-keys

Conversation

@abhijeetnardele24-hash

Copy link
Copy Markdown
Contributor

Closes #466. Adds \supportedAttachKeys\ to debug adapters to strictly warn on unknown adapterConfig keys during attach, replacing the previous deny-list behavior. Also includes a did-you-mean helper to provide typo suggestions (e.g. pathMapping -> pathMappings).

@debugmcpdev debugmcpdev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking on #466 — the direction is right (per-adapter declared key sets instead of a deny-list, plus did-you-mean; that's what the issue asked for), and the Levenshtein helper with the short-string threshold adjustment is a nice touch. It needs a round of changes before it can merge:

Must fix

  1. Un-stack the branch. This PR currently contains your #473 and #474 commits plus the regenerated pnpm-lock.yaml (which removes the security overrides block and downgrades esbuild) and a placeholder edit to pnpm-workspace.yaml. Rebase onto upstream main with only the #466 changes; the lockfile and workspace files must not appear in the diff. (#471 is now fixed on main via #476, and #474 has its own review.)
  2. Truthful warning semantics. For python, transformAttachConfig spreads unknown keys through to debugpy — so a key outside supportedAttachKeys is forwarded unrecognised, not "ignored". The issue was explicit about this refinement ("keep pass-through, just say so"): silence must never mean "understood", but the warning also must not claim we dropped something we actually sent. Suggest either (a) wording per adapter based on whether its transform passes unknowns through, or (b) actually stripping unknown keys when a supportedAttachKeys list exists, so "ignored" becomes true — (b) is cleaner but is a behavior change worth a sentence in the PR body.
  3. Keep stored state clean. session.attachDroppedConfigKeys now stores decorated strings like pathMapping (did you mean pathMappings?). That field is state, read later to build the user-facing warning — store bare key names and add the did-you-mean decoration only where the warning string is composed (session-manager-operations.ts ~line 2832). Otherwise any future consumer comparing those entries against real config keys breaks.
  4. Don't delete the #450 rationale. The doc comment on transformAttachConfig in packages/shared/src/interfaces/debug-adapter.ts explains why the deny-list+pass-through contract existed. Amend it to describe the new supportedAttachKeys layer on top; the history of why is what stops the next person from re-simplifying it.
  5. Tests. At minimum: unit tests for didYouMean (exact match, 1-edit typo, below/above threshold, short-string behavior), and an attach-warning test asserting the pathMappingpathMappings suggestion end-to-end — tests/core/unit/server/server-redefine-and-attach.test.ts (the assertion at line ~441) shows the existing pattern for the #450 warning and must keep passing.

Should fix

  1. Cite the source for each supportedAttachKeys list in a comment (debugpy's attach schema; js-debug's pwa-node attach options). These lists are now load-bearing for warnings — a reviewer needs to know they weren't guessed. If a key's status is unclear, leaving it off is fine since this is warn-only, but say so.

Adapters beyond python/javascript falling back to the old behavior is fine for this PR. This one isn't gating the 0.25.0 release, so there's no rush — but it's a wanted change and I'd like to land it with your name on it.

@debugmcpdev

Copy link
Copy Markdown
Collaborator

Closing in favor of #488, per your own note there (review feedback is on #488). Same lockfile heads-up as #474: once #479 lands, the packageManager pin will keep local pnpm versions from regenerating pnpm-lock.yaml incompatibly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

adapterConfig: unknown keys are accepted silently — a typo of a supported key (pathMapping vs pathMappings) gets no warning

2 participants