Repository navigation
Conversation
🧙 Wizard CIRun the Wizard CI and test your changes against wizard-workbench example apps by replying with a GitHub comment using one of the following commands: Test all apps:
Test all apps in a directory:
Test an individual app:
Show more apps
Test against a Context Mill branch:
Add Results will be posted here when complete. |
f8c053f to
f0d8c36
Compare
abb8407 to
699ef88
Compare
f0d8c36 to
c03101f
Compare
699ef88 to
7e6425d
Compare
7e6425d to
a5ec738
Compare
c03101f to
24a0712
Compare
a5ec738 to
4f429c7
Compare
24a0712 to
95d85e8
Compare
johncwaters
left a comment
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
| import { skillProgramOptions } from './skill-program-options'; | ||
| import type { Command } from './command'; | ||
|
|
||
| export const doctorCommand: Command = { |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
This command isn't wired in yet. main.ts:40 and src/cli/__tests__/wizard.test.ts:5 still import doctorCommand from ./src/tools/doctor/report, which no longer exports it. src/programs/posthog-doctor/index.ts:3 still imports POSTHOG_DOCTOR_PROGRAM, which this PR renames to POSTHOG_DOCTOR_FLOW. src/tools/cli-steering/__tests__/cli-add.test.ts still mocks @ui against the old runCliAdd(argv) signature. I assume a later PR in the stack fixes these. Can you confirm which one?
| }, | ||
| handler: (argv) => { | ||
| // `list` is the positional's one reserved value, not a skill id. | ||
| if (readSkillName(argv) === 'list') return exitWith(listSkills); |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
nit: skill list used to finish by letting the event loop drain. Through exitWith it now calls process.exit(0) right after the process.stdout.write calls. On macOS, writes to a piped stdout are async, so a long listing piped to a slow reader could get cut off. Setting process.exitCode for this path, instead of exiting, would avoid that.
| ProjectDataProfile, | ||
| } from './mcp-project-profile'; | ||
| import copyData from './mcp-role-prompts.copy.json'; | ||
| import copyData from './mcp-role-prompts.copy'; |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
nit: mcp-role-prompts.copy.json is still next to the new .copy.ts, and nothing imports it now. Its data matches the TS module exactly, minus the Slack card, so anyone who edits the JSON will see no effect. Delete it here?
There was a problem hiding this comment.
Handled later in the stack: mcp-role-prompts.copy.json is deleted in #1386, leaving only the .copy.ts module.
| * Any failure counts, not just a total wipeout: the step installs to every | ||
| * detected client, so one succeeding would otherwise mask the rest. | ||
| */ | ||
| export async function addMCPServerToClientsStep( |
There was a problem hiding this comment.
Note
Automated review. Not written by a human.
nit: the description asks reviewers to focus on each runner's exit codes, but no test pins them. Nothing covers addMCPServerToClientsStep (1 on any failure or no clients), removeMCPServerFromClientsStep, runDoctorReport, runProvision or listSkills. They now take an injected log and resolve a number, so a small table test per runner is cheap and would stop the codes drifting later in the stack.
4f429c7 to
6086517
Compare
95d85e8 to
4be5b3f
Compare
6086517 to
3fe2559
Compare
4be5b3f to
1b7941f
Compare
Generated-By: PostHog Desktop Task-Id: d14e92bb-6ee1-49b5-8502-39cb80079589
1b7941f to
81ab207
Compare
3fe2559 to
216a0dd
Compare
edwinyjlim
left a comment
There was a problem hiding this comment.
Review of #1384 at 81ab2073. Items outside this PR's diff lines:
[P3] A tools-layer test still drives the CLI command
src/tools/cli-steering/__tests__/cli-add.test.ts:54
It imports cliAddCommand from @cli/commands/cli/add. The PR's own rule is that tools never import the CLI, and #1391's references make this TS6307. It compiles here only because the stack lands together.
Suggested fix: If reviewed in isolation, call runCliAdd directly with a sink stub.
Drafted with Claude Code from a review of the whole Release C stack, re-verified at the stack tip 2e31e438. Line numbers are at this PR's head.
| ); | ||
| return; | ||
| } | ||
| withSignals(async (signal) => { |
There was a problem hiding this comment.
[P2] wizard doctor in a non-TTY loses its error code and message
Same root cause as exitWith in #1383. doctor is the one tool command with no catch around runTuiTool, so a non-TTY run gets a bare PHW_INTERNAL_UNHANDLED line. slack.ts:36-48 and mcp/tutorial.ts:36-48 handle the same rejection; mcp/add.ts:70-73 falls back to the console runner.
Suggested fix: Prefer fixing exitWith. If it stays as is, give doctor.ts the slack.ts shape: catch, log the error, emit CliInteractiveRequired, return 1.
Phase 2 of Release C (#1320), PR 8 of 16, after #1383. Review PR. PRs 1 to 14 of this phase split one change into reviewable pieces and compile only as a stack: review them one by one, then land the whole phase together.
Problem
On main,
mcp add,mcp remove, the MCP tutorial, Slack and doctor are registered as programs, though none of them runsrunProgram.skill list,provisionandcli addlive inside their CLI commands and exit the process themselves.Changes
src/toolsis the tools layer, with one entry (src/tools/index.ts):TOOL_REGISTRY,getTool,ToolId, and the console runnersaddMCPServerToClientsStep,removeMCPServerFromClientsStep,runDoctorReport,runProvision,runCliAddandlistSkills. Each runner resolves an exit code.addMCPServerToClientsStepandremoveMCPServerFromClientsStepmove fromsrc/shared/mcp-clients/install.tstosrc/tools/mcp/console.ts.install.tskeeps only the per-client operations every MCP surface shares, and logs to the debug file instead of the UI.program_idtags keep their values.src/tui/tools/<id>(doctor,mcp,slack) holds itsflowand screens as aTuiTool, whichgetTuiToolfinds andrunTuiToolruns.exitWith.wizard skill listis now theskillcommand's reservedlistvalue.mcp-role-prompts.copy.ts, with the same text less the Slack card, which moved in refactor: move every file into its layer #1375.@tools.Test plan
pnpm typecheck0 errors,pnpm lint0 errors, 236 test files (3,515 tests) pass,pnpm buildok.Created with PostHog Desktop