feat: add local debug capability with session recording, evidence bundles, and agent-friendly CLI/MCP tools

[Postroggy]

Summary

为 SuperDuck 建设本地开发阶段可用的 debug 系统，解决跨域排障困难的问题。

核心变更

Debug Session 模型

• 引入 debugSessionId 将 CRX、native-host、MCP、CLI 的事件串联
• 按运行域（sidepanel / agent-loop / tool-executor / screenshot / ax / js / workflow / native 等）采集结构化事件
• 环形缓冲 + 文件大小限制，默认关闭、本地保存、不上传

Evidence Bundle 导出

• 失败发生后可导出包含截图、AX 文本、JS 输出、tab 状态、native 状态的诊断材料包
• 支持 redaction 避免敏感信息落盘

CLI + MCP 接入

• 新增 superduck debug start/stop/status/collect/doctor CLI 子命令
• 注册 superduck_debug_status/collect/snapshot MCP tools，agent 可一键收集诊断材料
• sidepanel options UI 支持开关 debug 模式与 auto-start

File Server Bridge

• localhost HTTP file server 实现 native host ↔ CRX 大文件传输（transport cap 提升至 32MB）
• Go 侧 event buffer 解耦 CRX 与 native host 的事件流

E2E 测试

• 新增 real-LLM e2e 测试套件与 agent loop debug instrumentation
• 记录 4 个已知未修复 bug（BUG-001/004 等）供后续修复跟踪

文件结构

chrome-crx/src/debug/          # CRX 侧 debug 模块
chrome-native-host/internal/debugbundle/  # Go 侧 bundle 收集
chrome-native-host/cmd/superduck/cmd_debug.go  # CLI 子命令
docs/debug-capability-plan.md  # 完整设计文档

Test plan

• bun run test — vitest 单元测试通过（623 tests passed）
• bun run build:edge — 扩展构建通过
• 手动开启 debug 模式，触发 agent 任务，验证 bundle 导出内容完整
• 验证 MCP tools 注册后可被 agent 调用
• E2E：docker exec 进 webtop 容器运行 Playwright 套件

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added “Debug evidence recording” controls (enable/disable with session info), plus evidence collection/export with improved summaries.
  • Added local file upload support via the CLI (push-file) with supporting transfer behavior.
  • Expanded real-LLM end-to-end test coverage across browsing, search, forms, and extraction.

• Bug Fixes

  • Improved debug evidence capture for native/debugger failures and timeouts, including safer handling of large bundles.
  • Enhanced runtime/tool evidence artifacts to better surface runtime errors.

• Documentation

  • Added detailed debug system design and capability planning documentation.

[duckpr]

Calling os.Exit(1) directly here bypasses all deferred cleanup in main(): analytics event emission (emitAndFlush at line 295), the self-update hint check (lines 297–306), and error tracking/sentry breadcrumbs (lines 308–328). The standard pattern in this CLI is to return an error and let main() handle exit codes. Consider returning a sentinel error (e.g. errDoctorFailed) and have main() map it to exit code 1, or have cmdDebugDoctor return (bool, error) so the caller controls the exit.

Suggestion:

Suggested change

	if !report.OK {
	os.Exit(1)
	}
	return nil
	}
	if !report.OK {
	return errDoctorFailed
	}
	return nil
	}

[duckpr]

os.Stat errors are not checked with os.IsNotExist. Any I/O error (e.g., permission denied on the parent directory) will be silently treated as 'manifest not found', misleading the user. Only a genuine 'not found' should count as missing.

Suggestion:

