fix: resolve query-tool endpoint from destinations (explicit-first fallback)#44
Merged
Merged
Conversation
aa19bc7 to
d0b1daa
Compare
…llback) The query tools (tool-graph-query, tool-blob-read) read only the top-level context_intelligence_server_url/api_key scalars and never inspected the hook's destinations: block. A user configuring ONLY destinations: got working uploads but a dead query tool failing with "context-intelligence server URL not configured". Adapt the resolver split from PR #27 (HookConfigResolver / ToolConfigResolver) and add a per-field, explicit-first query-endpoint fallback in a shared helper context_intelligence/tool_resolver.py::resolve_query_endpoint: 1. first entry of the tool's own sources mapping (explicit read override; absent => synthesized {"default": ...} from the tool's explicit top-level scalars, config+coordinator only) 2. first hook destination (HookConfigResolver.destinations) — the bug-fix bridge 3. env AMPLIFIER_CONTEXT_INTELLIGENCE_SERVER_URL / _API_KEY (single canonical last-resort fallback; no *_PRIVATE_* names) else None. Each field (url, api_key) resolves independently; env never outranks the hook destination. Renamed ConfigResolver -> HookConfigResolver and the registered capability context_intelligence.config_resolver -> context_intelligence.hook_config_resolver (a ConfigResolver class alias is kept). Applied identically to graph-query and blob-read. Minimal port of PR #27's slice only (no skill-sync/fetcher machinery). Also add PYTHONPATH=${{ github.workspace }} to the module "Run tests" CI step so unit tests resolve the in-repo context_intelligence (incl. the new tool_resolver.py) by shadowing the @main-installed copy — the convention introduced in PR #27. Docs: consolidated the repeated README destinations sections into one authoritative block + a thin Quick-Start teaser, added a "Query tools — read-side endpoint" section documenting sources and the explicit-first/env model, fixed stale ConfigResolver/capability references, rewrote context/config-resolution.dot for the two-resolver model, and regenerated bundle.dot/bundle.png. Tests: full matrix (graph-query 44, blob-read 39, tool_resolver 19, hook 367) — read-config hit wins over destination; destinations-only falls through to the hook destination (core bug fix); per-field independence; explicit-first precedence; env as true fallback; all-miss -> None; multi-entry ordering determinism; blob-read parity; legacy scalar synthesis. Proven end-to-end in a Digital Twin with a real destinations-only config (single private-home-server destination, working_dir unavailable): the query resolves from the hook destination and succeeds. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…telligence-query The two context-intelligence read tools shipped as separate modules (tool-graph-query, tool-blob-read) that each built their own ToolConfigResolver and required sources to be configured twice (overrides.tool-graph-query.config AND overrides.tool-blob-read.config) -- two namespaces that could silently drift. Merge them into one module, tool-context-intelligence-query, that mounts BOTH tools. The single mount() builds ONE ToolConfigResolver and injects the same instance into both tools (constructor is now (coordinator, resolver=None); the unused config parameter is dropped from the tools). Result: one config namespace overrides.tool-context-intelligence-query.config.sources shared by both tools -- they can no longer diverge. Tool NAMES (graph_query, blob_read) and the explicit-first resolution semantics are unchanged; the lazy hook-resolver lookup stays at execute() time (tools mount before hooks -- a guaranteed kernel invariant). Idiomatic per the kernel/module contract (one module registering multiple tools, like tool-filesystem; mount() returns None -- non-callable returns are kernel-ignored; the shared resolver is plain injection, not a capability, since it is intra-module). Updated the sole composition site (agents/graph-analyst.md: two module entries -> one), the CI matrix (ci.yml: two lanes -> one, PYTHONPATH shadow kept), README (one sources block + repo tree), context/config-resolution.dot labels, the tool_resolver.py docstring, and regenerated bundle.dot/bundle.png. Tests: full per-tool matrix ported for both tools (85 passing) plus new module-level tests -- mount registers exactly two distinct tools; shared-resolver identity AND consistency; a late-mount timing test proving the hook lookup stays lazy at execute(); and malformed/empty destination inputs. CI shape (uv sync --frozen + PYTHONPATH shadow) green. Validation: end-to-end in a Digital Twin via the merged module's real installed code -- one mount() mounts BOTH tools, each resolves its endpoint from the first hook destination (tier 2), and both issue real READ requests (POST /cypher, GET /blobs) to a mock CI server carrying the destination's Bearer key; negative control (no hook resolver) reproduces the configuration_error. Bundle validator (full mode): packaging build passes, all bundles load "good", bundle.dot regenerated. No migration: sources is unreleased (introduced in this same PR), and the only composition site is in-repo, so the module-path change has no external consumers. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…ility, not events
metadata.json recorded `working_dir` by scraping it from event payloads
(`data.get("working_dir", "")`), which was wrong on three counts:
- working_dir is a SESSION attribute, not an event attribute;
- only session:start/fork events carry it, so if the first event seen for a
session was any other type, metadata was written with working_dir="" and only
corrected if/when session:start later reached this handler (and never, for
forwarded/partial sessions);
- a present-but-empty "" in the event payload was stored as-is (the default in
.get(key, "") only applies when the key is absent, not when it is empty).
Source it from the session instead. Add HookConfigResolver.working_dir -- a live
(uncached) read of the session.working_dir capability (returns "" when
unavailable), mirroring the access the resolver already uses for project_slug.
The LoggingHandler already holds the resolver, so both metadata sites now use
self._resolver.working_dir:
- _ensure_metadata: working_dir = self._resolver.working_dir
- _enrich_metadata_from_session_init: self._resolver.working_dir or meta.get(...)
(the `or meta.get` keeps a prior non-empty value if the capability read is
transiently empty).
Reading live (not cached) also tracks mid-session working-directory changes.
Tests: HookConfigResolver.working_dir (capability present / absent / empty /
non-str / no get_capability / not cached); and metadata sourcing -- working_dir
filled from the resolver when the first event is not session:start, a
present-but-empty event working_dir no longer wins, and an empty resolver value
does not clobber a previously-stored non-empty value. Full hook module suite: 380
passing.
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Make the read-side config constraints explicit in the "Query tools" section: - Only ONE read source is supported in this version; the read path does not fan out to multiple sources. If more than one `sources` entry is present, only the first (insertion order) is used and the rest are ignored. - State plainly that with nothing configured, the read tools use the FIRST configured `destination` in the hook config as their read source (tier 2). - Reinforce the single-source rule in the YAML example comments (name it `default`). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
The CI Lint job runs `ruff format --check .` from the repo root, whose ruff config differs slightly from the per-module venv config used during development. Reformat the new working_dir tests with the root config so the Lint job passes. No behavior change. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
cfbe526 to
f049931
Compare
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
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.
What this PR does
Fixes a real bug in the context-intelligence query path, introduces a coherent read-source config model, and corrects how session metadata records the working directory. All changes are scoped to the bundle's read/query side plus one hook metadata fix; the upload/fan-out write path is untouched .
sourcesis unreleased (introduced here), and the read-tool modules have a single in-repo composition site (agents/graph-analyst.md), so there is no migration for any existing user or external consumer.The bug being fixed (and why it mattered)
Symptom. With the server up and event upload working,
graph_queryfailed with:Root cause. Two orthogonal config-lookup paths that nothing bridged:
hook-context-intelligence) readsconfig.destinations["<name>"].url / .api_key.tool-graph-query,tool-blob-read) read only the top-level scalarscontext_intelligence_server_url/context_intelligence_api_key— it never inspected thedestinationsblock.After
destinations:became the primary documented config, a user who configures onlydestinations:got working uploads but a dead query tool. This is exactly the failure observed in the field with a single-destination, no-scalar, no-env config.The fix. Give the query tools an explicit-first, per-field fallback that finally reads the same config the uploader uses. See feature #1 below.
Features / changes
1.
fix:query-path endpoint resolution (explicit-first fallback) —747c5a7A shared helper
context_intelligence/tool_resolver.py::resolve_query_endpointresolves(server_url, api_key)independently per field, explicit-read-config first:sources(or adefaultsynthesized from explicit top-level scalars)destinationsentry — the bridge that makes adestinations-only setup "just work" for readsAMPLIFIER_CONTEXT_INTELLIGENCE_SERVER_URL/_API_KEY(single last-resort; no*_PRIVATE_*names)configuration_errorRationale for the ordering: an operator's explicit read config must win (least surprise), but absent it, the tools should transparently follow the same server the hook uploads to — so the common case needs zero extra config. Env is a true last resort and never outranks the hook destination.
Also adds
PYTHONPATH=${{ github.workspace }}to the module test step so unit tests resolve the in-repocontext_intelligence(incl. the newtool_resolver.py) instead of the published@maincopy.2.
refactor:merge the two read tools into one module —f944020The two read tools shipped as separate modules, each building its own resolver and requiring read config to be set twice (two namespaces that could silently drift). Merged into one module
tool-context-intelligence-querythat mounts both tools.mount()builds oneToolConfigResolverand injects the same instance into both tools; the tool constructor is now(coordinator, resolver=None)and the unusedconfigparam is dropped.overrides.tool-context-intelligence-query.config.sourcesserves both tools — they can no longer diverge.graph_query,blob_read); resolution semantics unchanged; the lazy hook lookup stays atexecute()(tools mount before hooks — a guaranteed kernel invariant). Idiomatic per the module contract (one module → multiple tools, à latool-filesystem;mount()returnsNonesince non-callable returns are kernel-ignored; plain injection, not a capability, for the intra-module resolver).Rationale: the tools always travel as a pair (the agent pulls both), so the natural brick boundary is "CI read tools," not "one tool per module." This removes the duplicate-config foot-gun and halves the packaging/lockfile/test/CI overhead.
3. Read-source config model —
sources(single source)The read config key is
sources(a mapping mirroring the hook'sdestinationsshape). Only a single read source is supported in this version — the read path does not fan out. With nothing configured, the read tools use the first configureddestinationin the hook config as their read source (tier 2).sourcesexists only for the read-replica / split-endpoint case.4.
fix(hook):source metadataworking_dirfrom the session capability, not events —6cd4f02metadata.jsonrecordedworking_dirby scraping event payloads (data.get("working_dir", "")), which was wrong on three counts:working_diris a session attribute, not an event attribute;session:start/forkevents carry it, so if the first event seen for a session was any other type, metadata was written withworking_dir: ""(and never corrected for forwarded/partial sessions);""in the event payload was stored as-is (the.get(key, "")default only applies when the key is absent).The fix. Added
HookConfigResolver.working_dir— a live (uncached) read of thesession.working_dircapability (the same source the resolver already uses forproject_slug;""when unavailable). TheLoggingHandlernow sources both metadata writes fromself._resolver.working_dir. Reading live also tracks mid-session working-directory changes. Rationale: session metadata should come from the session, deterministically, not depend on a particular event type arriving.Testing
mount()registers exactly two distinct tools; shared-resolver identity and consistency; a late-mount timing test proving the hook lookup stays lazy atexecute(); malformed/emptysourcesinputs. Hook metadata:working_dirfilled from the resolver when the first event isn'tsession:start, present-but-empty event value no longer wins, empty resolver doesn't clobber a prior value, and theworking_dirproperty handles missing/empty/non-str capability and is uncached. Query module 85 passing; hook module 380 passing; root 19 passing.mount()mounts both tools, each resolves its endpoint from the first hook destination (tier 2), and both issue real READ requests (POST /cypher,GET /blobs) to a mock CI server carrying the destination's Bearer key; negative control (no hook resolver) reproduces theconfiguration_error.bundle.dotregenerated.Commits
747c5a7destinations) + CI PYTHONPATH shadowf944020tool-context-intelligence-query(one shared resolver, onesourcesnamespace)6cd4f02working_dirfromsession.working_dircapability, not events0c20c12f049931