refactor: centralize model, file classification, and strategy constants

[rxolve]

• Add DEFAULT_MODEL constant in types.ts, replace hardcoded model strings across source and tests (claude-opus-4-6)
• Extract file-classifier.ts with isTestFile/isConfigFile/isSourceFile/ isSchemaFile, removing duplicate implementations from exclude-filter, base-framework, and analyzer
• Add CRITICAL_MODULE_PATTERN constant, replace 3 inline regex copies
• Add STRATEGY_THRESHOLDS constant, replace magic numbers in strategy-selector

[github-actions]

🤖 Dialectic PR Review

Framework: vanilla
Strategy: small
Files Reviewed: 17

Affected Areas: 🧪 Tests

Summary

This is a well-structured refactoring that consolidates duplicated file classification logic into a single source of truth and extracts magic numbers/strings into named constants. The model upgrade from claude-sonnet-4 to claude-opus-4-6 is a deliberate change. However, the consolidation introduces subtle behavioral regressions: the unified classifier merges two previously distinct classification lists (ExcludeFilter vs BaseFramework), widening the match criteria in both contexts. The most impactful change is that .md files are now classified as config files in the ExcludeFilter context (affecting configOnly flag), and migration files now trigger schemaChanged in the analyzer. These should be verified as intentional before merging.

Issues

🐛 Regex constructed from CRITICAL_MODULE_PATTERN.source produces incorrect pattern

File: src/core/smart-filter.ts (Line 21)
Type: bug
Confidence: high

The expression new RegExp(src${CRITICAL_MODULE_PATTERN.source}) concatenates the string src with \/(auth|payments|billing|security)\/, producing the pattern src\/(auth|payments|billing|security)\/. This matches src/auth/ but the original pattern was src\/(auth|payments|billing|security)\/ (with a literal src prefix). The original inline regex was /src\/(auth|payments|billing|security)\// which required the src/ prefix. However, CRITICAL_MODULE_PATTERN is defined as /\/(auth|payments|billing|security)\// (starts with /), so the composed regex becomes src\/(auth|payments|billing|security)\/ which requires src/ before the module name. This accidentally works for the src/ case, but the real issue is that CRITICAL_MODULE_PATTERN is a stateful regex (it's a module-level RegExp literal). Since it does NOT have the g flag, this is not a lastIndex statefulness bug. However, the pattern composition approach is fragile — if CRITICAL_MODULE_PATTERN ever gains flags or changes its source, this new RegExp(...) construction silently drops all flags. A safer approach would be to define a separate constant for the smart-filter pattern or use a string constant for the pattern source.

Suggestion:

Consider exporting the pattern source string as a constant (e.g., CRITICAL_MODULE_PATH_SEGMENT = '(auth|payments|billing|security)') and constructing both regexes from it, rather than extracting .source from one regex to build another. This avoids fragile coupling to the regex's internal representation.

---

🐛 Behavioral regression: configFile classification diverges between old ExcludeFilter and old BaseFramework

File: src/utils/file-classifier.ts (Line 8)
Type: bug
Confidence: high

The new unified file-classifier.ts merges config detection from two sources that had different lists. The old ExcludeFilter.isConfigFile had CONFIG_EXTENSIONS = ['.json', '.yaml', '.yml', '.toml', '.ini'] (no .md) and CONFIG_NAMES included 'nest-cli.json' but NOT '.eslintrc' or '.prettierrc'. The old BaseFramework.isConfigFile had CONFIG_EXTENSIONS including .md and CONFIG_NAMES including '.eslintrc' and '.prettierrc' but NOT 'nest-cli.json'. The new unified version includes ALL of them (.md, nest-cli.json, .eslintrc, .prettierrc). This means ExcludeFilter.isConfigFile now classifies .md files as config (previously it didn't), and BaseFramework.isConfigFile now classifies nest-cli.json as config (previously it didn't). The .md change is particularly impactful: markdown files will now be treated as config files by ExcludeFilter, which affects the configOnly flag in analyzeContext() — a PR with only .md and source files could have its configOnly flag incorrectly set if the source files are .md documentation. More critically, the shouldExclude method in ExcludeFilter does NOT use isConfigFile, but configOnly in the analyzer does: configOnly: paths.every((p) => this.excludeFilter.isConfigFile(p)). A PR touching only markdown files would now be flagged as configOnly: true, potentially receiving a lighter review strategy than intended.

Suggestion:

Either maintain separate classification lists for ExcludeFilter vs BaseFramework contexts, or explicitly verify that the unified superset is the intended behavior. At minimum, consider whether .md files should be classified as config files in the ExcludeFilter context, as this changes the configOnly flag semantics.

---

🐛 Behavioral regression: isSchemaFile now matches '/migrations/' but old ExcludeFilter did not

File: src/utils/file-classifier.ts (Line 10)
Type: bug
Confidence: high

