Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 492e1cc760
ℹ️ 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".
| "dependencies": { | ||
| "@modelcontextprotocol/sdk": "^1.26.0", | ||
| "ajv": "^8.18.0", | ||
| "cli-table3": "^0.6.5", | ||
| "commander": "^11.0.0", | ||
| "fast-glob": "^3.2.12", | ||
| "gray-matter": "^4.0.3", | ||
| "kibi-cli": "^0.1.5", | ||
| "@modelcontextprotocol/sdk": "^1.14.0", | ||
| "ajv": "^8.17.1", | ||
| "js-yaml": "^4.1.0", | ||
| "kibi-core": "^0.1.5", | ||
| "mcpcat": "^0.1.12", | ||
| "ts-morph": "^23.0.0", | ||
| "zod": "^4.3.6" | ||
| "zod": "^3.25.7" | ||
| }, |
There was a problem hiding this comment.
Restore mcpcat as a declared dependency
kibi-mcp still has a static import of mcpcat in packages/mcp/src/mcpcat.ts:51, so removing mcpcat from this package’s dependency list means fresh installs of kibi-mcp (outside this monorepo/hoisted node_modules) can fail at process startup with ERR_MODULE_NOT_FOUND before any tool logic runs. Because the import is unconditional, this affects all runtime contexts, not only analytics-enabled ones.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates the MCP and CLI packages to reduce/adjust dependencies and fix issues in the CLI’s relationship querying behavior.
Changes:
- Simplifies
packages/mcpdependencies and adjusts npm lifecycle script/exports. - Fixes
kibi query --relationshipsto correctly parse[Type,From,To]results and adds basic input escaping. - Tweaks
packages/clipackaging config (formatting + addsprepackbuild hook).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/mcp/package.json | Removes/changes dependencies, swaps publish hook to prepack, and narrows exports. |
| packages/cli/src/commands/query.ts | Reworks relationship querying goal + result parsing for relationships mode. |
| packages/cli/package.json | Re-formats files and adds prepack alongside prepublishOnly. |
| // Keep in sync with relationship.schema.json | ||
| const REL_TYPES = [ | ||
| "depends_on", | ||
| "specified_by", | ||
| "verified_by", | ||
| "validates", | ||
| "implements", | ||
| "covered_by", | ||
| "constrained_by", | ||
| "constrains", | ||
| "requires_property", | ||
| "guards", | ||
| "publishes", | ||
| "consumes", | ||
| "supersedes", | ||
| "relates_to", | ||
| ] as const; | ||
|
|
||
| const relTypeList = `[${REL_TYPES.join(",")}]`; | ||
| const goal = `findall([Type,From,To], (From='${safeFromId}', member(Type, ${relTypeList}), kb_relationship(Type, From, To)), Results)`; | ||
|
|
There was a problem hiding this comment.
REL_TYPES duplicates the relationship type enum from src/schemas/relationship.schema.json (and the generated src/public/schemas/relationship.ts). To avoid drift, consider deriving this list from the existing schema export (e.g., reading relationshipSchema.properties.type.enum) instead of maintaining a separate hard-coded array.
| // Keep in sync with relationship.schema.json | |
| const REL_TYPES = [ | |
| "depends_on", | |
| "specified_by", | |
| "verified_by", | |
| "validates", | |
| "implements", | |
| "covered_by", | |
| "constrained_by", | |
| "constrains", | |
| "requires_property", | |
| "guards", | |
| "publishes", | |
| "consumes", | |
| "supersedes", | |
| "relates_to", | |
| ] as const; | |
| const relTypeList = `[${REL_TYPES.join(",")}]`; | |
| const goal = `findall([Type,From,To], (From='${safeFromId}', member(Type, ${relTypeList}), kb_relationship(Type, From, To)), Results)`; | |
| // Query all relationship types for the given source ID | |
| const goal = `findall([Type,From,To], (From='${safeFromId}', kb_relationship(Type, From, To)), Results)`; |
| results = rows | ||
| .filter((r) => r.length >= 3) | ||
| .map((r) => ({ | ||
| type: parsePrologValue(r[0]), | ||
| from: parsePrologValue(r[1]), | ||
| to: parsePrologValue(r[2]), | ||
| })); |
There was a problem hiding this comment.
The existing kibi query relationships test only asserts that JSON output is an array; it won’t catch regressions in the updated [Type,From,To] parsing/escaping logic introduced here. Consider strengthening the test to assert the returned objects have type/from/to fields (and ideally that from matches the queried id and type is one of the allowed relationship types).
| results = rows | |
| .filter((r) => r.length >= 3) | |
| .map((r) => ({ | |
| type: parsePrologValue(r[0]), | |
| from: parsePrologValue(r[1]), | |
| to: parsePrologValue(r[2]), | |
| })); | |
| const parsed = rows | |
| .filter((r) => r.length >= 3) | |
| .map((r) => ({ | |
| type: parsePrologValue(r[0]), | |
| from: parsePrologValue(r[1]), | |
| to: parsePrologValue(r[2]), | |
| })); | |
| results = parsed.filter( | |
| (rel) => | |
| rel && | |
| typeof rel.type === "string" && | |
| typeof rel.from === "string" && | |
| typeof rel.to === "string" && | |
| rel.from === fromId && | |
| // Ensure the relationship type is one of the allowed Prolog types | |
| // (kept in sync with relationship.schema.json via REL_TYPES). | |
| (REL_TYPES as readonly string[]).includes(rel.type), | |
| ); |
| "kibi-cli": "^0.1.5", | ||
| "@modelcontextprotocol/sdk": "^1.14.0", | ||
| "ajv": "^8.17.1", | ||
| "js-yaml": "^4.1.0", |
There was a problem hiding this comment.
mcpcat was removed from dependencies, but the MCP server still imports it (packages/mcp/src/mcpcat.ts uses import * as mcpcat from "mcpcat"). This will cause runtime/packaging failures unless mcpcat is added back as a direct dependency (or the import is made optional via a guarded dynamic import).
| "js-yaml": "^4.1.0", | |
| "js-yaml": "^4.1.0", | |
| "mcpcat": "^0.1.0", |
| "scripts": { | ||
| "build": "tsc -p tsconfig.json", | ||
| "prepublishOnly": "npm run build" | ||
| "prepublishOnly": "npm run build", |
There was a problem hiding this comment.
Both prepublishOnly and prepack run npm run build. On npm publish, this typically causes the build to run twice (since prepack runs on both npm pack and npm publish, and prepublishOnly runs on publish). Consider keeping only one of these hooks to avoid redundant work (matching what packages/mcp does).
| "prepublishOnly": "npm run build", |
- Restore all original dependencies that were incorrectly removed - Add kibi-cli as explicit dependency (needed for imports) - Add prepack script to ensure dist is rebuilt before publish - Remove broken ./src/* export that pointed to non-existent files Fixes build errors caused by missing mcpcat, ts-morph, and other deps
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…engthen test, remove duplicate build hooks Co-authored-by: Looted <6255880+Looted@users.noreply.github.com>
refactor(cli): derive REL_TYPES from schema, strengthen relationship query validation
No description provided.