Repository navigation
feat(error-tracking): add command to mask stored frame code variables - #112617
ablaszkiewicz wants to merge 7 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MxaTpjkJ3QxjaCSrYxXFHW
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
This review was skipped because it would exceed your organization's monthly flex usage limit. Raise the limit in billing settings or wait until the next billing period resets limits.
🤖 CI report
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/error_tracking/backend/management/commands/drop_error_tracking_stack_frame_code_variables.py:130 |
products/error_tracking/backend/management/commands/mask_error_tracking_stack_frame_code_variables.py:141 |
11 | 84 |
✅ Duplication (TypeScript) — clean
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
🚨 Comment density — 7% of added code lines are comments (36 of 528)
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 |
|---|---|---|
products/error_tracking/backend/logic/code_variables_masking.py |
27 | 231 |
products/error_tracking/backend/management/commands/mask_error_tracking_stack_frame_code_variables.py |
7 | 150 |
products/error_tracking/backend/test/test_code_variables_masking.py |
2 | 97 |
This check does not block merging. It updates on every push and clears when the share drops.
⚠️ Backend coverage — 94.0% of changed backend lines covered — 16 uncovered
🧪 Backend test coverage
Patch coverage — changed backend lines (products + core): ███████████████████░ 94.0% (263 / 279)
| File | Patch | Uncovered changed lines |
|---|---|---|
products/error_tracking/backend/management/commands/mask_error_tracking_stack_frame_code_variables.py |
86.7% | 47–48, 50, 58, 64, 69, 75, 93, 96, 129 |
products/error_tracking/backend/logic/code_variables_masking.py |
96.0% | 195, 229, 246–247, 255, 262 |
🤖 Agents: add a test only if an uncovered line exposes a realistic regression that existing tests miss. Otherwise explain why no new test is needed under "How did you test this code?". Gap list: the patch-coverage artifact on this run (gh run download 437873362928856 -n patch-coverage), or the coverage-data block at the end of this comment.
Per-product line coverage (touched products)
| Product | Coverage | Lines |
|---|---|---|
error_tracking |
██████████████████░░ 91.6% |
16,551 / 18,061 |
Report-only. Patch coverage = changed backend lines covered vs origin/master. Sorted lowest first.
Known gaps: lines covered only by Temporal tests show as uncovered; core line numbers may drift if master changed the same file.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to A live run can mask benign text, and one deeply nested value can stop remediation before later frames are processed. Address both before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The command reduces exposure of historical secrets, defaults to preview mode and confines writes to selected teams. Conditional updates protect newer data. Completion depends on updated processing workers and a verification pass; masking-rule parity and production readiness were not fully verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings
🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/error_tracking/backend/management/commands/mask_error_tracking_stack_frame_code_variables.py-113-125 (1)
113-125: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit the raw-ID cursor to the requested team.
If
--start-at-team-idhas no stored frames,_next_team_idcan select a later team. The command then applies the original cursor to that team, excluding its lower raw IDs; a live run can leave those frames unmasked. Keep the cursor only when the selected team equalsstart_at_team_id.Suggested fix
- raw_id_cursor = start_after_raw_id + raw_id_cursor = start_after_raw_id if team_id == start_at_team_id else None
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
d8b8edfd-d97b-4b4e-ab40-d93a208b992a
📒 Files selected for processing (4)
products/error_tracking/backend/logic/code_variables_masking.pyproducts/error_tracking/backend/management/commands/mask_error_tracking_stack_frame_code_variables.pyproducts/error_tracking/backend/management/commands/test/test_mask_error_tracking_stack_frame_code_variables.pyproducts/error_tracking/backend/test/test_code_variables_masking.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
… prose Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MxaTpjkJ3QxjaCSrYxXFHW
HostHog preview —
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Do not exempt TitleCase bearer tokens as prose. · code_variables_masking.py:149-182
products/error_tracking/backend/logic/code_variables_masking.py:149-182
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not exempt TitleCase bearer tokens as prose.
A 15-character value such as
Bearer Abcdefghijklmnomeets the prose exception and remains unchanged. The live masking command then skips the stored row. The 16-character example is masked; it exceeds the exception’s 15-character limit.Suggested fix
and credential[:1].isascii() and credential[:1].isalpha() and all("a" <= c <= "z" for c in credential[1:]) + and (match.group(1).lower() != "bearer" or credential.islower())
🧹 Nitpick comments (1)
products/error_tracking/backend/management/commands/test/test_mask_error_tracking_stack_frame_code_variables.py (1)
20-61: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a stale-value regression test.
_mask_teamreadscode_variablesbefore_write_maskedapplies the stale-value filter. The current test does not change the stored value between those steps, so it would still pass if the filter were removed. Add a test that changescode_variablesafter the read and asserts that the command preserves the concurrent value. Without the filter, the update can overwrite it with a mask derived from the stale value.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
be607199-a4f4-4484-bf87-416f4e5889c1
📒 Files selected for processing (2)
products/error_tracking/backend/logic/code_variables_masking.pyproducts/error_tracking/backend/test/test_code_variables_masking.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.
…tial Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MxaTpjkJ3QxjaCSrYxXFHW
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/error_tracking/backend/logic/code_variables_masking.py-92-92 (1)
92-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecognize punctuation before later prose.
For
Bearer Sunflower, transportation services, the matcher stops atSunflower. Because_MORE_TEXTrequires whitespace immediately after the match, the comma makes the prose check fail and the masker can replace the benign word withREDACTED. Treat punctuation followed by a word as a prose boundary, and add this case to the benign-value tests.🐛 Suggested fix
-_MORE_TEXT = re.compile(r"\s+[A-Za-z]") +_MORE_TEXT = re.compile(r"\W+\b[A-Za-z]")("bearer transportation services",), + ("Bearer Sunflower, transportation services",), ("Basic Configuration loaded",),Source: Learnings
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
e3234706-0ead-4373-a6e7-0e83e0671a28
📒 Files selected for processing (2)
products/error_tracking/backend/logic/code_variables_masking.pyproducts/error_tracking/backend/test/test_code_variables_masking.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MxaTpjkJ3QxjaCSrYxXFHW
…list Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MxaTpjkJ3QxjaCSrYxXFHW
hpouillot
left a comment
hpouillot
left a comment
There was a problem hiding this comment.
I found two issues that can prevent this cleanup from completing. Please address them before merging.
--batch-size bounds distinct raw ids, not rows or bytes, so one page of large frames could exhaust memory. The command now lists the frame ids of a page and loads code variables 100 frames at a time. .iterator() would not bound memory behind pgbouncer, where server-side cursors are off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MxaTpjkJ3QxjaCSrYxXFHW
json.loads accepts an unpaired surrogate escape such as \ud800, which serde_json rejects. After masking a nested field, the re-serialized text held a lone surrogate, and PostgreSQL rejected the jsonb update, which stopped the team run. Such text now takes the ordinary string path, as in cymbal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MxaTpjkJ3QxjaCSrYxXFHW
|
Risk: Medium · 1 medium This PR adds an operator-run management command that masks secrets in the stored Sentinel reviewed |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
212a3569-5cb0-47de-b859-eff9dc4eb66e
📒 Files selected for processing (4)
products/error_tracking/backend/logic/code_variables_masking.pyproducts/error_tracking/backend/management/commands/mask_error_tracking_stack_frame_code_variables.pyproducts/error_tracking/backend/management/commands/test/test_mask_error_tracking_stack_frame_code_variables.pyproducts/error_tracking/backend/test/test_code_variables_masking.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| if not value.lstrip().startswith(("{", "[")): | ||
| return None | ||
| try: | ||
| parsed = json.loads(value, parse_constant=_reject_json_constant) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -u
file=products/error_tracking/backend/logic/code_variables_masking.py
printf '%s\n' '--- relevant source ---'
nl -ba "$file" | sed -n '1,285p'
printf '%s\n' '--- PR diff for target ---'
git diff --unified=8 d27b586f38d6d2e49e7582857d2a22dd728155a3 d55af8bafbf7562d8b947e635b7689c646ffa475 -- "$file"
printf '%s\n' '--- available Python version ---'
python3 --version
printf '%s\n' '--- bounded nested-JSON parser probe ---'
python3 - <<'PY'
import json
import sys
print("version:", sys.version)
for depth in (900, 990, 1000, 1020, 1024):
value = "[" * depth + "0" + "]" * depth
try:
json.loads(value, parse_constant=lambda item: item)
except Exception as exc:
print(f"depth={depth} length={len(value)} exception={type(exc).__name__}: {exc}")
else:
print(f"depth={depth} length={len(value)} parsed")
PYRepository: PostHog/posthog
Length of output: 25429
🌐 Web query:
Python 3.14.7 json.loads RecursionError nested JSON maximum recursion depth
💡 Result:
In **Python 3.14.7, `json.loads()` has no documented fixed maximum nesting depth**. The documentation says nesting limits are left to Python’s interpreter and the limits of its data types, so sufficiently deep valid JSON can raise `RecursionError`. ([docs.python.org](https://docs.python.org/3/library/json.html?highlight=json.loads&utm_source=openai))
```python
import json
try:
value = json.loads(text)
except RecursionError:
print("JSON nesting is too deep for this interpreter")
```
There isn’t a reliable universal “maximum number of levels”: the threshold can depend on the interpreter build and environment. A CPython issue confirms `json.loads()` can raise this error, but its example was tested on **Python 3.13**, not specifically 3.14.7. (GitHub issue 135835 in python/cpython (link omitted to avoid creating a cross-reference))
If you control the input, the safest fix is to reject documents that exceed an application-defined nesting limit. Raising Python’s recursion limit may not be safe for very deep input.
Citations:
- 1: https://docs.python.org/3/library/json.html?highlight=json.loads&utm_source=openai
- 2: GitHub issue 135835 in python/cpython (link omitted to avoid creating a cross-reference)
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- direct references ---'
rg -n -F -- 'mask_code_variables' . || test "$?" -eq 1
printf '%s\n' '--- masking module references ---'
rg -n -F -- 'code_variables_masking' . || test "$?" -eq 1
printf '%s\n' '--- nearby tests and package files ---'
rg --files products/error_tracking | rg 'code_variables|masking|test' | head -80Repository: PostHog/posthog
Length of output: 9147
🏁 Script executed:
#!/bin/bash
set -u
file=products/error_tracking/backend/management/commands/mask_error_tracking_stack_frame_code_variables.py
test=products/error_tracking/backend/management/commands/test/test_mask_error_tracking_stack_frame_code_variables.py
printf '%s\n' '--- command ---'
nl -ba "$file" | sed -n '1,210p'
printf '%s\n' '--- command tests ---'
nl -ba "$test" | sed -n '1,220p'Repository: PostHog/posthog
Length of output: 12565
Catch RecursionError when parsing nested JSON strings.
A string up to 2,048 characters can reach json.loads. Sufficiently nested JSON can raise RecursionError, which the current handler does not catch. The exception then propagates from the unguarded masking call and can stop the command before later frames are processed. Treat this case as ordinary text.
🐛 Suggested fix
- except ValueError:
+ except (ValueError, RecursionError):
return None
![parameterai[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
| # serde_json rejects it, and PostgreSQL rejects the character in jsonb. The encode raises | ||
| # UnicodeEncodeError, a ValueError, so such text stays an ordinary string, as in cymbal. | ||
| json.dumps(parsed, ensure_ascii=False).encode("utf-8") | ||
| except ValueError: |
There was a problem hiding this comment.
A single crafted stack-frame value aborts the operator's masking command, leaving the rest of that team's frames unmasked until someone notices and manually skips the frame. _parse_json_container only catches ValueError, but json.loads raises RecursionError (a RuntimeError) for nesting deeper than the interpreter recursion limit (~1000), so it escapes and kills the run.
How:
- An app reporting errors to the team stores a frame variable whose value is a string of ~1100+ nested brackets, e.g.
"[" * 1200— at 1200 chars it also passes the 2048-length cap atcode_variables_masking.py:174. - cymbal's serde_json has a 128-depth limit and keeps such text an ordinary string, so the value stays stored (this is exactly the pre-masking data the command exists to process).
- The operator runs
mask_error_tracking_stack_frame_code_variables --team-ids N --live-run. json.loadsat line 202 raisesRecursionError, theexcept ValueErrorat line 207 does not catch it, and the command dies mid-team; a plain rerun crashes on the same frame again.
I confirmed this end-to-end by importing the new module and calling mask_code_variables({"vars": "[" * 2048}) — it raises RecursionError. A balanced 1050-deep array (2100 chars) is caught by the length cap, but any ≤2048-char value with ~1100+ nesting levels triggers it.
Fix: Catch RecursionError alongside ValueError in _parse_json_container (returning None, as cymbal does when serde_json errors on depth).
React with 👍 if useful or 👎 if not
This branch has not been deployed
2 participants
Problem
The stack frames API still returns unmasked code variables from frames that cymbal stored before it masked them.
Changes
python manage.py mask_error_tracking_stack_frame_code_variables --team-ids 7,42 --live-run. The default is a dry run.--team-idsis required, and it is the only way to choose teams. Team 2 is left out of the list, because it drops its variables with the feat(error-tracking): add command to drop stored frame code variables #110760 command.logic/code_variables_masking.py, so stored frames match what cymbal stores today. A rule change has to land in both places.\ud800stays an ordinary string, as in cymbal.serde_jsonrejects it, and PostgreSQL rejects the character injsonb, so a parsed copy would fail the write and stop the team run.jsonb_setoncode_variables, filtered on the value the command read. A row that cymbal rewrote in the meantime is skipped, never overwritten.(team_id, raw_id, part)index.--batch-sizecounts raw ids, not rows or bytes..iterator()would not bound memory behind pgbouncer, where server-side cursors are off.--start-after-raw-idresumes a single-team run, and progress logs carry the last raw id. Running the same teams again is safe, because an already masked frame needs no write.Note
Run it after #112465 deploys. Until then, a cymbal pod can store unmasked variables again behind the cursor.
How did you test this code?
Test rationale:
test_code_variables_masking.pyruns the same cases as cymbal's masking tests, plus idempotence. It fails when the Python copy drifts from cymbal, for example a missed rule, a benign value that gets redacted, or JSON text with a lone surrogate that gets parsed. The masker is pure, so it is aSimpleTestCase.test_masks_only_the_listed_teamsfollows the drop command's test. It fails when a dry run writes, when a listed team is not masked, or when an unlisted team is. Dropping the team filter from the frame query fails it. It reads one frame per chunk over a raw frame with two parts, so a loop that skips a chunk fails it.Release status
Docs update
None. The command's docstring carries the usage and the run order.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5 (
claude-opus-5-5)/writing-tests,/writing-code-comments,/writing-pr-descriptions,/reviewing-with-coderabbit.🤖 Generated with Claude Code
https://claude.ai/code/session_01MxaTpjkJ3QxjaCSrYxXFHW