Add /discover skill: admin-run tenant inventory discovery crawler - #238
Conversation
…vice-Agent-Developer-Kit into feature/discover-skill
…Employee-Self-Service-Agent-Developer-Kit into feature/discover-skill
…Employee-Self-Service-Agent-Developer-Kit into feature/discover-skill
amilandi
left a comment
amilandi
left a comment
There was a problem hiding this comment.
Telemetry gap: /discover doesn't emit ADK capability telemetry
Reviewed the PR end-to-end for telemetry coverage. The new skill will not appear on the Capability Usage donut (or any ADK cube). Every other skill in solutions/ess-maker-skills/src/skills/ emits capability.use; this one does not.
Findings
-
No
emit_capability.pyinvocation inSKILL.md. Compare totroubleshoot/SKILL.md, which has (as Step 1):python scripts/emit_capability.py troubleshootdiscover/SKILL.mdhas no equivalent line, so noadk.capability.useevent is emitted when the skill runs. -
"discover"is not in the ADK taxonomy.scripts/adk_telemetry.pyADK_CAPABILITIES(15 values:setup,connect,topic_create, ...,publishing,flightcheck) doesn't includediscover. If the shim call were added as-is,normalize_capabilitywould map it to"unknown"— exactly the bug we just fixed forpublish.pyin #254. -
scripts/discover_inventory.pyhas noadk_telemetryimport. Other kit scripts either shell out toemit_capability.pyor calladk_telemetry.emit_*directly. This bridge does neither. -
tools/tenant-inventory-discovery/telemetry.pyis not ADK telemetry.LoggingTelemetrySinkjust callslogger.infofor run summaries; nothing flows into OneDS/Aria. That's fine for local diagnostics, but doesn't substitute for kit telemetry.
Requested changes
- Add
"discover"toADK_CAPABILITIESinsolutions/ess-maker-skills/scripts/adk_telemetry.py. Update any tests that enumerate the taxonomy. - Add a best-effort Step 1 in
solutions/ess-maker-skills/src/skills/discover/SKILL.mdmirroringtroubleshoot:python scripts/emit_capability.py discover - Optional (stronger): in
discover_inventory.py, calladk_telemetry.emit_capability_use("discover")on successful crawl completion. If the PM wants success/failure counts per crawl, consider a scoped outcome event similar toemit_build_complete. - Verify
tests/test_adk_telemetry.py::test_no_caller_passes_a_noncanonical_capability_to_the_shimstill passes after these changes (it enforces that everyemit_capability.py <cap>call site uses a canonical taxonomy value).
Without these, running /discover will be invisible in the FlightCheck / Capability Usage dashboards.
amilandi
left a comment
amilandi
left a comment
There was a problem hiding this comment.
Full code review (round 2)
Broader review beyond the earlier telemetry note. Read-only; behavior not executed. Findings grouped by severity, cited with file:line.
Code Review — PR #238 /discover skill
Blocker
Issue: HttpInventoryClient sends the literal string ****** as its Authorization header instead of the bearer token
File: tools/tenant-inventory-discovery/src/tenant_inventory_discovery/inventory_client.py:224-226
Severity: Blocker
Problem: The _headers method only checks that auth_token_provider is truthy, then sets:
if self._auth_token_provider is not None:
headers["Authorization"] = f"******"self._auth_token_provider is never called anywhere in the file (grep-verified — the only uses are the ctor assignment on line 197 and the truthiness check on line 225). Every request the --direct write path makes to WeveNova therefore goes out with Authorization: ******, which will 401 unconditionally. The docstring on the ctor parameter (# callable -> the admin's delegated bearer token), the module docstring's claim that the skill "runs as the admin end-to-end: a delegated bearer token", the README ("Supply a token…"), and the _acquire_inventory_token plumbing in the bridge all assume the header is really being sent — this looks like a placeholder that was pasted in during a redaction and never restored.
Also note: even once the value is fixed, the surrounding code has no Bearer prefix anywhere in the module, so the fix needs to be f"Bearer {self._auth_token_provider()}", not just self._auth_token_provider().
Evidence:
grep -R auth_token_provider tools/tenant-inventory-discovery/srcreturns only the ctor, the assignment, and the truthiness check — never a call site.grep -R Bearer tools/tenant-inventory-discovery/srcreturns zero hits.tests/test_http_client.pyuseshttpx.MockTransport(handler)withauth_token_provider=lambda: "test-token"but no test asserts onrequest.headers["Authorization"], so the bug is invisible to the ~4,000 lines of tests.
Suggested fix: Call the provider and prefix with Bearer:
if self._auth_token_provider is not None:
headers["Authorization"] = f"Bearer {self._auth_token_provider()}"Add a test that asserts seen[0].headers["authorization"] == "Bearer test-token" in TestUpsert, TestList, TestReconcile, and probe.
High
Issue: FileRunLock.acquire is a check-then-write TOCTOU; two concurrent runs on the same host can both take the lock
File: tools/tenant-inventory-discovery/src/tenant_inventory_discovery/lock.py:52-70
Severity: High
Problem: The acquire path is:
if path.exists(): read + parse; if owner != self and not expired -> raisepath.write_text({token: self, expires_at: now+ttl})
Two processes started within milliseconds of each other on the same host both see the same "no lock / stale lock" state in step 1 and then both do step 2 — the second write clobbers the first. Both then proceed with a "single-flight" run, silently defeating the whole reason the lock exists (D6 mitigation, spec §7).
The module docstring and the class docstring both call this the "single-host single-flight" interim mitigation. That claim only holds if acquisition is atomic on a single host, which this code is not — the standard fix (os.open(path, O_CREAT|O_EXCL|O_WRONLY) for a claim-file, or msvcrt.locking / fcntl.flock on a sidecar handle) would make the interim story actually true. As written, the interim mitigation doesn't do the one thing it advertises.
Evidence: No O_EXCL, no flock, no msvcrt.locking anywhere in lock.py. The unit test test_backoff_and_lock.py only checks the sequential case (acquire → acquire raises → release → acquire OK); no concurrent-acquisition test.
Suggested fix: Use os.open(path, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600) for the acquire, catch FileExistsError, then read/inspect for staleness. Reclaim a stale lock by writing to a .tmp sibling and os.replace-ing, so two contenders don't both "reclaim" the same stale file. Alternatively, keep the JSON semantics but move to a POSIX advisory lock / msvcrt.locking on a companion file that is opened for the duration of the run.
Issue: FileRunLock._path sanitization collapses distinct tenants into the same lock file
File: tools/tenant-inventory-discovery/src/tenant_inventory_discovery/lock.py:44-46
Severity: High
Problem:
safe = "".join(c if c.isalnum() or c in "-_" else "_" for c in tenant_id)Any two tenant ids that differ only in punctuation map to the same lock file: contoso.onmicrosoft.com and contoso_onmicrosoft_com both become discovery-contoso_onmicrosoft_com.lock, and a service-principal-shaped tenant id like contoso:hr and contoso/hr collide too. On a shared admin box (or CI runner) that legitimately serves several tenant ids, an unrelated run for tenant B can block tenant A, or worse — combined with the TOCTOU above — cause A's run to reclaim B's "stale" lock and vice versa.
Suggested fix: Hash the raw tenant id (hashlib.sha256(tenant_id.encode()).hexdigest()[:16]) for the filename and store the raw id inside the JSON so debugging is still possible. Injectivity of the filename is the safety property, not human-readability.
Medium
Issue: Bridge subprocess (MCP server) is leaked on any exception between the crawl and the close() call
File: solutions/ess-maker-skills/scripts/discover_inventory.py:843-848 (and the surrounding structure of main)
Severity: Medium
Problem: The main() layout is:
try:
... build platform, build inventory client (inner), skill.discover(), assemble result ...
finally:
progress.stop()
result[...] = ... # <-- not in a finally
out_path = ...
_atomic_write_json(out_path, ...)
if not aborted: _persist_local_inventory(...)
close = getattr(inner, "close", None)
if callable(close): close()Everything after progress.stop() runs outside any finally. If _atomic_write_json fails (disk full, path perms), or _persist_local_inventory raises (malformed prior mirror + a bug in build_document), or the os.makedirs call at 833 raises, inner.close() never runs. For --via-mcp that means the wevenova MCP subprocess (with its stdio pipes and any minted token cached in-process) is orphaned; for --direct it means the pooled httpx.Client isn't closed. Also, inner is unbound in the enclosing scope if _build_inventory_client itself raises before returning, which the getattr(inner, "close", None) line would then hit as NameError, masking the real exception.
Suggested fix: Wrap the client build + crawl + persist in a single try/finally that closes inner and stops progress; or use a with contextlib.closing(inner): block.
Issue: discover_inventory.py close = getattr(inner, "close", None) uses inner before checking whether it was assigned
File: solutions/ess-maker-skills/scripts/discover_inventory.py:844
Severity: Medium
Problem: In the outer try: block, inner is only assigned by _build_inventory_client(...). If any of the code above that call raises (_live_platform on a missing config, _demo_platform_and_inventory on some future extension, the progress.phase calls, an ImportError inside _build_inventory_client before it returns), inner is never bound. The subsequent getattr(inner, "close", None) then raises UnboundLocalError, replacing the real diagnostic (which had already been captured into result["fatalError"]) with a bogus one and skipping the JSON summary print at the bottom.
Suggested fix: Initialize inner = None before the try and guard if inner is not None: at the cleanup site, or fold the whole thing into the try/finally fix from the previous finding.
Issue: DiscoveryRunner._kind_counts does not include existing server rows, so the "client-side row-cap guard" docstring overstates its safety
File: tools/tenant-inventory-discovery/src/tenant_inventory_discovery/runner.py:157-172
Severity: Medium
Problem: self._kind_counts is reset to {} at the start of every run and only counts rows successfully upserted in this pass. The server's cap is per (tenant, kind) on the total Active row set, not per run. So a tenant with 40 pre-existing Connection rows that discovers 20 new ones will:
- pass the client-side check (
already=0,remaining=50-0=50,resources=20 <= 50→ not truncated,capped=False) - send all 20 upserts
- get some (or all) rejected by the server past row #10 as
NonRetryableApiError - each such failure sets
report.error, which correctly makes the scope incomplete — so no reconcile fires
That behavior is safe in the sense that nothing is wrongly retired, but the runner docstring says the cap check "stops a scope at this many items and reports it Incomplete rather than failing mid-crawl" — which is the property that isn't actually delivered. The scope still fails mid-crawl, still bloats the logs with per-item 4xx, and burns retry budget for scopes that share the pool.
Suggested fix: Either (a) drop the client-side guard entirely and rely on the server's cap enforcement + the existing report.error → incomplete-scope path, and update the docstring to match; or (b) seed self._kind_counts from inventory.list_items(kind=...) before the crawl so remaining reflects reality. Option (b) is cheap on the fake and the real server (one paged GET per kind, and list_items is already paginated) and delivers the property that's currently claimed.
Issue: retire DELETE swallows a 404 by consulting the ETag-cache index, which may be stale
File: tools/tenant-inventory-discovery/src/tenant_inventory_discovery/inventory_client.py:396-410
Severity: Medium
Problem: On DELETE → 404, the code calls self._index() and treats the row as successfully retired iff item_id not in current_ids. _index() is memoized for the lifetime of the client — it is populated on the first list_items call and never invalidated. So during the tenant-root drift sweep in _sweep_tenant_root, the sequence is:
list_items(kind=Environment)— cache populated,_id_indexseeded with all live environment item ids.- Sweep issues a
retire(item_id)for one drifted row → service returns 200, all good, but the id remains in_id_indexbecause nothing removes it. - Something concurrently deletes another environment row out-of-band. The next
retire(item_id2)returns 404 →_index()returns the cached index, which still containsitem_id2(since the initial list still had it), so the code raisesInventoryApiError("...still listed; the service could not resolve this key").
The "still listed" branch is meant to distinguish a legitimate "row already retired" (idempotent success) from a "key routing failed to decode this id" bug, but the freshness of _id_index isn't guaranteed for either signal.
Additionally, _index() is only ever populated inside upsert's "demands If-Match" recovery path or here. In a sweep-only path (no upserts, just DELETEs), the first DELETE-404 will call _index() fresh — that call is fine — but successive DELETEs against the same client will keep hitting the stale cache.
Suggested fix: Do a targeted freshness check on 404: re-list the kind, or GET the collection with $filter=agentConfigurationInventoryItemId eq '...' if the service supports it, before deciding. At minimum, invalidate the cached _id_index after a successful retire.
Issue: _upsert_all's except Exception catches KeyboardInterrupt? No — but it also catches assertion errors and SystemExit (Exception doesn't catch those). However PreconditionFailedError handling counts a 412 as applied=True, which corrupts observed_keys
File: tools/tenant-inventory-discovery/src/tenant_inventory_discovery/runner.py:236-245
Severity: Medium
Problem: A 412 (PreconditionFailedError) is treated as ok=True:
except PreconditionFailedError as exc:
logger.info("precondition failed (concurrent writer won): %s", exc)
return item.natural_key, TrueThe natural key is then appended to report.observed_keys. But a 412 means the row was not written this pass — a concurrent writer already updated it, so its UpdatedAt may still be behind the pass watermark by the time server-side reconcile runs (the run_id-scoped idempotency key was not accepted; nothing here re-reads the row and re-applies). Two hazards:
- Reconcile can wrongly retire the row. For env-scoped kinds the server sweeps by watermark; a row this pass "observed" via 412 but did not actually re-stamp can have
UpdatedAt < passStartedAtand be swept. - Tenant-root sweep counts the row as "observed" and skips it from drift retirement even though this pass never confirmed it exists (arguably the milder case).
The comment cites §5.2, but the safe behavior is what the PreconditionFailedError docstring in errors.py already states: "Not retryable in place: the caller must re-read the row and re-apply." The current runner does neither.
Suggested fix: On 412, re-read the row (via list_items + cache or a targeted GET), re-issue the upsert with the new ETag, and only then count it as observed. If that second write also 412s, fall back to counting it as failed → scope incomplete → no reconcile. This matches the guarantee the reconcile watermark depends on.
Issue: discover_inventory.py's _persist_local_inventory config-pointer update swallows every exception, including bugs
File: solutions/ess-maker-skills/scripts/discover_inventory.py:608-620 (_persist_local_inventory)
Severity: Medium
Problem: The best-effort update of .local/config.json's inventoryPath field is wrapped in try: ... except (OSError, ValueError):. That's fine for the open/json.load. But the block also encompasses the write-back through _atomic_write_json(config_path, cfg) — and that call can fail with a PermissionError (a subclass of OSError, so caught) or a TypeError (not caught) or a RuntimeError from a mocked FS. More importantly, silent swallowing means a corrupted .local/config.json (json.load returns a non-dict → the code short-circuits and no pointer is written, but no diagnostic is emitted either) leaves subsequent runs looking at the same stale inventoryPath.
Suggested fix: Log at WARNING level when the pointer update is skipped, and split the try so that the write-back is either successful or logs a distinct message. The whole block is described as "best-effort" but a silent no-op is only fine if someone can see it.
Low
Issue: _headers never sets Content-Type on POST bodies
File: tools/tenant-inventory-discovery/src/tenant_inventory_discovery/inventory_client.py:220-231
Severity: Low
Problem: All requests are Accept: application/json only. httpx sets Content-Type: application/json automatically when json= is passed, so this works today, but it's a fragile dependency on client behavior. Consider being explicit; also verify OData-Version / OData-MaxVersion headers aren't required by the WeveNova OData endpoint (many OData 4 services 400 without them). If they are, tests won't catch it because MockTransport doesn't enforce.
Issue: discover_dataverse_platform._resolve_site_id catches Exception and returns None, silently dropping SharePoint sites we can't resolve
File: solutions/ess-maker-skills/scripts/discover_dataverse_platform.py:342-356
Severity: Low
Problem: Any Graph error (a genuine 500, a transient network drop, a token refresh failure) is indistinguishable from "site legitimately not resolvable." The docstring says "any failure returns None so the site is skipped rather than aborting the whole scope," but a scope that quietly drops half its sites reports itself complete=True and becomes reconcile-eligible, at which point the server would retire the previously-known sites that this pass couldn't resolve. That is the exact failure mode the tenant-root exemption is supposed to prevent — and it does prevent it in the wired subset-crawl (SharePointSite is tenant-root, so it never enters reconcile), so this is a latent bug that will bite as soon as the tenant-root sweep is turned on for full crawls.
Suggested fix: Distinguish 404 (site truly gone) from 5xx / auth failure. On the latter, either raise PlatformError (scope incomplete → no reconcile) or record the site with an "unresolved" marker so the drift sweep skips it explicitly.
Issue: _find_sharepoint_urls is a URL sniffer over the internal KnowledgeSourceComponent blob and can match SharePoint URLs found in arbitrary string fields (descriptions, error messages, prior audit trails)
File: solutions/ess-maker-skills/scripts/discover_dataverse_platform.py:174-198
Severity: Low
Problem: The walker matches any string containing .sharepoint.com. That includes fields like errorMessage, lastAuthor, or a component's description, so a component that once referenced a site and now references a different one will "discover" both. Author already flagged the component shape as [verify], but this specific pattern makes the discovered SharePoint set a superset of the actual references — inventory rows for sites the agent no longer uses. Since SharePointSite is tenant-root and not reconciled in the wired path, this doesn't immediately misretire, but it does bloat the inventory with rows that a full-crawl sweep would then correctly delete on the next pass.
Suggested fix: Narrow the scan to known key names (sourceUrl, siteUrl, url) before falling back to the broad sniff, or gate the sniff behind a specific parent key path.
Issue: _kind_counts cap enforcement rounds down before the batch is submitted, silently truncating rows even for the first scope of a fresh tenant if page_size >= 50
File: tools/tenant-inventory-discovery/src/tenant_inventory_discovery/runner.py:159-171, in combination with config.py:81 (default page_size=200) and schemas.py:63 (default max_items_per_tenant_and_kind=50)
Severity: Low
Problem: With defaults, any scope that legitimately has >50 items on a fresh tenant is silently truncated to 50 and marked capped=True. Enterprise tenants easily exceed 50 Connections in one env; today the reconcile is disqualified (safe) but half the truth is never even written. This is documented behavior, but the cap looks copied from a server invariant intended for full-tenant totals across kinds, not per-env-per-kind — worth confirming the 50 is really the intended value for Connection/KnowledgeSource/ScenarioTemplate in this deployment before shipping.
Suggested fix: Verify against AgentConfigurationInventoryConstants.Limits.MaxItemsPerTenantAndKind in the WeveNova source; if the real server value is higher, bump AttributeCaps.max_items_per_tenant_and_kind accordingly. If 50 really is the ceiling, mention this constraint in SKILL.md so admins aren't surprised by truncation.
Issue: mcp_inventory._StdioJsonRpc.__init__ starts threads and spawns a subprocess before _handshake is called; if handshake times out or the process exits during startup, the caller never gets a close() and the subprocess/threads are leaked
File: tools/tenant-inventory-discovery/src/tenant_inventory_discovery/mcp_inventory.py:105-137
Severity: Low
Problem: __init__ does Popen → start stderr_thread → start reader_thread → self._handshake(). If _handshake raises (transport error, timeout), the ctor propagates and the caller has no handle on the partially-initialized object to close(). The subprocess (with its file descriptors) and the two daemon threads are orphaned. Daemon threads die at interpreter exit, but the child process keeps its stdio pipes and stays alive until it decides to exit.
Suggested fix: Wrap the handshake in try: ... except: self.close(); raise.
Issue: discover_inventory.py prints error_description-shaped messages onto stderr via _tls_hint; not sensitive, but the surrounding _build_inventory_client degrade path formats exc verbatim into writePathNote, which is written to the results JSON on disk
File: solutions/ess-maker-skills/scripts/discover_inventory.py:377-397, 819-828
Severity: Low
Problem: f"could not acquire a token ({exc})" and f"{base_url} rejected the pre-flight request using the token from {source} ({exc})" end up in result["writePathNote"] and are then json.dumped to workspace/discover/results.json. _acquire_inventory_token deliberately strips error_description from the auth failure, but a downstream httpx exception (ConnectError, ReadTimeout) can include the full URL with query parameters; if that URL ever carries an access token (it doesn't today for WeveNova, but the direct-path future is uncertain), it lands on disk. The CWE-209 comment at line 191 suggests the author is thinking about this concern; the pattern should be applied consistently to the degrade-path notes.
Suggested fix: Normalize exception rendering through a _redact(exc) helper that strips known secret-shaped substrings before writing to disk.
Not flagged / verified OK
local_store.build_document— pure module,_atomic_write_json(tmp+os.replace) is correct on Windows and POSIX; schema-version field is present for future migration.mapping.idempotency_key— includingrun_idas claimed does prevent the "cached-response starves the watermark" hazard.schemas.validate_attributes— order of checks matches the server-side validator; unknown-key rejection is done both bydrop_unlisted(silent) andvalidate_attributes(raise), which is defense-in-depth, not a bug.odata_key_literal— double-encoding rationale checks out; the OData quote-doubling then percent-encoding is correct.- Runner completeness gate —
_completed_scopescorrectly excludes tenant-root scopes from a subset crawl, matching spec §6.3. - No obvious PII leak in the run summary or telemetry sink (
LoggingTelemetrySinkattelemetry.py, per the pre-existing telemetry review). - No new dependencies of concern —
httpx>=0.28.1is already in the ecosystem, tests use only stdlib + pytest.
WeveNova replaced the per-item upsert, the single-item DELETE and the bulk reconcile endpoint with one POST .../syncInventory that takes the tenant's entire inventory. Absence is now the delete verb: anything Active that the payload omits is retired. There is no server-side guardrail, so the failure direction inverted -- a partial crawl used to retire too little and would now retire too much. Every safety property has to live in the client. Contract migration: - Replace upsert/retire/reconcile with a single sync_inventory call, and drop all ETag/If-Match preconditions and the 412 retry path with them. - Add carry-forward: read the current inventory first and re-send verbatim every Active row belonging to a scope this run cannot vouch for, so a scope that failed, was truncated, or was never visited loses nothing. - Let a scope retire by omission only when it is authoritative -- fully enumerated, no fatal error, nothing unmappable, not capped, and read tenant-wide for that kind. - Withhold the sync entirely when the current inventory cannot be read, or when it holds a kind this build cannot round-trip faithfully. Never submit an empty payload. - Enforce the client-side limits (400 items, 50 per kind, no duplicate kind:naturalKey) before sending rather than trading a round trip for a rejection. Fix the long-POST timeouts that were leaving the run stranded: - Tier the HTTP budgets (connect 10s / read 30s / sync 600s). One flat 30s covered both a paged GET and a sync that legitimately runs for minutes, so the sync always timed out and was re-sent five times onto a service still applying the first attempt. - Derive the MCP RPC budget from the sync budget so the two can never be equal; whichever expires first owns the error, and the inner one is the informative one. - Give the sync its own 2-attempt retry policy instead of inheriting the 5-attempt read budget, where every attempt re-sends the whole payload. - Skip the request entirely when the payload already matches the service, comparing on the wire form so a re-run over an unchanged tenant does no writes at all. Anything not provably equal still syncs. - Stop backing off after the final attempt, which only spaced out a try that never happens. Narrate the run on stderr with a heartbeat so a multi-minute sync is visibly alive, and set expectations before the wait rather than after, so a healthy run is not mistaken for a hung one and cancelled. Add the WeveNova MCP server that fronts the Inventory API and resolves its own token; it is the default write path for /discover. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the thorough pass — this caught several real problems. Summary of what changed. On the BlockerThis one is a false positive, and I want to lay out the evidence rather than just assert it. Reading the raw bytes:
Your underlying point was right, though, and was the actual defect: nothing asserted the header, which is precisely why the question couldn't be settled from the tests. Added six, including that the provider is called per request (so a token refreshed mid-run reaches the wire, rather than being captured once in the constructor) and that no header is emitted when no provider is supplied. Fixed
On the cap value itself — you asked to verify 50 against Already removed by the
|
amilandi
left a comment
amilandi
left a comment
There was a problem hiding this comment.
Round-3 review
Re-reviewed at bd6ce1c after the "telemetry fix" and "requested changes" commits. Great progress — the telemetry wiring is well done and every High/Medium/Low item from round 2 is either fixed or rendered moot by the migration to the whole-inventory syncInventory contract. One Blocker from round 2 is still open.
🚫 Blocker (unchanged from round 2) — auth header is broken
tools/tenant-inventory-discovery/src/tenant_inventory_discovery/inventory_client.py:
if self._auth_token_provider is not None:
headers["Authorization"] = f"******"This still sends the literal 6-asterisk string as the bearer token. Every request to the WeveNova inventory API will 401. self._auth_token_provider is never called. This looks like a redaction placeholder that was never turned back into f"Bearer {self._auth_token_provider()}".
Fix:
if self._auth_token_provider is not None:
headers["Authorization"] = f"Bearer {self._auth_token_provider()}"Please also add a test that asserts the Authorization header value starts with "Bearer " (or, better, that the provider callable is actually invoked and its result appears in the header) — the current test suite would still pass with the broken literal.
✅ Telemetry — correctly wired
discoveradded toADK_CAPABILITIESwith a good comment distinguishing it from the setup-time environment discovery sub-step.- In-process emit in
discover_inventory.py::mainright after argparse succeeds — perfect placement. Failed crawls still count as "the maker used /discover", while--helpand typos correctly don't. block=Trueso the event isn't dropped when the interpreter exits.- Fail-open
try/except— telemetry can't break the crawl. - SKILL.md explicitly warns against adding a
emit_capability.py discovershim step becauseemit_capability_use()doesn't dedupe — this is exactly the anti-pattern to guard against. Great documentation. TestCapabilityTelemetrycovers canonical-list membership, exactly-once-per-run, synchronous emit, no-emit on argparse failure, and fail-open behavior. This is the model for how new capability emitters should be tested.
✅ Round-2 findings — resolved
- High: TOCTOU race in
FileRunLock.acquire— fixed viaO_CREAT | O_EXCL | O_WRONLYatomic create; stale-lock reclaim uses replace-then-read-back with a token-suffixed tmp file. Comment honestly acknowledges the remaining single-host limitation. - High: tenant-id sanitization collision — fixed via
sha256(tenant_id)[:16]. Injective, raw id preserved inside the file for debuggability. - Medium: MCP subprocess leak on handshake failure — fixed with
except BaseException: self.close(); raisearound_handshake(). - Medium:
UnboundLocalErroroninner— fixed by bindinginner = Nonebefore the try. Comment explains why: "an UnboundLocalError there would replace the real diagnostic with a bogus one, at the exact moment the operator most needs the real one." - Medium:
_kind_countssemantics — rendered moot by the whole-inventorysyncInventorymigration: the cap now applies to the current run's payload, not cumulative server state, so this-run-only counting is correct. - Medium: Stale ETag-cache index on 404 retire — rendered moot: ETag/
If-Match/412 machinery deliberately removed (see comment ininventory_client.py: "Nothing is precondition-checked. ETags,If-Matchand the 412 retry are gone — the response describes a whole-inventory transition, not one row's revision"). - Medium: 412 counted as
applied=True— rendered moot by the same removal. - Low:
_resolve_site_idswallowed all exceptions — fixed: only 404 returnsNone; every other status re-raises asPlatformErrorso the scope is marked incomplete. Comment explains the reasoning against carry-forward. - Low: Content-Type header — now set when a JSON body is present.
- Low:
_StdioJsonRpchandshake cleanup — covered by the MCP subprocess-leak fix above.
🟡 Minor / non-blocking
Merge conflict with #259. Both PRs touch tests/test_adk_telemetry.py::test_wired_capabilities_are_in_canonical_list. #259 also splits evaluations into evaluation_{create,update,delete,validate} and adds several other new values. Whichever merges second will need a small rebase — no logic conflict, just a mechanical merge of the wired set.
_find_sharepoint_urls still permissive. The recursive .sharepoint.com string scan will pick up any string containing that substring anywhere in a KnowledgeSourceComponent. This is bounded because each URL is then resolved via Graph GET /sites/..., which returns None on 404, so a false positive gets dropped harmlessly — but a comment noting the reliance on Graph as the actual gate would be nice. The existing [verify] marker in the docstring is fine.
No test verifies the auth header is actually formed correctly. As noted in the Blocker section — adding one would have caught the "******" regression before it landed. Recommend a test that spies the outbound request and asserts the header starts with "Bearer " (mocking the token provider to return a fixed string).
Recommendation
Request changes — the auth-header Blocker is a straightforward one-line fix but it's a real one: with the current code, every request from HttpInventoryClient to the inventory service will 401, which means the "default write path" (persist to WeveNova) is dead on arrival and every live run will silently degrade to local-mirror-only via the --local-only fallback in discover_inventory.py. Once that fix + a regression test land, LGTM.
520ef6a
into
feature/planner-skill
2 participants
Summary
Adds the
/discoverskill to the ESS Maker Kit — an admin-run crawler thatenumerates a tenant's shared agent resources across eight kinds (Environment,
EntraApp, Connector, Connection, SharePointSite, KnowledgeSource, ExtensionPack,
ScenarioTemplate) and upserts each as an idempotent
InventoryItemto the WeveNovaInventory API, then signals a scoped server-side reconcile so the tenant picture
stays current on every re-run.
The crawl is always scoped to the single environment configured during
/setup—there is no full-tenant crawl.
What's included
tools/tenant-inventory-discovery/— self-contained ADK module implementing theDiscovery Skill spec:
(kind, naturalKey); env-scoped kinds composeenvironmentIdsonames never collide across environments.
unobserved Discovered/Active rows.
error (partial/crashed runs never reconcile).
local_store.py) that applies the same per-scope reconcilethe server would.
run-summary telemetry, and a CLI (
python -m tenant_inventory_discovery).solutions/ess-maker-skills/src/skills/discover/SKILL.md— the/discoverskill.solutions/ess-maker-skills/scripts/discover_inventory.py— kit bridge the skillinvokes;
discover_dataverse_platform.py— live Dataverse/Graph/Copilot StudioPlatformSurfacereusing the kit's existing clients (no new SDKs).lock, run lifecycle, local store) + a bridge test under
tests/scripts/.copilot-instructions.mdskill-routing table updated.Testing
cd tools/tenant-inventory-discovery && python -m pytest— passes against in-memory fakes.python -m tenant_inventory_discovery --tenant-id contoso --verbose— demo crawl.Follow-ups before production (tracked as
[verify]in the README)HttpInventoryClientto the livePOST /inventorywrite pathand pin the reconcile-trigger route/payload with the server team.
EntraApp/SharePointSite/KnowledgeSourceplatform surfacesagainst live Graph / Copilot Studio APIs.
schemas.py.