fix: truncate input before sanitization to fix 816s JS test timeout - #50810
Conversation
…ocessing of large inputs The "should truncate long content" test was timing out at ~816 seconds because sanitizeContentCore ran all expensive operations (hardenUnicodeText, neutralizeAllMentions, applyToNonCodeRegions, applyURLSanitizationPolicy, etc.) on the full 600,000-character input before truncating at line 1382. Fix: add an early applyTruncation call at the start of sanitizeContentCore so oversized inputs are cut down to maxLength (default 524,288) before any other processing begins. A second truncation pass is still applied later (after normalization may reduce length by stripping invisible chars). This drops the test from ~816s to under 1s. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Triage SummaryCategory: bug (perf fix) | Risk: low | Priority score: 55/100 Score breakdown: impact 25 + urgency 15 + quality 15 Recommended action: Small, well-scoped fix (5 additions, 1 file) that resolves an 816s test timeout by truncating input before expensive sanitization passes. No CI data available yet; no reviews posted. Low blast radius, high test-suite value.
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Hey One small thing to consider for completeness:
If you'd like to add test coverage, here's a prompt: Otherwise, this is ready to go! 🚀
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
|
✅ Test Quality Sentinel completed test quality analysis. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. No test files were added or modified in this PR. This PR only modifies the production file actions/setup/js/sanitize_content_core.cjs (a performance optimization for early truncation). Test Quality Sentinel skipped per guidelines. |
There was a problem hiding this comment.
Pull request overview
Moves truncation earlier in content sanitization to avoid expensive processing of oversized inputs.
Changes:
- Adds a raw-input truncation pre-pass.
- Retains post-normalization truncation.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/sanitize_content_core.cjs |
Adds early input truncation to the core sanitizer. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
| // Apply truncation early to avoid running expensive operations on oversized inputs. | ||
| // This is a pre-pass truncation on raw content; a second truncation pass is applied | ||
| // later after normalization (which may reduce length via stripping invisible chars). | ||
| content = applyTruncation(content, maxLength); |
| // Apply truncation early to avoid running expensive operations on oversized inputs. | ||
| // This is a pre-pass truncation on raw content; a second truncation pass is applied | ||
| // later after normalization (which may reduce length via stripping invisible chars). | ||
| content = applyTruncation(content, maxLength); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The fix is correct and well-scoped. Moving applyTruncation before expensive processing is the right approach to avoid the 816s timeout on oversized inputs. The dual-pass design (pre-normalization + post-normalization) is sound.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14.5 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /diagnosing-bugs — approving with one suggestion on test coverage.
📋 Key Themes & Highlights
Key Themes
- Correct root-cause fix: the expensive sanitization pipeline now never runs on inputs larger than
maxLength, eliminating the timeout. - Dual-pass design preserved: the second
applyTruncationcall after normalization is correctly retained for cases where stripping invisible characters reduces content length past the limit. - Regression test gap: there is no test that guards the new early-truncation path. A future refactor could silently reintroduce the timeout.
Positive Highlights
- ✅ Minimal, surgical change — 5 lines, zero deleted, no side-effects on other code paths
- ✅ Clear inline comments explain the dual-pass rationale
- ✅ PR description quantifies the fix (816,365 ms → 387 ms) — excellent diagnosis
|
|
||
| // Build list of allowed domains from environment and GitHub context | ||
| const allowedDomains = buildAllowedDomains(); | ||
|
|
There was a problem hiding this comment.
[/diagnosing-bugs] No regression test guards the early-truncation path — the 816s timeout could silently regress if applyTruncation is ever moved or conditionalised.
💡 Suggested regression test sketch
it("sanitizeContentCore completes quickly on oversized input", () => {
const huge = "a".repeat(600_000);
const start = Date.now();
sanitizeContentCore(huge, 524288, 10);
expect(Date.now() - start).toBeLessThan(1000); // must finish in <1 s
});A timing-based test (or at minimum a test that asserts the output length equals maxLength) would lock in this performance contract and catch future regressions early.
@copilot please address this.
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: performance fix is sound, one edge-case worth hardening
The core change (move applyTruncation before expensive Unicode/entity/mention processing) correctly resolves the 816s hang for oversized input and is a minimal, well-targeted diff.
💡 Themes and analysis
What was checked:
- Confirmed via manual simulation that the new early-truncation call short-circuits pathological-length inputs before the 3x
applyToNonCodeRegionspasses and other O(n) regex scans run, which matches the stated fix for the 816s timeout. - Diff is 5 lines, scoped to a single file, low blast radius.
- Found one non-blocking correctness/edge-case concern: double-applying
applyTruncation(once pre-normalization, once post-normalization) can produce a duplicated or embedded truncation marker if normalization doesn't shrink the string belowmaxLengthafter the first pass (e.g., entity/homoglyph decoding expansion). Verified via simulation thatapplyTruncationrun twice on already-marked content re-truncates the marker text itself rather than detecting/replacing it. This is pre-existing logic inapplyTruncation, not introduced by this PR, but the new early-truncation call increases the chance the function is invoked twice on marker-bearing content — worth a regression test asserting at most one truncation marker appears in output. - No test coverage was added in this PR to assert single-marker output invariants after the double-truncation path; existing
compute_text.test.cjstests only check that truncation occurs, not that it's clean.
Verdict rationale: Not requesting changes — the flagged issue is a pre-existing edge case in applyTruncation's design, not a new bug introduced by this specific diff, and the primary goal (fixing the test timeout) is achieved correctly. Recommend addressing the marker-duplication hardening in a follow-up.
| // Apply truncation early to avoid running expensive operations on oversized inputs. | ||
| // This is a pre-pass truncation on raw content; a second truncation pass is applied | ||
| // later after normalization (which may reduce length via stripping invisible chars). | ||
| content = applyTruncation(content, maxLength); |
There was a problem hiding this comment.
Running truncation twice on oversized input can leave the truncation marker itself embedded before the length cutoff, duplicating/garbling the trailer instead of cleanly signaling truncation once.
💡 Details
With the new early pass, content = applyTruncation(content, maxLength) truncates raw input and appends "\n[Content truncated due to length]". If normalization later still leaves the string over maxLength (e.g. it doesn't shrink, or entity/homoglyph decoding expands it back up), the second applyTruncation call at line ~1387 truncates again and appends the same marker a second time — but because substring(0, maxLength) is applied to text that already contains the first marker, the cut can land inside or immediately before the existing "[Content truncated due to length]" string, producing a confusing double-marker or a marker embedded mid-content rather than at a clean boundary.
This is a correctness/UX regression risk from adding a second truncation pass over content that may already carry a truncation marker. Consider either:
- Stripping any existing truncation marker before re-applying
applyTruncation, or - Reserving space for a single marker and asserting only one marker ever appears in output, with a regression test asserting
sanitized.match(/\[Content truncated/g).length <= 1.
// Example guard
function applyTruncation(content, maxLength) {
// Strip an already-applied marker before re-truncating
content = content.replace(/\n\[Content truncated due to (length|line count)\]$/, "");
...
}No existing test in compute_text.test.cjs currently checks for marker duplication after the double-truncation path introduced by this PR.
|
🎉 This pull request is included in a new release. Release: |
sanitizeContentCorewas running all expensive operations — Unicode normalization, entity decoding, homoglyph mapping, 3×applyToNonCodeRegionspasses, mention neutralization, URL policy — on the full raw input before truncating. A 600k-char input caused the"should truncate long content"test to hang for 816 seconds.Change
applyTruncationto the top ofsanitizeContentCore, before any other processing. Inputs exceedingmaxLength(default 524,288) are cut immediately.Result:
compute_text.test.cjs(51 tests) drops from 816,365 ms (1 failed) → 387 ms (all pass).