Refactor files under 400-line limit

[potatoqualitee]

No description provided.

[claude]

Code Review — PR #54: Refactor files under 400-line limit

Overview

This PR enforces a 400-line file-length ceiling across the codebase by splitting large classes into partial-class files organized by responsibility. It also extracts the PowerShell Core assembly resolver from an inline here-string in dbatools.library.psm1 into a standalone dbatools.library.CoreRedirector.cs file, and splits verbose documentation and test files in the same way. The mechanical approach is sound and the naming is intuitive.

---

Bugs

.codex/settings.json — wrong hook path (will silently never run)

.codex/settings.json line 8 references:

"${CLAUDE_PROJECT_DIR:-.}"/.claude/hooks/stop-file-length.sh

But the actual hook lives at .codex/hooks/stop-file-length.sh. The referenced path (.claude/hooks/…) does not exist, so the Codex hook will either error or silently no-op on every run, defeating the whole purpose of introducing it.

Fix:

"command": "\"${CLAUDE_PROJECT_DIR:-.}\"/.codex/hooks/stop-file-length.sh"

---

Potential Issues

CoreRedirector.cs — minor race condition in Register()

public static void Register(string libPath)
{
    if (_registered) return;      // (A) two threads can both pass here
    _libPath = libPath;
    AssemblyLoadContext.Default.Resolving += OnResolving;
    ...
    _registered = true;           // (B) set too late for the racing thread
}

Two concurrent calls (e.g. parallel PowerShell runspaces) can both read _registered == false at (A) and each register event handlers, resulting in duplicate resolver callbacks. In practice the PSM1 guards this with if (-not ("CoreRedirector" -as [type])), but a lock or Interlocked.CompareExchange on _registered would make the contract self-contained:

private static int _registered = 0;

public static void Register(string libPath)
{
    if (Interlocked.CompareExchange(ref _registered, 1, 0) != 0) return;
    _libPath = libPath;
    AssemblyLoadContext.Default.Resolving += OnResolving;
    AssemblyLoadContext.Default.ResolvingUnmanagedDll += OnResolvingUnmanagedDll;
}

CoreRedirector.cs — placement in repo root

The file lives at the repo root rather than under project/. It is not compiled by any .csproj; it is read and compiled at runtime via Add-Type. This is fine functionally, but the convention for every other C# source file is project/…. Consider a dedicated src/ or keeping it alongside the PSM1 is acceptable, but a note in CLAUDE.md or a comment in the file explaining why it lives at the root (i.e. it is a runtime-compiled source, not a build artifact) would help future contributors avoid confusion.

---

Style Concerns

String interpolation in newly-added test files

CsvDataReaderTest.ThreadSafety.cs (line 25) and several other new test partial files use $"..." string interpolation, which the project's CLAUDE.md prohibits without exception:

FORBIDDEN — String interpolation (any version)
var msg = $"Hello {name}"; // NO — not allowed in this project

Examples from the newly added files:

sb.AppendLine($"{i},Name{i},{i * 10}");   // ThreadSafety.cs:25
$"Column{i}"                              // CsvReaderBenchmarks.Benchmarks.cs:93

The test project does not set an explicit LangVersion for Debug/Release builds, so the compiler permits these, but the convention should be consistent. Replace with String.Format or StringBuilder concatenation to match the rest of the codebase.

---

Unexplained Deletion

.github/workflows/test-avoidconflicts.yml — entire workflow removed (87 lines)

The file is fully deleted with no explanation in the PR description or commit messages. If this workflow became redundant due to the resolver refactor, that reasoning should be stated. If it is being temporarily disabled, a comment should say so.

---

What's Done Well

• Partial-class split is clean and logical. Naming conventions like CsvDataReader.Constructors.cs, CsvDataReader.ParallelParsing.cs, ManagementConnection.CimDCOM.cs make it obvious what each file contains without needing to open it.
• dbatools.library.psm1 refactor. Extracting the CoreRedirector C# source from a 200-line here-string into a proper .cs file dramatically improves maintainability and makes the code editable with full IDE support.
• Documentation split. Breaking the monolithic README.md and MIGRATING-FROM-LUMENWORKS.md into focused files (USAGE.md, SCHEMA-AND-TYPES.md, MIGRATING-FROM-LUMENWORKS-EXAMPLE.md, MIGRATING-FROM-LUMENWORKS-FEATURES.md) improves discoverability.
• Test organisation. Grouping tests by category (DateTimeConversion, ThreadSafety, ParallelProcessing, etc.) as partial-class files makes large test suites much easier to navigate.
• stop-file-length.sh hook quality. The hook itself is well-written: it uses git ls-files (no untracked noise), skips binaries via a regex guard, and uses grep -qIl to detect text files correctly. The set -eu discipline matches the rest of the hooks in this repo.

