cycle/0009: PolicyResult class hierarchy#4
Merged
Conversation
- ContentResult, OutlineResult, RefusedResult — frozen classes with constructor validation, replacing the PolicyResult interface - evaluatePolicy returns class instances instead of plain objects - safe-read.ts and server.ts use instanceof dispatch instead of string-tag switching on projection - Policy tests use toBeInstanceOf instead of projection string checks - MCP/integration tests unchanged (JSON wire format preserved) - Zero projection === "string" comparisons in policy/operations code Systems-Style JavaScript P1 (runtime truth) + P3 (behavior on type) + P7 (runtime dispatch) applied to the policy layer.
Summary by CodeRabbit
WalkthroughRefactored Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/unit/policy/session-depth.test.ts (1)
55-63:⚠️ Potential issue | 🟡 MinorMissing assertion for
reasonin "both triggered" test case.The comment explicitly states "The tighter constraint wins: SESSION_CAP at 10 KB" but the test only asserts
toBeInstanceOf(OutlineResult)without verifyingresult.reason === "SESSION_CAP". This was presumably the original intent and matches the other OutlineResult tests in this file that do assert the reason.Proposed fix
// Both static (200 > 150 lines, 15KB > 12KB) and dynamic (15KB > 10KB mid) // The tighter constraint wins: SESSION_CAP at 10 KB expect(result).toBeInstanceOf(OutlineResult); + expect(result.reason).toBe("SESSION_CAP"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/policy/session-depth.test.ts` around lines 55 - 63, The test "session cap takes precedence reason when both static and dynamic triggered" calls evaluatePolicy and only asserts instance of OutlineResult but omits checking the result.reason; update the test to also assert that result.reason === "SESSION_CAP" (mirroring other tests) so the tighter constraint case is verified, referencing the evaluatePolicy call and the OutlineResult/result.reason fields.src/operations/safe-read.ts (1)
60-81: 🧹 Nitpick | 🔵 TrivialConsider explicit
instanceof OutlineResultfor exhaustiveness.The implicit else at line 71 works correctly today but silently assumes anything not
ContentResultorRefusedResultisOutlineResult. If the union expands in the future, TypeScript won't flag this as a missing case.Defensive exhaustiveness pattern
if (policy instanceof ContentResult) { return { ...base, content }; } if (policy instanceof RefusedResult) { return { ...base, next: [...policy.next], }; } - // projection === "outline" + // Explicit check for OutlineResult to catch future union expansion at compile time + if (!(policy instanceof OutlineResult)) { + // TypeScript will error here if union expands and this branch becomes reachable + const _exhaustive: never = policy; + throw new Error(`Unexpected policy result type: ${_exhaustive}`); + } + const outlineResult = extractOutline(content);This would require importing
OutlineResultas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operations/safe-read.ts` around lines 60 - 81, The current branch in the projection handling assumes any non-ContentResult/RefusedResult is an OutlineResult; make this exhaustive by explicitly checking for OutlineResult and adding a final `throw new Error` (or assert) for unknown types so new union members fail fast; import OutlineResult and replace the implicit else block with an `if (policy instanceof OutlineResult) { ... }` using extractOutline, and add a fallback that throws to surface future missing cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/policy/evaluate.ts`:
- Around line 105-112: The repeated pattern ...(sessionDepth !== undefined ? {
sessionDepth } : {}) used in multiple RefusedResult return paths should be
extracted to a small helper to avoid duplication; add a helper function or local
const (e.g., includeSessionDepth(sessionDepth) or optionalSessionDepth) that
returns either { sessionDepth } or {} and replace the conditional spread in each
RefusedResult construction (the lines that call new RefusedResult({...}) in
evaluate.ts) with a single spread of that helper.
- Around line 151-155: The nested ternary assigning finalReason in evaluate.ts
(using variables exceedsLines, exceedsBytes, hasSessionCap) is correct but hard
to read; replace it with an explicit if / else if chain (or add a short
clarifying comment) so the precedence is obvious: check if exceedsLines &&
!exceedsBytes -> set finalReason = "OUTLINE", else if exceedsBytes &&
hasSessionCap -> set finalReason = "SESSION_CAP", else set finalReason =
"OUTLINE" (keep the same conditions and return values but make the logic in the
function that computes finalReason explicit and readable).
---
Outside diff comments:
In `@src/operations/safe-read.ts`:
- Around line 60-81: The current branch in the projection handling assumes any
non-ContentResult/RefusedResult is an OutlineResult; make this exhaustive by
explicitly checking for OutlineResult and adding a final `throw new Error` (or
assert) for unknown types so new union members fail fast; import OutlineResult
and replace the implicit else block with an `if (policy instanceof
OutlineResult) { ... }` using extractOutline, and add a fallback that throws to
surface future missing cases.
In `@test/unit/policy/session-depth.test.ts`:
- Around line 55-63: The test "session cap takes precedence reason when both
static and dynamic triggered" calls evaluatePolicy and only asserts instance of
OutlineResult but omits checking the result.reason; update the test to also
assert that result.reason === "SESSION_CAP" (mirroring other tests) so the
tighter constraint case is verified, referencing the evaluatePolicy call and the
OutlineResult/result.reason fields.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: cdeb79fe-87c7-46b7-80c5-920951ca0b26
📒 Files selected for processing (10)
docs/method/backlog/asap/CC_policy-result-classes.mddocs/method/retro/0009-policy-result-classes/retro.mdsrc/mcp/server.tssrc/operations/safe-read.tssrc/policy/evaluate.tssrc/policy/types.tstest/unit/policy/bans.test.tstest/unit/policy/graftignore.test.tstest/unit/policy/session-depth.test.tstest/unit/policy/thresholds.test.ts
💤 Files with no reviewable changes (1)
- docs/method/backlog/asap/CC_policy-result-classes.md
Comment on lines
+105
to
+112
| return new RefusedResult({ | ||
| reason: ban.reason, | ||
| reasonDetail: ban.reasonDetail, | ||
| next: ban.next, | ||
| thresholds, | ||
| actual, | ||
| ...(sessionDepth !== undefined ? { sessionDepth } : {}), | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Repeated conditional spread pattern—consider extracting a helper.
The pattern ...(sessionDepth !== undefined ? { sessionDepth } : {}) appears in all four return paths. This is verbose and violates DRY.
♻️ Optional: Extract a helper for optional sessionDepth inclusion
+function optionalSessionDepth(sessionDepth: SessionDepth | undefined) {
+ return sessionDepth !== undefined ? { sessionDepth } : {};
+}
+
export function evaluatePolicy(input: PolicyInput, options?: PolicyOptions): PolicyResult {
// ... existing code ...
if (ban !== undefined) {
return new RefusedResult({
reason: ban.reason,
reasonDetail: ban.reasonDetail,
next: ban.next,
thresholds,
actual,
- ...(sessionDepth !== undefined ? { sessionDepth } : {}),
+ ...optionalSessionDepth(sessionDepth),
});
}
// Apply similarly to other return statementsAlso applies to: 120-127, 143-147, 157-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/policy/evaluate.ts` around lines 105 - 112, The repeated pattern
...(sessionDepth !== undefined ? { sessionDepth } : {}) used in multiple
RefusedResult return paths should be extracted to a small helper to avoid
duplication; add a helper function or local const (e.g.,
includeSessionDepth(sessionDepth) or optionalSessionDepth) that returns either {
sessionDepth } or {} and replace the conditional spread in each RefusedResult
construction (the lines that call new RefusedResult({...}) in evaluate.ts) with
a single spread of that helper.
Comment on lines
+151
to
155
| const finalReason: "OUTLINE" | "SESSION_CAP" = exceedsLines && !exceedsBytes | ||
| ? "OUTLINE" | ||
| : exceedsBytes && hasSessionCap | ||
| ? "SESSION_CAP" | ||
| : "OUTLINE"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nested ternary logic is correct but demands a double-take.
The cascaded ternary assigns SESSION_CAP only when bytes are exceeded and a dynamic session cap is active—otherwise it defaults to OUTLINE. This handles the edge case where both thresholds are exceeded but there's no session cap (returns OUTLINE).
Logic trace:
exceedsLines && !exceedsBytes→"OUTLINE"(line threshold only)exceedsBytes && hasSessionCap→"SESSION_CAP"(dynamic cap triggered)- fallback →
"OUTLINE"(static byte threshold exceeded, or both exceeded without session cap)
The logic is sound, but this reads like a riddle. Consider a comment clarifying the precedence or using an if/else if chain for maintainers who haven't memorized the matrix.
📖 Alternative: explicit if-else for clarity
- const finalReason: "OUTLINE" | "SESSION_CAP" = exceedsLines && !exceedsBytes
- ? "OUTLINE"
- : exceedsBytes && hasSessionCap
- ? "SESSION_CAP"
- : "OUTLINE";
+ // Determine reason: SESSION_CAP only when the *dynamic* byte cap triggered.
+ let finalReason: "OUTLINE" | "SESSION_CAP";
+ if (exceedsBytes && hasSessionCap) {
+ finalReason = "SESSION_CAP";
+ } else {
+ finalReason = "OUTLINE";
+ }This simplifies the logic—SESSION_CAP has a single narrow condition; everything else is OUTLINE.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const finalReason: "OUTLINE" | "SESSION_CAP" = exceedsLines && !exceedsBytes | |
| ? "OUTLINE" | |
| : exceedsBytes && hasSessionCap | |
| ? "SESSION_CAP" | |
| : "OUTLINE"; | |
| let finalReason: "OUTLINE" | "SESSION_CAP"; | |
| if (exceedsBytes && hasSessionCap) { | |
| finalReason = "SESSION_CAP"; | |
| } else { | |
| finalReason = "OUTLINE"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/policy/evaluate.ts` around lines 151 - 155, The nested ternary assigning
finalReason in evaluate.ts (using variables exceedsLines, exceedsBytes,
hasSessionCap) is correct but hard to read; replace it with an explicit if /
else if chain (or add a short clarifying comment) so the precedence is obvious:
check if exceedsLines && !exceedsBytes -> set finalReason = "OUTLINE", else if
exceedsBytes && hasSessionCap -> set finalReason = "SESSION_CAP", else set
finalReason = "OUTLINE" (keep the same conditions and return values but make the
logic in the function that computes finalReason explicit and readable).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace
PolicyResultinterface with frozen runtime-backed classes:ContentResult,OutlineResult,RefusedResult. Dispatch viainstanceofreplaces string-tag switching.Systems-Style JavaScript P1 + P3 + P7 applied to the policy layer.
projection === "string"comparisons in policy/operations codeTest plan
toBeInstanceOfinstead of string checks