test(api): reject ignored tool parameters - #285
Conversation
Remove the MCP tool whose handler always failed, improve Freerouting PCM/JAR and Java discovery, and update every public catalogue count and migration note. Refs mixelpixx#253
Forward schematic and PCB monochrome settings, schematic theme and page selection, and 3D unspecified-model policy to kicad-cli. Restrict the BOM format contract to KiCad's CSV output. Refs mixelpixx#251
Narrow eight schemas to behavior their handlers can verify, correct overstated tool descriptions, and document a replacement workflow for every public input removal. Refs mixelpixx#251
Resolve project paths to their sibling PCB, query KiCad's real open-document list, and return an honest per-path result instead of ignoring open_project.path. Refs mixelpixx#251
Schematic validators, exports, connectivity queries, and design-review audits transformed every library pin through each matching reference's first placement. Multi-unit parts therefore reported phantom pins, wrong nets, false audit findings, and unsafe connectivity snaps. Resolve only the selected unit at every per-instance call site, search all placed units for reference-level queries, identify units in additive response fields, and remove the temporary partial-review diagnostic. Regression fixtures place both units' pins at the same local coordinate so first-instance resolution cannot pass unnoticed. Refs mixelpixx#182
Preserve each library pin's KiCad electrical type and let the connection validator skip only power_in and power_out pins when requested, with explicit coverage in the result. Refs mixelpixx#251
Apply corner_radius as four circular KiCad arcs plus shortened sides over both IPC and the file fallback, rejecting overlapping or degenerate geometry. Refs mixelpixx#251
Validate supported standard pad shapes, update the pad header atomically, add or remove KiCad shape-specific children, and preserve an omitted size dimension. Refs mixelpixx#251
Validate timeout_seconds and apply one Tokio deadline to process detection plus IPC ping, reporting timeout state and elapsed time explicitly. Refs mixelpixx#251
Make every IPC-backed routing operation resolve the advertised board path and refuse to operate unless that exact board is active in KiCad. This prevents a valid request from silently mutating whichever board happens to be open.\n\nRefs mixelpixx#251
Derive registered handlers and their transitive helper/macro call graph from source, then compare every advertised schema property with the implementation's parameter reads. Same-file resolution prevents duplicate helper names from hiding real drift, and the full catalogue check needs no allowlist.\n\nRefs mixelpixx#251
Correct the tool-directory overview after removing the unavailable autoroute tool, and make the registry-derived documentation test require that exact overview phrase so this count cannot drift again.\n\nRefs mixelpixx#253
|
A maintainer note on this whole series (#264–#285, #294), since this draft is its integration branch. First: thank you. The individual work here is genuinely good — where I deep-reviewed (#266, #278, #294), the root-cause analysis was accurate, the tests assert on real argv/fixtures rather than circularly on the code's own behavior, and #294 anticipates the exact safety hazard a reviewer would raise. Four PRs from this series are merged as of tonight (#266, #271, #274, #276), and more will follow. But the series was submitted as ~22 independent PRs and it is not internally coordinated, which creates real review hazards I need you to fix before the rest can land:
On identity: the branch prefixes say Codex, and the volume says automation. That's fine here — the code quality carries it — but the coordination failures above are exactly what an automated pipeline doesn't catch on its own. Run the cross-check between your own open PRs before filing, and this series becomes easy to take. |
2 participants
User-visible problem and scope
Konnect had no durable check that a public MCP input actually reaches its registered handler. That allowed tools to accept values, ignore them, and sometimes echo them back as if honored.
This PR adds a catalogue-wide regression guard after fixing every true hit found by it. It closes the mechanical prevention requested in #251.
Closes #251
Root cause and design
The test derives tool registrations from source, locates each real registered handler, and follows reachable helper functions and macros. It lexes comments, ordinary/raw/byte strings, and character literals so examples cannot masquerade as implementation. Same-file resolution wins when helper names are duplicated, preventing an unrelated
ipc!macro from hiding an ignored field; cross-file helpers remain discoverable when no local definition exists.Every schema property must appear in the handler's transitive string literals. There is no tool or parameter allowlist. The complete 202-tool catalogue runs in about 0.24 seconds locally.
PR-series dependencies
The guard correctly fails on existing ignored inputs until the focused fixes land. This draft is therefore the tested integration branch for the series and will be rebased to a guard-only diff after its prerequisites merge:
Keeping the implementation fixes in separate PRs follows
CONTRIBUTING.md; keeping this integration PR open proves the final combined state now.Compatibility and migration
The guard itself changes no runtime API. Public removals are documented in
docs/API_MIGRATIONS.md; behavior fixes preserve existing names/defaults. Future contributors must either implement an advertised field or remove it with an explicit migration instead of adding an exemption.Validation
cargo test -p konnect --test schema_parameter_usage --locked -- --nocapture(5 passed; full catalogue included)cargo test --workspace --locked --doccargo clippy --workspace --locked --all-targets -- -D warningscargo fmt --all -- --checkKONNECT_STATE_DIR=/private/tmp/konnect-full-integration cargo test --workspace --locked --lib --tests: all integration/protocol/schema tests and 585/588konnect-coretests passed. The only failures are the same threeupdate_symbols_from_library_*tests reproduced on unmodifiedmain, where the installed KiCadDevice:Rlibrary shadows their fixture.Risk and rollback
The guard is test-only and reads Rust source plus the runtime registry. Its principal risk is a false positive from incomplete call-graph recognition; lexer, transitive-helper, macro, and duplicate-name regression tests cover the failure modes found during development. Rollback is removal of one test file, but keeping the guard prevents recurrence of the issue class.