---

Summary

Severity	Issue
Bug	.codex/settings.json references .claude/hooks/… instead of .codex/hooks/… — hook never runs
Minor	CoreRedirector.Register() has a TOCTOU race on _registered
Style	String interpolation ($"…") used in new test files, contrary to project convention
Info	.github/workflows/test-avoidconflicts.yml deletion is unexplained
✅	Partial-class refactoring is mechanically correct and well-organised
✅	PSM1 → external .cs file extraction is a clear improvement

[claude]

Code Review: PR #54 — Refactor files under 400-line limit

Overview

This PR applies a systematic refactoring across the library: large monolithic C# files (some were >4000 lines) are split into focused partial-class files using the ClassName.Category.cs naming convention. It also adds a new enforcement hook, extracts CoreRedirector from the psm1, adds extensive new tests, and splits documentation into topic-focused files.

---

What's done well

Structural refactoring:

• The partial class pattern is used correctly throughout. Each split file repeats the namespace + partial declaration with no duplication of member definitions.
• Member fields stay in the hub file (CsvDataReader.cs, CsvDataReader.Members.cs), methods move to semantically named files — this is a readable, navigable structure.
• All new files are appropriately named (CsvDataReader.ParallelPipeline.cs, ManagementConnection.CimDCom.cs, etc.) — the category suffix reads naturally.

Test coverage:

• New test files cover meaningful scenarios: thread-safety, parallel processing, date/time conversions with custom formats, buffer boundary conditions, stress tests, and malformed data. This is a significant coverage improvement.
• Tests correctly use String.Format instead of interpolation, matching the project's LangVersion 7.3 rule.
• Splitting the monolithic CsvDataReaderTest.cs (previously 2354 lines) into a partial-class family is exactly the right call.

Hook:

• stop-file-length.sh uses git ls-files (correct for tracking staged/committed files), has good binary exclusions, handles Windows path separators, and outputs a clear violation list.
• The set -eu without pipefail matches the documented project convention.

CoreRedirector extraction:

• Moving the assembly redirector from embedded C# in the psm1 to a standalone dbatools.library.CoreRedirector.cs is significantly cleaner. The file is well-structured and the Interlocked.CompareExchange for single-registration is correct.

---

Issues and suggestions

1. .codex/settings.json is in the wrong directory for Claude Code hooks

The settings.json uses Claude Code's hook format (hooks.Stop) and references a Claude-specific env var (CLAUDE_PROJECT_DIR), but it lives in .codex/ instead of .claude/. If the intent is for this hook to run in Claude Code sessions, it needs to be at .claude/settings.json (or merged into the existing settings file). As-is, Claude Code won't pick it up from .codex/.

If the intent is dual-use (Claude Code + OpenAI Codex CLI), the formats differ between the two systems and this needs separate configs.

2. Hook only catches tracked/staged files — misses brand-new unstaged files

git ls-files lists files in the index (staged + committed), so a file created but not yet git added won't be checked. This is a minor gap — the hook catches violations at the earliest reasonable moment (after staging), but worth documenting.

3. Thread-safety test uses Thread.Sleep as synchronization

In CsvDataReaderTest.ThreadSafety.cs, TestParallelProcessing_DisposeDuringRead calls Thread.Sleep(10) to race the dispose against the reader loop. This is inherently non-deterministic and can become a flaky test under load. Consider using a ManualResetEventSlim to signal that the reader has actually started iterating before disposing.

4. Empty XML doc summaries on DbaDateTimeBase constructors

Several constructors have /// <summary> with empty bodies (lines 148-295 of DbaDateTimeBase.cs). These are pre-existing, but since the file was touched in this PR it would be worth filling them in — or removing the doc comment if the constructor signature is self-documenting.

5. Minor: double blank lines in CsvDataReader.cs

