Comfy-Org / Comfy-Org/ComfyUI_frontend

Narrow ReportErrorOptions.context so the no-PII rule is enforced, not just documented

Open
#17,290 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TypeScript
Stars
2k
Forks
699
Avg merge
1d 7h
Merged PRs (30d)
490

Description

## Problem / Goal

ADR-TELEMETRY-ERRORS-0030 rule 7 says diagnostics must carry no PII and limits `context` to "ids, counts, enums, and booleans". Nothing enforces that:

```ts
// src/platform/telemetry/reportError.ts
context?: Record
```

`unknown` accepts a whole response body, a workflow name, a file path or a token as readily as a boolean. The rule is a convention that reviewers have to catch by eye, on a call site count that keeps growing.

This matters more now that `reportError` writes a console line for every report, because `context` reaches Sentry, RUM and the console.

Current call sites mostly comply — `has_cached_plans`, `attribution_stage`, `workspace_still_current`, `checkout_attempt_id` — so a narrowing is unlikely to be a large migration. `url` on the resource-load report in `App.vue` is one known outlier to decide on.

## Proposed Solution

Narrow the type to what the ADR permits, roughly:

```ts
type DiagnosticValue = string | number | boolean | null
context?: Record
```

`string` still admits PII, so this is a guard rail rather than a proof — but it stops whole objects and response bodies at compile time, which is the case that actually leaks.

Audit existing call sites for outliers first; the change is only worth making if the migration is small.

## Acceptance Criteria

- [ ] `context` rejects a nested object or array of objects at compile time
- [ ] Every existing `reportError` call site compiles or is migrated
- [ ] A decision recorded for `App.vue`'s `url` value (keep, truncate, or drop)
- [ ] ADR rule 7 references the type as the enforcement point

---

Offered as the real fix on #17001 (https://github.com/Comfy-Org/ComfyUI_frontend/pull/17001#discussion_r3962162535), where the alternative suggestion was redacting `context` at the log site — which would remove most of the console line's diagnostic value without addressing the sinks.

Contributor guide

Open the contributing guide

Research direction

Start with src/platform/telemetry/reportError.ts and audit existing reportError call sites, including the resource-load report in App.vue. Read ADR-TELEMETRY-ERRORS-0030 rule 7 and inspect the discussion linked from #17001. Done means nested objects are rejected at compile time, all call sites compile or are migrated, App.vue's url has a recorded decision, and the ADR references the type.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
observability, security
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.