fix(cloud-agent-next): diagnose kilo import failures#3632
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| function sanitizeImportDiagnostic(diagnostic: string, truncated: boolean): string { | ||
| const secrets = collectImportDiagnosticSecrets(); | ||
| let sanitized = stripUnsafeControlCharacters(diagnostic.replace(ANSI_ESCAPE_SEQUENCE, '')); |
There was a problem hiding this comment.
WARNING: When truncated=true, diagnostic is already "[REDACTED]" (set by readBoundedOutputTail), so the ANSI-strip and control-char-strip on this line are always a no-op in that branch. The truncated guard on line 180 then immediately overwrites sanitized with REDACTED anyway.
Consider moving the truncated early-exit before line 178 to make the flow explicit and avoid dead work:
function sanitizeImportDiagnostic(diagnostic: string, truncated: boolean): string {
if (truncated) return REDACTED;
const secrets = collectImportDiagnosticSecrets();
let sanitized = stripUnsafeControlCharacters(diagnostic.replace(ANSI_ESCAPE_SEQUENCE, ''));
// ... rest of redaction chain
}This is a clarity/correctness nit — the current code produces the correct output because retainSanitizedImportDiagnosticTail("[REDACTED]") returns "[REDACTED]" unchanged.
| } | ||
|
|
||
| function sanitizeImportDiagnostic(diagnostic: string, truncated: boolean): string { | ||
| const secrets = collectImportDiagnosticSecrets(); |
There was a problem hiding this comment.
SUGGESTION: collectImportDiagnosticSecrets() is called once per non-empty stream in logImportDiagnostics — potentially twice (once for stdout, once for stderr). Each call iterates process.env and may read KILOCODE_TOKEN_FILE from disk.
The secrets list is constant for the lifetime of a single import, so a single call shared between both sanitizeImportDiagnostic invocations would be cleaner. Either collect secrets outside sanitizeImportDiagnostic and pass them in, or call sanitizeImportDiagnostic once and cache.
| totalBytes += value.byteLength; | ||
| options?.observeBytes(value.byteLength); | ||
| if (truncated) continue; | ||
| if (totalBytes > maxBytes) { |
There was a problem hiding this comment.
WARNING: The function name readBoundedOutputTail implies keeping the tail (last N bytes) of the stream, but the actual behaviour when totalBytes > maxBytes is fail-closed: all previously retained bytes are discarded and truncated=true/REDACTED is returned. The actual tail-retention logic lives in the separate post-sanitisation step (retainSanitizedImportDiagnosticTail).
This is intentionally conservative and the tests confirm the design, but the name creates a misleading contract. Consider renaming to drainBoundedOutputStream or readBoundedOutput to reflect that the function drains the stream while enforcing a byte budget, with truncation on overflow. At minimum a doc comment would help.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Executive SummaryThe Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Reviewed by claude-4.6-sonnet-20260217 · 1,057,842 tokens Review guidance: REVIEW.md from base branch |
1 participant
Summary
kilo importfails during Cloud Agent session restore without exposing arbitrary CLI output through worker or UI error contracts.[REDACTED]marker.1 MiB, terminate imports after five minutes, and kill the detached process group so pipe-owning descendants cannot keep restore pending.restore-session: kilo import diagnosticstriage landmark for sandbox wrapper logs and uploaded archives.Verification
kiloharness rather than a live Cloud Agent sandbox.Visual Changes
N/A
Reviewer Notes
RestoreResult.errorstays generic, so CLI output does not propagate into worker state, WebSocket events, or user-visible errors.4096bytes. Truncated streams discard raw preview bytes and log[REDACTED]with byte counts.1 MiB.