fix(desktop): Send Diagnostics review fixes — consent accuracy, log-grade redaction, dismissal guard, linkless-success (review feedback)
Addresses @helix4u's review on #92020: - Consent notice now matches the real --nous contract: full logs up to 512KB each, likely conversation content/tool outputs/file paths, viewable by Nous staff AND allowlisted Discord moderators (all 5 locales). - Client-supplied text (error_context + extra_files) rides _redact_log_text — the same upload-safe redactor as backend logs (secrets + email masking), not the weaker bare secret pass; regression test covers both. - ok:true without view_url or id becomes a structured failure; a returned id without a link renders an upload-ID fallback the user can quote. - Generation guard in the store: dismissal is immediate in every phase (incl. mid-upload); a stale completion can no longer resurrect or overwrite the dialog. Cancel button never disabled.
This commit is contained in:
@@ -47,7 +47,11 @@ export function SendDiagnosticsHost() {
|
||||
const busy = state.phase === 'uploading'
|
||||
|
||||
return (
|
||||
<Dialog onOpenChange={open => (!open && !busy ? dismissSendDiagnostics() : undefined)} open>
|
||||
// Dismissal is allowed in EVERY phase, including mid-upload: the store's
|
||||
// generation guard makes a dismissed upload's completion a no-op, so Esc/
|
||||
// backdrop/Cancel are always an immediate way out (cancellation of the
|
||||
// in-flight request itself stays best-effort).
|
||||
<Dialog onOpenChange={open => (!open ? dismissSendDiagnostics() : undefined)} open>
|
||||
<DialogContent className="max-w-[30rem]">
|
||||
{state.phase === 'consent' || state.phase === 'uploading' ? (
|
||||
<>
|
||||
@@ -61,7 +65,7 @@ export function SendDiagnosticsHost() {
|
||||
</DialogDescription>
|
||||
</DialogHeader>
|
||||
<DialogFooter>
|
||||
<Button disabled={busy} onClick={dismissSendDiagnostics} variant="ghost">
|
||||
<Button onClick={dismissSendDiagnostics} variant="ghost">
|
||||
{copy.cancel}
|
||||
</Button>
|
||||
<Button disabled={busy} onClick={() => void confirmSendDiagnostics()}>
|
||||
@@ -98,12 +102,16 @@ export function SendDiagnosticsHost() {
|
||||
<DialogTitle>{copy.doneTitle}</DialogTitle>
|
||||
<DialogDescription className="text-left">{copy.doneDescription}</DialogDescription>
|
||||
</DialogHeader>
|
||||
{state.result?.viewUrl && (
|
||||
{(state.result?.viewUrl || state.result?.uploadId) && (
|
||||
<div className="flex items-center gap-2 rounded-md border border-(--ui-stroke-tertiary) px-3 py-2">
|
||||
<code className="min-w-0 flex-1 truncate text-[0.78rem] text-(--ui-text-secondary)">
|
||||
{state.result.viewUrl}
|
||||
{state.result.viewUrl ?? copy.uploadIdFallback(state.result.uploadId ?? '')}
|
||||
</code>
|
||||
<CopyButton appearance="inline" label={copy.copyLink} text={state.result.viewUrl} />
|
||||
<CopyButton
|
||||
appearance="inline"
|
||||
label={copy.copyLink}
|
||||
text={state.result.viewUrl ?? state.result.uploadId ?? ''}
|
||||
/>
|
||||
</div>
|
||||
)}
|
||||
<div className="text-[0.8rem] text-(--ui-text-secondary)">{copy.handoffLead}</div>
|
||||
|
||||
@@ -4,12 +4,13 @@ export const ar = defineLocale({
|
||||
sendDiagnostics: {
|
||||
title: 'إرسال التشخيصات إلى Nous',
|
||||
privacyNotice:
|
||||
'سيؤدي هذا إلى رفع حزمة تصحيح إلى التخزين الداخلي لدى Nous (ليست لصيقة عامة). تتضمن معلومات النظام (نظام التشغيل، الإصدارات، المزوّد — وليس مفاتيح API الخاصة بك أبداً) وسجلات حديثة للوكيل والبوابة وسطح المكتب، وقد تحتوي على محتوى المحادثات ومسارات الملفات. تُحجب الأسرار قبل الرفع. لا يمكن الاطلاع عليها إلا لموظفي Nous، وتُحذف تلقائياً بعد 14 يوماً.',
|
||||
'سيؤدي هذا إلى رفع حزمة تصحيح إلى التخزين الداخلي لدى Nous (ليست لصيقة عامة). تتضمن معلومات النظام (نظام التشغيل، الإصدارات، المزوّد، وأنواع مفاتيح API المُهيأة — وليس المفاتيح نفسها أبداً) والسجلات الكاملة للوكيل والبوابة وسطح المكتب (حتى 512 كيلوبايت لكل منها، ومن المرجح أن تحتوي على محتوى المحادثات ومخرجات الأدوات ومسارات الملفات). تُحجب الأسرار قبل الرفع. لا يمكن الاطلاع عليها إلا لموظفي Nous ومشرفي Discord المعتمدين، وتُحذف تلقائياً بعد 14 يوماً.',
|
||||
upload: 'رفع',
|
||||
uploading: 'جارٍ الرفع…',
|
||||
cancel: 'إلغاء',
|
||||
close: 'إغلاق',
|
||||
copyLink: 'نسخ الرابط',
|
||||
uploadIdFallback: id => `لم يتم إرجاع رابط عرض — اذكر معرّف الرفع ${id} للدعم`,
|
||||
doneTitle: 'تم إرسال التشخيصات',
|
||||
doneDescription: 'تم رفع الحزمة بشكل خاص. شارك الرابط أدناه في محادثة الدعم لكي يتمكن الفريق من رؤية سجلاتك.',
|
||||
failedTitle: 'فشل الرفع',
|
||||
|
||||
@@ -213,12 +213,13 @@ export const en: Translations = {
|
||||
sendDiagnostics: {
|
||||
title: 'Send diagnostics to Nous',
|
||||
privacyNotice:
|
||||
'This uploads a debug bundle to Nous-internal storage (not a public paste). It includes system info (OS, versions, provider — never your API keys) and recent agent, gateway, and desktop logs, which may contain conversation content and file paths. Secrets are redacted before upload. Only Nous staff can view it, and it auto-deletes after 14 days.',
|
||||
'This uploads a debug bundle to Nous-internal storage (not a public paste). It includes system info (OS, versions, provider, which API keys are configured — never the keys themselves) and full agent, gateway, and desktop logs (up to 512 KB each), which likely contain conversation content, tool outputs, and file paths. Secrets are redacted before upload. The bundle is viewable only by Nous staff and allowlisted Discord moderators, and auto-deletes after 14 days.',
|
||||
upload: 'Upload',
|
||||
uploading: 'Uploading…',
|
||||
cancel: 'Cancel',
|
||||
close: 'Close',
|
||||
copyLink: 'Copy link',
|
||||
uploadIdFallback: id => `No view link returned — quote upload ID ${id} to support`,
|
||||
doneTitle: 'Diagnostics sent',
|
||||
doneDescription: 'Your bundle was uploaded privately. Share the link below in your support thread so the team can see your logs.',
|
||||
failedTitle: 'Upload failed',
|
||||
|
||||
@@ -214,12 +214,13 @@ export const ja = defineLocale({
|
||||
sendDiagnostics: {
|
||||
title: 'Nous に診断情報を送信',
|
||||
privacyNotice:
|
||||
'デバッグバンドルを Nous 内部ストレージにアップロードします(公開ペーストではありません)。システム情報(OS、バージョン、プロバイダー — API キーは含まれません)と、最近のエージェント/ゲートウェイ/デスクトップのログ(会話内容やファイルパスを含む場合があります)が含まれます。シークレットはアップロード前にマスクされます。閲覧できるのは Nous スタッフのみで、14 日後に自動削除されます。',
|
||||
'デバッグバンドルを Nous 内部ストレージにアップロードします(公開ペーストではありません)。システム情報(OS、バージョン、プロバイダー、設定済み API キーの種類 — キー自体は含まれません)と、エージェント/ゲートウェイ/デスクトップの完全なログ(各最大 512 KB。会話内容、ツール出力、ファイルパスを含む可能性が高い)が含まれます。シークレットはアップロード前にマスクされます。閲覧できるのは Nous スタッフと許可された Discord モデレーターのみで、14 日後に自動削除されます。',
|
||||
upload: 'アップロード',
|
||||
uploading: 'アップロード中…',
|
||||
cancel: 'キャンセル',
|
||||
close: '閉じる',
|
||||
copyLink: 'リンクをコピー',
|
||||
uploadIdFallback: id => `表示リンクが返されませんでした — サポートにアップロード ID ${id} をお伝えください`,
|
||||
doneTitle: '診断情報を送信しました',
|
||||
doneDescription: 'バンドルは非公開でアップロードされました。サポートスレッドで以下のリンクを共有すると、チームがログを確認できます。',
|
||||
failedTitle: 'アップロードに失敗しました',
|
||||
|
||||
@@ -258,6 +258,7 @@ export interface Translations {
|
||||
cancel: string
|
||||
close: string
|
||||
copyLink: string
|
||||
uploadIdFallback: (id: string) => string
|
||||
doneTitle: string
|
||||
doneDescription: string
|
||||
failedTitle: string
|
||||
|
||||
@@ -207,12 +207,13 @@ export const zhHant = defineLocale({
|
||||
sendDiagnostics: {
|
||||
title: '向 Nous 傳送診斷資訊',
|
||||
privacyNotice:
|
||||
'這會將偵錯套件上傳到 Nous 內部儲存空間(並非公開貼上板)。內容包括系統資訊(作業系統、版本、服務商 — 絕不包含您的 API 金鑰)以及最近的 agent、gateway 與桌面端日誌(可能包含對話內容與檔案路徑)。上傳前會先遮罩機密資訊。僅 Nous 員工可檢視,14 天後自動刪除。',
|
||||
'這會將偵錯套件上傳到 Nous 內部儲存空間(並非公開貼上板)。內容包括系統資訊(作業系統、版本、服務商、已設定的 API 金鑰種類 — 絕不包含金鑰本身)以及完整的 agent、gateway 與桌面端日誌(每個最多 512 KB,很可能包含對話內容、工具輸出與檔案路徑)。上傳前會先遮罩機密資訊。僅 Nous 員工與獲准的 Discord 版主可檢視,14 天後自動刪除。',
|
||||
upload: '上傳',
|
||||
uploading: '上傳中…',
|
||||
cancel: '取消',
|
||||
close: '關閉',
|
||||
copyLink: '複製連結',
|
||||
uploadIdFallback: id => `未回傳檢視連結 — 請向支援人員提供上傳 ID ${id}`,
|
||||
doneTitle: '診斷資訊已傳送',
|
||||
doneDescription: '偵錯套件已私密上傳。在您的支援討論串中分享以下連結,團隊即可檢視您的日誌。',
|
||||
failedTitle: '上傳失敗',
|
||||
|
||||
@@ -207,12 +207,13 @@ export const zh: Translations = {
|
||||
sendDiagnostics: {
|
||||
title: '向 Nous 发送诊断信息',
|
||||
privacyNotice:
|
||||
'这会将调试包上传到 Nous 内部存储(并非公开粘贴板)。内容包括系统信息(操作系统、版本、服务商 — 绝不包含您的 API 密钥)以及最近的 agent、gateway 和桌面端日志(可能包含对话内容与文件路径)。上传前会先脱敏。仅 Nous 员工可查看,14 天后自动删除。',
|
||||
'这会将调试包上传到 Nous 内部存储(并非公开粘贴板)。内容包括系统信息(操作系统、版本、服务商、已配置的 API 密钥种类 — 绝不包含密钥本身)以及完整的 agent、gateway 和桌面端日志(每个最多 512 KB,很可能包含对话内容、工具输出与文件路径)。上传前会先脱敏。仅 Nous 员工与获准的 Discord 版主可查看,14 天后自动删除。',
|
||||
upload: '上传',
|
||||
uploading: '上传中…',
|
||||
cancel: '取消',
|
||||
close: '关闭',
|
||||
copyLink: '复制链接',
|
||||
uploadIdFallback: id => `未返回查看链接 — 请向支持人员提供上传 ID ${id}`,
|
||||
doneTitle: '诊断信息已发送',
|
||||
doneDescription: '调试包已私密上传。在您的支持会话中分享以下链接,团队即可查看您的日志。',
|
||||
failedTitle: '上传失败',
|
||||
|
||||
@@ -141,4 +141,36 @@ describe('send-diagnostics store', () => {
|
||||
|
||||
expect($sendDiagnostics.get()).toBeNull()
|
||||
})
|
||||
|
||||
it('dismissal mid-upload is immediate and a stale completion cannot resurrect the dialog', async () => {
|
||||
let resolveRequest: (value: unknown) => void = () => {}
|
||||
const request = vi.fn().mockImplementation(
|
||||
() => new Promise(resolve => (resolveRequest = resolve))
|
||||
)
|
||||
const restoreGateway = stubGateway(request as never)
|
||||
const restoreDesktop = stubDesktopLogs(null)
|
||||
|
||||
try {
|
||||
requestSendDiagnostics()
|
||||
const pending = confirmSendDiagnostics()
|
||||
|
||||
// Wait for the request to actually start, then dismiss mid-flight.
|
||||
await vi.waitFor(() => expect(request).toHaveBeenCalled())
|
||||
dismissSendDiagnostics()
|
||||
expect($sendDiagnostics.get()).toBeNull()
|
||||
|
||||
// The upload completes AFTER dismissal — it must not write back.
|
||||
resolveRequest({ ok: true, view_url: 'https://nas.example/view/stale' })
|
||||
await pending
|
||||
|
||||
expect($sendDiagnostics.get()).toBeNull()
|
||||
|
||||
// A NEW dialog opened after the stale completion is untouched by it.
|
||||
requestSendDiagnostics('fresh')
|
||||
expect($sendDiagnostics.get()?.phase).toBe('consent')
|
||||
} finally {
|
||||
restoreDesktop()
|
||||
restoreGateway()
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
@@ -33,12 +33,22 @@ export interface SendDiagnosticsState {
|
||||
|
||||
export const $sendDiagnostics = atom<SendDiagnosticsState | null>(null)
|
||||
|
||||
// Generation token: bumped on every open AND every dismiss. An in-flight
|
||||
// upload captures the generation it started under and only writes its
|
||||
// completion back when the token still matches — so dismissing mid-upload is
|
||||
// immediate and a stale completion can't resurrect or overwrite the dialog.
|
||||
// Request cancellation stays best-effort (the WS call runs to completion
|
||||
// server-side; we just ignore the result).
|
||||
let generation = 0
|
||||
|
||||
/** Open the consent modal. No network I/O happens until the user confirms. */
|
||||
export function requestSendDiagnostics(errorContext?: string): void {
|
||||
generation += 1
|
||||
$sendDiagnostics.set({ errorContext, phase: 'consent' })
|
||||
}
|
||||
|
||||
export function dismissSendDiagnostics(): void {
|
||||
generation += 1
|
||||
$sendDiagnostics.set(null)
|
||||
}
|
||||
|
||||
@@ -76,6 +86,11 @@ export async function confirmSendDiagnostics(): Promise<void> {
|
||||
return
|
||||
}
|
||||
|
||||
const startedGeneration = generation
|
||||
|
||||
// Only write back while the dialog the upload belongs to is still open.
|
||||
const stillCurrent = () => generation === startedGeneration
|
||||
|
||||
$sendDiagnostics.set({ ...current, phase: 'uploading' })
|
||||
|
||||
try {
|
||||
@@ -87,6 +102,10 @@ export async function confirmSendDiagnostics(): Promise<void> {
|
||||
|
||||
const extraFiles = await collectLocalExtras()
|
||||
|
||||
if (!stillCurrent()) {
|
||||
return
|
||||
}
|
||||
|
||||
const response = await gateway.request<ShareNousResponse>(
|
||||
'diagnostics.share_nous',
|
||||
{
|
||||
@@ -96,6 +115,10 @@ export async function confirmSendDiagnostics(): Promise<void> {
|
||||
SHARE_TIMEOUT_MS
|
||||
)
|
||||
|
||||
if (!stillCurrent()) {
|
||||
return
|
||||
}
|
||||
|
||||
if (!response.ok) {
|
||||
throw new Error(response.error || 'upload failed')
|
||||
}
|
||||
@@ -110,6 +133,10 @@ export async function confirmSendDiagnostics(): Promise<void> {
|
||||
}
|
||||
})
|
||||
} catch (error) {
|
||||
if (!stillCurrent()) {
|
||||
return
|
||||
}
|
||||
|
||||
$sendDiagnostics.set({
|
||||
...current,
|
||||
error: error instanceof Error ? error.message : String(error),
|
||||
|
||||
@@ -76,6 +76,40 @@ def test_share_nous_attaches_redacted_error_context(captured_upload):
|
||||
assert secret not in context, "secret leaked through error_context redaction"
|
||||
|
||||
|
||||
def test_share_nous_client_text_gets_upload_safe_log_redaction(captured_upload):
|
||||
"""Client artifacts must ride the SAME redactor as backend logs
|
||||
(_redact_log_text): secrets AND email addresses — not just the bare
|
||||
secret pass, which leaves emails through (review finding on #92020)."""
|
||||
secret = "sk-abc123def456ghi789jkl012mno345pqr678"
|
||||
result = _handler()(
|
||||
"rid-2b",
|
||||
{
|
||||
"error_context": "user reported by alice@example.com",
|
||||
"extra_files": {"desktop.log": f"login bob@example.com token={secret}"},
|
||||
},
|
||||
)
|
||||
assert result["result"]["ok"] is True
|
||||
|
||||
files = _envelope(captured_upload["blob"])["files"]
|
||||
assert "alice@example.com" not in files["error-context.txt"]
|
||||
assert "[REDACTED_EMAIL]" in files["error-context.txt"]
|
||||
assert "bob@example.com" not in files["client/desktop.log"]
|
||||
assert secret not in files["client/desktop.log"]
|
||||
|
||||
|
||||
def test_share_nous_linkless_success_is_a_failure(monkeypatch):
|
||||
"""ok:true with neither view_url nor id would strand the user with an
|
||||
unreferencable upload — surface it as a structured failure instead."""
|
||||
import hermes_cli.diagnostics_upload as du
|
||||
|
||||
monkeypatch.setattr(du, "share_to_nous", lambda blob: {})
|
||||
|
||||
result = _handler()("rid-2c", {})
|
||||
payload = result["result"]
|
||||
assert payload["ok"] is False
|
||||
assert "no view URL" in payload["error"]
|
||||
|
||||
|
||||
def test_share_nous_extra_files_sanitized_and_redacted(captured_upload):
|
||||
secret = "sk-abc123def456ghi789jkl012mno345pqr678"
|
||||
result = _handler()(
|
||||
|
||||
@@ -486,8 +486,11 @@ def _(rid, params: dict) -> dict:
|
||||
upload failures inline.
|
||||
"""
|
||||
try:
|
||||
from agent.redact import redact_sensitive_text
|
||||
from hermes_cli.debug import build_nous_bundle, collect_share_bundle
|
||||
from hermes_cli.debug import (
|
||||
_redact_log_text,
|
||||
build_nous_bundle,
|
||||
collect_share_bundle,
|
||||
)
|
||||
from hermes_cli.diagnostics_upload import share_to_nous
|
||||
|
||||
log_lines = params.get("log_lines")
|
||||
@@ -496,10 +499,14 @@ def _(rid, params: dict) -> dict:
|
||||
|
||||
bundle = collect_share_bundle(log_lines=log_lines, redact=True)
|
||||
|
||||
# Client-supplied text goes through the SAME upload-safe log redactor
|
||||
# as backend-collected logs (_redact_log_text = force secret redaction
|
||||
# + email masking) — never the weaker bare secret pass, so the remote
|
||||
# path can't upload what the CLI pipeline would have removed.
|
||||
error_context = params.get("error_context")
|
||||
if isinstance(error_context, str) and error_context.strip():
|
||||
bundle["error-context.txt"] = redact_sensitive_text(
|
||||
error_context.strip()[:8_000], force=True
|
||||
bundle["error-context.txt"] = _redact_log_text(
|
||||
error_context.strip()[:8_000]
|
||||
)
|
||||
|
||||
# Client-side artifacts (local desktop.log on remote connections).
|
||||
@@ -520,18 +527,25 @@ def _(rid, params: dict) -> dict:
|
||||
safe_label = safe_label.lstrip(".").strip()
|
||||
if not safe_label or not text.strip():
|
||||
continue
|
||||
bundle[f"client/{safe_label}"] = redact_sensitive_text(
|
||||
text[:524_288], force=True
|
||||
)
|
||||
bundle[f"client/{safe_label}"] = _redact_log_text(text[:524_288])
|
||||
|
||||
blob = build_nous_bundle(bundle, redact=True)
|
||||
res = share_to_nous(blob)
|
||||
view_url = res.get("viewUrl") or res.get("view_url")
|
||||
upload_id = res.get("id")
|
||||
if not view_url and not upload_id:
|
||||
# An upload the user can't reference is useless to support —
|
||||
# surface it as a failure instead of a linkless success.
|
||||
return _ok(
|
||||
rid,
|
||||
{"ok": False, "error": "upload succeeded but returned no view URL or id"},
|
||||
)
|
||||
return _ok(
|
||||
rid,
|
||||
{
|
||||
"ok": True,
|
||||
"view_url": res.get("viewUrl") or res.get("view_url"),
|
||||
"upload_id": res.get("id"),
|
||||
"view_url": view_url,
|
||||
"upload_id": upload_id,
|
||||
"expires_at": res.get("expiresAt") or res.get("expires_at"),
|
||||
},
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user