fix(mcp): repair query-filter, facet-default, and prompt-tool integrity - #3445
Conversation
…jection Problem: query()'s default (unit-source) projection only passed expression/limit/continuation to query_units -- origin/tag/repo/since/ until/min_messages/max_messages/min_words were accepted by the tool's input schema but silently dropped before reaching query_units, and an unrecognised origin was accepted rather than rejected. Live evidence: query(expression="messages where role:user | count", origin="claude-code-session") returned the whole-archive count (208,061) instead of the origin-scoped count (141,652, matching the CLI's `--origin claude-code-session find ...`). Solution: query_units already accepts origin/tag/repo/since/until/ min_messages/max_messages/min_words as keyword filters (polylogue/ api/archive.py:3459) -- the MCP dispatcher in polylogue/mcp/ server_cutover.py just never forwarded them for the default projection. Forward them. Add loud rejection of unrecognised origin tokens (against core.sources.CORE_SCHEMA_ORIGINS, the same vocabulary the CLI's --origin validator uses) and of `sort` on the default projection, which has no session-level ordering concept for unit-source rows. Verification: - tests/unit/mcp/test_query_default_projection_filters.py (new, 3 tests): origin-filtered vs unfiltered counts differ correctly on a two-origin seeded archive; unknown origin returns invalid_argument; sort on default projection returns invalid_argument. devtools test tests/unit/mcp/test_query_default_projection_filters.py -> 3 passed. - Regression check: devtools test tests/unit/mcp/test_query_gap_projections.py tests/unit/mcp/test_query_request_contracts.py tests/unit/mcp/test_bounded_query_transport.py -> 22 passed. - Live archive (/realm/db/polylogue, read-only), in-process build_server(): origin="claude-code-session" -> count=141652 (CLI --origin claude-code-session find "messages where role:user | count" -> 141651; the off-by-one is archive growth between the two calls, not drift); no origin -> count=208061 (the old wrong answer, now only returned when no filter is given); origin="bogus-origin" -> ok:false, code:invalid_argument instead of the previous silent unfiltered pass-through. Ref polylogue-hnl7 Co-Authored-By: Claude <noreply@anthropic.com>
…a real connection Problem: polylogue-f5tq's AST sweep found _archive_facet_buckets(..., include_deferred=True) -- the shipped default, which feeds the facets() verb's distribution breakdowns -- was structurally unable to be exercised by its only existing test (test_archive_facet_buckets_count_unique_sessions_for_duplicate_hits), which constructs its archive stub with _conn=None and passes include_deferred=False. Passing True against that stub would crash dereferencing the None connection, so the real SQL-aggregation branch (_archive_aggregate_facet_families) has never run under test. Solution: add test_archive_facet_buckets_include_deferred_default_populates_sql_families, which seeds a real two-session ArchiveStore and calls _archive_facet_buckets(archive, None, include_deferred=True) against its live connection, asserting role_counts/message_types are actually populated by the SQL aggregation rather than left as the include_deferred=False branch's hard-coded empty dicts. Anti-vacuity: reverting either the include_deferred default or inverting its branch condition makes this new test (and the existing duplicate-hits test) fail with a real AttributeError / AssertionError -- verified locally, mutation reverted before commit. Triage of the sweep's other 15 untested-default findings: reproduced the described AST-sweep methodology (bare-name keyword-argument matching) locally. It reproduces the bead's own noted caveat -- bare-name matching is noisy -- concretely: it flagged storage/blob_gc.py's run_blob_gc(dry_run: bool = False) as an untested default, but grepping call sites shows the real (non-dry-run) default path is exercised by over a dozen tests that omit the kwarg entirely (test_blob_gc.py, test_blob_repair.py, test_blob_gc_generation_gate.py, test_blob_store_contracts.py) -- a false positive from keyword-only matching, not a real gap. Spot- checking the bead-named examples (exclude_none on model_json_document/to_json, detail on build_coordination_envelope/ embedding_status/embedding_readiness_info, require_overlays on verify_demo_archive, include_rows on plan_raw_backed_blob_reference_recovery) shows each toggles serialization verbosity or reporting detail, not control flow with a distinct correctness outcome -- the untested branch is a cosmetic variant of the tested one, not a shipped defect surface like facet_buckets was. Recorded here rather than adding a permanent sweep gate: the operator's standing "no completeness-check theater" rule says a gate is only warranted once the debt behind it is migrated, and manual triage did not surface a second concrete defect to migrate in the current 15. Verification: - devtools test tests/unit/api/test_facade_contracts.py -k facet_buckets -> 2 passed. - Anti-vacuity: inverted the include_deferred branch condition in polylogue/api/archive.py locally -> both facet_buckets tests fail (AttributeError: 'NoneType' object has no attribute 'execute' on the duplicate-hits test; AssertionError: role_counts must be populated on the new test). Reverted before commit. Ref polylogue-f5tq Co-Authored-By: Claude <noreply@anthropic.com>
Problem: 6 of 7 declared TARGET_PROMPTS (all but agent_coordination_brief) rendered call-sequence instructions naming tools retired at the six/ten-tool cutover -- find_resume_candidates, get_resume_brief, blackboard_list, find_abandoned_sessions, get_session_summary, get_postmortem_bundle, get_pathologies, list_assertion_claims, search, find_stuck_sessions, list_marks, list_annotations. An agent following a prompt's own instructions calls a tool that does not exist. The inverse gap also existed: 5 live-registered prompts (analyze_errors, summarize_week, extract_code, compare_sessions, extract_patterns) were absent from TARGET_PROMPTS, so every completeness/discovery consumer reading the declaration was blind to them. Neither gap was caught because EXPECTED_PROMPT_NAMES in tests/infra/mcp.py -- unlike the EXPECTED_TOOL_NAMES it sits next to -- was a hand-copied, stale set never referenced by any test (confirmed dead via grep). A sibling constant, EXPECTED_RESOURCE_URIS/EXPECTED_RESOURCE_TEMPLATE_URIS, was equally dead and equally stale against live resource registration in both directions. Solution: - Rewrote the 6 broken prompts' instruction text in polylogue/mcp/server_prompts.py to call only the live 10-tool surface: resume_context -> context(intent="resume", ...) + status(scope= "coordination") + query(projection="blackboard"); postmortem_last -> query(projection="abandoned_sessions"/"stuck_sessions"/"postmortem"/ "pathologies") + get(ref="session:..."); decisions_about -> query(expression=...) for recorded assertions + query(projection= "sessions", expression=...) for the ranked free-text fallback (the replacement for the retired search() tool); unacknowledged_failures and sessions_touching_file similarly. cost_of was already fixed by the concurrently-landed #3430 (rebased onto cleanly). - Added the 5 live-but-undeclared prompts to TARGET_PROMPTS (polylogue/mcp/declarations/registry.py), each tagged migration_owner="polylogue-il50". - Made EXPECTED_PROMPT_NAMES declaration-derived ({entry.name for entry in TARGET_PROMPTS}) instead of hand-copied, mirroring how EXPECTED_TOOL_NAMES is derived from declared_tool_names(). Removed the dead, doubly-stale EXPECTED_RESOURCE_URIS/ EXPECTED_RESOURCE_TEMPLATE_URIS rather than "fixing" them to match today's live registrations: TARGET_RESOURCES describes an aspirational future resource surface (polylogue-t46.8.2/t46.8.3) that does not match live server_resources.py registration, so deriving from it would assert something not yet true. Left a comment pointing at the real gap instead of duplicating that migration here. - New tests/unit/mcp/test_prompt_registry_pinning.py: (1) the live server._prompt_manager._prompts set equals EXPECTED_PROMPT_NAMES in both directions; (2) every declared prompt's rendered instruction text contains no call-like name outside the live tool surface -- a direct regression guard against any prompt drifting back to a retired name. - devtools render all regenerated the agent-integration doc mirrors (deep-reference.md, agent-integration-reference.md, integration- manifest/spec.json, mcp-equivalence.json) that TARGET_PROMPTS feeds. Verification: - devtools test tests/unit/mcp/test_prompt_registry_pinning.py tests/unit/mcp/test_prompt_query_parity.py -> 14 passed. - devtools test tests/unit/agent_integration/test_manual_contract.py tests/unit/agent_integration/test_installer.py tests/unit/agent_integration/test_assets_and_cli.py tests/unit/mcp/test_tool_declarations.py tests/unit/mcp/test_prompt_registry_pinning.py tests/unit/mcp/test_prompt_query_parity.py -> 50 passed. - devtools render all --check -> clean (grepped for "out of sync"). - Anti-vacuity: reverted decisions_about's query(projection="sessions", ...) fix back to search(...) -> test_prompt_instructions_reference_ only_live_tools[decisions_about] fails with the exact retired name ("references non-tool or retired-tool call-like names ['search']"). Deleted the analyze_errors TARGET_PROMPTS entry -> test_registered_prompts_match_target_prompts fails ("registered-but-undeclared: ['analyze_errors']"). Both mutations reverted before commit. - devtools verify --quick: the only failure (verify topology, 2 orphaned modules: polylogue/cli/commands/compare.py, polylogue/insights/measurement/registered_metrics.py) is pre-existing on rebased origin/master -- reproduced with `git stash` (all of this branch's changes removed) on the same commit; unrelated to this PR (introduced by the concurrently-merged #3430). Ref polylogue-il50 Co-Authored-By: Claude <noreply@anthropic.com>
Problem: the previous commit (fix(mcp): reconcile declared prompts with the live 10-tool surface) grew TARGET_PROMPTS from 7 to 12 declarations. polylogue://capabilities/query embeds the full mcp_algebra roster (read_transactions/resources/prompts, each via dataclasses.asdict) in one byte-budgeted discovery payload. The extra 5 prompt entries pushed the raw payload from 24694 to 25745 bytes -- over MCP_RESPONSE_BUDGET_BYTES (25000) -- which triggers the server's own budget-exceeded wrapping, but for this payload shape (almost entirely metadata, not a list of trimmable "items") the wrapped notice was itself still 25752 bytes: still over budget. tests/unit/mcp/test_envelope_contracts.py:: test_query_capability_resource_exposes_mcp_algebra_and_valid_terminal_forms caught this regression. Solution: drop migration_owner from the three asdict()'d algebra lists via a new _without_migration_owner() helper. migration_owner is an internal tracking-bead reference for this repo's own migration bookkeeping (values like "polylogue-t46.8.2", "polylogue-il50") -- not agent-facing capability data a caller needs to plan a query. Applied uniformly to read_transactions/resources/prompts rather than only the prompts list that grew, since the same non-value-adding field was inflating all three. Verification: - Payload size before this fix (in-process build_server(), real polylogue://capabilities/query resource): 25745 bytes raw / 25752 wrapped (over budget). After: 24465 bytes, returned directly (no wrapping needed). - devtools test tests/unit/mcp/test_envelope_contracts.py -> 25 passed. - devtools test tests/unit/mcp/ tests/unit/agent_integration/ -> 285 passed. - mypy polylogue/mcp/server_resources.py -> Success: no issues found. - ruff check/format polylogue/mcp/server_resources.py -> clean. - devtools render all --check -> clean (grepped for "out of sync"). Ref polylogue-il50 Co-Authored-By: Claude <noreply@anthropic.com>
Problem: origin/master's devtools verify --quick (and this branch's pre-push hook, which runs the same check) fails verify topology with two orphaned modules: polylogue/cli/commands/compare.py and polylogue/insights/measurement/registered_metrics.py. Both were added by the concurrently-merged "wire or delete unwired judgment/reference- pipeline/cost primitives" (#3430, dd912a7) without a `devtools render topology-projection` regeneration -- reproduced identically on a bare rebase of this branch onto origin/master with none of this branch's own commits applied (isolated via a temporary `git worktree add` at that exact commit), confirming it predates and is independent of this branch's polylogue-hnl7/f5tq/il50 work. Solution: `devtools render topology-projection`, which registers both modules and clears the two orphans (no other topology entries changed). Verification: - devtools verify --quick -> all 18 steps ok, exit 0 (previously failed only at "verify topology"). Ref polylogue-il50 Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da3585cd04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 1. list_assertion_claims(kinds="decision,judgment,lesson", statuses="active,candidate", limit={limit}) — recorded decisions (authoritative when user-authored). | ||
| 2. query(expression={assertion_query!r}) — targeted assertion search. | ||
| 3. search(query={ranked_query!r}, limit=10) — decision discussions never recorded as assertions. | ||
| 1. query(expression={assertion_query!r}, limit={limit}) — recorded decision assertions about the topic (authoritative when user-authored). |
There was a problem hiding this comment.
When decisions_about is rendered, this replacement query no longer preserves the retired call's kinds="decision,judgment,lesson" and statuses="active,candidate" constraints: it omits judgments and lessons while returning rejected, superseded, inactive, and deleted decision rows because assertion queries apply no implicit lifecycle filter. The following guidance only distinguishes candidates and calls recorded assertions authoritative, so an agent can present a rejected decision as settled; include both the original kind set and lifecycle filter in the terminal assertion expression.
Useful? React with 👍 / 👎.
| if origin is not None: | ||
| from polylogue.core.sources import CORE_SCHEMA_ORIGINS | ||
|
|
||
| bad_origins = [token.strip() for token in origin.split(",") if token.strip()] | ||
| bad_origins = [token for token in bad_origins if token not in CORE_SCHEMA_ORIGINS] |
There was a problem hiding this comment.
When callers filter default or session projections with origin="unknown-export", this validation now returns invalid_argument because CORE_SCHEMA_ORIGINS intentionally excludes that token from CLI choices, even though it is a valid persisted Origin and is needed to query unclassified sessions. Validate against enum_values(Origin) as the existing MCP query-contract validator does rather than using the narrower CLI-choice tuple.
AGENTS.md reference: AGENTS.md:L238-L242
Useful? React with 👍 / 👎.
| expression, | ||
| limit=limit, | ||
| continuation=continuation, | ||
| origin=origin, | ||
| tag=tag, |
There was a problem hiding this comment.
When origin contains multiple comma-separated tokens, the new validation explicitly splits and accepts each token, but this call forwards the original CSV string through the singular origin parameter. query_unit_session_filters() consequently emits origin = 'claude-code-session,chatgpt-export' rather than an origins IN (...) filter, so a valid multi-origin default query returns no rows. Pass the parsed values through origins or reject CSV input instead of accepting it and changing its meaning.
Useful? React with 👍 / 👎.
| 2. Pick the most recent relevant session; orient with get_session_summary(id=<session_id>). | ||
| 3. get_postmortem_bundle(repo="{repo_name}", since="{since}") — forensic bundle: timeline, decisions, tool errors. | ||
| 4. get_pathologies(repo="{repo_name}", since="{since}") — detected anti-patterns in the same window. | ||
| 1. query(projection="abandoned_sessions", repo="{repo_name}", since="{since}") and query(projection="stuck_sessions", repo="{repo_name}", since="{since}") — candidates with dangling work or stuck tool calls. |
There was a problem hiding this comment.
When this prompt is used in an archive containing multiple repositories, the repo argument on projection="stuck_sessions" is silently ignored: _query_insight_projection() constructs SessionLatencyProfileInsightQuery with only origin, since, until, and limit. The prompt therefore presents archive-wide stuck sessions as candidates for the requested repository, which can make the agent postmortem unrelated work; either wire repository filtering into this projection or instruct the caller to scope the returned items explicitly.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes three MCP/CLI query-surface defects found by a surface-coherence audit and a shipped-defaults sweep, ahead of a report that cites these surfaces as evidence:
query()'s default projection silently droppedorigin/tag/repo/since/until/min_messages/max_messages/min_words, and accepted an unrecognisedoriginwithout error._archive_facet_buckets(include_deferred=True)— the shipped default feeding thefacetsverb — was structurally untestable by its only existing test.EXPECTED_PROMPT_NAMES/EXPECTED_RESOURCE_URISwere dead, unreferenced test constants.Problem
hnl7:
query(expression='messages where role:user | count', origin='claude-code-session')returned 208,061 (the whole-archive count) instead of the origin-scoped count, becausepolylogue/mcp/server_cutover.py's default-projection dispatch only forwardedexpression/limit/continuationtoquery_units, even thoughquery_units(polylogue/api/archive.py:3459) already accepts every one of these filters as keyword arguments.origin='bogus-origin'was accepted silently rather than rejected, where the CLI's--originvalidator raises.f5tq: an AST sweep for untested shipped boolean defaults found
_archive_facet_buckets(..., include_deferred=True)(the default, and the branch that does the real SQL aggregation) was never exercised — the one existing test builds its archive stub with_conn=Noneand passesinclude_deferred=False, so passingTrueagainst that stub would crash on theNoneconnection.il50:
polylogue/mcp/server_prompts.py'sresume_context/postmortem_last/decisions_about/unacknowledged_failures/sessions_touching_fileprompts (5 of 6 broken;cost_ofwas already fixed by the concurrently-merged #3430) named retired tools (find_resume_candidates,get_session_summary,search,list_marks,blackboard_list, etc.) in their own call-sequence instructions. An agent following a prompt's own guidance calls a tool that doesn't exist.analyze_errors/summarize_week/extract_code/compare_sessions/extract_patternswere live-registered but absent fromTARGET_PROMPTS, leaving completeness/discovery consumers blind to them.Solution
origin/tag/repo/since/until/min_messages/max_messages/min_wordstoquery_unitsfor the default projection. Reject unrecognisedorigintokens loudly againstcore.sources.CORE_SCHEMA_ORIGINS(the same vocabulary the CLI's--originvalidator uses). Rejectsorton the default projection (no session-level ordering exists for unit-source rows) instead of silently ignoring it._archive_facet_buckets(..., include_deferred=True)against a seededArchiveStoreconnection, asserting the SQL-aggregated families (role_counts,message_types) are populated. Triaged the sweep's other ~15 findings in the commit body: reproducing the sweep locally reproduces its own noted caveat (bare-name keyword matching is noisy — e.g. it false-flagsrun_blob_gc'sdry_run=Falsedefault even though a dozen tests exercise it by omitting the kwarg), and manual spot-checks of the bead-named examples (exclude_none,detail,require_overlays,include_rows) show serialization/reporting-detail toggles, not a second concrete defect like the facet-buckets case.context(intent="resume", ...),query(projection=...),status(scope=...),get(ref=...)). Added the 5 undeclared prompts toTARGET_PROMPTS. MadeEXPECTED_PROMPT_NAMESdeclaration-derived (mirroringEXPECTED_TOOL_NAMES) instead of hand-copied. Removed the dead, doubly-staleEXPECTED_RESOURCE_URIS/EXPECTED_RESOURCE_TEMPLATE_URISrather than "fixing" them to match today's registration —TARGET_RESOURCESdescribes an aspirational future surface (polylogue-t46.8.2/t46.8.3) that doesn't match liveserver_resources.pyregistration, so deriving from it would assert something not yet true; left a pointer comment instead of duplicating that migration here. Addedtests/unit/mcp/test_prompt_registry_pinning.py: registered-prompts-equal-declared-prompts, and every prompt's rendered text references only live tool names. A follow-on commit drops the internalmigration_ownerbookkeeping field from thepolylogue://capabilities/querydiscovery payload'smcp_algebraroster — growingTARGET_PROMPTSfrom 7 to 12 pushed that byte-budgeted resource overMCP_RESPONSE_BUDGET_BYTES, caught by the existingtest_query_capability_resource_exposes_mcp_algebra_and_valid_terminal_forms. A last commit regenerates the topology projection for two modules (polylogue/cli/commands/compare.py,polylogue/insights/measurement/registered_metrics.py) added by the concurrently-merged wire or delete unwired judgment/reference-pipeline/cost primitives #3430 without that regeneration — reproduced as pre-existing/unrelated on a bare rebase via an isolatedgit worktree add, but needed to getdevtools verify --quick(the pre-push gate) green at all.Per-bead AC disposition
polylogue-hnl7: satisfied. Origin-filtered count now matches the CLI; bogus origin now rejected loudly;
sorton the default projection now rejected loudly instead of silently ignored.polylogue-f5tq:
_archive_facet_buckets(include_deferred=True)satisfied with a real test + anti-vacuity check (inverting the branch condition makes both facet-bucket tests fail). The 17-item sweep triage is satisfied as a documented spot-check rather than 15 individual tests: reproducing the sweep found it structurally noisy (false positive onrun_blob_gc), and the bead-named examples inspected are cosmetic-branch toggles, not confirmed second defects. Nodevtools lab policygate was added, per the operator's standing no-completeness-check-theater rule — this pass did not surface a second migratable defect to justify one.polylogue-il50: satisfied both directions — declared prompts now name only live tools (5 rewritten +
cost_ofalready fixed upstream), the 5 undeclared prompts are now declared, and both directions are pinned by a real test instead of a dead constant. TheEXPECTED_RESOURCE_URIScross-reference is intentionally not duplicated here (per the bead's own "cross-reference, do not duplicate" framing forpolylogue-t46.8.2); the dead constant was removed with a pointer to the real gap rather than force-fit to today's registration.Verification
devtools test tests/unit/mcp/test_query_default_projection_filters.py→ 3 passed.devtools test tests/unit/mcp/test_query_gap_projections.py tests/unit/mcp/test_query_request_contracts.py tests/unit/mcp/test_bounded_query_transport.py→ 22 passed.devtools test tests/unit/api/test_facade_contracts.py -k facet_buckets→ 2 passed.devtools test tests/unit/mcp/test_prompt_registry_pinning.py tests/unit/mcp/test_prompt_query_parity.py→ 14 passed.devtools test tests/unit/mcp/ tests/unit/api/test_facade_contracts.py tests/unit/agent_integration/→ 569 passed, 1 pre-existing unrelated real-clock failure (test_archive_tiers_api_raw_artifacts_read_source_tier, explicitly documented as pre-existing on unmodified master in the concurrently-merged wire or delete unwired judgment/reference-pipeline/cost primitives #3430's own commit message).devtools render all --check→ clean (grepped for "out of sync").devtools verify --quick→ exit 0, all 18 steps ok./realm/db/polylogue, read-only, in-processbuild_server()):origin="claude-code-session"→ 141,652 (CLI--origin claude-code-session find "messages where role:user | count"→ 141,651; off-by-one is archive growth between the two calls); no filter → 208,061 (the old wrong answer, now scoped correctly);origin="bogus-origin"→{"ok": false, "code": "invalid_argument", ...}._archive_facet_buckets's branch condition, revertingdecisions_about's query fix to the retiredsearch()call, deleting ananalyze_errorsTARGET_PROMPTSentry — all three make the corresponding new test fail with the expected error.Not run
devtools verify --all(full non-integration suite) — the touched-surfacedevtools testruns above cover the changed modules; a full run wasn't judged necessary for this scope.