Skip to content

fix: mask filesystem paths in telemetry error text - #1117

Open
ralphstodomingo wants to merge 50 commits into
mainfrom
fix/telemetry-mask-file-paths
Open

fix: mask filesystem paths in telemetry error text#1117
ralphstodomingo wants to merge 50 commits into
mainfrom
fix/telemetry-mask-file-paths

Conversation

@ralphstodomingo

@ralphstodomingoralphstodomingo commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Closes#1118

Why this PR exists

While triaging this product's error telemetry, we found a live 32-machine core_failure / file_not_found cluster whose messages carried complete /Users/<name>/… paths — the OS username and the customer's repository layout, verbatim, in App Insights. maskString (the telemetry redactor every error message and masked_args value passes through) deliberately covered API keys, bearer tokens, emails, internal hosts, and quoted strings — but had no rule for filesystem paths. The gap had stayed invisible for a subtle reason: quoted paths were coincidentally destroyed by the quoted-string rule, so only unquoted paths (File not found: /Users/… — the most common way tools print them) leaked.

This PR makes path-shaped content in telemetry text mask to <path> while keeping the surrounding words intact.

Why surgical masking instead of the industry shortcuts

Most mature CLIs never face this problem: the .NET SDK, AWS SAM, Homebrew, and dbt Core send no free text at all (exception names, enumerated events, timings). That is the strongest design, but this codebase sends error messages by design — the downstream error-triage pipeline clusters on them, so the message content is the product value being protected.

The other established shortcut is VS Code's: one crude segment-chain regex that turns anything word/word-shaped into <REDACTED: user-file-path>, plus nuking any property that merely contains the words password/token/key/secret. We replayed that strategy over 800 distinct live error messages from this product's own telemetry:

VS Code-style outcome on our corpusCount
Whole message destroyed by the secret-word rule48 (6%) — the credential/auth error class, among the most actionable
Help-URLs destroyed14
Over-masked (package names in install instructions eaten, fragmented redactions)328
Leaks that the rules in this PR catch4 (all benign carve-outs)

VS Code's telemetry is stack traces, where the words around a path carry no value. Ours is error prose, where those words — package names, support URLs, "a password must be specified" — are the diagnostic payload. So this PR does the harder thing: mask the path, keep the sentence.

How it works

Masking runs in layers, ordered after the credential rules and before the email/internal-host rules (so whole URIs mask before userinfo can fragment):

  1. Preprocessing. ANSI CSI/OSC escape sequences are stripped (colored subprocess stderr otherwise hides paths — and credentials — from every rule), and runs of 4+ bare separators collapse via one cheap rule.
  2. Structural rules — six regexes composed from shared, documented fragments: cloud URIs (s3gsabfsswasbsadldbfshdfsfile:), Windows homes, Windows drive/UNC/drive-relative/rooted forms, POSIX homes, POSIX absolute/dot-relative/shallow forms, and tildes (incl. ~user and PowerShell ~\). Home-rooted paths and cloud keys get an unconditional spaced-tail (usernames and object keys are the high-PII class); everything else takes a trailing word only at end-of-string or punctuation. Delimiters and quotes inside components count as path content only when a later separator or attached dotted filename proves them — otherwise they are boundaries, which is what keeps cd /a/b;ls, closing parens, and quote pairing intact.
  3. Known-prefix literals. The local user's home directory and cwd are known values, so paths under them are additionally replaced by exact literal match (JSON-doubled-backslash and swapped-separator variants, token-boundary guarded) — the approach Salesforce's telemetry and gatsby-telemetry use. This pass runs after the structural rules as a mop-up: structure must see the original string, or a stripped prefix orphans terminal spaced components.

Fast-path gates keep the cost honest: separator-free strings skip the whole path stack, and the two widest Windows rules additionally skip unless a backslash, boundary drive-colon, or non-scheme // is present.

Doctrine, applied consistently: over-masking is the correct failure mode; public https links are deliberately preserved (support-doc URLs in errors are triage signal); past an unproven delimiter, "path content vs prose" is undecidable and stays a documented boundary rather than a heuristic guess.

How it was hardened

The rules went through ~30 rounds of adversarial AI review (Codex, cubic, Kilo, CodeRabbit), with every finding probe-verified against the real corpus before any fix — several were rejected with evidence instead. Every round re-ran all 800 distinct 30-day live telemetry messages through the patched chain: zero residual path content throughout (intentional survivors: public https doc-links). Catches worth naming:

  • a nested-quantifier ReDoS shape (proven exponential) and later a quadratic delimiter-run scan (measured ~400ms) — both eliminated; every proof lookahead is now bounded by the filesystem's own 255-byte component limit, and the repo's upstream DoS-budget chaos suite passes;
  • an engine divergence: under /iu, spec-conformant engines case-fold \p{Lu} (V8 folds, JSC does not), which would have turned the capitalized-tail gate into "eat all prose" on any V8 runtime while tests stayed green on Bun — the i flag is gone, case-insensitivity is hand-expanded per letter, behavior verified identical on both engines;
  • a genuine unmask regression (an eaten closing quote shifted downstream quote pairing and leaked the next quoted value) — caught by a reviewer, fixed, and pinned;
  • WSL-mounted homes (/mnt/c/Users/Jane Doe), JSON-doubled separators, NBSP-bearing usernames, and legacy Documents and Settings roots — all real-world spellings of the same high-PII class.

Performance

Mean over the live corpus: ~220µs/message; ~4 masked fields per telemetry event, so well under 1ms per event, entirely off the interactive path. Adversarial inputs are bounded (a 20k-character separator run costs <1ms). Timing regression tests pin the adversarial shapes.

Verification

  • test/telemetry/mask-file-paths.test.ts: ~120 tests / 250+ assertions, organized by subject (preprocessing, each path family, anchors, in-component delimiters, extensions, the must-not-mask set, performance budgets) with doctrine comments per section.
  • The 800-message live-corpus sweep: 0 residual paths.
  • Full masking-adjacent suites (incl. the v1.4.0 chaos/fuzz files) and tsgo --noEmit pass; cross-engine spot-checks on JSC and V8 agree.

