fix: address review follow-up findings (#13, #14, #15, #16) - #17
Merged
Conversation
KIMI_CONFIG_DOMAINS dropped disabledSkills, extraAgentDirs, secondaryModel, persistDefaultModel and agentsMdExpandIncludes even though both the v1 KimiConfig schema and the v2 config sections know them under the same names, so SDK consumers on the v2 engine silently lost those settings. Pin v2's materialized section defaults for the newly mapped domains in the parity projection. Closes #13
The v2 session skill catalog's ready promise only covers the initial load; a config-driven source reload could race the listing and serve a partially-merged view. Await awaitPendingReloads() instead, as the ISessionSkillCatalog contract requires of listing route handlers. Closes #14
…fig schemas extra_agent_dirs was missing from both the kap-server and the public protocol REST config schemas, and secondary_model from the protocol one, so REST clients could not round-trip keys the engine already supports (the routes map domains generically, so no handler change is needed). Closes #15
compactRowLabel hardcoded 1w for every weekly window, so a two-week quota window rendered as 1w. Render the duration like every other unit. Refs #16
…r noise The modelCatalogResolving fake lacked getRequester, so requester resolution crashed with a TypeError logged as [unexpected] across the agent tool suite; give it a stub requester. Stringify the error output in the two expected-runnable-execution throws, clearing the last four repo-wide lint errors (no-base-to-string/restrict-template-expressions). Closes #16
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
1 participant
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.
Problem
Follow-up issues filed from the PR #12 review (pre-existing findings, not merge fallout): #13, #14, #15, #16.
What changed
One commit per issue:
fix(node-sdk):KIMI_CONFIG_DOMAINSin the v2 config mapper now keepsdisabledSkills,extraAgentDirs,secondaryModel,persistDefaultModelandagentsMdExpandIncludes— both engines know them under identical names, so the field pick just needed the entries. v2's materialized section defaults for the newly mapped domains (incl. env-bindingsecondaryModel: {}) are pinned in the parityV2_INJECTED_SECTION_DEFAULTSprojection.fix(node-sdk):listSkillsawaitsawaitPendingReloads()instead ofready, per theISessionSkillCatalogcontract — a config-driven reload can no longer race the listing with a stale view.fix(protocol):extra_agent_dirsadded to the kap-server and public protocol REST config schemas;secondary_modeladded to the protocol one. No handler change needed — the routes map config domains generically (camel↔snake).fix(tui)+test(agent-core-v2):compactRowLabelrenders the actual window duration (2winstead of a hardcoded1w); the agent-tool suite'smodelCatalogResolvingfake gained a stubgetRequester(silences the[unexpected] TypeErrornoise), and the twoexpected runnable executionthrows stringify non-string output — clearing the last 4 repo-wide lint errors.Verification
pnpm run lint: 0 errors repo-wide (was 4); 0 warnings on touched filestsc --noEmitclean in node-sdk, protocol, kap-server, kimi-codeNote: one unrelated pre-existing stderr trace remains in tool.test.ts ("renders global tool restrictions…", harness
ConfigBackedModelCatalogwith an unconfigured model) — out of scope here.Closes #13, closes #14, closes #15, closes #16.