Lines around 130-131, 145-146, and 191-193 have double blank lines between member groups. Single blank lines are the C# convention.

6. SetDbatoolsConfigCommand and WriteMessageCommand still inherit from PSCmdlet

Both are listed as legacy exemptions in CLAUDE.md so this is expected — flagging for completeness since neither was migrated to DbaBaseCmdlet in this refactor. That is a separate track of work.

---

Specific nits

• ManagementConnection.cs line 91 has a trailing space after the assignment.
• CsvDataReaderTest.DateTimeConversion.cs is 364 lines — under the limit, but the closest to it in the test suite. Keep an eye as new date/time test cases are added.
• CsvSchemaInference.cs is 349 lines — same note.

---

Summary

This is a well-executed mechanical refactor. The partial class approach is exactly right for LangVersion 7.3, the naming is consistent, and the new test coverage is a genuine improvement. The main actionable issue is the .codex/settings.json placement — if this hook is meant for Claude Code, it should live under .claude/. Everything else is minor polish.

[claude]

Follow-up Review — PR #54 (after fix commits)

This reviews the two fix commits pushed after the earlier reviews: "fix: address refactor review feedback" and "fix: compile core redirector in module loader".

---

Issues Resolved ✅

• Hook path corrected: .codex/settings.json now correctly references .codex/hooks/stop-file-length.sh. The wrong-path bug from the first review is gone.
• Race condition fixed: CoreRedirector.Register() now uses lock (_syncRoot) — clean and idiomatic for C# 7.3.
• String interpolation: CsvDataReaderTest.ThreadSafety.cs and all other new test files use String.Format throughout — no $\"...\" found.
• CoreRedirector extracted properly: dbatools.library.psm1 now reads dbatools.library.CoreRedirector.cs from disk and compiles it via Add-Type with the correct assembly references (System.Runtime.Loader, System.Runtime.InteropServices, etc.). Clean.

---

Remaining Issue — Hook Won't Run in Claude Code

.codex/settings.json uses Claude Code's hook format (hooks.Stop) and the Claude Code environment variable CLAUDE_PROJECT_DIR, but Claude Code reads settings from .claude/settings.json — not .codex/settings.json. There is no .claude/ directory in the repo, so this hook will never fire in Claude Code sessions.

If the intent is for Claude Code to enforce the 400-line limit, the entry needs to be in .claude/settings.json:

{
  \"hooks\": {
    \"Stop\": [{
      \"hooks\": [{
        \"type\": \"command\",
        \"command\": \"\\\"${CLAUDE_PROJECT_DIR:-.}\\\"/.codex/hooks/stop-file-length.sh\",
        \"timeout\": 15,
        \"statusMessage\": \"Checking file lengths (400-line limit)...\"
      }]
    }]
  }
}

If the .codex/settings.json is intentionally for the OpenAI Codex CLI instead, the CLAUDE_PROJECT_DIR variable won't be set in that environment and the hook path lookup will fall back to . (working directory). That may or may not work depending on where Codex CLI runs from — worth verifying. In any case, the intent should be documented with a comment or README entry.

---

Minor: Workflow path filter gap

test-avoidconflicts.yml triggers on changes to dbatools.library.psm1, but the psm1 now loads dbatools.library.CoreRedirector.cs at runtime. A change to CoreRedirector.cs alone would silently skip the AvoidConflicts tests. Add the new file to the path filter:

paths:
  - 'dbatools.library.psm1'
  - 'dbatools.library.CoreRedirector.cs'   # ← add this
  - 'dbatools.library.psd1'
  - 'tests/test-avoidconflicts.ps1'
  - '.github/workflows/test-avoidconflicts.yml'

---

Everything Else Looks Good

The partial-class refactoring is mechanically correct throughout. The CoreRedirector extraction is a genuine improvement. The test reorganisation and new test coverage are solid. The benchmark project's LangVersion>latest is appropriate — the 7.3 constraint applies only to the main library. The stop-file-length.sh script itself is well-written. The two actionable items above are both small and targeted.

[claude]

Code Review: Refactor files under 400-line limit

Overview

This PR mechanically splits large C# source files (some originally 4000+ lines) into partial-class files to stay under a new 400-line limit, extracts the PowerShell Core assembly resolver into a standalone .cs file, and splits large test and documentation files. Net additions: 13,193 lines; deletions: 12,493 lines — almost entirely rearrangement with no logic changes.