The old ExcludeFilter did NOT have isSchemaFile — the schema check in analyzer.ts used a regex /\.(entity|schema|model)\.(ts|js)$/ which did NOT match /migrations/. The old BaseFramework.isSchemaFile DID match /migrations/. The new unified isSchemaFile includes /migrations/ and is now used in analyzer.ts via isSchemaFile(p). This means analyzer.ts's schemaChanged flag will now be true for migration files, which was NOT the case before. This could cause strategy escalation or different review behavior for PRs that only touch migration files.

Suggestion:

Verify that matching migration files as schema changes in the analyzer context is intentional. If not, the analyzer should use the stricter regex pattern it had before, or isSchemaFile should be split into variants.

---

Metadata

• Tokens Used: 10,619
• Duration: 32.89s

Powered by Dialectic PR Review

[github-actions]

🤖 Dialectic PR Review

Framework: vanilla
Strategy: small
Files Reviewed: 19

Affected Areas: 🧪 Tests

Summary

Well-structured refactoring that consolidates duplicated file classification logic into a single source of truth and extracts magic constants. The changes are largely positive. Three actionable issues were identified: (1) a subtle behavioral regression where .md files are now classified as config files in a broader context than before, which could affect review strategy selection; (2) the formatModelName function has limited pattern coverage and will produce odd output for date-suffixed model IDs; (3) the shared module-level regex CRITICAL_MODULE_PATTERN is safe today but fragile against future flag additions. The model change from Sonnet to Opus is an intentional upgrade. Test updates correctly reference the DEFAULT_MODEL constant.

Issues

🐛 Stateful regex with global-like behavior used in Array.some() — CRITICAL_MODULE_PATTERN uses RegExp.test() which is safe here, but the regex is module-level mutable state

File: src/core/types.ts (Line 16)
Type: bug
Confidence: high

CRITICAL_MODULE_PATTERN is a module-level RegExp instance created via new RegExp(...). Currently it has no g or y flag, so RegExp.prototype.test() does not advance lastIndex and is safe. However, this is a subtle invariant: if anyone later adds a g or y flag (e.g. for matchAll usage), every call site using .test() inside Array.some() (in analyzer.ts, smart-filter.ts, base-framework.ts) would silently produce intermittent false negatives due to lastIndex statefulness. A RegExp literal recreated at each call site would be immune to this class of bug. Since the pattern is shared across many critical modules, this is worth hardening.

Suggestion:

Either (a) freeze the regex with Object.freeze(CRITICAL_MODULE_PATTERN) and add a comment warning against adding flags, or (b) export CRITICAL_MODULE_PATH only and let each call site construct a local regex or use String.prototype.match(). Option (b) is safest.

---

🐛 formatModelName only handles claude-*-N-N pattern — will silently return raw IDs for valid models like claude-sonnet-4-6

File: src/core/review-engine.ts (Line 24)
Type: bug
Confidence: high

The regex ^claude-(\w+)-(\d+)-(\d+) matches claude-opus-4-6 → Claude Opus 4.6 correctly. However, it also matches claude-sonnet-4-20250514 (the old model ID still valid in the API) and would produce Claude Sonnet 4.20250514 which is nonsensical. More importantly, any model ID that doesn't match the pattern (e.g., a future claude-5-opus or a non-Claude model) falls through to return modelId which returns the raw API identifier as a display name. This is a minor cosmetic bug but it's in user-facing output (GitHub comment footer).

Suggestion:

Add handling for the date-suffixed format (claude-sonnet-4-20250514) and consider a lookup map for known models rather than regex parsing. At minimum, add a test case for the old model ID format.

---

🐛 Behavioral regression: isConfigFile in file-classifier.ts includes .md but ExcludeFilter.isConfigFile did not — and vice versa for nest-cli.json

File: src/utils/file-classifier.ts (Line 10)
Type: bug
Confidence: high

The old BaseFramework.isConfigFile included .md in configExtensions and .eslintrc/.prettierrc in configNames. The old ExcludeFilter.isConfigFile did NOT include .md or .eslintrc/.prettierrc but DID include nest-cli.json. The new unified file-classifier.ts merges both lists, so ExcludeFilter.isConfigFile now considers .md files as config (behavioral change) and BaseFramework now considers nest-cli.json as config (minor expansion). The .md change is significant: markdown files like README.md or CHANGELOG.md will now be classified as config files by ExcludeFilter, which affects the configOnly flag in analyzeContext(). A PR that only changes .md files would now be flagged as configOnly: true, potentially receiving a less thorough review.

Suggestion:

Verify that .md files should indeed be treated as config files in the ExcludeFilter context. If not, consider splitting the config lists or removing .md from CONFIG_EXTENSIONS and handling it separately in framework-specific logic.

---

Metadata

• Tokens Used: 12,028
• Duration: 29.50s

Powered by Dialectic PR Review