feat: enrich install completion summary and stream agent file ops#186
feat: enrich install completion summary and stream agent file ops#186nicknisi wants to merge 1 commit into
Conversation
Phase 4 (Install UX) of the Akshay friction-log fixes. Completion summary: - Add CompletionData type + pure buildCompletionData builder (lockfile-aware dev command via resolveDevCommand, app URL via detectPort, changed files, composed next steps, per-framework docs + dashboard URLs). - New buildingCompletion machine state + actor computes the payload between postInstall and complete; errors degrade to the static fallback box. - renderCompletionSummary renders the structured success box (files capped at 5, next steps, per-framework docs footer); falls back to the static box when no completion data is present. Failure branch unchanged. - Widen the complete event with an optional completion payload; all three adapters (CLI, Dashboard, Headless) render/emit it. Headless spreads the fields only when present, preserving the existing NDJSON shape. Agent progress: - CLI adapter: remove the 2s spinner-message reset that clobbered phase text; persist file:write/file:edit as append-only step lines above the spinner (stop -> log -> restart), path-only with consecutive-path dedupe. - Headless adapter: stream path-only file:write/file:edit NDJSON (never content). - Stretch: surface Bash commands via a new agent:tool event emitted from the post-permission tool_use branch; rendered as step lines (CLI) and NDJSON (headless). - Stretch: optional UIConfig.getSignInSnippet surfaced as a next step; defined for Next.js (client-side refreshAuth guidance, per the AuthKit skill). Validated: pnpm build, pnpm test (2241), pnpm typecheck, oxlint, oxfmt all pass.
Greptile SummaryThis PR enriches install completion output and streams agent progress more directly. The main changes are:
Confidence Score: 4/5This PR needs a targeted redaction fix before merging. The structured completion flow and adapter wiring are covered, but the new command progress stream can expose sensitive command values.
 T-Rex LogsWhat T-Rex did
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (toolName === 'Bash' && input) { | ||
| const command = input.command as string; | ||
| if (command) { | ||
| emitter?.emit('agent:tool', { kind: 'command', detail: command }); |
There was a problem hiding this comment.
agent:tool emits the full Bash command into CLI/headless progress streams. When an allowed command contains an inline credential such as WORKOS_API_KEY=... pnpm ... or --password ..., that secret is logged verbatim by the adapters. Redact known sensitive assignment and flag values before emitting this event.
Rule Used: Do not log sensitive fields like access_token, ref... (source)
Artifacts
Repro: focused headless adapter harness
- Contains supporting evidence from the run (text/typescript; charset=utf-8).
Repro: command output showing verbatim secret exposure
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/agent-interface.ts
Line: 846-849
Comment:
**Redact surfaced commands**
`agent:tool` emits the full Bash command into CLI/headless progress streams. When an allowed command contains an inline credential such as `WORKOS_API_KEY=... pnpm ...` or `--password ...`, that secret is logged verbatim by the adapters. Redact known sensitive assignment and flag values before emitting this event.
**Rule Used:** Do not log sensitive fields like access_token, ref... ([source](https://app.greptile.com/workos/-/custom-context?memory=395edb09-f95b-4759-834f-94cb847721f5))
How can I resolve this? If you propose a fix, please make it concise.![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| private handleAgentTool = ({ detail }: InstallerEvents['agent:tool']): void => { | ||
| const cmd = detail.length > 80 ? `${detail.slice(0, 77)}…` : detail; | ||
| this.logAboveSpinner(() => clack.log.step(`Running ${chalk.dim(cmd)}`)); | ||
| }; |
There was a problem hiding this comment.
🟡 File operation notifications are silently suppressed when a tool call intervenes between two operations on the same file
The deduplication of file-operation notifications does not reset when a tool-call event fires (handleAgentTool at src/lib/adapters/cli-adapter.ts:413), so a write-to-A → tool-call → edit-of-A sequence suppresses the edit notification even though the operations are not truly consecutive.
Impact: Users miss that a file was edited after an intervening command, making the install log incomplete.
Mechanism: shared lastFileOp is never cleared by tool-call handlers
lastFileOp is checked in both handleFileWrite (src/lib/adapters/cli-adapter.ts:400) and handleFileEdit (src/lib/adapters/cli-adapter.ts:407), but handleAgentTool (src/lib/adapters/cli-adapter.ts:413) does not reset it. The same pattern exists in the headless adapter (src/lib/adapters/headless-adapter.ts:302). So the sequence:
file:write { path: 'src/auth.ts' }→ logged,lastFileOp = 'src/auth.ts'agent:tool { detail: 'pnpm add ...' }→ logged, butlastFileOpunchangedfile:edit { path: 'src/auth.ts' }→ silently dropped becausepath === lastFileOp
The comment at line 400 says "dedupe consecutive same-path ops", but the tool call breaks consecutiveness without resetting the dedup state.
| private handleAgentTool = ({ detail }: InstallerEvents['agent:tool']): void => { | |
| const cmd = detail.length > 80 ? `${detail.slice(0, 77)}…` : detail; | |
| this.logAboveSpinner(() => clack.log.step(`Running ${chalk.dim(cmd)}`)); | |
| }; | |
| private handleAgentTool = ({ detail }: InstallerEvents['agent:tool']): void => { | |
| this.lastFileOp = null; // tool call breaks the consecutive-file-op chain | |
| const cmd = detail.length > 80 ? `${detail.slice(0, 77)}…` : detail; | |
| this.logAboveSpinner(() => clack.log.step(`Running ${chalk.dim(cmd)}`)); | |
| }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| private handleAgentTool = ({ kind, detail }: InstallerEvents['agent:tool']): void => { | ||
| writeNDJSON({ type: 'agent:tool', kind, detail }); | ||
| }; |
There was a problem hiding this comment.
🟡 Headless adapter also fails to reset file-operation dedup state on tool calls
The headless adapter's handleAgentTool (src/lib/adapters/headless-adapter.ts:302) does not reset lastFileOp, so a file:write → agent:tool → file:edit sequence on the same path silently drops the NDJSON event for the edit.
Impact: CI/CD consumers of the NDJSON stream miss file-edit events that follow a tool call on the same path.
Same root cause as the CLI adapter
lastFileOp at src/lib/adapters/headless-adapter.ts:33 is shared across handleFileWrite (line 290) and handleFileEdit (line 296), but handleAgentTool (line 302) never resets it. An intervening tool call should break the dedup chain.
| private handleAgentTool = ({ kind, detail }: InstallerEvents['agent:tool']): void => { | |
| writeNDJSON({ type: 'agent:tool', kind, detail }); | |
| }; | |
| private handleAgentTool = ({ kind, detail }: InstallerEvents['agent:tool']): void => { | |
| this.lastFileOp = null; // tool call breaks the consecutive-file-op chain | |
| writeNDJSON({ type: 'agent:tool', kind, detail }); | |
| }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| private logAboveSpinner(render: () => void): void { | ||
| const wasRunning = this.spinner !== null; | ||
| this.spinner?.stop(); | ||
| this.spinner = null; | ||
| render(); | ||
| if (wasRunning) { | ||
| this.spinner = clack.spinner(); | ||
| this.spinner.start(this.lastAgentMessage); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔍 Spinner stopped without a message — inconsistent with all other call sites
Every other call to spinner.stop() in the CLI adapter passes an explicit message string (e.g. stopSpinner('Agent completed'), this.spinner.stop('Cancelled'), this.spinner.stop('Authenticated')). The new logAboveSpinner method at src/lib/adapters/cli-adapter.ts:390 calls this.spinner?.stop() without a message. Depending on clack's internal behavior, this may print the spinner's last message as a "completed" line (with a checkmark), creating a misleading visual artifact where the current phase (e.g. "Configuring middleware") appears completed before the spinner immediately restarts with the same message. The test mocks stop as a no-op so this visual behavior is untested. Consider passing an empty string or a neutral message to stop() to control the output.
Was this helpful? React with 👍 or 👎 to provide feedback.
| frameworkNextSteps: cfg?.ui.getOutroNextSteps?.({}) ?? [], | ||
| signInSnippet: cfg?.ui.getSignInSnippet?.({}), |
There was a problem hiding this comment.
🔍 Framework config functions called with empty context object
In src/lib/run-with-core.ts:373-374, getOutroNextSteps and getSignInSnippet are called with {} as the context argument. For the current Next.js integration this is safe because both functions ignore their parameter (() => [...] and () => '...'). However, other framework configs (e.g. Next.js's getOutroChanges) do use the context to extract router type via context.router. If a future framework's getOutroNextSteps or getSignInSnippet depends on context (e.g. to vary copy by router or platform), the empty object would silently produce incorrect output. The gatherContext result from the state machine is available in the machine context but is not threaded through to buildCompletion.
Was this helpful? React with 👍 or 👎 to provide feedback.
| private handleFileWrite = ({ path }: InstallerEvents['file:write']): void => { | ||
| if (path === this.lastFileOp) return; // dedupe consecutive same-path ops | ||
| this.lastFileOp = path; | ||
| const rel = relative(process.cwd(), path); | ||
| this.logAboveSpinner(() => clack.log.step(`Creating ${chalk.dim(rel)}`)); | ||
| }; | ||
|
|
||
| private handleFileEdit = ({ path }: InstallerEvents['file:edit']): void => { | ||
| if (path === this.lastFileOp) return; | ||
| this.lastFileOp = path; | ||
| const rel = relative(process.cwd(), path); | ||
| this.logAboveSpinner(() => clack.log.step(`Editing ${chalk.dim(rel)}`)); | ||
| }; |
There was a problem hiding this comment.
🔍 Cross-type file dedup: a write followed by an edit to the same path is suppressed
The lastFileOp field in both CLI (src/lib/adapters/cli-adapter.ts:45) and headless (src/lib/adapters/headless-adapter.ts:33) adapters is shared between handleFileWrite and handleFileEdit. This means a file:write to path A immediately followed by a file:edit to path A will suppress the edit notification. This is a plausible agent pattern (create a file, then immediately refine it). The tests only verify same-type dedup (edit→edit). Whether this cross-type suppression is intentional or an oversight depends on the desired UX — it reduces noise but hides a meaningful operation change (create vs. edit).
Was this helpful? React with 👍 or 👎 to provide feedback.
| ...(completion | ||
| ? { | ||
| integration: completion.integration, | ||
| devCommand: completion.devCommand, | ||
| url: completion.url, | ||
| files: completion.files, | ||
| nextSteps: completion.nextSteps, | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
🔍 Headless complete event spreads completion fields but omits docsUrl and dashboardUrl
In src/lib/adapters/headless-adapter.ts:398-406, the structured completion data is spread into the NDJSON complete event, but only integration, devCommand, url, files, and nextSteps are included. The docsUrl and dashboardUrl fields from CompletionData are omitted. The test at src/lib/adapters/headless-adapter.spec.ts:416-426 uses expect.objectContaining which doesn't catch missing fields. If NDJSON consumers need docs/dashboard URLs, they won't find them in the event. This may be intentional (keeping the NDJSON payload lean) but is worth confirming.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Closing in favor of a single combined PR from nicknisi/akshay. |
1 participant
What
buildCompletionDatabuilder assembles the lockfile-aware dev command, app URL with the detected port, changed files (capped at 5), composed next steps, and per-framework docs/dashboard URLs. It runs in a newbuildingCompletionstate betweenpostInstallandcomplete, and all three adapters (CLI, Dashboard, Headless) render it via a widened optionalcomplete.completionpayload with a graceful static fallback. Previously the success branch ofsummary-box.tsdiscarded the rich per-framework summary the installer already built and printed two static lines.setIntervalthat clobbered the AI-agent phase text, and subscribed the already-emittedfile:write/file:editevents as persistent step lines (CLI) and NDJSON events (headless), with dedupe.agent:toolevent surfaces Bash commands the agent runs (emitted from the post-permission branch ofhandleSDKMessage); a "add a sign-in link" snippet in the summary — Next.js only, since inventing route copy for unverified frameworks would violate accuracy.Why
From the friction log: after a successful install the user got two generic lines and no idea what to do next (dev command, URL, what changed), and during the agent phase a spinner overwrote its own progress text every 2 seconds while file-operation events went unconsumed.
Testing
completion-data.spec.ts; strengthenedsummary-box.spec.ts(replacing the bug-encoding title-only assertion) and adapter specs, including a fake-timers no-clobber case and NDJSONfile:write/file:editassertions that fail on pre-fix code.pnpm build && pnpm test && pnpm typecheck(plus oxlint/oxfmt) green.Stack 2/7 (
akshay-friction-log): merge after #185, then rebase this branch ontomain(squash-merge stack).