feat: add /feedback command for diagnostic data collection#1078
feat: add /feedback command for diagnostic data collection#1078
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
src/kimi_cli/feedback/collector.py
Outdated
| # Find ERROR lines | ||
| for line in content.split("\n"): | ||
| if "| ERROR" in line or "| CRITICAL" in line: | ||
| last_error = shorten(line.strip(), width=500) |
There was a problem hiding this comment.
🔴 last_error from log is not redacted, potentially leaking sensitive data
The last_error field extracted from log lines is not passed through redact_log_content(), unlike recent_exceptions which are redacted at line 180. Error log lines can contain sensitive data such as API keys or Bearer tokens (e.g., in HTTP error response messages logged at ERROR level), which would then be included unredacted in the feedback report sent to the server.
Root Cause
At src/kimi_cli/feedback/collector.py:171, the code assigns:
last_error = shorten(line.strip(), width=500)But a few lines below at src/kimi_cli/feedback/collector.py:180, traceback text IS redacted:
recent_exceptions.append(redact_log_content(tb_text))The redact_log_content function (src/kimi_cli/feedback/redact.py:57-62) strips patterns like sk-... API keys, Bearer ... tokens, and api_key=... values. Since last_error skips this redaction, any ERROR/CRITICAL log line containing these patterns would be sent in plaintext to the feedback API.
Impact: Sensitive credentials (API keys, bearer tokens) could be leaked to the feedback collection endpoint if they appear in error log lines.
| last_error = shorten(line.strip(), width=500) | |
| last_error = redact_log_content(shorten(line.strip(), width=500)) | |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
Adds a new interactive /feedback CLI command to collect and upload diagnostic reports (two-phase upload), and surfaces the feature in the shell UI (welcome text, status bar hint, and error toast hint).
Changes:
- Implement
/feedbackslash command flow with interactive prompts and Phase 1/Phase 2 upload. - Add
kimi_cli.feedbackmodule for diagnostics collection, redaction helpers, display formatting, and uploading (including local debug mode). - Add UI hints for
/feedbackin the bottom toolbar, welcome message, and after certain error conditions.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/kimi_cli/ui/shell/slash.py | Implements /feedback command (collection, confirmation UI, uploads). |
| src/kimi_cli/ui/shell/prompt.py | Adds /feedback shortcut text to bottom toolbar when space allows. |
| src/kimi_cli/ui/shell/init.py | Shows a toast hinting /feedback after provider/unexpected errors; updates welcome help text. |
| src/kimi_cli/feedback/uploader.py | Implements Phase 1 JSON + Phase 2 multipart upload (and local save mode). |
| src/kimi_cli/feedback/redact.py | Adds config/path/git/log redaction helpers. |
| src/kimi_cli/feedback/models.py | Adds Pydantic models for feedback payloads/responses. |
| src/kimi_cli/feedback/display.py | Renders feedback summary/details in Rich panels/pager. |
| src/kimi_cli/feedback/collector.py | Collects Phase 1 metadata + Phase 2 attachments (context/wire/log/source). |
| src/kimi_cli/feedback/init.py | Exposes feedback public API via re-exports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Read full context.jsonl, capped at MAX_CONTEXT_SIZE.""" | ||
| context_path = soul.context.file_backend | ||
| if not context_path.exists(): | ||
| return b"" | ||
| try: | ||
| return await asyncio.to_thread(_read_file_tail_bytes, context_path, MAX_CONTEXT_SIZE) | ||
| except Exception: | ||
| logger.debug("Failed to read context file") | ||
| return b"" | ||
|
|
||
|
|
||
| async def collect_phase2_wire_tail(soul: KimiSoul) -> bytes: | ||
| """Read last N lines of wire.jsonl.""" | ||
| wire_path = soul.runtime.session.wire_file.path | ||
| if not wire_path.exists(): | ||
| return b"" | ||
| try: | ||
| return await asyncio.to_thread(_read_file_tail_lines, wire_path, MAX_WIRE_LINES) |
There was a problem hiding this comment.
Phase 2 collectors return raw context.jsonl and wire.jsonl tails without any redaction/scrubbing, yet these files can contain embedded content (including uploaded file contents) and tool arguments that may include secrets. Add redaction (at least basic secret-pattern scrubbing) and/or filter out high-risk fields before uploading, or gate these attachments behind an explicit opt-in prompt separate from source.zip.
| """Read full context.jsonl, capped at MAX_CONTEXT_SIZE.""" | |
| context_path = soul.context.file_backend | |
| if not context_path.exists(): | |
| return b"" | |
| try: | |
| return await asyncio.to_thread(_read_file_tail_bytes, context_path, MAX_CONTEXT_SIZE) | |
| except Exception: | |
| logger.debug("Failed to read context file") | |
| return b"" | |
| async def collect_phase2_wire_tail(soul: KimiSoul) -> bytes: | |
| """Read last N lines of wire.jsonl.""" | |
| wire_path = soul.runtime.session.wire_file.path | |
| if not wire_path.exists(): | |
| return b"" | |
| try: | |
| return await asyncio.to_thread(_read_file_tail_lines, wire_path, MAX_WIRE_LINES) | |
| """Read full context.jsonl, capped at MAX_CONTEXT_SIZE, with basic redaction.""" | |
| context_path = soul.context.file_backend | |
| if not context_path.exists(): | |
| return b"" | |
| try: | |
| raw = await asyncio.to_thread(_read_file_tail_bytes, context_path, MAX_CONTEXT_SIZE) | |
| redacted = redact_log_content(raw.decode("utf-8", errors="replace")) | |
| return redacted.encode("utf-8") | |
| except Exception: | |
| logger.debug("Failed to read context file") | |
| return b"" | |
| async def collect_phase2_wire_tail(soul: KimiSoul) -> bytes: | |
| """Read last N lines of wire.jsonl with basic redaction.""" | |
| wire_path = soul.runtime.session.wire_file.path | |
| if not wire_path.exists(): | |
| return b"" | |
| try: | |
| raw = await asyncio.to_thread(_read_file_tail_lines, wire_path, MAX_WIRE_LINES) | |
| redacted = redact_log_content(raw.decode("utf-8", errors="replace")) | |
| return redacted.encode("utf-8") |
| console.print( | ||
| Panel( | ||
| Group(*renderables), | ||
| title="Feedback Summary (sensitive data redacted)", | ||
| border_style="cyan", | ||
| ) |
There was a problem hiding this comment.
The panel title says "sensitive data redacted", but the summary includes Phase 2 attachment sizes for context/wire/log and the flow can also upload source.zip, some of which are not fully redacted (and source.zip is unredacted by design). Consider tightening the wording to specify exactly what is redacted (e.g., Phase 1 metadata + log scrubbing) to avoid giving users a false sense of safety.
| def redact_config(config: Config) -> dict[str, Any]: | ||
| """Serialize config and redact all sensitive fields.""" | ||
| data = config.model_dump(mode="json", exclude_none=True) | ||
|
|
||
| # Redact providers | ||
| for provider_data in data.get("providers", {}).values(): | ||
| if "api_key" in provider_data: | ||
| provider_data["api_key"] = REDACTED | ||
| if "oauth" in provider_data and provider_data["oauth"]: | ||
| provider_data["oauth"]["key"] = REDACTED | ||
| if "custom_headers" in provider_data: | ||
| provider_data["custom_headers"] = REDACTED | ||
| if "env" in provider_data: | ||
| provider_data["env"] = REDACTED | ||
|
|
||
| # Redact services | ||
| services = data.get("services", {}) | ||
| if services: | ||
| for svc_name in ("moonshot_search", "moonshot_fetch"): | ||
| svc = services.get(svc_name) | ||
| if not svc: | ||
| continue | ||
| if "api_key" in svc: | ||
| svc["api_key"] = REDACTED | ||
| if "custom_headers" in svc: | ||
| svc["custom_headers"] = REDACTED | ||
| if "oauth" in svc and svc["oauth"]: | ||
| svc["oauth"]["key"] = REDACTED | ||
|
|
||
| return data | ||
|
|
||
|
|
||
| def anonymize_path(path: str) -> str: | ||
| """Replace home directory with ~.""" | ||
| home = str(Path.home()) | ||
| if path.startswith(home): | ||
| return "~" + path[len(home) :] | ||
| return path | ||
|
|
||
|
|
||
| def redact_git_url(url: str) -> str: | ||
| """Remove auth tokens from git URLs.""" | ||
| return re.sub(r"(https?://)([^@]+)@", r"\1***@", url) | ||
|
|
||
|
|
||
| def redact_log_content(content: str) -> str: | ||
| """Remove secret patterns from log content.""" | ||
| content = re.sub(r"(sk-)[a-zA-Z0-9]{10,}", r"\1***", content) | ||
| content = re.sub(r"(Bearer\s+)[a-zA-Z0-9._-]+", r"\1***", content) | ||
| content = re.sub(r"(api_key[\"\s=:]+)[\"']?[a-zA-Z0-9._-]+", r"\1***", content) | ||
| return content |
There was a problem hiding this comment.
New redaction helpers (redact_config/redact_git_url/redact_log_content/anonymize_path) are security-sensitive, but there are no unit tests validating that common secret patterns and config fields are reliably redacted. Add focused tests covering provider api_key, oauth.key, custom_headers, and log/token patterns to prevent regressions.
| async for record in soul.wire_file.iter_records(): | ||
| msg = record.to_wire_message() | ||
| if first_ts is None: | ||
| first_ts = record.timestamp | ||
| last_ts = record.timestamp | ||
|
|
There was a problem hiding this comment.
_collect_wire_metrics() iterates the entire wire.jsonl via soul.wire_file.iter_records() with no size/record cap. For long sessions this can make /feedback noticeably slow (replay_recent_history explicitly skips wire files >20MB); consider adding a similar size guard and/or scanning only the tail needed for summary metrics.
| async for record in soul.wire_file.iter_records(): | |
| msg = record.to_wire_message() | |
| if first_ts is None: | |
| first_ts = record.timestamp | |
| last_ts = record.timestamp | |
| # Avoid scanning excessively large wire files, which can make /feedback slow. | |
| # Mirror the behavior of replay_recent_history, which skips wire files >20MB. | |
| max_wire_bytes = 20 * 1024 * 1024 | |
| wire_path = getattr(soul.wire_file, "path", None) | |
| wire_size: int | None = None | |
| if wire_path is not None: | |
| try: | |
| wire_size = os.path.getsize(wire_path) | |
| except OSError: | |
| wire_size = None | |
| if wire_size is not None and wire_size > max_wire_bytes: | |
| logger.info( | |
| "Skipping wire scan for feedback metrics: wire file too large (%s bytes)", | |
| wire_size, | |
| ) | |
| else: | |
| async for record in soul.wire_file.iter_records(): | |
| msg = record.to_wire_message() | |
| if first_ts is None: | |
| first_ts = record.timestamp | |
| last_ts = record.timestamp |
| def _collect_chat_summary(soul: KimiSoul) -> ChatSummary: | ||
| """Get representative messages: first 3 + last 7 (deduped), each truncated.""" | ||
| history = soul.context.history | ||
| total_count = len(history) | ||
| messages: list[ChatMessage] = [] | ||
|
|
||
| # Select representative messages: first 3 + last 7 (dedup when overlapping) | ||
| selected = list(history) if total_count <= 10 else list(history[:3]) + list(history[-7:]) | ||
|
|
||
| for msg in selected: | ||
| # Extract text content | ||
| content_text = "" | ||
| try: | ||
| content_text = msg.extract_text(" ") | ||
| except Exception: | ||
| for part in msg.content: | ||
| if hasattr(part, "text"): | ||
| content_text += getattr(part, "text", "") | ||
| content_text = shorten(content_text, width=500, placeholder="...(truncated)") | ||
|
|
||
| # Extract tool call names | ||
| tool_call_names: list[str] | None = None | ||
| if msg.tool_calls: | ||
| tool_call_names = [tc.function.name for tc in msg.tool_calls] | ||
|
|
||
| messages.append( | ||
| ChatMessage( | ||
| role=msg.role, | ||
| content=content_text, | ||
| tool_calls=tool_call_names, | ||
| ) | ||
| ) | ||
|
|
||
| return ChatSummary(total_count=total_count, messages=messages) |
There was a problem hiding this comment.
Chat message excerpts collected in _collect_chat_summary() are uploaded in Phase 1 but are not run through any redaction/scrubbing. Since user/assistant messages can easily contain API keys, tokens, or credentials, this is a high-risk data leak; apply secret-pattern redaction (similar to redact_log_content) to content_text (and potentially tool_call_names/arguments if later added) before including it in Phase1Request.
| stdout, _ = await proc.communicate() | ||
| if proc.returncode == 0: | ||
| info.commit = stdout.decode().strip() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| ) | ||
| await proc.communicate() | ||
| info.dirty = proc.returncode != 0 | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| stdout, _ = await proc.communicate() | ||
| if proc.returncode == 0: | ||
| info.remote_url = redact_git_url(stdout.decode().strip()) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
src/kimi_cli/feedback/collector.py
Outdated
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as exc: | |
| # Git information is optional; failures here should not break feedback collection. | |
| logger.debug("Failed to count git tracked files for %s: %s", work_dir, exc) |
src/kimi_cli/ui/shell/slash.py
Outdated
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as e: | |
| # Phase 2 data is optional; log and continue with empty values. | |
| console.print(f"[yellow]Warning:[/yellow] Failed to collect optional diagnostic data: {e}") |
1 participant
Summary
Adds a new
/feedbackcommand that allows users to submit diagnostic reports directly from the CLI.Key Changes
New feedback module (
src/kimi_cli/feedback/):UI enhancements:
/feedbackslash command with interactive promptsUser Flow
/feedback(optionally with a message)Testing
KIMI_FEEDBACK_LOCAL_DIRenv varKIMI_FEEDBACK_API_BASEenv varChecklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.