Conversation
Migrate root-level requirements, scenarios, tests, and facts into the documentation tree according to migration inventory. Migrations: - requirements/req1.md -> documentation/requirements/REQ-001.md - requirements/main-only.md -> documentation/requirements/REQ-MAIN-ONLY.md - scenarios/scen1.md -> documentation/scenarios/SCEN-001.md - tests/test1.md -> documentation/tests/TEST-ROOT-1.md - facts/FACT-007.md -> documentation/facts/FACT-007.md - facts/FACT-008.md -> documentation/facts/FACT-008.md - facts/FACT-009.md -> documentation/facts/FACT-009.md - facts/FACT-010.md -> documentation/facts/FACT-010.md - facts/FACT-011.md -> documentation/facts/FACT-011.md Changes: - Updated frontmatter 'source' field to reflect new paths - Assigned IDs where missing (TEST-ROOT-1, REQ-001, REQ-MAIN-ONLY) - Removed empty root directories after migration - No duplicate canonical copies remain
There was a problem hiding this comment.
Pull request overview
This PR migrates knowledge base artifacts into the documentation/ tree, updates CLI defaults/tests accordingly, and reduces the public MCP surface to four core tools (query/upsert/delete/check) while adjusting related integrations.
Changes:
- Migrate entity/document paths to
documentation/**and update CLI/E2E/unit tests to match. - Remove (or stop advertising) non-core MCP tools; update VS Code integration and MCP server tests to use
kb_query. - Add branch resolution/config utilities and tighten hook + branch KB lifecycle behavior (including new
post-rewritehook and staged markdown validation).
Reviewed changes
Copilot reviewed 185 out of 185 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
packages/cli/src/utils/config.ts |
Introduces shared default config + sync-compatible config loading for new documentation/** layout. |
packages/cli/src/commands/sync.ts |
Builds glob patterns from directory-style config paths; aligns sync cache keys with new layout. |
packages/cli/src/commands/check.ts + packages/cli/src/traceability/* |
Adds staged markdown validation to block embedded entity authoring. |
packages/cli/src/commands/branch.ts + hooks |
Improves branch KB creation semantics (copy-from-default-branch) and hook behavior. |
packages/mcp/src/server.ts + tests/docs |
MCP server now advertises only the four public tools; tests assert removed tools are rejected. |
packages/vscode/src/extension.ts + packages/vscode/README.md |
Switches source-linked context lookup from removed kbcontext to kb_query. |
documentation/** + brief.md |
Updates requirements/scenarios/facts to reflect new tool surface and branch semantics (but one spec mismatch remains). |
You can also share your feedback on Copilot code review. Take the survey.
| links: | ||
| - type: constrains | ||
| target: FACT-KB-SCOPE | ||
| - type: requires_property | ||
| target: FACT-KB-REPO-LOCAL | ||
| - type: requires_property | ||
| target: FACT-KB-PER-BRANCH | ||
| - type: specified_by | ||
| target: scen1 |
There was a problem hiding this comment.
The requirement links to target: scen1, but the scenario in the documentation tree is SCEN-001 (and the old scen1 artifact was removed). As written, sync/check won’t be able to resolve this relationship, causing coverage/traceability rules to misfire. Update the link target to the new scenario ID (or add the missing scenario entity with id scen1).
| - `kb.check`: run built-in consistency rules and return violations. | ||
| - `kb.branch.ensure`: ensure branch KB exists (copy-from-main if missing). | ||
| - `kb.branch.ensure`: ensure branch KB exists (copy-from-default-branch if missing, using the resolver order above). | ||
| - `kb.branch.gc`: list/delete stale branch KBs. | ||
|
|
||
| The MCP server MUST be branch-aware: every tool call MUST accept `branch` explicitly or default to the caller’s current git branch. |
There was a problem hiding this comment.
brief.md still specifies MCP tools kb.branch.ensure/kb.branch.gc and states every tool call accepts branch, but this PR removes MCP tools and constrains the public MCP surface to four tools elsewhere (server tests/docs). Please update this requirements doc to match the new public MCP contract, otherwise it will be an authoritative spec mismatch for future changes.
| // Debug: log resolved default branch for tests | ||
| // eslint-disable-next-line no-console | ||
| console.log(`DEBUG_DEFAULT_BRANCH=${defaultBranch}`); |
There was a problem hiding this comment.
This tool logs DEBUG_DEFAULT_BRANCH=... to stdout unconditionally. That will pollute CLI output (and any scripts parsing it) and can break tests/users relying on stable kibi gc output. Please remove this debug print or gate it behind an explicit debug env var (e.g., KIBI_DEBUG) and keep the default output unchanged.
| const scenarioFields = ["scenarios", "given", "when", "then", "steps"]; | ||
| for (const field of scenarioFields) { | ||
| if (field in data) { | ||
| const value = data[field]; | ||
| if ( | ||
| Array.isArray(value) || | ||
| (typeof value === "object" && value !== null) | ||
| ) { |
There was a problem hiding this comment.
detectEmbeddedEntities only flags embedded scenario/test fields when the value is an array or object. If a requirement frontmatter embeds a scenario using scalar fields (e.g. given: "...", when: "...", then: "..."), this code will not detect it and the invalid authoring rule can be bypassed. Consider treating the presence of these keys (with any non-null value) as an embedded entity violation, or at least include string values in the check.
| // Branch changes are now handled automatically by the server. | ||
| // The branch parameter is ignored since the server re-resolves the active | ||
| // branch on each request and re-attaches to the correct KB transparently. | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const _unusedBranchCheck = branch && activeBranch && branch !== activeBranch; |
There was a problem hiding this comment.
This change makes the branch argument effectively a no-op (it’s ignored via an unused variable) even when it doesn’t match activeBranch. If branch is still part of the handler’s public contract, silently ignoring it can lead callers to believe they queried a different branch than the one actually attached. Please either remove branch from ContextArgs/the tool surface entirely, or keep the explicit mismatch error/warning behavior (per ADR-011’s intent).
|
@copilot open a new pull request to apply changes based on the comments in this thread Also please fix failing unit tests. |
Co-authored-by: Looted <6255880+Looted@users.noreply.github.com>
Fix review comments: REQ-001 link, brief.md spec, gc.ts debug log, scalar embedded entity detection, context.ts branch
No description provided.