Skip to content

fix(cymbal): resolve sourcemap references as urls, not paths - #99987

Merged
trunk-io[bot] merged 1 commit into
masterfrom
hp/fix-cymbal-sourcemap-url-resolution
Sep 14, 2026
Merged

trunk-io[bot] merged 1 commit into
masterfrom
hp/fix-cymbal-sourcemap-url-resolution

Conversation

@hpouillot

@hpouillot hpouillot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

JavaScript stack frames stay minified when sourceMappingURL references contain query strings or parent-relative paths.

Path-only resolution percent-encodes query delimiters, preserves stale base queries, and resolves ../ from the wrong directory.

This PR replaces #90230 because repository policy requires signed commits.

Changes

  • Frames now resolve source maps with query strings and parent-relative references.
  • Url::join handles absolute, protocol-relative, root-relative, and directory-relative URL references.
  • Existing host validation still checks every resolved source map URL before fetching it.

How did you test this code?

  • Ran the focused sourcemap URL-resolution tests and the cache-busted end-to-end fetch test locally.
  • Ran cargo fmt -p cymbal -- --check.
  • Not completed locally: database-backed Cymbal tests and Clippy require a local Postgres instance. CI remains authoritative for these checks.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Automatic notifications

  • Publish to changelog?

Docs update

None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Human input selected #90230 as the source and requested a signed replacement PR.

The agent reapplied the existing public patch onto current master, ran local checks, pushed the branch, and opened this PR.

Skills invoked: /error-tracking, /writing-tests, /writing-code-comments, /pr, /writing-pr-descriptions, /reviewing-with-coderabbit, and /running-ci-preflight.

CodeRabbit was installed but signed out, so the PR opened without a local CodeRabbit pass.

@hpouillot hpouillot self-assigned this Sep 14, 2026
@trunk-io

trunk-io Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

😎 Merged successfully - details.

@hpouillot hpouillot added the stamphog Request AI approval (no full review) label Sep 14, 2026
@github-actions

github-actions Bot commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane (rust:crate:cymbal)

This PR is assigned to the non-backend lane (rust:crate:cymbal). It does not run backend Python tests and may merge in parallel with PRs in other lanes.

🚨 Comment density — 14% of added code lines are comments (27 of 189)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
rust/cymbal/src/core/symbolication/symbol_store/sourcemap.rs 27 189

This check does not block merging. It updates on every push and clears when the share drops.

@hpouillot
hpouillot marked this pull request as ready for review September 14, 2026 09:38
@greptile-apps

greptile-apps Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Reviews (1): Last reviewed commit: "fix(cymbal): resolve sourcemap reference..." | Re-trigger Greptile

@stamphog stamphog Bot 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.

Not approved — escalated to a human reviewer.

Re-add the stamphog label to request another review once you have addressed this.

The diff itself is a well-tested, contained fix by an owning-team author, but the PR description discloses an unusual provenance story — an agent recreated another PR's patch, attributed the commit to a different named person than the actual submitter, and had it signed with "the maintainer's SSH key" to get a verified badge. That kind of commit authorship/signing-key custody claim needs a human to verify before merge; it's not something a diff read can confirm or rule out.

  • Author wrote 0% of the modified lines and has 7 merged PRs in these paths (familiarity MODERATE).
  • PR description states the commit author was preserved as a third party ("Chad Wells") distinct from the actual PR submitter (hpouillot), and that it was signed with "the maintainer's SSH key" to obtain a verified-signature badge — this needs human confirmation that key custody and authorship attribution are legitimate before merging.
  • The code itself touches sourcemap URL resolution (a fetch path for external URLs); host validation via ensure_fetchable_host is preserved and applied to the final resolved URL, and the change is well covered by new tests, so this is not itself a blocker.
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 258L, 1F substantive — within ceiling
tier ✓ T1-agent / T1c-medium (258L, 1F, single-area, fix)
stamphog 2.0.0b4 .stamphog/policy.yml @ f0fdfce · reviewed head f0fdfce

@stamphog stamphog Bot removed the stamphog Request AI approval (no full review) label Sep 14, 2026
@hpouillot hpouillot added the stamphog Request AI approval (no full review) label Sep 14, 2026

@stamphog stamphog Bot 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.

Approved.

This touches the sourcemap-fetching path used during error-tracking symbolication, but the resolved URL still passes through the unchanged host-validation check before any fetch, so the fix doesn't weaken the existing SSRF protection; author is on the owning team-error-tracking, satisfying the independent-assurance bar for this risky-adjacent surface, and the change is well covered by new tests matching the described bug scenarios.

  • Author wrote 0% of the modified lines and has 7 merged PRs in these paths (familiarity MODERATE).
  • 👍 on the PR from greptile-apps[bot].
Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✓ no deny categories matched
size ✓ 258L, 1F substantive — within ceiling
tier ✓ T1-agent / T1c-medium (258L, 1F, single-area, fix)
stamphog 2.0.0b4 .stamphog/policy.yml @ f0fdfce · reviewed head f0fdfce

@trunk-io

trunk-io Bot commented Sep 14, 2026

Copy link
Copy Markdown

Static Badge   Static Badge   Static Badge

View Full Report ↗︎ ⋅ Docs

@trunk-io
trunk-io Bot merged commit 504f2b5 into master Sep 14, 2026
393 of 395 checks passed
@trunk-io
trunk-io Bot deleted the hp/fix-cymbal-sourcemap-url-resolution branch September 14, 2026 10:08
@deployment-status-posthog

deployment-status-posthog Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Deploy status

Environment Status Deployed At Workflow
dev ✅ Deployed 2026-09-14 10:41 UTC Run
prod-us ✅ Deployed 2026-09-14 11:05 UTC Run
prod-eu ✅ Deployed 2026-09-14 11:07 UTC Run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stamphog Request AI approval (no full review)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants