From 3774ef5cdd911831be3d570c6bc29831a6ea8443 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:40:10 +0800 Subject: [PATCH 1/5] feat(coding-agent): make a feedback report investigable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A feedback report used to carry the user's sentence, a session id pointing at a file on their machine, and little else; the comment was the only thing a maintainer could read. Everything needed to reproduce a problem was already on disk but never reached the report. One submission now carries the evidence, with no per-item selection: picking what to attach produced reports that were just the user's own sentence. - `step.usage_provenance` records the dispatch facts a later config change would hide: API dialect, catalog context window and output limit, and, at settlement, the request duration, time to first token, wire outcome, HTTP status, who answered, and how far the model got. A request the user cancelled because it looked stuck is therefore reportable (`durationSeconds`, `ttftMs: null`, `reached: "thinking"`). - `session-summary.ts` derives the content-free summary from the session branch: model, effort, counts and failure classes, tokens, cost, cache hit rate, thinking time, per-compaction before/after sizes, plan/goal/task state, the last turn's tool names, and the newest request id. It walks `parentId` from the file's last entry, so abandoned turns and forks no longer distort the numbers; a pass no request followed yet is marked `unresolved` and left out of the size totals. - `evidence.ts` gathers the summary, the log excerpt and the archive from one session so they cannot describe different conversations, and keeps the ten-minute recency rule for a guessed session — which now also stays inside the current project's folder instead of picking the newest file anywhere. - The archive keeps the head and tail of an oversized transcript (with a marker between) and attaches the full tool output files the transcript referenced, resolved inside the session's own `tool-output` directory. - Redaction is one policy: credentials everywhere (scanned over the whole file, so an early secret also redacts its later echoes), the home directory collapsed to `~` in the transcript, tool output, dev log, `cwd` and provider error text, and absolute paths, e-mails and URLs removed from diagnostics. - Each archived transcript row keeps its entry and gains a `feedback` object with that row's own tokens, cache hit rate, cost, timing and request id. This is written to the bundle copy only; the session file is never rewritten, so the growth is +2.2% uncompressed and +1.2-1.8% gzipped, once, on submission. - The report header names the build it came from: `versionSource` (embedded, override, or the `0.1.0` fallback a source build reports), `buildTime`, `runtime` (node/electron/bun), arch, OS release, locale and install source. - `/feedback ` no longer bypasses the evidence path, and both hosts show the same consent page; with no terminal the submission prints what it attached. The archive is only built when a bundle endpoint can receive it. - `pending-store` re-validates a saved body strictly before retry, including the new summary, request and build fields and the nested records, and accepts a stored archive that carries `tool-output/` entries or a negative `reclaimed` — a field written but not allowlisted would have silently lost every offline report. Covered by new summary, row-metric and archive round-trip tests, plus updated CLI/TUI interaction tests. `docs/feedback-report.md` states what a report carries, its bounds, and the host integration contract. --- apps/cli/src/main.ts | 4 + docs/AGENTS.md | 1 + docs/feedback-report.md | 140 ++++ .../src/core/model-request-observer.ts | 20 +- .../src/core/request-provenance.ts | 29 + packages/coding-agent/src/core/sdk.ts | 48 +- .../coding-agent/src/step/build-identity.ts | 22 + .../src/step/feedback/build-env.ts | 2 + .../coding-agent/src/step/feedback/bundle.ts | 312 ++++++-- .../coding-agent/src/step/feedback/command.ts | 114 +-- .../coding-agent/src/step/feedback/consent.ts | 32 +- .../coding-agent/src/step/feedback/context.ts | 57 +- .../src/step/feedback/evidence.ts | 90 +++ .../coding-agent/src/step/feedback/index.ts | 2 + .../src/step/feedback/pending-store.ts | 381 +++++++++- .../src/step/feedback/redact-home.ts | 25 + .../src/step/feedback/row-metrics.ts | 242 ++++++ .../src/step/feedback/session-summary.ts | 690 ++++++++++++++++++ .../src/step/feedback/submission.ts | 23 +- .../coding-agent/src/step/feedback/types.ts | 52 ++ .../coding-agent/src/step/slash-commands.ts | 129 +++- .../test/feedback-command.test.ts | 52 +- .../test/feedback-context.test.ts | 47 ++ .../test/feedback-pending-body.test.ts | 77 ++ .../test/feedback-row-metrics.test.ts | 163 +++++ .../test/feedback-summary.test.ts | 537 ++++++++++++++ packages/coding-agent/test/feedback.test.ts | 61 ++ .../test/step-slash-commands.test.ts | 91 +-- scripts/build-coding-agent-bundle.mjs | 4 + 29 files changed, 3191 insertions(+), 256 deletions(-) create mode 100644 docs/feedback-report.md create mode 100644 packages/coding-agent/src/step/feedback/evidence.ts create mode 100644 packages/coding-agent/src/step/feedback/redact-home.ts create mode 100644 packages/coding-agent/src/step/feedback/row-metrics.ts create mode 100644 packages/coding-agent/src/step/feedback/session-summary.ts create mode 100644 packages/coding-agent/test/feedback-row-metrics.test.ts create mode 100644 packages/coding-agent/test/feedback-summary.test.ts diff --git a/apps/cli/src/main.ts b/apps/cli/src/main.ts index d3dc737b9..e15901b62 100644 --- a/apps/cli/src/main.ts +++ b/apps/cli/src/main.ts @@ -674,10 +674,14 @@ try { const feedbackSettingsManager = currentStepSettingsManager ?? createStepSettingsManager(process.cwd(), resolveStepAgentDir()); const feedbackIdentity = readCurrentFeedbackIdentity(); + const permissionPreset = feedbackSettingsManager.getStepPermissionPreset(); const exitCode = await runFeedbackCommand(process.argv.slice(3), { storageRootDir: resolveStepStorageRoot(process.env), ...(feedbackIdentity.uid ? { uid: feedbackIdentity.uid } : {}), ...(feedbackIdentity.username ? { username: feedbackIdentity.username } : {}), + // The session file does not record the permission tier; a blocked + // command is the first thing an investigator checks. + runtime: permissionPreset ? { permissionPreset } : {}, telemetry: telemetryReporter, settings: feedbackSettingsManager, env: process.env, diff --git a/docs/AGENTS.md b/docs/AGENTS.md index 495ce6b8c..1929d786c 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -7,6 +7,7 @@ Inherits the [root rules](../AGENTS.md). This directory maintains repository-wid - `development.md`, `testing.md`: coding conventions, collaboration, failure handling, and validation selection. - `step-configuration.md`, `step-unified-config-and-mcp.md`: configuration, directories, trust, and MCP startup / write contracts. - `command-permissions.md`: command analysis and product approvals. `orchestration-lifecycle.md`, `goal-lifecycle.md`: orchestration and goal lifecycles. +- `feedback-report.md`: what a user feedback report carries, its bounds and redaction, and the host integration contract. - `tui-rendering-pipeline.md`, `step-goal-ui.md`: CLI UI and terminal rendering integration. - `open-source-status.md`, `THIRD_PARTY_PROVENANCE.md`: public-source boundaries and third-party provenance. - Other topic / research documents must state their scope in the title and text; they do not automatically define current implementation rules. diff --git a/docs/feedback-report.md b/docs/feedback-report.md new file mode 100644 index 000000000..fb4607918 --- /dev/null +++ b/docs/feedback-report.md @@ -0,0 +1,140 @@ +# Feedback report + +A feedback report is a user-initiated bug report that has to be investigable without the user's screen. It carries three things: what the user said, the conversation it is about, and enough state to read that conversation. One implementation lives in `packages/coding-agent/src/step/feedback/`; every host (`step feedback`, `/feedback` in the TUI, and a host serving the same session over RPC) calls it. + +## Report contents + +Attached by default, no per-item choice: + +| Part | What it is | Bound | +| --- | --- | --- | +| `feedbackId`, `at`, `category`, `comment` | The report itself | comment ≤ 4000 characters | +| `context` | `channel`, `version`, `versionSource`, `buildTime`, `commit`, `runtime` (node/electron/bun), `platform`, `arch`, `osRelease`, `locale`, `installSource`, `sessionId`, `deviceId`, `uid`, `username`; `surface` is top-level | each value ≤ 256 characters | +| `request` | The newest model request: `requestId`, `provider`, `model`, `reasoning`, `sourceLeafId` | — | +| `sessionSummary` | Counts and enums only: model (provider/id/api), effort, turns, tool calls and failures, aborted and truncated attempts, model-request count, compactions with before/after sizes, tokens and cost, cache hit rate, thinking time, `cwd` (home collapsed), plan/goal/task state, last turn's tool names, recent error kinds, and live host facts (permission tier, context used/window, tool/skill counts, MCP server names) | bounded per field (≤ 50 tool rows × 256 chars, ≤ 20 compactions and errors) | +| `diagnostics` | Error context lines from the host's daily dev log | ≤ 40 lines × 512 characters, ≤ 16 KiB | +| session archive | `events.jsonl` (each row annotated with its own metrics, see below), the full output files the session referenced (`tool-output/`), and the log tail, gzipped | ≤ 8 MiB compressed; ≤ 24 MiB transcript; ≤ 10 output files, 2 MiB each | + +`context.versionSource` says where the version came from: `embedded` (release pipeline), `override` (local env) or `fallback` (a source build with nothing injected). A `fallback` version is the product default (`0.1.0`), not a release, so a report from a source build is never mistaken for a shipped one. `context.runtime` names the interpreter — Node, Electron or Bun — which is what a native-module or packaged-app report starts from. `buildTime` is present only when the pipeline embedded it. + +`request` is the join key and the dispatch record: `requestId`, provider, model, API dialect, reasoning effort, source leaf, the catalog `contextWindow`/`maxTokens` in force, who answered (`responseProvider`/`responseModel` when auth or a proxy redirected it), HTTP status and duration. It is read from `step.usage_provenance`, whose coverage depends on the usage-recording extension being mounted by the host; a session without it simply omits `request` and its `modelRequests` count stays 0. + +`sessionSummary.compactions` is one row per compaction, oldest first: `tokensBefore` as the kernel recorded it, `tokensAfter` as the input size of the first request that followed. A long session that compacted ten times has ten rows (the newest twenty keep their detail, `counts.compactions` counts all of them). A pass that did not shrink the window shows two close numbers; a pass no request followed yet carries `unresolved: true`. + +`compactionTotals` counts every pass in `passes` but sums sizes only over passes a request followed (`resolved`), so `tokensBefore - tokensAfter === reclaimed` always holds. + +Numbers describe the **branch the session file ends on**, not every line in it: navigating back or forking leaves abandoned entries in the same file, and counting them would report turns the user rewound and a request id from another branch. + +`sessionSummary` and the archive describe the **same** session: `resolveFeedbackSession` picks it once and both read that file. + +### Per-row metrics + +Each line of the bundle's `events.jsonl` keeps the entry it always had and gains a `feedback` object describing that row alone: + +```jsonc +{ "id": "da49b9c4", "type": "message", "message": { "role": "assistant", "...": "..." }, + "feedback": { + "turn": 1, + "tokens": { "input": 14854, "output": 1752, "cacheRead": 1152, "cacheWrite": 0, "total": 17758 }, + "cacheHitRate": 0.072, // this request only: cacheRead / (input + cacheRead) + "costUsd": 0.004, + "model": "step-5-preview", "provider": "step", "stopReason": "stop", + "requestId": "2be772f5-…", // joins the server-side request log + "durationMs": 4200, + "ttftMs": 640, // null = no stream event ever arrived + "outcome": "ok", + "thinkingMs": 11800, + "reached": "thinking" // only on a cancelled or failed attempt + } } +``` + +A tool result row gets `elapsedMs` (call issued → result recorded) only when the batch had a single call; a parallel batch shares one start, so a per-call duration would be invented. Rows with nothing to add keep their exact original bytes. + +Sources: the token figures and cache reads are the message's own `usage`, which the session already records per assistant message — the bundle copy just derives the rate and puts it on the row. Timing, `requestId` and `outcome` are joined from the `step.usage_provenance` entry written directly after that message (`usageEntryId`); thinking time from `stepcode-web.thinking-timing`. The join runs in two passes because those entries follow the message they describe. + +**Size.** Only the bundle copy is annotated; the session file on disk is never rewritten. Measured on real sessions: +2.2% uncompressed, +1.2–1.8% gzipped. The transcript bound (24 MiB) applies to the pre-annotation excerpt, and the annotated copy is re-bounded if it would exceed it. + +### Which session is attached + +The conversation a report describes is chosen once, and both the summary and the archive read that same file. + +- `--session ` or a host-supplied session file/id: used as given. +- Nothing named: the newest session **of the current project's own session folder**, and only when it was active within the last ten minutes. A guess never reaches into another project's folder and never attaches an old conversation: the wrong conversation is a privacy incident, not a degraded report. + +The rule also decides the summary: a guessed session outside the window contributes neither a transcript nor a session id, so a report is never about a conversation the user was not just using. + +### Transcript excerpt + +A session larger than the transcript bound keeps its **head and tail**, cut at line boundaries, with a marker line where the middle was removed. A tail-only excerpt lost the request that started the session; a head-only excerpt lost the turn that broke. + +### Full tool output + +When a tool result was larger than the model-visible limit, the tool saved the full text under `/tool-output/tool-.txt`. The transcript keeps a preview plus that path, which does not exist on the receiving machine. The report attaches the files the transcript actually names, resolved inside the session's own `tool-output` directory — names from the transcript are untrusted input and never used as arbitrary paths. + +### What a report answers + +| Question | Field | +| --- | --- | +| Which version, host, platform, install kind | `context.*` | +| Which model, provider, API dialect, effort | `sessionSummary.model`, `thinkingLevel`, `request.api` | +| What the model's window was, and how full it got | `request.contextWindow`/`maxTokens`, `runtime.contextWindow`/`contextUsed` | +| Did compaction run, from what to what | `sessionSummary.compactions[].tokensBefore` / `tokensAfter` | +| How much was spent, and was the cache working | `usage.*`, `usage.costUsd`, `usage.cacheHitRate` | +| Did the model hit its output limit or get stopped | `counts.truncatedAttempts`, `counts.abortedAttempts`, `counts.failedAttempts` | +| Which request to look up server-side | `request.requestId` | +| What was blocked by permissions | `runtime.permissionPreset` | +| Did an MCP server or skill take part | `runtime.toolCount`, and `runtime.mcpServerNames` / `runtime.skillCount` when the host supplies them | +| Which tool failed, and on which turn | `recentErrors`, `lastTurnTools`, `toolResult` in `events.jsonl` | +| Why the provider rejected a request | `recentErrors[].message` (status + response body), `errorKind` | +| Did it hang, and did the user cancel it | `recentErrors[]` with `kind: "aborted"`: `durationMs`, `ttftMs` (absent = no token ever arrived), `reached` | + +Still not collected: the assembled system prompt, the per-request retry log, and per-request token history (totals, the latest request, and compaction deltas only). Add them only with a concrete investigation that needs them. + +### Cancelled and failed attempts + +A user often cancels because the request looked stuck. Three values make that diagnosable without the user explaining it: + +- `durationMs` — how long it ran before settling; +- `ttftMs` — time to the first stream event, `null` when nothing ever arrived (a request that never started returning looks different from a slow one); +- `reached` — `thinking`, `text` or `toolCall`: how far the model got, so "cancelled while thinking" is distinguishable from "cancelled mid-answer". + +The timing comes from `step.usage_provenance`, the entry written directly after the assistant message; the summary joins the two by entry id. Reports from sessions written before those fields existed carry the cancellation and `reached` but no timing. + +## Redaction + +| Applied to | Rule | +| --- | --- | +| Every archived file, the comment, diagnostics | Credentials are replaced with ``; the scan covers the whole file, so a secret written early also redacts its later echoes | +| Transcript, tool output, `cwd` | The home directory is written as `~`, so paths keep their structure without carrying the OS user name | +| Diagnostics | Also redacts absolute paths, bare emails and URLs | + +A file that cannot be scanned safely (a line too long to scan, an unreadable or symlinked file) is **omitted**, never sent unredacted. + +Never collected: credentials (`auth.json`, tokens, cookies), other sessions, workspace source files, environment variables, hostnames, shell history. + +## Consent + +The user sees one review page with the category, their comment, the identity fields sent with the report, and a file list with sizes and truncation notes. It is one decision: send, or cancel. There is no per-item selection — a report assembled by choosing what to include was usually just the user's own sentence, which cannot be investigated. + +`/feedback ` submits the same evidence behind the same review page; the comment comes from the command and the category is recorded as `other`. When there is no terminal to show the page on (`--json`, a scripted caller), the submission prints one line naming what it attached, so a non-interactive report is never silent about the transcript. `--no-session-bundle` sends the report without it. With a terminal, a comment supplied on the command line skips the questions but still shows the consent page. + +`feedbackEnabled = false` in `step config.toml` (`[feedback] enabled`) or `STEPCODE_DISABLE_FEEDBACK=1` disables the flow entirely; the entry point reports why. + +## Delivery and retry + +Two requests: the JSON report to the feedback endpoint, then the gzip archive to the bundle endpoint with `?feedbackId=`. A failed body is written under `/feedback/` and re-sent by `step feedback --retry`; the archive is saved beside it and uploaded after the body succeeds. A pending report is reported as pending, never as submitted. + +A saved body is re-read strictly before retry: unknown fields, unsafe strings or mismatched ids make it invalid rather than sending something that changed on disk. + +## Opt-outs for scripted callers + +`--no-diagnostics` and `--no-session-bundle` skip those parts. Both are for automation; the guided flows never ask. The content-free `sessionSummary` is always attached. + +## Adding a host + +A host supplies a session file or id and an identity, then calls: + +- `prepareFeedbackEvidence` — resolves the session once and gathers the summary, diagnostics and archive. +- `submitFeedback` — validates, builds the report body, confirms (if the host wants a review page), delivers and records telemetry. + +A host must not assemble its own payload: the surface, the attached evidence, and the redaction are decided here. diff --git a/packages/coding-agent/src/core/model-request-observer.ts b/packages/coding-agent/src/core/model-request-observer.ts index 99719fef3..5641aff54 100644 --- a/packages/coding-agent/src/core/model-request-observer.ts +++ b/packages/coding-agent/src/core/model-request-observer.ts @@ -68,6 +68,14 @@ export interface ModelRequestObserver { export interface ObserveModelRequestStreamOptions { /** Local, best-effort terminal hook; independent of optional remote observers. */ readonly onTerminal?: (message: AssistantMessage) => void; + /** + * Local, best-effort completion hook with the timing the observer computed. + * + * Runs after {@link onTerminal} so a caller recording provenance on the + * message can add time-to-first-token and the wire outcome. It is offered + * independently of a remote observer, which may not be mounted. + */ + readonly onCompleted?: (message: AssistantMessage, completion: ModelRequestCompleted) => void; /** Timestamp captured immediately before the provider stream is requested. */ readonly startedAt?: number; /** Session identity forwarded to the observer, when available. */ @@ -142,7 +150,7 @@ export function observeModelRequestStream( observer: ModelRequestObserver | undefined, options: ObserveModelRequestStreamOptions = {}, ): AssistantMessageEventStream { - if (!observer && !options.onTerminal) return source; + if (!observer && !options.onTerminal && !options.onCompleted) return source; const startedAt = options.startedAt ?? Date.now(); const started = createStartedRequest(model, options, startedAt); @@ -166,7 +174,6 @@ export function observeModelRequestStream( } catch { /* Local diagnostics cannot change streaming. */ } - if (!observer) return; const completedAt = Date.now(); const statusCode = readStatusCode(options.getStatusCode); const effectiveModel = readModel(model, options.getModel); @@ -187,7 +194,14 @@ export function observeModelRequestStream( usage: copyUsage(message.usage), ...(unexpectedError ? { errorType: readErrorType(unexpectedError) } : undefined), }; - notifyCompleted(observer, completion); + if (options.onCompleted) { + try { + options.onCompleted(message, completion); + } catch { + /* Local diagnostics cannot change streaming. */ + } + } + if (observer) notifyCompleted(observer, completion); }; void (async () => { diff --git a/packages/coding-agent/src/core/request-provenance.ts b/packages/coding-agent/src/core/request-provenance.ts index 64dea0fa8..1a266ccb0 100644 --- a/packages/coding-agent/src/core/request-provenance.ts +++ b/packages/coding-agent/src/core/request-provenance.ts @@ -1,5 +1,13 @@ import type { AssistantMessage } from "@step-harness/providers/compat"; +/** + * What the kernel observed about one model request. + * + * Persisted as `step.usage_provenance`, which makes a feedback report joinable + * with the server-side request log. The optional members were added after the + * first version: a session written by an older build simply lacks them, so + * every reader must treat them as unknown rather than missing-but-expected. + */ export interface RequestUsageProvenance { requestId: string; sessionId: string; @@ -7,6 +15,27 @@ export interface RequestUsageProvenance { reasoningAtDispatch: string; requestedProvider: string; requestedModel: string; + /** API dialect the request used; a wrong-looking reply usually starts here. */ + api?: string; + /** Catalog limits at dispatch, which a later config change would hide. */ + contextWindow?: number; + maxTokens?: number; + /** Provider that actually handled the request (auth or proxy may redirect it). */ + responseProvider?: string; + responseModel?: string; + /** HTTP status of the attempt; 0 or absent when the request never got one. */ + statusCode?: number; + /** Wall-clock duration of the attempt. */ + durationMs?: number; + /** Time to the first stream event. Absent means no event ever arrived. */ + ttftMs?: number | null; + /** `ok`, or which side of the wire failed; a user stop is a transport outcome. */ + outcome?: "ok" | "http_error" | "transport_error"; + /** + * How far the attempt got before it settled. An attempt the user stopped + * while the model was still thinking has only `thinking`. + */ + reached?: "thinking" | "text" | "toolCall"; } const provenance = new WeakMap(); export function rememberRequestUsage(message: AssistantMessage, value: RequestUsageProvenance): void { diff --git a/packages/coding-agent/src/core/sdk.ts b/packages/coding-agent/src/core/sdk.ts index af5c39c06..57233cef8 100644 --- a/packages/coding-agent/src/core/sdk.ts +++ b/packages/coding-agent/src/core/sdk.ts @@ -39,6 +39,17 @@ import { withFileMutationQueue, } from "./tools/index.ts"; +/** The furthest kind of block the attempt produced, for reporting a user stop. */ +function reachedOf(message: { content?: readonly { type?: string }[] }): "thinking" | "text" | "toolCall" | undefined { + let reached: "thinking" | "text" | "toolCall" | undefined; + for (const block of message.content ?? []) { + if (block.type === "toolCall") return "toolCall"; + if (block.type === "text") reached = "text"; + else if (block.type === "thinking" && reached === undefined) reached = "thinking"; + } + return reached; +} + // Preserve the pre-0.81 fallback for extensions that construct Agent instances // or invoke low-level agent loops without supplying streamFn. Agent core remains // provider-agnostic and does not import pi-ai/compat itself. @@ -343,6 +354,11 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} reasoningAtDispatch: options?.reasoning ?? "off", requestedProvider: model.provider, requestedModel: model.id, + // Dispatch-time facts: a later model or config change must not + // rewrite what this request actually ran with. + api: model.api, + ...(model.contextWindow > 0 ? { contextWindow: model.contextWindow } : {}), + ...(model.maxTokens > 0 ? { maxTokens: model.maxTokens } : {}), }; const startedAt = Date.now(); let statusCode = 0; @@ -366,15 +382,14 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} websocketConnectTimeoutMs, maxRetries: options?.maxRetries ?? providerRetrySettings.maxRetries, maxRetryDelayMs: options?.maxRetryDelayMs ?? providerRetrySettings.maxRetryDelayMs, - ...(modelRequestObserver || options?.onResponse - ? { - onResponse: async (response, resolvedModel: Model) => { - statusCode = response.status; - if (modelRequestObserver) responseModel = resolvedModel; - await options?.onResponse?.(response, resolvedModel); - }, - } - : undefined), + // Always installed: the recorded provenance needs the status code and + // the model that actually answered, whether or not a telemetry + // observer is mounted (public builds mount none). + onResponse: async (response, resolvedModel: Model) => { + statusCode = response.status; + responseModel = resolvedModel; + await options?.onResponse?.(response, resolvedModel); + }, transformHeaders: async (requestHeaders) => { const headers = mergeProviderAttributionHeaders(model, options?.sessionId, requestHeaders); return headerRunner?.hasHandlers("before_provider_headers") @@ -394,7 +409,20 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} throw error; } return observeModelRequestStream(model, stream, modelRequestObserver, { - onTerminal: (message) => rememberRequestUsage(message, usageProvenance), + // A user stop during a slow or stuck request is a report of its own: + // the timings say whether a token ever arrived. + onCompleted: (message, completion) => + rememberRequestUsage(message, { + ...usageProvenance, + durationMs: completion.durationMs, + ttftMs: completion.ttftMs, + outcome: completion.outcome, + reached: reachedOf(message), + ...(completion.statusCode > 0 ? { statusCode: completion.statusCode } : {}), + // Auth or a proxy can redirect the request; record who answered. + ...(completion.provider !== model.provider ? { responseProvider: completion.provider } : {}), + ...(completion.model !== model.id ? { responseModel: completion.model } : {}), + }), startedAt, sessionId: options?.sessionId, getStatusCode: () => statusCode, diff --git a/packages/coding-agent/src/step/build-identity.ts b/packages/coding-agent/src/step/build-identity.ts index 1634528a0..9db801853 100644 --- a/packages/coding-agent/src/step/build-identity.ts +++ b/packages/coding-agent/src/step/build-identity.ts @@ -12,6 +12,8 @@ export type StepBuildChannel = "dev" | "release"; export interface StepBuildIdentity { readonly channel: StepBuildChannel; readonly commit?: string; + /** ISO timestamp the artifact was built, when the release pipeline embedded one. */ + readonly buildTime?: string; } const MAX_COMMIT_LENGTH = 40; @@ -28,6 +30,8 @@ const INLINED_CLI_BUILD_CHANNEL = process.env.STEPCODE_BUILD_CHANNEL; const INLINED_LEGACY_BUILD_CHANNEL = process.env.STEP_HARNESS_BUILD_CHANNEL; const INLINED_CLI_BUILD_COMMIT = process.env.STEPCODE_BUILD_COMMIT; const INLINED_LEGACY_BUILD_COMMIT = process.env.STEP_HARNESS_BUILD_COMMIT; +const INLINED_CLI_BUILD_TIME = process.env.STEPCODE_BUILD_TIME; +const INLINED_LEGACY_BUILD_TIME = process.env.STEP_HARNESS_BUILD_TIME; /** Resolve the channel and commit using one precedence rule for both domains. */ export function readStepBuildIdentity(env: NodeJS.ProcessEnv = process.env): StepBuildIdentity { @@ -46,10 +50,18 @@ export function readStepBuildIdentity(env: NodeJS.ProcessEnv = process.env): Ste useEmbeddedValues ? INLINED_CLI_BUILD_COMMIT : undefined, useEmbeddedValues ? INLINED_LEGACY_BUILD_COMMIT : undefined, ); + const buildTimeValue = firstNonBlank( + env.STEPCODE_BUILD_TIME, + env.STEP_HARNESS_BUILD_TIME, + useEmbeddedValues ? INLINED_CLI_BUILD_TIME : undefined, + useEmbeddedValues ? INLINED_LEGACY_BUILD_TIME : undefined, + ); const commit = normalizeStepCommit(commitValue); + const buildTime = normalizeStepBuildTime(buildTimeValue); return { channel: channelValue === "release" ? "release" : "dev", ...(commit ? { commit } : {}), + ...(buildTime ? { buildTime } : {}), }; } @@ -58,6 +70,16 @@ export function normalizeStepCommit(value: unknown): string | undefined { return normalizeBoundedPrintable(value, MAX_COMMIT_LENGTH); } +/** + * Only accept a parseable ISO timestamp. A malformed value would make every + * report look like it came from a different build, so it is dropped instead. + */ +export function normalizeStepBuildTime(value: unknown): string | undefined { + const bounded = normalizeBoundedPrintable(value, 40); + if (!bounded || !Number.isFinite(Date.parse(bounded))) return undefined; + return new Date(bounded).toISOString(); +} + /** Keep an opaque install id inside the collector's VARCHAR(64) boundary. */ export function normalizeStepDeviceId(value: unknown): string | undefined { return normalizeBoundedPrintable(value, MAX_DEVICE_ID_LENGTH); diff --git a/packages/coding-agent/src/step/feedback/build-env.ts b/packages/coding-agent/src/step/feedback/build-env.ts index 0b9a0e29b..5d9c80e13 100644 --- a/packages/coding-agent/src/step/feedback/build-env.ts +++ b/packages/coding-agent/src/step/feedback/build-env.ts @@ -5,6 +5,8 @@ export type FeedbackBuildChannel = "dev" | "release"; export interface FeedbackBuildEnv { channel: FeedbackBuildChannel; commit?: string; + /** ISO timestamp of the artifact, when the release pipeline embedded one. */ + buildTime?: string; } export function readFeedbackBuildEnv(env: NodeJS.ProcessEnv = process.env): FeedbackBuildEnv { diff --git a/packages/coding-agent/src/step/feedback/bundle.ts b/packages/coding-agent/src/step/feedback/bundle.ts index 806012448..2a3fa1958 100644 --- a/packages/coding-agent/src/step/feedback/bundle.ts +++ b/packages/coding-agent/src/step/feedback/bundle.ts @@ -7,6 +7,8 @@ import { normalizeStepWireSessionId } from "../build-identity.ts"; import { resolveStepSessionDir } from "../environment.ts"; import { createSecretRedactionCollector, redactSecretString } from "../secret-redaction.ts"; import { resolveStderrDevLogPath } from "../stderr-dev-log.ts"; +import { collapseHomePaths } from "./redact-home.ts"; +import { annotateTranscriptRows } from "./row-metrics.ts"; import type { FeedbackBundle } from "./types.ts"; import { FEEDBACK_BUNDLE_MAX_BYTES, FEEDBACK_SESSION_RECENCY_WINDOW_MS } from "./types.ts"; import { sanitizeTerminalText } from "./validate.ts"; @@ -24,6 +26,12 @@ const DEV_LOG_MAX_LINES = 2000; const DEV_LOG_MAX_BYTES = 512 * 1024; const SESSION_HEADER_READ_CHUNK_BYTES = 4 * 1024; const SESSION_HEADER_MAX_BYTES = 1024 * 1024; +/** Full tool output a tool call referenced: these are the files the model was shown a preview of. */ +const TOOL_OUTPUT_DIR = "tool-output"; +const TOOL_OUTPUT_NAME = /^tool-[0-9a-f]{32}\.txt$/u; +const TOOL_OUTPUT_MAX_FILES = 10; +const TOOL_OUTPUT_MAX_FILE_BYTES = 2 * 1024 * 1024; +const TOOL_OUTPUT_MAX_TOTAL_BYTES = 8 * 1024 * 1024; export type FeedbackBundleResult = | { status: "ready"; bundle: FeedbackBundle } @@ -35,12 +43,35 @@ interface BundleEntry { note?: string; } -interface SelectedSession { +export interface SelectedSession { filePath: string; sessionId: string; } -type SessionEntryResult = { status: "ready"; entry: BundleEntry } | { status: "missing" } | { status: "unsafe" }; +type SessionEntryResult = + | { status: "ready"; entry: BundleEntry; text: string } + | { status: "missing" } + | { status: "unsafe" }; + +/** + * The session a report is about, as a file on disk. + * + * Exported so the summary and the bundle resolve the same session: two + * independent guesses could describe one conversation and attach another. + */ +export async function resolveFeedbackSession(input: { + sessionFile?: string; + sessionId?: string; + sessionDir?: string; + env?: NodeJS.ProcessEnv; + /** Project the user is filing from; a guess never leaves this folder. */ + cwd?: string; +}): Promise { + if (input.sessionFile) return resolveExplicitSessionFile(input.sessionFile, input.sessionId); + const sessionRoot = path.resolve(input.sessionDir ?? resolveStepSessionDir(input.env)); + if (input.sessionId) return findSessionFile(sessionRoot, input.sessionId); + return findNewestSessionFile(sessionRoot, input.cwd ?? process.cwd()); +} export async function buildFeedbackSessionBundle(input: { sessionFile?: string; @@ -50,14 +81,18 @@ export async function buildFeedbackSessionBundle(input: { env?: NodeJS.ProcessEnv; at?: Date; now?: Date; + /** Project the report is filed from; a guessed session never leaves it. */ + cwd?: string; + /** + * The caller resolved the session itself because the user named none. The + * recency rule still applies: a conversation nobody used recently is not + * what the report is about, and attaching the wrong one is a privacy + * incident rather than a degraded report. + */ + guessedSession?: boolean; }): Promise { - const sessionRoot = path.resolve(input.sessionDir ?? resolveStepSessionDir(input.env)); - const guessed = !input.sessionFile && !input.sessionId; - const selectedSession = input.sessionFile - ? await resolveExplicitSessionFile(input.sessionFile, input.sessionId) - : input.sessionId - ? await findSessionFile(sessionRoot, input.sessionId) - : await findNewestSessionFile(sessionRoot); + const guessed = input.guessedSession ?? (!input.sessionFile && !input.sessionId); + const selectedSession = await resolveFeedbackSession(input); if (!selectedSession) return { status: "skipped", reason: "no-session" }; const { filePath: sessionFile, sessionId } = selectedSession; @@ -78,20 +113,26 @@ export async function buildFeedbackSessionBundle(input: { if (eventsResult.status === "missing") return { status: "skipped", reason: "no-session" }; if (eventsResult.status === "unsafe") return { status: "skipped", reason: "unsafe" }; let events = eventsResult.entry; + // A tool call's full output is the one thing the model itself only saw a + // preview of, and the preview names a path that does not exist on the + // receiver's machine. Attach the files this session referenced. + let toolOutputs = await readToolOutputEntries(sessionFile, eventsResult.text); + const entries = (): BundleEntry[] => [events, ...toolOutputs, ...(devLog ? [devLog] : [])]; let archive = await compressArchive({ at, sessionId, lastActivityAt: stats.mtime, - entries: [events, ...(devLog ? [devLog] : [])], + entries: entries(), }); if (archive.byteLength > FEEDBACK_BUNDLE_MAX_BYTES) { events = limitRedactedSessionEntry(events, EVENTS_FALLBACK_BYTES); + toolOutputs = toolOutputs.slice(0, 3).map((entry) => limitRedactedSessionEntry(entry, 512 * 1024)); archive = await compressArchive({ at, sessionId, lastActivityAt: stats.mtime, - entries: [events, ...(devLog ? [devLog] : [])], + entries: entries(), }); } if (archive.byteLength > FEEDBACK_BUNDLE_MAX_BYTES) return { status: "skipped", reason: "too-large" }; @@ -100,7 +141,7 @@ export async function buildFeedbackSessionBundle(input: { status: "ready", bundle: { data: archive, - files: describeEntries([events, ...(devLog ? [devLog] : [])]), + files: describeEntries(entries()), sessionId, lastActivityAt: stats.mtime, }, @@ -112,7 +153,9 @@ export function describeFeedbackBundle(bundle: FeedbackBundle): string { return [ ...bundle.files.map((file) => `${file.name} (${file.bytes} bytes)${file.note ? ` - ${file.note}` : ""}`), `session ${bundle.sessionId}, last active ${bundle.lastActivityAt.toISOString()}.`, - "It contains the conversation itself: your prompts, the model's replies, tool calls and their output.", + "It contains the conversation itself: your prompts, the model's replies, tool calls and their output,", + "the full output files those calls referenced, and the tail of the host log. Credentials are removed", + "and your home directory is written as ~.", ].join("\n"); } @@ -151,54 +194,148 @@ function describeEntries(entries: readonly BundleEntry[]): Array<{ name: string; } async function readSessionEntry(filePath: string, maxBytes: number): Promise { - const entry = await readTailEntry(filePath, EVENTS_ENTRY, maxBytes); - if (!entry) return { status: "missing" }; + const linkStats = await fs.lstat(filePath).catch(() => undefined); + if (!linkStats?.isFile()) return { status: "unsafe" }; + const handle = await fs.open(filePath, "r").catch(() => undefined); + if (!handle) return { status: "unsafe" }; try { - const target = entry.content.toString("utf8"); - const collector = createSecretRedactionCollector(target); - const linkStats = await fs.lstat(filePath).catch(() => undefined); - if (!linkStats?.isFile()) return { status: "unsafe" }; - const handle = await fs.open(filePath, "r"); - try { - const decoder = new StringDecoder("utf8"); - const chunk = Buffer.allocUnsafe(64 * 1024); - while (true) { - const { bytesRead } = await handle.read(chunk, 0, chunk.byteLength, null); - if (bytesRead === 0) break; - collector.write(decoder.write(chunk.subarray(0, bytesRead))); - } - collector.write(decoder.end()); - } finally { - await handle.close().catch(() => undefined); + const { size } = await handle.stat(); + if (size === 0) return { status: "missing" }; + // The archived excerpt is read first and becomes the redaction target: the + // collector rewrites exactly this text, so a target smaller than the file + // would silently drop the rest of the conversation. + const excerpt = + size <= maxBytes ? await readBytes(handle, 0, size) : await readHeadTailBytes(handle, size, maxBytes); + if (excerpt.byteLength === 0) return { status: "missing" }; + const collector = createSecretRedactionCollector(excerpt.toString("utf8")); + // The whole file is still scanned, so a credential written once early and + // echoed later in the excerpt is recognised in both places. + const decoder = new StringDecoder("utf8"); + const chunk = Buffer.allocUnsafe(64 * 1024); + let position = 0; + while (position < size) { + const { bytesRead } = await handle.read(chunk, 0, Math.min(chunk.byteLength, size - position), position); + if (bytesRead === 0) break; + position += bytesRead; + collector.write(decoder.write(chunk.subarray(0, bytesRead))); } + collector.write(decoder.end()); const result = collector.finish(); if (result.status === "unsafe") return { status: "unsafe" }; - const redacted = Buffer.from(result.value, "utf8"); - if (redacted.byteLength <= maxBytes) { - return { status: "ready", entry: { ...entry, content: redacted } }; + const text = collapseHomePaths(result.value); + const redacted = Buffer.from(text, "utf8"); + const bounded = + redacted.byteLength <= maxBytes ? redacted : headAndTailAtLineBoundary(redacted, maxBytes).content; + // Every row keeps the plain transcript it had, plus a `feedback` object + // with that row's own usage, cache hit rate, and timing. Only this copy is + // annotated; the session file on disk is never rewritten. + const annotated = Buffer.from(annotateTranscriptRows(bounded.toString("utf8")), "utf8"); + const content = + annotated.byteLength <= maxBytes ? annotated : headAndTailAtLineBoundary(annotated, maxBytes).content; + if (content.byteLength === 0) { + return { + status: "ready", + text, + entry: { + name: EVENTS_ENTRY, + content, + note: `omitted after redaction, a single line exceeds ${formatBytes(maxBytes)}`, + }, + }; } - - const content = tailAtLineBoundary(redacted, maxBytes); - const redactionNote = - content.byteLength === 0 - ? `omitted after redaction, a single line exceeds ${formatBytes(maxBytes)}` - : `tail re-limited after redaction, ${formatBytes(redacted.byteLength)} before re-limit`; + const trimmed = redacted.byteLength > maxBytes; + const annotatedTrimmed = annotated.byteLength > maxBytes; + const note = trimmed + ? `head and tail of ${formatBytes(size)}, re-limited after redaction` + : annotatedTrimmed + ? `head and tail of ${formatBytes(size)}, re-limited after per-row metrics` + : size > maxBytes + ? `head and tail of ${formatBytes(size)}` + : undefined; return { status: "ready", - entry: { - ...entry, - content, - note: entry.note ? `${entry.note}; ${redactionNote}` : redactionNote, - }, + text, + entry: { name: EVENTS_ENTRY, content, ...(note ? { note } : {}) }, }; } catch { return { status: "unsafe" }; + } finally { + await handle.close().catch(() => undefined); + } +} + +async function readBytes( + handle: Awaited>, + position: number, + length: number, +): Promise { + const buffer = Buffer.alloc(length); + const { bytesRead } = await handle.read(buffer, 0, length, position); + return buffer.subarray(0, bytesRead); +} + +/** + * Both ends of an oversized file, cut at line boundaries on each inner edge and + * separated by a marker: a reader must be able to tell that the middle is + * missing rather than assume the conversation skipped a turn. + */ +async function readHeadTailBytes( + handle: Awaited>, + size: number, + maxBytes: number, +): Promise { + const marker = Buffer.from(`\n… [middle of this session omitted, ${formatBytes(size)} on disk] …\n`, "utf8"); + const budget = Math.max(0, maxBytes - marker.byteLength); + const headBudget = Math.floor(budget / 2); + const head = await readBytes(handle, 0, headBudget); + const headEnd = head.lastIndexOf(0x0a); + const tailBudget = budget - headBudget; + const tail = await readBytes(handle, size - tailBudget, tailBudget); + const tailStart = tail.indexOf(0x0a); + const headPart = headEnd <= 0 ? Buffer.alloc(0) : head.subarray(0, headEnd + 1); + const tailPart = tailStart === -1 ? Buffer.alloc(0) : tail.subarray(tailStart + 1); + // The marker only makes sense between two kept parts. + if (headPart.byteLength === 0 || tailPart.byteLength === 0) return Buffer.concat([headPart, tailPart]); + return Buffer.concat([headPart, marker, tailPart]); +} + +/** + * Attach the full output of tool calls the transcript referenced. + * + * Only names are taken from the transcript, and they are resolved inside this + * session's own `tool-output` directory: a transcript is untrusted input, so it + * must not be able to name an arbitrary file. + */ +async function readToolOutputEntries(sessionFile: string, transcript: string): Promise { + const names = new Set(); + for (const match of transcript.matchAll(/tool-[0-9a-f]{32}\.txt/gu)) names.add(match[0]); + if (names.size === 0) return []; + const directory = path.join(path.dirname(path.resolve(sessionFile)), TOOL_OUTPUT_DIR); + const entries: BundleEntry[] = []; + let total = 0; + for (const name of [...names].slice(0, TOOL_OUTPUT_MAX_FILES)) { + if (!TOOL_OUTPUT_NAME.test(name)) continue; + const filePath = path.join(directory, name); + const stats = await fs.lstat(filePath).catch(() => undefined); + if (!stats?.isFile() || stats.size === 0) continue; + if (total + Math.min(stats.size, TOOL_OUTPUT_MAX_FILE_BYTES) > TOOL_OUTPUT_MAX_TOTAL_BYTES) break; + const read = await readHeadEntry(filePath, `${TOOL_OUTPUT_DIR}/${name}`, TOOL_OUTPUT_MAX_FILE_BYTES); + if (!read) continue; + const redacted = Buffer.from(collapseHomePaths(redactSecretString(read.content.toString("utf8"))), "utf8"); + const bounded = limitRedactedSessionEntry({ ...read, content: redacted }, TOOL_OUTPUT_MAX_FILE_BYTES); + total += bounded.content.byteLength; + const complete = stats.size <= TOOL_OUTPUT_MAX_FILE_BYTES; + entries.push({ + ...bounded, + note: `${bounded.note ? `${bounded.note}; ` : ""}${complete ? "full tool output" : "tool output, head only"}`, + }); } + return entries; } function limitRedactedSessionEntry(entry: BundleEntry, maxBytes: number): BundleEntry { if (entry.content.byteLength <= maxBytes) return entry; - const content = tailAtLineBoundary(entry.content, maxBytes); + const content = headAndTailAtLineBoundary(entry.content, maxBytes).content; const note = content.byteLength === 0 ? `omitted in compressed-size fallback, a single line exceeds ${formatBytes(maxBytes)}` @@ -244,7 +381,9 @@ async function readDevLogErrorLines(storageRootDir: string, at: Date): Promise { + const linkStats = await fs.lstat(filePath).catch(() => undefined); + if (!linkStats?.isFile()) return undefined; + const handle = await fs.open(filePath, "r").catch(() => undefined); + if (!handle) return undefined; + try { + const { size } = await handle.stat(); + if (size === 0) return undefined; + const length = Math.min(size, maxBytes); + const buffer = Buffer.alloc(length); + const { bytesRead } = await handle.read(buffer, 0, length, 0); + const read = buffer.subarray(0, bytesRead); + // A truncated read must not end mid-record, so prefer whole lines. A file + // that is one long line has none to cut at; keep the window, which is all + // a caller can use anyway (the session scan re-reads the file itself). + const lastNewline = read.lastIndexOf(0x0a); + const content = bytesRead < size && lastNewline > 0 ? read.subarray(0, lastNewline + 1) : read; + if (content.byteLength === 0) return undefined; + return { name, content }; + } catch { + return undefined; + } finally { + await handle.close().catch(() => undefined); + } +} + function countLines(content: Buffer): number { if (content.byteLength === 0) return 0; let lines = 0; @@ -342,8 +523,24 @@ async function findSessionFile(storageRootDir: string, sessionId: string): Promi return match; } -async function findNewestSessionFile(storageRootDir: string): Promise { - const files = await collectJsonlFiles(storageRootDir); +/** + * The newest session **of the project the user is in**. + * + * Walking every project directory and taking the newest file would attach a + * conversation from another project — possibly one they are not thinking + * about — as the context of this report. A guess therefore stays inside this + * cwd's own session folder; an explicit `--session` is the only way out. + * + * Two layouts exist: the manager's per-project folders, and a flat directory + * (an explicit `--session-dir` or a test fixture). A flat directory is only + * guessed from when it has no project folders at all, so an ordinary install + * can never pick another project's conversation. + */ +async function findNewestSessionFile(storageRootDir: string, cwd: string): Promise { + const projectDir = path.join(storageRootDir, sessionFolderName(cwd)); + const hasProjectDir = await isDirectory(projectDir); + if (!hasProjectDir && (await hasProjectFolders(storageRootDir))) return undefined; + const files = await collectJsonlFiles(hasProjectDir ? projectDir : storageRootDir); let newest: { session: SelectedSession; mtime: number } | undefined; for (const filePath of files) { const stats = await fs.stat(filePath).catch(() => undefined); @@ -354,6 +551,23 @@ async function findNewestSessionFile(storageRootDir: string): Promise { + const stats = await fs.stat(target).catch(() => undefined); + return stats?.isDirectory() === true; +} + +/** Any `--project--` folder means the sessions are partitioned; do not cross them. */ +async function hasProjectFolders(storageRootDir: string): Promise { + const names = await fs.readdir(storageRootDir).catch(() => [] as string[]); + return names.some((name) => name.startsWith("--") && name.endsWith("--")); +} + +/** Session folders are named after the project path, mirroring the manager. */ +function sessionFolderName(cwd: string): string { + const resolved = path.resolve(cwd); + return `--${resolved.replace(/^[/\\]/, "").replace(/[/\\:]/gu, "-")}--`; +} + async function validateSessionFile(filePath: string): Promise { if (!filePath.endsWith(".jsonl")) return undefined; const resolved = path.resolve(filePath); diff --git a/packages/coding-agent/src/step/feedback/command.ts b/packages/coding-agent/src/step/feedback/command.ts index 649ad50e1..d6359c7e8 100644 --- a/packages/coding-agent/src/step/feedback/command.ts +++ b/packages/coding-agent/src/step/feedback/command.ts @@ -2,7 +2,7 @@ import { stdin as input, stdout as output } from "node:process"; import * as readline from "node:readline/promises"; import { resolveStepStorageRoot } from "../storage-root.ts"; import { type StepTelemetryReporter, trackStepTelemetry } from "../telemetry.ts"; -import { buildFeedbackSessionBundle, describeFeedbackBundle, type FeedbackBundleResult } from "./bundle.ts"; +import { describeFeedbackBundle, type FeedbackBundleResult } from "./bundle.ts"; import { neutralizeFeedbackConsentMetadata } from "./consent.ts"; import { deliverFeedback, @@ -13,8 +13,9 @@ import { type FeedbackDeliveryOutcome, retryPendingFeedback, } from "./delivery.ts"; -import { readFeedbackDiagnostics } from "./diagnostics.ts"; import { resolveFeedbackEndpoint } from "./endpoints.ts"; +import { prepareFeedbackEvidence } from "./evidence.ts"; +import type { FeedbackRuntimeFacts, FeedbackSessionSummary } from "./session-summary.ts"; import { resolveFeedbackSettings } from "./settings.ts"; import { buildFeedbackSubmission } from "./submission.ts"; import { @@ -23,6 +24,7 @@ import { type FeedbackCategory, type FeedbackDiagnostics, type FeedbackSubmission, + type FeedbackSurface, } from "./types.ts"; import { normalizeFeedbackCategory } from "./validate.ts"; @@ -41,6 +43,8 @@ export interface FeedbackCommandDependencies { env?: NodeJS.ProcessEnv; interactive?: boolean; prompt?: FeedbackCommandPrompt; + /** Live host state (permission tier, live context size) the session file does not carry. */ + runtime?: FeedbackRuntimeFacts | (() => FeedbackRuntimeFacts | Promise); } export interface FeedbackCommandPrompt { @@ -51,6 +55,8 @@ export interface FeedbackSubmitInput { category?: string; comment: string; diagnostics?: FeedbackDiagnostics; + /** Content-free session state, attached by default; see `session-summary.ts`. */ + sessionSummary?: FeedbackSessionSummary; sessionId?: string; sessionBundle?: FeedbackBundleResult; storageRootDir: string; @@ -61,7 +67,7 @@ export interface FeedbackSubmitInput { telemetry?: StepTelemetryReporter; fetchImpl?: typeof fetch; env?: NodeJS.ProcessEnv; - surface: "cli" | "tui"; + surface: FeedbackSurface; confirm?: (submission: FeedbackSubmission) => Promise; } @@ -121,6 +127,8 @@ export async function submitFeedback(input: FeedbackSubmitInput): Promise 0; + // A terminal means the consent page can be shown, whether or not the comment + // was typed on the command line. Only the questions depend on that. const guided = canPrompt && !hasExplicitComment; - if (!guided && !category && !comment) { + if (!canPrompt && !category && !comment) { write(stderr, "step feedback needs a comment or --category when no interactive terminal is available\n"); return 1; } let ownedPrompt: readline.Interface | undefined; let prompt: FeedbackCommandPrompt | undefined; - if (guided) { + if (canPrompt) { if (dependencies.prompt) prompt = dependencies.prompt; else { ownedPrompt = readline.createInterface({ input: stdin, output: stdout }); @@ -257,7 +267,7 @@ export async function runFeedbackCommand( } } try { - if (prompt && !category) { + if (guided && prompt && !category) { category = await askCategory(prompt); if (!category) { write(stdout, "Cancelled; nothing was submitted\n"); @@ -265,29 +275,37 @@ export async function runFeedbackCommand( } } const readAt = new Date(); - const shouldReadDiagnostics = - parsed.diagnostics === true || (prompt !== undefined && parsed.diagnostics !== false); - let diagnostics = shouldReadDiagnostics - ? await readFeedbackDiagnostics({ storageRootDir, at: readAt }) - : undefined; - const shouldBuildBundle = - bundleEndpoint !== undefined && - (parsed.sessionBundle === true || (prompt !== undefined && parsed.sessionBundle !== false)); - let sessionBundle = shouldBuildBundle - ? await buildFeedbackSessionBundle({ - storageRootDir, - ...(!parsed.session && dependencies.sessionFile ? { sessionFile: dependencies.sessionFile } : {}), - sessionId: parsed.session ?? dependencies.sessionId, - at: readAt, - env, - }) - : undefined; + // Everything is gathered by default. A report without the conversation + // cannot be investigated, and asking the user to assemble it produced + // reports that were just their own sentence. The two flags stay as + // explicit opt-outs for scripted callers. + const evidence = await prepareFeedbackEvidence({ + storageRootDir, + sessionFile: parsed.session ? undefined : dependencies.sessionFile, + sessionId: parsed.session ?? dependencies.sessionId, + at: readAt, + env, + // A bundle that cannot be uploaded is 24 MiB of reading and gzip for + // nothing, and it would be announced on the consent page. + withoutBundle: parsed.sessionBundle === false || bundleEndpoint === undefined, + withoutDiagnostics: parsed.diagnostics === false, + ...(dependencies.runtime + ? { + runtime: + typeof dependencies.runtime === "function" ? await dependencies.runtime() : dependencies.runtime, + } + : {}), + }); + const sessionBundle = evidence.bundle; if (!parsed.json && sessionBundle?.status === "skipped") { write(stdout, `${formatFeedbackBundleSkipMessage(sessionBundle)}\n`); } - if (prompt && parsed.diagnostics === undefined) diagnostics = await askDiagnostics(prompt, diagnostics); - if (prompt && parsed.sessionBundle === undefined) sessionBundle = await askBundle(prompt, sessionBundle); - if (prompt) { + // With no terminal to show the review page on, say what goes with the + // report so a scripted submission is never silent about the transcript. + if (!prompt && !parsed.json && sessionBundle?.status === "ready") { + write(stderr, `Attaching: ${descriptionsOf(sessionBundle)}\n`); + } + if (guided && prompt) { const answer = await prompt.question("Feedback: "); if (answer === undefined) { write(stdout, "Cancelled; nothing was submitted\n"); @@ -303,11 +321,12 @@ export async function runFeedbackCommand( : sessionArgumentLooksLikePath ? undefined : parsed.session - : dependencies.sessionId; + : (evidence.sessionId ?? dependencies.sessionId); const result = await submitFeedback({ category, comment, - ...(diagnostics ? { diagnostics: diagnostics.diagnostics } : {}), + ...(evidence.diagnostics ? { diagnostics: evidence.diagnostics.diagnostics } : {}), + ...(evidence.summary ? { sessionSummary: evidence.summary } : {}), ...(sessionBundle ? { sessionBundle } : {}), storageRootDir, ...(submissionSessionId ? { sessionId: submissionSessionId } : {}), @@ -322,8 +341,8 @@ export async function runFeedbackCommand( ...(prompt ? { confirm: async (submission: FeedbackSubmission) => { - if (submission.diagnostics && diagnostics) { - previewDiagnostics(stdout, diagnostics.displayPath, submission.diagnostics); + if (submission.diagnostics && evidence.diagnostics) { + previewDiagnostics(stdout, evidence.diagnostics.displayPath, submission.diagnostics); } previewBundle(stdout, sessionBundle); return await confirm(prompt, "Submit feedback? [y/N] "); @@ -426,10 +445,8 @@ function feedbackHelp(): string { "Options:", " --category bug, bad-result, good-result, safety-check, other", " --message Feedback comment", - " --diagnostics Attach bounded diagnostics", - " --no-diagnostics Do not attach diagnostics", - " --session-bundle Attach the current session archive", - " --no-session-bundle Do not attach a session archive", + " --no-diagnostics Send without the log excerpt (it is attached by default)", + " --no-session-bundle Send without the conversation archive (attached by default)", " --session Session to archive", " --retry Retry locally pending feedback", " --json Emit JSON only on stdout", @@ -452,23 +469,18 @@ async function askCategory(prompt: FeedbackCommandPrompt): Promise>, -): Promise>> { - if (!candidate) return undefined; - const displayPath = neutralizeFeedbackConsentMetadata(candidate.displayPath); - return (await confirm(prompt, `Attach diagnostics from ${displayPath}? [Y/n] `, true)) ? candidate : undefined; -} - -async function askBundle( - prompt: FeedbackCommandPrompt, - candidate: FeedbackBundleResult | undefined, -): Promise { - if (!candidate || candidate.status !== "ready") return candidate; - return (await confirm(prompt, `${describeFeedbackBundle(candidate.bundle)}\nAttach session bundle? [Y/n] `, true)) - ? candidate - : undefined; +/** One line naming what the archive carries, for a non-interactive submission. */ +function descriptionsOf(candidate: FeedbackBundleResult): string { + if (candidate.status !== "ready") return "nothing"; + const names = candidate.bundle.files.map((file) => file.name); + const toolOutputs = names.filter((name) => name.startsWith("tool-output/")).length; + const parts = [ + "conversation transcript", + ...(toolOutputs > 0 ? [`${toolOutputs} full tool output file${toolOutputs === 1 ? "" : "s"}`] : []), + ...(names.includes("dev.log") ? ["log tail"] : []), + "session summary", + ]; + return `${parts.join(", ")}; pass --no-session-bundle to send the report without the transcript`; } function previewBundle(stream: NodeJS.WritableStream, candidate: FeedbackBundleResult | undefined): void { diff --git a/packages/coding-agent/src/step/feedback/consent.ts b/packages/coding-agent/src/step/feedback/consent.ts index 2a97471a4..1883a4cc6 100644 --- a/packages/coding-agent/src/step/feedback/consent.ts +++ b/packages/coding-agent/src/step/feedback/consent.ts @@ -34,9 +34,39 @@ export function formatFeedbackConsentPreview(input: FeedbackConsentPreviewInput) `Comment:\n${input.submission.comment || "(empty comment)"}`, [ "Identity sent with this feedback:", - ...(["deviceId", "uid", "username", "channel", "version", "platform", "commit"] as const).map( + ...( + [ + "deviceId", + "uid", + "username", + "channel", + "version", + "versionSource", + "buildTime", + "commit", + "platform", + "arch", + "osRelease", + "locale", + "installSource", + "sessionId", + ] as const + ).map( (field) => `${field}: ${neutralizeFeedbackConsentMetadata(input.submission.context[field] ?? "(not set)")}`, ), + // The interpreter versions are one line: three near-identical rows add + // noise to a page whose job is to be read once. + `runtime: ${neutralizeFeedbackConsentMetadata( + ["node", "electron", "bun"] + .flatMap((name) => { + const value = input.submission.context.runtime?.[name as "node" | "electron" | "bun"]; + return value ? [`${name} ${value}`] : []; + }) + .join(", ") || "(not set)", + )}`, + ...(input.submission.surface + ? [`surface: ${neutralizeFeedbackConsentMetadata(input.submission.surface)}`] + : []), ].join("\n"), ]; const diagnostics = input.submission.diagnostics; diff --git a/packages/coding-agent/src/step/feedback/context.ts b/packages/coding-agent/src/step/feedback/context.ts index 2b31db576..fd9f88674 100644 --- a/packages/coding-agent/src/step/feedback/context.ts +++ b/packages/coding-agent/src/step/feedback/context.ts @@ -1,8 +1,9 @@ +import { arch, release } from "node:os"; import { normalizeStepDeviceId, normalizeStepUid, normalizeStepWireSessionId } from "../build-identity.ts"; import { readStepDeviceId } from "../device-id.ts"; import { resolveStepCodeVersion } from "../version.ts"; import { readFeedbackBuildEnv } from "./build-env.ts"; -import type { FeedbackContext } from "./types.ts"; +import type { FeedbackContext, FeedbackRuntime } from "./types.ts"; const MAX_USERNAME_LENGTH = 64; const PLAUSIBLE_USERNAME = /^[\w.@:-]+$/u; @@ -15,6 +16,12 @@ export function readFeedbackUsername(env: NodeJS.ProcessEnv = process.env, expli return raw && raw.length <= MAX_USERNAME_LENGTH && PLAUSIBLE_USERNAME.test(raw) ? raw : undefined; } +/** + * A report names the surface it came from, the exact platform it ran on, and + * how it was installed. `platform` alone ("darwin") cannot distinguish an + * Apple Silicon dmg from an Intel npm install, which is the first question for + * a native-module or upgrade report. + */ export async function resolveFeedbackContext(input: { storageRootDir: string; sessionId?: string; @@ -23,15 +30,27 @@ export async function resolveFeedbackContext(input: { env?: NodeJS.ProcessEnv; }): Promise { const env = input.env ?? process.env; - const { channel, commit } = readFeedbackBuildEnv(env); + const { channel, commit, buildTime } = readFeedbackBuildEnv(env); + const version = resolveStepCodeVersion(env); const deviceId = normalizeStepDeviceId(await readStepDeviceId(input.storageRootDir)); const username = readFeedbackUsername(env, input.username); const sessionId = normalizeStepWireSessionId(input.sessionId); const uid = normalizeStepUid(input.uid); + const installSource = readInstallSource(env); + const locale = readLocale(env); return { channel, - version: resolveStepCodeVersion(env).value, + version: version.value, + // A fallback version says nothing about which release this is, so a + // report from a source build is not mistaken for a shipped one. + versionSource: version.source, + ...(buildTime ? { buildTime } : {}), + runtime: readRuntimeVersions(), platform: process.platform, + arch: arch(), + osRelease: release(), + ...(locale ? { locale } : {}), + ...(installSource ? { installSource } : {}), ...(commit ? { commit } : {}), ...(sessionId ? { sessionId } : {}), ...(deviceId ? { deviceId } : {}), @@ -39,3 +58,35 @@ export async function resolveFeedbackContext(input: { ...(username ? { username } : {}), }; } + +/** The interpreter the code is running under; desktop and binary builds differ. */ +function readRuntimeVersions(): FeedbackRuntime { + const node = process.versions.node; + const electron = process.versions.electron; + const bun = process.versions.bun; + return { + ...(plausibleVersion(node) ? { node } : {}), + ...(plausibleVersion(electron) ? { electron } : {}), + ...(plausibleVersion(bun) ? { bun } : {}), + }; +} + +/** A version string reaches a report, so bound it like every other context value. */ +function plausibleVersion(value: string | undefined): value is string { + return typeof value === "string" && /^[0-9][\w.+-]{0,31}$/u.test(value); +} + +/** Where this binary came from, when the launcher or packaging recorded it. */ +function readInstallSource(env: NodeJS.ProcessEnv): string | undefined { + for (const name of ["STEPCODE_INSTALL_SOURCE", "STEPCODE_DISTRIBUTION"] as const) { + const value = env[name]?.trim(); + if (value && /^[a-z0-9._-]{1,32}$/iu.test(value)) return value.toLowerCase(); + } + return undefined; +} + +function readLocale(env: NodeJS.ProcessEnv): string | undefined { + const value = (env.LC_ALL || env.LC_MESSAGES || env.LANG || "").trim(); + const match = /^([a-z]{2,3})[-_]([A-Z]{2})/u.exec(value); + return match ? `${match[1].toLowerCase()}-${match[2]}` : undefined; +} diff --git a/packages/coding-agent/src/step/feedback/evidence.ts b/packages/coding-agent/src/step/feedback/evidence.ts new file mode 100644 index 000000000..0f7d5bad1 --- /dev/null +++ b/packages/coding-agent/src/step/feedback/evidence.ts @@ -0,0 +1,90 @@ +/** + * Everything a feedback report attaches, gathered once. + * + * Both hosts the product ships (the terminal and the CLI's top-level command) + * call this, so a report filed from either surface carries the same evidence: + * the session transcript, the full output files it referenced, the host log + * tail, and the content-free session summary. A host decides only how to ask + * the user; it never decides what a report contains. + */ + +import fs from "node:fs/promises"; +import { buildFeedbackSessionBundle, type FeedbackBundleResult, resolveFeedbackSession } from "./bundle.ts"; +import { type FeedbackDiagnosticsSelection, readFeedbackDiagnostics } from "./diagnostics.ts"; +import { type FeedbackRuntimeFacts, type FeedbackSessionSummary, summarizeSessionFile } from "./session-summary.ts"; +import { FEEDBACK_SESSION_RECENCY_WINDOW_MS } from "./types.ts"; + +export interface FeedbackEvidence { + bundle?: FeedbackBundleResult; + diagnostics?: FeedbackDiagnosticsSelection; + summary?: FeedbackSessionSummary; + /** The session the summary and bundle describe, when one could be resolved. */ + sessionId?: string; + sessionFile?: string; +} + +export async function prepareFeedbackEvidence(input: { + storageRootDir: string; + sessionFile?: string; + sessionId?: string; + at?: Date; + env?: NodeJS.ProcessEnv; + /** Skip the archive (tests, or a host without a session). */ + withoutBundle?: boolean; + /** Skip the log excerpt. */ + withoutDiagnostics?: boolean; + /** Live host state the session file cannot provide. */ + runtime?: FeedbackRuntimeFacts; + /** Project the report is filed from; a guessed session never leaves it. */ + cwd?: string; +}): Promise { + const at = input.at ?? new Date(); + // Whether the user named a session decides the recency rule below; compute it + // before resolving, because a resolved file no longer looks guessed. + const guessed = !input.sessionFile && !input.sessionId; + const resolved = await resolveFeedbackSession({ + ...(input.sessionFile ? { sessionFile: input.sessionFile } : {}), + ...(input.sessionId ? { sessionId: input.sessionId } : {}), + ...(input.env ? { env: input.env } : {}), + cwd: input.cwd ?? process.cwd(), + }); + const bundle = input.withoutBundle + ? undefined + : await buildFeedbackSessionBundle({ + storageRootDir: input.storageRootDir, + // Pass the file the summary described: resolving a second time could + // pick a different session and attach a conversation the report does + // not describe. + ...(resolved ? { sessionFile: resolved.filePath } : input.sessionId ? { sessionId: input.sessionId } : {}), + at, + ...(input.env ? { env: input.env } : {}), + cwd: input.cwd ?? process.cwd(), + guessedSession: guessed, + }); + // A guessed session outside the recency window is not this report's + // conversation: describing it (even without content) would attach the wrong + // session id. Checked here as well as in the bundle, because a host without a + // bundle endpoint skips the bundle entirely. + const stale = + (bundle?.status === "skipped" && bundle.reason === "stale") || + (guessed && resolved && (await isStale(resolved.filePath, at))); + const summary = + resolved && !stale + ? await summarizeSessionFile(resolved.filePath, { ...(input.runtime ? { runtime: input.runtime } : {}) }) + : undefined; + const diagnostics = input.withoutDiagnostics + ? undefined + : await readFeedbackDiagnostics({ storageRootDir: input.storageRootDir, at }); + return { + ...(bundle ? { bundle } : {}), + ...(diagnostics ? { diagnostics } : {}), + ...(summary ? { summary } : {}), + ...(resolved && !stale ? { sessionId: resolved.sessionId, sessionFile: resolved.filePath } : {}), + }; +} + +/** The same window the bundle applies to a guessed session. */ +async function isStale(sessionFile: string, at: Date): Promise { + const stats = await fs.stat(sessionFile).catch(() => undefined); + return stats === undefined || at.getTime() - stats.mtime.getTime() >= FEEDBACK_SESSION_RECENCY_WINDOW_MS; +} diff --git a/packages/coding-agent/src/step/feedback/index.ts b/packages/coding-agent/src/step/feedback/index.ts index d2711dffa..964454cb6 100644 --- a/packages/coding-agent/src/step/feedback/index.ts +++ b/packages/coding-agent/src/step/feedback/index.ts @@ -4,7 +4,9 @@ export * from "./context.ts"; export * from "./delivery.ts"; export * from "./diagnostics.ts"; export * from "./endpoints.ts"; +export * from "./evidence.ts"; export * from "./pending-store.ts"; +export * from "./session-summary.ts"; export * from "./settings.ts"; export * from "./submission.ts"; export * from "./types.ts"; diff --git a/packages/coding-agent/src/step/feedback/pending-store.ts b/packages/coding-agent/src/step/feedback/pending-store.ts index e69588a32..706a3766d 100644 --- a/packages/coding-agent/src/step/feedback/pending-store.ts +++ b/packages/coding-agent/src/step/feedback/pending-store.ts @@ -43,6 +43,28 @@ const BUNDLE_ENTRY_LIMITS: Readonly> = { "resume.json": 8 * 1024 * 1024, "dev.log": 512 * 1024, }; +/** + * Full tool output attached by the bundle: one directory level, fixed prefix, + * hex name. A retry re-reads these, so an archive that carried them must stay + * valid; without this a stored archive is rejected and can never be re-sent. + */ +const TOOL_OUTPUT_ENTRY = /^tool-output\/tool-[0-9a-f]{32}\.txt$/u; +const TOOL_OUTPUT_ENTRY_LIMIT = 2 * 1024 * 1024; +/** The largest a single archive entry may be, whichever rule names it. */ +const MAX_BUNDLE_ENTRY_BYTES = 24 * 1024 * 1024; +/** events + dev.log + manifest + the bundle's ten tool-output files, with headroom. */ +const MAX_BUNDLE_ENTRIES = 16; + +function bundleEntryLimit(name: string): number | undefined { + return BUNDLE_ENTRY_LIMITS[name] ?? (TOOL_OUTPUT_ENTRY.test(name) ? TOOL_OUTPUT_ENTRY_LIMIT : undefined); +} + +/** A legacy archive names its members differently; both layouts are valid input. */ +function anyEntryLimit(name: string): number | undefined { + if (name === LEGACY_SESSION_ENTRY) return BUNDLE_ENTRY_LIMITS["events.jsonl"]; + if (name === LEGACY_MANIFEST_ENTRY) return BUNDLE_ENTRY_LIMITS[BUNDLE_MANIFEST_ENTRY]; + return bundleEntryLimit(name); +} export interface PendingFeedbackEntry { feedbackId: string; @@ -133,7 +155,7 @@ export async function normalizePendingFeedbackBundle(data: Buffer): Promise entry.name)); if (names.has(BUNDLE_MANIFEST_ENTRY)) { - if (entries.some((entry) => BUNDLE_ENTRY_LIMITS[entry.name] === undefined)) return undefined; + if (entries.some((entry) => bundleEntryLimit(entry.name) === undefined)) return undefined; const manifest = entries.find((entry) => entry.name === BUNDLE_MANIFEST_ENTRY); const createdAt = manifest ? validCurrentBundleManifest(manifest.content, entries) : undefined; if (!createdAt) return undefined; @@ -238,7 +260,19 @@ async function readSubmission( const submission = value as Record; if ( Object.keys(submission).some( - (key) => !["feedbackId", "category", "comment", "at", "context", "diagnostics"].includes(key), + (key) => + ![ + "feedbackId", + "category", + "comment", + "at", + "context", + "diagnostics", + "schemaVersion", + "surface", + "sessionSummary", + "request", + ].includes(key), ) ) { return undefined; @@ -258,8 +292,12 @@ async function readSubmission( submission.at.length > MAX_AT_LENGTH || CONTROL_CHARACTERS.test(submission.at) || !Number.isFinite(Date.parse(submission.at)) || + !isStrictSchemaVersion(submission.schemaVersion) || + !isStrictSurface(submission.surface) || !isStrictContext(submission.context) || - !isStrictDiagnostics(submission.diagnostics) + !isStrictDiagnostics(submission.diagnostics) || + !isStrictSummary(submission.sessionSummary) || + !isStrictRequest(submission.request) ) { return undefined; } @@ -277,12 +315,36 @@ async function rejectSymbolicLink(filePath: string): Promise { function isStrictContext(value: unknown): value is FeedbackSubmission["context"] { if (!value || typeof value !== "object" || Array.isArray(value)) return false; const context = value as Record; - const allowed = new Set(["channel", "version", "platform", "commit", "sessionId", "deviceId", "uid", "username"]); + const allowed = new Set([ + "channel", + "version", + "versionSource", + "buildTime", + "runtime", + "platform", + "arch", + "osRelease", + "locale", + "installSource", + "commit", + "sessionId", + "deviceId", + "uid", + "username", + ]); if (Object.keys(context).some((key) => !allowed.has(key))) return false; for (const key of ["channel", "version", "platform"] as const) { const field = context[key]; if (!isSafeContextString(field, MAX_CONTEXT_VALUE_LENGTH)) return false; } + for (const key of ["arch", "osRelease", "locale", "installSource"] as const) { + const field = context[key]; + if (field !== undefined && !isSafeContextString(field, MAX_CONTEXT_VALUE_LENGTH)) return false; + } + if (context.versionSource !== undefined && !isKnownVersionSource(context.versionSource)) return false; + if (context.buildTime !== undefined && normalizeStepCommitForPending(context.buildTime) !== context.buildTime) + return false; + if (context.runtime !== undefined && !isStrictRuntimeVersions(context.runtime)) return false; if (context.commit !== undefined && normalizeStepCommitForPending(context.commit) !== context.commit) return false; if (context.deviceId !== undefined && normalizeStepDeviceId(context.deviceId) !== context.deviceId) return false; if (context.sessionId !== undefined && normalizeStepWireSessionId(context.sessionId) !== context.sessionId) @@ -303,6 +365,23 @@ function normalizeStepCommitForPending(value: unknown): string | undefined { : undefined; } +function isKnownVersionSource(value: unknown): boolean { + return value === "override" || value === "embedded" || value === "fallback"; +} + +/** Interpreter versions: short, printable, and never free text. */ +function isStrictRuntimeVersions(value: unknown): boolean { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const runtime = value as Record; + if (Object.keys(runtime).some((key) => !["node", "electron", "bun"].includes(key))) return false; + for (const key of ["node", "electron", "bun"] as const) { + const field = runtime[key]; + if (field === undefined) continue; + if (typeof field !== "string" || !/^[0-9][\w.+-]{0,31}$/u.test(field)) return false; + } + return true; +} + function isSafeContextString(value: unknown, maxLength: number): value is string { return ( typeof value === "string" && @@ -456,7 +535,7 @@ function validCurrentBundleManifest(content: Buffer, entries: readonly TarEntry[ const descriptor = file as Record; if (Object.keys(descriptor).some((key) => !descriptorKeys.has(key))) return undefined; if (typeof descriptor.name !== "string" || describedNames.has(descriptor.name)) return undefined; - if (BUNDLE_ENTRY_LIMITS[descriptor.name] === undefined) return undefined; + if (bundleEntryLimit(descriptor.name) === undefined) return undefined; const entry = payloadEntries.get(descriptor.name); if ( !entry || @@ -491,24 +570,27 @@ function readTarEntries(tar: Buffer): TarEntry[] | undefined { if (!tar.subarray(offset).every((byte) => byte === 0)) return undefined; return entries; } - if (entries.length >= Object.keys(BUNDLE_ENTRY_LIMITS).length) return undefined; + if (entries.length >= MAX_BUNDLE_ENTRIES) return undefined; if (!validTarChecksum(header)) return undefined; const name = readTarString(header, 0, 100); - if (!name || name.includes("/") || readTarString(header, 345, 155) !== "" || names.has(name)) { + // One nested directory is allowed only for the fixed tool-output name; + // every other member is flat. + const known = name === undefined ? undefined : anyEntryLimit(name); + const nested = name !== undefined && TOOL_OUTPUT_ENTRY.test(name); + if ( + !name || + known === undefined || + (!nested && name.includes("/")) || + readTarString(header, 345, 155) !== "" || + names.has(name) + ) { return undefined; } const typeFlag = header[156]; if (typeFlag !== 0 && typeFlag !== 0x30) return undefined; const size = readTarOctal(header, 124, 12); if (size === undefined) return undefined; - const knownLimit = - BUNDLE_ENTRY_LIMITS[name] ?? - (name === LEGACY_SESSION_ENTRY - ? BUNDLE_ENTRY_LIMITS["events.jsonl"] - : name === LEGACY_MANIFEST_ENTRY - ? BUNDLE_ENTRY_LIMITS[BUNDLE_MANIFEST_ENTRY] - : undefined); - if (knownLimit === undefined || size > knownLimit) return undefined; + if (known === undefined || size > Math.min(known, MAX_BUNDLE_ENTRY_BYTES)) return undefined; const contentStart = offset + TAR_BLOCK_BYTES; const contentEnd = contentStart + size; if (contentEnd > tar.byteLength) return undefined; @@ -645,3 +727,272 @@ function writeTarOctal(target: Buffer, offset: number, length: number, value: nu const text = `${value.toString(8)}\0`.padStart(length, "0").slice(-length); Buffer.from(text, "ascii").copy(target, offset, 0, length); } + +function isStrictSchemaVersion(value: unknown): boolean { + return value === undefined || (Number.isInteger(value) && (value as number) >= 1 && (value as number) <= 100); +} + +const FEEDBACK_SURFACES = new Set(["cli", "tui", "web", "desktop", "sdk"]); + +function isStrictSurface(value: unknown): boolean { + return value === undefined || (typeof value === "string" && FEEDBACK_SURFACES.has(value)); +} + +/** + * The summary is content-free, so it is validated by shape and bounds only: + * counts and token totals are non-negative integers, every string is a safe + * bounded identifier, and no unexpected key can smuggle free text in. + */ +function isStrictSummary(value: unknown): boolean { + if (value === undefined) return true; + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const summary = value as Record; + const allowed = new Set([ + "model", + "thinkingLevel", + "cwd", + "counts", + "usage", + "thinkingMs", + "recentErrors", + "lastTurnTools", + "features", + "request", + "compactions", + "compactionTotals", + "runtime", + "truncated", + ]); + if (Object.keys(summary).some((key) => !allowed.has(key))) return false; + if (summary.model !== undefined) { + const model = summary.model; + if (!model || typeof model !== "object" || Array.isArray(model)) return false; + const entry = model as Record; + if (Object.keys(entry).some((key) => !["provider", "id", "api"].includes(key))) return false; + if (!isSafeContextString(entry.provider, MAX_CONTEXT_VALUE_LENGTH)) return false; + if (!isSafeContextString(entry.id, MAX_CONTEXT_VALUE_LENGTH)) return false; + if (entry.api !== undefined && !isSafeContextString(entry.api, MAX_CONTEXT_VALUE_LENGTH)) return false; + } + for (const key of ["thinkingLevel", "cwd"] as const) { + if (summary[key] !== undefined && !isSafeContextString(summary[key], MAX_CONTEXT_VALUE_LENGTH)) return false; + } + if (!isStrictCounts(summary.counts)) return false; + if (!isEverySummaryRecord(summary.compactions, 20, isStrictSummaryCompaction)) return false; + if (summary.compactionTotals !== undefined && !isStrictCompactionTotals(summary.compactionTotals)) return false; + if (summary.runtime !== undefined && !isStrictRuntimeFacts(summary.runtime)) return false; + if (!isStrictUsage(summary.usage)) return false; + if (summary.thinkingMs !== undefined && !isCount(summary.thinkingMs)) return false; + if (!isEverySummaryRecord(summary.recentErrors, 20, isStrictSummaryError)) return false; + if (!isEverySummaryRecord(summary.lastTurnTools, 50, isStrictSummaryTool)) return false; + if (summary.features !== undefined) { + const features = summary.features; + if (!features || typeof features !== "object" || Array.isArray(features)) return false; + const entry = features as Record; + if (Object.keys(entry).some((key) => !["planMode", "planSource", "goal", "tasks"].includes(key))) return false; + if (entry.planMode !== undefined && typeof entry.planMode !== "boolean") return false; + for (const key of ["planSource", "goal"] as const) { + if (entry[key] !== undefined && !isSafeContextString(entry[key], MAX_CONTEXT_VALUE_LENGTH)) return false; + } + if (entry.tasks !== undefined && !isStrictCounts(entry.tasks)) return false; + } + if (summary.request !== undefined && !isStrictRequest(summary.request)) return false; + if (summary.truncated !== undefined && typeof summary.truncated !== "boolean") return false; + return true; +} + +function isStrictRequest(value: unknown): boolean { + if (value === undefined) return true; + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const request = value as Record; + const allowed = [ + "requestId", + "provider", + "model", + "api", + "reasoning", + "sourceLeafId", + "contextWindow", + "maxTokens", + "responseProvider", + "responseModel", + "statusCode", + "durationMs", + "ttftMs", + "outcome", + ]; + if (Object.keys(request).some((key) => !allowed.includes(key))) return false; + if (!isSafeContextString(request.requestId, MAX_CONTEXT_VALUE_LENGTH)) return false; + for (const key of [ + "provider", + "model", + "api", + "reasoning", + "sourceLeafId", + "responseProvider", + "responseModel", + ] as const) { + if (request[key] !== undefined && !isSafeContextString(request[key], MAX_CONTEXT_VALUE_LENGTH)) return false; + } + for (const key of ["contextWindow", "maxTokens", "statusCode", "durationMs"] as const) { + if (request[key] !== undefined && !isCount(request[key])) return false; + } + if (request.ttftMs !== undefined && request.ttftMs !== null && !isCount(request.ttftMs)) return false; + if ( + request.outcome !== undefined && + request.outcome !== "ok" && + request.outcome !== "http_error" && + request.outcome !== "transport_error" + ) + return false; + return true; +} + +/** Host facts: identifiers, counts, and the live context size. No URLs or free text. */ +function isStrictRuntimeFacts(value: unknown): boolean { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const facts = value as Record; + const allowed = ["permissionPreset", "contextUsed", "contextWindow", "toolCount", "skillCount", "mcpServerNames"]; + if (Object.keys(facts).some((key) => !allowed.includes(key))) return false; + if (facts.permissionPreset !== undefined && !isSafeContextString(facts.permissionPreset, MAX_CONTEXT_VALUE_LENGTH)) + return false; + for (const key of ["contextUsed", "contextWindow", "toolCount", "skillCount"] as const) { + if (facts[key] !== undefined && !isCount(facts[key])) return false; + } + if (facts.mcpServerNames !== undefined) { + if (!Array.isArray(facts.mcpServerNames) || facts.mcpServerNames.length > 32) return false; + if (!facts.mcpServerNames.every((name) => isSafeContextString(name, MAX_CONTEXT_VALUE_LENGTH))) return false; + } + return true; +} + +function isStrictCompactionTotals(value: unknown): boolean { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const totals = value as Record; + const allowed = ["passes", "resolved", "tokensBefore", "tokensAfter", "reclaimed"]; + if (Object.keys(totals).some((key) => !allowed.includes(key))) return false; + for (const key of allowed) { + const value = totals[key]; + if (value === undefined) continue; + // `reclaimed` is a difference: a compaction that did not shrink the window + // is negative, and that is exactly the case a report must still deliver. + if (key === "reclaimed") { + if (typeof value !== "number" || !Number.isFinite(value) || Math.abs(value) > 1e12) return false; + continue; + } + if (!isCount(value)) return false; + } + return true; +} + +function isStrictCounts(value: unknown): boolean { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const entry = value as Record; + const keys = Object.keys(entry); + if (keys.length === 0) return false; + for (const key of keys) { + if ( + ![ + "turns", + "assistantMessages", + "toolCalls", + "toolErrors", + "failedAttempts", + "abortedAttempts", + "truncatedAttempts", + "modelRequests", + "compactions", + "customEntries", + "completed", + "total", + ].includes(key) + ) + return false; + if (!isCount(entry[key])) return false; + } + return true; +} + +function isStrictUsage(value: unknown): boolean { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const entry = value as Record; + const keys = Object.keys(entry); + if (keys.length === 0) return false; + for (const key of keys) { + if (key === "costUsd") { + if (typeof entry[key] !== "number" || !Number.isFinite(entry[key] as number)) return false; + continue; + } + if (key === "cacheHitRate") { + const rate = entry[key]; + if (typeof rate !== "number" || !Number.isFinite(rate) || rate < 0 || rate > 1) return false; + continue; + } + if (!["input", "output", "cacheRead", "cacheWrite", "reasoning", "total"].includes(key)) return false; + if (!isCount(entry[key])) return false; + } + return true; +} + +function isCount(value: unknown): boolean { + return typeof value === "number" && Number.isFinite(value) && value >= 0 && value <= 1e12; +} + +/** + * The summary's free-text and identifier fields. + * + * A pending body can be edited after it was written, so this is the last place + * that can refuse one: an unbounded `message` or a `toolName` carrying markup + * would reach the collector as-is. + */ +const SUMMARY_ERROR_KEYS = new Set([ + "kind", + "toolName", + "stopReason", + "errorKind", + "message", + "durationMs", + "ttftMs", + "reached", + "outcome", + "at", +]); +const SUMMARY_TOOL_KEYS = new Set(["name", "isError"]); +const SUMMARY_COMPACTION_KEYS = new Set(["tokensBefore", "tokensAfter", "at", "unresolved"]); + +function isStrictSummaryError(value: unknown): boolean { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const error = value as Record; + if (Object.keys(error).some((key) => !SUMMARY_ERROR_KEYS.has(key))) return false; + if (error.kind !== "tool" && error.kind !== "model" && error.kind !== "aborted") return false; + for (const key of ["toolName", "stopReason", "errorKind", "reached", "outcome"] as const) { + if (error[key] !== undefined && !isSafeContextString(error[key], MAX_CONTEXT_VALUE_LENGTH)) return false; + } + // The provider's own text is the one long field; bounded like the comment. + if (error.message !== undefined && !isSafeContextString(error.message, FEEDBACK_COMMENT_MAX_RUNES)) return false; + for (const key of ["durationMs", "ttftMs"] as const) { + if (error[key] !== undefined && error[key] !== null && !isCount(error[key])) return false; + } + if (error.at !== undefined && (typeof error.at !== "string" || !Number.isFinite(Date.parse(error.at)))) return false; + return true; +} + +function isStrictSummaryTool(value: unknown): boolean { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const tool = value as Record; + if (Object.keys(tool).some((key) => !SUMMARY_TOOL_KEYS.has(key))) return false; + return isSafeContextString(tool.name, MAX_CONTEXT_VALUE_LENGTH) && typeof tool.isError === "boolean"; +} + +function isStrictSummaryCompaction(value: unknown): boolean { + if (!value || typeof value !== "object" || Array.isArray(value)) return false; + const pass = value as Record; + if (Object.keys(pass).some((key) => !SUMMARY_COMPACTION_KEYS.has(key))) return false; + if (!isCount(pass.tokensBefore)) return false; + if (pass.tokensAfter !== undefined && !isCount(pass.tokensAfter)) return false; + if (pass.at !== undefined && (typeof pass.at !== "string" || !Number.isFinite(Date.parse(pass.at)))) return false; + return pass.unresolved === undefined || typeof pass.unresolved === "boolean"; +} + +function isEverySummaryRecord(value: unknown, max: number, check: (record: unknown) => boolean): boolean { + return value === undefined || (Array.isArray(value) && value.length <= max && value.every(check)); +} diff --git a/packages/coding-agent/src/step/feedback/redact-home.ts b/packages/coding-agent/src/step/feedback/redact-home.ts new file mode 100644 index 000000000..5fb61943f --- /dev/null +++ b/packages/coding-agent/src/step/feedback/redact-home.ts @@ -0,0 +1,25 @@ +import { homedir } from "node:os"; + +/** + * Collapse the user's home directory to `~`. + * + * A report is read by someone else, so an absolute path leaks the OS user name + * on every line it appears. Collapsing keeps the path structure an investigator + * needs (`~/code/app/src/main.ts`) without carrying who the user is. Matching + * happens only at a path boundary, so `/Users/bob2` is not touched by a + * `/Users/bob` prefix. + */ +export function collapseHomePaths(value: string, home?: string): string { + const normalized = (home ?? homeDirectory()).replace(/[\\/]+$/u, ""); + if (normalized.length < 2) return value; + const escaped = normalized.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"); + return value.replace(new RegExp(`${escaped}(?=[\\\\/]|$|["'\\s])`, "gu"), "~"); +} + +function homeDirectory(): string { + try { + return homedir(); + } catch { + return ""; + } +} diff --git a/packages/coding-agent/src/step/feedback/row-metrics.ts b/packages/coding-agent/src/step/feedback/row-metrics.ts new file mode 100644 index 000000000..797ef71a4 --- /dev/null +++ b/packages/coding-agent/src/step/feedback/row-metrics.ts @@ -0,0 +1,242 @@ +/** + * Per-row metrics for the transcript copy inside a feedback bundle. + * + * A session line already carries the model's usage for that message, which is + * where a per-request cache hit rate comes from. Timing lives in the + * `step.usage_provenance` entry written right after the message, joined by + * `usageEntryId`. This module folds both onto the row they describe, so an + * investigator reads one line instead of correlating three. + * + * The annotations are written to the **bundle copy only**. The canonical + * session file is never rewritten: a report is a projection, and growing every + * session on disk to serve support would be the wrong trade. + */ + +import type { FeedbackRequestProvenance } from "./session-summary.ts"; + +/** Metrics attached to one transcript row under the `feedback` key. */ +export interface FeedbackRowMetrics { + /** 1-based turn the row belongs to (a user message opens a turn). */ + turn?: number; + model?: string; + provider?: string; + stopReason?: string; + /** Tokens this single request or result was billed for. */ + tokens?: { input: number; output: number; cacheRead: number; cacheWrite: number; total: number }; + /** cacheRead / (input + cacheRead) for this request alone. */ + cacheHitRate?: number; + costUsd?: number; + /** Wall-clock duration of the model request that produced this row. */ + durationMs?: number; + /** Time to the first stream event; absent when nothing arrived. */ + ttftMs?: number | null; + outcome?: "ok" | "http_error" | "transport_error"; + /** How far the model got, for an attempt that was cancelled or failed. */ + reached?: "thinking" | "text" | "toolCall"; + /** Host-observed thinking time for this message. */ + thinkingMs?: number; + /** The server-side request this row came from. */ + requestId?: string; + /** Tool result: call issued to result recorded, when the batch had one call. */ + elapsedMs?: number; +} + +const METRICS_KEY = "feedback"; + +/** + * Annotate a JSONL transcript excerpt. + * + * Line-oriented and tolerant: a line that is not a JSON object, or that has no + * metrics, is passed through byte-for-byte, so an annotated transcript is still + * a valid transcript. Returns the original text unchanged when nothing matched. + */ +export function annotateTranscriptRows(text: string): string { + if (text.length === 0) return text; + const rows = text.split("\n"); + const entries: Array | undefined> = rows.map((row) => parseRecord(row)); + const metrics = collectMetrics(entries); + if (metrics.size === 0) return text; + let annotated = 0; + const output = rows.map((row, index) => { + const entry = entries[index]; + if (!entry) return row; + const id = typeof entry.id === "string" ? entry.id : undefined; + const values = id ? metrics.get(id) : undefined; + if (!values) return row; + annotated += 1; + return JSON.stringify({ ...entry, [METRICS_KEY]: values }); + }); + // Preserve a trailing newline: the archive is read by tools that split on it. + const trailing = text.endsWith("\n"); + const joined = output.join("\n"); + return annotated === 0 ? text : trailing && !joined.endsWith("\n") ? `${joined}\n` : joined; +} + +function parseRecord(line: string): Record | undefined { + if (line.trim().length === 0) return undefined; + try { + const value: unknown = JSON.parse(line); + return value !== null && typeof value === "object" && !Array.isArray(value) + ? (value as Record) + : undefined; + } catch { + return undefined; + } +} + +function collectMetrics(entries: readonly (Record | undefined)[]): Map { + const metrics = new Map(); + /** requestId and timing by the assistant entry they belong to. */ + const requests = new Map(); + /** thinking time by the assistant message timestamp it was observed for. */ + const thinking = new Map(); + /** The assistant message that issued each pending tool call, for elapsed time. */ + const callOrigins = new Map(); + let turn = 0; + + // Requests and thinking time are written *after* the message they describe, + // so they are collected first and applied on a second pass. + for (const entry of entries) { + if (entry?.type === "custom") readCustom(entry); + } + + for (const entry of entries) { + if (!entry) continue; + if (entry.type !== "message") continue; + const message = asRecord(entry.message); + if (!message) continue; + const id = typeof entry.id === "string" ? entry.id : undefined; + const role = message.role; + if (role === "user") { + turn += 1; + continue; + } + if (role === "assistant") { + const values = assistantMetrics(message, id ? requests.get(id) : undefined); + const timestamp = typeof message.timestamp === "number" ? message.timestamp : undefined; + const thought = timestamp === undefined ? undefined : thinking.get(timestamp); + if (values || thought) { + metrics.set(id ?? `assistant-${metrics.size}`, { + ...(turn > 0 ? { turn } : {}), + ...(values ?? {}), + ...(thought ? { thinkingMs: thought } : {}), + }); + } + if (id && timestamp !== undefined) { + const calls = Array.isArray(message.content) + ? message.content.filter((block) => asRecord(block)?.type === "toolCall").length + : 0; + for (const block of Array.isArray(message.content) ? message.content : []) { + const call = asRecord(block); + if (call?.type !== "toolCall" || typeof call.id !== "string") continue; + callOrigins.set(call.id, { entryId: id, timestamp, batchSize: calls }); + } + } + continue; + } + if (role !== "toolResult") continue; + const origin = typeof message.toolCallId === "string" ? callOrigins.get(message.toolCallId) : undefined; + const timestamp = typeof message.timestamp === "number" ? message.timestamp : undefined; + // A parallel batch shares one start, so a per-call duration would be a guess. + const elapsed = + origin && origin.batchSize === 1 && timestamp !== undefined && timestamp >= origin.timestamp + ? timestamp - origin.timestamp + : undefined; + // A row with nothing to report is left exactly as it was. + if (id && elapsed !== undefined) metrics.set(id, { ...(turn > 0 ? { turn } : {}), elapsedMs: elapsed }); + } + + return metrics; + + function readCustom(entry: Record): void { + const data = asRecord(entry.data); + if (!data) return; + if (entry.customType === "step.usage_provenance") { + const entryId = data.usageEntryId; + const requestId = data.requestId; + if (typeof entryId !== "string" || typeof requestId !== "string") return; + requests.set(entryId, { + requestId, + ...(typeof data.requestedProvider === "string" ? { provider: data.requestedProvider } : {}), + ...(typeof data.requestedModel === "string" ? { model: data.requestedModel } : {}), + ...(typeof data.api === "string" ? { api: data.api } : {}), + ...(typeof data.reasoningAtDispatch === "string" ? { reasoning: data.reasoningAtDispatch } : {}), + ...(typeof data.durationMs === "number" ? { durationMs: data.durationMs } : {}), + ...(data.ttftMs === null || typeof data.ttftMs === "number" ? { ttftMs: data.ttftMs } : {}), + ...readOutcome(data.outcome), + }); + return; + } + if (entry.customType === "stepcode-web.thinking-timing") { + const messageTimestamp = data.messageTimestamp; + const timings = asRecord(data.thinkingTimings); + if (typeof messageTimestamp !== "number" || !timings) return; + let total = 0; + for (const timing of Object.values(timings)) { + const value = asRecord(timing); + if (typeof value?.startedAt !== "number" || typeof value.endedAt !== "number") continue; + const elapsed = value.endedAt - value.startedAt; + if (elapsed > 0) total += elapsed; + } + if (total > 0) thinking.set(messageTimestamp, total); + } + } +} + +function assistantMetrics( + message: Record, + request: FeedbackRequestProvenance | undefined, +): FeedbackRowMetrics | undefined { + const usage = asRecord(message.usage); + const tokens = usage + ? { + input: nonNegative(usage.input), + output: nonNegative(usage.output), + cacheRead: nonNegative(usage.cacheRead), + cacheWrite: nonNegative(usage.cacheWrite), + total: nonNegative(usage.totalTokens ?? usage.total), + } + : undefined; + const cacheable = tokens ? tokens.input + tokens.cacheRead : 0; + const cost = asRecord(usage?.cost); + const values: FeedbackRowMetrics = { + ...(tokens ? { tokens, cacheHitRate: cacheable > 0 ? tokens.cacheRead / cacheable : 0 } : {}), + ...(typeof cost?.total === "number" && Number.isFinite(cost.total) ? { costUsd: cost.total } : {}), + ...(typeof message.model === "string" ? { model: message.model } : {}), + ...(typeof message.provider === "string" ? { provider: message.provider } : {}), + ...(typeof message.stopReason === "string" ? { stopReason: message.stopReason } : {}), + ...(request?.requestId ? { requestId: request.requestId } : {}), + ...(typeof request?.durationMs === "number" ? { durationMs: request.durationMs } : {}), + ...(request?.ttftMs === null || typeof request?.ttftMs === "number" ? { ttftMs: request.ttftMs } : {}), + ...(request?.outcome ? { outcome: request.outcome } : {}), + ...(message.stopReason === "aborted" || message.stopReason === "error" ? readReached(message) : {}), + }; + return Object.keys(values).length > 0 ? values : undefined; +} + +function readReached(record: Record): Pick { + const content = record.content; + if (!Array.isArray(content)) return {}; + let reached: "thinking" | "text" | "toolCall" | undefined; + for (const part of content) { + const type = asRecord(part)?.type; + if (type === "toolCall") return { reached: "toolCall" }; + if (type === "text") reached = "text"; + else if (type === "thinking" && reached === undefined) reached = "thinking"; + } + return reached ? { reached } : {}; +} + +function readOutcome(value: unknown): Pick { + return value === "ok" || value === "http_error" || value === "transport_error" ? { outcome: value } : {}; +} + +function asRecord(value: unknown): Record | undefined { + return value !== null && typeof value === "object" && !Array.isArray(value) + ? (value as Record) + : undefined; +} + +function nonNegative(value: unknown): number { + return typeof value === "number" && Number.isFinite(value) && value > 0 ? value : 0; +} diff --git a/packages/coding-agent/src/step/feedback/session-summary.ts b/packages/coding-agent/src/step/feedback/session-summary.ts new file mode 100644 index 000000000..f25009da3 --- /dev/null +++ b/packages/coding-agent/src/step/feedback/session-summary.ts @@ -0,0 +1,690 @@ +/** + * A content-free summary of the session a feedback report is about. + * + * The transcript itself is attached separately. This summary answers "in what + * state was the session when it went wrong" without a single character of user + * content: which model and effort were in use, how many turns and tool calls + * had run, which ones failed, what was spent, and which Step features were + * active. Investigations start here, then read the transcript only for the + * turn that matters. + * + * Derived from the persisted session entries, so the CLI (a file on disk), the + * TUI and a host holding the branch in memory all produce the same values. + */ + +import { createReadStream } from "node:fs"; +import fs from "node:fs/promises"; +import { createInterface } from "node:readline"; +import type { Usage } from "@step-harness/providers"; +import type { SessionEntry } from "../../core/session-manager.ts"; +import { redactSecretString } from "../secret-redaction.ts"; +import { collapseHomePaths } from "./redact-home.ts"; + +/** How many failed calls keep their identity in the summary. */ +const RECENT_ERROR_LIMIT = 10; +/** How many tool calls of the latest turn are kept; enough to see the shape of a run. */ +const LAST_TURN_TOOL_LIMIT = 50; +/** How many compactions keep their before/after sizes. */ +const COMPACTION_LIMIT = 20; +/** Guard against a pathological file: the summary never needs more lines than this. */ +const MAX_SCANNED_LINES = 200_000; + +export interface FeedbackRequestProvenance { + /** Correlates this report with the model request in server-side logs. */ + requestId: string; + provider?: string; + model?: string; + /** API dialect, which selects the adapter and its compat switches. */ + api?: string; + reasoning?: string; + sourceLeafId?: string | null; + /** Catalog limits at dispatch. */ + contextWindow?: number; + maxTokens?: number; + /** Who answered, when auth or a proxy redirected the request. */ + responseProvider?: string; + responseModel?: string; + statusCode?: number; + durationMs?: number; + /** Absent means the stream produced no event before it settled. */ + ttftMs?: number | null; + outcome?: "ok" | "http_error" | "transport_error"; +} + +export interface FeedbackSessionError { + /** + * `tool` is a failed tool call, `model` a failed attempt the user did not + * stop, `aborted` an attempt the user (or the host) stopped. The last kind is + * how "it hung, so I cancelled" reaches a report. + */ + kind: "tool" | "model" | "aborted"; + toolName?: string; + stopReason?: string; + /** Structured provider failure class: rate_limit, quota, auth, transient, invalid_request. */ + errorKind?: string; + /** + * The provider's own text, which for an HTTP failure carries the status and + * the response body. Bounded and secret-redacted; this is what names a + * rejected request ("concurrency reached, current: 11, limit: 10"). + */ + message?: string; + /** How long the attempt ran before it settled. */ + durationMs?: number; + /** Time to the first stream event; absent means nothing ever arrived. */ + ttftMs?: number | null; + /** How far it got: `thinking` means it was cancelled before any output. */ + reached?: "thinking" | "text" | "toolCall"; + /** Which side of the wire failed; a user stop is a transport outcome. */ + outcome?: "ok" | "http_error" | "transport_error"; + at?: string; +} + +/** + * One compaction, before and after. + * + * A long session compacts repeatedly; each row is one pass. `tokensAfter` is the + * input size of the first request that followed, so a compaction that failed to + * shrink the window shows up as a row whose two numbers are close. `unresolved` + * marks the newest pass when no request followed it yet. + */ +export interface FeedbackCompaction { + tokensBefore: number; + tokensAfter?: number; + at?: string; + unresolved?: boolean; +} + +/** Facts only a live host knows: the session file records none of them. */ +export interface FeedbackRuntimeFacts { + /** ask | approve-for-me | full-access. */ + permissionPreset?: string; + /** Context tokens the host currently counts (may differ from the last request). */ + contextUsed?: number | null; + contextWindow?: number; + toolCount?: number; + skillCount?: number; + /** Configured MCP server names; identifiers only, never URLs or headers. */ + mcpServerNames?: string[]; +} + +export interface FeedbackSessionSummary { + model?: { provider: string; id: string; api?: string }; + thinkingLevel?: string; + /** Home directory collapsed to `~`; absent when the session header had none. */ + cwd?: string; + counts: { + turns: number; + assistantMessages: number; + toolCalls: number; + toolErrors: number; + failedAttempts: number; + /** Attempts the user stopped, not failures of their own. */ + abortedAttempts: number; + /** Attempts that hit the model's output limit (`stopReason: "length"`). */ + truncatedAttempts: number; + /** Model requests recorded on the branch (`step.usage_provenance`). */ + modelRequests: number; + compactions: number; + customEntries: number; + }; + usage: { + input: number; + output: number; + cacheRead: number; + cacheWrite: number; + reasoning: number; + total: number; + /** Provider-reported cost, when the catalog has pricing for the model. */ + costUsd?: number; + /** cacheRead / (input + cacheRead): a low rate explains slow or costly turns. */ + cacheHitRate?: number; + }; + /** + * Before and after each compaction, oldest first, bounded to the last 20. + * `compactionTotals` summarises every pass, including ones dropped from the + * array, so a long session is still readable at a glance. + */ + compactions: FeedbackCompaction[]; + /** + * Every pass is counted in `passes`; the three size sums cover only passes a + * request followed, so `tokensBefore - tokensAfter === reclaimed` always holds. + * A pass still waiting on a request has no "after" and is excluded. + */ + compactionTotals?: { + passes: number; + resolved: number; + tokensBefore: number; + tokensAfter: number; + reclaimed: number; + }; + /** Host-supplied state the session file does not carry (permission tier, live context size). */ + runtime?: FeedbackRuntimeFacts; + /** Thinking time the host observed, when it recorded any. */ + thinkingMs?: number; + recentErrors: FeedbackSessionError[]; + /** The most recent turn's tool calls, in call order; names and outcomes only. */ + lastTurnTools: { name: string; isError: boolean }[]; + features: { + planMode?: boolean; + planSource?: string; + goal?: string; + tasks?: { completed: number; total: number }; + }; + /** The newest model request recorded on the branch, for server-side correlation. */ + request?: FeedbackRequestProvenance; + /** The scan stopped at the line cap; counts cover only the newest lines and may be partial. */ + truncated?: boolean; +} + +/** `home` is what `cwd` is collapsed against; tests and sandboxed hosts override it. */ +export interface FeedbackSummaryOptions { + readonly home?: string; + /** Live host state the session file cannot provide. */ + readonly runtime?: FeedbackRuntimeFacts; +} + +/** + * Summarize the session **branch** the file ends on. + * + * Session files are append-only trees: navigating back, undoing, or forking + * leaves abandoned entries in the same file, and a fork copies provenance + * entries in. Counting every line would include turns the user rewound, pair a + * compaction with a request from another branch, and report a `requestId` from + * a branch nobody is looking at. So the last entry is followed back through + * `parentId` to the root, and only that chain is summarized. + */ +export async function summarizeSessionFile( + sessionFile: string, + options: FeedbackSummaryOptions = {}, +): Promise { + const stats = await fs.stat(sessionFile).catch(() => undefined); + if (!stats?.isFile() || stats.size === 0) return undefined; + const entries: (Record | undefined)[] = []; + let truncated = false; + const lines = createInterface({ + input: createReadStream(sessionFile, "utf8"), + crlfDelay: Number.POSITIVE_INFINITY, + }); + try { + for await (const line of lines) { + entries.push(parseRecord(parseLine(line))); + // A pathological file keeps its newest lines: the request and the error + // a report is about are at the end, not the beginning. Evict in chunks; + // one shift per line is quadratic on a file that is already too big. + if (entries.length > MAX_SCANNED_LINES * 2) { + entries.splice(0, MAX_SCANNED_LINES); + truncated = true; + } + } + } finally { + lines.close(); + } + const summary = createSummary(options); + if (truncated) { + // The branch cannot be walked across a gap, and the newest lines are the + // ones a report needs, so keep the tail and say the counts are partial. + summary.truncated = true; + for (const entry of entries) summary.accept(entry); + return summary.finish(); + } + for (const entry of branchOf(entries)) summary.accept(entry); + return summary.finish(); +} + +/** The chain the file ends on, oldest first. */ +function branchOf(entries: readonly (Record | undefined)[]): Record[] { + const byId = new Map>(); + for (const entry of entries) { + if (entry && typeof entry.id === "string") byId.set(entry.id, entry); + } + // A trailing blank line or a crash-truncated final record leaves `undefined` + // at the end; the leaf is the last entry that actually parsed. + let endIndex = entries.length - 1; + while (endIndex >= 0 && entries[endIndex] === undefined) endIndex -= 1; + const last = endIndex >= 0 ? entries[endIndex] : undefined; + if (!last) return []; + const chain: Record[] = []; + const seen = new Set(); + let current: Record | undefined = last; + while (current) { + const id = typeof current.id === "string" ? current.id : undefined; + if (id && seen.has(id)) break; + if (id) seen.add(id); + chain.push(current); + const parent: unknown = current.parentId; + current = typeof parent === "string" ? byId.get(parent) : undefined; + } + // A file with no `parentId` links (older format, or a single entry) is its + // own branch; keep file order rather than inventing a chain. + return chain.length <= 1 && entries.length > 1 ? entries.filter(isRecord) : chain.reverse(); +} + +function isRecord(value: Record | undefined): value is Record { + return value !== undefined; +} + +function parseRecord(value: unknown): Record | undefined { + return value !== null && typeof value === "object" && !Array.isArray(value) + ? (value as Record) + : undefined; +} + +/** Summarize entries a host already holds in memory (the same values as the file). */ +export function summarizeSessionEntries( + entries: readonly SessionEntry[], + options: FeedbackSummaryOptions = {}, +): FeedbackSessionSummary { + const summary = createSummary(options); + for (const entry of entries) summary.accept(entry); + return summary.finish(); +} + +function parseLine(line: string): unknown { + if (line.trim().length === 0) return undefined; + try { + return JSON.parse(line); + } catch { + // One unreadable line is not a reason to lose every count around it. + return undefined; + } +} + +function createSummary(options: FeedbackSummaryOptions): { + accept(entry: unknown): void; + finish(): FeedbackSessionSummary; + truncated: boolean; +} { + const counts = { + turns: 0, + assistantMessages: 0, + toolCalls: 0, + toolErrors: 0, + failedAttempts: 0, + abortedAttempts: 0, + truncatedAttempts: 0, + modelRequests: 0, + compactions: 0, + customEntries: 0, + }; + const usage = { input: 0, output: 0, cacheRead: 0, cacheWrite: 0, reasoning: 0, total: 0 }; + let costUsd: number | undefined; + const compactions: FeedbackCompaction[] = []; + /** + * Compactions still waiting for the next request's input size. The list, not + * a single slot, so back-to-back passes all resolve instead of the earlier + * ones silently staying unresolved. + */ + let unresolved: FeedbackCompaction[] = []; + const compactionTotals = { passes: 0, resolved: 0, tokensBefore: 0, tokensAfter: 0, reclaimed: 0 }; + const state: { + model?: FeedbackSessionSummary["model"]; + thinkingLevel?: string; + cwd?: string; + thinkingMs?: number; + planMode?: boolean; + planSource?: string; + goal?: string; + tasks?: { completed: number; total: number }; + request?: FeedbackRequestProvenance; + truncated: boolean; + } = { truncated: false }; + const recentErrors: FeedbackSessionError[] = []; + /** + * Failed or cancelled attempts by session entry id. The timing lives in the + * `step.usage_provenance` entry that follows the message, so the two are + * joined here rather than left as a count. + */ + const attemptsByEntry = new Map(); + /** One row per call in the latest turn; a result resolves the row it belongs to. */ + const lastTurnTools: { id?: string; name: string; isError: boolean }[] = []; + const callRows = new Map(); + + const pushError = (error: FeedbackSessionError, entryId?: string): void => { + recentErrors.push(error); + if (recentErrors.length > RECENT_ERROR_LIMIT) recentErrors.shift(); + if (entryId) attemptsByEntry.set(entryId, { error }); + }; + + const pushCall = (id: string | undefined, name: string): void => { + if (id) callRows.set(id, lastTurnTools.length); + lastTurnTools.push({ ...(id ? { id } : {}), name, isError: false }); + }; + + const resolveCall = (id: string | undefined, name: string, isError: boolean): void => { + const row = id ? callRows.get(id) : undefined; + if (row === undefined) { + lastTurnTools.push({ ...(id ? { id } : {}), name, isError }); + return; + } + lastTurnTools[row] = { ...lastTurnTools[row], isError }; + }; + + const acceptCustom = (record: Record): void => { + const data = record.data; + if (!data || typeof data !== "object") return; + const payload = data as Record; + switch (record.customType) { + case "step-plan": + if (typeof payload.enabled === "boolean") state.planMode = payload.enabled; + if (typeof payload.planSource === "string") state.planSource = payload.planSource; + return; + case "step-goal": + if (typeof payload.status === "string") state.goal = payload.status; + return; + case "step-tasks": { + const list = Array.isArray(payload.tasks) ? payload.tasks : []; + let total = 0; + let completed = 0; + for (const task of list) { + const status = (task as { status?: unknown } | undefined)?.status; + if (status === "deleted") continue; + total += 1; + if (status === "completed") completed += 1; + } + if (total > 0) state.tasks = { completed, total }; + return; + } + case "step.usage_provenance": { + const requestId = payload.requestId; + if (typeof requestId !== "string" || !requestId) return; + counts.modelRequests += 1; + // The timing of the attempt this provenance belongs to. + const attempt = + typeof payload.usageEntryId === "string" ? attemptsByEntry.get(payload.usageEntryId) : undefined; + if (attempt) { + if (typeof payload.durationMs === "number" && Number.isFinite(payload.durationMs)) { + attempt.error.durationMs = payload.durationMs; + } + if (payload.ttftMs === null || typeof payload.ttftMs === "number") { + attempt.error.ttftMs = payload.ttftMs; + } + const outcome = readOutcome(payload.outcome); + if (outcome) attempt.error.outcome = outcome; + } + state.request = { + requestId, + ...(typeof payload.requestedProvider === "string" ? { provider: payload.requestedProvider } : {}), + ...(typeof payload.requestedModel === "string" ? { model: payload.requestedModel } : {}), + ...(typeof payload.api === "string" ? { api: payload.api } : {}), + ...(typeof payload.reasoningAtDispatch === "string" ? { reasoning: payload.reasoningAtDispatch } : {}), + ...(typeof payload.sourceLeafId === "string" ? { sourceLeafId: payload.sourceLeafId } : {}), + ...(isPositiveNumber(payload.contextWindow) ? { contextWindow: payload.contextWindow } : {}), + ...(isPositiveNumber(payload.maxTokens) ? { maxTokens: payload.maxTokens } : {}), + ...(typeof payload.responseProvider === "string" ? { responseProvider: payload.responseProvider } : {}), + ...(typeof payload.responseModel === "string" ? { responseModel: payload.responseModel } : {}), + ...(isPositiveNumber(payload.statusCode) ? { statusCode: payload.statusCode } : {}), + ...(isPositiveNumber(payload.durationMs) ? { durationMs: payload.durationMs } : {}), + ...(payload.ttftMs === null || isPositiveNumber(payload.ttftMs) ? { ttftMs: payload.ttftMs } : {}), + ...(readOutcome(payload.outcome) ? { outcome: readOutcome(payload.outcome) } : {}), + }; + return; + } + case "stepcode-web.thinking-timing": { + const timings = payload.thinkingTimings; + if (!timings || typeof timings !== "object") return; + for (const timing of Object.values(timings as Record)) { + const value = timing as { startedAt?: unknown; endedAt?: unknown } | null; + if (typeof value?.startedAt !== "number" || typeof value.endedAt !== "number") continue; + const elapsed = value.endedAt - value.startedAt; + if (elapsed > 0) state.thinkingMs = (state.thinkingMs ?? 0) + elapsed; + } + return; + } + default: + return; + } + }; + + const addUsage = (value: unknown): void => { + if (!value || typeof value !== "object") return; + const record = value as Partial & Record; + for (const key of ["input", "output", "cacheRead", "cacheWrite", "reasoning"] as const) { + const amount = record[key]; + if (typeof amount === "number" && Number.isFinite(amount)) usage[key] += amount; + } + const total = record.totalTokens ?? record.total; + if (typeof total === "number" && Number.isFinite(total)) usage.total += total; + const cost = record.cost as { total?: unknown } | undefined; + if (typeof cost?.total === "number" && Number.isFinite(cost.total)) { + costUsd = (costUsd ?? 0) + cost.total; + } + // The first request after a compaction reports the context that is left. + if (unresolved.length > 0 && typeof record.input === "number" && Number.isFinite(record.input)) { + const cacheRead = typeof record.cacheRead === "number" ? record.cacheRead : 0; + for (const pass of unresolved) { + pass.tokensAfter = record.input + cacheRead; + compactionTotals.resolved += 1; + compactionTotals.tokensBefore += pass.tokensBefore; + compactionTotals.tokensAfter += pass.tokensAfter; + compactionTotals.reclaimed += pass.tokensBefore - pass.tokensAfter; + } + unresolved = []; + } + }; + + const acceptAssistant = (record: Record, entryId?: string): void => { + counts.assistantMessages += 1; + const provider = record.provider; + const id = record.model; + if (typeof provider === "string" && typeof id === "string") { + state.model = { provider, id, ...(typeof record.api === "string" ? { api: record.api } : {}) }; + } + addUsage(record.usage); + if (record.stopReason === "aborted") { + counts.abortedAttempts += 1; + pushError({ ...attemptTiming(record), kind: "aborted", stopReason: "aborted" }, entryId); + } + if (record.stopReason === "length") counts.truncatedAttempts += 1; + if (record.stopReason === "error") { + counts.failedAttempts += 1; + const message = record.errorMessage; + pushError( + { + kind: "model", + stopReason: "error", + ...attemptTiming(record), + ...(typeof record.errorKind === "string" ? { errorKind: record.errorKind } : {}), + // The provider's own text: for an HTTP failure it carries the status + // and the rejected-request body, which is the whole diagnosis. + ...(typeof message === "string" && message.trim() + ? { message: boundErrorText(collapseHomePaths(redactSecretString(message), options.home)) } + : {}), + }, + entryId, + ); + } + const content = record.content; + if (!Array.isArray(content)) return; + for (const part of content) { + if (!part || typeof part !== "object") continue; + const block = part as Record; + if (block.type !== "toolCall") continue; + counts.toolCalls += 1; + if (typeof block.name === "string") { + pushCall(typeof block.id === "string" ? block.id : undefined, block.name); + } + } + }; + + const acceptMessage = (message: unknown, entryId?: string): void => { + if (!message || typeof message !== "object") return; + const record = message as Record; + if (record.role === "user") { + counts.turns += 1; + lastTurnTools.length = 0; + callRows.clear(); + return; + } + if (record.role === "toolResult") { + const isError = record.isError === true; + const toolName = typeof record.toolName === "string" ? record.toolName : undefined; + resolveCall( + typeof record.toolCallId === "string" ? record.toolCallId : undefined, + toolName ?? "unknown", + isError, + ); + if (!isError) return; + counts.toolErrors += 1; + pushError({ + kind: "tool", + ...(toolName ? { toolName } : {}), + ...(typeof record.timestamp === "number" ? { at: new Date(record.timestamp).toISOString() } : {}), + }); + return; + } + if (record.role === "assistant") acceptAssistant(record, entryId); + }; + + return { + get truncated(): boolean { + return state.truncated; + }, + set truncated(value: boolean) { + state.truncated = value; + }, + accept(entry: unknown): void { + if (!entry || typeof entry !== "object") return; + const record = entry as Record; + switch (record.type) { + case "session": + if (typeof record.cwd === "string" && record.cwd) { + state.cwd = collapseHomePaths(record.cwd, options.home); + } + return; + case "model_change": + if (typeof record.provider === "string" && typeof record.modelId === "string") { + state.model = { provider: record.provider, id: record.modelId }; + } + return; + case "thinking_level_change": + if (typeof record.thinkingLevel === "string") state.thinkingLevel = record.thinkingLevel; + return; + case "compaction": { + counts.compactions += 1; + if (typeof record.tokensBefore === "number" && Number.isFinite(record.tokensBefore)) { + const entry: FeedbackCompaction = { + tokensBefore: record.tokensBefore, + ...(typeof record.timestamp === "string" ? { at: record.timestamp } : {}), + }; + compactions.push(entry); + if (compactions.length > COMPACTION_LIMIT) compactions.shift(); + unresolved.push(entry); + compactionTotals.passes += 1; + } + return; + } + case "custom": + counts.customEntries += 1; + acceptCustom(record); + return; + case "message": + acceptMessage(record.message, typeof record.id === "string" ? record.id : undefined); + return; + default: + return; + } + }, + finish(): FeedbackSessionSummary { + // A pass no request followed yet has no "after": say so rather than + // leaving a reader to guess whether it shrank. + for (const pass of unresolved) pass.unresolved = true; + const cacheable = usage.input + usage.cacheRead; + const runtime = options.runtime; + const contextWindow = runtime?.contextWindow ?? state.request?.contextWindow; + const model = state.model ?? requestModelOf(state.request); + return { + ...(model ? { model } : {}), + ...(state.thinkingLevel ? { thinkingLevel: state.thinkingLevel } : {}), + ...(state.cwd ? { cwd: state.cwd } : {}), + counts, + usage: { + ...usage, + ...(costUsd !== undefined ? { costUsd } : {}), + ...(cacheable > 0 ? { cacheHitRate: usage.cacheRead / cacheable } : {}), + }, + compactions, + ...(compactionTotals.passes > 0 ? { compactionTotals: { ...compactionTotals } } : {}), + ...(runtime ? { runtime: runtimeFacts(runtime, contextWindow) } : {}), + ...(state.thinkingMs && state.thinkingMs > 0 ? { thinkingMs: state.thinkingMs } : {}), + recentErrors, + lastTurnTools: lastTurnTools.slice(-LAST_TURN_TOOL_LIMIT).map(({ name, isError }) => ({ name, isError })), + features: { + ...(state.planMode === undefined ? {} : { planMode: state.planMode }), + ...(state.planSource ? { planSource: state.planSource } : {}), + ...(state.goal ? { goal: state.goal } : {}), + ...(state.tasks ? { tasks: state.tasks } : {}), + }, + ...(state.request ? { request: state.request } : {}), + ...(state.truncated ? { truncated: true } : {}), + }; + }, + }; +} + +function isPositiveNumber(value: unknown): value is number { + return typeof value === "number" && Number.isFinite(value) && value > 0; +} + +/** `model` may only carry provider/id/api; the window comes from the request or the host. */ +function requestModelOf(request: FeedbackRequestProvenance | undefined): FeedbackSessionSummary["model"] | undefined { + if (!request?.provider || !request.model) return undefined; + return { + provider: request.provider, + id: request.model, + ...(request.api ? { api: request.api } : {}), + }; +} + +function runtimeFacts(facts: FeedbackRuntimeFacts, contextWindow: number | undefined): FeedbackRuntimeFacts { + const window = facts.contextWindow ?? contextWindow; + const used = facts.contextUsed; + return { + ...(facts.permissionPreset ? { permissionPreset: facts.permissionPreset } : {}), + ...(typeof used === "number" && Number.isFinite(used) ? { contextUsed: used } : {}), + ...(window && window > 0 ? { contextWindow: window } : {}), + ...(facts.toolCount !== undefined ? { toolCount: facts.toolCount } : {}), + ...(facts.skillCount !== undefined ? { skillCount: facts.skillCount } : {}), + ...(facts.mcpServerNames?.length ? { mcpServerNames: facts.mcpServerNames.slice(0, 32) } : {}), + }; +} + +/** + * What a settled message itself records: when it settled and how far it got. + * Duration and time-to-first-token are not on the message — they are written to + * the `step.usage_provenance` entry that follows and joined by entry id. + */ +function attemptTiming(record: Record): Pick { + return { + ...(typeof record.timestamp === "number" ? { at: new Date(record.timestamp).toISOString() } : {}), + ...reachedOf(record), + }; +} + +/** The furthest block kind the attempt produced, when the message still has it. */ +function reachedOf(record: Record): Pick { + const content = record.content; + if (!Array.isArray(content)) return {}; + let reached: "thinking" | "text" | "toolCall" | undefined; + for (const part of content) { + const type = (part as { type?: unknown } | undefined)?.type; + if (type === "toolCall") return { reached: "toolCall" }; + if (type === "text") reached = "text"; + else if (type === "thinking" && reached === undefined) reached = "thinking"; + } + return reached ? { reached } : {}; +} + +/** One provider error line is enough to name the failure; the rest is noise. */ +const ERROR_TEXT_MAX_CHARS = 600; + +function boundErrorText(value: string): string { + const text = value.trim().replace(/\s+/gu, " "); + const characters = [...text]; + return characters.length <= ERROR_TEXT_MAX_CHARS + ? text + : `${characters.slice(0, ERROR_TEXT_MAX_CHARS).join("")}… [truncated]`; +} + +function readOutcome(value: unknown): "ok" | "http_error" | "transport_error" | undefined { + return value === "ok" || value === "http_error" || value === "transport_error" ? value : undefined; +} diff --git a/packages/coding-agent/src/step/feedback/submission.ts b/packages/coding-agent/src/step/feedback/submission.ts index a9c58aefe..e0f33f4c6 100644 --- a/packages/coding-agent/src/step/feedback/submission.ts +++ b/packages/coding-agent/src/step/feedback/submission.ts @@ -1,7 +1,13 @@ import { randomUUID } from "node:crypto"; import { redactSecretString } from "../secret-redaction.ts"; import { resolveFeedbackContext } from "./context.ts"; -import type { FeedbackDiagnostics, FeedbackSubmission } from "./types.ts"; +import type { FeedbackRequestProvenance, FeedbackSessionSummary } from "./session-summary.ts"; +import { + FEEDBACK_SCHEMA_VERSION, + type FeedbackDiagnostics, + type FeedbackSubmission, + type FeedbackSurface, +} from "./types.ts"; import { redactFeedbackDiagnostics, validateFeedbackInput } from "./validate.ts"; export type BuildFeedbackSubmissionResult = { ok: true; submission: FeedbackSubmission } | { ok: false; error: string }; @@ -10,6 +16,8 @@ export async function buildFeedbackSubmission(input: { category?: string; comment: string; diagnostics?: FeedbackDiagnostics; + sessionSummary?: FeedbackSessionSummary; + surface?: FeedbackSurface; storageRootDir: string; sessionId?: string; uid?: string; @@ -21,21 +29,34 @@ export async function buildFeedbackSubmission(input: { const validation = validateFeedbackInput({ category: input.category, comment: input.comment }); if (!validation.ok) return validation; const diagnostics = input.diagnostics ? redactFeedbackDiagnostics(input.diagnostics) : undefined; + // The request id is repeated at the top level: a collector indexes it without + // walking the summary, and an older client that sends neither field is still + // a valid report. + const request = input.sessionSummary?.request; return { ok: true, submission: { + schemaVersion: FEEDBACK_SCHEMA_VERSION, + // A host states its surface; an omitted one stays absent rather than + // being mislabelled as the CLI. + ...(input.surface ? { surface: input.surface } : {}), feedbackId: input.feedbackId ?? randomUUID(), ...(validation.category ? { category: validation.category } : {}), comment: redactSecretString(validation.comment), at: (input.at ?? new Date()).toISOString(), context: await resolveFeedbackContext({ storageRootDir: input.storageRootDir, + ...(input.surface ? { surface: input.surface } : {}), ...(input.sessionId ? { sessionId: input.sessionId } : {}), ...(input.uid ? { uid: input.uid } : {}), ...(input.username ? { username: input.username } : {}), ...(input.env ? { env: input.env } : {}), }), ...(diagnostics ? { diagnostics } : {}), + ...(input.sessionSummary ? { sessionSummary: input.sessionSummary } : {}), + ...(request ? { request } : {}), }, }; } + +export type { FeedbackRequestProvenance }; diff --git a/packages/coding-agent/src/step/feedback/types.ts b/packages/coding-agent/src/step/feedback/types.ts index cce2e2a12..a046b7b05 100644 --- a/packages/coding-agent/src/step/feedback/types.ts +++ b/packages/coding-agent/src/step/feedback/types.ts @@ -1,3 +1,5 @@ +import type { FeedbackRequestProvenance, FeedbackSessionSummary } from "./session-summary.ts"; + export const FEEDBACK_CATEGORIES = ["bug", "bad_result", "good_result", "safety_check", "other"] as const; export type FeedbackCategory = (typeof FEEDBACK_CATEGORIES)[number]; @@ -35,10 +37,47 @@ export const FEEDBACK_BUNDLE_CONTENT_TYPE = "application/gzip"; */ export const FEEDBACK_SESSION_RECENCY_WINDOW_MS = 10 * 60 * 1000; +/** Which product surface filed the report. A host states it; it is never guessed. */ +export type FeedbackSurface = "cli" | "tui" | "web" | "desktop" | "sdk"; + +/** Bumped when the report shape gains fields a consumer may rely on. Additive only. */ +export const FEEDBACK_SCHEMA_VERSION = 2; + +/** + * The runtime that produced the report. A native-module, ESM or packaged-app + * failure is only diagnosable with these, and they are not derivable from the + * product version. + */ +export interface FeedbackRuntime { + /** Node version, also set when Node runs inside Electron (Electron's mode). */ + node?: string; + /** Present only in the Desktop build. */ + electron?: string; + /** Present only in a Bun-compiled binary. */ + bun?: string; +} + export interface FeedbackContext { channel: string; version: string; + /** + * Where `version` came from: `override` (local env), `embedded` (release + * pipeline) or `fallback` (a source build with nothing injected). A fallback + * version does not identify a release, so triage must know. + */ + versionSource?: "override" | "embedded" | "fallback"; + /** ISO timestamp of the artifact, when the pipeline embedded one. */ + buildTime?: string; + /** The interpreter/host the code ran under. */ + runtime?: FeedbackRuntime; platform: string; + /** `process.arch`, so a native-module or binary report names the real target. */ + arch?: string; + /** `os.release()`, which `platform` alone does not carry. */ + osRelease?: string; + locale?: string; + /** npm, brew, standalone, dmg, nsis — upgrade and path problems start here. */ + installSource?: string; commit?: string; sessionId?: string; deviceId?: string; @@ -57,12 +96,25 @@ export interface FeedbackDiagnostics { } export interface FeedbackSubmission { + /** + * Absent on reports written before v2 (a pending body on disk, an older + * client). Readers must treat a missing value as v1, never reject it. + */ + schemaVersion?: number; + surface?: FeedbackSurface; feedbackId: string; category?: FeedbackCategory; comment: string; at: string; context: FeedbackContext; diagnostics?: FeedbackDiagnostics; + /** Content-free session state; see `session-summary.ts`. */ + sessionSummary?: FeedbackSessionSummary; + /** + * The newest model request id, repeated at the top level so a collector can + * index it without parsing the summary. + */ + request?: FeedbackRequestProvenance; } export interface FeedbackBundle { diff --git a/packages/coding-agent/src/step/slash-commands.ts b/packages/coding-agent/src/step/slash-commands.ts index 689e6db5c..72844023f 100644 --- a/packages/coding-agent/src/step/slash-commands.ts +++ b/packages/coding-agent/src/step/slash-commands.ts @@ -15,7 +15,6 @@ import { } from "../render/btw-panel.ts"; import { getAvailableThemes } from "../theme/theme.ts"; import { copyToClipboard } from "../utils/clipboard.ts"; -import { buildFeedbackSessionBundle } from "./feedback/bundle.ts"; import { type FeedbackSubmitResult, formatFeedbackBundleFailureDetails, @@ -24,9 +23,9 @@ import { formatFeedbackSubmittedMessage, submitFeedback, } from "./feedback/command.ts"; -import { confirmFeedbackSubmission, neutralizeFeedbackConsentMetadata } from "./feedback/consent.ts"; -import { readFeedbackDiagnostics } from "./feedback/diagnostics.ts"; +import { confirmFeedbackSubmission } from "./feedback/consent.ts"; import { resolveFeedbackEndpoint } from "./feedback/endpoints.ts"; +import { prepareFeedbackEvidence } from "./feedback/evidence.ts"; import { resolveFeedbackSettings } from "./feedback/settings.ts"; import { FEEDBACK_CATEGORIES, @@ -149,7 +148,7 @@ export function registerStepPiCommandAdapters( { description: "Send feedback about the current session", handler: async (args, ctx) => { - await handleStepFeedbackCommand(args, ctx, telemetry, stepSettings, feedbackIdentity); + await handleStepFeedbackCommand(args, ctx, telemetry, stepSettings, feedbackIdentity, permissions); }, }, telemetry, @@ -311,6 +310,7 @@ async function handleStepFeedbackCommand( telemetry?: StepTelemetryReporter, stepSettings?: () => { getStepSettings(): { feedbackEnabled?: boolean } } | undefined, feedbackIdentity?: () => FeedbackIdentity, + permissions?: () => string, ): Promise { const feedbackSettings = resolveFeedbackSettings({ env: process.env, @@ -339,10 +339,41 @@ async function handleStepFeedbackCommand( const identity = feedbackIdentity?.() ?? {}; const shortcutComment = args.trim(); if (shortcutComment) { + // `/feedback ` used to send the sentence alone, which is exactly the + // report that cannot be investigated. It now carries the same evidence as + // the guided flow; only the category and the comment come from the command. + const readAt = new Date(); + const usage = typeof ctx.getContextUsage === "function" ? ctx.getContextUsage() : undefined; + const permission = permissions?.(); + const evidence = await prepareFeedbackEvidence({ + storageRootDir, + ...sessionSourceOf(ctx), + at: readAt, + // Nothing can receive the archive without a bundle endpoint; building it + // would read and compress up to 24 MiB and promise it on the review page. + withoutBundle: bundleEndpoint === undefined, + runtime: { + ...permissionFacts(permission), + ...(usage ? { contextUsed: usage.tokens, contextWindow: usage.contextWindow } : {}), + ...(typeof ctx.getToolCatalog === "function" ? { toolCount: ctx.getToolCatalog().length } : {}), + }, + }); + const sessionBundle = evidence.bundle; + if (sessionBundle?.status === "skipped") { + ctx.ui.notify( + formatFeedbackBundleSkipMessage(sessionBundle), + sessionBundle.reason === "too-large" || sessionBundle.reason === "unsafe" ? "warning" : "info", + ); + } const result = await submitFeedback({ + category: "other", comment: shortcutComment, + ...(evidence.diagnostics ? { diagnostics: evidence.diagnostics.diagnostics } : {}), + ...(evidence.summary ? { sessionSummary: evidence.summary } : {}), + ...(sessionBundle ? { sessionBundle } : {}), storageRootDir, - sessionId: ctx.sessionManager.getSessionId(), + sessionId: + sessionBundle?.status === "ready" ? sessionBundle.bundle.sessionId : ctx.sessionManager.getSessionId(), endpoint, ...(bundleEndpoint ? { bundleEndpoint } : {}), telemetry, @@ -350,6 +381,12 @@ async function handleStepFeedbackCommand( surface: "tui", ...(identity.uid ? { uid: identity.uid } : {}), ...(identity.username ? { username: identity.username } : {}), + confirm: async (submission) => + await confirmFeedbackSubmission(ctx, { + submission, + ...(evidence.diagnostics ? { diagnosticsDisplayPath: evidence.diagnostics.displayPath } : {}), + ...(sessionBundle?.status === "ready" ? { bundle: sessionBundle.bundle } : {}), + }), }); notifyFeedbackResult(ctx, result); return; @@ -373,45 +410,38 @@ async function handleStepFeedbackCommand( if (rawComment === undefined) return; const comment = rawComment.trim(); const readAt = new Date(); - const diagnosticsCandidate = await readFeedbackDiagnostics({ storageRootDir, at: readAt }); - const bundleCandidate = bundleEndpoint - ? await buildFeedbackSessionBundle({ - storageRootDir, - sessionFile: ctx.sessionManager.getSessionFile(), - sessionId: ctx.sessionManager.getSessionId(), - at: readAt, - }) - : undefined; - if (bundleCandidate?.status === "skipped") { + // One submission carries everything: the transcript, the full output files + // it referenced, the log tail, and the content-free session summary. Asking + // the user to pick what to attach produced reports that could not be + // investigated; the only decision left is whether to send at all. + // Minimal embedders (and tests) may not implement every context accessor. + const usage = typeof ctx.getContextUsage === "function" ? ctx.getContextUsage() : undefined; + const permission = permissions?.(); + const evidence = await prepareFeedbackEvidence({ + storageRootDir, + ...sessionSourceOf(ctx), + at: readAt, + withoutBundle: bundleEndpoint === undefined, + runtime: { + ...permissionFacts(permission), + ...(usage ? { contextUsed: usage.tokens, contextWindow: usage.contextWindow } : {}), + ...(typeof ctx.getToolCatalog === "function" ? { toolCount: ctx.getToolCatalog().length } : {}), + }, + }); + const sessionBundle = evidence.bundle; + if (sessionBundle?.status === "skipped") { ctx.ui.notify( - formatFeedbackBundleSkipMessage(bundleCandidate), - bundleCandidate.reason === "too-large" || bundleCandidate.reason === "unsafe" ? "warning" : "info", - ); - } - const availableFiles = [ - ...(diagnosticsCandidate ? [diagnosticsCandidate.displayPath] : []), - ...(bundleCandidate?.status === "ready" ? bundleCandidate.bundle.files.map((file) => file.name) : []), - ].map(neutralizeFeedbackConsentMetadata); - let includeAttachments = false; - if (availableFiles.length > 0) { - const uploadChoice = await ctx.ui.select( - `UPLOAD LOGS?\n${availableFiles.join(", ")}`, - ["Yes · Include the listed files", "No · Send feedback without files"], - { overlay: true }, + formatFeedbackBundleSkipMessage(sessionBundle), + sessionBundle.reason === "too-large" || sessionBundle.reason === "unsafe" ? "warning" : "info", ); - if (!uploadChoice) return; - includeAttachments = uploadChoice === "Yes · Include the listed files"; - } else if (bundleCandidate?.status !== "skipped") { - ctx.ui.notify("No logs or session files are currently available.", "info"); } - const diagnostics = includeAttachments ? diagnosticsCandidate?.diagnostics : undefined; - const sessionBundle = includeAttachments && bundleCandidate?.status === "ready" ? bundleCandidate : undefined; const submissionSessionId = sessionBundle?.status === "ready" ? sessionBundle.bundle.sessionId : ctx.sessionManager.getSessionId(); const result = await submitFeedback({ category, comment, - ...(diagnostics ? { diagnostics } : {}), + ...(evidence.diagnostics ? { diagnostics: evidence.diagnostics.diagnostics } : {}), + ...(evidence.summary ? { sessionSummary: evidence.summary } : {}), ...(sessionBundle ? { sessionBundle } : {}), storageRootDir, sessionId: submissionSessionId, @@ -425,13 +455,36 @@ async function handleStepFeedbackCommand( confirm: async (submission) => await confirmFeedbackSubmission(ctx, { submission, - ...(diagnosticsCandidate ? { diagnosticsDisplayPath: diagnosticsCandidate.displayPath } : {}), - ...(sessionBundle ? { bundle: sessionBundle.bundle } : {}), + ...(evidence.diagnostics ? { diagnosticsDisplayPath: evidence.diagnostics.displayPath } : {}), + ...(sessionBundle?.status === "ready" ? { bundle: sessionBundle.bundle } : {}), }), }); notifyFeedbackResult(ctx, result); } +/** + * The permission tier as the report names it. + * + * `step.ts` hands this callback the preset's display label for the status bar; + * the report documents ids, and the CLI host already sends the id. Normalize + * here so one tier never arrives under two spellings. + */ +function permissionFacts(value: string | undefined): { permissionPreset?: string } { + const normalized = (value ?? "") + .trim() + .toLowerCase() + .replace(/[^a-z]+/gu, "-") + .replace(/^-|-$/gu, ""); + return normalized === "" || normalized === "unknown" ? {} : { permissionPreset: normalized }; +} + +/** A minimal embedder may expose only the id; the bundle then resolves by id. */ +function sessionSourceOf(ctx: ExtensionCommandContext): { sessionFile?: string; sessionId: string } { + const manager = ctx.sessionManager; + const sessionFile = typeof manager.getSessionFile === "function" ? manager.getSessionFile() : undefined; + return { ...(sessionFile ? { sessionFile } : {}), sessionId: manager.getSessionId() }; +} + function notifyFeedbackResult(ctx: ExtensionCommandContext, result: FeedbackSubmitResult): void { if (result.status === "cancelled") return; if (result.status === "invalid") { diff --git a/packages/coding-agent/test/feedback-command.test.ts b/packages/coding-agent/test/feedback-command.test.ts index a9114f612..0331e300b 100644 --- a/packages/coding-agent/test/feedback-command.test.ts +++ b/packages/coding-agent/test/feedback-command.test.ts @@ -108,11 +108,11 @@ describe("feedback command interaction", () => { expect(stdout.text()).toBe(""); }); - test("treats a TTY command with a comment as a shortcut", async () => { + test("a TTY command with a comment skips the questions but still shows the consent page", async () => { const root = await makeRoot(); const stdout = captureOutput(); const stderr = captureOutput(); - const question = vi.fn(); + const question = vi.fn().mockResolvedValue("y"); const fetchImpl = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); const exitCode = await runFeedbackCommand(["the", "composer", "dropped", "a", "key"], { @@ -126,7 +126,8 @@ describe("feedback command interaction", () => { }); expect(exitCode).toBe(0); - expect(question).not.toHaveBeenCalled(); + // Only the confirmation: the comment and category were not asked for. + expect(question.mock.calls.map(([query]) => query)).toEqual(["Submit feedback? [y/N] "]); expect(fetchImpl).toHaveBeenCalledOnce(); const submission = JSON.parse(String(fetchImpl.mock.calls[0]?.[1]?.body)) as Record; expect(submission).toMatchObject({ comment: "the composer dropped a key" }); @@ -139,11 +140,11 @@ describe("feedback command interaction", () => { test.each([ { label: "an explicit empty --message", argv: ["--category", "bug", "--message="] }, { label: "an explicit empty positional comment", argv: ["--category", "bug", ""] }, - ])("keeps $label out of the guided flow", async ({ argv }) => { + ])("keeps $label out of the questions but still asks to submit", async ({ argv }) => { const root = await makeRoot(); const stdout = captureOutput(); const stderr = captureOutput(); - const question = vi.fn(); + const question = vi.fn().mockResolvedValue("y"); const fetchImpl = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); const exitCode = await runFeedbackCommand(argv, { @@ -157,7 +158,7 @@ describe("feedback command interaction", () => { }); expect(exitCode).toBe(0); - expect(question).not.toHaveBeenCalled(); + expect(question.mock.calls.map(([query]) => query)).toEqual(["Submit feedback? [y/N] "]); const submission = JSON.parse(String(fetchImpl.mock.calls[0]?.[1]?.body)) as Record; expect(submission).toMatchObject({ category: "bug", comment: "" }); expect(stdout.text()).toContain("Submitted. Feedback ID:"); @@ -199,12 +200,7 @@ describe("feedback command interaction", () => { await writeFile(resolveStderrDevLogPath(root), `before\nError token=${secret}\nafter\n`); const stdout = captureOutput(); const stderr = captureOutput(); - const question = vi - .fn() - .mockResolvedValueOnce("1") - .mockResolvedValueOnce("") - .mockResolvedValueOnce("It failed") - .mockResolvedValueOnce("y"); + const question = vi.fn().mockResolvedValueOnce("1").mockResolvedValueOnce("It failed").mockResolvedValueOnce("y"); const fetchImpl = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); const exitCode = await runFeedbackCommand([], { @@ -221,13 +217,14 @@ describe("feedback command interaction", () => { }); expect(exitCode).toBe(0); + // Attachments are not a question: the guided flow asks for the category, + // the comment, and one confirmation. expect(question.mock.calls.map(([query]) => query)).toEqual([ expect.stringContaining("Category ("), - expect.stringContaining("Attach diagnostics from logs/"), "Feedback: ", "Submit feedback? [y/N] ", ]); - expect(String(question.mock.calls[1]?.[0])).toContain("[Y/n]"); + expect(stdout.text()).toContain("Diagnostics: logs/"); const submission = JSON.parse(String(fetchImpl.mock.calls[0]?.[1]?.body)) as { diagnostics: { lines: string[]; truncated: boolean }; }; @@ -250,12 +247,7 @@ describe("feedback command interaction", () => { await writeFile(join(diagnosticsDir, fileName), "safe trace line\n"); const stdout = captureOutput(); const stderr = captureOutput(); - const question = vi - .fn() - .mockResolvedValueOnce("1") - .mockResolvedValueOnce("") - .mockResolvedValueOnce("It failed") - .mockResolvedValueOnce("y"); + const question = vi.fn().mockResolvedValueOnce("1").mockResolvedValueOnce("It failed").mockResolvedValueOnce("y"); const fetchImpl = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); const exitCode = await runFeedbackCommand([], { @@ -272,10 +264,9 @@ describe("feedback command interaction", () => { }); const safePath = "diagnostics/input-trace-\\x1b[31msecret\\x1b[0m\\rspoof\\nline.jsonl"; - const diagnosticsQuestion = String(question.mock.calls[1]?.[0]); expect(exitCode).toBe(0); - expect(diagnosticsQuestion).toContain(`Attach diagnostics from ${safePath}?`); - expect(diagnosticsQuestion).not.toMatch(/[\u001b\r\n]/u); + // The path is only shown in the preview, and never with terminal controls. + expect(question.mock.calls.map(([query]) => String(query))).not.toContain(expect.stringContaining("input-trace")); expect(stdout.text()).toContain(`Diagnostics: ${safePath} —`); expect(stdout.text()).not.toContain("\u001b"); expect(stdout.text()).not.toContain("\r"); @@ -315,7 +306,7 @@ describe("feedback command interaction", () => { expect(stderr.text()).toBe(""); }); - test("keeps an automatically discovered bundle session out of the top-level feedback context", async () => { + test("names the automatically discovered bundle session, so the report and its archive join", async () => { const root = await makeRoot(); const agentDir = join(root, "agent"); const sessionDir = join(agentDir, "sessions", "project"); @@ -326,12 +317,7 @@ describe("feedback command interaction", () => { ); const stdout = captureOutput(); const stderr = captureOutput(); - const question = vi - .fn() - .mockResolvedValueOnce("1") - .mockResolvedValueOnce("") - .mockResolvedValueOnce("It failed") - .mockResolvedValueOnce("y"); + const question = vi.fn().mockResolvedValueOnce("1").mockResolvedValueOnce("It failed").mockResolvedValueOnce("y"); const fetchImpl = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); const exitCode = await runFeedbackCommand([], { @@ -353,7 +339,9 @@ describe("feedback command interaction", () => { const submission = JSON.parse(String(fetchImpl.mock.calls[0]?.[1]?.body)) as { context: Record; }; - expect(submission.context.sessionId).toBeUndefined(); + // The archive already contains this conversation; leaving the id out only made + // the report impossible to correlate with the session it describes. + expect(submission.context.sessionId).toBe("guessed-session"); expect(String(fetchImpl.mock.calls[1]?.[0])).toContain("/bundle?"); expect(stdout.text()).toContain("session guessed-session"); expect(stderr.text()).toBe(""); @@ -424,7 +412,7 @@ describe("feedback command interaction", () => { const submission = JSON.parse(body) as { context: Record }; expect(submission.context.sessionId).toBe("header-session"); expect(body).not.toContain(sessionFile); - expect(stderr.text()).toBe(""); + expect(stderr.text()).toContain("Attaching: conversation transcript"); }); test.each(["absolute", "relative", "backslash", "jsonl-filename"] as const)( diff --git a/packages/coding-agent/test/feedback-context.test.ts b/packages/coding-agent/test/feedback-context.test.ts index 67be6767e..77bdf296e 100644 --- a/packages/coding-agent/test/feedback-context.test.ts +++ b/packages/coding-agent/test/feedback-context.test.ts @@ -22,6 +22,53 @@ describe("feedback context identity boundaries", () => { expect(readFeedbackUsername({ STEPCODE_USER: "not a username" })).toBeUndefined(); }); + it("names the build a report came from, including when the version is a fallback", async () => { + // A source build reports the fallback version; without `versionSource` a + // reader would take "0.1.0" for a release. + const context = await resolveFeedbackContext({ storageRootDir: await makeRoot(), env: {} }); + expect(context.version).toBe("0.1.0"); + expect(context.versionSource).toBe("fallback"); + expect(context.channel).toBe("dev"); + expect(context.runtime?.node).toBe(process.versions.node); + expect(context.runtime?.electron).toBeUndefined(); + }); + + it("prefers an embedded release identity and its build time over the fallback", async () => { + const context = await resolveFeedbackContext({ + storageRootDir: await makeRoot(), + env: { + STEPCODE_BUILD_VERSION: "v1.4.2", + STEPCODE_BUILD_CHANNEL: "release", + STEPCODE_BUILD_COMMIT: "3de15c16", + STEPCODE_BUILD_TIME: "2026-10-23T02:00:00.000Z", + } as NodeJS.ProcessEnv, + }); + expect(context).toMatchObject({ + version: "1.4.2", + versionSource: "embedded", + channel: "release", + commit: "3de15c16", + buildTime: "2026-10-23T02:00:00.000Z", + }); + }); + + it("drops a build time that is not a timestamp", async () => { + const context = await resolveFeedbackContext({ + storageRootDir: await makeRoot(), + env: { STEPCODE_BUILD_TIME: "yesterday" } as NodeJS.ProcessEnv, + }); + expect(context.buildTime).toBeUndefined(); + }); + + it("records how the code was installed when the launcher says so", async () => { + const context = await resolveFeedbackContext({ + storageRootDir: await makeRoot(), + env: { STEPCODE_INSTALL_SOURCE: "DMG", LC_ALL: "zh_CN.UTF-8" } as NodeJS.ProcessEnv, + }); + expect(context.installSource).toBe("dmg"); + expect(context.locale).toBe("zh-CN"); + }); + it("keeps valid host and session identities after trimming", async () => { const context = await resolveFeedbackContext({ storageRootDir: await makeRoot(), diff --git a/packages/coding-agent/test/feedback-pending-body.test.ts b/packages/coding-agent/test/feedback-pending-body.test.ts index 067b84f3a..a172243dd 100644 --- a/packages/coding-agent/test/feedback-pending-body.test.ts +++ b/packages/coding-agent/test/feedback-pending-body.test.ts @@ -36,6 +36,83 @@ function makeSubmission(feedbackId: string): FeedbackSubmission { } describe("pending feedback body validation", () => { + test("retries a body written with the build identity and session summary", async () => { + // A pending file is re-validated before retry. If a new field is not + // allowed here, a report saved offline is silently dropped on the next run. + const root = await makeRoot(); + const feedbackId = "20000000-0000-4000-8000-000000000001"; + const submission = { + ...makeSubmission(feedbackId), + schemaVersion: 2, + surface: "cli", + context: { + channel: "release", + version: "1.4.2", + versionSource: "embedded", + buildTime: TIMESTAMP, + runtime: { node: "24.21.0", electron: "44.7.0" }, + platform: "darwin", + arch: "arm64", + osRelease: "25.0.0", + locale: "zh-CN", + installSource: "dmg", + commit: "3de15c16", + }, + sessionSummary: { + counts: { turns: 1, toolCalls: 2, truncatedAttempts: 0 }, + usage: { input: 10, output: 5, total: 15, cacheHitRate: 0.5 }, + // A pass that did not shrink the window: `reclaimed` is negative, and + // the report must still be retryable. + compactions: [{ tokensBefore: 100, tokensAfter: 120 }], + compactionTotals: { passes: 1, resolved: 1, tokensBefore: 100, tokensAfter: 120, reclaimed: -20 }, + runtime: { permissionPreset: "approve-for-me", mcpServerNames: ["playwright"] }, + }, + request: { requestId: "req-1", contextWindow: 200000, ttftMs: null, outcome: "ok" }, + }; + await writeFile(resolveFeedbackPendingPath(root, feedbackId), `${JSON.stringify(submission, null, 2)}\n`); + const fetchImpl = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); + + const [result] = await retryPendingFeedback({ + storageRootDir: root, + endpoint: "https://feedback.test/feedback", + fetchImpl, + }); + + expect(result?.body?.status).toBe("delivered"); + expect(fetchImpl).toHaveBeenCalledOnce(); + }); + + test("rejects a pending body whose runtime versions are not versions", async () => { + const root = await makeRoot(); + const feedbackId = "20000000-0000-4000-8000-000000000002"; + const submission = { + ...makeSubmission(feedbackId), + context: { + channel: "release", + version: "1.4.2", + runtime: { node: "" }, + platform: "darwin", + }, + }; + await writeFile(resolveFeedbackPendingPath(root, feedbackId), `${JSON.stringify(submission)}\n`); + // Paired with an archive so the invalid body is surfaced for inspection + // rather than hidden (a standalone invalid body is not listed at all). + await writePendingFeedbackBundle({ storageRootDir: root, feedbackId, data: Buffer.from("archive") }); + const fetchImpl = vi.fn(); + + const [result] = await retryPendingFeedback({ + storageRootDir: root, + endpoint: "https://feedback.test/feedback", + fetchImpl, + }); + + // The invalid body is reported as a rejected, still-pending report that + // names its file, and nothing is posted. + expect(result?.body).toMatchObject({ status: "pending", reason: "rejected" }); + expect(result?.body?.status === "pending" ? result.body.pendingPath : undefined).toContain(feedbackId); + expect(fetchImpl).not.toHaveBeenCalled(); + }); + test("posts only a valid body and leaves every invalid body unchanged", async () => { const root = await makeRoot(); const invalidBodies = [ diff --git a/packages/coding-agent/test/feedback-row-metrics.test.ts b/packages/coding-agent/test/feedback-row-metrics.test.ts new file mode 100644 index 000000000..614fd14fc --- /dev/null +++ b/packages/coding-agent/test/feedback-row-metrics.test.ts @@ -0,0 +1,163 @@ +import { describe, expect, test } from "vitest"; +import { annotateTranscriptRows } from "../src/step/feedback/row-metrics.ts"; + +/** + * The transcript inside a feedback bundle carries each row's own numbers: the + * tokens that request was billed for, its cache hit rate, and how long it took. + * + * Only the bundle copy is annotated. The canonical session file on disk is not + * rewritten, so a support report never grows the user's session. + */ + +function line(value: unknown): string { + return JSON.stringify(value); +} + +function transcript(): string { + const entries = [ + { type: "session", id: "s1", timestamp: "2026-10-23T00:00:00.000Z", cwd: "/w" }, + { id: "u1", type: "message", message: { role: "user", content: [{ type: "text", text: "fix it" }] } }, + { + id: "a1", + type: "message", + message: { + role: "assistant", + provider: "step", + model: "step-5", + api: "openai-completions", + stopReason: "toolUse", + timestamp: 1000, + content: [{ type: "toolCall", id: "call-1", name: "read_file", arguments: {} }], + usage: { + input: 1000, + output: 200, + cacheRead: 3000, + cacheWrite: 0, + totalTokens: 4200, + cost: { total: 0.02 }, + }, + }, + }, + { + id: "t1", + type: "message", + message: { role: "toolResult", toolCallId: "call-1", toolName: "read_file", isError: false, timestamp: 1350 }, + }, + { + type: "custom", + customType: "step.usage_provenance", + data: { + requestId: "req-1", + usageEntryId: "a1", + requestedProvider: "step", + requestedModel: "step-5", + durationMs: 2500, + ttftMs: 640, + outcome: "ok", + }, + }, + { + type: "custom", + customType: "stepcode-web.thinking-timing", + data: { messageTimestamp: 1000, thinkingTimings: { 0: { startedAt: 100, endedAt: 400 } } }, + }, + ]; + return `${entries.map(line).join("\n")}\n`; +} + +function parse(text: string): Array> { + return text + .split("\n") + .filter((row) => row.trim().length > 0) + .map((row) => JSON.parse(row) as Record); +} + +describe("feedback row metrics", () => { + test("each assistant row carries its own tokens, cache hit rate and timing", () => { + const rows = parse(annotateTranscriptRows(transcript())); + const assistant = rows.find((row) => row.id === "a1"); + expect(assistant?.feedback).toEqual({ + turn: 1, + tokens: { input: 1000, output: 200, cacheRead: 3000, cacheWrite: 0, total: 4200 }, + // 3000 / (1000 + 3000), i.e. this request alone, not the session average. + cacheHitRate: 0.75, + costUsd: 0.02, + model: "step-5", + provider: "step", + stopReason: "toolUse", + requestId: "req-1", + durationMs: 2500, + ttftMs: 640, + outcome: "ok", + thinkingMs: 300, + }); + }); + + test("a single tool call carries the elapsed time from issue to result", () => { + const rows = parse(annotateTranscriptRows(transcript())); + const result = rows.find((row) => row.id === "t1"); + expect(result?.feedback).toEqual({ turn: 1, elapsedMs: 350 }); + }); + + test("a parallel batch records no per-call duration, because it would be a guess", () => { + const entries = [ + { id: "u1", type: "message", message: { role: "user" } }, + { + id: "a1", + type: "message", + message: { + role: "assistant", + timestamp: 1000, + content: [ + { type: "toolCall", id: "c1", name: "read_file" }, + { type: "toolCall", id: "c2", name: "read_file" }, + ], + usage: { input: 1, output: 1, cacheRead: 0, totalTokens: 2 }, + }, + }, + { id: "t1", type: "message", message: { role: "toolResult", toolCallId: "c1", timestamp: 1400 } }, + { id: "t2", type: "message", message: { role: "toolResult", toolCallId: "c2", timestamp: 1400 } }, + ]; + const rows = parse(annotateTranscriptRows(`${entries.map(line).join("\n")}\n`)); + // No elapsed time is invented, so the rows are left untouched. + expect(rows.find((row) => row.id === "t1")?.feedback).toBeUndefined(); + expect(rows.find((row) => row.id === "t2")?.feedback).toBeUndefined(); + }); + + test("keeps every original field and the original line endings", () => { + const text = transcript(); + const annotated = annotateTranscriptRows(text); + const before = parse(text); + const after = parse(annotated); + expect(after).toHaveLength(before.length); + for (const [index, row] of after.entries()) { + const original = before[index]!; + expect(Object.keys(row)).toEqual(expect.arrayContaining(Object.keys(original))); + for (const [key, value] of Object.entries(original)) { + if (key === "feedback") continue; + expect(row[key]).toEqual(value); + } + } + expect(annotated.endsWith("\n")).toBe(true); + }); + + test("passes a transcript with no metrics through unchanged", () => { + const text = `${line({ type: "session", id: "s1" })}\n${line({ id: "u1", type: "message", message: { role: "user" } })}\n`; + expect(annotateTranscriptRows(text)).toBe(text); + }); + + test("tolerates a malformed line without losing the rest", () => { + const text = `not json\n${line({ id: "u1", type: "message", message: { role: "user" } })}\n${line({ + id: "a1", + type: "message", + message: { role: "assistant", usage: { input: 10, cacheRead: 0, output: 1, totalTokens: 11 } }, + })}\n`; + const rows = parse( + annotateTranscriptRows(text) + .split("\n") + .filter((row) => row !== "not json") + .join("\n"), + ); + expect(rows.find((row) => row.id === "a1")?.feedback).toMatchObject({ cacheHitRate: 0 }); + }); +}); diff --git a/packages/coding-agent/test/feedback-summary.test.ts b/packages/coding-agent/test/feedback-summary.test.ts new file mode 100644 index 000000000..2d85a9096 --- /dev/null +++ b/packages/coding-agent/test/feedback-summary.test.ts @@ -0,0 +1,537 @@ +import { mkdtemp, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, test } from "vitest"; +import { prepareFeedbackEvidence } from "../src/step/feedback/evidence.ts"; +import { collapseHomePaths } from "../src/step/feedback/redact-home.ts"; +import { summarizeSessionEntries, summarizeSessionFile } from "../src/step/feedback/session-summary.ts"; + +/** + * The summary is what makes a report investigable without reading the + * transcript: model, effort, counts, failures, spend, and the request id that + * joins the server-side log. It must never carry user content. + */ + +function session(): unknown[] { + return [ + { type: "session", id: "s1", timestamp: "2026-10-23T00:00:00.000Z", cwd: "/Users/me/code/demo" }, + { type: "model_change", provider: "step", modelId: "step-5" }, + { type: "thinking_level_change", thinkingLevel: "high" }, + { + type: "custom", + customType: "step.usage_provenance", + data: { + requestId: "req-1", + requestedProvider: "step", + requestedModel: "step-5", + reasoningAtDispatch: "high", + sourceLeafId: "leaf-1", + }, + }, + { type: "message", message: { role: "user", content: [{ type: "text", text: "please fix" }] } }, + { + type: "message", + message: { + role: "assistant", + provider: "step", + model: "step-5", + api: "openai-completions", + content: [ + { type: "text", text: "looking" }, + { type: "toolCall", id: "t1", name: "read_file", arguments: {} }, + { type: "toolCall", id: "t2", name: "run_command", arguments: {} }, + ], + usage: { input: 100, output: 20, cacheRead: 5, cacheWrite: 0, reasoning: 3, totalTokens: 128 }, + }, + }, + { + type: "message", + message: { role: "toolResult", toolCallId: "t1", toolName: "read_file", isError: false, content: [] }, + }, + { + type: "message", + message: { role: "toolResult", toolCallId: "t2", toolName: "run_command", isError: true, content: [] }, + }, + { + type: "message", + message: { role: "assistant", content: [{ type: "text", text: "failed" }], stopReason: "error" }, + }, + { type: "compaction", summary: "…", firstKeptEntryId: "x", tokensBefore: 10 }, + { type: "custom", customType: "step-plan", data: { enabled: true, planSource: "user" } }, + { type: "custom", customType: "step-goal", data: { status: "active" } }, + { + type: "custom", + customType: "step-tasks", + data: { tasks: [{ status: "completed" }, { status: "pending" }, { status: "deleted" }] }, + }, + { + type: "custom", + customType: "stepcode-web.thinking-timing", + data: { thinkingTimings: { 0: { startedAt: 1000, endedAt: 1500 } } }, + }, + ]; +} + +describe("feedback session summary", () => { + test("counts the shape of the session without any message text", () => { + const summary = summarizeSessionEntries(session() as never, { home: "/Users/me" }); + expect(summary.counts).toEqual({ + turns: 1, + assistantMessages: 2, + toolCalls: 2, + toolErrors: 1, + failedAttempts: 1, + abortedAttempts: 0, + truncatedAttempts: 0, + modelRequests: 1, + compactions: 1, + customEntries: 5, + }); + expect(summary.usage).toEqual({ + input: 100, + output: 20, + cacheRead: 5, + cacheWrite: 0, + reasoning: 3, + total: 128, + // The synthetic message carries no pricing, so only the derived rate appears. + cacheHitRate: 5 / 105, + }); + expect(summary.recentErrors).toEqual([ + expect.objectContaining({ kind: "tool", toolName: "run_command" }), + expect.objectContaining({ kind: "model" }), + ]); + expect(summary.lastTurnTools).toEqual([ + { name: "read_file", isError: false }, + { name: "run_command", isError: true }, + ]); + expect(summary).toMatchObject({ + model: { provider: "step", id: "step-5", api: "openai-completions" }, + thinkingLevel: "high", + thinkingMs: 500, + features: { planMode: true, planSource: "user", goal: "active", tasks: { completed: 1, total: 2 } }, + }); + // The home directory never reaches a report as an absolute path. + expect(summary.cwd).toBe("~/code/demo"); + expect(JSON.stringify(summary)).not.toContain("please fix"); + expect(JSON.stringify(summary)).not.toContain("looking"); + }); + + test("reports each compaction's size before and after", () => { + // A compaction that does not shrink the context, or that fires repeatedly, + // is the first thing to check for a "context full" or "slow" report. + const summary = summarizeSessionEntries( + [ + { type: "session", cwd: "/w" }, + { type: "message", message: { role: "user" } }, + { type: "compaction", tokensBefore: 190000, timestamp: "2026-10-23T00:00:01.000Z" }, + { + type: "message", + message: { role: "assistant", usage: { input: 42000, cacheRead: 0, output: 10, cacheWrite: 0 } }, + }, + { type: "message", message: { role: "user" } }, + { type: "compaction", tokensBefore: 188000, timestamp: "2026-10-23T00:05:00.000Z" }, + { + type: "message", + message: { role: "assistant", usage: { input: 40000, cacheRead: 0, output: 10, cacheWrite: 0 } }, + }, + ] as never, + { home: "/w" }, + ); + expect(summary.compactions).toEqual([ + { tokensBefore: 190000, tokensAfter: 42000, at: "2026-10-23T00:00:01.000Z" }, + { tokensBefore: 188000, tokensAfter: 40000, at: "2026-10-23T00:05:00.000Z" }, + ]); + }); + + test("keeps every compaction of a long session, plus the totals", () => { + // A long session compacts repeatedly. Each pass is one row, and a pass that + // never shrank the window is visible as two close numbers. + const passes = [ + { before: 190000, after: 42000 }, + { before: 188000, after: 40000 }, + { before: 195000, after: 190000 }, + ]; + const entries: unknown[] = [{ type: "session", cwd: "/w" }]; + for (const [index, pass] of passes.entries()) { + entries.push({ type: "compaction", tokensBefore: pass.before, timestamp: `2026-10-23T0${index}:00:00.000Z` }); + if (pass.after !== undefined) { + entries.push({ + type: "message", + message: { role: "assistant", usage: { input: pass.after, cacheRead: 0, output: 1, cacheWrite: 0 } }, + }); + } + } + const summary = summarizeSessionEntries(entries as never, { home: "/w" }); + expect(summary.counts.compactions).toBe(3); + expect(summary.compactions.map((pass) => [pass.tokensBefore, pass.tokensAfter])).toEqual([ + [190000, 42000], + [188000, 40000], + [195000, 190000], + ]); + expect(summary.compactionTotals).toEqual({ + passes: 3, + resolved: 3, + tokensBefore: 573000, + tokensAfter: 272000, + reclaimed: 301000, + }); + }); + + test("marks a compaction no request followed yet as unresolved", () => { + const summary = summarizeSessionEntries( + [ + { type: "session", cwd: "/w" }, + { type: "compaction", tokensBefore: 100 }, + ] as never, + { home: "/w" }, + ); + expect(summary.compactions[0]).toEqual({ tokensBefore: 100, unresolved: true }); + }); + + test("records the provider's HTTP error body, redacted and bounded", () => { + const summary = summarizeSessionEntries( + [ + { type: "session", cwd: "/w" }, + { + type: "message", + message: { + role: "assistant", + stopReason: "error", + errorKind: "rate_limit", + errorMessage: '429: {"message":"concurrency reached, current: 11, limit: 10","type":"rate_limited"}', + }, + }, + ] as never, + { home: "/w" }, + ); + expect(summary.recentErrors[0]).toMatchObject({ + kind: "model", + stopReason: "error", + errorKind: "rate_limit", + message: '429: {"message":"concurrency reached, current: 11, limit: 10","type":"rate_limited"}', + }); + }); + + test("collapses the home directory inside a provider error body", () => { + // The one free-text field in the summary; an ENOENT body names a real path. + const summary = summarizeSessionEntries( + [ + { type: "session", cwd: "/Users/me/w" }, + { + type: "message", + message: { + role: "assistant", + stopReason: "error", + errorMessage: "ENOENT: open '/Users/me/code/app/.env'", + }, + }, + ] as never, + { home: "/Users/me" }, + ); + expect(summary.recentErrors[0]?.message).toBe("ENOENT: open '~/code/app/.env'"); + }); + + test("records a cancelled request with its timing and how far it got", () => { + // The reported case: the model hangs or keeps thinking, the user cancels. + // A bare count would not say whether a token ever arrived. + const summary = summarizeSessionEntries( + [ + { type: "session", cwd: "/w" }, + { + type: "message", + message: { + role: "assistant", + stopReason: "aborted", + errorMessage: "Request was aborted", + content: [{ type: "thinking", thinking: "…" }], + timestamp: 1792675967000, + }, + }, + ] as never, + { home: "/w" }, + ); + expect(summary.counts.abortedAttempts).toBe(1); + // The message records when and how far; duration and first-token time come + // from the provenance entry that follows it (asserted in the join test). + expect(summary.recentErrors[0]).toMatchObject({ kind: "aborted", stopReason: "aborted", reached: "thinking" }); + expect(summary.recentErrors[0]?.at).toBe(new Date(1792675967000).toISOString()); + }); + + test("joins the timing from the provenance entry that belongs to the attempt", () => { + // The message carries no duration; the provenance entry that follows it does. + const summary = summarizeSessionEntries( + [ + { type: "session", cwd: "/w" }, + { + id: "entry-1", + type: "message", + message: { role: "assistant", stopReason: "aborted", content: [{ type: "thinking", thinking: "…" }] }, + }, + { + type: "custom", + customType: "step.usage_provenance", + data: { + requestId: "req-2", + requestedProvider: "step", + requestedModel: "step-5", + usageEntryId: "entry-1", + durationMs: 187000, + ttftMs: null, + outcome: "transport_error", + }, + }, + ] as never, + { home: "/w" }, + ); + expect(summary.recentErrors[0]).toMatchObject({ + kind: "aborted", + durationMs: 187000, + ttftMs: null, + outcome: "transport_error", + }); + }); + + test("a compaction that did not shrink the window reports a negative reclaim", () => { + // The "two close numbers" case; the report must still be accepted downstream. + const summary = summarizeSessionEntries( + [ + { type: "session", cwd: "/w" }, + { type: "compaction", tokensBefore: 100 }, + { type: "message", message: { role: "assistant", usage: { input: 120, cacheRead: 0, output: 1 } } }, + ] as never, + { home: "/w" }, + ); + expect(summary.compactionTotals).toMatchObject({ passes: 1, resolved: 1, reclaimed: -20 }); + }); + + test("a trailing blank or truncated line does not empty the summary", async () => { + const root = await mkdtemp(join(tmpdir(), "feedback-tail-")); + const file = join(root, "20261023_session-1.jsonl"); + const lines = [ + JSON.stringify({ type: "session", id: "h", parentId: null, cwd: "/w" }), + JSON.stringify({ type: "message", id: "u1", parentId: "h", message: { role: "user" } }), + "", + '{"type":"message","id":"a1","par', + ]; + await writeFile(file, `${lines.join("\n")}\n`); + const summary = await summarizeSessionFile(file, { home: "/w" }); + expect(summary?.counts.turns).toBe(1); + }); + + test("carries the context window and pricing limits observed at dispatch", () => { + const summary = summarizeSessionEntries( + [ + { type: "session", cwd: "/w" }, + { + type: "custom", + customType: "step.usage_provenance", + data: { + requestId: "req-9", + requestedProvider: "step", + requestedModel: "step-5", + api: "anthropic-messages", + reasoningAtDispatch: "high", + contextWindow: 200000, + maxTokens: 8192, + responseProvider: "step-proxy", + responseModel: "step-5-2026", + statusCode: 200, + durationMs: 4200, + }, + }, + ] as never, + { home: "/w" }, + ); + expect(summary.request).toEqual({ + requestId: "req-9", + provider: "step", + model: "step-5", + api: "anthropic-messages", + reasoning: "high", + contextWindow: 200000, + maxTokens: 8192, + responseProvider: "step-proxy", + responseModel: "step-5-2026", + statusCode: 200, + durationMs: 4200, + }); + expect(summary.counts.modelRequests).toBe(1); + }); + + test("keeps the live host facts a session file does not record", () => { + const summary = summarizeSessionEntries([{ type: "session", cwd: "/Users/me/w" }] as never, { + home: "/Users/me", + runtime: { + permissionPreset: "approve-for-me", + contextUsed: 61000, + contextWindow: 200000, + toolCount: 24, + skillCount: 3, + mcpServerNames: ["playwright"], + }, + }); + expect(summary.runtime).toEqual({ + permissionPreset: "approve-for-me", + contextUsed: 61000, + contextWindow: 200000, + toolCount: 24, + skillCount: 3, + mcpServerNames: ["playwright"], + }); + }); + + test("counts a user stop separately from a failure, and reports total cost", () => { + const summary = summarizeSessionEntries( + [ + { type: "session", cwd: "/w" }, + { + type: "message", + message: { + role: "assistant", + stopReason: "aborted", + usage: { input: 10, output: 5, cacheRead: 0, cacheWrite: 0, cost: { total: 0.25 } }, + }, + }, + ] as never, + { home: "/w" }, + ); + expect(summary.counts.abortedAttempts).toBe(1); + expect(summary.counts.failedAttempts).toBe(0); + expect(summary.usage.costUsd).toBe(0.25); + }); + + test("carries the newest model request id so the report joins the server log", () => { + const summary = summarizeSessionEntries(session() as never, { home: "/Users/me" }); + expect(summary.request).toEqual({ + requestId: "req-1", + provider: "step", + model: "step-5", + reasoning: "high", + sourceLeafId: "leaf-1", + }); + }); + + test("bounds the latest turn's tool list instead of growing with the session", () => { + const assistant = { + type: "message", + message: { + role: "assistant", + content: Array.from({ length: 80 }, (_, index) => ({ + type: "toolCall", + id: `t${index}`, + name: "read_file", + })), + }, + }; + const summary = summarizeSessionEntries([{ type: "message", message: { role: "user" } }, assistant] as never); + expect(summary.lastTurnTools).toHaveLength(50); + expect(summary.counts.toolCalls).toBe(80); + }); + + test("summarizes the branch the file ends on, not abandoned turns", async () => { + // Navigating back leaves the abandoned turn in the file. Counting it would + // report turns the user rewound and a requestId from another branch. + const root = await mkdtemp(join(tmpdir(), "feedback-branch-")); + const file = join(root, "20261023_session-1.jsonl"); + const abandoned = [ + { type: "session", id: "h", parentId: null, cwd: "/w" }, + { type: "message", id: "u1", parentId: "h", message: { role: "user" } }, + { type: "message", id: "a1", parentId: "u1", message: { role: "assistant", usage: { input: 5, output: 1 } } }, + { + type: "custom", + id: "p1", + parentId: "a1", + customType: "step.usage_provenance", + data: { requestId: "abandoned", usageEntryId: "a1" }, + }, + ]; + const live = [ + { type: "message", id: "u2", parentId: "h", message: { role: "user" } }, + { type: "message", id: "a2", parentId: "u2", message: { role: "assistant", usage: { input: 7, output: 2 } } }, + { + type: "custom", + id: "p2", + parentId: "a2", + customType: "step.usage_provenance", + data: { requestId: "live", usageEntryId: "a2" }, + }, + ]; + await writeFile(file, `${[...abandoned, ...live].map((entry) => JSON.stringify(entry)).join("\n")}\n`); + + const summary = await summarizeSessionFile(file, { home: "/w" }); + + expect(summary?.counts.turns).toBe(1); + expect(summary?.usage.input).toBe(7); + expect(summary?.request?.requestId).toBe("live"); + }); + + test("reads the same values from the session file", async () => { + const root = await mkdtemp(join(tmpdir(), "feedback-summary-")); + const file = join(root, "20261023_session-1.jsonl"); + await writeFile( + file, + `${session() + .map((value) => JSON.stringify(value)) + .join("\n")}\n`, + ); + const fromFile = await summarizeSessionFile(file, { home: "/Users/me" }); + expect(fromFile).toEqual(summarizeSessionEntries(session() as never, { home: "/Users/me" })); + }); + + test("an unreadable line does not lose the counts around it", async () => { + const root = await mkdtemp(join(tmpdir(), "feedback-summary-")); + const file = join(root, "20261023_session-1.jsonl"); + await writeFile( + file, + `{"type":"session","cwd":"/tmp/x"}\nnot json\n{"type":"message","message":{"role":"user"}}\n`, + ); + const summary = await summarizeSessionFile(file); + expect(summary?.counts.turns).toBe(1); + }); + + test("collapses only a real home prefix", () => { + expect(collapseHomePaths("/Users/bob/code/app", "/Users/bob")).toBe("~/code/app"); + expect(collapseHomePaths("/Users/bob2/code/app", "/Users/bob")).toBe("/Users/bob2/code/app"); + expect(collapseHomePaths("see /Users/bob and /Users/bob/x", "/Users/bob")).toBe("see ~ and ~/x"); + }); +}); + +describe("feedback evidence", () => { + test("gathers the transcript, the log tail and the summary from one session", async () => { + const root = await mkdtemp(join(tmpdir(), "feedback-evidence-")); + const sessionFile = join(root, "20261023_session-1.jsonl"); + await writeFile( + sessionFile, + `${session() + .map((value) => JSON.stringify(value)) + .join("\n")}\n`, + ); + const evidence = await prepareFeedbackEvidence({ + storageRootDir: root, + sessionFile, + env: { STEP_CODING_AGENT_DIR: join(root, "agent") } as NodeJS.ProcessEnv, + }); + expect(evidence.sessionId).toBe("s1"); + expect(evidence.sessionFile).toBe(sessionFile); + expect(evidence.summary?.counts.turns).toBe(1); + expect(evidence.bundle?.status).toBe("ready"); + }); + + test("an explicit opt-out still produces a report without the archive", async () => { + const root = await mkdtemp(join(tmpdir(), "feedback-evidence-")); + const sessionFile = join(root, "20261023_session-1.jsonl"); + await writeFile(sessionFile, '{"type":"session","id":"s1"}\n'); + const evidence = await prepareFeedbackEvidence({ + storageRootDir: root, + sessionFile, + withoutBundle: true, + withoutDiagnostics: true, + env: { STEP_CODING_AGENT_DIR: join(root, "agent") } as NodeJS.ProcessEnv, + }); + expect(evidence.bundle).toBeUndefined(); + expect(evidence.diagnostics).toBeUndefined(); + // The summary is content-free, so it survives both opt-outs. + expect(evidence.summary).toBeDefined(); + }); +}); diff --git a/packages/coding-agent/test/feedback.test.ts b/packages/coding-agent/test/feedback.test.ts index 854471c0a..82783de07 100644 --- a/packages/coding-agent/test/feedback.test.ts +++ b/packages/coding-agent/test/feedback.test.ts @@ -13,6 +13,7 @@ import { readFeedbackDiagnostics } from "../src/step/feedback/diagnostics.ts"; import { resolveFeedbackEndpoint } from "../src/step/feedback/endpoints.ts"; import { listPendingFeedback, + normalizePendingFeedbackBundle, resolveFeedbackPendingPath, writePendingFeedback, writePendingFeedbackBundle, @@ -1116,6 +1117,66 @@ describe("feedback diagnostics and bundles", () => { expect(explicit.status).toBe("ready"); }); + test("a bundle with full tool output survives pending normalization", async () => { + // The archive carries `tool-output/*.txt`; a stored copy is re-validated + // before retry, so an archive it cannot re-read is unrecoverable. + const root = await makeRoot(); + const sessionDir = join(root, "sessions", "project"); + await mkdir(join(sessionDir, "tool-output"), { recursive: true }); + const sessionFile = join(sessionDir, "20260831_session-1.jsonl"); + const toolName = `tool-${"a".repeat(32)}.txt`; + await writeFile( + sessionFile, + `${JSON.stringify({ + type: "message", + message: { role: "toolResult", content: [{ type: "text", text: `Full output: ${toolName}` }] }, + })}\n`, + ); + await writeFile(join(sessionDir, "tool-output", toolName), "very long tool output\n"); + + const result = await buildFeedbackSessionBundle({ storageRootDir: root, sessionFile }); + expect(result.status).toBe("ready"); + if (result.status !== "ready") return; + expect(result.bundle.files.map((file) => file.name)).toContain(`tool-output/${toolName}`); + await expect(normalizePendingFeedbackBundle(Buffer.from(result.bundle.data))).resolves.toBeDefined(); + }); + + test("a guessed session never comes from another project", async () => { + // Attaching a conversation from a project the user is not filing from is a + // privacy incident, not a helpful guess. + const root = await makeRoot(); + const sessions = join(root, "agent", "sessions"); + const other = join(sessions, "--other-project--"); + await mkdir(other, { recursive: true }); + const otherFile = join(other, "20260831_other.jsonl"); + await writeFile(otherFile, '{"type":"session","id":"other"}\n'); + const cwd = join(root, "current-project"); + await mkdir(cwd, { recursive: true }); + + const guessed = await buildFeedbackSessionBundle({ storageRootDir: sessions, sessionDir: sessions, cwd }); + expect(guessed).toEqual({ status: "skipped", reason: "no-session" }); + // The same file is still reachable when the user names it. + const explicit = await buildFeedbackSessionBundle({ storageRootDir: sessions, sessionFile: otherFile }); + expect(explicit.status).toBe("ready"); + }); + + test("a guessed session in this project still respects the recency window", async () => { + const root = await makeRoot(); + const sessions = join(root, "agent", "sessions"); + const cwd = join(root, "project"); + const projectDir = join(sessions, `--${cwd.replace(/^\//, "").replace(/[/:]/g, "-")}--`); + await mkdir(projectDir, { recursive: true }); + const sessionFile = join(projectDir, "20260101_session.jsonl"); + await writeFile(sessionFile, '{"type":"session","id":"old"}\n'); + const old = new Date("2026-01-01T00:00:00.000Z"); + await utimes(sessionFile, old, old); + + const now = new Date("2026-08-31T00:00:00.000Z"); + await expect( + buildFeedbackSessionBundle({ storageRootDir: sessions, sessionDir: sessions, cwd, now }), + ).resolves.toEqual({ status: "skipped", reason: "stale" }); + }); + test("redacts credentials in the session bundle", async () => { const root = await makeRoot(); const sessionFile = join(root, "20260831_session-1.jsonl"); diff --git a/packages/coding-agent/test/step-slash-commands.test.ts b/packages/coding-agent/test/step-slash-commands.test.ts index c176f60ef..fc78b77b4 100644 --- a/packages/coding-agent/test/step-slash-commands.test.ts +++ b/packages/coding-agent/test/step-slash-commands.test.ts @@ -440,13 +440,18 @@ describe("Step Pi slash command adapters", () => { expect(notify).toHaveBeenCalledWith("/feedback requires an interactive UI.", "warning"); }); - test("submits a shortcut comment without opening guided confirmation or attachments", async () => { + test("a shortcut comment carries the same evidence as the guided flow", async () => { const root = await mkdtemp(join(tmpdir(), "step-slash-feedback-")); temporaryRoots.push(root); vi.stubEnv("STEPCODE_STORAGE_ROOT_DIR", root); vi.stubEnv("STEPCODE_FEEDBACK_ENDPOINT", "https://feedback.test/feedback"); const fetchMock = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); vi.stubGlobal("fetch", fetchMock); + const sessionFile = join(root, "20260831_session-1.jsonl"); + await writeFile( + sessionFile, + '{"type":"session","id":"session-1"}\n{"type":"message","message":{"role":"user"}}\n', + ); const { commands } = registerCommands(); const confirm = vi.fn().mockResolvedValue(true); const notify = vi.fn(); @@ -454,15 +459,16 @@ describe("Step Pi slash command adapters", () => { await getCommand(commands, "feedback").handler("the composer dropped a key", { hasUI: true, ui: { confirm, notify }, - sessionManager: { getSessionId: () => "session-1" }, + sessionManager: { getSessionId: () => "session-1", getSessionFile: () => sessionFile }, } as unknown as ExtensionCommandContext); - expect(confirm).not.toHaveBeenCalled(); + // The category and comment come from the command; everything else is the + // same evidence the guided flow submits, behind the same consent page. + expect(confirm).toHaveBeenCalledOnce(); expect(fetchMock).toHaveBeenCalledOnce(); const body = JSON.parse(String(fetchMock.mock.calls[0]?.[1]?.body)) as Record; - expect(body).toMatchObject({ comment: "the composer dropped a key" }); - expect(body.category).toBeUndefined(); - expect(body.diagnostics).toBeUndefined(); + expect(body).toMatchObject({ comment: "the composer dropped a key", category: "other", surface: "tui" }); + expect(body.sessionSummary).toBeDefined(); const feedbackId = String(body.feedbackId); expect(notify).toHaveBeenCalledWith( `Submitted. Feedback ID: ${feedbackId} — include this ID when contacting support.`, @@ -485,7 +491,7 @@ describe("Step Pi slash command adapters", () => { const context = { hasUI: true, - ui: { notify: vi.fn() }, + ui: { notify: vi.fn(), confirm: vi.fn().mockResolvedValue(true) }, sessionManager: { getSessionId: () => "session-1" }, } as unknown as ExtensionCommandContext; await getCommand(commands, "feedback").handler("before login", context); @@ -539,10 +545,7 @@ describe("Step Pi slash command adapters", () => { vi.stubEnv("STEPCODE_FEEDBACK_ENDPOINT", "https://feedback.test/feedback"); vi.stubGlobal("fetch", vi.fn()); const { commands } = registerCommands(); - const select = vi - .fn() - .mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior.") - .mockResolvedValueOnce("No · Send feedback without files"); + const select = vi.fn().mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior."); const input = vi.fn().mockResolvedValue("It failed"); await getCommand(commands, "feedback").handler("", { @@ -557,10 +560,10 @@ describe("Step Pi slash command adapters", () => { expect(select.mock.calls[0]?.[2]).toEqual({ overlay: true }); expect(input.mock.calls[0]?.[2]).toEqual({ overlay: true }); - expect(select.mock.calls[1]?.[2]).toEqual({ overlay: true }); + expect(select).toHaveBeenCalledOnce(); }); - test("neutralizes a diagnostics filename before showing the upload selector", async () => { + test("neutralizes a diagnostics filename in the consent preview", async () => { const root = await mkdtemp(join(tmpdir(), "step-slash-feedback-")); temporaryRoots.push(root); const diagnosticsDir = join(root, "diagnostics"); @@ -573,10 +576,7 @@ describe("Step Pi slash command adapters", () => { vi.stubEnv("STEPCODE_FEEDBACK_ENDPOINT", "https://feedback.test/feedback"); vi.stubGlobal("fetch", vi.fn()); const { commands } = registerCommands(); - const select = vi - .fn() - .mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior.") - .mockResolvedValueOnce("No · Send feedback without files"); + const select = vi.fn().mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior."); const confirm = vi.fn().mockResolvedValue(false); await getCommand(commands, "feedback").handler("", { @@ -594,11 +594,12 @@ describe("Step Pi slash command adapters", () => { }, } as unknown as ExtensionCommandContext); - const uploadTitle = String(select.mock.calls[1]?.[0]); - expect(uploadTitle).not.toContain("\x1b[2JOWNED"); - expect(uploadTitle).not.toContain("line\nbreak"); - expect(uploadTitle).toContain("line\\nbreak"); - expect(uploadTitle).toContain("\\x1b[2JOWNED.jsonl"); + // The filename only reaches the consent preview, where controls are escaped. + const preview = String(confirm.mock.calls[0]?.[1]); + expect(preview).not.toContain("\x1b[2JOWNED"); + expect(preview).not.toContain("line\nbreak"); + expect(preview).toContain("line\\nbreak"); + expect(preview).toContain("\\x1b[2JOWNED.jsonl"); expect(confirm).toHaveBeenCalledOnce(); }); @@ -617,10 +618,7 @@ describe("Step Pi slash command adapters", () => { const fetchMock = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); vi.stubGlobal("fetch", fetchMock); const { commands } = registerCommands(); - const select = vi - .fn() - .mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior.") - .mockResolvedValueOnce("Yes · Include the listed files"); + const select = vi.fn().mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior."); const input = vi.fn().mockResolvedValue("It failed"); const confirm = vi.fn().mockImplementation(async () => { await rm(sessionFile); @@ -639,18 +637,9 @@ describe("Step Pi slash command adapters", () => { }, } as unknown as ExtensionCommandContext); - expect(select).toHaveBeenCalledTimes(2); - const uploadTitle = String(select.mock.calls[1]?.[0]); - expect(uploadTitle).toBe(`UPLOAD LOGS?\nlogs/${basename(devLogPath)}, events.jsonl, dev.log`); - expect(select).toHaveBeenNthCalledWith( - 2, - uploadTitle, - ["Yes · Include the listed files", "No · Send feedback without files"], - { overlay: true }, - ); - expect(uploadTitle).not.toContain("before"); - expect(uploadTitle).not.toContain("Error: renderer failed"); - expect(uploadTitle).not.toContain("after"); + // There is no attachment question any more: the report always carries the + // conversation, and the consent page is the only place it is described. + expect(select).toHaveBeenCalledOnce(); expect(confirm).toHaveBeenCalledOnce(); expect(custom).not.toHaveBeenCalled(); expect(fetchMock).toHaveBeenCalledTimes(2); @@ -674,9 +663,8 @@ describe("Step Pi slash command adapters", () => { expect(finalPreview).toContain("dev.log ("); expect(finalPreview).toContain("context lines around 1 error"); expect(finalPreview).toContain("session session-1, last active "); - expect(finalPreview).toContain( - "It contains the conversation itself: your prompts, the model's replies, tool calls and their output.", - ); + expect(finalPreview).toContain("It contains the conversation itself: your prompts, the model's replies"); + expect(finalPreview).toContain("the full output files those calls referenced"); const uploadedBundle = fetchMock.mock.calls[1]?.[1]?.body as Uint8Array; expect(finalPreview).toContain(`Session bundle (${uploadedBundle.byteLength} compressed bytes):`); const bundleUrl = new URL(String(fetchMock.mock.calls[1]?.[0])); @@ -721,7 +709,7 @@ describe("Step Pi slash command adapters", () => { expect(body.context.sessionId).toBe("header-session"); }); - test("submits guided feedback without any files when upload is declined", async () => { + test("attaches the conversation and log without asking, so one submission is enough to investigate", async () => { const root = await mkdtemp(join(tmpdir(), "step-slash-feedback-")); temporaryRoots.push(root); const sessionFile = join(root, "20260831_session-1.jsonl"); @@ -734,10 +722,7 @@ describe("Step Pi slash command adapters", () => { const fetchMock = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); vi.stubGlobal("fetch", fetchMock); const { commands } = registerCommands(); - const select = vi - .fn() - .mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior.") - .mockResolvedValueOnce("No · Send feedback without files"); + const select = vi.fn().mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior."); const confirm = vi.fn().mockResolvedValue(true); await getCommand(commands, "feedback").handler("", { @@ -754,12 +739,15 @@ describe("Step Pi slash command adapters", () => { }, } as unknown as ExtensionCommandContext); - expect(select).toHaveBeenCalledTimes(2); + expect(select).toHaveBeenCalledOnce(); expect(confirm).toHaveBeenCalledOnce(); - expect(fetchMock).toHaveBeenCalledOnce(); + // One report body plus one archive: the transcript and the log go with it. + expect(fetchMock).toHaveBeenCalledTimes(2); expect(fetchMock.mock.calls[0]?.[0]).toBe("https://feedback.test/feedback"); const body = JSON.parse(String(fetchMock.mock.calls[0]?.[1]?.body)) as Record; - expect(body.diagnostics).toBeUndefined(); + expect(body.diagnostics).toBeDefined(); + expect(body.sessionSummary).toBeDefined(); + expect(body.surface).toBe("tui"); }); test("explains when no matching session archive can be attached", async () => { @@ -958,10 +946,7 @@ describe("Step Pi slash command adapters", () => { .mockResolvedValueOnce(new Response(null, { status: 409 })); vi.stubGlobal("fetch", fetchMock); const { commands } = registerCommands(); - const select = vi - .fn() - .mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior.") - .mockResolvedValueOnce("Yes · Include the listed files"); + const select = vi.fn().mockResolvedValueOnce("bug: Crash, error, hang, or broken behavior."); const input = vi.fn().mockResolvedValue("It failed"); const confirm = vi.fn().mockResolvedValue(true); const notify = vi.fn(); diff --git a/scripts/build-coding-agent-bundle.mjs b/scripts/build-coding-agent-bundle.mjs index af1d775aa..7f6ba4da2 100644 --- a/scripts/build-coding-agent-bundle.mjs +++ b/scripts/build-coding-agent-bundle.mjs @@ -120,12 +120,16 @@ function readStepBuildDefines() { const version = process.env.STEPCODE_BUILD_VERSION; const channel = process.env.STEPCODE_BUILD_CHANNEL; const commit = process.env.STEPCODE_BUILD_COMMIT; + // When this artifact was produced; a report from a stale install is a + // different investigation than one from the build that just shipped. + const buildTime = process.env.STEPCODE_BUILD_TIME; const feedbackEndpoint = process.env.STEPCODE_FEEDBACK_ENDPOINT; const feedbackBundleEndpoint = process.env.STEPCODE_FEEDBACK_BUNDLE_ENDPOINT; const aliases = [ ["STEPCODE_BUILD_VERSION", version], ["STEPCODE_BUILD_CHANNEL", channel], ["STEPCODE_BUILD_COMMIT", commit], + ["STEPCODE_BUILD_TIME", buildTime], ["STEPCODE_FEEDBACK_ENDPOINT", feedbackEndpoint], ["STEPCODE_FEEDBACK_BUNDLE_ENDPOINT", feedbackBundleEndpoint], ]; From 1c92b0a4679321a41ee112a5943aa46022e1c9ad Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Fri, 23 Oct 2026 13:09:06 +0800 Subject: [PATCH 2/5] fix(coding-agent): collapse a home path only at a real boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running the built CLI against a scratch session showed the home directory surviving into the report two ways. `$HOME` is often a symlinked prefix of the resolved path it appears in: with `HOME=/var/...` and a resolved cwd of `/private/var/...`, replacing the first occurrence produced `/private~/project` — a corrupted path that still names the machine's directory layout. Matching now requires a boundary before the match as well as after, and the resolved spelling of the home directory is collapsed too, which is the spelling a tool prints after `realpath`. A host that supplies no live facts also produced an empty `runtime: {}` in the summary, which reads as "nothing to say" rather than "nothing to add"; the field is omitted when empty. Covered by the boundary cases in the summary tests and by a local end-to-end run of the built CLI: body, archive, per-row metrics, offline retry and the no-bundle-endpoint path. --- docs/feedback-report.md | 2 + .../src/step/feedback/redact-home.ts | 40 +++++++++++++++---- .../src/step/feedback/session-summary.ts | 5 ++- .../test/feedback-summary.test.ts | 4 ++ 4 files changed, 43 insertions(+), 8 deletions(-) diff --git a/docs/feedback-report.md b/docs/feedback-report.md index fb4607918..bd0f39acf 100644 --- a/docs/feedback-report.md +++ b/docs/feedback-report.md @@ -50,6 +50,8 @@ Each line of the bundle's `events.jsonl` keeps the entry it always had and gains A tool result row gets `elapsedMs` (call issued → result recorded) only when the batch had a single call; a parallel batch shares one start, so a per-call duration would be invented. Rows with nothing to add keep their exact original bytes. +`turn` numbers the user messages **in this file**, including turns the user later rewound (the file keeps them); `sessionSummary.counts.turns` counts the live branch only. The two can differ by design, and the difference is itself a useful signal that the user navigated back before filing. Tool-output names come from that transcript, so the archive carries them even when the transcript is only a head-and-tail excerpt. + Sources: the token figures and cache reads are the message's own `usage`, which the session already records per assistant message — the bundle copy just derives the rate and puts it on the row. Timing, `requestId` and `outcome` are joined from the `step.usage_provenance` entry written directly after that message (`usageEntryId`); thinking time from `stepcode-web.thinking-timing`. The join runs in two passes because those entries follow the message they describe. **Size.** Only the bundle copy is annotated; the session file on disk is never rewritten. Measured on real sessions: +2.2% uncompressed, +1.2–1.8% gzipped. The transcript bound (24 MiB) applies to the pre-annotation excerpt, and the annotated copy is re-bounded if it would exceed it. diff --git a/packages/coding-agent/src/step/feedback/redact-home.ts b/packages/coding-agent/src/step/feedback/redact-home.ts index 5fb61943f..924ba6865 100644 --- a/packages/coding-agent/src/step/feedback/redact-home.ts +++ b/packages/coding-agent/src/step/feedback/redact-home.ts @@ -1,3 +1,4 @@ +import { realpathSync } from "node:fs"; import { homedir } from "node:os"; /** @@ -5,15 +6,40 @@ import { homedir } from "node:os"; * * A report is read by someone else, so an absolute path leaks the OS user name * on every line it appears. Collapsing keeps the path structure an investigator - * needs (`~/code/app/src/main.ts`) without carrying who the user is. Matching - * happens only at a path boundary, so `/Users/bob2` is not touched by a - * `/Users/bob` prefix. + * needs (`~/code/app/src/main.ts`) without carrying who the user is. + * + * Matching requires a path boundary on both sides. `$HOME` is often a symlinked + * prefix of a resolved path (`/var/...` inside `/private/var/...`), and matching + * there would produce `/private~/project`; `/Users/bob2` must not be touched by + * a `/Users/bob` prefix either. */ export function collapseHomePaths(value: string, home?: string): string { - const normalized = (home ?? homeDirectory()).replace(/[\\/]+$/u, ""); - if (normalized.length < 2) return value; - const escaped = normalized.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"); - return value.replace(new RegExp(`${escaped}(?=[\\\\/]|$|["'\\s])`, "gu"), "~"); + let output = value; + for (const candidate of homeSpellings(home ?? homeDirectory())) { + const escaped = candidate.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"); + output = output.replace(new RegExp(`(?= 2) spellings.add(resolved); + } catch { + // A home directory that cannot be resolved is still worth collapsing as given. + } + return [...spellings]; } function homeDirectory(): string { diff --git a/packages/coding-agent/src/step/feedback/session-summary.ts b/packages/coding-agent/src/step/feedback/session-summary.ts index f25009da3..e599bebbc 100644 --- a/packages/coding-agent/src/step/feedback/session-summary.ts +++ b/packages/coding-agent/src/step/feedback/session-summary.ts @@ -592,6 +592,9 @@ function createSummary(options: FeedbackSummaryOptions): { const runtime = options.runtime; const contextWindow = runtime?.contextWindow ?? state.request?.contextWindow; const model = state.model ?? requestModelOf(state.request); + // An empty object reads as "the host supplied nothing to say" and is only + // noise in a report. + const facts = runtime ? runtimeFacts(runtime, contextWindow) : undefined; return { ...(model ? { model } : {}), ...(state.thinkingLevel ? { thinkingLevel: state.thinkingLevel } : {}), @@ -604,7 +607,7 @@ function createSummary(options: FeedbackSummaryOptions): { }, compactions, ...(compactionTotals.passes > 0 ? { compactionTotals: { ...compactionTotals } } : {}), - ...(runtime ? { runtime: runtimeFacts(runtime, contextWindow) } : {}), + ...(facts && Object.keys(facts).length > 0 ? { runtime: facts } : {}), ...(state.thinkingMs && state.thinkingMs > 0 ? { thinkingMs: state.thinkingMs } : {}), recentErrors, lastTurnTools: lastTurnTools.slice(-LAST_TURN_TOOL_LIMIT).map(({ name, isError }) => ({ name, isError })), diff --git a/packages/coding-agent/test/feedback-summary.test.ts b/packages/coding-agent/test/feedback-summary.test.ts index 2d85a9096..514536824 100644 --- a/packages/coding-agent/test/feedback-summary.test.ts +++ b/packages/coding-agent/test/feedback-summary.test.ts @@ -494,6 +494,10 @@ describe("feedback session summary", () => { expect(collapseHomePaths("/Users/bob/code/app", "/Users/bob")).toBe("~/code/app"); expect(collapseHomePaths("/Users/bob2/code/app", "/Users/bob")).toBe("/Users/bob2/code/app"); expect(collapseHomePaths("see /Users/bob and /Users/bob/x", "/Users/bob")).toBe("see ~ and ~/x"); + // A home directory that is a symlinked prefix of the resolved path must not + // match inside it; the result would be a corrupted path in the report. + expect(collapseHomePaths("/private/var/tmp/project", "/var/tmp/project")).toBe("/private/var/tmp/project"); + expect(collapseHomePaths("cwd=/Users/bob/w", "/Users/bob")).toBe("cwd=~/w"); }); }); From c4c4dbe5e988be71204400fe27c5075c9646b602 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:05:40 +0800 Subject: [PATCH 3/5] test(coding-agent): make the keep-alive TTL wait bounded instead of racy The idle-expiry case advanced 1ms of virtual time at a time, up to 100 times, while waiting for a lane's final notification. That notification is sent only after the child's details are read from disk, so the wait was really a race against file I/O: it passes on an idle laptop and fails on a loaded CI runner, where the assertion then failed on `inputs[1]` with a bare TypeError instead of saying what happened. Advance in 10ms steps for a bounded virtual budget, fail the wait explicitly when it runs out, and let the assertion name the reply text and the notifications that did arrive. Behaviour under test is unchanged. --- .../test/step-subagent-events.test.ts | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/coding-agent/test/step-subagent-events.test.ts b/packages/coding-agent/test/step-subagent-events.test.ts index d479ccc46..eadf36904 100644 --- a/packages/coding-agent/test/step-subagent-events.test.ts +++ b/packages/coding-agent/test/step-subagent-events.test.ts @@ -1015,19 +1015,30 @@ test("idle keep-alive children are stopped after the TTL and a reply resumes qui }, })(api); const lane = laneIdOf(await startLane(tools, { agent: "general", task: "first" })); - const settle = async (count: number) => { - for (let i = 0; i < 100 && sent.filter((m) => m.details?.event === "background_done").length < count; i++) { - await vi.advanceTimersByTimeAsync(1); + /** + * A finished lane sends its notification only after reading the child's + * details from disk, so waiting on 1ms of virtual time is a race against + * file I/O that a loaded CI runner loses. Advance in steps for a bounded + * virtual budget instead, and report what did arrive. + */ + const settle = async (count: number): Promise => { + for (let i = 0; i < 500; i += 1) { + if (sent.filter((m) => m.details?.event === "background_done").length >= count) return true; + await vi.advanceTimersByTimeAsync(10); } + return false; }; - await settle(1); + const notifications = () => sent.map((m) => m.details?.event ?? "?").join(", ") || "none"; + expect(await settle(1), `notifications after the first lane: ${notifications()}`).toBe(true); await vi.advanceTimersByTimeAsync(LANE_IDLE_TTL_MS - 1_000); expect(stopped).toHaveLength(0); await vi.advanceTimersByTimeAsync(2_000); expect(stopped).toEqual([inputs[0]!.sessionId]); - await agentSend(tools, { lane_id: lane, action: "reply", prompt: "second" }); - await settle(2); + const reply = textOf(await agentSend(tools, { lane_id: lane, action: "reply", prompt: "second" })); + const resumed = await settle(2); + expect(resumed, `agent_send said: ${reply}; notifications: ${notifications()}`).toBe(true); + expect(inputs, `agent_send said: ${reply}`).toHaveLength(2); expect(inputs[1]!.sessionId).toBe(inputs[0]!.sessionId); // A planned idle stop is not reported as a crash restart. expect(sent.some((message) => message.details?.event === "background_restarted")).toBe(false); From 5da1097b47cfdac1adee6021a2ec33b9b13ead06 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:13:37 +0800 Subject: [PATCH 4/5] test(coding-agent): pin which project a feedback report may draw from `-C/--cwd` runs a session in one project while the process keeps its launch directory, and `prepareFeedbackEvidence` falls back to `process.cwd()` when a host names no project. A host that has the session (the terminal) therefore now passes the session's own `cwd`, so a report can only ever describe the conversation of the project it was filed from. Covered by the resolution cases in `feedback-cwd-project.test.ts` and by the terminal handler test: a session whose file does not exist yet contributes no summary rather than a neighbouring project's conversation. --- .../coding-agent/src/step/slash-commands.ts | 14 ++- .../test/feedback-cwd-project.test.ts | 95 +++++++++++++++++++ .../test/step-slash-commands.test.ts | 39 ++++++++ 3 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 packages/coding-agent/test/feedback-cwd-project.test.ts diff --git a/packages/coding-agent/src/step/slash-commands.ts b/packages/coding-agent/src/step/slash-commands.ts index 72844023f..4e847e3b7 100644 --- a/packages/coding-agent/src/step/slash-commands.ts +++ b/packages/coding-agent/src/step/slash-commands.ts @@ -478,11 +478,19 @@ function permissionFacts(value: string | undefined): { permissionPreset?: string return normalized === "" || normalized === "unknown" ? {} : { permissionPreset: normalized }; } -/** A minimal embedder may expose only the id; the bundle then resolves by id. */ -function sessionSourceOf(ctx: ExtensionCommandContext): { sessionFile?: string; sessionId: string } { +/** + * Where this session lives, for the evidence collector. + * + * `cwd` is the session's project, which under `-C/--cwd` is not the process's + * working directory. A session that has not written its file yet is found by + * id, and a guessed one only inside this project's folder. + * + * A minimal embedder may expose only the id; the bundle then resolves by id. + */ +function sessionSourceOf(ctx: ExtensionCommandContext): { sessionFile?: string; sessionId: string; cwd: string } { const manager = ctx.sessionManager; const sessionFile = typeof manager.getSessionFile === "function" ? manager.getSessionFile() : undefined; - return { ...(sessionFile ? { sessionFile } : {}), sessionId: manager.getSessionId() }; + return { ...(sessionFile ? { sessionFile } : {}), sessionId: manager.getSessionId(), cwd: ctx.cwd }; } function notifyFeedbackResult(ctx: ExtensionCommandContext, result: FeedbackSubmitResult): void { diff --git a/packages/coding-agent/test/feedback-cwd-project.test.ts b/packages/coding-agent/test/feedback-cwd-project.test.ts new file mode 100644 index 000000000..740cd7703 --- /dev/null +++ b/packages/coding-agent/test/feedback-cwd-project.test.ts @@ -0,0 +1,95 @@ +import { mkdir, mkdtemp, realpath, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, test } from "vitest"; +import { resolveFeedbackSession } from "../src/step/feedback/bundle.ts"; +import { prepareFeedbackEvidence } from "../src/step/feedback/evidence.ts"; + +/** + * The project a report is filed from decides which conversation a guessed + * session may attach. With `-C/--cwd` the session's project is not the + * process's working directory, so the caller has to say which one it means. + */ + +const roots: string[] = []; +afterEach(async () => { + await Promise.all(roots.splice(0).map((root) => rm(root, { recursive: true, force: true }))); +}); + +interface Fixture { + root: string; + sessions: string; + project: string; + other: string; + env: NodeJS.ProcessEnv; +} + +async function fixture(): Promise { + const root = await realpath(await mkdtemp(join(tmpdir(), "feedback-cwd-"))); + roots.push(root); + const sessions = join(root, "agent", "sessions"); + const project = join(root, "project"); + const other = join(root, "other"); + for (const directory of [project, other]) { + const folder = join(sessions, `--${directory.replace(/^\//, "").replace(/[/:]/g, "-")}--`); + await mkdir(folder, { recursive: true }); + await writeFile( + join(folder, "2026-10-23T00-00-00-000Z_session.jsonl"), + `${JSON.stringify({ type: "session", id: `session-${directory.split("/").pop()}`, cwd: directory })}\n`, + ); + } + return { + root, + sessions, + project, + other, + env: { + STEP_CODING_AGENT_DIR: join(root, "agent"), + STEP_CODING_AGENT_SESSION_DIR: sessions, + STEPCODE_STORAGE_ROOT_DIR: join(root, "storage"), + } as NodeJS.ProcessEnv, + }; +} + +describe("feedback session resolution follows the session's project", () => { + test("resolves inside the project the caller names", async () => { + const { project, env } = await fixture(); + const resolved = await resolveFeedbackSession({ env, cwd: project }); + expect(resolved?.sessionId).toBe("session-project"); + }); + + test("another project resolves its own session", async () => { + const { other, env } = await fixture(); + const resolved = await resolveFeedbackSession({ env, cwd: other }); + expect(resolved?.sessionId).toBe("session-other"); + }); + + test("the evidence describes the session of the project it was given", async () => { + const { other, env } = await fixture(); + const evidence = await prepareFeedbackEvidence({ + storageRootDir: env.STEPCODE_STORAGE_ROOT_DIR as string, + cwd: other, + withoutBundle: true, + withoutDiagnostics: true, + env, + }); + expect(evidence.sessionId).toBe("session-other"); + expect(evidence.summary?.cwd?.endsWith("/other")).toBe(true); + }); + + test("a project with no session of its own attaches nothing", async () => { + const { root, env } = await fixture(); + const empty = join(root, "empty-project"); + await mkdir(empty, { recursive: true }); + const evidence = await prepareFeedbackEvidence({ + storageRootDir: env.STEPCODE_STORAGE_ROOT_DIR as string, + cwd: empty, + withoutBundle: true, + withoutDiagnostics: true, + env, + }); + // The fixture has other project folders, so crossing them is refused. + expect(evidence.sessionId).toBeUndefined(); + expect(evidence.summary).toBeUndefined(); + }); +}); diff --git a/packages/coding-agent/test/step-slash-commands.test.ts b/packages/coding-agent/test/step-slash-commands.test.ts index fc78b77b4..6aa19ddef 100644 --- a/packages/coding-agent/test/step-slash-commands.test.ts +++ b/packages/coding-agent/test/step-slash-commands.test.ts @@ -512,6 +512,45 @@ describe("Step Pi slash command adapters", () => { }); }); + test("a session with no file yet is not replaced by another project's conversation", async () => { + // The session id is authoritative: when its file has not been written, the + // report must carry no conversation rather than a neighbouring one. This is + // also why `-C/--cwd` cannot change what a report attaches: that flag routes + // to a session, not to `step feedback`, and a run in another project still + // names its own session id. + const root = await mkdtemp(join(tmpdir(), "step-slash-feedback-")); + temporaryRoots.push(root); + const sessions = join(root, "agent", "sessions"); + const project = join(root, "project"); + const folder = join(sessions, `--${project.replace(/^\//, "").replace(/[/:]/g, "-")}--`); + await mkdir(folder, { recursive: true }); + await writeFile( + join(folder, "2026-10-23T00-00-00-000Z_other.jsonl"), + `${JSON.stringify({ type: "session", id: "other-session", cwd: project, parentId: null })}\n`, + ); + vi.stubEnv("STEP_CODING_AGENT_DIR", join(root, "agent")); + vi.stubEnv("STEP_CODING_AGENT_SESSION_DIR", sessions); + vi.stubEnv("STEPCODE_STORAGE_ROOT_DIR", root); + vi.stubEnv("STEPCODE_FEEDBACK_ENDPOINT", "https://feedback.test/feedback"); + const fetchMock = vi.fn().mockResolvedValue(new Response(null, { status: 204 })); + vi.stubGlobal("fetch", fetchMock); + const { commands } = registerCommands(); + + await getCommand(commands, "feedback").handler("unwritten", { + cwd: project, + hasUI: true, + ui: { notify: vi.fn(), confirm: vi.fn().mockResolvedValue(true) }, + sessionManager: { getSessionId: () => "unwritten-session", getSessionFile: () => undefined }, + } as unknown as ExtensionCommandContext); + + const body = JSON.parse(String(fetchMock.mock.calls[0]?.[1]?.body)) as { + context: { sessionId?: string }; + sessionSummary?: unknown; + }; + expect(body.sessionSummary).toBeUndefined(); + expect(body.context.sessionId).toBe("unwritten-session"); + }); + test("cancels immediately when comment input is dismissed with Esc", async () => { vi.stubEnv("STEPCODE_FEEDBACK_ENDPOINT", "https://feedback.test/feedback"); const fetchMock = vi.fn(); From f58a9396d62c134bac40dc877040857c21c3e9c7 Mon Sep 17 00:00:00 2001 From: liuedcson <332840681+liuedcson@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:17:27 +0800 Subject: [PATCH 5/5] test(cli): stop killing a cold CLI start on a slow runner The context-projection case spawns the built CLI and SIGKILLs it after 20 s. That guard exists to end a hang, but 20 s is also roughly a cold start on a loaded CI runner, so the child was killed while still booting and the failure read as a session with no requests rather than as a timeout. Raise the guard to 90 s; the assertions are unchanged. --- apps/cli/test/runtime-context-overrides.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/cli/test/runtime-context-overrides.test.ts b/apps/cli/test/runtime-context-overrides.test.ts index 7ffb2ae28..0b0d648f7 100644 --- a/apps/cli/test/runtime-context-overrides.test.ts +++ b/apps/cli/test/runtime-context-overrides.test.ts @@ -43,7 +43,9 @@ async function firstRequest(config: string, flags: string[]) { const child = spawn(process.execPath, args, { cwd, env: { PATH: process.env.PATH, HOME: home, USERPROFILE: home, STEP_CODING_AGENT_DIR: agentDir, STEP_NO_LOCAL_LLM: "1", AWS_EC2_METADATA_DISABLED: "true" }, stdio: ["ignore", "pipe", "pipe"] }); let output = ""; child.stdout.on("data", (chunk) => { output += chunk; }); child.stderr.on("data", (chunk) => { output += chunk; }); - const timeout = setTimeout(() => child.kill("SIGKILL"), 20000); + // A cold CLI start on a loaded CI runner is much slower than on a laptop; + // the SIGKILL guard is here to stop a hang, not to bound normal startup. + const timeout = setTimeout(() => child.kill("SIGKILL"), 90_000); const code = await new Promise((done, reject) => { child.once("error", reject); child.once("close", done); }); clearTimeout(timeout); expect(code, output.slice(-12000)).toBe(0);