feat: warn on unknown keys dropped in MCPDependency.from_dict()#1674
feat: warn on unknown keys dropped in MCPDependency.from_dict()#1674danielmeppiel wants to merge 2 commits into
Conversation
When from_dict() discards unrecognised keys it now emits a single aggregated [!] warning naming every dropped key and the dependency context, instead of silently ignoring them. Known keys (name, transport/type, env, args, version, registry, package, headers, tools, url, command) are unchanged. Acceptance tests added in tests/unit/test_mcp_from_dict_unknown_keys.py. Addresses #1670 (warn-on-dropped-keys; passthrough escape-hatch tracked separately, remains needs-design) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves MCP dependency parsing ergonomics by making MCPDependency.from_dict() emit a single aggregated [!] warning when it drops unrecognized keys, instead of silently discarding them. This helps users diagnose forward-compat / harness-specific fields (e.g., oauth) that APM does not model.
Changes:
- Add a known-key whitelist (
_KNOWN_DICT_KEYS) and emit an aggregated warning for any extra keys infrom_dict(). - Update the
from_dict()docstring to reflect the new warn-on-drop behavior. - Add unit tests covering warning emission, non-warning for known keys, and unchanged parsing/to_dict behavior.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/models/dependency/mcp.py |
Adds _KNOWN_DICT_KEYS and an aggregated _rich_warning when unknown dict keys are present. |
tests/unit/test_mcp_from_dict_unknown_keys.py |
Adds acceptance tests for warning behavior and ensures known-key parsing remains unchanged. |
Copilot's findings
- Files reviewed: 2/2 changed files
- Comments generated: 1
| unknown = sorted(k for k in d if k not in _KNOWN_DICT_KEYS) | ||
| if unknown: | ||
| _rich_warning( | ||
| f"MCP dependency '{d['name']}': unknown key(s) dropped: {', '.join(unknown)}", | ||
| symbol="warning", | ||
| ) |
- Use str(k) in sorted() comprehension so integer/non-string YAML dict
keys do not raise TypeError in the warning path
- Sanitize dep name and key strings via ascii()[1:-1] before interpolation
to guarantee printable-ASCII CLI output on all platforms (cp1252-safe)
- Add two regression-trap tests: test_non_string_key_no_type_error and
test_non_ascii_name_warning_is_ascii_only (15 tests total, all passing)
- Add CHANGELOG.md [Unreleased] Changed entry for the user-observable
warn-on-drop behavior introduced in this PR
Folds: Copilot comment #3362016289 (LEGIT: TypeError + ASCII-unsafe)
panel recommended follow-ups (sorted robustness, CHANGELOG, ascii guard)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 1 | 1 | Well-scoped warn-on-drop change; one defensive-coding gap (non-string dict keys) addressed in fold commit. |
| CLI Logging Expert | 0 | 1 | 2 | Warning message is well-formed and follows conventions; sorted() on non-string keys crash risk addressed. |
| DevX UX Expert | 0 | 0 | 1 | Warning message is clear, contextual, and consistent with existing _rich_warning patterns; no blocking UX issues. |
| Supply Chain Security | 0 | 0 | 0 | Warn-on-dropped-keys is safe: markup=False blocks Rich injection, key names are user-local, no credential surface touched. |
| OSS Growth Hacker | 0 | 0 | 0 | Strong contributor-experience win: silent data loss -> visible warning earns trust and prevents frustration-churn. |
| Doc Writer | 0 | 1 | 1 | No docs corpus drift; CHANGELOG entry added in fold commit. |
| Test Coverage Expert | 0 | 1 | 0 | 13 new unit tests (now 15 after fold) cover the warning behavior well; non-string key gap addressed. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 3 follow-ups (FOLDED in commit 955c972)
- [Python Architect / CLI Logging / Test Coverage] Guard sorted() against non-string keys with str() coercion + ascii() sanitization + 2 companion regression-trap tests -- a TypeError crash in the warning path inverts the promise of graceful degradation. RESOLVED.
- [Doc Writer] Add CHANGELOG.md [Unreleased] Changed entry for user-observable dropped-key warning -- user-facing behavior change without release-note entry. RESOLVED.
- [Python Architect] Sanitize dep_label with ascii() before interpolation into warning string -- prevents terminal escape codes or non-ASCII from reaching warning output. RESOLVED.
Remaining deferred items:
- DiagnosticCollector deduplication for repeated unknown-key warnings across N deps (cli-logging nit; cross-cutting refactor beyond this PR's scope)
- Derive _KNOWN_DICT_KEYS from dataclass fields (python-architect nit; future optimization)
- CommandLogger lifecycle refactor for model-layer direct output (cli-logging nit; existing precedent consistent)
Architecture
classDiagram
direction LR
class MCPDependency {
<<Dataclass>>
+name: str
+transport: str | None
+from_string(s) MCPDependency
+from_dict(d) MCPDependency
+validate(strict)
+to_dict() dict
}
class _KNOWN_DICT_KEYS {
<<ModuleConstant>>
frozenset of 12 keys
}
class _rich_warning {
<<IOBoundary>>
+message: str
+symbol: str
}
MCPDependency ..> _KNOWN_DICT_KEYS : membership test
MCPDependency ..> _rich_warning : emits warning
classDef touched fill:#fff3b0,stroke:#d47600
class MCPDependency:::touched
class _KNOWN_DICT_KEYS:::touched
flowchart TD
A["CLI: apm install / apm run"] --> B["parse_manifest()"]
B --> C["MCPDependency.from_dict(d)"]
C --> D{"'name' in d?"}
D -- No --> E["raise ValueError"]
D -- Yes --> F["Compute unknown = sorted(str(k) for k in d ...)"]
F --> G{"unknown non-empty?"}
G -- Yes --> H["[I/O] _rich_warning(ascii-safe dropped keys)"]
G -- No --> I["Skip warning"]
H --> J["Extract transport via legacy alias"]
I --> J
J --> K["Construct MCPDependency instance"]
K --> L{"registry is False?"}
L -- Yes --> M["validate(strict=True)"]
L -- No --> N["validate(strict=False)"]
M --> O["Return instance"]
N --> O
Reservations carried from strategic-alignment
- SCOPE GUARD: This PR is the warn-on-dropped-keys sub-fix ONLY. Unknown-key passthrough/preservation is OUT OF SCOPE and remains needs-design. The PR must remain 'Addresses MCP model silently drops harness-specific keys (e.g. Claude Code
oauth) — no passthrough for remote-MCP OAuth client config #1670' (not 'Closes MCP model silently drops harness-specific keys (e.g. Claude Codeoauth) — no passthrough for remote-MCP OAuth client config #1670'). All passthrough-related suggestions were deferred. Scope guard respected by all panelists.
Copilot signals reviewed
- Comment #3362016289 (mcp.py:79): LEGIT -- sorted() TypeError on non-string keys; non-ASCII interpolation before validation. Folded in commit 955c972 (str() coercion + ascii() sanitization).
Lint evidence
ruff check src/ tests/ -> All checks passed!
ruff format --check ... -> 1225 files already formatted
pylint R0801 -> Your code has been rated at 10.00/10
lint-auth-signals.sh -> [+] auth-signal lint clean
CI evidence
CI was ALL GREEN on commit a2b8ed4 (pre-fold). Post-fold commit 955c972 is pending CI.
Mergeability
| PR | SHA | CEO Stance | Iter | Folds | Deferrals | Copilot Rounds | CI | Mergeable | Merge State | Notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #1674 | 955c972 | ship_with_followups | 1 | 3 | 3 | 1 | pending | MERGEABLE | pending | awaiting CI on fold commit |
APM Review Panel:
|
| PR | SHA | CEO Stance | Iter | Folds | Deferrals | Copilot Rounds | CI | Mergeable | Merge State | Notes |
|---|---|---|---|---|---|---|---|---|---|---|
| #1674 | 955c972 | ship_now | 1 | 5 | 3 | 1 | green | MERGEABLE | BLOCKED | awaiting required review approval |
TL;DR
MCPDependency.from_dict()previously discarded unrecognised keys silently.This PR adds a single aggregated
[!]warning that names every dropped key andthe dependency context, so users are no longer left debugging silent data loss.
Unknown-key passthrough/preservation is out of scope and tracked separately.
Addresses #1670 (warn-on-dropped-keys; passthrough escape-hatch tracked separately, remains needs-design)
Problem (WHY)
When a user supplies a field that APM's MCP model does not recognise -- for
example Claude Code's
oauthblock --apm installcompletes with no outputand the field is gone from the generated
.mcp.json. The user gets no signalthat their configuration was discarded, leading to authentication failures or
other silent misconfigurations that are very hard to debug.
Issue #1670 identifies two distinct problems: (1) no passthrough for
harness-specific MCP fields (needs-design, out of scope here), and (2) silent
drop with no warning (this PR). Fixing the silent drop first is the minimal,
safe intervention that already improves developer experience without requiring
any schema or API change.
Approach (WHAT)
Extend
from_dict()with a key-diff check: compute the set of input keys thatare not in the known-key whitelist, and if any exist emit one aggregated
_rich_warningcall naming all dropped keys and the dependencyname.flowchart TD A["from_dict(d)"] --> B{"'name' in d?"} B -->|No| C["raise ValueError"] B -->|Yes| D["unknown = keys(d) - KNOWN_DICT_KEYS"] D --> E{"unknown\nnon-empty?"} E -->|Yes| F["_rich_warning:\nunknown key(s) dropped: ..."] E -->|No| G["(no output)"] F --> H["construct MCPDependency\nfrom known keys only"] G --> H H --> I["validate() + return"]Key design decisions:
symbol='warning'-- emits the[!]prefix via the repo'sSTATUS_SYMBOLSconvention; ASCII-safe on all platforms.to_dict()output are unchanged._KNOWN_DICT_KEYS) is a module-levelfrozensetso the diff is O(1) per call and the set is trivially auditable.Implementation (HOW)
src/apm_cli/models/dependency/mcp.py_rich_warningimport; introduce_KNOWN_DICT_KEYSfrozenset; insert unknown-key check + warning at the top offrom_dict(). Update docstring to reflect new behaviour.tests/unit/test_mcp_from_dict_unknown_keys.pyTrade-offs
Validation evidence
Acceptance tests (13 passing)
Lint (all gates green)
How to test
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com