feat: add support for disabling rules within a package#41
Conversation
📝 WalkthroughWalkthroughOrchestrator now resolves overrides and available packs, filters prompts by configured scanPaths/packs and resolved overrides, preserves input order in concurrent runs, skips prompts failing with MissingDependencyError, tightens error messaging, and normalizes formatting/initializers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI
participant Orchestrator
participant Resolver as "Resolution Resolver"
participant Filter as "Prompt Filter"
participant Runner as "Concurrency Runner"
participant Worker
participant Evaluator
participant Reporter
CLI->>Orchestrator: start scan (scanPaths, resolution)
Orchestrator->>Resolver: resolve config & packs
Resolver-->>Orchestrator: resolved.packs, resolved.overrides
Orchestrator->>Filter: filter prompts by packs & overrides
Filter-->>Orchestrator: filtered prompt list (ordered)
Orchestrator->>Runner: dispatch ordered work items
loop per item (ordered)
Runner->>Worker: execute item
Worker->>Evaluator: evaluate prompt
alt missing dependency
Evaluator-->>Worker: MissingDependencyError
Worker-->>Runner: warn and mark skipped
else success/failure
Evaluator-->>Worker: result
Worker-->>Runner: return result (preserve position)
end
end
Runner->>Reporter: emit ordered results (normalized fields)
Reporter-->>CLI: final report
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
src/cli/orchestrator.ts (1)
795-803: Optional: Consider combining filter operations.The current implementation filters prompts twice in succession. While functionally correct and arguably more readable, this could be optimized into a single filter pass.
🔎 View suggested refactor:
-// Filter prompts by active packs -let activePrompts = prompts.filter( - (p) => p.pack && resolution.packs.includes(p.pack) -); - -// Filter out disabled rules (Pack.RuleId=disabled) -activePrompts = activePrompts.filter((p) => { - const disableKey = `${p.pack}.${p.meta.id}`; - return resolution.overrides[disableKey] !== "disabled"; -}); +// Filter prompts by active packs and exclude disabled rules +const activePrompts = prompts.filter((p) => { + if (!p.pack || !resolution.packs.includes(p.pack)) return false; + if (!p.meta?.id) return true; + const disableKey = `${p.pack}.${p.meta.id}`; + return resolution.overrides[disableKey] !== "disabled"; +});This reduces iteration overhead, though the current approach may be clearer for maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli/orchestrator.ts(37 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cli/orchestrator.ts (10)
src/output/vale-json-formatter.ts (1)
ValeJsonFormatter(26-86)src/cli/types.ts (9)
ProcessViolationsParams(82-92)ProcessCriterionParams(94-100)ProcessCriterionResult(102-107)ProcessPromptResultParams(114-117)ErrorTrackingResult(40-46)RunPromptEvaluationParams(119-126)RunPromptEvaluationResult(128-130)EvaluateFileParams(132-136)EvaluateFileResult(138-140)src/output/location.ts (1)
locateEvidenceWithMatch(107-151)src/errors/index.ts (1)
handleUnknownError(46-51)src/output/json-formatter.ts (1)
JsonFormatter(54-94)src/evaluators/evaluator-registry.ts (1)
createEvaluator(69-77)src/evaluators/index.ts (1)
createEvaluator(21-21)src/prompts/prompt-loader.ts (1)
PromptFile(8-8)src/schemas/prompt-schemas.ts (1)
PromptFile(64-64)src/boundaries/scan-path-resolver.ts (1)
ScanPathResolver(18-72)
🔇 Additional comments (5)
src/cli/orchestrator.ts (5)
1-44: LGTM: Import formatting improvements.The consistent use of double quotes and improved formatting enhances code readability without altering functionality.
49-686: LGTM: Style normalization with safer defaults.The changes consistently use empty strings as defaults for match fields and improve code formatting. These modifications enhance code safety and readability without affecting core behavior.
694-743: LGTM: Override application logic is sound.The function correctly applies overrides to prompt metadata and maintains proper error handling. The style improvements enhance readability.
748-780: LGTM: File evaluation logic with proper backward compatibility.The function correctly:
- Initializes evaluation state and determines applicable prompts
- Processes overrides into an efficient lookup map (O(1) access)
- Maintains backward compatibility when
scanPathsis not configured- Properly aggregates results and handles errors including
MissingDependencyErrorThe implementation preserves existing behavior while adding the new filtering functionality.
Also applies to: 806-901
908-965: LGTM: Style improvements in public API.The formatting changes improve consistency without altering the public API behavior. Error handling and result aggregation remain intact.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/orchestrator.ts (1)
802-814: Override map lookup will fail—key extraction doesn't match prompt identification.The code splits override keys on the first dot (line 805), extracting the pack name into
promptId. For a key like"Acme.GrammarCheck.strictness", it stores the value atoverrideMap["Acme"]["GrammarCheck.strictness"]. However, line 817 looks up overrides usingprompt.id, which is the filename (e.g.,"GrammarCheck"), not the pack name. This mismatch means property overrides will never be retrieved.Either split on the last dot to extract the property name, or look up using the full
"${prompt.pack}.${prompt.meta.id}"key instead ofprompt.id.
♻️ Duplicate comments (1)
src/cli/orchestrator.ts (1)
795-800: Consider case-insensitive matching for "disabled" value.The null safety concern from previous reviews is now addressed by the optional chaining at line 797. However, the case-sensitivity issue at line 799 remains: strict equality (
!== "disabled") won't match variations like"Disabled","DISABLED", or booleanfalse. Users might expect case-insensitive configuration.Based on learnings, past reviews suggested handling both string variations and boolean values for better UX.
🔎 View suggested improvement:
- return resolution.overrides[disableKey] !== "disabled"; + const value = resolution.overrides[disableKey]; + return typeof value === "string" + ? value.toLowerCase() !== "disabled" + : value !== false;
🧹 Nitpick comments (2)
src/cli/orchestrator.ts (2)
64-75: Cleaner worker construction pattern.The refactored concurrency logic is more idiomatic. The undefined check at line 71 is defensive—if the
itemsarray never contains undefined entries by contract, this guard could be removed for clarity.
704-713: Clarify or remove outdated comment.The comment at lines 706-710 mentions handling nested properties like
"GrammarChecker.strictness=9", but the override preprocessing at lines 802-814 already splits dotted keys, so overrides here are flat (e.g.,"strictness"). Consider updating or removing this comment to avoid confusion.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli/orchestrator.ts(37 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/cli/orchestrator.ts (3)
src/output/reporter.ts (1)
printIssueRow(39-110)src/cli/types.ts (8)
ProcessViolationsParams(82-92)ProcessCriterionParams(94-100)ProcessCriterionResult(102-107)ErrorTrackingResult(40-46)RunPromptEvaluationParams(119-126)RunPromptEvaluationResult(128-130)EvaluateFileParams(132-136)EvaluateFileResult(138-140)src/boundaries/scan-path-resolver.ts (1)
ScanPathResolver(18-72)
🔇 Additional comments (2)
src/cli/orchestrator.ts (2)
49-51: LGTM: Clean helper extraction.The
resolveEvaluatorTypehelper improves readability by centralizing the default evaluator type logic.
849-856: LGTM: Graceful handling of missing dependencies.The new error path that skips prompts with
MissingDependencyErrorimproves user experience by avoiding hard failures when optional dependencies aren't available. The warning messages provide clear guidance.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/orchestrator.ts (1)
801-818: Critical bug: Override map keyed incorrectly.The override pre-processing splits on the first dot (line 804), which creates map keys using only the pack name (e.g.,
"Acme"). However, prompts are identified by the combination of pack and rule ID (e.g.,"Acme.GrammarCheck"as seen on line 793).When line 816 looks up
overrideMap.get(prompt.id), it won't find the correct overrides because:
- For config
Acme.GrammarCheck.severity=warning, the map stores:{"Acme": {"GrammarCheck.severity": "warning"}}- But it should store:
{"Acme.GrammarCheck": {"severity": "warning"}}This means prompt-specific overrides (other than
disabled) will not be applied correctly.🔎 Proposed fix
// Pre-process overrides into a map for O(1) lookup const overrideMap = new Map<string, Record<string, unknown>>(); for (const [key, value] of Object.entries(resolution.overrides)) { - const dotIndex = key.indexOf("."); - if (dotIndex > 0) { - const promptId = key.substring(0, dotIndex); - const prop = key.substring(dotIndex + 1); + const parts = key.split("."); + if (parts.length >= 2) { + const pack = parts[0]; + const ruleId = parts[1]; + const promptKey = `${pack}.${ruleId}`; + const prop = parts.slice(2).join("."); + + // Only process property overrides (skip Pack.Rule=disabled which is handled above) + if (prop) { - if (!overrideMap.has(promptId)) { - overrideMap.set(promptId, {}); + if (!overrideMap.has(promptKey)) { + overrideMap.set(promptKey, {}); + } + overrideMap.get(promptKey)![prop] = value; } - overrideMap.get(promptId)![prop] = value; } } for (const prompt of activePrompts) { - const promptOverrides = overrideMap.get(prompt.id) || {}; + const promptKey = `${prompt.pack}.${prompt.meta?.id || prompt.id}`; + const promptOverrides = overrideMap.get(promptKey) || {}; toRun.push({ prompt, overrides: promptOverrides }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli/orchestrator.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/cli/orchestrator.ts (4)
src/output/reporter.ts (1)
printIssueRow(39-110)src/output/location.ts (1)
locateEvidenceWithMatch(107-151)src/output/json-formatter.ts (1)
JsonFormatter(54-94)src/boundaries/scan-path-resolver.ts (1)
ScanPathResolver(18-72)
🔇 Additional comments (3)
src/cli/orchestrator.ts (3)
57-78: LGTM! Concurrency runner preserves order correctly.The refactored
runWithConcurrencyfunction now explicitly pre-allocates the results array and uses index-based assignment to preserve input order, which is an improvement over the previous implementation. The undefined check at line 71 adds defensive safety.
790-799: LGTM! Rule disabling logic is correct and addresses past concerns.The filtering logic properly:
- Checks pack membership (line 791)
- Handles missing
meta.idsafely with optional chaining (line 792) ✅ Addresses past null safety concern- Performs case-insensitive matching for "disabled" with type guard (lines 795-798) ✅ Addresses past case-sensitivity concern
The logic correctly filters out prompts where
Pack.RuleIdis set to any case variation of "disabled" in the configuration.
846-855: Good addition: graceful handling of missing dependencies.The code now distinguishes
MissingDependencyErrorfrom other failures and skips the evaluation with a warning rather than counting it as a request failure. This provides a better user experience when optional dependencies are unavailable.
* feat: add support for disabling rules within a package * feat: add null check when filtering disabled rules * feat: make rule disabled key case-insensitive * fix: remove rule property overrides from config
2 participants
Summary
Adds support for selectively disabling individual rules within a package via configuration.
Also removes support for property-level overrides (e.g.,
Pack.Rule.severity=warning) - rule properties should be defined in the rule file itself, not overridden via config.Usage