Documented residue (by design, not defects): one prose word may be over-masked after an extensionless home/cloud path; a non-home path's terminal spaced component can leak one structure word mid-sentence (no personal names in that class); an unproven delimiter is a permanent boundary; spaced terminal filenames may span interior words only when the run ends in a dotted extension (4 words on deep paths, 12 on explicit ./-style paths).

🤖 Generated with Claude Code

https://claude.ai/code/session_01LKJeLDMhBaYu16LrjGCf25


Summary by cubic

Masks filesystem, UNC (incl. FQDN), cloud-storage, and tilde paths in telemetry error text to prevent leaking usernames and repo structure. Previously, unquoted non-HTTP paths were sent verbatim; now they are replaced with . Public https links remain, and error-grouping keys may change once as variants collapse.

  • Centralizes path masking in packages/opencode using composed PATH_RULES; covers gs://, s3:///s3a://, abfss://, wasbs://, adl://, dbfs://, hdfs://, and file:/|//|///; Windows drive/UNC (extended-length, forward-slash UNC, current-drive rooted, drive-relative); POSIX absolute/dot-relative/single-file; and tildes (~, ~user, PowerShell ~\). Handles spaces, Unicode/combining marks, NBSP, and backslash-in-slash paths.
  • Ordering: runs after credential masking and before email/internal-host; preserves delimiters and quote pairing; public https URLs are untouched.
  • Bounded bridging: spaced continuation is now a proof (≤2 words, excludes delimiters and colon) to a later separator or dotted filename, which restores classification and prevents swallowing clauses between a path and URLs/dates/MIME types.
  • Performance/safety: 8 KB input cap; fast-path skip when no separators; collapses 4+ separator runs; bounded scans; eliminates drive-colon quadratic with opener bounds and a plausible-filename gate. Email masking gains a left boundary to avoid quadratic cases.
  • Known-prefix literals: exact os.homedir() and dynamic process.cwd() prefixes (incl. JSON-doubled and swapped separators) mop up after structural rules; adjacent markers collapse.
  • Tests: adds packages/opencode/test/telemetry/mask-file-paths.test.ts with Unicode/NBSP cases, anchors/delimiters, and growth-based perf assertions.

Written for commit 4acad5e. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Privacy Enhancements
    • Telemetry error messages now redact filesystem paths and cloud-storage locations across Unix, Windows, network, Unicode, URI-based, and relative path formats.
    • Existing protection for email addresses, credentials, internal hosts, and public URLs remains supported.
  • Bug Fixes
    • Improved masking for paths containing spaces, quotes, brackets, symbols, punctuation, semicolons, and nested components.
    • Added support for intermediate home-directory paths and named-user home paths.
    • Improved consistency and performance for complex or unusually formatted error messages while preserving surrounding text and delimiters.

maskString deliberately covers API keys, bearer tokens, emails, internal
hosts and quoted strings — but had no rule for filesystem paths, so
UNQUOTED paths in error messages reached App Insights raw. Home-directory
paths carry the OS username and project-rooted paths leak client repo
structure (a live 32-machine core_failure/file_not_found cluster carried
full /Users/<name>/... paths). Quoted paths were coincidentally destroyed
by the quote rule, which is why the asymmetry went unnoticed.
Adds three ordered rules (Windows drive/UNC, POSIX absolute with 2+
segments, home-relative ~/) that replace the whole path with <path> —
matching the chain's existing "over-masking is the correct failure mode"
doctrine. Ordered after the URL rule (a public URL's path segment is never
word-anchored, so it cannot match) and before quote masking. Bare
single-segment tokens like "/mcp", MIME types, dates and version specs are
proven untouched by the test matrix.
Side effect, intended: hashError operates on the masked message, so
grouping keys for affected messages change once — and path-variant
messages that previously split now collapse into one group.
@ralphstodomingoralphstodomingo self-assigned this Aug 19, 2026
@coderabbitai

coderabbitaiBot commented Aug 19, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Telemetry.maskString now handles additional filesystem path boundaries and nested POSIX home paths. Tests cover path forms, URI forms, delimiters, Unicode, relative paths, preservation rules, and linear-time behavior.

Changes

Telemetry path masking

Layer / File(s)Summary
Extend path redaction rules
packages/opencode/src/altimate/telemetry/index.ts
Path matching now consumes semicolon delimiters when followed by path separators. POSIX home paths now support intermediate directories before Users or home.
Validate path masking boundaries
packages/opencode/test/telemetry/mask-file-paths.test.ts
Tests cover filesystem paths, cloud-storage and file: URIs, Unicode, spacing, delimiters, relative paths, URL preservation, existing masking behavior, nested home roots, and linear-time adversarial input.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk:🟡 Moderate · up to 88013

The current masking rules can leave semicolon-attached filename or object-key suffixes unmasked, allowing sensitive path structure to reach telemetry. This bounded privacy issue should be fixed and covered by regression tests before merging.

Suggested reviewers:sahrizvi, anandgupta42

Poem

A rabbit checks each hidden trail,
With semicolons in the veil.
Home paths fade to <path> white,
While URLs keep their public light.
Tests guard spaces, roots, and flight.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Linked Issues check✅ PassedThe changes address issue #1118 by masking filesystem paths and cloud-storage URIs while preserving intended non-path masking behavior.
Out of Scope Changes check✅ PassedThe expanded path handling and adversarial tests remain related to the linked telemetry masking objective, with no unrelated code changes identified.
Docstring Coverage✅ PassedDocstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check✅ PassedThe title clearly and concisely describes the primary change: masking filesystem paths in telemetry error text.
Description check✅ PassedThe description thoroughly explains the issue, implementation, scope, performance safeguards, and verification, although some template headings and checklist items are omitted.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/telemetry-mask-file-paths

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

This PR doesn't fully meet our contributing guidelines and PR template.

What needs to be fixed:

  • PR description is missing required template sections. Please use the PR template.

Please edit this PR description to address the above within 2 hours, or it will be automatically closed.

If you believe this was flagged incorrectly, please let a maintainer know.

@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

@codex review

@github-actions

