Fix forty-two verified bugs across all subsystems; add read-only mode and RAG tool hints#24
Draft
secure-ssid wants to merge 10 commits into
Draft
Fix forty-two verified bugs across all subsystems; add read-only mode and RAG tool hints#24secure-ssid wants to merge 10 commits into
secure-ssid wants to merge 10 commits into
Conversation
- config._exc_resp_text: CentralClient.post/post_async raise a bare Exception with no .response attribute (the body is baked into the message), so the duplicate-VLAN and already-exists suppression checks in create_vlan/create_port_profile always fell through to reporting spurious errors. Fall back to str(exc). - config.assign_device_to_site: legacy candidate payloads called int(site_id) eagerly, so a non-numeric site_id (a name or UUID-style scope id) crashed with an unhandled ValueError before even trying the first (New Central) candidate. Build legacy payloads lazily and skip them with a structured error when site_id isn't numeric. - monitoring.detect_ssh_brute_force: min()/max() over a generator filtered on truthy "time" crashed with ValueError when all flagged events lacked timeAt (a real shape from Central's events payload). Materialize the times list first, matching detect_client_flapping. - tool_router.find_tool: the semantic-match loop's break condition recomputed the "unused keyword budget" against len(by_name), which grows as semantic hits are added, so the effective threshold shrank every iteration and the loop stopped near top_k/2 instead of top_k when keyword hits were scarce. Snapshot the keyword count before the loop. - shared.device_type_for_troubleshoot: an explicit device_type="SWITCH" (the literal deviceType value list_devices/inventory records use) mapped to the invalid URL segment "switch" instead of falling through to CX/AOS-S auto-classification, so the natural copied-from-inventory value 404s where omitting it entirely works. - shared.redact_sensitive: the "_key" sensitive-suffix rule matched bound_collection_response's own "_pagination.list_key" metadata field, so every redacted paginated response silently lost the caller's ability to tell which key was sliced. Pass the helper-generated "_pagination" block through unredacted. Each fix ships with regression tests exercising the previously-broken path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
Prior art review (FastMCP/MCP ecosystem, AWS's official MCP servers) surfaced two applicable, low-risk improvements — most other ideas from that pass didn't apply since this repo runs the official mcp Python SDK's FastMCP (1.28.1), not the third-party jlowin/fastmcp package the research initially drew from (confirmed no mount()/timeout=/task=True/ enable_components() on the installed API before acting on anything). - CENTRALMCP_READONLY: a server-wide write/destructive-tool kill switch, independent of the existing CENTRALMCP_PRODUCT_ACCESS gate which only covers optional product starters. When set, non-read-only tools are excluded from _load_all_backends/find_tool indexing and invoke_tool dispatch across every backend, core included — useful for demo/read-only-dashboard deployments that must never reach a write tool regardless of which client connects. - rag.py's three tools (search_docs/lookup_api/ask_docs) query only a local index (LanceDB/SQLite, or Ollama+Redis Stack under the alternate backend) — never the live Central/GLP API — so they get a new READ_ONLY_LOCAL annotation (openWorldHint=False) distinguishing them from every other READ_ONLY tool that does call a live API. Documented the new env var in README.md and docs/tool-router.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
secure-ssid
changed the title
CentralClient.post()/post_async() raised a bare Exception with no .response attribute (unlike get/put/patch/delete, which use raise_for_status()), so every "duplicate"/"already exists" detection in s6_configure.py that reads getattr(exc, "response", None).text always saw "" — idempotent re-runs treated existing roles, port profiles, device profiles, and VLANs as hard failures instead of recognizing and skipping/updating them. Both POST methods now attach the response to the raised exception (keeping the existing message format for logs) so callers see the real body. Also fixed two per-item loop bugs in ConfigureStage._execute that this surfaced: the global layer2-vlan scope-map in _push_vlan_interface had no try/except at all (unlike its device-scope sibling right below it), and both the L2 VLAN and VLAN-interface push loops were wrapped in a single try/except around the whole loop rather than per iteration — so one VLAN's duplicate/failure on a resumed run silently aborted every VLAN after it in the same device's batch, while still reporting StageStatus.SUCCESS with an undercounted vlans_pushed/ vlan_interfaces_pushed. Each item is now handled independently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
secure-ssid
changed the title
….py parity
A background review of mcp_servers/_middleware/ raised a "Critical" claim
that routing through tool_router.invoke_tool disables rate limiting,
response envelope, and MAC normalization entirely, since backend modules'
install_middleware() calls live inside `if __name__ == "__main__":` and
never run when imported by the router. Verified with a live repro this
does not hold: those three middlewares all operate on the return-value
tree (or are a single account-wide bucket by design), so the router's
own middleware chain — installed on its own FastMCP instance and wrapping
the outer invoke_tool/invoke_read_tool call — still applies them
correctly to whatever a backend tool returns. The suggested fix
(installing middleware on every backend at import time) was not applied
since it would double-wrap results for the routed path.
Three narrower, independently-verified gaps from the same review were
real and are fixed here:
- ResponseEnvelopeMiddleware never classified status="FAILED" as blocked.
atroubleshoot_poll (shared.py) explicitly treats FAILED as a legitimate
terminal state and returns it without raising, so a genuinely failed
device operation (reboot, cableTest, poeBounce, etc., and
get_alert_action_status) looked like a success to the middleware,
contradicting its own stated purpose of giving callers a reliable
ok=false signal. Added "failed" to the blocked-status map.
- UnknownToolSuggestMiddleware's on_error substitute bypassed after_call
entirely, so a mistyped router-level tool name got a bespoke
{error, hint, suggestions} shape instead of the same {ok, status, data,
...} envelope every other failure gets. on_error substitutes now route
through after_call before being returned.
- glp.py was the only core backend missing install_middleware(...) in its
__main__ block, so GLP tools got zero null-stripping or rate-limiting
when run standalone (e.g. via .cursor/mcp.dev.json). Added for parity
with the other five backends, plus a static test guarding against the
same drift recurring.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
secure-ssid
changed the title
- lance_client/ingest_docs: upload_lancedb overwrote the LIVE "docs"
table (mode="overwrite") on the FIRST 512-row batch, before the rest
of the corpus (53k+ chunks) was processed. A crash partway through a
rebuild — OOM, disk full, Ctrl-C — left the on-disk index truncated
to whatever fraction landed before the crash, silently served by
ask_docs/search_docs with no warning. Now builds into a "docs__staging"
table, validates it (R2 empty-source check), and only then does a
single atomic create_table(..., mode="overwrite") swap via the new
lance_client.promote_staging_table() — LanceDB OSS has no
rename_table, but Lance's versioned storage format commits this swap
atomically, so a crash before promotion leaves the previous good
index fully intact.
- chunking.py: RecursiveCharacterTextSplitter had no character-level
("") fallback separator, so an unbroken run of text (a long URL,
base64 blob, minified code) with no paragraph/line/sentence/word
break produced one unbounded chunk instead of the target 800-char
size — chunks past EmbedClient._MAX_CHARS (6000) were then silently
truncated for embedding while the full untruncated text stayed in
the index, so retrieval could return content the embedding never
represented. Added "" as the final separator.
- ingest_docs._schema_to_text: crashed on a non-dict property schema
(JSON Schema permits boolean schemas like {"properties": {"x": true}},
which OpenAPI 3.1 fully embeds) with no try/except anywhere in the
call chain, aborting the entire ingestion run and discarding every
already-collected chunk from every other source. Added the same
isinstance(fdef, dict) guard specs_index._walk_fields already uses
for the same shape of data.
- ingest_docs.collect_points: hashed stable_id from the raw, un-resolved
Path yielded by source_dir.rglob(...), so the same logical file got a
different chunk id depending on invocation style/cwd (unlike
collect_openapi_points, which already resolves-then-relativizes).
Only matters for --backend redis's incremental dedup, but a rerun
from a different cwd then fails to match existing ids and duplicates
the whole corpus. Now hashes the same cwd-invariant relative path
collect_openapi_points already computes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
secure-ssid
changed the title
Docs drift (verified against actual code/tests before fixing): - config.py's module docstring was the only core server missing its "(N tools)" count — the count also let it silently skip test_module_docstring_tool_counts_match_registered_tools' regex check entirely (that test `continue`s past any docstring it can't parse a count from), so the count could have drifted with no guard. Added "(64 tools)"; the existing test now covers this module too. - CLAUDE.md's tool-adding checklist still listed only READ_ONLY|DIAGNOSTIC|DESTRUCTIVE|IDEMPOTENT_WRITE, missing the READ_ONLY_LOCAL annotation added earlier this session. - CLAUDE.md's verb table had no update_* row despite 6 real tools using it (more than push_* or build_*, both of which do have rows). Test coverage: nac.py had zero test coverage across all 18 of its DESTRUCTIVE/IDEMPOTENT_WRITE tools; config.py had coverage on only 5 of 40. Added focused coverage for the highest-risk, irreversible subset: all 8 nac.py delete_* tools (parametrized — dry_run short- circuits, success path, structured-error path), config.py's delete_role_acl/delete_gw_policy/delete_config_assignment/delete_role/ delete_webhook/delete_device_groups, trigger_device_upgrade, plus the two remaining gaps in otherwise well-covered modules (glp.glp_add_devices_bulk, ops.acknowledge_alert). Writing trigger_device_upgrade's dry_run test surfaced a real instance of the validate-first bug class fixed repeatedly elsewhere this session: it built a CentralClient via get_client() before checking whether device_function/scope_id could even be resolved. Moved the client construction to immediately before its first use (after the dry_run/validation short-circuits), matching every other fix of this pattern this session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
secure-ssid
changed the title
- edgeconnect.edgeconnect_list_tunnels was the only list tool across all six optional product backends that sent limit upstream to the live Orchestrator API without a matching offset param, then re-sliced that already-limited response locally with bound_collection_response using the same limit/offset. Since the upstream fetch was always capped to the first `limit` items regardless of the caller's offset, any offset >= limit produced an empty page with truncated=False — silently hiding everything past the first page. limit is no longer forwarded upstream; the tool now fetches the full filtered set and lets bound_collection_response do the slicing entirely client-side, matching its sibling edgeconnect_list_appliances. Updated the existing test that had asserted the old (buggy) upstream limit forwarding as expected behavior, and added a two-page regression test proving offset=50 now returns real data instead of an empty page. - uxi.py was the only one of the six optional backends missing install_middleware(...) in its __main__ block — UXI tools got zero null-stripping or rate-limiting when run standalone. Added for parity, and extended the existing core-backend middleware parity test (added for glp.py earlier this session) to also cover all six optional backends so this class of drift is caught going forward. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
secure-ssid
changed the title
…d or exposed An 8-angle adversarial review of this branch's full diff (line-by-line, removed-behavior, cross-file tracing, reuse, simplification, efficiency, altitude, conventions) surfaced and verified these issues — several in code this PR itself added: - tool_router: find_tool's SEMANTIC branch never applied the new CENTRALMCP_READONLY filter — catalog hits for core write tools leaked into results and, being absent from the filtered index, were mislabeled read_only=False/destructive=False (reproduced live). Excluded names are now tracked at load in _readonly_excluded, filtered from semantic hits, and dispatch returns the informative global_write_blocked message instead of the misleading "Unknown tool". - lance_client: hybrid_search raised RuntimeError on a table with no FTS index (reproduced live) — so a rebuild crash between the staging swap and build_fts_index, or any query during the post-swap index build, broke every ask_docs/search_docs call. It now degrades to vector-only search with a warning. promote_staging_table also no longer materializes the whole corpus via to_arrow() (a ~250MB+ peak-memory regression vs the old streaming writes) — it streams 4k-row windows through a RecordBatchReader into the same single atomic overwrite. - central_client/config/s6: consolidated error-body extraction into one error_body(exc) helper. _exc_resp_text's str(exc) fallback (added earlier in this PR) could mistake a non-HTTP failure whose message merely contained "duplicate"/"already exists" for an idempotency hit and silently drop the error — with _post_error attaching .response to every CentralClient HTTP error, the fallback was also dead code for its intended path. Removed; s6's four inline getattr idioms now share _skip_exc()/error_body(), and its step-4 scope-maps loop one handler. - edgeconnect_list_tunnels: this PR's earlier fix dropped the upstream limit entirely, fetching the FULL tunnel inventory (thousands of records) for a limit=5 call. Now sends limit=offset+limit+1 upstream — bounded transfer, correct page-2 data, and the +1 sentinel keeps the truncated flag honest. - redact_sensitive: the _pagination carve-out passed the WHOLE subtree through unredacted and unrecursed (reproduced: a nested api_key under a payload key named _pagination leaked). Now only the list_key field is exempt; the rest of the subtree is scrubbed. - detect_ssh_brute_force: IP-less events (IPv6/hostname descriptions) were lumped into one "unknown" pseudo-attacker, inflating false positives — now reported as unattributed_failures. min_failures=0 no longer flags every source (clamped to >= 1). - ops poe_bounce/port_bounce/cable_test: an unresolvable device type reported "not supported on Access Points" — now says the type could not be determined. - ingest_docs.collect_points: a symlink resolving outside the sources tree crashed the whole run via relative_to ValueError — now skips the one file. Added a migration note that redis-backend chunk ids changed. - s6 VLAN loops now report vlan_scope_map_failures / vlan_interface_failures in StageResult so a partial push is visible instead of a bare SUCCESS with an undercount. - Cleanup from the same review: shared env_flag() replaces three divergent truthy-env parsers; _write_blocked_response() deduplicates the two blocked-response shapers; assign_device_to_site builds its legacy payload once; docs clarify CENTRALMCP_READONLY also blocks DIAGNOSTIC tools. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
secure-ssid
changed the title
A review pass over the final un-reviewed corners of the codebase
(pipeline/clients/{token_manager,mcp_client,glp_client,specs_index,
redis_client}, scripts/) — every finding independently verified against
source before fixing:
- mcp_client.get_site_by_name searched only the FIRST 50 sites (it
reused the paged get_sites() default against an API that returns the
full list): site #51+ was invisible, so s2_validate re-created
existing sites, s6_configure could not find a site it just created
("site not found after creation" -> CONFIGURE_FAILED), and
get_site_health_summary reported real sites missing. It now searches
the full unsliced list via a new _all_sites() helper.
- get_device_by_serial/find_client gave up silently after 1,000 items,
returning None indistinguishable from "not found" — defeating the
duplicate-migration guard and failing onboarding polls on larger
fleets. Page cap raised to 50 pages (early-exit makes it free for
small fleets) and cap exhaustion now logs an explicit
"inconclusive, not not-found" warning.
- specs_index.build() unlinked the live SQLite index BEFORE building in
place — an interrupted build left a valid-but-empty DB that
lookup_api silently served [] from (doctor's existence check still
passed). Now builds into a .tmp sibling and os.replace()s only after
the full build commits.
- download_indexes.py extracted straight into live data/, interleaving
old and new files: an interrupted run corrupted the Lance tables, and
stale local files absent from the archive (higher-numbered Lance
version manifests from local ingests) survived — so a "successful"
download could keep serving the OLD index. Now extracts to a staging
dir and swaps whole artifacts into place via renames, removing the
old copy only after the new one is live.
- token_manager wrote the shared-across-processes cache file with an
in-place truncate-and-write — concurrent readers could see torn JSON
and concurrent writers could leave corrupt bytes as the final state
(silently disabling the cache and re-hitting the rate-limited HPE SSO
endpoint per process). Now writes a per-PID .tmp and os.replace()s.
- doctor.py crashed with UnicodeDecodeError/PermissionError instead of
reporting FAIL when .mcp.json, credentials.yaml, or .env was
unreadable/non-UTF8 (three unguarded read_text() calls next to a
_load_json that guards exactly this).
- setup_wizard wrote credentials.yaml and .env (client secrets, product
API tokens) world-readable under the default umask — now 0600 via a
_write_secret_file helper, matching the token cache.
- GLPClient.get_device swallowed ALL exceptions as None ("not found"),
so a transient 500/auth failure made stages tell the operator the
device was missing from GLP. None now means only a confirmed empty
result; transport errors re-raise (Stage.run converts them to FAILED
with the real reason).
- _resolve_subscription_id interpolated the caller-supplied key into an
OData filter unvalidated, while the same file's resolve_device_id
documents and enforces exactly this defense for serials. Same guard
applied.
- redis_client.vector_search interpolated source_filter into the
RediSearch query string unvalidated (reachable from search_docs'
source arg under the redis backend) — now validated with the same
pattern lance_client already uses.
12 new regression tests across 7 test files; 791 unit tests pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
secure-ssid
changed the title
A focused review of the previous commit's ~420-line diff found five
issues in its own fixes, the first one serious:
- download_indexes._swap_into_place used os.rename for the staging->live
step, which raises EXDEV when data/ is a symlink/mount to another
filesystem (plausible for multi-GB indexes). Reproduced: the exception
cascaded into the staging-cleanup finally deleting the NEW copy, and a
retry then deleted the .old-tmp backup first — leaving data/ empty.
The pre-fix extractor handled cross-fs fine, so this was a regression
introduced by the atomic-swap fix. Now uses shutil.move (copies on
EXDEV) and rolls the live artifact back from .old-tmp before
propagating any failure.
- doctor._load_local_env still crashed on UTF-16 .env files (embedded
NULs survive errors="replace" decoding and blow up os.environ
assignment with "embedded null byte") — exactly the failure class the
previous commit claimed fixed. NULs are now stripped after decode.
- The new redis source-filter ValueError escaped search_docs/ask_docs
uncaught under CENTRALMCP_RAG_BACKEND=redis, while the lancedb path
returns an [{"error": ...}] envelope for the same input — and
RediSearch tag matching is case-insensitive, so an uppercase source
that previously worked became an unhandled exception. _search_redis
now returns the same envelope as its lancedb sibling.
- setup_wizard._write_secret_file chmod'd AFTER writing — a pre-existing
0644 file held the new secrets world-readable for the write window.
Now fchmod's the descriptor before any secret bytes land.
- token_manager left a token-bearing .tmp orphan behind when the cache
write failed mid-way — now removed in a finally.
3 new regression tests; 794 unit tests pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53
secure-ssid
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A full-codebase hardening pass: every subsystem reviewed, every finding independently verified (live reproduction where possible) before fixing, and every fix shipped with a regression test confirmed to fail without its source change. Two rounds of adversarial self-audit over the PR's own diffs caught and fixed regressions the PR's earlier fixes introduced.
Coverage: all six core MCP backends, all six optional product backends, the tool router, the middleware stack, the migration pipeline (stages + all API clients), the RAG ingestion pipeline,
scripts/, and the PR's own diff (twice).1. Core-server correctness (
config.py,monitoring.py,shared.py,tool_router.py) — dead idempotency detection, crash on non-numeric site IDs, crash on missing timestamps,find_toolunder-filling, invalid device-type URL segment, redaction clobbering pagination metadata.2. Migration-pipeline error handling (
central_client.py,s6_configure.py) — POST errors now carry.responseso idempotency checks work; per-item loop resilience in VLAN pushes; partial-failure counts in stage results.3. Middleware gaps —
status="FAILED"never flagged as blocked; unknown-tool errors bypassing the envelope;glp.py/uxi.pymissing middleware installs (parity test guards all 12 backends now).4. RAG index lifecycle (
ingestion/,lance_client.py,specs_index.py,download_indexes.py) — the docs LanceDB index, the specs SQLite index, AND the prebuilt-index download all destructively modified live artifacts before their replacement was complete. All three now build/extract into staging and swap atomically — including a cross-filesystem-safe swap with rollback (the first version of the swap was itself destructive on EXDEV, caught by self-audit).hybrid_searchdegrades to vector-only instead of erroring when the FTS index is missing.5. Site/device lookup correctness at scale (
mcp_client.py) —get_site_by_nameonly searched the first 50 sites; client-side searches silently gave up at 1,000 items indistinguishably from "not found".6. Token & secret hygiene (
token_manager.py,setup_wizard.py) — atomic cross-process token-cache writes with tmp-orphan cleanup; wizard-writtencredentials.yaml/.envnow 0600 with the descriptor tightened before secret bytes land.7. Error masking & injection (
glp_client.py,redis_client.py,doctor.py,rag.py) — GLP transport failures no longer masquerade as "device not found"; OData/RediSearch filter-injection gaps closed; doctor survives unreadable/non-UTF8/UTF-16 config files; the redis backend returns the same error envelope as the lancedb backend for invalid source filters.8.
CENTRALMCP_READONLYkill switch — hides every non-read-only tool (including DIAGNOSTIC) from discovery and dispatch across all backends; self-audit closed a semantic-search leak in its first implementation.9. RAG tool safety hints — new
READ_ONLY_LOCALannotation (openWorldHint=False) for the three local-index-only tools.Checklist
find_tool,invoke_read_tool,invoke_tool) where possible.uv run python scripts/validate_release.py --skip-rag— 794 unit tests pass (1 skipped), RAG eval auto-skipped (local indexes absent), tool catalog floor satisfied (346 ≥ 204), exit 0.Security
For exploitable details or accidental credential exposure, do not use this pull request. Follow SECURITY.md.
Contributor notes
See CONTRIBUTING.md for local setup, validation, and no-secret contribution guidance.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AoFhWvujADVzy5GJq4aQ53