feat(vscode): add workflow lanes, provider management and skill commands - #22
Merged
Conversation
Every subagent in a DynamicWorkflow batch shares one parentToolCallId, so the webview funnelled all of them into a single flat subagent_steps array and targeted the array tail as the "current step". Two agents streaming at once wrote text, thinking, and tool calls into each other's steps. Steps now carry the emitting agent's identity and are targeted per agent. The subagent lifecycle events the adapter already received but discarded (started/completed/failed/suspended) are mapped to a SubagentStatus event, which gives each lane a status, a duration, and a result or error. On top of that attribution, DynamicWorkflow renders as per-agent lanes instead of an escaped-JSON argument dump: live activity, a step count, a status, and a progress bar filled relative to the busiest lane. There is no per-agent step total to divide by, so an absolute percentage would be fabricated; the caption names the denominator. Lane derivation lives in lib/workflow-lanes.ts rather than the store module, and the pure tool-argument helpers move to lib/tool-args.ts. Tool rendering is mutually recursive, so WorkflowCard receives its step-item renderer as a prop rather than importing it and closing an import cycle. The new test file is excluded from the extension tsconfig, which is where the webview-side tests are kept out of the extension program.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds subagent lifecycle events and workflow rendering, provider management, permission-mode persistence, asynchronous skill commands, publishing automation, TUI updates, and a repository-wide migration from Pythinker Code identifiers to Kimi Code identifiers. ChangesSubagent workflow lifecycle
VS Code platform controls
Kimi Code provider migration
TUI and presentation updates
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime as EventAdapter
participant Store as event-handlers
participant Lanes as workflow-lanes
participant UI as WorkflowCard
Runtime->>Store: dispatch SubagentStatus and SubagentEvent
Store->>Lanes: provide agent-scoped steps and statuses
Lanes->>UI: provide ordered workflow lanes
UI-->>UI: render progress, status, errors, and nested steps
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/vscode/test/event-adapter.test.ts (1)
477-478: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove conditional expected-payload construction from this test.
Put the expected status-specific fields in each
it.eachrow. Then compare the supplied complete payload directly. This removes both Oxlint warnings and makes each test case explicit.As per coding guidelines, use oxlint for linting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/test/event-adapter.test.ts` around lines 477 - 478, Update the test cases using it.each so every row supplies its complete expected payload, including status-specific error or result_summary fields. Remove the conditional expectedPayload construction and compare each supplied payload directly, eliminating the related Oxlint warnings.Sources: Coding guidelines, Linters/SAST tools
apps/vscode/webview-ui/src/components/WorkflowCard.tsx (1)
66-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-expandedto the lane toggle.The button controls a collapsible region at Lines 78-91. Without
aria-expanded, assistive technology cannot report whether the lane is open or closed.♿ Proposed fix
- <button onClick={() => setExpanded(!expanded)} className="w-full flex items-center gap-2 py-1 hover:bg-muted/50 transition-colors text-left" disabled={lane.stepCount === 0}> + <button onClick={() => setExpanded(!expanded)} aria-expanded={expanded} className="w-full flex items-center gap-2 py-1 hover:bg-muted/50 transition-colors text-left" disabled={lane.stepCount === 0}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/webview-ui/src/components/WorkflowCard.tsx` at line 66, Add aria-expanded to the button in WorkflowCard’s lane toggle, binding it to the expanded state so assistive technology receives the current open or closed status. Keep the existing click handler and disabled behavior unchanged.apps/vscode/test/event-handlers.test.ts (1)
27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the imports above the
vi.mockcalls.oxlint reports
import(first)for Lines 30-32. Vitest hoistsvi.mockabove all imports, so placing the three import statements before thevi.mockcalls keeps the mocks effective and clears the lint warnings.♻️ Proposed import reordering
import { beforeEach, describe, expect, it, vi } from "vitest"; +import { useChatStore } from "../webview-ui/src/stores/chat.store"; +import { deriveWorkflowLanes, maxLaneStepCount } from "../webview-ui/src/lib/workflow-lanes"; +import type { UIStepItem } from "../webview-ui/src/stores/chat.store"; const boundary = vi.hoisted(() => ({ @@ vi.mock("`@/components/ui/sonner`", () => ({ toast: { error: boundary.toastError, warning: boundary.toastWarning }, })); - -import { useChatStore } from "../webview-ui/src/stores/chat.store"; -import { deriveWorkflowLanes, maxLaneStepCount } from "../webview-ui/src/lib/workflow-lanes"; -import type { UIStepItem } from "../webview-ui/src/stores/chat.store";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/test/event-handlers.test.ts` around lines 27 - 33, Move the three import statements for useChatStore, deriveWorkflowLanes/maxLaneStepCount, and UIStepItem above the vi.mock call in event-handlers.test.ts so the file follows the import(first) rule while still allowing Vitest to hoist the mock correctly. Keep the existing vi.mock("`@/components/ui/sonner`", ...) setup unchanged and only reorder the import block relative to the mock.Source: Linters/SAST tools
apps/vscode/webview-ui/src/lib/tool-args.ts (1)
19-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the three identical path cases.
ReadFile,WriteFile, andStrReplaceFilereturn the same expression. Use case fallthrough to remove the duplication.♻️ Proposed consolidation
case "ReadFile": - return (args.path as string)?.split("/").pop() || "file"; case "WriteFile": - return (args.path as string)?.split("/").pop() || "file"; case "StrReplaceFile": return (args.path as string)?.split("/").pop() || "file";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/webview-ui/src/lib/tool-args.ts` around lines 19 - 24, Merge the ReadFile, WriteFile, and StrReplaceFile branches in the tool-argument switch by using shared case fallthrough, keeping one copy of the existing path-expression return.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/vscode/webview-ui/src/components/WorkflowCard.tsx`:
- Around line 70-73: Update the step-count label in WorkflowCard so the text
rendered from lane.stepCount uses the singular form for 1 and the plural form
otherwise, matching the existing agent pluralization pattern elsewhere in the
same component. Keep the queued and duration rendering unchanged, and adjust
only the step-count string logic in WorkflowCard.
In `@apps/vscode/webview-ui/src/lib/tool-args.ts`:
- Around line 3-12: Update parseArgs in tool-args.ts to validate the result of
JSON.parse before returning it, since scalar JSON like null or 12 does not
satisfy the Record<string, unknown> contract. Keep the existing empty-input and
parse-failure fallback behavior, but reject any non-object or null parse result
and return the raw-args fallback instead so WorkflowCard.tsx can safely read
parseArgs(call.arguments).description without a TypeError.
In `@apps/vscode/webview-ui/src/stores/event-handlers.ts`:
- Around line 284-295: Gate the subagent-status failure sweep in
apps/vscode/webview-ui/src/stores/event-handlers.ts (lines 284-295) on
result.return_value.is_error alongside toolItem?.subagent_status, preserving the
existing spawned/running updates only for aborted parent results. Add a
companion non-error test in apps/vscode/test/event-handlers.test.ts (lines
188-222) asserting spawned and running lanes remain unchanged when is_error is
false.
---
Nitpick comments:
In `@apps/vscode/test/event-adapter.test.ts`:
- Around line 477-478: Update the test cases using it.each so every row supplies
its complete expected payload, including status-specific error or result_summary
fields. Remove the conditional expectedPayload construction and compare each
supplied payload directly, eliminating the related Oxlint warnings.
In `@apps/vscode/test/event-handlers.test.ts`:
- Around line 27-33: Move the three import statements for useChatStore,
deriveWorkflowLanes/maxLaneStepCount, and UIStepItem above the vi.mock call in
event-handlers.test.ts so the file follows the import(first) rule while still
allowing Vitest to hoist the mock correctly. Keep the existing
vi.mock("`@/components/ui/sonner`", ...) setup unchanged and only reorder the
import block relative to the mock.
In `@apps/vscode/webview-ui/src/components/WorkflowCard.tsx`:
- Line 66: Add aria-expanded to the button in WorkflowCard’s lane toggle,
binding it to the expanded state so assistive technology receives the current
open or closed status. Keep the existing click handler and disabled behavior
unchanged.
In `@apps/vscode/webview-ui/src/lib/tool-args.ts`:
- Around line 19-24: Merge the ReadFile, WriteFile, and StrReplaceFile branches
in the tool-argument switch by using shared case fallthrough, keeping one copy
of the existing path-expression return.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4d666f4-d986-483b-8895-45f3f93cfd57
📒 Files selected for processing (16)
apps/vscode/shared/legacy-sdk.tsapps/vscode/src/runtime/event-adapter.tsapps/vscode/src/runtime/replay-adapter.tsapps/vscode/test/event-adapter.test.tsapps/vscode/test/event-handlers.test.tsapps/vscode/test/replay-adapter.test.tsapps/vscode/test/replay-resume.integration.test.tsapps/vscode/tsconfig.jsonapps/vscode/webview-ui/src/components/ChatMessage.tsxapps/vscode/webview-ui/src/components/ToolRenderers.tsxapps/vscode/webview-ui/src/components/WorkflowCard.tsxapps/vscode/webview-ui/src/lib/tool-args.tsapps/vscode/webview-ui/src/lib/workflow-lanes.tsapps/vscode/webview-ui/src/stores/chat.store.tsapps/vscode/webview-ui/src/stores/event-handlers.tsapps/vscode/webview-ui/tsconfig.json
Surface live monthly npm downloads for @pythoughts/pythinker-code in the hero, above the install command. Unify the badge accent across the site and README on the brand blue (#2b89ff).
Pluralize the lane step count, add aria-expanded to the lane toggle, guard parseArgs against non-object JSON.parse results, merge duplicate tool-label cases, and gate the lane-failure sweep on the parent ToolResult's is_error flag so a successful batch result no longer marks still-running lanes as failed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/vscode/test/event-handlers.test.ts`:
- Around line 223-243: Update the success-path test around workflowToolItem and
processEvent so it asserts the parent tool item stores the successful
ToolResult, proving the event was handled. Add a second agent that remains in
spawned status, then assert both agents retain their expected statuses and have
undefined endedAt after the successful result.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79a959f8-dbfa-400a-8e1e-60fb7880f3b9
📒 Files selected for processing (7)
README.mdapps/site/src/App.vueapps/site/src/components/LegacyDownloadsPopup.vueapps/vscode/test/event-handlers.test.tsapps/vscode/webview-ui/src/components/WorkflowCard.tsxapps/vscode/webview-ui/src/lib/tool-args.tsapps/vscode/webview-ui/src/stores/event-handlers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/vscode/webview-ui/src/lib/tool-args.ts
- apps/vscode/webview-ui/src/stores/event-handlers.ts
- apps/vscode/webview-ui/src/components/WorkflowCard.tsx
Assert the successful ToolResult actually lands on the tool item, and cover a still-spawned lane alongside the running one so the test can fail if the is_error gate regresses.
The periwinkle that #21 wrote into --primary and --muted-foreground tinted every neutral surface, so both tokens go back to their original oklch values. The accent it was compensating for now lives in its own --brand token, applied only where an accent is actually wanted: inline code in Markdown, and the DynamicWorkflow lane bars and running status dot, which were invisible against --muted once --primary went back to a near-black neutral. The scroll-to-bottom button and the effort toggle return to their blue accents for the same reason. Also: - a finished workflow lane renders a full bar. The bar is scaled to the busiest agent, so a lane that completed in fewer steps kept a permanent gap. - the generation-speed pill in the thinking row no longer wraps "46.0" and "t/s" onto two lines, and uses tabular-nums so it stops resizing.
The extension modelled approval as a yolo/afk flag pair that it mapped onto the engine's permission mode on every change. Three things were wrong with it: - `withGlobalYoloMode` re-applied the `pythinker.yoloMode` setting on every attach and resume, so a `/yolo` a user typed was silently reverted the next time the session was opened. - `applyLegacyApproval` never wrote the flags back, so nothing was persisted for a resume to restore in the first place. - the two call sites that did try to persist called `session.updateMetadata`, which does not exist on the SDK session — the optional call quietly did nothing, and the tests passed because the fake implemented that same non-existent method. The flag pair is gone. A session now carries the engine's `PermissionMode` directly, persisted in session metadata (`vscode_permission_mode`, in the `custom` bag the SDK actually reads back) whenever it changes. The setting seeds sessions that have never recorded a mode and is applied to the live sessions when the user changes it — it no longer overrides on attach. `/yolo` and `/auto` now toggle between their mode and manual and accept `on`/`off`, matching the CLI. The one behaviour change: `/auto off` returns to manual instead of falling back to a remembered yolo, because a single mode has no second flag to fall back to. Sessions saved before this change have no stored mode and start from the setting; their old `vscode_legacy_approval` metadata is ignored.
The extension already read the same ~/.pythinker-code/config.toml the CLI does, so providers added in the terminal appeared in the model picker — but there was no way to add one without leaving the editor. A Providers modal, reached from the gear menu, lists what config.toml defines and adds a provider from the models.dev catalog: search, paste a key or point at an environment variable, optionally pick a default model. Removal goes through the same harness call the CLI uses. Keys are written to config.toml exactly as the CLI writes them, so a provider added here works in the terminal too; the key itself never crosses into the Webview, which only learns whether one is configured and where it comes from. The import rules — dropping stale aliases before a re-import, and restoring the previous defaults only while they still resolve against the refreshed catalog — now live in one SDK routine that both the CLI and the extension call, instead of only in the CLI.
Skills never appeared in the extension's command menu, so they could only be run by typing the command by hand. The listing called `harness.listWorkspaceSkills`, which does not exist on the SDK harness — the optional call resolved to nothing every time, and the test that covered it mocked the same non-existent method, so the suite agreed. The catalog is session-scoped, so the list now comes from `session.listSkills` as it does in the CLI, and the commands are re-broadcast when a session is created or resumed — the Webview loads before a session exists, which is why a one-shot fetch at startup could never carry them. Built-in skills keep their bare command name, which the host parser previously could not route: it only recognized the `skill:` prefix, so a built-in skill invoked by name was sent to the model as plain text. The projection from a skill catalog to slash commands now lives in the SDK, shared with the CLI instead of duplicated per client.
A provider that authenticates over OAuth is required by the config schema to carry no apiKey at all, so the providers list flagged it in red as "No key configured" — a warning about a provider that is working. It now reads as signed in, and each row shows the provider's API host rather than only its wire type, which is what identifies a managed provider at a glance.
The menu matched a query against command descriptions as a loose subsequence, so the letters of "research" were found scattered through almost every description and the list came back unfiltered. Typing then looked like it did nothing but move a highlight that never left the first row. Matching is now ranked, with every way of matching a command name ordered ahead of a description match, and it stays forgiving about skipped letters and dropped separators. The selection also resets when the query changes, since a reordered list left it pointing at an unrelated command.
The managed OAuth provider authenticates against auth.kimi.com and serves its models from api.kimi.com, but it was registered as `managed:pythinker-code`. In a client that talks to several providers that reads as a first-party service rather than the one it actually is, and the models it installed carried a `pythinker-code/` prefix that said the same thing. It is now `managed:kimi-code`, with `kimi-code/*` model aliases and credentials under `oauth/kimi-code`. The provider name also stops being redeclared as a bare literal in four places and comes from the single exported constant instead. Existing configs are not rewritten: signing in again provisions the provider under its current name.
A description is prose, so matching it pulled in commands unrelated to what was typed: "/sk" reached "/yolo" because its description contains those letters, and the selection sat on it. Matching is now limited to the command name, which is what the user is typing and the only part they can predict.
The SGR pattern these tests used matched the `[0;…m` tail but not the ESC that introduces it, so every stripped frame kept a stray control byte between the styled spans. Assertions spanning two spans could then never match: a label and its value are coloured separately, so `Search: cwd` and `Kimi K2 Kimi ← current` had an invisible escape sitting in the middle. Ten assertions across six suites were failing on this. The pattern now includes the escape, and the two startup assertions that compared raw frames strip them like the rest.
elkaix
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/oauth/src/toolkit.ts (1)
261-273: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Exploitability: Moderate
Reachability path
● Entry packages/oauth/src/managed-usage.ts:199 fetchManagedUsage │ ▼ ● Sink packages/oauth/src/toolkit.tsReject non-HTTPS custom base URLs before authenticated requests.
managedUsageUrlandmanagedFeedbackUrlaccepthttp:URLs, then send the bearer token to them. Apply HTTPS validation in a shared URL normalizer for usage, feedback, and model requests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oauth/src/toolkit.ts` around lines 261 - 273, Update the shared URL normalization used by managed usage, managed feedback, and model requests to reject custom base URLs whose protocol is not HTTPS before obtaining or sending an access token. Ensure managedUsageUrl and managedFeedbackUrl, along with the model-request URL path, all use this validator while preserving existing default URL behavior.
🟡 Minor comments (19)
apps/pythinker-code/test/cli/export.test.ts-83-83 (1)
83-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the managed provider key in the OAuth mock.
KIMI_CODE_PROVIDER_NAMEmust remain'managed:kimi-code'. The current override makes telemetry request tokens with the stale'pythinker-code'key and can hide provider-key migration failures. Preserve the actual export or set the mock to'managed:kimi-code'.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/pythinker-code/test/cli/export.test.ts` at line 83, Update the KIMI_CODE_PROVIDER_NAME override in the OAuth mock to use the managed provider key "managed:kimi-code", or preserve the actual export so telemetry requests exercise the migrated key; do not use the stale "pythinker-code" value.Source: Path instructions
packages/node-sdk/test/catalog.test.ts-238-246 (1)
238-246: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse neutral provider fixture values.
Replace
anthropic,Anthropic,api.anthropic.com,@ai-sdk/anthropic, andANTHROPIC_API_KEYwith neutral example values. Use identifiers such asexample-provider,https://api.example.test, andEXAMPLE_PROVIDER_API_KEY.As per coding guidelines, “Use neutral placeholders such as
example.com,example.test, andYOUR_API_KEYinstead of real internal identifiers in public text and test data.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/node-sdk/test/catalog.test.ts` around lines 238 - 246, Update the fixture object in the catalog test to use neutral provider values: replace the Anthropic provider name, API host, npm package, and environment variable with example-provider equivalents such as example-provider, api.example.test, `@ai-sdk/example-provider`, and EXAMPLE_PROVIDER_API_KEY, while preserving the fixture structure.Source: Coding guidelines
packages/oauth/src/toolkit.ts-398-398 (1)
398-398: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the Unicode flag to the regular expressions in
packages/oauth/src/toolkit.ts:398andpackages/node-sdk/test/catalog.test.ts:301,313. Runpnpm lint:fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oauth/src/toolkit.ts` at line 398, Update the regular expressions in packages/oauth/src/toolkit.ts at 398 and packages/node-sdk/test/catalog.test.ts at 301 and 313 to include the Unicode flag, then run pnpm lint:fix.Sources: Coding guidelines, Linters/SAST tools
packages/oauth/src/toolkit.ts-50-50 (1)
50-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove redundant
undefinedfrom the optional properties.Use
configAdapter?: ManagedKimiConfigAdapter<TConfig>andprovision?: ManagedKimiCodeProvisionResult.Proposed fix
- readonly configAdapter?: ManagedKimiConfigAdapter<TConfig> | undefined; + readonly configAdapter?: ManagedKimiConfigAdapter<TConfig>; @@ - readonly provision?: ManagedKimiCodeProvisionResult | undefined; + readonly provision?: ManagedKimiCodeProvisionResult;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oauth/src/toolkit.ts` at line 50, Remove the explicit undefined union from the optional configAdapter and provision properties, leaving configAdapter typed as ManagedKimiConfigAdapter<TConfig> and provision typed as ManagedKimiCodeProvisionResult while preserving their optionality.Source: Coding guidelines
packages/node-sdk/test/catalog.test.ts-199-199 (1)
199-199: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse type-safe fixtures instead of type assertions.
Normalize
initialinto a validPythinkerConfigbefore assigningconfig, and remove the assertion on the rebuilt state. Typeentrywithsatisfies Parameters<typeof importCatalogProvider>[1]['entry']instead ofas unknown as ...so contract changes remain visible to the test compiler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/node-sdk/test/catalog.test.ts` at line 199, Update the catalog test fixture setup around config and entry construction: normalize initial into a valid PythinkerConfig before assigning config, remove the rebuilt-state type assertion, and type entry using satisfies Parameters<typeof importCatalogProvider>[1]['entry'] instead of an unknown-based assertion.Source: Path instructions
packages/node-sdk/test/auth-facade.test.ts-540-543 (1)
540-543: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the production-like endpoint in this fixture.
The logout path removes
pythoughtsFetchby its service key. It does not requirehttps://api.pythinker.com/coding/v1/fetchas test data. Use a neutral endpoint such ashttps://api.example.test/coding/v1/fetch.As per coding guidelines, use neutral placeholders such as
example.com,example.test, andYOUR_API_KEYinstead of real internal identifiers in public text and test data.Proposed fix
- base_url = "https://api.pythinker.com/coding/v1/fetch" + base_url = "https://api.example.test/coding/v1/fetch"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/node-sdk/test/auth-facade.test.ts` around lines 540 - 543, Update the [services.pythoughts_fetch] fixture endpoint to a neutral placeholder such as https://api.example.test/coding/v1/fetch, preserving the service key and other fixture fields unchanged.Source: Coding guidelines
packages/oauth/test/managed-kimi-code.test.ts-63-63 (1)
63-63: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the
uflag to this regular expression.Oxlint reports
require-unicode-regexpfor this pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oauth/test/managed-kimi-code.test.ts` at line 63, Update the regular expression in the devKey assertion to include the Unicode (`u`) flag, while preserving its existing matching pattern.Source: Linters/SAST tools
packages/agent-core/src/services/modelCatalog/modelCatalogService.ts-163-166 (1)
163-166: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winComplete the Kimi Code provider-label migration.
These changed paths use Kimi provider identifiers but still expose Pythinker labels. Update each path to use one canonical Kimi Code identity.
packages/agent-core/src/services/modelCatalog/modelCatalogService.ts#L163-L166: replace'Pythinker Code'inprovider_namewith the canonical Kimi Code display name.apps/pythinker-code/src/tui/commands/auth.ts#L57-L59: route to a Kimi-named handler and update the handler's stalePythinker OAuthtext toKimi (OAuth).packages/agent-core/src/services/auth/managedAuth.ts#L68-L73: update the provisioning failure text from Pythinker to Kimi terminology.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/services/modelCatalog/modelCatalogService.ts` around lines 163 - 166, Complete the Kimi Code provider-label migration across all three sites: in packages/agent-core/src/services/modelCatalog/modelCatalogService.ts lines 163-166, use the canonical Kimi Code display name for provider_name; in apps/pythinker-code/src/tui/commands/auth.ts lines 57-59, route to a Kimi-named handler and change its stale Pythinker OAuth text to Kimi (OAuth); in packages/agent-core/src/services/auth/managedAuth.ts lines 68-73, replace Pythinker terminology in the provisioning failure message with Kimi terminology.apps/pythinker-code/test/tui/pythinker-tui-startup.test.ts-45-48 (1)
45-48: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the
uflag to both ANSI SGR regular expressions.Change
/gto/guforANSI_SGRand the inline expression at line 398.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/pythinker-code/test/tui/pythinker-tui-startup.test.ts` around lines 45 - 48, Add the Unicode flag to both ANSI SGR regular expressions: update the ANSI_SGR constant and the inline expression near the referenced startup test location from /g to /gu, without changing their matching behavior.Source: Linters/SAST tools
apps/pythinker-code/test/tui/components/dialogs/model-selector.test.ts-9-9 (1)
9-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the Unicode flag to the changed regular expressions.
Oxlint reports
require-unicode-regexpfor these expressions. Adduto each regular-expression literal.
apps/pythinker-code/test/tui/components/dialogs/model-selector.test.ts#L9-L9: change the ANSI expression suffix from/gto/gu.apps/pythinker-code/test/tui/components/dialogs/model-selector.test.ts#L98-L98: adduto the provider-output assertion.apps/pythinker-code/test/cli/provider.test.ts#L536-L536: adduto the provider-list assertion.apps/pythinker-code/test/tui/components/dialogs/provider-manager.test.ts#L104-L104: change the ANSI expression suffix from/gto/gu.apps/pythinker-code/test/tui/components/messages/goal-markers.test.ts#L8-L8: change the ANSI expression suffix from/gto/gu.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/pythinker-code/test/tui/components/dialogs/model-selector.test.ts` at line 9, Update every affected regular-expression literal to include the Unicode flag: in apps/pythinker-code/test/tui/components/dialogs/model-selector.test.ts lines 9-9 and 98-98, apps/pythinker-code/test/cli/provider.test.ts lines 536-536, apps/pythinker-code/test/tui/components/dialogs/provider-manager.test.ts lines 104-104, and apps/pythinker-code/test/tui/components/messages/goal-markers.test.ts lines 8-8. Preserve the existing patterns and flags while adding u, including changing the ANSI expressions from g to gu.Source: Linters/SAST tools
apps/pythinker-code/test/cli/provider.test.ts-500-500 (1)
500-500: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a neutral host in this test fixture.
Line 500 uses the production host
https://api.pythinker.com/coding/v1. If this test does not validate that exact host, replace it withhttps://example.com/coding/v1or another neutral fixture host.As per coding guidelines, use neutral placeholders such as
example.com,example.test, andYOUR_API_KEYinstead of real internal identifiers in public text and test data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/pythinker-code/test/cli/provider.test.ts` at line 500, Replace the production URL assigned to baseUrl in the test fixture with a neutral host such as https://example.com/coding/v1, preserving the existing path and test behavior.Source: Coding guidelines
apps/pythinker-code/test/cli/run-prompt.test.ts-121-121 (1)
121-121: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse
managed:kimi-codein both OAuth mocks.
KIMI_CODE_PROVIDER_NAMEismanaged:kimi-code, but both mocks override it withpythinker-code. Update both values and add an assertion against the production constant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/pythinker-code/test/cli/run-prompt.test.ts` at line 121, Update both OAuth mocks in apps/pythinker-code/test/cli/run-prompt.test.ts:121-121 and apps/pythinker-code/test/cli/run-shell.test.ts:114-114 to use managed:kimi-code for KIMI_CODE_PROVIDER_NAME, and add assertions in the relevant tests comparing the mock value with the production KIMI_CODE_PROVIDER_NAME constant.packages/agent-core/test/config/configs.test.ts-62-72 (1)
62-72: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a reserved host in the changed fixture.
Line 64 hard-codes
https://api.pythinker.com/coding/v1in test data. Replace it withhttps://api.example.test/coding/v1. This parser test does not need a product endpoint.Proposed change
- base_url = "https://api.pythinker.com/coding/v1" + base_url = "https://api.example.test/coding/v1"As per coding guidelines, use neutral placeholders such as
example.com,example.test, andYOUR_API_KEYinstead of real internal identifiers in public text and test data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/test/config/configs.test.ts` around lines 62 - 72, Update the changed fixture’s base_url in the providers."managed:kimi-code" configuration to use the reserved placeholder host https://api.example.test/coding/v1 instead of the product endpoint, leaving the rest of the parser test data unchanged.Source: Coding guidelines
packages/agent-core/test/services/oauth-service.test.ts-82-83 (1)
82-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the provider forwarded to
facade.logout. Pass'managed:kimi-code'toimpl.logoutand assertmock.logoutCalls[0].providerName. This prevents the result assertion from relying on the mock's fallback value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/test/services/oauth-service.test.ts` around lines 82 - 83, Update the OAuth logout test around the mocked logout result to pass providerName 'managed:kimi-code' explicitly to impl.logout, then assert mock.logoutCalls[0].providerName matches it. Keep the existing result assertion while ensuring it no longer depends on the mock's fallback provider value.Source: Path instructions
apps/vscode/src/handlers/slash-command.ts-160-167 (1)
160-167: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAn unrecognized argument toggles the permission mode.
Line 162 maps only
onandoffto an explicit mode. Every other argument yieldsundefined, so line 171 toggles. A typo such as/yolo oftherefore enables auto-approval of tool actions when the user intended to disable it. The emitted message reports the new mode, but the action has already been applied.Reject an unknown argument instead of toggling.
🐛 Proposed fix
const subcommand = args.trim().toLowerCase(); + if (subcommand !== "" && subcommand !== "on" && subcommand !== "off") { + emit(`Usage: /${mode} [on|off]`); + return; + } const requested = subcommand === "on" ? mode : subcommand === "off" ? "manual" : undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/src/handlers/slash-command.ts` around lines 160 - 167, Update the slash-command argument handling around subcommand and requested so unrecognized non-empty arguments are rejected rather than treated as a toggle. Preserve the existing explicit on/off mode behavior and ensure invalid input returns before applying or reporting any permission-mode change.apps/vscode/test/pythinker-harness.integration.test.ts-432-439 (1)
432-439: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStrengthen the no-session fallback assertion.
Line 438 asserts only that no command name starts with
skill:. That assertion also passes if the handler returns an empty array, so it does not prove the fallback returns the released commands. Assert the returned list equalsSLASH_COMMANDS.💚 Proposed fix
- expect((commands as Array<{ name: string }>).some((command) => command.name.startsWith("skill:"))).toBe(false); + expect(commands).toEqual(SLASH_COMMANDS);Import
SLASH_COMMANDSfrom the module that defines it if it is not already in scope.As per path instructions: "Tests must be able to fail: flag assertions that pass vacuously (empty-set matches...)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/test/pythinker-harness.integration.test.ts` around lines 432 - 439, Update the no-session fallback test around configHandlers[Methods.GetSlashCommands] to assert that the returned command list equals SLASH_COMMANDS, importing that symbol from its defining module if needed. Replace the startsWith("skill:")-only assertion so the test verifies the released command set rather than allowing an empty result.Source: Path instructions
packages/node-sdk/src/skill-commands.ts-44-64 (1)
44-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate command names collapse silently.
commandMap.set(commandName, skill.name)at line 55 overwrites any earlier entry with the same key, whilecommandsstill contains both entries. Two skills can produce the same command name: an explicitskill.commandNamecan equal another skill's derived name, and a built-in name can equal a user skill'scommandName. The result is a duplicated slash-command entry in the menu that always dispatches to the last skill in sort order.Built-ins sort first, so the user skill wins the map. Drop later duplicates instead, so the first (built-in) skill keeps the name.
🐛 Proposed fix
- .map((skill) => { + .flatMap((skill) => { const commandName = skill.commandName ?? (skill.source === 'builtin' || skill.isSubSkill === true ? skill.name : `skill:${skill.name}`); + if (commandMap.has(commandName)) return []; commandMap.set(commandName, skill.name); - return { + return [{ name: commandName, aliases: [] as readonly string[], description: skill.description ?? '', argumentHint: skill.argumentHint, - }; + }]; });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/node-sdk/src/skill-commands.ts` around lines 44 - 64, Update buildSkillSlashCommands so duplicate command names are excluded from commands and commandMap after the first occurrence; check whether commandMap already contains commandName before adding the skill, preserving sorted order so the first skill—especially a built-in—retains the name and later duplicates are dropped.apps/vscode/webview-ui/src/components/ChatArea.tsx-18-18 (1)
18-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse semantic theme tokens with a theme-specific foreground.
bg-brandandtext-primary-foregroundare defined, but the dark-theme--brandvalue has only 1.97:1 contrast with--primary-foreground. Usebg-brand text-primary-foreground dark:text-primarywithhover:opacity-90so the icon remains readable in both themes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/webview-ui/src/components/ChatArea.tsx` at line 18, Update the button’s className in ChatArea to use the semantic theme tokens bg-brand and text-primary-foreground, add dark:text-primary for dark-theme readability, and replace the hover background color with hover:opacity-90 while preserving the existing layout and visual utility classes.apps/vscode/webview-ui/src/components/inputarea/hooks/useSlashMenu.ts-52-57 (1)
52-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the selection when
slashCommandschanges.A live command refresh can reorder or shorten
filteredCommandswhilequeryis unchanged. The old index can then select a different command or point past the list end. Include the command collection, or the memoized filtered list, in this reset effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/webview-ui/src/components/inputarea/hooks/useSlashMenu.ts` around lines 52 - 57, Update the selection-reset useEffect in useSlashMenu to also depend on slashCommands or the memoized filteredCommands, so setSelectedIndex(0) runs when the command collection changes even if query is unchanged. Preserve the existing reset behavior for query changes.
🧹 Nitpick comments (13)
packages/oauth/src/managed-feedback.ts (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the Unicode flag to the trailing-slash expressions.
Oxlint reports
require-unicode-regexpat all three sites. Change/\/+$/to/\/+$/u.
packages/oauth/src/managed-feedback.ts#L33-L33: Add theuflag.packages/oauth/src/managed-kimi-code.ts#L222-L222: Add theuflag.packages/oauth/src/managed-usage.ts#L38-L38: Add theuflag.As per coding guidelines, use oxlint for linting and
pnpm lint:fixfor automatic formatting fixes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oauth/src/managed-feedback.ts` at line 33, Add the Unicode flag to each trailing-slash regular expression: update `/\/+$/` to use the `u` flag in packages/oauth/src/managed-feedback.ts lines 33-33, packages/oauth/src/managed-kimi-code.ts lines 222-222, and packages/oauth/src/managed-usage.ts lines 38-38. Run oxlint and use pnpm lint:fix for formatting.Sources: Coding guidelines, Linters/SAST tools
packages/oauth/src/managed-kimi-code.ts (1)
38-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove explicit
| undefinedfrom optional property types.Use
property?: Type, notproperty?: Type | undefined. Keep passingundefineddirectly at object construction sites.
packages/oauth/src/managed-kimi-code.ts#L38-L95: Update optional model, result, OAuth-reference, and environment properties.packages/oauth/src/managed-kimi-code.ts#L115-L183: Update optional configuration, adapter, and provisioning properties.packages/oauth/src/openai-codex-oauth.ts#L567-L575: Update optional OAuth configuration properties.As per coding guidelines, “Optional properties should not additionally include
undefinedin their types.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oauth/src/managed-kimi-code.ts` around lines 38 - 95, Remove explicit “| undefined” unions from all optional property declarations in packages/oauth/src/managed-kimi-code.ts at lines 38-95 and 115-183, including model, result, OAuth-reference, environment, configuration, adapter, and provisioning properties; retain direct undefined values at object construction sites. Apply the same optional-property cleanup in packages/oauth/src/openai-codex-oauth.ts at lines 567-575 for the OAuth configuration properties.Source: Coding guidelines
apps/pythinker-code/test/tui/components/dialogs/effort-selector.test.ts (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the Unicode flag to every updated ANSI regex.
Oxlint reports
require-unicode-regexpat all eight locations. Change each/gsuffix to/gu. This preserves the explicit ESC matching behavior.
apps/pythinker-code/test/tui/components/dialogs/effort-selector.test.ts#L7-L7: Change the ANSI regex to use/gu.apps/pythinker-code/test/tui/components/messages/tool-renderers/chip.test.ts#L11-L11: Change the ANSI regex to use/gu.apps/pythinker-code/test/tui/components/messages/tool-renderers/media.test.ts#L13-L13: Change the ANSI regex to use/gu.apps/pythinker-code/test/tui/components/messages/tool-renderers/registry.test.ts#L12-L12: Change the ANSI regex to use/gu.apps/pythinker-code/test/tui/components/panels/footer-goal-badge.test.ts#L8-L8: Change the SGR regex to use/gu.apps/pythinker-code/test/tui/controllers/session-event-handler-goal-queue.test.ts#L235-L235: Change the transcript regex to use/gu.apps/pythinker-code/test/tui/task-output-viewer.test.ts#L8-L8: Change the SGR regex to use/gu.apps/pythinker-code/test/tui/tasks-browser.test.ts#L13-L13: Change the SGR regex to use/gu.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/pythinker-code/test/tui/components/dialogs/effort-selector.test.ts` at line 7, Update the ANSI/SGR/transcript regular expressions to use the Unicode flag by changing their global suffix from /g to /gu at apps/pythinker-code/test/tui/components/dialogs/effort-selector.test.ts:7-7, apps/pythinker-code/test/tui/components/messages/tool-renderers/chip.test.ts:11-11, apps/pythinker-code/test/tui/components/messages/tool-renderers/media.test.ts:13-13, apps/pythinker-code/test/tui/components/messages/tool-renderers/registry.test.ts:12-12, apps/pythinker-code/test/tui/components/panels/footer-goal-badge.test.ts:8-8, apps/pythinker-code/test/tui/controllers/session-event-handler-goal-queue.test.ts:235-235, apps/pythinker-code/test/tui/task-output-viewer.test.ts:8-8, and apps/pythinker-code/test/tui/tasks-browser.test.ts:13-13; preserve the existing explicit ESC matching patterns.Source: Linters/SAST tools
apps/vscode/test/bridge-handler.test.ts (1)
651-651: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the
uflag to the regex.Oxlint reports
require-unicode-regexpon this line.🧹 Proposed fix
- expect((response as { error?: string }).error).toMatch(/needs an API key/); + expect((response as { error?: string }).error).toMatch(/needs an API key/u);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/test/bridge-handler.test.ts` at line 651, Update the regular expression in the response error assertion to include the Unicode flag required by the require-unicode-regexp lint rule, while preserving the existing /needs an API key/ match behavior.Source: Linters/SAST tools
apps/vscode/webview-ui/src/components/inputarea/hooks/slash-command-match.ts (1)
16-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the
uflag to both regexes.Oxlint reports
require-unicode-regexpon lines 17 and 23. The flag does not change the semantics of these character classes.🧹 Proposed fix
function letters(text: string): string { - return text.toLowerCase().replaceAll(/[^a-z0-9]/g, ""); + return text.toLowerCase().replaceAll(/[^a-z0-9]/gu, ""); } function matchesAnyWordPrefix(text: string, query: string): boolean { return text .toLowerCase() - .split(/[^a-z0-9]+/) + .split(/[^a-z0-9]+/u) .some((word) => word.startsWith(query)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/webview-ui/src/components/inputarea/hooks/slash-command-match.ts` around lines 16 - 25, Add the Unicode (`u`) flag to the regular expressions used in letters and matchesAnyWordPrefix, preserving their existing character classes and matching behavior.Source: Linters/SAST tools
apps/vscode/src/bridge-handler.ts (1)
156-168: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRe-broadcast the slash commands only when the session changes.
getOrCreateSessionruns on every prompt.chat.handler.tscalls it before each turn, so line 166 triggers alistSkills()call and a webview post on every message, even when the session already exists. The comment at line 201 states the intent is a one-time refresh after create or resume.Broadcast only when the returned runtime is new for this view.
♻️ Proposed refactor
getOrCreateSession: async (model, effort, sessionId) => { + const previous = this.runtime.getSessionForView(webviewId); const runtime = await this.runtime.openSession({ webviewId, workDir: this.requireWorkDir(webviewId), model, effort, yoloMode: VSCodeSettings.yoloMode, ...(sessionId === undefined ? {} : { sessionId }), }); this.fileManager.setSession(webviewId, baselineSession(runtime)); - void this.broadcastSlashCommands(webviewId); + if (previous?.id !== runtime.id) void this.broadcastSlashCommands(webviewId); return runtime; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/src/bridge-handler.ts` around lines 156 - 168, Update getOrCreateSession so broadcastSlashCommands is called only when openSession returns a runtime that is new for the current webview, while retaining the broadcast for newly created or resumed sessions. Avoid rebroadcasting when the existing session is reused on subsequent prompts, and preserve the existing session setup and return behavior.apps/vscode/src/handlers/provider.handler.ts (2)
20-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the catalog cache cannot serve a stale document for the host lifetime.
catalogCacheis a module-level promise that is cleared only when the fetch rejects. After one successful fetch,GetProviderCatalogreturns the same document until the extension host restarts. A user who cannot find a newly published provider has no way to force a refresh.The failure path is correct: the
.catchhandler runs after the??=assignment, so a rejected fetch clears the field and the next call retries.Consider adding a time-to-live or a refresh parameter on
Methods.GetProviderCatalog.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/src/handlers/provider.handler.ts` around lines 20 - 32, Update loadCatalog and the GetProviderCatalog request flow so a successful catalog fetch cannot remain cached for the entire extension-host lifetime. Add an appropriate TTL or explicit refresh mechanism, while preserving the existing rejection cleanup that retries failed fetches.
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
anyparameters with the imported catalog and config types.
toConfiguredProvidertypes bothproviderandconfigasany, andtoCatalogSummarytypesentryasany. The file already importsCatalog, soentrycan beCatalogProviderEntry, andctx.harness.getConfig()returns a typed config. Withany, every field access here is unchecked:provider.oauth,provider.source?.kind, andmodel.capability?.max_context_tokenswould not fail the build if the schema renamed a field.The optional chaining at line 139 is one symptom.
catalogProviderModelsreturnsCatalogModel, whosecapabilityis accessed without?.inside the SDK, so the guard here only exists because the type was erased.♻️ Proposed typing
-import { +import { CatalogProviderError, DEFAULT_CATALOG_URL, catalogConnectionWire, catalogProviderModels, fetchCatalog, importCatalogProvider, type Catalog, + type CatalogProviderEntry, } from "`@pythoughts/pythinker-code-sdk`";-function toCatalogSummary(id: string, entry: any): CatalogProviderSummary { +function toCatalogSummary(id: string, entry: CatalogProviderEntry): CatalogProviderSummary {Derive the
providerandconfigparameter types from the return type ofctx.harness.getConfig.Also applies to: 129-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/src/handlers/provider.handler.ts` around lines 42 - 45, Replace the any annotations in toConfiguredProvider and toCatalogSummary with the imported catalog types and the config type derived from ctx.harness.getConfig(). Type entry as CatalogProviderEntry, and derive provider/config parameter types from the harness configuration return type so accesses such as provider.oauth, provider.source?.kind, config.models, and model.capability are schema-checked; preserve the existing behavior.packages/node-sdk/src/catalog.ts (1)
214-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the transient invalid default explicit.
Line 222 passes
selectedModelId: ''when the caller requests no default.applyCatalogProviderthen writesconfig.defaultModel = "<providerId>/", which is not a resolvable alias. Lines 226-230 replace it, so the value never persists. The correctness of the whole function depends on that later assignment, which is easy to break during a refactor.Consider deriving the placeholder from the first imported model instead, so the intermediate config stays valid on its own.
♻️ Optional refactor
- selectedModelId: options.defaultModel ?? '', + selectedModelId: options.defaultModel ?? models[0]!.id,Note that this changes the intermediate value only; lines 226-230 still decide the persisted
defaultModel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/node-sdk/src/catalog.ts` around lines 214 - 235, Update the catalog provider setup around applyCatalogProvider to avoid passing an empty selectedModelId when options.defaultModel is undefined. Derive the placeholder selected model from the first imported model so applyCatalogProvider produces a resolvable intermediate default, while preserving the existing lines that restore previousDefaultModel or clear config.defaultModel for the persisted result.apps/vscode/src/handlers/slash-command.ts (1)
62-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated
skill:fallback.Lines 63 and 70 contain the identical expression. Extract it into a small local helper so the prefix length
6and the shape appear once.♻️ Optional refactor
+ const skillPrefixFallback = (): HostSlashCommand | undefined => + name.startsWith("skill:") ? { name, args, raw, skillName: name.slice("skill:".length) } : undefined; + - if (listSkills === undefined) { - return name.startsWith("skill:") ? { name, args, raw, skillName: name.slice(6) } : undefined; - } + if (listSkills === undefined) return skillPrefixFallback();Also applies to: 70-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/src/handlers/slash-command.ts` around lines 62 - 63, Extract the duplicated `skill:` fallback expression used in the `listSkills === undefined` branch and the corresponding later branch into a local helper near the surrounding slash-command logic. Have the helper centralize the `skill:` prefix check, `name.slice(6)`, and returned object shape, then reuse it at both call sites while preserving the existing undefined behavior for non-skill names.apps/vscode/test/pythinker-harness.integration.test.ts (1)
389-400: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the
listSkillsfixture instead of casting tonever.
listSkills as neverat lines 395 and 398 removes all type checking from the fixture. A missing or renamedSkillSummaryfield would not surface here. Cast through the parameter type so the fixture stays honest.♻️ Optional refactor
- const listSkills = async () => [ + const listSkills = async (): Promise<readonly SkillSummary[]> => [ { name: "gen-changesets", description: "", path: "/s", source: "builtin", type: "prompt" }, ]; await expect( - parseHostSlashCommand("/gen-changesets", listSkills as never), + parseHostSlashCommand("/gen-changesets", listSkills), ).resolves.toMatchObject({ skillName: "gen-changesets" });If
SkillSummaryrequires fields the test does not care about, add them to the fixture rather than widening the type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/test/pythinker-harness.integration.test.ts` around lines 389 - 400, Update the listSkills fixture in the “resolves a built-in skill invoked under its bare name” test to use the actual parameter type expected by parseHostSlashCommand instead of casting it to never. Add any required SkillSummary fields to the fixture while preserving the existing assertions and behavior.apps/vscode/webview-ui/src/components/WorkflowCard.tsx (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive the status dot a text equivalent.
StatusDotconveys the lane status through color only. The adjacent label at line 74 reportsqueuedanddone · N steps, but arunninglane and afailedlane both render justN steps. A user who cannot distinguish the colors has no status signal for a failed lane unlesslane.erroris also set.Add an accessible name to the dot.
♿ Proposed change
-function StatusDot({ status }: { status: WorkflowLane["status"] }) { +function StatusDot({ status }: { status: WorkflowLane["status"] }) { const color = status === "running" ? "bg-brand" : status === "done" ? "bg-success" : status === "failed" ? "bg-destructive" : "bg-muted-foreground"; - return <span className={cn("inline-block size-2 rounded-full shrink-0", color)} />; + return <span role="img" aria-label={status} title={status} className={cn("inline-block size-2 rounded-full shrink-0", color)} />; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/webview-ui/src/components/WorkflowCard.tsx` around lines 42 - 46, Update the StatusDot component to provide an accessible text equivalent for each status, including running and failed, rather than conveying status through color alone. Add an appropriate accessible name to the rendered span while preserving its existing visual classes and status-to-color mapping.packages/node-sdk/src/skill-commands.ts (1)
17-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct Vitest coverage for the public skill-command helpers.
Add tests in
packages/node-sdk/testforskill:namespacing, built-in ordering,userInvocable === false, andtype === 'reference'. Existing coverage is in application-level tests, not the public SDK package.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/node-sdk/src/skill-commands.ts` around lines 17 - 25, Add direct Vitest tests under the node-sdk test suite for the public skill-command helpers, including skill: namespacing, built-in ordering, and exclusion behavior for userInvocable === false and type === 'reference'. Exercise the exported helpers from skill-commands.ts directly and verify the expected results without relying on application-level coverage.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/vscode/src/handlers/slash-command.ts`:
- Around line 62-70: Wrap the awaited listSkills() call in parseHostSlashCommand
with error handling so a rejection falls back to the existing skill: prefix
check, matching the behavior for unlisted skills. Preserve the current
commandMap resolution when listing succeeds and return undefined for
non-skill-prefixed commands on failure.
In `@apps/vscode/webview-ui/src/components/ProvidersModal.tsx`:
- Around line 315-321: Update the provider selection handler around setSelected
and the modal’s Back handler to reset apiKey whenever the user changes or leaves
a provider. Preserve the existing resets for useEnvVar and defaultModel, and
clear the key in both interaction paths so credentials cannot carry over between
providers.
In `@packages/node-sdk/src/auth.ts`:
- Line 213: Update the managed authentication flow around
resolveKimiCodeRuntimeAuth and normalizeBaseUrl to validate that the configured
endpoint uses HTTPS before sending bearer-token requests. Reject HTTP or
otherwise unsafe managed endpoints and prevent redirects from bypassing this
scheme requirement, while preserving existing behavior for valid HTTPS
endpoints.
In `@packages/node-sdk/src/catalog.ts`:
- Around line 203-243: Serialize the read-modify-write sequence in the catalog
configuration update around ensureConfigFile, getConfig, applyCatalogProvider,
and setConfig. Use the store’s atomic update operation or mutation lock to hold
exclusive access across the complete sequence, preventing concurrent writers
from being overwritten by the final whole-section update.
---
Outside diff comments:
In `@packages/oauth/src/toolkit.ts`:
- Around line 261-273: Update the shared URL normalization used by managed
usage, managed feedback, and model requests to reject custom base URLs whose
protocol is not HTTPS before obtaining or sending an access token. Ensure
managedUsageUrl and managedFeedbackUrl, along with the model-request URL path,
all use this validator while preserving existing default URL behavior.
---
Minor comments:
In `@apps/pythinker-code/test/cli/export.test.ts`:
- Line 83: Update the KIMI_CODE_PROVIDER_NAME override in the OAuth mock to use
the managed provider key "managed:kimi-code", or preserve the actual export so
telemetry requests exercise the migrated key; do not use the stale
"pythinker-code" value.
In `@apps/pythinker-code/test/cli/provider.test.ts`:
- Line 500: Replace the production URL assigned to baseUrl in the test fixture
with a neutral host such as https://example.com/coding/v1, preserving the
existing path and test behavior.
In `@apps/pythinker-code/test/cli/run-prompt.test.ts`:
- Line 121: Update both OAuth mocks in
apps/pythinker-code/test/cli/run-prompt.test.ts:121-121 and
apps/pythinker-code/test/cli/run-shell.test.ts:114-114 to use managed:kimi-code
for KIMI_CODE_PROVIDER_NAME, and add assertions in the relevant tests comparing
the mock value with the production KIMI_CODE_PROVIDER_NAME constant.
In `@apps/pythinker-code/test/tui/components/dialogs/model-selector.test.ts`:
- Line 9: Update every affected regular-expression literal to include the
Unicode flag: in
apps/pythinker-code/test/tui/components/dialogs/model-selector.test.ts lines 9-9
and 98-98, apps/pythinker-code/test/cli/provider.test.ts lines 536-536,
apps/pythinker-code/test/tui/components/dialogs/provider-manager.test.ts lines
104-104, and
apps/pythinker-code/test/tui/components/messages/goal-markers.test.ts lines 8-8.
Preserve the existing patterns and flags while adding u, including changing the
ANSI expressions from g to gu.
In `@apps/pythinker-code/test/tui/pythinker-tui-startup.test.ts`:
- Around line 45-48: Add the Unicode flag to both ANSI SGR regular expressions:
update the ANSI_SGR constant and the inline expression near the referenced
startup test location from /g to /gu, without changing their matching behavior.
In `@apps/vscode/src/handlers/slash-command.ts`:
- Around line 160-167: Update the slash-command argument handling around
subcommand and requested so unrecognized non-empty arguments are rejected rather
than treated as a toggle. Preserve the existing explicit on/off mode behavior
and ensure invalid input returns before applying or reporting any
permission-mode change.
In `@apps/vscode/test/pythinker-harness.integration.test.ts`:
- Around line 432-439: Update the no-session fallback test around
configHandlers[Methods.GetSlashCommands] to assert that the returned command
list equals SLASH_COMMANDS, importing that symbol from its defining module if
needed. Replace the startsWith("skill:")-only assertion so the test verifies the
released command set rather than allowing an empty result.
In `@apps/vscode/webview-ui/src/components/ChatArea.tsx`:
- Line 18: Update the button’s className in ChatArea to use the semantic theme
tokens bg-brand and text-primary-foreground, add dark:text-primary for
dark-theme readability, and replace the hover background color with
hover:opacity-90 while preserving the existing layout and visual utility
classes.
In `@apps/vscode/webview-ui/src/components/inputarea/hooks/useSlashMenu.ts`:
- Around line 52-57: Update the selection-reset useEffect in useSlashMenu to
also depend on slashCommands or the memoized filteredCommands, so
setSelectedIndex(0) runs when the command collection changes even if query is
unchanged. Preserve the existing reset behavior for query changes.
In `@packages/agent-core/src/services/modelCatalog/modelCatalogService.ts`:
- Around line 163-166: Complete the Kimi Code provider-label migration across
all three sites: in
packages/agent-core/src/services/modelCatalog/modelCatalogService.ts lines
163-166, use the canonical Kimi Code display name for provider_name; in
apps/pythinker-code/src/tui/commands/auth.ts lines 57-59, route to a Kimi-named
handler and change its stale Pythinker OAuth text to Kimi (OAuth); in
packages/agent-core/src/services/auth/managedAuth.ts lines 68-73, replace
Pythinker terminology in the provisioning failure message with Kimi terminology.
In `@packages/agent-core/test/config/configs.test.ts`:
- Around line 62-72: Update the changed fixture’s base_url in the
providers."managed:kimi-code" configuration to use the reserved placeholder host
https://api.example.test/coding/v1 instead of the product endpoint, leaving the
rest of the parser test data unchanged.
In `@packages/agent-core/test/services/oauth-service.test.ts`:
- Around line 82-83: Update the OAuth logout test around the mocked logout
result to pass providerName 'managed:kimi-code' explicitly to impl.logout, then
assert mock.logoutCalls[0].providerName matches it. Keep the existing result
assertion while ensuring it no longer depends on the mock's fallback provider
value.
In `@packages/node-sdk/src/skill-commands.ts`:
- Around line 44-64: Update buildSkillSlashCommands so duplicate command names
are excluded from commands and commandMap after the first occurrence; check
whether commandMap already contains commandName before adding the skill,
preserving sorted order so the first skill—especially a built-in—retains the
name and later duplicates are dropped.
In `@packages/node-sdk/test/auth-facade.test.ts`:
- Around line 540-543: Update the [services.pythoughts_fetch] fixture endpoint
to a neutral placeholder such as https://api.example.test/coding/v1/fetch,
preserving the service key and other fixture fields unchanged.
In `@packages/node-sdk/test/catalog.test.ts`:
- Around line 238-246: Update the fixture object in the catalog test to use
neutral provider values: replace the Anthropic provider name, API host, npm
package, and environment variable with example-provider equivalents such as
example-provider, api.example.test, `@ai-sdk/example-provider`, and
EXAMPLE_PROVIDER_API_KEY, while preserving the fixture structure.
- Line 199: Update the catalog test fixture setup around config and entry
construction: normalize initial into a valid PythinkerConfig before assigning
config, remove the rebuilt-state type assertion, and type entry using satisfies
Parameters<typeof importCatalogProvider>[1]['entry'] instead of an unknown-based
assertion.
In `@packages/oauth/src/toolkit.ts`:
- Line 398: Update the regular expressions in packages/oauth/src/toolkit.ts at
398 and packages/node-sdk/test/catalog.test.ts at 301 and 313 to include the
Unicode flag, then run pnpm lint:fix.
- Line 50: Remove the explicit undefined union from the optional configAdapter
and provision properties, leaving configAdapter typed as
ManagedKimiConfigAdapter<TConfig> and provision typed as
ManagedKimiCodeProvisionResult while preserving their optionality.
In `@packages/oauth/test/managed-kimi-code.test.ts`:
- Line 63: Update the regular expression in the devKey assertion to include the
Unicode (`u`) flag, while preserving its existing matching pattern.
---
Nitpick comments:
In `@apps/pythinker-code/test/tui/components/dialogs/effort-selector.test.ts`:
- Line 7: Update the ANSI/SGR/transcript regular expressions to use the Unicode
flag by changing their global suffix from /g to /gu at
apps/pythinker-code/test/tui/components/dialogs/effort-selector.test.ts:7-7,
apps/pythinker-code/test/tui/components/messages/tool-renderers/chip.test.ts:11-11,
apps/pythinker-code/test/tui/components/messages/tool-renderers/media.test.ts:13-13,
apps/pythinker-code/test/tui/components/messages/tool-renderers/registry.test.ts:12-12,
apps/pythinker-code/test/tui/components/panels/footer-goal-badge.test.ts:8-8,
apps/pythinker-code/test/tui/controllers/session-event-handler-goal-queue.test.ts:235-235,
apps/pythinker-code/test/tui/task-output-viewer.test.ts:8-8, and
apps/pythinker-code/test/tui/tasks-browser.test.ts:13-13; preserve the existing
explicit ESC matching patterns.
In `@apps/vscode/src/bridge-handler.ts`:
- Around line 156-168: Update getOrCreateSession so broadcastSlashCommands is
called only when openSession returns a runtime that is new for the current
webview, while retaining the broadcast for newly created or resumed sessions.
Avoid rebroadcasting when the existing session is reused on subsequent prompts,
and preserve the existing session setup and return behavior.
In `@apps/vscode/src/handlers/provider.handler.ts`:
- Around line 20-32: Update loadCatalog and the GetProviderCatalog request flow
so a successful catalog fetch cannot remain cached for the entire extension-host
lifetime. Add an appropriate TTL or explicit refresh mechanism, while preserving
the existing rejection cleanup that retries failed fetches.
- Around line 42-45: Replace the any annotations in toConfiguredProvider and
toCatalogSummary with the imported catalog types and the config type derived
from ctx.harness.getConfig(). Type entry as CatalogProviderEntry, and derive
provider/config parameter types from the harness configuration return type so
accesses such as provider.oauth, provider.source?.kind, config.models, and
model.capability are schema-checked; preserve the existing behavior.
In `@apps/vscode/src/handlers/slash-command.ts`:
- Around line 62-63: Extract the duplicated `skill:` fallback expression used in
the `listSkills === undefined` branch and the corresponding later branch into a
local helper near the surrounding slash-command logic. Have the helper
centralize the `skill:` prefix check, `name.slice(6)`, and returned object
shape, then reuse it at both call sites while preserving the existing undefined
behavior for non-skill names.
In `@apps/vscode/test/bridge-handler.test.ts`:
- Line 651: Update the regular expression in the response error assertion to
include the Unicode flag required by the require-unicode-regexp lint rule, while
preserving the existing /needs an API key/ match behavior.
In `@apps/vscode/test/pythinker-harness.integration.test.ts`:
- Around line 389-400: Update the listSkills fixture in the “resolves a built-in
skill invoked under its bare name” test to use the actual parameter type
expected by parseHostSlashCommand instead of casting it to never. Add any
required SkillSummary fields to the fixture while preserving the existing
assertions and behavior.
In
`@apps/vscode/webview-ui/src/components/inputarea/hooks/slash-command-match.ts`:
- Around line 16-25: Add the Unicode (`u`) flag to the regular expressions used
in letters and matchesAnyWordPrefix, preserving their existing character classes
and matching behavior.
In `@apps/vscode/webview-ui/src/components/WorkflowCard.tsx`:
- Around line 42-46: Update the StatusDot component to provide an accessible
text equivalent for each status, including running and failed, rather than
conveying status through color alone. Add an appropriate accessible name to the
rendered span while preserving its existing visual classes and status-to-color
mapping.
In `@packages/node-sdk/src/catalog.ts`:
- Around line 214-235: Update the catalog provider setup around
applyCatalogProvider to avoid passing an empty selectedModelId when
options.defaultModel is undefined. Derive the placeholder selected model from
the first imported model so applyCatalogProvider produces a resolvable
intermediate default, while preserving the existing lines that restore
previousDefaultModel or clear config.defaultModel for the persisted result.
In `@packages/node-sdk/src/skill-commands.ts`:
- Around line 17-25: Add direct Vitest tests under the node-sdk test suite for
the public skill-command helpers, including skill: namespacing, built-in
ordering, and exclusion behavior for userInvocable === false and type ===
'reference'. Exercise the exported helpers from skill-commands.ts directly and
verify the expected results without relying on application-level coverage.
In `@packages/oauth/src/managed-feedback.ts`:
- Line 33: Add the Unicode flag to each trailing-slash regular expression:
update `/\/+$/` to use the `u` flag in packages/oauth/src/managed-feedback.ts
lines 33-33, packages/oauth/src/managed-kimi-code.ts lines 222-222, and
packages/oauth/src/managed-usage.ts lines 38-38. Run oxlint and use pnpm
lint:fix for formatting.
In `@packages/oauth/src/managed-kimi-code.ts`:
- Around line 38-95: Remove explicit “| undefined” unions from all optional
property declarations in packages/oauth/src/managed-kimi-code.ts at lines 38-95
and 115-183, including model, result, OAuth-reference, environment,
configuration, adapter, and provisioning properties; retain direct undefined
values at object construction sites. Apply the same optional-property cleanup in
packages/oauth/src/openai-codex-oauth.ts at lines 567-575 for the OAuth
configuration properties.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…essage The logo only rendered inside the streaming thinking indicator, so a finished message showed a bare step dot and nothing identified the assistant.
A step opens with its thinking or tool block, so a marker in the step gutter sat beside that block rather than beside the text the logo is meant to label.
The step-less branch rendered its markdown behind a bare left padding, so a plain text reply carried no logo. Both branches now share one logo gutter row.
Picking a command sent it on its own and cleared the input, so a command written into a longer message discarded that message. It now fills the token in place, like a file mention, and the user keeps typing.
- Blink repeats every 5s instead of playing once (WELCOME_BLINK_INTERVAL_MS) - Antenna bulb spins through ◐◓◑◒ frames for 6s at banner load, then restores the static ● (WELCOME_ANTENNA_SPIN_* constants) - Logo renderer gains renderPythinkerLogoAntennaRow + antennaFrame param - Animator renamed WelcomeLogoEyeAnimator → WelcomeLogoAnimator with per-loop timers (blink + spin) and a setAntennaFrame host hook
Shrinks pythinker-logo.png from 264 KB to 59 KB and adds the animated logo variant beside it.
A multi-step reply draws a connector between its step markers. The logo sits in that same gutter, so it cut the line in half. Multi-step replies now keep the timeline and drop the logo; single-step replies still show it.
The extension shipped from its own 0.6.x line while the CLI was on 0.8.x, so the two versions could not be matched up when reporting an issue.
VSCE_AZURE_CREDENTIAL=1 passes vsce's --azure-credential, so a maintainer can publish as the identity az login already established instead of minting a PAT. CI keeps using VSCE_PAT.
Bumping, building, packaging six targets, publishing and tagging were separate manual steps, and the Marketplace token had to be pasted in by hand each time. The script runs them in order behind preflight checks and reads the token from the keychain.
The icon was a raw Windows DIB carrying a .png extension, so the storefront rendered a broken image. Re-encodes it as a real 256x256 PNG and drops the unreferenced duplicate beside it.
A timeout mid-run aborted the loop, leaving one target live, four unattempted and the release untagged. Transient failures now retry with backoff, a failed target no longer stops the others, an auth failure stops everything at once, and the summary names which targets are live so a re-run can finish the job.
pi-tui paints on requestRender before ui.start(), so frames rendered during construction and mounting were anchored at the shell cursor and the fixed layout's scroll-to-home pushed the panel border into scrollback. Rendering is now gated until the event loop starts, which runs before the main TUI mounts. The gate writes a private pi-tui field, so a test asserts both the gate and ui.start() clearing it — an upgrade that changes either fails there.
Skill resolution was reachable only through a live session, so a freshly opened panel showed the built-in commands alone and skills appeared only after the first message. Resolution now also runs at workspace scope, using the same roots a session resolves, and the panel calls that when it has no session yet. A live session is still preferred, since only it knows its MCP prompts.
A skill-catalog failure rejected out of the slash parser, whose caller awaits it outside any try block, so every message starting with "/" was dropped. It now degrades to the skill prefix check, matching how an unlisted skill is handled. Selecting a different provider, or leaving the form, kept the API key already typed, so a key entered for one provider could be saved under another's id.
elkaix
changed the title
Contributor
Author
|
All 4 inline findings triaged in d76fa06 — 2 fixed, 2 deferred with rationale on their threads (now resolved). Fixed
Deferred (rationale on each thread)
Title shortened to 72 chars and made imperative, per the title check. Also in this push: skills now list in the slash menu before a session exists, the TUI no longer anchors its first frames to the shell cursor, publishing retries transient registry failures, and the Marketplace icon is a decodable PNG. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/agent-core/src/rpc/core-impl.ts (1)
1159-1159: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve
managed:pythinker-codeduring provider migration.Existing configs retain the legacy key. Normalize the legacy provider and OAuth references before both lookups, or support the legacy alias. Add Vitest coverage for legacy and native configurations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/rpc/core-impl.ts` at line 1159, Update the provider migration flow around the KIMI_CODE_PROVIDER_NAME lookup to preserve the legacy managed:pythinker-code key. Normalize legacy provider and OAuth references before both lookups, or make the native lookup accept the legacy alias, and add Vitest coverage for both legacy and native configurations.
🧹 Nitpick comments (3)
apps/vscode/test/publish-retry.test.ts (1)
54-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove conditionals from the test bodies.
Oxlint flags both conditionals. Configure
publishOnewith orderedmockResolvedValueOnceandmockRejectedValueOnceresults instead. This keeps each test branch explicit.Also applies to: 78-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/vscode/test/publish-retry.test.ts` around lines 54 - 58, Remove the conditional logic from the publishOne mock in the affected tests. Configure its ordered outcomes with mockResolvedValueOnce and mockRejectedValueOnce so the failing darwin-arm64 attempt and subsequent successful publishes are explicit, while preserving the existing test behavior.Source: Linters/SAST tools
packages/agent-core/src/rpc/core-impl.ts (1)
620-632: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd Vitest coverage for the public workspace-skills RPC.
CoreAPI,CoreRPCClient,SDKRpcClientBase,PythinkerHarness, and the generic bridge already forwardlistWorkspaceSkills. Add a contract test that invokesPythinkerHarness.listWorkspaceSkills()and asserts the returnedSkillSummary[].🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/rpc/core-impl.ts` around lines 620 - 632, Add Vitest contract coverage for the public listWorkspaceSkills RPC by invoking PythinkerHarness.listWorkspaceSkills() through the existing bridge and asserting the returned SkillSummary[] contents. Reuse the established harness setup and fixture patterns, covering the workspace root resolution exposed by CoreAPI.listWorkspaceSkills without changing the RPC implementation.Source: Path instructions
apps/pythinker-code/src/tui/tui-state.ts (1)
74-78: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid coupling startup to pi-tui's private field.
Line 78 writes an implementation detail through a runtime cast and assumes that
TUI.start()restores the field correctly. The dependency inpackage.json, Line 69, is ranged as^0.83.0, so a package update can change this behavior without a TypeScript error. Prefer a supported pi-tui lifecycle hook. If none exists, pin the tested version and keep the regression test as the upgrade gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/pythinker-code/src/tui/tui-state.ts` around lines 74 - 78, Remove the direct private-field mutation of ui.stopped in the TUI initialization flow and replace it with a supported pi-tui lifecycle hook that prevents construction-time rendering until ui.start() begins. If no suitable public hook exists, pin the tested pi-tui version in package.json and retain or add the regression test covering startup rendering before allowing dependency upgrades.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/pythinker-code/test/tui/create-tui-state.test.ts`:
- Line 49: Remove the duplicate const ui declaration in the test block, keeping
a single declaration with the existing stopped/start type assertion so the test
can be collected successfully.
In `@apps/vscode/scripts/release-extension.mjs`:
- Around line 114-122: Update the release flow around the manifest write and
dryRun branch to capture the original manifest content before modification, then
restore it in a finally block whenever dryRun is enabled, including if build or
packaging fails. Preserve the existing dry-run logging and return behavior while
ensuring the manifest is restored after all paths.
In `@apps/vscode/src/handlers/config.handler.ts`:
- Around line 99-104: Update the skills selection expression around
session.listSkills and ctx.harness.listWorkspaceSkills to eliminate negated
conditions by reversing each conditional and swapping its corresponding
branches, while preserving the existing fallback to an empty array. Run pnpm
lint:fix to apply the required Oxlint formatting.
In `@apps/vscode/test/publish-retry.test.ts`:
- Around line 14-15: Replace the real publisher identifier in the classifyError
test case with a neutral placeholder such as “example,” while preserving the
timeout message and expected “transient” classification.
In `@packages/agent-core/src/rpc/core-impl.ts`:
- Around line 627-630: Update the workspace-skill listing method around
listWorkspaceSkills to call reloadRuntimeConfig() before resolving session skill
configuration, matching the runtime configuration path used during session
creation instead of directly calling readConfigForWrite(). Add regression
coverage verifying invalid skill settings fall back to salvaged or existing
runtime configuration without throwing.
In `@packages/agent-core/src/skill/workspace.ts`:
- Around line 33-51: Add Vitest coverage in matching test files for
packages/agent-core/src/skill/workspace.ts lines 33-51, covering root discovery
and summary projection without creating a session;
packages/agent-core/src/rpc/core-api.ts lines 390-393 and 501, covering the
ListWorkspaceSkillsPayload contract and CoreAPI.listWorkspaceSkills operation;
packages/node-sdk/src/rpc.ts lines 252-256, verifying SDKRpcClientBase forwards
{ workDir }; and packages/node-sdk/src/pythinker-harness.ts lines 239-243,
verifying PythinkerHarness.listWorkspaceSkills delegates and returns the RPC
result.
---
Outside diff comments:
In `@packages/agent-core/src/rpc/core-impl.ts`:
- Line 1159: Update the provider migration flow around the
KIMI_CODE_PROVIDER_NAME lookup to preserve the legacy managed:pythinker-code
key. Normalize legacy provider and OAuth references before both lookups, or make
the native lookup accept the legacy alias, and add Vitest coverage for both
legacy and native configurations.
---
Nitpick comments:
In `@apps/pythinker-code/src/tui/tui-state.ts`:
- Around line 74-78: Remove the direct private-field mutation of ui.stopped in
the TUI initialization flow and replace it with a supported pi-tui lifecycle
hook that prevents construction-time rendering until ui.start() begins. If no
suitable public hook exists, pin the tested pi-tui version in package.json and
retain or add the regression test covering startup rendering before allowing
dependency upgrades.
In `@apps/vscode/test/publish-retry.test.ts`:
- Around line 54-58: Remove the conditional logic from the publishOne mock in
the affected tests. Configure its ordered outcomes with mockResolvedValueOnce
and mockRejectedValueOnce so the failing darwin-arm64 attempt and subsequent
successful publishes are explicit, while preserving the existing test behavior.
In `@packages/agent-core/src/rpc/core-impl.ts`:
- Around line 620-632: Add Vitest contract coverage for the public
listWorkspaceSkills RPC by invoking PythinkerHarness.listWorkspaceSkills()
through the existing bridge and asserting the returned SkillSummary[] contents.
Reuse the established harness setup and fixture patterns, covering the workspace
root resolution exposed by CoreAPI.listWorkspaceSkills without changing the RPC
implementation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a197d492-ba6c-4457-ad35-40068f81a263
⛔ Files ignored due to path filters (5)
apps/vscode/resources/pythinker-icon-from-ico.pngis excluded by!**/*.png,!**/*.pngapps/vscode/resources/pythinker-icon-storefront.pngis excluded by!**/*.png,!**/*.pngapps/vscode/webview-ui/public/pythinker-logo.pngis excluded by!**/*.png,!**/*.pngapps/vscode/webview-ui/public/pythinker_animated.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.changeset/session-free-skill-listing.md.changeset/tui-render-gate.md.changeset/vscode-chat-logo-and-slash-insert.mdapps/pythinker-code/package.jsonapps/pythinker-code/src/tui/components/chrome/pythinker-logo.tsapps/pythinker-code/src/tui/components/chrome/welcome-logo-animation.tsapps/pythinker-code/src/tui/components/chrome/welcome.tsapps/pythinker-code/src/tui/pythinker-tui.tsapps/pythinker-code/src/tui/tui-state.tsapps/pythinker-code/test/tui/components/chrome/pythinker-logo.test.tsapps/pythinker-code/test/tui/components/chrome/welcome-eye-animation.test.tsapps/pythinker-code/test/tui/create-tui-state.test.tsapps/vscode/package.jsonapps/vscode/scripts/ovsx-publish.mjsapps/vscode/scripts/publish-retry.mjsapps/vscode/scripts/release-extension.mjsapps/vscode/scripts/vsix-publish.mjsapps/vscode/src/handlers/config.handler.tsapps/vscode/test/publish-retry.test.tsapps/vscode/webview-ui/src/components/ChatMessage.tsxapps/vscode/webview-ui/src/components/inputarea/InputArea.tsxflake.nixpackage.jsonpackages/agent-core/src/rpc/core-api.tspackages/agent-core/src/rpc/core-impl.tspackages/agent-core/src/skill/index.tspackages/agent-core/src/skill/workspace.tspackages/node-sdk/src/pythinker-harness.tspackages/node-sdk/src/rpc.ts
Listing workspace skills used the strict config read, so a warning in an unrelated section could make it throw where session creation would not; it now uses the same lenient runtime read. A dry-run release left the bumped manifest behind, tripping the next run's own clean-tree check. Adds coverage for the session-free skill catalog and drops a real publisher name from test data.
Merged
elkaix
pushed a commit
that referenced
this pull request
Aug 5, 2026
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @pythoughts/pythinker-code@0.9.0 ### Minor Changes - [#22](#22) [`45be822`](45be822) - Name the managed OAuth provider after the platform that serves it. It is reached over `auth.kimi.com` and `api.kimi.com`, but it was registered as `managed:pythinker-code`, which read as a first-party service in a client that talks to several providers. The provider id is now `managed:kimi-code`, its models are aliased `kimi-code/*`, and its credentials are stored under `oauth/kimi-code`. This is a breaking change for an existing config: the previous entries are not rewritten, so run `pythinker login` once to provision the managed provider under its current name, then remove the stale `managed:pythinker-code` entry. - [#22](#22) [`45be822`](45be822) - Resolve a workspace's skills without opening a session, so an editor panel can list them before its first message. ### Patch Changes - [#22](#22) [`45be822`](45be822) - Add an SDK routine that imports a catalog provider and its models into the persisted config, and use it for the CLI provider import so both entry points preserve existing defaults the same way. - [#22](#22) [`45be822`](45be822) - Stop the fixed-layout TUI anchoring its first frames to the shell cursor, which pushed the panel border into scrollback. ## @pythoughts/pythinker-code-sdk@0.11.0 ### Minor Changes - [#22](#22) [`45be822`](45be822) - Name the managed OAuth provider after the platform that serves it. It is reached over `auth.kimi.com` and `api.kimi.com`, but it was registered as `managed:pythinker-code`, which read as a first-party service in a client that talks to several providers. The provider id is now `managed:kimi-code`, its models are aliased `kimi-code/*`, and its credentials are stored under `oauth/kimi-code`. This is a breaking change for an existing config: the previous entries are not rewritten, so run `pythinker login` once to provision the managed provider under its current name, then remove the stale `managed:pythinker-code` entry. - [#22](#22) [`45be822`](45be822) - Add an SDK routine that imports a catalog provider and its models into the persisted config, and use it for the CLI provider import so both entry points preserve existing defaults the same way. ## pythinker-code@0.8.4 ### Patch Changes - [#22](#22) [`45be822`](45be822) - Show the assistant logo beside replies without breaking the step timeline, complete a picked slash command in the input instead of sending it on its own, ship a decodable Marketplace icon, and retry transient registry failures when publishing. - Updated dependencies [[`45be822`](45be822), [`45be822`](45be822)]: - @pythoughts/pythinker-code-sdk@0.11.0 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Aug 6, 2026
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.
Related Issue
No issue — the problems were found while using the extension. They are described below.
Problem
Several unrelated defects in the VS Code extension and one naming problem in the engine:
/yolodid not survive re-attach. The globalyoloModesetting overrode the session's own mode every time a session attached or resumed, the code meant to persist the mode calledsession.updateMetadata, which does not exist on the SDK, and the extension carried a legacy two-flag (yolo+afk) model in front of the engine's three-valuePermissionMode.getSlashCommandscalledharness.listWorkspaceSkills, which is not a method on the harness; the optional call swallowed it and returned an empty list. Commands were also fetched once, before any session existed, and built-in skills were only routable under askill:prefix.researchwere found scattered through nearly every description and the list came back unfiltered — typing appeared only to move a highlight that never left the first row.apiKey, but the providers list flagged that as "No key configured" in red.auth.kimi.comand serves models fromapi.kimi.com, yet registered itself asmanaged:pythinker-codeand installedpythinker-code/*model aliases. In a client that talks to several providers that reads as a first-party service rather than the one it is.What changed
Extension
PermissionMode(manual/auto/yolo). The legacy flag-pair shim is deleted. A stored session mode is now authoritative; the global setting only seeds a session that has never recorded one, and applies to open sessions on an explicit change. The sensitive-file and git-control-path policies deliberately keep running ahead of yolo-approve.session.listSkills()and are re-broadcast when a session is created or resumed. Built-in skills keep their bare name. The catalog-to-slash-command projection moved into the SDK so the CLI and the extension share one implementation.config.tomlthe CLI reads. The import routine it shares with the CLI moved into the SDK so both preserve existing defaults identically.--primaryto the neutral token and introduced a separate--brandaccent, so accent surfaces no longer re-tint body chrome.Engine
managed:kimi-code, withkimi-code/*aliases and credentials underoauth/kimi-code. Its name also stops being redeclared as a bare literal in four places and comes from the single exported constant. This is a breaking change for an existing config: previous entries are not rewritten, so signing in again provisions the provider under its current name.Tests
[0;…mtail but not the ESC byte introducing it, leaving a stray control character between styled spans. Any assertion spanning two spans could never match —Search: cwdandKimi K2 Kimi ← currenteach had an invisible escape in the middle. Fixing the pattern clears all ten failures.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.Summary by CodeRabbit
New Features
Bug Fixes
Style
Changes