Suggested change

	if _, err := os.Stat(p); err == nil {
	if _, err := os.Stat(p); err == nil {
	// manifest exists
	} else if !os.IsNotExist(err) {
	// I/O error other than not-exists; surface it
	manifestCheck.Status = "fail"
	manifestCheck.Message = fmt.Sprintf("cannot stat manifest path %s: %v", p, err)
	report.OK = false
	report.Checks = append(report.Checks, manifestCheck)
	continue
	} else {

[duckpr]

The usage message omits the enable and disable subcommands, which are actually implemented and routed in the switch statement below. This inconsistency can confuse users.

Suggestion:

Suggested change

	return fmt.Errorf("usage: superduck debug <start|stop|status|collect|doctor>")
	return fmt.Errorf("usage: superduck debug <enable|disable|start|stop|status|collect|doctor>")

[duckpr]

Potential nil pointer dereference: s.recorder.Events() will panic if s.recorder is nil. While NewServer() currently always initializes recorder, the companion method enrichDebugBundle in the same file defensively checks if s.recorder == nil { return rawResponse }. For consistency and defensive safety, the same nil-guard should be applied here.

Suggestion:

Suggested change

	func (s *Server) handleGetGoDebugEvents(raw []byte) {
	var req struct {
	Limit int `json:"limit"`
	}
	_ = json.Unmarshal(raw, &req)
	events := s.recorder.Events()
	func (s *Server) handleGetGoDebugEvents(raw []byte) {
	var req struct {
	Limit int `json:"limit"`
	}
	_ = json.Unmarshal(raw, &req)
	if s.recorder == nil {
	s.sendToChrome(map[string]any{
	"type": "go_debug_events_response",
	"events": []any{},
	"count": 0,
	})
	return
	}
	events := s.recorder.Events()

[duckpr]

Security: Raw audit log lines are sent to Chrome without redaction. The companion method injectAuditLog applies redactAuditLine to strip URL query strings (which may contain tokens or sensitive params), but handleGetAuditLog sends raw lines directly. This creates an inconsistent security posture — the same audit data is redacted when bundled but exposed raw when fetched individually. The AuditRecord struct includes url and error fields that may contain sensitive data.

Suggestion:

Suggested change

	if err != nil {
	s.sendToChrome(map[string]any{
	"type": "audit_log_response",
	"error": err.Error(),
	})
	return
	}
	s.sendToChrome(map[string]any{
	"type": "audit_log_response",
	"lines": lines,
	})
	if err != nil {
	s.sendToChrome(map[string]any{
	"type": "audit_log_response",
	"error": err.Error(),
	})
	return
	}
	redactedLines := make([]json.RawMessage, len(lines))
	for i, line := range lines {
	redactedLines[i] = redactAuditLine(line)
	}
	s.sendToChrome(map[string]any{
	"type": "audit_log_response",
	"lines": redactedLines,
	})

[duckpr]

Diagnostic accuracy: The ts field uses time.Now() (the current bundling time) instead of the actual timestamp from the audit log line. The AuditRecord struct has a ts field recording when the event occurred. Using time.Now() makes the diagnostic bundle's timeline incorrect, which undermines its primary purpose of debugging sequence-of-events issues. The original timestamp from each audit line should be extracted and used instead.

Suggestion:

Suggested change

	auditEvent := map[string]any{
	"schemaVersion": 1,
	"eventId": debugrec.GenID(),
	"ts": time.Now().UTC().Format(time.RFC3339Nano),
	"debugSessionId": bundle.Session.DebugSessionID,
	"domain": "mcp-server",
	"event": "cli.audit_record",
	"level": "debug",
	"data": redactedData,
	}
	var auditRec struct {
	TS string `json:"ts"`
	}
	_ = json.Unmarshal([]byte(line), &auditRec)
	eventTS := auditRec.TS
	if eventTS == "" {
	eventTS = time.Now().UTC().Format(time.RFC3339Nano)
	}
	auditEvent := map[string]any{
	"schemaVersion": 1,
	"eventId": debugrec.GenID(),
	"ts": eventTS,
	"debugSessionId": bundle.Session.DebugSessionID,
	"domain": "mcp-server",
	"event": "cli.audit_record",
	"level": "debug",
	"data": redactedData,
	}

[duckpr]

Missing file size limit check: The file is read entirely into memory via os.ReadFile without any size guard. The native host's FileStore.Put enforces a 64 MB limit (DefaultMaxFileSize = 64 << 20 in internal/fileserver/store.go), but the CLI side will still load arbitrarily large files into memory before sending them over UDS — only to be rejected by the server. A large file (e.g., multi-GB) would cause excessive memory consumption or OOM in the CLI process. Consider checking info.Size() against the known server limit (64 MB) right after os.Stat and returning an early error before os.ReadFile.

Suggestion:

Suggested change

	info, err := os.Stat(absPath)
	if err != nil {
	return fmt.Errorf("stat file: %w", err)
	}
	if !info.Mode().IsRegular() {
	return fmt.Errorf("not a regular file: %s", absPath)
	}
	data, err := os.ReadFile(absPath)
	info, err := os.Stat(absPath)
	if err != nil {
	return fmt.Errorf("stat file: %w", err)
	}
	if !info.Mode().IsRegular() {
	return fmt.Errorf("not a regular file: %s", absPath)
	}
	const maxFileSize = 64 << 20 // matches server DefaultMaxFileSize
	if info.Size() > maxFileSize {
	return fmt.Errorf("file too large: %d bytes (max %d)", info.Size(), maxFileSize)
	}
	data, err := os.ReadFile(absPath)

[duckpr]

MIME type detection by extension only: detectMIME relies solely on the file extension, which can be spoofed (e.g., an executable named image.png). The Go standard library provides http.DetectContentType(data) which sniffs the actual file content. Since the file data is already loaded into data at this point, consider falling back to content sniffing for unknown types or using it as a primary detection method for better accuracy and safety.

Suggestion:

Suggested change

	filename := filepath.Base(absPath)
	mimeType := *mime
	if mimeType == "" {
	mimeType = detectMIME(filename)
	}
	filename := filepath.Base(absPath)
	mimeType := *mime
	if mimeType == "" {
	mimeType = detectMIME(filename)
	// For unknown types, sniff from content for better accuracy
	if mimeType == "application/octet-stream" {
	mimeType = http.DetectContentType(data)
	}
	}

[duckpr]

The file_server_info response returns s.udsAuth (the UDS auth token) to the caller. While UDS connections are already authenticated (they needed this token to connect), exposing the raw auth token back over the wire is an unnecessary security risk. If any intermediate process, log, or debug bundle captures this response, the token is leaked and could be used for unauthorized access to the file server or other UDS endpoints. Consider either omitting the token entirely (the caller already has it) or returning a separate, scoped file-server-only token.

Suggestion:

Suggested change

	_ = protocol.SendMessage(writer, map[string]any{
	"type": "file_server_info_response",
	"url": s.fileServer.BaseURL(),
	"port": s.fileServer.Port(),
	"token": s.udsAuth,
	})
	// The caller already possesses the UDS auth token (required to connect).
	// Return only connection metadata, not the token itself.
	_ = protocol.SendMessage(writer, map[string]any{
	"type": "file_server_info_response",
	"url": s.fileServer.BaseURL(),
	"port": s.fileServer.Port(),
	})

[duckpr]

The doc comment states "Non-URL strings are returned unchanged," but the implementation does not validate whether s is actually a URL. Any arbitrary string containing a ? (e.g., a log message like "Is this working?", a JSON snippet, or natural language text) will have everything after the first ? truncated and replaced with ?[redacted-query]. This can silently corrupt non-URL data in the debug bundle.

Additionally, URL fragments (e.g., https://example.com/path#access_token=xyz) are not redacted — fragments can also carry sensitive data.

Consider adding a lightweight URL sanity check (e.g., strings.HasPrefix(s, "http://") || strings.HasPrefix(s, "https://")) before stripping, and also handling the fragment portion.

Suggestion:

Suggested change

	func RedactURL(s string) string {
	idx := strings.IndexByte(s, '?')
	if idx < 0 {
	return s
	}
	return s[:idx] + "?[redacted-query]"
	}
	func RedactURL(s string) string {
	// Only redact strings that look like URLs to avoid corrupting
	// arbitrary log lines that happen to contain '?'.
	if !strings.HasPrefix(s, "http://") && !strings.HasPrefix(s, "https://") {
	return s
	}
	// Strip everything from the first '?' or '#'.
	idxQ := strings.IndexByte(s, '?')
	idxH := strings.IndexByte(s, '#')
	idx := -1
	if idxQ >= 0 {
	idx = idxQ
	}
	if idxH >= 0 && (idx < 0 || idxH < idx) {
	idx = idxH
	}
	if idx < 0 {
	return s
	}
	return s[:idx] + "?[redacted]"
	}

[duckpr]

Path Traversal Risk: strings.ReplaceAll(domain, "/", "-") only sanitizes forward slashes but does not protect against .. sequences or backslashes (\ on Windows). Since domain comes from JSON deserialization of the CRX bundle, a malicious or malformed domain key like ../../etc/cron.d could write files outside the events directory. Consider using filepath.Clean, validating against an allowlist of safe domain names, or rejecting keys containing .. or path separators.

Suggestion:

Suggested change

	fname := strings.ReplaceAll(domain, "/", "-") + ".jsonl"
	if strings.ContainsAny(domain, "\\/..") {
	continue // skip unsafe domain keys
	}
	fname := strings.ReplaceAll(domain, "/", "-") + ".jsonl"

[duckpr]

Path Traversal Risk: sanitizeFilename only replaces /, \, and : but does not handle .. sequences. If a.ID contains .. (e.g., ../../etc/passwd), the resulting path filepath.Join(sub, fname) could escape the intended artifacts/<subdir>/ directory, leading to arbitrary file writes. Since a.ID originates from JSON sent by the CRX, it should be treated as untrusted input. Consider using filepath.Base() or rejecting IDs containing ...

Suggestion:

Suggested change

	func sanitizeFilename(s string) string {
	r := strings.NewReplacer("/", "-", "\\", "-", ":", "-")
	return r.Replace(s)
	}
	func sanitizeFilename(s string) string {
	s = strings.NewReplacer("/", "-", "\\", "-", ":", "-").Replace(s)
	s = strings.ReplaceAll(s, "..", "")
	return filepath.Base(s)
	}

[duckpr]

API Design Issue: PrintDoctor takes *os.File as the writer parameter, which unnecessarily restricts it to file types. This makes the function untestable with *bytes.Buffer or other io.Writer implementations. Consider accepting io.Writer instead, which is the idiomatic Go pattern for output functions.

Suggestion:

Suggested change

	func PrintDoctor(r *DoctorReport, w *os.File) {
	func PrintDoctor(r *DoctorReport, w io.Writer) {

[duckpr]

The error from crypto/rand.Read is silently ignored. In the same codebase, internal/fileserver/store.go:188 handles this error explicitly (via panic), and internal/udsauth/udsauth.go:35 returns it. If rand.Read fails under low-entropy conditions, the buffer b remains all zeros, producing an ID like nh_000000000000000000000000 — leading to duplicate IDs across events and undermining the reliability of the debug evidence bundle. Recommend handling the error consistently with the rest of the codebase, e.g. by falling back to a timestamp-based component or at least logging a warning.

Suggestion:

Suggested change

	func GenID() string {
	var b [12]byte
	_, _ = rand.Read(b[:])
	return fmt.Sprintf("nh_%x", b[:])
	}
	func GenID() string {
	var b [12]byte
	if _, err := rand.Read(b[:]); err != nil {
	// Fall back to timestamp-based uniqueness if crypto/rand fails.
	binary.BigEndian.PutUint64(b[:8], uint64(time.Now().UnixNano()))
	}
	return fmt.Sprintf("nh_%x", b[:])
	}

[duckpr]

Reset zeroes count and head but leaves old Event structs (with their Data and IDs maps) referenced in the underlying events slice. These maps won't be garbage-collected until each slot is overwritten by a new event. For a 500-slot buffer this is minor, but explicitly nil-ing the slots on reset is a good hygiene practice to release memory promptly.

Suggestion:

Suggested change

	// Reset clears the buffer.
	func (r *Recorder) Reset() {
	r.mu.Lock()
	r.count = 0
	r.head = 0
	r.mu.Unlock()
	}
	// Reset clears the buffer.
	func (r *Recorder) Reset() {
	r.mu.Lock()
	for i := range r.events {
	r.events[i] = Event{}
	}
	r.count = 0
	r.head = 0
	r.mu.Unlock()
	}

[duckpr]

golangci-lint may not be installed on every developer's machine (it's a separate tool that requires make lint-install per AGENTS.md). With set -e, if the binary is missing the command returns 127 and the whole pre-commit hook fails, blocking the commit. The sibling script lint-staged-govet.sh doesn't have this problem because go is always available. Consider guarding the call so a missing golangci-lint is a warning rather than a hard failure (e.g. command -v golangci-lint >/dev/null 2>&1 || { echo 'golangci-lint not found, skipping'; exit 0; }), or at minimum document this requirement prominently.

Suggestion:

Suggested change

	set -e
	cd "$(dirname "$0")/../chrome-native-host"
	golangci-lint run --timeout=5m
	set -e
	if ! command -v golangci-lint >/dev/null 2>&1; then
	echo "warning: golangci-lint not found in PATH, skipping (run 'make lint-install' to install)" >&2
	exit 0
	fi
	cd "$(dirname "$0")/../chrome-native-host"
	golangci-lint run --timeout=5m

[duckpr]

Put() enforces a per-file size cap but there is no limit on the total number of entries or aggregate memory. A caller with the Bearer token can make many Put() calls within the 1-hour TTL window, each up to 64 MB, causing unbounded memory growth (OOM) before the cleanup loop evicts anything. Consider adding a MaxEntries (or MaxTotalSize) field to StoreConfig and rejecting Put() when the store is at capacity.

Suggestion:

Suggested change

	func (s *FileStore) Put(filename, mimeType string, data []byte) (string, error) {
	if int64(len(data)) > s.config.MaxFileSize {
	return "", ErrFileTooLarge
	}
	id := generateID()
	func (s *FileStore) Put(filename, mimeType string, data []byte) (string, error) {
	if int64(len(data)) > s.config.MaxFileSize {
	return "", ErrFileTooLarge
	}
	s.mu.Lock()
	if len(s.files) >= s.config.MaxEntries {
	s.mu.Unlock()
	return "", ErrStoreFull
	}
	id := generateID()
	// ... rest of insertion under the same lock
	s.files[id] = entry
	s.mu.Unlock()
	return id, nil
	}

[duckpr]

Get() returns a direct pointer to the internal FileEntry, whose Data slice is mutable. If any caller were to modify the returned Data, it would corrupt the store's internal state. Although current callers (handleGetFile and WriteFileToDisk) only read the data, this is a latent immutability hazard. Consider returning a copy of the entry (or at least a copy of the Data slice), or documenting that the returned *FileEntry must not be mutated.

Suggestion:

Suggested change

	func (s *FileStore) Get(id string) *FileEntry {
	s.mu.RLock()
	entry, ok := s.files[id]
	s.mu.RUnlock()
	if !ok {
	return nil
	}
	if time.Now().After(entry.ExpiresAt) {
	// Get returns a defensive copy of the file entry. The caller may freely
	// use or modify the returned Data without affecting the store.
	func (s *FileStore) Get(id string) *FileEntry {
	s.mu.RLock()
	entry, ok := s.files[id]
	if !ok {
	s.mu.RUnlock()
	return nil
	}
	// Check expiry under RLock first; only upgrade if deletion is needed.
	if time.Now().Before(entry.ExpiresAt) {
	copyEntry := *entry
	copyEntry.Data = append([]byte(nil), entry.Data...)
	s.mu.RUnlock()
	return &copyEntry
	}
	s.mu.RUnlock()
	// Expired – upgrade to write lock and delete.
	s.mu.Lock()
	if e, exists := s.files[id]; exists && time.Now().After(e.ExpiresAt) {
	delete(s.files, id)
	}
	s.mu.Unlock()
	return nil
	}

[duckpr]

Close() signals the cleanup goroutine to stop but does not clear the files map. On application shutdown, large file payloads (up to 64 MB each) will remain referenced and un-GC'd until the process exits. Consider clearing the map so the memory is released immediately.

Suggestion:

Suggested change

	func (s *FileStore) Close() {
	s.mu.Lock()
	defer s.mu.Unlock()
	if !s.stopped {
	s.stopped = true
	close(s.stopCh)
	}
	}
	func (s *FileStore) Close() {
	s.mu.Lock()
	defer s.mu.Unlock()
	if !s.stopped {
	s.stopped = true
	close(s.stopCh)
	}
	s.files = make(map[string]*FileEntry)
	}

[duckpr]

Security Risk (Stored XSS via client-controlled Content-Type): The Content-Type header for GET /f/{id} is derived from the uploader-supplied Content-Type header (see handlePutFile). If an attacker (or a compromised CRX) uploads HTML or SVG content with Content-Type: text/html or image/svg+xml, the browser will render active content when navigating to the URL. Since the server runs on 127.0.0.1, this is a same-origin XSS vector against any localhost service or against the browser's privileged origins.

Suggestion: Add X-Content-Type-Options: nosniff and a restrictive Content-Security-Policy header (e.g., default-src 'none') to handleGetFile responses. Better yet, force Content-Disposition: attachment or strip/deny dangerous MIME types (text/html, image/svg+xml, etc.) unless explicitly required.

Suggestion:

Suggested change

	w.Header().Set("Content-Type", entry.MIMEType)
	if entry.Filename != "" {
	safe := sanitizeHeaderValue(entry.Filename)
	w.Header().Set("Content-Disposition", fmt.Sprintf(`inline; filename=%q`, safe))
	}
	w.Header().Set("Content-Type", entry.MIMEType)
	w.Header().Set("X-Content-Type-Options", "nosniff")
	w.Header().Set("Content-Security-Policy", "default-src 'none'")
	if entry.Filename != "" {
	safe := sanitizeHeaderValue(entry.Filename)
	w.Header().Set("Content-Disposition", fmt.Sprintf(`inline; filename=%q`, safe))
	}

[duckpr]

Security Risk (Unrestricted file overwrite within home directory): validateWritePath ensures the target is within $HOME, but does not prevent overwriting sensitive files like ~/.ssh/authorized_keys, ~/.bashrc, ~/.aws/credentials, etc. If the targetPath is ever influenced by external/untrusted input (directly or transitively), this could lead to permanent system compromise via arbitrary file overwrite.

Suggestion: Consider restricting writes to a designated subdirectory (e.g., ~/.<appname>/downloads/) rather than the entire home tree. At minimum, add an explicit allowlist or denylist of protected paths (e.g., ~/.ssh, ~/.config, ~/.aws).

Suggestion:

Suggested change

	homeClean := filepath.Clean(home)
	if !strings.HasPrefix(clean, homeClean+string(filepath.Separator)) && clean != homeClean {
	return "", fmt.Errorf("path must be within home directory (%s): %s", homeClean, p)
	}
	homeClean := filepath.Clean(home)
	if !strings.HasPrefix(clean, homeClean+string(filepath.Separator)) && clean != homeClean {
	return "", fmt.Errorf("path must be within home directory (%s): %s", homeClean, p)
	}
	// Deny well-known sensitive paths to prevent accidental or malicious overwrite.
	for _, denied := range []string{filepath.Join(homeClean, ".ssh"), filepath.Join(homeClean, ".aws"), filepath.Join(homeClean, ".config")} {
	if strings.HasPrefix(clean, denied+string(filepath.Separator)) || clean == denied {
	return "", fmt.Errorf("write to sensitive path denied: %s", p)
	}
	}

[duckpr]

Reliability Issue (Silent serve failure): If s.server.Serve(listener) fails immediately (e.g., the listener is closed or another error occurs), the error is only logged inside the background goroutine. NewServer has already returned successfully, so the caller (and any code that subsequently sends file_server_ready to the CRX) will assume the server is alive. The CRX will then try to fetch from a dead URL and fail.

Suggestion: Use a readiness signal (e.g., a buffered channel or errgroup) so that NewServer blocks until the server is confirmed listening, or surface the serve error to the caller. For example, accept Serve's error synchronously or add a health check round-trip before returning.

Suggestion:

Suggested change

	go func() {
	if err := s.server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) {
	slog.Error("fileserver serve error", "error", err)
	}
	}()
	// Option A: verify readiness via a self-request before returning
	// Option B: propagate the error via a channel
	serveErr := make(chan error, 1)
	go func() {
	if err := s.server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) {
	serveErr <- err
	}
	}()
	// Caller can select on serveErr for early failure detection.

[duckpr]

The Data and Content fields have identical types (json.RawMessage) and overlapping names, but their intended purposes are different and undocumented. The TypeScript counterpart (DebugArtifact in schema.ts) clarifies: data is "Small inline metadata (≤64 KB), stored directly in the artifact record" while content is "Large payload stored in a separate content store, merged at export time." Without these comments, a Go maintainer has no way to know the distinction, and collect.go only reads Content (never Data) during writeArtifactContents — inline metadata is intentionally left in metadata.json. Please add the same clarifying comments to prevent confusion about which field to use.

Suggestion:

Suggested change

	Data json.RawMessage `json:"data,omitempty"`
	Content json.RawMessage `json:"content,omitempty"`
	// Small inline metadata (≤64 KB), stored directly in the artifact record.
	Data json.RawMessage `json:"data,omitempty"`
	// Large payload stored in a separate content store, merged at export time.
	Content json.RawMessage `json:"content,omitempty"`

[duckpr]

The file_server_ready message is sent directly via protocol.SendMessage(os.Stdout, ...), bypassing the sendToChrome() helper and its chromeOutput() indirection. While this works today because readChromeStdio hasn't started yet (it starts in server.Run()), this inconsistency could cause subtle bugs if the startup order changes. Consider using server.sendToChrome() or documenting why a direct stdout write is safe here.

Suggestion:

Suggested change

	if err := protocol.SendMessage(os.Stdout, map[string]any{
	"type": "file_server_ready",
	"url": fs.BaseURL(),
	"token": token,
	}); err != nil {
	if err := server.sendToChrome(map[string]any{
	"type": "file_server_ready",
	"url": fs.BaseURL(),
	"token": token,
	}); err != nil {

[duckpr]

DuckPR reviewer: ocr
Model: anthropic/glm-5.2

OpenCodeReview found 68 issue(s) in this PR.

• 67 posted as inline comment(s)
• 1 shown in this summary

Warnings:

• [object Object]
• [object Object]

---

chrome-native-host/cmd/native-host/main.go

Shown in summary because of missing line.

All audit log events are stamped with time.Now().UTC() at enrichment time rather than parsing the original timestamp from each audit line. This means the chronological order in the debug bundle will be wrong — all audit events will appear to have occurred at the moment of debug collection, making timeline correlation with other events misleading. Consider parsing the timestamp from the audit log JSON line (e.g., a ts or time field) and using that instead.

Suggested change

Before:

			"ts":             time.Now().UTC().Format(time.RFC3339Nano),

After:

// Parse ts from audit line if available; fall back to now.
var ts string
if parsed, ok := parseAuditTimestamp(line); ok {
    ts = parsed
} else {
    ts = time.Now().UTC().Format(time.RFC3339Nano)
}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

chrome-crx/src/options/components/PermissionsTab.tsx (2)

488-495: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Give the evidence checkbox an accessible name.

The label wraps only the hidden input and visual switch, so assistive tech cannot associate the control with “Debug evidence recording.”

Proposed fix

-                <div className="font-large text-text-100">
+                <div id="debug-evidence-recording-label" className="font-large text-text-100">
                   <FormattedMessage
                     defaultMessage="Debug evidence recording"
                     id="debug_evidence_recording"
                   />
                 </div>
-                <div className="text-text-400 font-base-sm mt-1">
+                <div id="debug-evidence-recording-description" className="text-text-400 font-base-sm mt-1">
                   {evidenceEnabled ? (
                     <FormattedMessage
                       defaultMessage="Recording all runtime events (tools, agent loop, CDP, screenshots). Export with: superduck debug collect"
                       id="debug_evidence_enabled_description"
@@
                   checked={evidenceEnabled}
+                  aria-labelledby="debug-evidence-recording-label"
+                  aria-describedby="debug-evidence-recording-description"
                   onChange={(event) => {
                     void toggleEvidence(event.target.checked);
                   }}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@chrome-crx/src/options/components/PermissionsTab.tsx` around lines 488 - 495,
The evidence toggle in PermissionsTab lacks an accessible name because the
surrounding label only wraps the hidden checkbox and switch UI. Update the
checkbox markup so the control is explicitly associated with “Debug evidence
recording” using an accessible label pattern in this component, preserving the
existing toggle behavior in toggleEvidence and the input/onChange wiring.

---

279-289: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Handle debug-status load failures.

load() can reject through getDebugStatusFromStorage(), and both callers intentionally discard the promise with void, which can produce unhandled rejections from the options page.

Proposed fix

     const load = async () => {
-      const status = await getDebugStatusFromStorage();
-      if (!mounted) return;
-      setEvidenceEnabled(status.enabled);
-      setEvidenceSessionId(status.session?.debugSessionId ?? null);
+      try {
+        const status = await getDebugStatusFromStorage();
+        if (!mounted) return;
+        setEvidenceEnabled(status.enabled);
+        setEvidenceSessionId(status.session?.debugSessionId ?? null);
+      } catch (err) {
+        if (mounted) {
+          console.error('[options] load evidence status failed:', err);
+        }
+      }
     };

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@chrome-crx/src/options/components/PermissionsTab.tsx` around lines 279 - 289,
The debug-status loader in PermissionsTab’s load() can reject and the current
void calls discard that promise, leading to unhandled rejections. Wrap the
getDebugStatusFromStorage() await in load() with error handling (or catch the
returned promise) and make sure both the initial load invocation and the
storage-change handler keep failures contained. Use the existing load, handler,
and getDebugStatusFromStorage symbols to place the fix.

🧹 Nitpick comments (4)

docs/debug-system-design.md (4)

556-566: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add text language specifier to ASCII art code block.

The "Go 启动" sequence diagram is a fenced code block without a language specifier, triggering markdownlint (MD040). Add text to satisfy linting and preserve plain-text rendering.

📝 Proposed fix

- ```
+ ```text
  Go 启动
    ├─ fileserver.Start() → 随机端口，生成 token
    ...
    └─ stdout: { type: "pong" }                            ← 时序 B

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @docs/debug-system-design.md around lines 556 - 566, The ASCII art sequence
diagram in the markdown code fence is missing a language specifier, which
triggers markdownlint MD040. Update the fenced block around the “Go 启动” diagram
in the docs so it uses a plain-text language tag, preserving the existing
content while making the block lint-compliant.

</details>

<!-- cr-comment:v1:fa36d0e7443d215eaa842722 -->

---

`656-664`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_

**Add `text` language specifier to bundle tree diagram.**

The bundle directory tree lacks a language specifier, triggering markdownlint (MD040).

<details>
<summary>📝 Proposed fix</summary>

```diff
- ```
+ ```text
  2026-06-30T123456Z-a1b2c3d4/
  ├── 00-readme.md          ← "阅读顺序：1→2→3→4→5"
  ...
  └── artifacts/            ← 截图/ax/js 的实体文件

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @docs/debug-system-design.md around lines 656 - 664, The bundle tree diagram
in the markdown section is missing a language specifier, causing MD040. Update
the fenced code block that shows the directory tree to use the text language
identifier so the diagram in the docs renders as a properly labeled code block.
Locate the tree snippet near the bundle structure example and adjust only the
fence, not the contents.

</details>

<!-- cr-comment:v1:1f86df3ffe27f7872ccb8ea6 -->

---

`606-611`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_

**Add `text` language specifier to design goals list.**

The design goals block (绑定地址, 认证, 存储, 并发) lacks a language specifier, triggering markdownlint (MD040).

<details>
<summary>📝 Proposed fix</summary>

```diff
- ```
+ ```text
  绑定地址:   127.0.0.1（loopback only）
  ...
  并发:       < 5 连接（只服务 CRX 一个客户端）

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @docs/debug-system-design.md around lines 606 - 611, The design goals code
block in the documentation is missing a language specifier, which triggers
markdownlint MD040. Update the fenced block around the bindings/认证/存储/并发 list to
use a text language tag so the block is explicitly marked as plain text, and
keep the content unchanged.

</details>

<!-- cr-comment:v1:305eb80d0277257495f7e18c -->

---

`326-330`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_

**Remove spurious code block around prose paragraph.**

The explanatory text starting with "每个域自定义自己的 data shape..." is wrapped in triple backticks but is not code. This triggers markdownlint (MD040) and renders the paragraph as monospaced code instead of normal body text.

<details>
<summary>📝 Proposed fix</summary>

```diff
- ```
  每个域自定义自己的 data shape，不进 schema。诊断规则通过 `e.data?.fieldName` 读取，如果字段不存在就回退到 undefined。这意味着**新增插桩点只需要在调用侧加字段，不需要改 schema.ts**。
- ```

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/debug-system-design.md` around lines 326 - 330, The prose paragraph
about “每个域自定义自己的 data shape...” is incorrectly wrapped in a code fence, causing
markdownlint MD040 and rendering it as monospaced text. Remove the stray triple
backticks around that paragraph in the markdown section near the event naming
examples, and keep the explanatory text as normal body prose while preserving
the surrounding list/content structure.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@chrome-crx/src/options/components/PermissionsTab.tsx`:
- Line 305: The `disableDebugEverywhere()` flow can report success even when
clearing the persistent auto-start state fails, leaving evidence recording able
to resume after restart. Update the `PermissionsTab` caller and/or the
`disableDebugEverywhere` helper so persistence is cleared successfully before
stopping the session, and surface any failure instead of swallowing it. Make
sure the UI only marks evidence recording as disabled after the persistent flag
has been cleared, using the `disableDebugEverywhere` and related
persistence-clearing logic as the main fix points.

---

Outside diff comments:
In `@chrome-crx/src/options/components/PermissionsTab.tsx`:
- Around line 488-495: The evidence toggle in PermissionsTab lacks an accessible
name because the surrounding label only wraps the hidden checkbox and switch UI.
Update the checkbox markup so the control is explicitly associated with “Debug
evidence recording” using an accessible label pattern in this component,
preserving the existing toggle behavior in toggleEvidence and the input/onChange
wiring.
- Around line 279-289: The debug-status loader in PermissionsTab’s load() can
reject and the current void calls discard that promise, leading to unhandled
rejections. Wrap the getDebugStatusFromStorage() await in load() with error
handling (or catch the returned promise) and make sure both the initial load
invocation and the storage-change handler keep failures contained. Use the
existing load, handler, and getDebugStatusFromStorage symbols to place the fix.

---

Nitpick comments:
In `@docs/debug-system-design.md`:
- Around line 556-566: The ASCII art sequence diagram in the markdown code fence
is missing a language specifier, which triggers markdownlint MD040. Update the
fenced block around the “Go 启动” diagram in the docs so it uses a plain-text
language tag, preserving the existing content while making the block
lint-compliant.
- Around line 656-664: The bundle tree diagram in the markdown section is
missing a language specifier, causing MD040. Update the fenced code block that
shows the directory tree to use the text language identifier so the diagram in
the docs renders as a properly labeled code block. Locate the tree snippet near
the bundle structure example and adjust only the fence, not the contents.
- Around line 606-611: The design goals code block in the documentation is
missing a language specifier, which triggers markdownlint MD040. Update the
fenced block around the bindings/认证/存储/并发 list to use a text language tag so the
block is explicitly marked as plain text, and keep the content unchanged.
- Around line 326-330: The prose paragraph about “每个域自定义自己的 data shape...” is
incorrectly wrapped in a code fence, causing markdownlint MD040 and rendering it
as monospaced text. Remove the stray triple backticks around that paragraph in
the markdown section near the event naming examples, and keep the explanatory
text as normal body prose while preserving the surrounding list/content
structure.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1da92cb9-54f2-4da3-a3de-71bf2154826d

📥 Commits

Reviewing files that changed from the base of the PR and between ac8688a and 6255bc6.

📒 Files selected for processing (23)

• .gitignore
• chrome-crx/e2e/helpers/realAgent.ts
• chrome-crx/e2e/helpers/realLLM.ts
• chrome-crx/src/background/fileServerBridge.ts
• chrome-crx/src/debug/diagnostics.ts
• chrome-crx/src/debug/exportBundle.ts
• chrome-crx/src/debug/recorder.ts
• chrome-crx/src/debug/redaction.ts
• chrome-crx/src/debug/runtimeMap.ts
• chrome-crx/src/debug/store.ts
• chrome-crx/src/mcpRuntime/inputTools/computerTool.ts
• chrome-crx/src/mcpRuntime/screenshot/annotatedScreenshot.ts
• chrome-crx/src/mcpRuntime/toolExecution/toolExecutor.ts
• chrome-crx/src/options/components/PermissionsTab.tsx
• chrome-crx/src/sidepanel/hooks/agentLoop/executeToolUses.ts
• chrome-crx/src/sidepanel/hooks/agentLoop/streamAndProcess.ts
• chrome-crx/src/sidepanel/hooks/useAgentLoop.ts
• chrome-crx/src/sidepanel/hooks/useSidepanelDebug.ts
• chrome-crx/test-results/.last-run.json
• chrome-native-host/cmd/superduck/cmd_upload_file.go
• chrome-native-host/internal/fileserver/server.go
• chrome-native-host/internal/fileserver/store.go
• docs/debug-system-design.md

💤 Files with no reviewable changes (1)

• chrome-crx/test-results/.last-run.json

✅ Files skipped from review due to trivial changes (3)

• chrome-crx/src/sidepanel/hooks/agentLoop/streamAndProcess.ts
• chrome-crx/src/sidepanel/hooks/agentLoop/executeToolUses.ts
• .gitignore

🚧 Files skipped from review as they are similar to previous changes (17)

• chrome-crx/src/sidepanel/hooks/useSidepanelDebug.ts
• chrome-crx/src/mcpRuntime/screenshot/annotatedScreenshot.ts
• chrome-crx/src/mcpRuntime/toolExecution/toolExecutor.ts
• chrome-crx/src/sidepanel/hooks/useAgentLoop.ts
• chrome-crx/src/mcpRuntime/inputTools/computerTool.ts
• chrome-native-host/cmd/superduck/cmd_upload_file.go
• chrome-crx/src/background/fileServerBridge.ts
• chrome-crx/src/debug/diagnostics.ts
• chrome-crx/src/debug/runtimeMap.ts
• chrome-crx/src/debug/store.ts
• chrome-crx/e2e/helpers/realAgent.ts
• chrome-crx/src/debug/exportBundle.ts
• chrome-native-host/internal/fileserver/store.go
• chrome-crx/e2e/helpers/realLLM.ts
• chrome-crx/src/debug/redaction.ts
• chrome-native-host/internal/fileserver/server.go
• chrome-crx/src/debug/recorder.ts

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not report evidence recording as disabled until persistence is cleared.

disableDebugEverywhere() currently swallows failures while clearing the persistent auto-start flag before stopping the session. That can leave evidence recording enabled again after a browser restart even though this UI appears to turn it off.

Proposed helper-side fix

 export async function disableDebugEverywhere(): Promise<DebugSessionMeta | null> {
-  try {
-    await chrome.storage.local.set({ [DEBUG_PERSISTENT_KEY]: false });
-  } catch {
-    // ignore
-  }
+  await chrome.storage.local.set({ [DEBUG_PERSISTENT_KEY]: false });
   return stopDebugSession();
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@chrome-crx/src/options/components/PermissionsTab.tsx` at line 305, The
`disableDebugEverywhere()` flow can report success even when clearing the
persistent auto-start state fails, leaving evidence recording able to resume
after restart. Update the `PermissionsTab` caller and/or the
`disableDebugEverywhere` helper so persistence is cleared successfully before
stopping the session, and surface any failure instead of swallowing it. Make
sure the UI only marks evidence recording as disabled after the persistent flag
has been cleared, using the `disableDebugEverywhere` and related
persistence-clearing logic as the main fix points.

[duckpr]

DuckPR reviewer: ocr
Model: anthropic/glm-5.2

OpenCodeReview found 45 issue(s) in this PR.

• 45 posted as inline comment(s)
• 0 shown in this summary

Warnings:

• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]
• [object Object]

---

Inline comments shown in summary

---

chrome-crx/src/background/fileServerBridge.ts (L54-L57)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

The file_server_ready handler only checks that url and token are strings via typeof, but does not verify they are non-empty. If the native host ever sends empty strings (e.g., due to a startup race condition or bug), url and token would be silently set to '', and fetchFileFromHost would then fail with a confusing 'file server not ready' error rather than catching the malformed message early. Consider adding an emptiness check (e.g., message.url && message.token) to reject invalid messages defensively.

Suggested change

Before:

        if (typeof message.url === 'string' && typeof message.token === 'string') {
          url = message.url;
          token = message.token;
        }

After:

        if (message.url && typeof message.url === 'string' && message.token && typeof message.token === 'string') {
          url = message.url;
          token = message.token;
        }

---

chrome-crx/e2e/helpers/realAgent.ts (L103-L103)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

This line is excessively long (~130+ characters), making it hard to read. Consider breaking it across multiple lines for better maintainability.

Suggested change

Before:

  const ctx: RealAgentCtx = { result, targetPage, sidepanel, serviceWorker: opts.serviceWorker, finalUrl, sdDebug, llmCalls: result.llmCalls };

After:

  const ctx: RealAgentCtx = {
    result,
    targetPage,
    sidepanel,
    serviceWorker: opts.serviceWorker,
    finalUrl,
    sdDebug,
    llmCalls: result.llmCalls,
  };

---

chrome-crx/src/debug/diagnostics.ts (L30-L30)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

The RuleFn type declares (events, artifacts) => DiagnosisFinding[], but none of the 13 rule functions accept artifacts, and the wrapper arrow functions in RULES silently drop the artifacts argument: (events) => nativeToolTimeoutNoCrxStart(events). This means artifacts are never actually passed to any rule, making the artifacts parameter in diagnose() and the RuleFn type misleading. Either:

1. If artifacts are not needed by any current rule, simplify RuleFn to (events: DebugBaseEvent[]) => DiagnosisFinding[] to avoid confusion.
2. If future rules are intended to use artifacts, pass them through: (events, artifacts) => nativeToolTimeoutNoCrxStart(events, artifacts).

Suggested change

Before:

type RuleFn = (events: DebugBaseEvent[], artifacts: DebugArtifact[]) => DiagnosisFinding[];

After:

// Option 1: Simplify if artifacts are never used
type RuleFn = (events: DebugBaseEvent[]) => DiagnosisFinding[];

// Option 2: If artifacts will be used, update rule signatures and wrappers accordingly

---

chrome-crx/src/debug/diagnostics.ts (L208-L208)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

The ts field is typed as string (ISO timestamp per DebugBaseEvent). The comparison n.ts <= s.ts performs lexicographic string comparison, which works for ISO 8601 format but is fragile. If timestamps could ever be in a different format, this would silently produce wrong results. Consider comparing via Date.parse() or using the monotonicMs field if available for reliable numeric comparison.

Suggested change

Before:

    const priorNav = navs.find((n) => n.ts <= s.ts && idsMatch(n, s, 'tabId'));

After:

    const priorNav = navs.find(
      (n) => new Date(n.ts).getTime() <= new Date(s.ts).getTime() && idsMatch(n, s, 'tabId')
    );

---

chrome-crx/src/debug/recorder.ts (L509-L510)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

Bug: When input.content is a TypedArray (e.g. Uint8Array) that is a view into a larger backing ArrayBuffer with byteOffset > 0, slicing .buffer from offset 0 copies the wrong region of memory. The stored artifact content will contain bytes from the beginning of the backing buffer rather than the actual view contents.

Fix: Use TypedArray.prototype.slice() which respects byteOffset and byteLength, instead of slicing the underlying .buffer. This also correctly handles the case where the view doesn't start at offset 0.

Suggested change

Before:

      } else if (ArrayBuffer.isView(input.content)) {
        storedContent = (input.content as ArrayBufferView).buffer.slice(0, MAX_ARTIFACT_BYTES);

After:

      } else if (ArrayBuffer.isView(input.content)) {
        const view = input.content as Uint8Array;
        storedContent = view.slice(0, MAX_ARTIFACT_BYTES);

---

chrome-crx/src/debug/recorder.ts (L480-L487)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

Nested ternary expressions are prohibited per the project's code quality standards — they hurt readability and are hard to debug. This is a triple-nested ternary. Refactor to use a helper function or an if/else block for clarity.

Suggested change

Before:

    const shaInput =
      input.content != null
        ? isBinary
          ? input.content
          : typeof input.content === 'string'
            ? input.content
            : JSON.stringify(input.content)
        : JSON.stringify(redactedData);

After:

    function computeShaInput(): string | ArrayBuffer | Uint8Array {
      const content = input.content;
      if (content == null) return JSON.stringify(redactedData);
      if (content instanceof ArrayBuffer || ArrayBuffer.isView(content)) return content;
      if (typeof content === 'string') return content;
      return JSON.stringify(content);
    }
    const shaInput = computeShaInput();

---

chrome-crx/src/debug/ringBuffer.ts (L50-L51)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

If a negative value is passed for n, Math.min(n, total) returns a negative number, and new Array(take) will throw RangeError: Invalid array length. Consider clamping take to 0 to guard against invalid input.

Suggested change

Before:

    const take = Math.min(n, total);
    if (take === 0) return [];

After:

    const take = Math.max(0, Math.min(n, total));
    if (take === 0) return [];

---

chrome-crx/src/debug/schema.ts (L29-L45)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

The DebugDomain type union and the DEBUG_DOMAINS array are maintained independently, which means a new domain added to one could be forgotten in the other without any compile-time error. Consider deriving the type from the array to guarantee they stay in sync, e.g.:

export const DEBUG_DOMAINS = [
  'sidepanel', 'agent-loop', ...
] as const;
export type DebugDomain = (typeof DEBUG_DOMAINS)[number];

This eliminates the risk of drift entirely while preserving all existing usages (the array still works for iteration, and the type still works for annotations).

Suggested change

Before:

export const DEBUG_DOMAINS: readonly DebugDomain[] = [
  'sidepanel',
  'agent-loop',
  'lightning',
  'tool-runtime',
  'permission',
  'tab-state',
  'cdp',
  'input',
  'screenshot-ref',
  'javascript',
  'workflow-recording',
  'native-bridge',
  'cli',
  'mcp-server',
  'diagnosis'
];

After:

export const DEBUG_DOMAINS = [
  'sidepanel',
  'agent-loop',
  'lightning',
  'tool-runtime',
  'permission',
  'tab-state',
  'cdp',
  'input',
  'screenshot-ref',
  'javascript',
  'workflow-recording',
  'native-bridge',
  'cli',
  'mcp-server',
  'diagnosis',
] as const;
export type DebugDomain = (typeof DEBUG_DOMAINS)[number];

---

chrome-crx/src/debug/redaction.ts (L228-L234)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

The _type parameter is accepted but never used in the function body. Since the caller (recorder.ts:470) passes input.type, this is either an incomplete implementation (type-specific redaction logic was planned but not yet implemented) or a dead parameter. If type-specific redaction is planned for the future, consider adding a TODO comment. If not, consider removing the parameter to avoid confusion.

Suggested change

Before:

export function redactArtifactData(
  data: unknown,
  _type: DebugArtifactType,
  opts: RedactionOptions = {}
): ArtifactRedactionResult {
  return { data: redactValue(data, opts), redacted: true };
}

After:

// TODO: implement type-specific redaction based on `_type` (e.g., screenshots vs text payloads)
export function redactArtifactData(
  data: unknown,
  _type: DebugArtifactType,
  opts: RedactionOptions = {}
): ArtifactRedactionResult {
  return { data: redactValue(data, opts), redacted: true };
}

---

chrome-crx/src/debug/redaction.ts (L198-L200)

GitHub could not post this inline: Unprocessable Entity: "Line could not be resolved"

The seen.delete(value) in the finally block removes the object reference after processing each branch. While this allows the same object to appear in multiple sibling positions without false-positive circular detection, it also means that a true circular reference through a shared sibling reference could slip through and cause unbounded recursion. The depth > maxDepth guard prevents an infinite loop, but for large maxDepth values with deeply aliased graphs, this could still cause a stack overflow. Consider whether removing the delete (accepting false positives for shared sibling references) or keeping it is the right trade-off for debug payload safety.

Suggested change

Before:

  } finally {
    seen.delete(value as object);
  }

After:

// Option 1: Remove the delete to guarantee circular safety (may over-report [Circular] for shared DAG nodes):
// // (remove finally block entirely)
//
// Option 2: Keep current behavior but document the trade-off:
  } finally {
    seen.delete(value as object); // allows sibling reuse but relies on maxDepth for cycle safety
  }

...and 35 more inline comment(s).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6255bc6b09

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Delay file_server_ready until after the port handshake

In the startup path this notification is sent before server.Run() starts reading stdin, so it is always delivered before the CRX can receive the pong that completes connect(). I checked chrome-crx/src/background/nativeHost.ts and the temporary handshake listener ignores every non-pong message, then the real handleNativeMessage listener is only installed after the handshake, so this one-shot file_server_ready carrying the URL/token is dropped and fetchFileFromHost() remains permanently “not ready” for that connection.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Cap debug collect responses at the native protocol limit

This serializer allows a bundle up to 32 MiB, but the Go side still uses protocol.ReadMessage for Chrome stdin/UDS responses and rejects any frame over 1 MiB (chrome-native-host/internal/protocol/chrome.go:21). In a normal debug session with screenshot artifacts or enough events, superduck_debug_collect can therefore send a >1 MiB tool_response, causing the native host/CLI read to fail with “message too large” instead of writing the bundle.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Avoid sending file bytes through the 1 MiB UDS frame

Putting the entire file byte slice into the control message means JSON encodes it as base64 and sends it through the same protocol.ReadMessage path that rejects payloads over 1 MiB. As a result, superduck push-file --path fails for files around 750 KiB or larger even though the new file store advertises a 64 MiB max, so the large-file transfer path is unusable for most non-trivial files.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Return an error when debug collect has no active session

When recording is disabled this sends a successful tool response whose content is just {"error":"no active debug session"}. The CLI then calls debugbundle.ParseBundleJSON, and Go's JSON decoder accepts that object as an empty Bundle, so superduck debug collect writes a misleading nosession bundle instead of failing and telling the user to start/enable debug recording.

Useful? React with 👍 / 👎.

[duckpr]

Run failed. View the logs →

  ^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using anthropic/glm-5.2 ｜ 𝕏}