feat: detect unsafe TypeScript type assertions via AST (issue #22) - #23
Conversation
Add AST-based detection for three unsafe assertion forms using the
TypeScript Compiler API: as unknown as T, as Record<...>/as { ... },
and as T[]/as Array<T>. All default to high severity, configurable
via severityOverrides (including 'off' to silence a pattern).
BREAKING: as unknown as T was previously suppressed as 'safe' and now
reports at high severity. Silence via:
severityOverrides: { 'unsafe_double_type_assertion': 'off' }
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesUnsafe assertion detection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant SourceFile
participant analyzeFile
participant findUnsafeAssertions
participant SeverityOverrides
participant IssueList
SourceFile->>analyzeFile: analyze TypeScript file
analyzeFile->>findUnsafeAssertions: inspect AST assertions
findUnsafeAssertions-->>analyzeFile: return unsafe findings
analyzeFile->>SeverityOverrides: resolve effective severity
SeverityOverrides-->>analyzeFile: return severity or off
analyzeFile->>IssueList: append enabled findings
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
codeant-ai
Bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if (prevLineIsSuppressed(sourceLine)) { | ||
| ts.forEachChild(node, visit); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Suggestion: The AST suppression path only checks the immediately preceding physical line, so inline suppressions such as an assertion followed by eslint-disable-line, @ts-ignore, or @ts-expect-error are reported even though the existing unsafe-pattern scanner honors same-line suppression comments. Reuse the established same-line and previous-line suppression rules for AST findings. [logic error]
Severity Level: Major ⚠️
- ❌ Inline suppression fails for unsafe assertions.
- ⚠️ ESLint and TypeScript suppression behavior becomes inconsistent.
- ⚠️ Intentional assertions remain visible in detector reports.Steps of Reproduction ✅
1. Add `const value = input as Record<string, unknown>; // eslint-disable-line` to a
scanned `.ts` file.
2. `AISlopDetector.analyzeFile()` reaches the AST scan at `ai-slop-detector.ts:1151-1163`
after the regex loop.
3. `findUnsafeAssertions()` computes the assertion location and only calls
`prevLineIsSuppressed()` at `ai-slop-detector.ts:216-219`.
4. Because `prevLineIsSuppressed()` checks only the preceding physical line at
`ai-slop-detector.ts:183-190`, the same-line `eslint-disable-line` comment is ignored and
an `unsafe_object_assertion` issue is emitted.
5. The existing regex suppression logic at `ai-slop-detector.ts:981-987` does inspect
suppression text on the current line, demonstrating inconsistent behavior.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ai-slop-detector.ts
**Line:** 216:219
**Comment:**
*Logic Error: The AST suppression path only checks the immediately preceding physical line, so inline suppressions such as an assertion followed by `eslint-disable-line`, `@ts-ignore`, or `@ts-expect-error` are reported even though the existing unsafe-pattern scanner honors same-line suppression comments. Reuse the established same-line and previous-line suppression rules for AST findings.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const pattern = this.detectionPatterns.find(p => p.id === finding.type); | ||
| const effectiveSeverity = pattern ? pattern.severity : finding.severity; |
There was a problem hiding this comment.
Suggestion: Custom patterns can now use these IDs, but the built-in placeholder patterns are selected first and the pattern loop unconditionally skips every matching ID. Consequently, a custom pattern named unsafe_array_assertion, unsafe_object_assertion, or unsafe_double_type_assertion is silently ignored, and its configured message and severity cannot take effect. Reject these reserved IDs or distinguish custom patterns from the AST-only built-ins. [api mismatch]
Severity Level: Major ⚠️
- ❌ Custom rules using reserved IDs are silently ignored.
- ⚠️ Configured messages and severities become ineffective.
- ⚠️ AST findings use unintended built-in metadata.Steps of Reproduction ✅
1. Configure `.karpesloprc.json` with a custom pattern whose ID is
`unsafe_array_assertion`, using a custom message and severity.
2. `AISlopDetector.loadConfig()` accepts that ID during validation at
`ai-slop-detector.ts:546-572`, appends the custom pattern at
`ai-slop-detector.ts:629-641`, and does not reject reserved IDs.
3. During scanning, `analyzeFile()` unconditionally skips every pattern with that ID at
`ai-slop-detector.ts:967-972`, so the custom regex is never evaluated.
4. For an input such as `const rows = data as EventRow[];`, `findUnsafeAssertions()`
creates an AST finding at `ai-slop-detector.ts:245-257`.
5. The lookup at `ai-slop-detector.ts:1164` returns the earlier built-in placeholder
pattern rather than the appended custom pattern, so the configured custom message and
severity are not applied.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** ai-slop-detector.ts
**Line:** 1164:1165
**Comment:**
*Api Mismatch: Custom patterns can now use these IDs, but the built-in placeholder patterns are selected first and the pattern loop unconditionally skips every matching ID. Consequently, a custom pattern named `unsafe_array_assertion`, `unsafe_object_assertion`, or `unsafe_double_type_assertion` is silently ignored, and its configured message and severity cannot take effect. Reject these reserved IDs or distinguish custom patterns from the AST-only built-ins.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| test('severityOverrides with "off" silences unsafe_double_type_assertion without crashing', () => { | ||
| const fixtureFile = path.resolve(process.cwd(), 'tests/fixtures/temp-off-fixture.ts'); | ||
| const configFile = path.resolve(process.cwd(), '.karpesloprc.json'); | ||
| const savedConfig = fs.readFileSync(configFile, 'utf-8'); | ||
|
|
||
| fs.writeFileSync(fixtureFile, 'const x = value as unknown as Foo;\n', 'utf-8'); | ||
|
|
||
| const overriddenConfig = JSON.parse(savedConfig); | ||
| overriddenConfig.severityOverrides = { "unsafe_double_type_assertion": "off" }; | ||
| fs.writeFileSync(configFile, JSON.stringify(overriddenConfig), 'utf-8'); | ||
|
|
||
| const result = spawnSync( | ||
| process.execPath, | ||
| ['--import', 'tsx', path.resolve(process.cwd(), 'ai-slop-detector.ts'), '--strict', fixtureFile], | ||
| { encoding: 'utf-8', cwd: process.cwd() } | ||
| ); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Suggestion: This test mutates the repository-wide .karpesloprc.json and uses a fixed fixture path, so parallel tests or another detector process can observe the temporary off override and produce nondeterministic results. The cleanup is also registered only after spawnSync, so setup or process-launch failures can leave the repository configuration modified. Use a temporary project directory/config or register cleanup before any mutation. [race condition]
Severity Level: Major ⚠️
- ⚠️ Concurrent detector runs observe temporary configuration.
- ⚠️ Parallel tests can overwrite the fixed fixture.
- ❌ Setup failures can leave repository configuration mutated.Steps of Reproduction ✅
1. Start two test processes, or run another detector process, from the repository checkout
while this test executes.
2. The test writes the fixed file `tests/fixtures/temp-off-fixture.ts` at
`tests/ai-slop-detector.behavior.test.ts:128-132` and replaces the repository-wide
`.karpesloprc.json` at lines `134-136`.
3. The spawned detector uses `cwd: process.cwd()` at
`tests/ai-slop-detector.behavior.test.ts:138-142`; `AISlopDetector.loadConfig()` reads
that shared configuration from `ai-slop-detector.ts:610-628`.
4. A concurrent detector can therefore observe
`severityOverrides.unsafe_double_type_assertion = "off"` and suppress a finding
unexpectedly, while a concurrent test can see or overwrite the fixed fixture.
5. If fixture creation, JSON parsing, configuration writing, or process launch fails
before `try` begins at line `144`, the restoration at lines `148-150` is never registered
and the repository configuration can remain modified.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/ai-slop-detector.behavior.test.ts
**Line:** 127:144
**Comment:**
*Race Condition: This test mutates the repository-wide `.karpesloprc.json` and uses a fixed fixture path, so parallel tests or another detector process can observe the temporary `off` override and produce nondeterministic results. The cleanup is also registered only after `spawnSync`, so setup or process-launch failures can leave the repository configuration modified. Use a temporary project directory/config or register cleanup before any mutation.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ai-slop-detector.ts (1)
1109-1181: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSkip AST assertion findings in test/mock files when their flags are set.
The AST finding loop ignores the existing generic
pattern.skipTests/pattern.skipMockschecks andisTestFile/isMockFile, so full scans reportunsafe_double_type_assertion,unsafe_object_assertion, andunsafe_array_assertioninside tests/mock code even when those patterns haveskipTestsenabled. Add the same skip check before publishing AST findings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ai-slop-detector.ts` around lines 1109 - 1181, Update the AST finding loop in the TypeScript handling around findUnsafeAssertions to apply each finding’s pattern skipTests and skipMocks settings before pushing to this.issues. Reuse the existing test/mock file detection logic or equivalent isTestFile/isMockFile checks, and skip findings when the corresponding pattern flags are enabled; preserve severity filtering and publishing for allowed findings.
🧹 Nitpick comments (3)
ai-slop-detector.ts (3)
956-975: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider checking
severity === 'off'earlier in the loop.The
offshort-circuit (line 1132) sits after all the special-case filtering logic (fetch/try-catch handling, per-pattern whitelist checks, etc.), so disabled patterns still pay for that work on every match. Moving the check to right after theskipTests/skipMocksguard (around line 960) would skip the rest of the per-pattern branch entirely for disabled patterns.Also applies to: 1132-1132
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ai-slop-detector.ts` around lines 956 - 975, Move the pattern severity check into the loop immediately after the existing skipTests/skipMocks guard, returning or continuing for patterns with severity === 'off' before special-case filtering and regex work. Remove the later redundant severity short-circuit near the existing per-pattern handling, while preserving processing for all enabled patterns.
193-209: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueName-based type matching can misfire on locally-shadowed
Record/Arraytypes.
isUnsafeObjectType/isArrayTypematch by identifier text only (name === 'Record'/name === 'Array'), not by resolved symbol. A codebase with its owninterface Record<K, V>ortype Array<T>alias shadowing the global utility types would triggerunsafe_object_assertion/unsafe_array_assertioneven though it isn't the built-in type. This is an inherent trade-off of syntax-only (non-type-checked) detection and may be an accepted limitation, but is worth documenting given the tool's precision is central to its value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ai-slop-detector.ts` around lines 193 - 209, Document the syntax-only limitation in isUnsafeObjectType and isArrayType: their name-based Record/Array checks can incorrectly match locally shadowed aliases instead of the built-in types. Add concise documentation near these helpers describing the potential false positives and accepted precision trade-off; do not alter the detection behavior.
213-214: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant
getLineAndCharacterOfPositioncalls.
sourceFile.getLineAndCharacterOfPosition(node.getStart())is computed twice for the same position. Destructuring once avoids the duplicate call.♻️ Proposed simplification
- const sourceLine = sourceFile.getLineAndCharacterOfPosition(node.getStart()).line + 1; - const column = sourceFile.getLineAndCharacterOfPosition(node.getStart()).character + 1; + const { line: lineIdx, character: charIdx } = sourceFile.getLineAndCharacterOfPosition(node.getStart()); + const sourceLine = lineIdx + 1; + const column = charIdx + 1;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ai-slop-detector.ts` around lines 213 - 214, Update the position handling near sourceLine and column to call sourceFile.getLineAndCharacterOfPosition(node.getStart()) once, destructure the returned line and character values, and derive both one-based coordinates from them.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/ai-slop-detector.behavior.test.ts`:
- Around line 127-151: Protect the full configuration mutation sequence in the
severityOverrides test by moving the overriddenConfig write and spawnSync
invocation inside the existing try/finally, ensuring restoration runs if any
setup or execution step throws. Prefer an isolated temporary config or directory
if the test infrastructure supports it, so the test does not mutate the shared
project .karpesloprc.json during concurrent runs.
---
Outside diff comments:
In `@ai-slop-detector.ts`:
- Around line 1109-1181: Update the AST finding loop in the TypeScript handling
around findUnsafeAssertions to apply each finding’s pattern skipTests and
skipMocks settings before pushing to this.issues. Reuse the existing test/mock
file detection logic or equivalent isTestFile/isMockFile checks, and skip
findings when the corresponding pattern flags are enabled; preserve severity
filtering and publishing for allowed findings.
---
Nitpick comments:
In `@ai-slop-detector.ts`:
- Around line 956-975: Move the pattern severity check into the loop immediately
after the existing skipTests/skipMocks guard, returning or continuing for
patterns with severity === 'off' before special-case filtering and regex work.
Remove the later redundant severity short-circuit near the existing per-pattern
handling, while preserving processing for all enabled patterns.
- Around line 193-209: Document the syntax-only limitation in isUnsafeObjectType
and isArrayType: their name-based Record/Array checks can incorrectly match
locally shadowed aliases instead of the built-in types. Add concise
documentation near these helpers describing the potential false positives and
accepted precision trade-off; do not alter the detection behavior.
- Around line 213-214: Update the position handling near sourceLine and column
to call sourceFile.getLineAndCharacterOfPosition(node.getStart()) once,
destructure the returned line and character values, and derive both one-based
coordinates from them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c79d8196-f82a-4add-8fb9-4e54e7328529
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
CHANGELOG.mdai-slop-detector.tspackage.jsontests/ai-slop-detector.behavior.test.tstests/fixtures/false-positives.tstests/fixtures/unsafe-assertions.ts
💤 Files with no reviewable changes (1)
- tests/fixtures/false-positives.ts
- Add skipTests/skipMocks check to AST finding loop - Move severity === 'off' check earlier in regex loop - Document syntax-only limitation in isUnsafeObjectType/isArrayType - Destructure getLineAndCharacterOfPosition call once - Protect config mutation in test with try/finally
1 participant
Why this exists
Issue #22: KarpeSlop had no coverage for unsafe TypeScript
asassertions beyondas any. Double assertions (as unknown as T), unchecked object casts (as Record<...>), and unchecked array casts (as T[]) could bypass structural type safety at database and external-data boundaries without any warning in strict mode. The existing regex-basedunsafe_double_type_assertionpattern actively suppressedas unknown asas a false positive — the exact pattern the issue asked to detect.Design decisions
Decision: AST over regex for the new patterns
Why: The issue's scope boundary required avoiding false positives for
as const,as typeof,as keyof, andsatisfiesexpressions. A regex approach would need heuristic exclusions (English-word filters, comment detection) that the existing codebase already struggled with. The TypeScript Compiler API (ts.createSourceFile+ts.isAsExpression) naturally excludes comments and non-assertion syntax, and distinguishesAsExpressionfromSatisfiesExpressionat the AST level.Alternatives considered: Extending the existing regex patterns with more exclusions. Rejected because the heuristic false-positive filter (English-word list at the old line 946-950) was already fragile and would need to grow for each new form.
Tradeoff: Adds
typescriptas a runtime dependency (~50MB). The AST visitor runs on every .ts/.tsx file scanned, adding parse time proportional to file size.Decision:
'off'as a severity override valueWhy: The breaking change (
as unknown as Tnow reports at high severity) needed a project-wide opt-out mechanism.severityOverrideswas the existing per-pattern config knob; extending it with'off'was additive and reused the existing override loop and consumer lookup.Alternatives considered: A new
disabledPatternsconfig field. Rejected because it would add a parallel config surface with its own validation, documentation, and consumer logic.Tradeoff:
'off'is a config-level sentinel, not an issue severity. The type system enforces this boundary —AISlopIssue['severity']excludes'off', and the push-site guards prevent'off'from ever reaching the issue list.Decision: Reused pattern ID
unsafe_double_type_assertionWhy: The old regex pattern used this ID. Existing user
severityOverrideskeys targeting it continue to work without migration.Alternatives considered: A new ID
unsafe_double_assertion. Rejected because it would silently break existing user configs.Decision:
typescriptas a runtime dependencyWhy:
findUnsafeAssertionsimportsts.createSourceFileat runtime on every .ts/.tsx scan. The published CLI runs the source via tsx, which doesn't bundle typescript.Alternatives considered: Lazy
import('typescript')inside the function to keep it optional. Rejected for simplicity — the tool only scans TypeScript/JavaScript projects, so typescript is always needed.What this enables
A follow-up to migrate the remaining
as anydetection (unsafe_type_assertion) from regex to the AST visitor, unifying all assertion detection under one code path and eliminating the regex path's heuristic false-positive filters entirely.Risks and safeguards
Risk: Breaking change —
as unknown as Twas previously suppressed as safe and now reports at high severity.Mitigation: Configurable via
severityOverrides: { "unsafe_double_type_assertion": "off" }in .karpesloprc.json. Documented in CHANGELOG.Risk:
typescriptas a runtime dependency increases install size for JS-only consumers.Mitigation: The tool's primary use case is TypeScript projects. JS-only consumers are a minority; the dependency is correct for the primary audience.
Validation
Automated:
Evidence:
unsafe_double_type_assertion, 288unsafe_object_assertion, 190unsafe_array_assertionfindings — all three new patterns detected in real-world codeas const/as typeof/as keyof/satisfiesacross the entire codebase'off'severity override verified via integration test (temp config + CLI spawn)CodeAnt-AI Description
Detect unsafe TypeScript type assertions with precise exceptions
What Changed
as unknown as T, object casts such asas Record<...>oras { ... }, and array casts such asas T[]oras Array<T>as high-severity findings.as unknown as Tas automatically safe, while avoiding reports foras const,as typeof,as keyof, declaration files, comments, and explicitly suppressed lines.severityOverridesusing theoffvalue.Impact
✅ Finds unsafe casts at external-data and database boundaries✅ Fewer false positives from comments and intentional TypeScript syntax✅ Configurable assertion warnings without disabling other checks💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
New Features
Breaking Changes
as unknown as Tassertions are now reported as high-severity issues instead of being treated as safe.Tests