Copy link
Copy Markdown

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

  1. Open an issue describing the bug/feature (if one doesn't exist)
  2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:167a5f2b94

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Review + a live-data sweep hardened the path rules. Ran all 800 distinct
30-day telemetry messages through the patched maskString hunting for
residual path content:
- 1 true leak survived: a gs:// URI carrying a client bucket name + data
layout — cloud-storage URIs (gs, s3, abfss, wasbs, adl, dbfs, hdfs,
file) are not http so the URL rule never sees them, and not
slash-anchored so the path rule missed them. New leading rule masks the
whole URI.
- Review caught unquoted paths with embedded spaces (macOS
"/Users/Jane Doe/client repo/…") escaping the [^\s] character classes.
Paths now continue across a space whenever a later chunk carries another
separator, plus one optional trailing spaced filename — while trailing
prose ("… /app/a.sql was deleted upstream") is proven untouched.
Public https doc-links (provider help URLs) remain deliberately unmasked —
the other 32 sweep hits were all of that class. After both fixes the
800-message sweep reports zero residual path content.
@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:4899189e74

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Second review round, all three confirmed by construction:
- Anchor classes include [ and { so bracketed paths ("failed [/Users/…]")
match; quote masking never applied to those.
- Cloud-URI rule gets the same embedded-whitespace continuation as
filesystem paths, and continuation chunks may span MULTIPLE spaced words
before the next separator ("dir with spaces/part …") — the single-word
unit broke the chain.
- One trailing spaced WORD is consumed at end-of-string or before
punctuation ("Directory not found: /Users/jdoe/client repo"). The
general "path + space + word" case is undecidable against trailing
prose ("client repo" vs "x.sql was"), so mid-sentence prose stays
protected; the accepted residue is a single end-of-string prose word
eaten (over-masking doctrine), and a username can never leak through
this case — it is always slash-terminated and therefore always inside
the masked span.
15 unit tests; 800-message live sweep still reports zero residual leaks;
adjacent suites + typecheck green.
@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:9d56682f10

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Third review round. The P1 caught a real overclaim: "a username can never
leak" was false for SPACED usernames — "/Users/Jane Doe does not exist"
left the surname outside the masked span mid-sentence. Home-rooted paths
(/Users, /home, C:\Users — where the spaced-username stake lives) now
consume one unconditional trailing word, suppressed by an
extension-lookbehind when the path already ended in a dotted file so
"x.sql was deleted" prose stays intact. Honest residue, now documented in
the rule comment: one prose word may be over-masked after an extensionless
home path; a NON-home path's terminal spaced component can still leak one
structure word mid-sentence (no personal names there).
P2: the Windows span consumed closing delimiters — "(C:\Users\x.sql)"
became "(<path>". Aligned its character classes with the POSIX/cloud rules
(Program Files-style spaced dirs still continue via the word chunks, which
keep parens).
19 unit tests; 800-message live sweep still zero residuals; adjacent
suites + typecheck green.
@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:40cb7fea4f

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Fourth review round — three real findings and one improvement they implied:
- RFC 8089 single-slash file URIs (file:/Users/...) matched neither the
scheme rule (required //) nor the POSIX rule (cannot anchor after ':').
The scheme rule now accepts file:/ file:// file:///.
- Windows home roots match case-insensitively (C:\users\Jane Doe) — both
home rules carry the i flag now.
- The extension lookbehind covered 1-4 chars, so .jsonl/.parquet paths ate
a following prose word (and skewed hashError grouping). Replaced with a
single variable-length (1-8) lookbehind — and applied the same lookbehind
to the generic rules' end-of-string trailing-word tails, so prose after
any dotted filename now survives everywhere ("dbt.exe ENOENT" keeps
ENOENT, improving on the previous round's accepted loss).
The fourth reported finding (terminal spaced components mid-sentence) is
stale for its own example — /Users/Jane Doe masks since the previous round
(probe-verified) — and remains only as the documented no-personal-names
residue on ~ and cloud paths.
22 unit tests; 800-message live sweep zero residuals; suites + typecheck
green.
@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:b466d176df

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Fifth review round, both real (probe-confirmed before fixing):
- The continuation/terminal word classes were ASCII \w, so a spaced
unicode component ("/Users/Jane García/…") broke the chain mid-word and
leaked "ía/client/model.sql". All six path rules now use \p{L}\p{N}
classes with the u flag (continuation words, terminal words, and the
POSIX base segments so "/données/…" roots match at all). Unspaced
unicode already worked via the permissive tails.
- ":" joins the anchor classes so "ENOENT:/Users/…" and
"source:s3://bucket/key" mask. URL interiors stay safe structurally:
after "https:" comes "//", and a double slash can never start the POSIX
segment chain — locked with negative tests for public URLs,
protocol-relative refs, and host:port/db strings.
25 unit tests; 800-message live sweep zero residuals; adjacent suites +
typecheck green.
@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:12728e0a1a

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Sixth review round:
- Word classes gain \p{M} and curly apostrophes: NFD-decomposed macOS
names ("Garci" + combining accent) and "O'Connor"-style components mask
whole instead of splitting mid-word (test constructs NFD explicitly via
normalize("NFD") so the decomposition is real, not editor-normalized).
- ";" and "<" join the anchor classes ("ENOENT;/Users/…",
"failed </Users/…/model.sql>"), with ";" and ">" joining the closing
exclusions so delimiters survive ("<<path>>", "…;<path>;done").
- The ">" exclusion exposed a latent ordering hazard: the email rule ran
BEFORE the path rules and fragmented URI userinfo
("abfss://container@account…") into "<email>" mid-URI. Path rules now
run after credentials but before email/internal-host — whole URIs mask
before anything can fragment them, which is strictly better privacy.
28 unit tests; 800-message live sweep zero residuals; suites + typecheck
green.
@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:acef14f0e6

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Seventh review round, one finding: "/Users/Jane/O'Connor/…" stopped at the
apostrophe and leaked the surname + repo layout. The subtlety is that '
is both the quote-rule delimiter and legitimate path content; the
disambiguator is what follows — a word character means path content
(O'Connor), space/punctuation means a closing quote. Implemented as a
guarded alternation '(?=[\p{L}\p{N}_]) in the span, continuation, and
terminal classes, so quoted paths keep their closing quote for the quote
rule ("open '/Users/…/x.sql' failed" still collapses to "open ? failed",
username-free — locked with a test).
30 unit tests; 800-message live sweep zero residuals; suites + typecheck
green.

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
- known-prefix literals run AFTER the structural rules: replacing the
prefix first stripped the opener and orphaned terminal spaced
components ($HOME/client repo leaked 'repo'); structure now sees the
original string and literals mop up what it missed
- literal matchers carry a token-boundary lookbehind so they never fire
mid-URL or mid-token
- drive-relative single-component files (C:secret.sql) mask via a
letter-first dotted-terminal proof; the main gate opens on a boundary
drive-colon since such strings have no separator
@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

@codex review

@chatgpt-codex-connectorchatgpt-codex-connectorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:ced57f47df

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
- the backslash-tilde proof reuses the rooted proof's component classes
(+& at length 3+, spaced continuations): ~\C++ Projects\... masks
- the drive-relative terminal proof accepts any (spaced) run whose
EXTENSION carries a letter — C:123_report.sql, C:.env.local, and
C:customer secret.sql mask while versions and ratios (C:8.5, C:1.2.3)
have all-numeric extensions and never qualify; the fast-path gate
opens on any non-space after a boundary drive-colon

@kilo-code-botkilo-code-botBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incremental review at a32d716 (21 commits since a5f79d8): regex-rule breadth changes, fast-path gates, known-prefix literal layer, and the restructured test suite.

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts Outdated
- windowsHome's structured separators accept doubling ({1,2}), so
JSON-printed UNC/drive homes keep spaced-name protection
- the NBSP tests now construct U+00A0 explicitly (one relied on an
invisible literal byte, the other tested plain spaces)
- layer-ordering comment corrected (literals mop up AFTER structure);
the trailing-word doctrine now states the spaced-filename interior
reach; a duplicated timing probe got a distinct spaced-run shape

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated

@sahrizvisahrizvi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary — request changes

The privacy goal here is real and the write-up is unusually rigorous. The high-PII coverage genuinely works: WSL mounts (/mnt/c/… and \\wsl$\…), Documents and Settings, extended-length UNC, backslash UNC with FQDN hosts, NBSP usernames, NFD-decomposed names, 8.3 short names, and the file: URI forms all mask correctly in my testing. The \p{Lu} / /iu engine-divergence catch is a genuinely subtle find, and hand-expanding the case classes is the right call.

Two things block merge, both left as inline comments:

  • Over-masking (pmChunks) — any later slash in the message makes the path run swallow everything in between. 9 of 12 realistic error messages lost the majority of their content, and distinct errors now collide under hashError(masked). This works directly against the clustering value the PR is written to preserve.
  • Quadratic in PATH_RULES.windows — 2.4 s on a 32 KB drive-colon string, reachable from ordinary Snowflake v:col SQL via register.ts:433, which masks the full statement before truncating.

Plus three correctness items inline: the performance tests can't detect the class of bug they're named for; FQDN forward-slash UNC leaks; process.cwd() goes stale after chdir.

Everything below is non-blocking.

Leaks worth closing

inputoutput
Error: could not read %2FUsers%2Fjsmith%2Fsecret%2Fa.sqlunchanged
GET https://example.com/?file=%2FUsers%2Fjdoe%2Fclient%2Fsecret.sqlunchanged
stat /Volumes/Clients/Jane Doe does not existstat <path> Doe does not exist
stat /mnt/shared/Jane Doe does not existstat <path> Doe does not exist
ENOENT: $HOME/projects/jsmith/a.sqlunchanged
ENOENT: %USERPROFILE%\Documents\jsmith.sqlunchanged
read $XDG_CONFIG_HOME/app/config.jsonunchanged
stat ~jane does not existunchanged
cannot stat Users/jsmith/a.sqlunchanged
open vscode://file/Users/jdoe/x.sql failedunchanged

Notes on a few of these:

  • Percent-encoded separators. Rules inspect only literal / and \. These show up in HTTP-layer errors and are routinely decoded by whoever is reading App Insights. Detecting case-insensitive %2f / %5c would close it.
  • Generic non-home paths carrying names. The terminal rule takes only one trailing word, and the comment claims "no personal names in that class" — external volumes, network mounts, and container mounts routinely carry them. Applying the capitalized/caseless name-tail logic to generic absolute paths would fix it.
  • Env-var roots. Note that adding $% to PM_ANCHOR does not fix $HOME/... — the $ sits before HOME, not before the separator. Explicit root alternatives are needed: \$\{?HOME\}?, \$PWD, %USERPROFILE%, %HOMEDRIVE%%HOMEPATH%.
  • Bare ~user. The tilde rule requires a following separator, so a bare home reference — which the description lists as covered — is missed.

Over-masking beyond the blocking case

invalid pattern /^abc$/ in configinvalid pattern <path> in config. Validation errors that echo the offending pattern lose it.

Tests

Two assertions are environment-dependent. Lines 42 and 51 bake in the assumption that posixHome matched and consumed a trailing word. When home isn't under /Users/ or /home/, only the literal pass fires and the assertion breaks:

os.homedir()line 41line 42line 46line 51
/Users/jdoepasspasspasspass
/home/runnerpasspasspasspass
/rootpassfailpassfail
/github/homepassfailpasspass

/root is the default for containers running as root, /github/home for GitHub Actions container jobs. The suite is green today because the runner happens to use /home/runner. Asserting the invariant instead — not.toContain(home) plus toContain("<path>") — or stubbing os.homedir() would make it portable.

Line 706 pins a repo-structure leak as correct.mask("/Users/jdoe/client\tsecret/models/a.sql")"<path> secret/models/a.sql". It's documented as the undecidable-fragment boundary, but repo structure is the second thing this PR sets out to protect, and the doctrine elsewhere is that over-masking is the correct failure mode. Worth making that call deliberately rather than inheriting it from a regex boundary.

Two assertions can't fail.expect(mask("open '/Users/jdoe/x.sql' failed")).not.toContain("jdoe") passes for almost any output — thin, given the PR history cites a quote-pairing regression. And it("separator-free strings take the fast path unchanged") asserts only that output equals input, which is equally true if the whole stack ran and matched nothing; it cannot fail if the gate is deleted.

Gaps

  1. Path + a second slash in one message — the shape behind the blocking over-masking bug. No test contains both, which is why it got this far.
  2. Growth-rate assertions rather than thresholds.
  3. Drive-colon-without-separator input at any size.
  4. Old-vs-new differential on the live corpus, gating on over-mask count, not only leak count.
  5. FQDN UNC servers; chdir after import; non-home mounts with multi-word names; $VAR / %VAR% roots; percent-encoded separators; homedir() stubbing.
  6. Multi-line messages — stack traces with a path per line are the dominant real shape, and only one test touches it despite the \s+ collapse interacting with it.

Smaller things

  • /Users/j/x sk-abcdefghij…<path>*** (was <path> sk-***). Credential still masked; only the marker identifying the secret class is lost.
  • dbfs:/… and hdfs:/… leave the scheme visible (dbfs:<path>) where file: handles :\/{1,3}. Cosmetic, no leak.
  • The out = out / .replace(...) chains around lines 1453-1495 sit at mixed indentation levels, which makes the gated Windows block harder to read than it should be — in the one place where a misread costs a leak.

Method

Findings were verified by extracting the shipped PATH_RULES and maskString verbatim and running them on Node 25 / V8, with per-rule isolation for the timing attribution. Happy to share the probe scripts if useful.

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts Outdated
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts
…DN homes
Addresses the human review's five blockers:
- spaced continuation is now a PROOF, not a reach: bridge words use a
class excluding delimiters and colon (prose 'failed,' / 'https:' can
never join), at most two words may bridge, and the proving separator
must lead to a letter-bearing chain — so URLs, dates, fractions, MIME
types, and source refs no longer swallow the clause between a path
and a later slash. <internal-host> classification is restored. An
old-vs-new corpus diff (800 live messages) shows one real message
regaining its swallowed diagnostic, zero leaks.
- drive-colon quadratic eliminated three ways: 8 KB entry truncation,
a plausible-filename gate for separator-free input, and {1,256}
bounds on every opener run ('a:' x 8000: 2986ms -> 1ms). The
pre-existing email-rule quadratic gets a left boundary.
- known-prefix literals: homedir cached once, cwd variants rebuilt
whenever process.cwd() changes (the CLI chdirs after import).
- timing tests assert growth ratios at two sizes (median-of-3), not
single-size wall clocks; the drive-colon shape is covered explicitly.
- the home rule accepts FQDN UNC hosts (//server.example.com/...
/Users/... masks); schemed public URLs remain preserved.

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts Outdated

@sahrizvisahrizvi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round-2 review — 4acad5ea

Verified by extracting the shipped PATH_RULES and maskString from both 4acad5ea and 22f0c541 and running them side by side under Node 25 / V8. bun test still can't start (preload not found "@opentui/solid/preload"), so I replayed the suite's 122 assertions through a shim: 121 pass, the one failure being a shim artifact (require in ESM), not a real one.

Verdict: request changes — 4 blocking, 5 smaller.

Ledger on the five blockers

verdict
B1 unbounded bridgingpartially fixed — fixed in five families; windowsHome was never bounded, and the new bound introduced a leak
B2 drive-colon quadraticfixed at the root — but the truncation added alongside it introduces a credential leak
B3 perf testspartially fixed — they catch the two known quadratics, but the 0.05 ms floor means they still can't detect the class
B4 FQDN UNCpartially fixed/Users/ masks, /home/ doesn't
B5 stale cwdfixed

What's genuinely fixed, with the evidence

B2 — the quadratic is gone at the source, not hidden behind the truncation. Applying PATH_RULES.windows directly to "a:".repeat(n) with no truncation at all: 30.2 ms at 16 KB, 60.9 at 32 KB, 121.4 at 64 KB, 244.6 at 128 KB — exactly linear. Every other rule stays ~0.1 ms at all sizes. The full chain across twelve adversarial shapes runs 0.01–0.18 ms at 8 KB with ratios 0.3–3.6. The email left-boundary fix works too: the separator-free shape went from ratio 64 to 0.98. Fixing that pre-existing quadratic wasn't asked for and was the right call.

B3 — the growth tests are non-vacuous. Ran the shipped growth() helper against the old implementation: drive-colon 66.29, separator-free 64.42, both over the bound. They'd have caught the exact regressions they were written for.

B5 — the cwd rebuild works. Inspected pmKnownPrefixes() before and after process.chdir(): the import-time literal is replaced by the new cwd. Cost is 1.56 µs per maskString call on a typical message. You were also right that a property assertion is the correct shape for that test rather than pinning exact output.

B1's headline damage is gone. Dates, fractions, MIME types, scoped npm packages, URLs and second paths no longer bridge, and <internal-host> classification is restored — all confirmed.

Blocking items

Left as inline comments: the truncation-before-redaction credential leak, the four-word directory-component regression, the unbounded windowsHome prefix, the FQDN /home/ gap, and the growth-ratio floor.

Worth saying plainly: the truncation one is my fault. I suggested "truncate before masking at the entry point" last round as suggestion #1, and it was wrong — it puts the cut ahead of the length-gated credential rules. You implemented what I asked for. The fix is to reorder, and the timing data shows the cap isn't needed for performance anyway.

Three of the four blocking items are the same shape: the bounding and disambiguating work landed on some of the six PATH_RULES and not the others. Probably worth one pass over all six together rather than four separate patches. The truncation is independent and is a one-line reorder.

Smaller items

process.cwd() can throw during masking. Already flagged by an automated reviewer and not yet answered — I'll just add the characterisation, since the answer is narrower than "it throws":

chdir(deep); rm -rf(parent); process.cwd() -> THREW ENOENT 5/5 runs
chdir(deep); process.cwd(); rm -rf(parent); process.cwd() -> ok (cached) 3/3 runs

libuv caches the cwd on the first successful call after a chdir and never re-syscalls, so this only reaches maskString if the directory is removed before the first path-bearing mask after the CLI's chdir. Node resolves paths constantly, so it's usually already warm — but the window is real, it runs inside error handling, and if it does throw the cache never warms, so every later call throws too. The suggested try/catch returning pmCwdCache is right and costs nothing.

errorHash collisions persist for the bounded-bridge residue. The new distinctness test passes, but the class isn't closed — three of four realistic pairs still collapse to identical masked text:

load /app/a.sql see doc/page now -> "load <path>"
load /app/b.sql see doc/other now -> "load <path>"
Permission denied: /var/lib/a.parquet (uid/gid mismatch) -> "Permission denied: <path>)"
Permission denied: /var/lib/b.parquet (perm/mode mismatch) -> "Permission denied: <path>)"
dbt deps failed in /Users/j/p: package hub/dbt-utils not found -> "dbt deps failed in <path> found"
dbt deps failed in /Users/j/q: package hub/dbt-date not found -> "dbt deps failed in <path> found"

Two corpus lines also don't preserve their clauses, against the reply's "all six of your shapes plus the realistic corpus lines now preserve their clauses": Permission denied: /var/lib/warehouse/data.parquet (uid/gid mismatch) is still Permission denied: <path>), and dbt run failed in /Users/j/proj: 3/5 models errored is dbt run failed in <path>/5 models errored. Bounded rather than unbounded is the substantive win and this isn't blocking — but the pinning test covers only the pair that happens to survive. The pattern is consistent: a clause is swallowed when a slash appears within two words of the path, preserved at three or more. Worth stating in the doctrine comment.

require() in an ESM test file. The new chdir test uses const os = require("os") in a file that already has import os from "os" at the top. Bun tolerates it; Node and Vitest don't.

Still open from the last round

Re-ran all of these against 4acad5ea — every one behaves identically to 22f0c541, so recording rather than re-arguing. The tab-boundary case is excluded: that one you answered with corpus evidence and I'm not reopening it.

  • Environment-dependent test assertions. Verified by stubbing homedir(). With /root: stat ${home} does not existstat <path> does not exist (expects stat <path> not exist) and stat ${home}/client repo does not existstat <path> repo does not exist (expects stat <path> does not exist) — two failures. With /github/home: one failure. With /home/runner and /Users/x: all pass. This breaks under Docker-as-root and some GH Actions container images.
  • %2FUsers%2Fjdoe%2F… percent-encoded paths unmasked; $HOME/…, ${HOME}/… and %USERPROFILE%\… unmasked; bare ~jdoe unmasked; bare relative Users/jdoe/… and home/jdoe/… unmasked; pattern /^a.*b$/ masked as a path; /mnt/projects/Jane Doe failed<path> Doe failed.

The forward-slash regex-literal case is worth a second look given how much work went into keeping backslash regex prose (\d+\.\d+, \bword\b, \n\r\t) preserved — the forward-slash form has no equivalent guard.

Missing tests

  • A credential and a quoted string straddling the 8,192 cut; a long-SQL pair differing only past 8,192, for the hash.
  • Bridging controls per rule family, especially the UNC-home prefix — the current ones are POSIX-only.
  • FQDN UNC /home/ and /homes/, plus public URLs containing /home/.
  • Directory components of 4, 6 and 8 words per family; drive-relative separator-free filenames with 3+ spaces; c:.hidden.
  • process.cwd() throwing.
  • errorHash distinctness for the residue class, not just the surviving pair.
  • The seven perf shapes that lost their guard.
  • A homedir()-stubbed run so the preprocessing block stops depending on the host's home directory.

Positives

The quadratic fix is done properly — bounded at the source, and I could verify that independently of the truncation. The growth tests genuinely fail on the pre-fix code. Fixing the pre-existing email-rule quadratic while in the neighbourhood was unasked-for and correct. The cwd rebuild is minimal. And every claim in the reply threads was specific enough to test directly, which is what made this round fast — where a claim overstated the result, it was still precise about what to check.

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
…res, safe truncation
Second human-review round:
- 4+-word directory components mask again in every family: a 3-6 word
bridge is accepted only when the continuation itself looks like a
path (another separator or a dotted extension) — prose bridging to a
lone slashed token still stops. First-segment proof (SEG_B) rejects
all-digit dates/fractions without slash-crossing. Home roots may
anchor mid-token when the root immediately follows (a glued or
URL-embedded /Users/ carries a username; over-masking is correct).
- windowsHome's segment loop is bounded and proven like every other
family (no more 40-word or date bridges to a later /Users/); home
roots gain home/homes so FQDN UNC /home/ shares mask.
- truncation moves after the length-gated credential rules and the
quote collapse, and backs off to whitespace — a Bearer/sk-/quoted
value straddling the 8 KB cut can no longer ship a fragment.
- growth tests measure the small size over 100 iterations (no floor):
both sides are real milliseconds, the ratio is a real growth rate.
The truncation test is a ratio too.
- process.cwd() throwing (deleted cwd) keeps the cached prefixes.
- the two heaviest rules skip unless a home-root literal is present.
Corpus (800 live msgs): 0 leaks, 6 messages preserve more, 0 mask more.
@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

@sahrizvi ready for re-review at d67f2d51d. Correction first: an earlier comment here claimed all blockers were addressed while your 08-21 threads were still open — that was premature, and I've removed it. Current state: all ten of your threads are fixed, replied inline, and resolved — the 08-20 five (bounded bridges, drive-colon quadratic, fresh cwd, growth-ratio tests, FQDN UNC homes) and the 08-21 five (two-tier bridges so 4+-word components mask again while prose still stops; windowsHome bounded and proven like the other families; /home+/homes shares; truncation moved after the credential/quote rules with a whitespace back-off so nothing straddles the cut; growth tests with a real small-size measurement, no floor). Verification on the 800-message live corpus vs the previous head: 0 leaks, 6 messages preserve more content, 0 mask more. Honest cost note: mean per-message rose to ~367µs from ~226µs with the deep-bridge proof; the expensive tier is confined to tail chunks and the two heaviest rules now skip unless a home-root literal is present. The 76 stale codex/kilo/cubic threads from 08-19 are resolved (each carried a fix reply; cubic's summary confirmed them addressed). The red Kilo check remains that bot's pipeline crashing on this PR's size — no findings since round 4.

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts
…e back-off
The cut now runs before every pass so all work is bounded (the flat-
cost claim holds for the whole chain, not just the path stack), and it
cannot fail a rule open: it backs off to the last whitespace of any
kind (tabs/newlines included) so no token straddles the boundary, then
to before any unbalanced quote so a quoted value never survives half-
open. Straddle pins are property-based (nothing of the token ships).
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts Outdated

@sahrizvisahrizvi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consensus review of 77d613ef60 — 3 CRITICAL and 6 MAJOR findings posted inline below. Minor and nit findings, positives, and the missing-test list follow in a separate combined comment.

Every finding was reproduced by calling Telemetry.maskString directly at this head; the existing 128 tests all pass. Prior resolved threads were checked first and are not re-raised.

Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/src/altimate/telemetry/index.ts Outdated
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts
Comment threadpackages/opencode/test/telemetry/mask-file-paths.test.ts
@sahrizvi

Copy link
Copy Markdown
Contributor

Consensus review of 77d613ef60 — minor findings, positives, and open questions

The 3 CRITICAL and 6 MAJOR findings are posted as inline comments. This comment carries everything that is either lower severity or not anchorable to a single line.

Every finding was reproduced by calling Telemetry.maskString directly at this head. The existing 128 tests pass. Prior resolved threads were read first and are not re-raised — where a finding confirms one of the still-open threads, it says so.

Severity criterion used: CRITICAL = customer PII or secret content reaches telemetry in the clear under plausible product traffic. MAJOR = a leak requiring narrower conditions, or destruction of the triage value the masking exists to preserve. MINOR = narrow leak or over-mask with limited blast radius.


MINOR

1. The drive-colon fast-path gate is narrower than the rule it guards (index.ts:1525) — confirms the open thread. The gate allows {0,2} spaced words; the windows rule's dotted opener allows {0,6}. So mask("failed C:my big client folder.sql now") is unchanged, while the 3-word C:my big folder.sql masks. Widen the gate to {0,6}, or generate the gate and the structural opener from one definition so their accepted languages cannot drift again.

2. The truncation back-off tests never run the branch they name (test:73-74) — confirms the open thread. "x".repeat(8100) plus the path is 8138 characters, below the 8192 cut, so not.toContain("jdoe") holds for the wrong reason and would hold with the back-off deleted. Raise the padding past 8192.

3. Vacuous perf test — "backslash runs grow linearly" (test:903). The generator is '"' + "\\".repeat(n) + '"', and the quoted-string rule runs before the path stack and collapses the whole thing: mask('"' + "\\".repeat(1000) + '"') === "?". The test measures the quote regex, not backslash-run cost in the path rules. Unquoted, the same input masks to <path>. Dropping the quotes makes it real.

4. Vacuous combining-marks test (test:699). "read ./customer.é failed" uses precomposed U+00E9 (LATIN SMALL LETTER E WITH ACUTE), which \p{L} already matches — remove \p{M} from PM_EXT and the test still passes. Use e\u0301 to actually exercise the mark.

5. The cwd-freshness test does not depend on the cwd layer (test:776). The temp directory is a deep absolute path that the generic POSIX rule masks on its own: with no chdir at all, mask("stat " + real + " failed") is already "stat <path>". Both assertions hold with the cwd literal refresh deleted. Point process.cwd() at a shallow, extensionless root that the structural rules cannot mask — which is the case the comment says this layer exists for.

6. Typographic quotes are not anchors (index.ts:75, PM_ANCHOR). mask("Cannot open “/opt/customer/repo/model.sql”") is unchanged — the full path leaks. Ranked MINOR only because smart quotes are rare in machine-generated error text; if any appear in the live corpus this belongs at CRITICAL. Adding the common Unicode quotation marks to PM_ANCHOR and to the quote preprocessing would close it.

7. Forward-slash UNC with an FQDN server leaks (index.ts:169). mask("ENOENT: //files.corp.example/customer-share/private/model.sql") is unchanged. Dotted servers are excluded deliberately to protect protocol-relative URLs, which is a defensible trade-off — hence MINOR rather than higher — but FQDN UNC servers are common in corporate environments. Gating on filesystem context (ENOENT, open, stat, file:) would recover them without touching URL handling.

8. The tilde rule over-masks approximations.

mask("approx ~5/10 done")// -> "approx <path>"mask("takes ~2/3 of the budget")// -> "takes <path> the budget"mask("about ~10/15 rows")// -> "about <path>"mask("requirement dbt-core~=1.11.0 not satisfied")// unchanged (correct)

Requiring a path-ish continuation after ~, or excluding an all-digit component immediately following it, would fix this without affecting real ~/ and ~user/ paths.

9. URL-encoded paths leak.mask("open failed %2FUsers%2Fjdoe%2Fclient%2Fmodel.sql") is unchanged — the OS username in the clear. Reachable wherever a file:// URI or a URL-parse error is echoed into error text. A %2F-separated variant, or a decode pass before masking, would close it.

NIT

1. Regex literals in error text mask as paths.mask("invalid pattern /^ab+c$/ at index 3") -> "invalid pattern <path> at index 3". Over-masking is the stated correct failure mode, but this removes the pattern that identifies the error, and regex-syntax errors are a plausible telemetry class.

2. Env-var-rooted paths are unmasked and undocumented.%USERPROFILE%\client\model.sql and $HOME/clientrepo/model.sql pass through intact. No username leaks — the variable name stands in for it — but customer repo layout does. Either handle them or name them in the doctrine comment's residue list, which currently does not mention them. Relatedly, pmSpFile caps at 4 words while pmSpFileX allows 12, and the doctrine comment does not explain the difference.


Missing tests

  • A genuinely unbalanced quote opened early in a >8 KB string; a window containing no whitespace; a spaced component straddling the cut (the three CRITICAL truncation cases).
  • Two adjacent paths in one message — no coverage of this shape at all.
  • Any SQL input, despite maskString running on full query bodies at sql-execute.ts:126 and connections/register.ts:433.
  • HTTP request targets, to pin whether multi-segment routes are meant to mask.
  • Terminal filenames past 4 words and directory components past 6.
  • /homes/ POSIX roots; mixed-case glued /USERS/ and /HOME/.
  • An absolute per-call cost ceiling at the 8 KB cap, alongside the existing growth ratios.
  • URL-encoded and smart-quoted path forms.

What this change gets right

Worth stating plainly, because the findings above are a short list against a large and genuinely well-built change:

  • ANSI/OSC stripping runs before the credential and path rules. An escape sequence mid-token would otherwise hide it from every rule downstream — this closes a real gap that is easy to miss.
  • The known-prefix literal pass runs after the structural rules, so structure sees the original string and terminal spaced components are not orphaned, with a freshly recomputed cwd and a (?<![\w.-]) token-boundary guard. That ordering is right, and the guard avoids the false positives the literal approach usually brings.
  • Coverage is broad and holds up under adversarial probing well beyond the suite. WSL /mnt/c/Users/…, /cygdrive/c/…, \\?\C:\…, //?/UNC/…, JSON-doubled \/Users\/…, all-caps /USERS/JDOE/… at a proper anchor, paths glued after error/, */, +/, @/, #/, ->/, stack frames with :line:col, and bracket/brace/colon anchors — all mask correctly.
  • The restraint against over-masking is real where it matters most. GitHub and provider API URLs, application/json, react-dom/client, dates, fractions, and/or, scoped npm specs and dbt raw/orders refs all survive intact. The corpus-replay argument in the description is borne out; the SQL and HTTP-route findings are the exceptions to an otherwise well-held line.
  • Composing six rules from shared fragments after the lockstep-edit drift described in the PR history is the right call, and the doctrine comments explain intent rather than restating the regex.
  • Growth-rate perf testing is better methodology than single-size wall clocks. The MAJOR finding on it asks for one absolute ceiling to complement the ratios, not to replace them.
  • The truncation-before-masking design is sound in principle and resisted most probing; the CRITICAL finding is about the back-off's floors, not about the approach.

One open question for the author

HTTPS cloud-object URLs (https://storage.googleapis.com/<bucket>/…, *.s3.amazonaws.com/…, *.blob.core.windows.net) are still unmasked at this head — reproduced. This is not being raised as a new defect: it was acknowledged on the index.ts:1418 thread and deferred to a follow-up PR, with a request for the human reviewer to confirm that split. Flagging it here so the decision is visible rather than buried in a resolved thread — deferring looks reasonable given how much this PR already carries, provided the follow-up issue exists before merge.


Also considered and not included

  • "~ /file.txt masks as ~ <path>, the tilde should be inside the marker" — not a defect. A space-separated ~ is not part of the path, and the current output is correct. The real tilde issue is Minor 8 above.
  • "The escaped-quote thread is not a leak, because the quote rules already handle escapes" — that reasoning used a balanced escaped quote. With the closing quote past the 8 KB cut the value does leak, which is CRITICAL (a) inline.

…, keep adjacent paths whole
Review round 3:
- truncation: no floors — the head is cut at whitespace of any kind (a
whitespace-free window emits nothing), half-open quotes are closed with an
escape-aware scan mirroring the quote rules, and the head is masked both
alone and with 2 KB of lookahead; only the prefix both readings agree on is
emitted, so a spaced component whose proving separator falls past the cut
can no longer strand its words
- the mid-token home anchor is built from the same case fragments as the rule
body (`/USERS/`, `/HOME/`, `/homes/` glued to a word masked nothing)
- a trailing word never eats the drive letter of a following path: `:` ends
it only before whitespace/end (`copy C:\a C:\b` -> `<path> <path>`)
- drive-relative runs exclude `:` and the separator-free form needs the
canonical uppercase drive letter with no `:` after the extension — Snowflake
VARIANT SQL (`v:col`, `v:geo.city::string`) survives masked_sql, and the
colon-dense worst case at the cap drops from ~170 ms to <5 ms
- posix home root accepts `/homes/` like the other two layers
- HTTP request targets after a method/route keyword are not filesystem paths
- spaced filenames and directory components are bounded by the 255-byte
component limit, not a word count (past a cap the remainder shipped clear)
- tests: drive-colon growth shape now passes the gate, absolute per-call
budgets at the cap, SQL-body suite, adjacent-path and cut-band regressions
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GayJFsfg2q2FoAG2XSVsZF
@ralphstodomingo

Copy link
Copy Markdown
ContributorAuthor

Round 3 addressed in e573a20 — 9/9 threads replied and resolved above.

  • Truncation: no floors; whitespace-of-any-kind cut, escape-aware quote closing, and a head-alone vs head-with-lookahead agreement cut so a component whose separator falls past the cap is dropped, never stranded (swept over every offset in the band).
  • Home anchor built from the rule body's case fragments; /homes/ in the posix home rule.
  • Trailing words never eat a following path's drive letter; drive-relative runs stop at : and the separator-free form needs an uppercase drive letter — Snowflake VARIANT SQL survives masked_sql.
  • HTTP targets after a method/route keyword are not paths; spaced runs bounded by component length, not word count.
  • Tests: drive-colon growth shape passes the gate; absolute budgets at the cap (colon-dense 0.35/4.7 ms, was 171); SQL-bodies, adjacent-path and cut-band suites. 141 tests in the file, 363 across test/telemetry.
  • Corpus (800 msgs): 0 leaks; 31 mask more (29 = one prose family behind an extensionless path — the length bound's designed over-mask); 2 mask less (CSS a:link, Cannot POST /sse — intended); mean 428 µs (was 377).

Two things stated rather than buried: the over-masked prose family above, and a pre-existing ~55 µs-per-matched-path constant (pinned at 150 ms for 900 paths/8 KB).

…the cut is exercised
The two inputs were 8138 chars — under the 8 KB cap — so the structural home
rule masked them and the truncation branch never ran. At 8223 chars the only
whitespace is the tab/newline at 8185: the cut must land there and drop the
path whole.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GayJFsfg2q2FoAG2XSVsZF
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Telemetry: unquoted filesystem paths and cloud-storage URIs reach App Insights unmasked

2 participants

@ralphstodomingo@sahrizvi