---

Critical Issue: Hook is Missing for Claude Code

The hook that enforces the 400-line limit is deployed under .codex/hooks/stop-file-length.sh and .codex/settings.json, but the corresponding .claude/ versions are deleted (the git status shows D .claude/hooks/stop-file-length.sh and D .claude/settings.json).

CLAUDE.md explicitly describes hooks under .claude/ as the enforcement mechanism for project rules. Without .claude/hooks/stop-file-length.sh and .claude/settings.json, the 400-line limit cannot be enforced during Claude Code agent sessions — which is the primary use case for this project. This defeats the stated purpose of the PR.

The hook itself is well-written (uses set -eu, skips binary/build output files) — it just needs to be restored to .claude/ as well as existing in .codex/.

Additional hook note: The skip_prefixes pattern exempts .claude/ from the length check. This means files added under .claude/ in the future would silently bypass the limit — worth considering whether that's intentional.

---

Partial Class Splits — Generally Good

Class	Original	Resulting Files	Quality
CsvDataReader	4026 lines	16 files	Good — logical by concern
ManagementConnection	786 lines	5 files	Good — split by protocol
DbaDateTimeBase	590 lines	4 files	Good — split by category
DbaInstanceParameter	452 lines	4 files	Acceptable
WriteMessageCommand	488 lines	3 files	Good — fields/helpers/lifecycle
SetDbatoolsConfigCommand	171 lines	2 files	Fine

The [TestClass] attribute is correctly retained on the root partial file for all test class splits — MSTest applies the attribute to the full partial class, so this is correct.

---

Files Approaching the Limit (no action needed, just awareness)

Several files land in the 360–387 line range — these are under the limit but have no headroom:

• DbaTimeSpan.cs — 387 lines
• CsvWriter.cs — 386 lines
• CsvDataReader.DirectParsingHelpers.cs — 381 lines
• ManagementConnection.cs (main) — 373 lines

---

Specific Code Issues

1. DbaInstanceParameter.Formatting.cs is only 25 lines. At this size it adds file-system noise without meaningful organization benefit. The content would fit cleanly in the main file.

2. ManagementConnection.cs (main, 373 lines) still includes the DisabledConnectionTypes property body. This would fit better in .ConnectionTypes.cs for thematic consistency, and would also reduce the main file's size.

3. test-resolver-stress.ps1 dot-sources test-resolver-stress.continuation.ps1. Variables set in the parent script (e.g., $testResults, $test4Result) are referenced in the continuation file, so the continuation cannot be run independently. This is a minor usability concern.

4. Pre-existing: empty <summary> tags in DbaDateTimeBase.Methods.cs. Not introduced by this PR, but now spread across more files and more visible. Worth cleaning up.

---

CoreRedirector.cs — Substantive Change, Looks Correct

Extracting the PowerShell Core assembly resolver from an inline here-string in psm1 into a proper .cs file is a good improvement. The logic is correct:

• Checks already-loaded assemblies before probing the filesystem
• Handles both managed (AssemblyLoadContext.Default.Resolving) and unmanaged (ResolvingUnmanagedDll) events
• Tries platform-specific RID paths before the root lib folder
• Handles common native extension variants (.dll, .dylib, .so, lib prefix)

Minor inconsistency: The older Redirector class for Windows PowerShell (net472) remains as an inline here-string in psm1. A follow-up PR should extract it to a .cs file as well for consistency.

No unit tests for CoreRedirector.cs: The new class is only tested indirectly via the stress test script. The OnResolving, GetManagedAssemblyPaths, and GetNativeLibraryNames methods have no isolated unit tests.

---

C# Language Version Compliance

No C# 8+ features in any new or modified .cs files. No string interpolation, nullable reference types, switch expressions, using declarations, or ??=. The out var pattern used in WriteMessageCommand.Processing.cs is C# 7.0 (allowed at 7.3). No Assembly.LoadFile() or ThrowTerminatingError calls. Full compliance confirmed.

---

Summary

The refactoring is mechanically sound and the partial class splits are logically organized. C# 7.3 compliance is maintained throughout.

The blocking issue is the missing .claude/hooks/ files — restoring .claude/hooks/stop-file-length.sh and .claude/settings.json is required to actually enforce the rule this PR introduces. Everything else is advisory.