Repository navigation
refactor: extract @agent-device/command-output - #4120
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 81 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex SentinelVerdict: Since the last review: 3 new · 0 still open · 3 no longer raised 3 finding(s): 2 minor, 1 nit. Verdict: approve. reached max rounds (1). 🟡 Minor (2)
🔵 Nit (1)
Not in the reviewers' prompt (1 file)
Reply under an inline comment with |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 4 new findings on 23907a4. The verdict and the full list are in the summary comment.
|
I reviewed 23907a4 and found no code problems that need a change before merge. The extraction reads as formatting-only. Not blocking, and you can take or leave these: The open threads on the R86 named-import gap (#4120 (comment)), the settle command list that is not pinned to the trait (#4120 (comment)), and the ADR 0008 line 14 contradiction (#4120 (comment)) still apply. Three lower-priority threads also still apply: the sample-outputs comment, the duplicated params type, and the unexported CI is not settled. Smoke Tests is still running, and CodeQL failed with no excerpt I could read. The PR moves about 40 formatter files, so a CodeQL alert in a moved file may be an old one that now shows as new. Please check the alert and dismiss it if it is old, let Smoke Tests finish, then fix the threads that still apply. |
There was a problem hiding this comment.
Apex Sentinel: 3 new findings on 0e344e5. The verdict and the full list are in the summary comment.
|
The code at fd9a924 looks good. The earlier findings from #4120 (comment) are fixed, and nothing new came up in this pass. CI is green at fd9a924 (24 checks, none failing). I did not rerun check:affected or the vitest suites, so the delta-gate claim rests on the author's comment and CI. The PR now has merge conflicts with main (ff8c228). #4121 changed package.json, pnpm-lock.yaml, examples/sdk/tsconfig.json, scripts/layering/package-entry-surface.ts and src/commands/cli-runner.ts. Two of these need a manual merge, not a side pick: package-entry-surface.ts, where both PRs add a package entry pin, and cli-runner.ts, where both change imports. I did not try the merge myself. After the rebase, the R86 PROCESS_ENTRIES list and the layering ranks must also include @agent-device/client. Then rerun check:affected. No inline thread from other reviewers still applies at fd9a924. These are fixed in the code and already resolved: #4120 (comment) (process imports in the pure package), #4120 (comment) (settle command assertion), #4120 (comment) (ADR 0008 formatter ownership), #4120 (comment) and #4120 (comment) (interactiveOnly on -i snapshots), #4120 (comment) and #4120 (comment) (tests call formatCliOutput from the index), #4120 (comment) (layering rank), #4120 (comment) (conformance comment), #4120 (comment) (parseableOutput param type), #4120 (comment) (FormatCliOutputParams type), #4120 (comment) (ColorStream export), #4120 (comment) (FormatCliOutputParams export). Two are harmless and need no change: #4120 (comment) (doctorCliOutput uses the same stderrIsTTY boolean as the other formatters, so it only differs in signature style) and #4120 (comment) (PURE_PRESENTATION_ROOTS already covers the only presentation package, and a global stream ban would widen R86 beyond this extraction). |
Move the shared CLI and MCP command presentation (family output formatters, warning routing, color, human error lines, device and result serialization) out of src/commands into a private workspace package with one explicit entry. The command surface keeps declaration and projection and gains no presentation import. The package performs no process I/O. The purity split lands with the move: - formatHumanErrorLines returns the stderr lines; printHumanError stays a root writer in src/commands/output/human-error.ts, and printJson stays root. - supportsColor, the diff renderers and the doctor formatter take the terminal fact (stdout stream, stderrIsTTY) as input; the CLI and MCP root boundaries pass it from their process. - CommandProgressState belongs to the formatter owner; the stderr progress sink stays root. - responseWarningRoute derives the warning route from descriptor metadata, so the light entry needs no command-surface value import. The entry names its exports and loads the formatter tables (formatCliOutput), and the snapshot and screenshot diff renderers, on first call, so color, serialization and CLI startup users do not evaluate the formatter closure. MCP no longer evaluates it at startup either. Deep callers in scripts and tests render through the production formatCliOutput. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ages off process state Rank the command-output zone above the command surface, pin its single named entry, extend R86 so packages/command-output/src reads no process member (streams and TTY facts arrive as input), assert the entry evaluates no formatter table, snapshot or diff renderer, and keep the SDK and command surface entries free of the package. The provider import ban covers the new package root. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> # Conflicts: # scripts/layering/package-entry-surface.ts
… metadata generics Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…e client extraction Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
fd9a924 to
bc1c0de
Compare
There was a problem hiding this comment.
Apex Sentinel: 3 new findings on bc1c0de. The verdict and the full list are in the summary comment.
|
The code looks good at f4795df. The conflict from the earlier review (#4120 (comment)) is fixed, and the rebase keeps both entry pins and the client process-entry and rank. CI is green on all 24 checks, and there are no conflicts. I checked the conflict resolution through the diff against the new merge-base ff8c228 and the range-diff. I did not run check:affected, layering, or the vitest suites locally, so the gate results rest on CI. Not blocking, take or leave: No open thread still applies. These three do not apply and are already resolved: the one-argument |
thymikee
added
the
ready-for-human
Summary
Extracts the shared CLI/MCP presentation (family formatters, warning routing, color, human error lines, serialization) into private
@agent-device/command-output. Sources moved withgit mv.command-surfacegains no presentation import; MCP stays root. 82 files, +663/−277 rename-aware.processmember (R86, including named imports).printHumanError,printJsonand the stderr progress sink stay root; stdout/stderrIsTTYarrive as input.".", explicit names, includingFormatCliOutputParams.formatCliOutputand the diff renderers load the formatter tables on first call.formatCliOutput.-iconformance samples, named-import guard.ff8c2285c8(refactor: extract client workspace package #4121 client extraction). Conflicts were manifests, entry pins,cli-runner.tsandrender-fixtures.ts; both packages stay registered. Lockfile regenerated with pnpm (only the new importer).range-diff: the last four commits are unchanged; one added commit merges a duplicate import.Eager closure at the old base:
cli.ts256→256, SDK 241→241, MCP server 254→230, entry 19 (ceiling 20); the budget gate passed on the rebased head.Validation
Full gate at
bc1c0dece1:pnpm check --head bc1c0dece1passed,exactCommit: true(tooling, fallow, unit); merge-tree against current main is clean. CI then failed AgentGuidance (docs/agents 40,007 bytes > 40,000);f4795dfaa7shortens one sentence incode-ownership.md, no cap raised. Docs-only delta tof4795dfaa7:check:affected --base bc1c0dece1 --head f4795dfaa7selectedagent-guidance, passed,exactCommit: true; not a new full gate. Earlier CI passes apply to older heads. Not device-facing. CI-only lanes pending.🤖 Generated with Claude Code