Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends Kibi’s MCP tooling to (1) provide non-mutating semantic modeling guidance for prose-heavy requirements and (2) keep long-running MCP sessions consistent when a same-branch KB snapshot is externally rebuilt, while also bumping CLI/MCP release metadata and updating docs/tests accordingly.
Changes:
- Add a new
kb_semantic_advisorMCP tool and surface semantic-advisor receipts/warnings inkb_validate_upsertand successfulkb_upsertresponses. - Expand predicate-suggestion coverage (including usage hints) and add broad deterministic test coverage for semantic-advisor and predicate classification.
- Implement stat-based same-branch KB freshness detection/refresh in MCP session handling, plus CLI schema migration marker for semantic-advisor backfill state.
Reviewed changes
Copilot reviewed 71 out of 72 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Document semantic modeling guidance capability. |
| packages/opencode/CHANGELOG.md | Wording updates in release notes. |
| packages/mcp/tests/tools/validate-upsert.test.ts | Add coverage for advisor warnings/receipt in validate-upsert. |
| packages/mcp/tests/tools/upsert.test.ts | Relax assertions for expanded structured output fields. |
| packages/mcp/tests/tools/upsert-semantic-advisor.test.ts | New test: upsert returns advisor receipt/warnings. |
| packages/mcp/tests/tools/upsert-guidance-errors.test.ts | Update wording in guidance-error test. |
| packages/mcp/tests/tools/suggest-predicates.test.ts | Expand predicate catalog tests + usage-hints coverage. |
| packages/mcp/tests/tools/status.test.ts | Add coverage for registered tool path + freshness gate. |
| packages/mcp/tests/tools/semantic-advisor.test.ts | New tests for kb_semantic_advisor tool behavior. |
| packages/mcp/tests/tools/query.test.ts | Add coverage for registered query tool freshness gate. |
| packages/mcp/tests/server/tools-coverage.test.ts | Add tool coverage for kb_semantic_advisor registration/runtime wiring. |
| packages/mcp/tests/server/session.test.ts | Add same-branch freshness/refresh behavior tests. |
| packages/mcp/tests/server/kb-freshness.test.ts | New unit tests for stat-based stamp logic and error type. |
| packages/mcp/tests/server.test.ts | E2E regression: same process sees fresh KB after external rebuild. |
| packages/mcp/tests/semantic-advisor/prose-coverage-fixture.ts | New deterministic prose corpus fixture. |
| packages/mcp/tests/semantic-advisor/prose-coverage-corpus.test.ts | New corpus evaluator test ensuring full coverage. |
| packages/mcp/tests/semantic-advisor/analyze-prose.test.ts | Large new test suite for advisor analysis/suggestions. |
| packages/mcp/src/tools/validate-upsert.ts | Surface advisor receipt + warnings in validate-upsert results. |
| packages/mcp/src/tools/upsert.ts | Surface advisor receipt + warnings in upsert structured output. |
| packages/mcp/src/tools/semantic-advisor.ts | New MCP tool handler for non-mutating prose analysis. |
| packages/mcp/src/tools-config.ts | Add kb_semantic_advisor schema + update tool descriptions. |
| packages/mcp/src/server/tools.ts | Register new tool and extend runtime contract. |
| packages/mcp/src/server/session.ts | Add same-branch KB stamp validation + refresh flow. |
| packages/mcp/src/server/kb-freshness.ts | New module: stamp read/compare + error type + diagnostics. |
| packages/mcp/src/semantic-advisor/types.ts | New types for advisor receipts/signals/suggestions. |
| packages/mcp/src/semantic-advisor/prose-coverage-evaluator.ts | New evaluator to measure deterministic prose coverage. |
| packages/mcp/package.json | Version bump + dependency bump + formatting. |
| packages/mcp/CHANGELOG.md | Document new advisor behavior + freshness refresh changes. |
| packages/core/src/checks.pl | Update readiness guidance wording. |
| packages/core/CHANGELOG.md | Wording updates in release notes. |
| packages/cli/tests/commands/migrate.test.ts | Add migration test for semantic-advisor backfill marker. |
| packages/cli/src/utils/schema-version.ts | Bump latest schema version to 3. |
| packages/cli/src/utils/config.ts | Add semanticAdvisorBackfill config field support. |
| packages/cli/src/commands/migrate.ts | Persist semantic-advisor backfill marker during migrations. |
| packages/cli/schema/config.json | Extend config schema for new field + schemaVersion. |
| packages/cli/package.json | Version bump + formatting. |
| packages/cli/CHANGELOG.md | Release notes for schema/backfill marker + freshness. |
| documentation/tests/TEST-mcp-semantic-advisor-preflight.md | New doc test entity for semantic-advisor preflight. |
| documentation/tests/TEST-cli-migrate.md | Update doc test to mention backfill marker. |
| documentation/symbol-coordinates.yaml | Update symbol coordinate artifact for new/shifted symbols. |
| documentation/scenarios/SCEN-mcp-semantic-advisor-preflight.md | New scenario entity for advisor preflight behavior. |
| documentation/requirements/REQ-mcp-semantic-advisor-preflight.md | New requirement entity for advisor preflight behavior. |
| documentation/requirements/REQ-mcp-kb-freshness.md | New requirement entity for same-branch refresh behavior. |
| documentation/requirements/REQ-core-prolog-process-management.md | Add linkage to freshness requirement. |
| documentation/requirements/REQ-core-persistence.md | Add linkage to freshness requirement. |
| documentation/requirements/REQ-cli-schema-migration.md | Extend requirement text to include backfill marker. |
| documentation/facts/FACT-CAP-001.md | Replace “Align” naming with “example product”. |
| documentation/facts/FACT-ARC-018.md | Replace “Align” naming with “example product”. |
| documentation/facts/FACT-ARC-015.md | Replace “Align” naming with “example product”. |
| documentation/facts/FACT-ARC-013.md | Replace “Align” naming with “example product”. |
| documentation/facts/FACT-ARC-011.md | Replace “Align” naming with “example product”. |
| documentation/facts/FACT-ARC-010.md | Replace “Align” naming with “example product”. |
| documentation/facts/FACT-ARC-009.md | Replace “Align” naming with “example product”. |
| documentation/facts/FACT-ARC-008.md | Replace “Align” naming with “example product”. |
| documentation/facts/FACT-ARC-006.md | Replace “Align” naming with “example product”. |
| documentation/adr/ADR-021.md | New ADR for stat-based MCP KB freshness detection. |
| documentation/adr/ADR-019.md | Replace repo-specific naming with generic “example product”. |
| documentation/adr/ADR-015.md | Wording update (“Synchronize”). |
| docs/superpowers/plans/2026-06-07-semantic-advisor-preflight.md | New implementation plan doc for advisor preflight. |
| docs/superpowers/plans/2026-06-07-semantic-advisor-modeling-suggestions.md | New plan doc for expanding advisor suggestions. |
| docs/superpowers/plans/2026-06-06-kibi-agent-modeling-guidance-tooling.md | Generalize repo-specific references. |
| docs/proposals/2026-03-22-kb-contradiction-model-plan.md | Generalize repo-specific references. |
| docs/prompts/product-kb-improvement-agent-instructions.md | Rename/generalize KB improvement prompt doc. |
| docs/modeling-cheatsheet.md | Add workflow guidance for semantic advisor receipts. |
| docs/mcp-reference.md | Document kb_semantic_advisor + expanded predicate catalog/usage hints + freshness behavior. |
| docs/inference-rules.md | Note advisor receipts are advisory-only in v1. |
| docs/cli-reference.md | Document sync --rebuild interaction with MCP refresh. |
| .sisyphus/evidence/task-9-cli-core-reqs.txt | Remove evidence artifact from repo. |
| .sisyphus/evidence/task-7-verification.txt | Remove evidence artifact from repo. |
Comment on lines
+249
to
+271
| async function refreshAttachedBranchKbWithRetry( | ||
| prolog: PrologProcess, | ||
| kbPath: string, | ||
| currentStamp: BranchKbStamp, | ||
| assertGeneration: () => Promise<void>, | ||
| ): Promise<BranchKbStamp> { | ||
| let postAttachStamp = await refreshAttachedBranchKb( | ||
| prolog, | ||
| kbPath, | ||
| assertGeneration, | ||
| ); | ||
|
|
||
| if (sameBranchKbStamp(postAttachStamp, currentStamp)) { | ||
| return postAttachStamp; | ||
| } | ||
|
|
||
| postAttachStamp = await refreshAttachedBranchKb(prolog, kbPath, assertGeneration); | ||
| if (!sameBranchKbStamp(postAttachStamp, currentStamp)) { | ||
| throw new KbRefreshError("KB refresh failed: stamp changed during attach"); | ||
| } | ||
|
|
||
| return postAttachStamp; | ||
| } |
Comment on lines
+343
to
363
| const shouldRefresh = | ||
| attachedBranchKbPath === kbPath && | ||
| attachedBranchStamp !== null && | ||
| usesBranchKbPath(kbPath) && | ||
| (!hasValidatedSameBranchRefresh || | ||
| !sameBranchKbStamp(currentStamp, attachedBranchStamp)); | ||
|
|
||
| if (shouldRefresh) { | ||
| attachedBranchStamp = await refreshAttachedBranchKbWithRetry( | ||
| prologProcess, | ||
| kbPath, | ||
| currentStamp, | ||
| assertGeneration, | ||
| ); | ||
| hasValidatedSameBranchRefresh = true; | ||
| } else { | ||
| attachedBranchKbPath = kbPath; | ||
| attachedBranchStamp = currentStamp; | ||
| } | ||
| attachedBranchKbPath = kbPath; | ||
| return prologProcess; |
Comment on lines
+70
to
+85
| function canonicalize(value: unknown): string { | ||
| if (Array.isArray(value)) { | ||
| return `[${value.map((item) => canonicalize(item)).join(",")}]`; | ||
| } | ||
| if (isRecord(value)) { | ||
| return `{${Object.keys(value) | ||
| .sort() | ||
| .map((key) => `${JSON.stringify(key)}:${canonicalize(value[key])}`) | ||
| .join(",")}}`; | ||
| } | ||
| return JSON.stringify(value); | ||
| } | ||
|
|
||
| function payloadHash(payload: Record<string, unknown>): string { | ||
| return createHash("sha256").update(canonicalize(payload)).digest("hex"); | ||
| } |
…_upsert saves After kb_upsert writes runtime relationships and calls kb_save, the MCP session's attachedBranchStamp was not updated. This caused the next ensureProlog() to trigger a stale-detach refresh, unloading the in-memory RDF graph and reloading from disk — losing runtime state. - Export attachedBranchKbPath and add updateAttachedBranchStamp() in session.ts so mutation tools can sync the session stamp after saves. - In handleKbUpsert, read the fresh disk stamp after kb_save and update the session stamp to prevent spurious refresh on the next tool call. - In handleKbCheck, add prolog.invalidateCache() for safety, aligning with kb_graph's behavior and ensuring no stale query cache.
handleKbCheck now calls prolog.invalidateCache() at the start, but
multiple test files created mock prolog objects with only a
method. Add invalidateCache: () => {} to all mock prolog objects
used with handleKbCheck so tests pass.
Add tests confirming: 1. Failed relationship upserts roll back cleanly — entity remains intact 2. Failed relationship upserts do not remove existing relationships 3. Symbol coverage check sees relationships after successful upsert All tests pass, ruling out transaction rollback corruption as the cause of the remaining 42 symbol-coverage violations.
- Add fail-fast validation for relationship types in manifest and markdown extractors - Reject invalid relationship types with ManifestError/FrontmatterError - Validate relationship direction (from/to required, no self-references) - Add executable_for and validates to VALID_RELATIONSHIP_TYPES allowlist - Add executable_for and validates to canonical BaseRelationship type union - Update symbol coordinates for changed source files
When all retries in executePrologQuery exhaust with query-failed or session-related errors, attempt one best-effort terminate()/start() reset before propagating the final error. This prevents cascading failures where a corrupted Prolog session poisons all subsequent queries.
Previously, passing relationships=[] to kb_upsert would delete all existing relationships. Now empty array is treated the same as omitting the field: existing relationships are preserved. Only a non-empty array replaces them.
…covery - manifest.test.ts: invalid type and direction validation - markdown.test.ts: invalid type and self-reference validation - persistence.test.ts: Prolog session restart on query-failed cascade - upsert.test.ts: empty array preserves existing relationships
The validates relationship type was supported by the Prolog schema and canonical TypeScript type, but was missing from the manifest and markdown extractor allowlists. This caused staged validation to reject valid test fixture frontmatter that uses validates relationships.
Add non-negotiable rule that pre-existing test failures must always be fixed. Agents must never ship past broken tests or skip/bypass pre-existing failures.
- Wrap interactive mode queries in catch/3 (matching one-shot mode resilience) - Prolog kb_assert_relationship now throws descriptive errors when entities don't exist or relationship validation fails, instead of silently failing - JS translateError recognizes entity existence errors and invalid relationship errors with entity IDs and validation details - Sync persistence shows categorized tips for missing entities vs invalid relationships instead of generic message
kb_assert_entity_no_audit previously used rdf_retractall to wipe ALL triples for an entity before re-asserting properties. This destroyed existing relationship triples, making incremental relationship additions via kb_upsert impossible without resending the full relationship set. Now only property triples (non-entity-URI objects) are retracted during upsert. Relationship triples (entity URI objects matching kb:entity/*) are preserved across property updates.
…ection kb_assert_entity_no_audit now preserves relationships during upsert (post-MCP-safe behavior). projectStagedEntities needs explicit stale relationship cleanup before re-asserting entities with new relationships. New predicate kb_retract_entity_relationships/1 selectively removes only relationship triples (entity-URI objects) while preserving properties. projectStagedEntities calls it before kb_assert_entity_no_audit to ensure old relationships are cleared during projection.
The existence_error and type_error exceptions thrown by kb_assert_relationship are formatted by SWI-Prolog's print_message/2 as human-readable messages, not raw Prolog terms. The previous regex checks for existence_error(entity,... which never appears in stderr. Updated translateError to match the actual formatted output: - entity 'REQ-XXX' does not exist (with backtick quote prefix from Prolog) - Type error: bad_rel' This ensures rebuild relationship failures show descriptive messages like 'Target entity does not exist: REQ-XXX' instead of opaque 'Query failed'.
…format
The previous condition errorText.includes("entity '") failed because
SWI-Prolog's print_message/2 formats entity references with a backtick
prefix: entity `'REQ-FOO''. The new condition uses a regex character
class [`'"] that matches regardless of the quoting style.
Also enhances the rebuild relationship failure tip to list specific
missing entity IDs instead of showing a generic message.
…revent destructive process reset The interactive mode polling loop (prolog.ts:310) returned hardcoded "Query failed" when Prolog's catch/3 wrote false. to stdout, without checking errorBuffer for the actual error from print_message/2 on stderr. This caused a cascade: 1. All relationship failures during rebuild return "Query failed" 2. isQueryFailedError matches "entity does not exist" as a session failure 3. tryResetPrologProcess terminates and restarts Prolog 4. kb_attach state is lost on restart 5. kb_save is a no-op on unattached process 6. No kb.rdf written after rebuild Fix: - false-handler now checks errorBuffer before falling back to "Query failed" - isQueryFailedError no longer matches data errors (entity missing, invalid relationship) — only session-corruption patterns that benefit from a restart
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.
No description provided.