feat(dotnet): port paths, environment, and config loading (slice 2) - #4
Conversation
Implements vertical slice 2 of the .NET port: NM_HOME/app directory
layout and configuration loading.
NoMistakes.Core:
- Paths: resolves NM_HOME or ~/.no-mistakes and derives the DB, socket,
PID, config, repos, worktrees, logs, and server-PID locations;
EnsureDirs creates them.
NoMistakes.Config (new project):
- GoDuration: parses Go's time.ParseDuration format so ci_timeout values
("168h", "2h30m", "-5m", keywords) stay wire-compatible.
- ConfigLoader.LoadGlobal: strict global parsing (unknown top-level
fields error, mirroring yaml.v3 KnownFields(true), so
allow_repo_commands is rejected globally), scalar-or-list agent,
legacy babysit aliases, and agent_args_override validation.
- LoadRepo/LoadRepoFromBytes: lenient repo parsing.
- EffectiveRepoConfig: enforces the trust boundary so code-executing
fields (commands, agent) come only from the trusted default-branch
copy unless allow_repo_commands opts in.
- Merge + AutoFixLimit: global+repo layering with defaults (review
auto-fix disabled by default).
Tests port the Go paths and config suites: default paths, NM_HOME,
config defaults, repo parsing, effective-config merging, the security
trust boundary, ci_timeout parsing, and a default-template compatibility
check.
Deferred: native-agent PATH resolution (ResolveAgent) lands with the
native-agent slice.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VGneFMSR4ZXgoAjrN3yeQp
|
Tip ✅ Code Review #1 Complete |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
WalkthroughAdds a new NoMistakes.Config project (with NoMistakes.Core.Paths) implementing filesystem layout resolution via NM_HOME, Go-compatible duration parsing, strict YAML global/repo config loading, a repo trust boundary enforcement mechanism, and config merging, wired into the solution and covered by extensive unit tests and documentation updates. ChangesPaths and Config Loading
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ConfigLoader
participant GlobalConfig
participant RepoConfig
participant Config
Caller->>ConfigLoader: LoadGlobal(path)
ConfigLoader-->>Caller: GlobalConfig
Caller->>ConfigLoader: LoadRepo(dir) / LoadRepoFromBytes
ConfigLoader-->>Caller: RepoConfig
Caller->>ConfigLoader: EffectiveRepoConfig(pushed, trusted, allowRepoCommands)
ConfigLoader-->>Caller: RepoConfig (trust-enforced)
Caller->>ConfigLoader: Merge(GlobalConfig, effectiveRepoConfig)
ConfigLoader-->>Caller: Config
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (6)
dotnet/src/NoMistakes.Config/ConfigTypes.cs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialRouting automated review feedback for this file to
@tinycode-labs.As per path instructions, "for code review feedback by coderabbitai then inform "
@tinycode-labs" per file, not per section of file."🤖 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 `@dotnet/src/NoMistakes.Config/ConfigTypes.cs` at line 1, The review feedback routing for this file should be handled by `@tinycode-labs` at the file level, not per section. Update the relevant review-handling logic around ConfigTypes / file-level routing so automated coderabbitai feedback for this file is forwarded to `@tinycode-labs` consistently.Source: Path instructions
dotnet/src/NoMistakes.Config/Names.cs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialRouting automated review feedback for this file to
@tinycode-labs.As per path instructions, "for code review feedback by coderabbitai then inform "
@tinycode-labs" per file, not per section of file."🤖 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 `@dotnet/src/NoMistakes.Config/Names.cs` at line 1, The review routing for this file should be updated so automated feedback is sent to `@tinycode-labs` at the file level, not per section. Adjust the handling around Names.cs so the reviewer assignment is applied once for the entire file, matching the path instruction and avoiding section-specific routing.Source: Path instructions
dotnet/src/NoMistakes.Config/Config.cs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialRouting automated review feedback for this file to
@tinycode-labs.As per path instructions, "for code review feedback by coderabbitai then inform "
@tinycode-labs" per file, not per section of file."🤖 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 `@dotnet/src/NoMistakes.Config/Config.cs` at line 1, The review feedback routing for this file should be set at the file level, not per section, so update the automation/config handling around Config.cs to inform `@tinycode-labs` for any coderabbitai feedback on this file. Use the file-level path instructions logic in the Config.cs-related routing path to ensure the mention is applied once for the whole file.Source: Path instructions
dotnet/src/NoMistakes.Config/ConfigLoader.cs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialRouting automated review feedback for this file to
@tinycode-labs.As per path instructions, "for code review feedback by coderabbitai then inform "
@tinycode-labs" per file, not per section of file."🤖 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 `@dotnet/src/NoMistakes.Config/ConfigLoader.cs` at line 1, Update the file-level review routing for NoMistakes.Config/ConfigLoader.cs so automated feedback is assigned to `@tinycode-labs` once per file, not per section; adjust the review-handling logic around ConfigLoader.cs to emit a single file-level mention for coderabbitai-originated feedback and avoid repeating it for individual hunks.Source: Path instructions
dotnet/tests/NoMistakes.Tests/ConfigTests.cs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 TrivialRouting automated review feedback for this file to
@tinycode-labs.As per path instructions, "for code review feedback by coderabbitai then inform "
@tinycode-labs" per file, not per section of file."🤖 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 `@dotnet/tests/NoMistakes.Tests/ConfigTests.cs` at line 1, The review routing in this test file should be applied at the file level, not per section, so update the annotation/comment handling to inform `@tinycode-labs` once for the whole file. Adjust the relevant test/config review routing around NoMistakes.Tests.ConfigTests so the file-level instruction matches the path guidance and avoids section-specific tagging.Source: Path instructions
dotnet/src/NoMistakes.Config/NoMistakes.Config.csproj (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider updating YamlDotNet.
Version 13.7.1 is valid but several major versions behind current (18.1.0). Not blocking, but worth tracking for future upgrade given this is a brand-new dependency.
🤖 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 `@dotnet/src/NoMistakes.Config/NoMistakes.Config.csproj` at line 3, The YamlDotNet dependency is pinned to an older major version and should be reviewed for an upgrade. Update the PackageReference for YamlDotNet in the NoMistakes.Config project to the current targeted version, then verify the config code still builds and behaves correctly with the newer package.
🤖 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 `@dotnet/src/NoMistakes.Config/ConfigLoader.cs`:
- Line 190: The current ValidateKnownFields call in ConfigLoader only validates
the top-level global config map, so unknown nested keys under auto_fix, intent,
and test.evidence can still slip through. Update ConfigLoader to also validate
each nested map using the corresponding known-field sets for those sections, and
reject any extra keys before the config is accepted. Use the existing validation
flow around ValidateKnownFields, KnownGlobalFields, and the nested parsing logic
in ConfigLoader to wire this in.
---
Nitpick comments:
In `@dotnet/src/NoMistakes.Config/Config.cs`:
- Line 1: The review feedback routing for this file should be set at the file
level, not per section, so update the automation/config handling around
Config.cs to inform `@tinycode-labs` for any coderabbitai feedback on this file.
Use the file-level path instructions logic in the Config.cs-related routing path
to ensure the mention is applied once for the whole file.
In `@dotnet/src/NoMistakes.Config/ConfigLoader.cs`:
- Line 1: Update the file-level review routing for
NoMistakes.Config/ConfigLoader.cs so automated feedback is assigned to
`@tinycode-labs` once per file, not per section; adjust the review-handling logic
around ConfigLoader.cs to emit a single file-level mention for
coderabbitai-originated feedback and avoid repeating it for individual hunks.
In `@dotnet/src/NoMistakes.Config/ConfigTypes.cs`:
- Line 1: The review feedback routing for this file should be handled by
`@tinycode-labs` at the file level, not per section. Update the relevant
review-handling logic around ConfigTypes / file-level routing so automated
coderabbitai feedback for this file is forwarded to `@tinycode-labs` consistently.
In `@dotnet/src/NoMistakes.Config/Names.cs`:
- Line 1: The review routing for this file should be updated so automated
feedback is sent to `@tinycode-labs` at the file level, not per section. Adjust
the handling around Names.cs so the reviewer assignment is applied once for the
entire file, matching the path instruction and avoiding section-specific
routing.
In `@dotnet/src/NoMistakes.Config/NoMistakes.Config.csproj`:
- Line 3: The YamlDotNet dependency is pinned to an older major version and
should be reviewed for an upgrade. Update the PackageReference for YamlDotNet in
the NoMistakes.Config project to the current targeted version, then verify the
config code still builds and behaves correctly with the newer package.
In `@dotnet/tests/NoMistakes.Tests/ConfigTests.cs`:
- Line 1: The review routing in this test file should be applied at the file
level, not per section, so update the annotation/comment handling to inform
`@tinycode-labs` once for the whole file. Adjust the relevant test/config review
routing around NoMistakes.Tests.ConfigTests so the file-level instruction
matches the path guidance and avoids section-specific tagging.
🪄 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
Run ID: 9d1124f0-fe29-4f93-aa49-cb461ee1329c
📒 Files selected for processing (15)
VERTICAL_SLICES.mddotnet/README.mddotnet/no-mistakes.slndotnet/src/NoMistakes.Config/Config.csdotnet/src/NoMistakes.Config/ConfigLoader.csdotnet/src/NoMistakes.Config/ConfigTypes.csdotnet/src/NoMistakes.Config/GoDuration.csdotnet/src/NoMistakes.Config/Names.csdotnet/src/NoMistakes.Config/NoMistakes.Config.csprojdotnet/src/NoMistakes.Core/Paths.csdotnet/tests/NoMistakes.Tests/ConfigTests.csdotnet/tests/NoMistakes.Tests/GoDurationTests.csdotnet/tests/NoMistakes.Tests/NoMistakes.Tests.csprojdotnet/tests/NoMistakes.Tests/PathsTests.csdotnet/tests/NoMistakes.Tests/TempDir.cs
| return cfg; | ||
| } | ||
|
|
||
| ValidateKnownFields(map, KnownGlobalFields, "global config", "globalConfigRaw"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does go-yaml v3 KnownFields(true) apply recursively to nested struct fields during Decode?
💡 Result:
Yes, in go-yaml v3, KnownFields(true) applies recursively to nested structs during decoding [1]. When the Decoder has KnownFields(true) enabled, the underlying decoder state is passed down through the recursive calls of the unmarshal process [1]. If the decoder encounters a key in a YAML mapping that does not correspond to a field in the struct currently being unmarshaled, it will trigger an error [1][2]. This behavior persists as the decoder descends into nested struct fields, ensuring that unknown keys at any level of the struct hierarchy result in an error [1]. However, there are important limitations and nuances to be aware of: 1. Custom Unmarshalers: If a nested struct implements the Unmarshaler interface (e.g., UnmarshalYAML), the KnownFields setting may not be automatically tracked or enforced within that custom unmarshaling logic unless it is explicitly handled [3][4]. 2. Decoder Inheritance: While the flag applies recursively during standard struct decoding, issues have been reported where complex custom unmarshaling paths or certain node-based decoding operations do not inherit the parent decoder's settings by default [4]. In summary, for standard nested struct hierarchies, KnownFields(true) effectively enforces strict schema validation recursively [1].
Citations:
- 1: https://github.com/go-yaml/yaml/blob/v3/decode.go
- 2: https://tessl.io/registry/tessl/golang-gopkg-in--yaml-v3/3.0.1/files/docs/streaming-operations.md
- 3: v3: KnownFields aren't tracked with Unmarshalers go-yaml/yaml#642
- 4: Fix: option KnownFields not being respected inside custom UnmarshalYAML() yaml/go-yaml#332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant methods and surrounding context.
FILE="dotnet/src/NoMistakes.Config/ConfigLoader.cs"
echo "== File size =="
wc -l "$FILE"
echo
echo "== AST outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== Relevant line ranges =="
sed -n '150,240p' "$FILE"
echo
sed -n '340,520p' "$FILE"
echo
sed -n '560,720p' "$FILE"
echo
echo "== Search for AllowRepoCommands usages =="
rg -n "AllowRepoCommands|EffectiveRepoConfig|ShallowClone|ValidateKnownFields|ParseAutoFixRaw|ParseIntentRaw|ParseTestRaw" dotnet/src -SRepository: leonj1/no-mistakes
Length of output: 19160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the config type definition and any YAML decoding/custom unmarshaling that could affect KnownFields behavior.
rg -n "UnmarshalYAML|KnownFields|auto_fix|intent|test|evidence|AllowRepoCommands" dotnet/src/NoMistakes.Config -S
echo
echo "== ConfigTypes excerpts =="
FILE2="dotnet/src/NoMistakes.Config/ConfigTypes.cs"
wc -l "$FILE2"
sed -n '1,220p' "$FILE2"Repository: leonj1/no-mistakes
Length of output: 8899
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every usage of AllowRepoCommands and EffectiveRepoConfig across the repo.
rg -n "AllowRepoCommands|EffectiveRepoConfig\(" . -S
echo
echo "== Call sites of EffectiveRepoConfig =="
rg -n "EffectiveRepoConfig\(" . -S -A 2 -B 2Repository: leonj1/no-mistakes
Length of output: 12954
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="dotnet/tests/NoMistakes.Tests/ConfigTests.cs"
echo "== Relevant tests around global/repo parsing and EffectiveRepoConfig =="
sed -n '120,230p' "$FILE"
echo
sed -n '280,360p' "$FILE"
echo
sed -n '500,620p' "$FILE"Repository: leonj1/no-mistakes
Length of output: 10173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "unknown|field .* not found|auto_fix|intent|test|evidence|AllowRepoCommands" dotnet/tests/NoMistakes.Tests/ConfigTests.cs -SRepository: leonj1/no-mistakes
Length of output: 1341
@tinycode-labs Validate nested config keys
ValidateKnownFields only covers the top-level map. Unknown keys inside auto_fix, intent, or test.evidence are still ignored, so a typo like thresholdd or tset silently drops the setting. Consider rejecting extra keys in those nested maps too.
🤖 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 `@dotnet/src/NoMistakes.Config/ConfigLoader.cs` at line 190, The current
ValidateKnownFields call in ConfigLoader only validates the top-level global
config map, so unknown nested keys under auto_fix, intent, and test.evidence can
still slip through. Update ConfigLoader to also validate each nested map using
the corresponding known-field sets for those sections, and reject any extra keys
before the config is accepted. Use the existing validation flow around
ValidateKnownFields, KnownGlobalFields, and the nested parsing logic in
ConfigLoader to wire this in.
2 participants
Summary
Implements vertical slice 2 of the .NET port (
VERTICAL_SLICES.md):NM_HOME/app directory layout and configuration loading, ported from Go'sinternal/pathsandinternal/config.Changes
NoMistakes.CorePaths.cs— resolvesNM_HOMEor~/.no-mistakesand derives the DB, socket, PID, config, repos, worktrees, logs, and server-PID locations;EnsureDirscreates them.NoMistakes.Config(new project, uses YamlDotNet)GoDuration.cs— parses Go'stime.ParseDurationformat soci_timeoutvalues (168h,2h30m,-5m, keywords) stay wire-compatible with the Go implementation.ConfigLoader.csLoadGlobal— strict global parsing (unknown top-level fields error, mirroring yaml.v3KnownFields(true), soallow_repo_commandsis rejected in the global config), scalar-or-listagent, legacybabysit_timeout/auto_fix.babysitaliases, andagent_args_overridevalidation.LoadRepo/LoadRepoFromBytes— lenient per-repo parsing.EffectiveRepoConfig— enforces the trust boundary: code-executing fields (commands,agent) come only from the trusted default-branch copy unlessallow_repo_commandsopts in; forced empty otherwise.Merge+AutoFixLimit— global+repo layering with defaults (review auto-fix disabled by default).Tests (
NoMistakes.Tests) port the Go paths and config suites, covering all four slice-2 acceptance checks:NM_HOME, config defaults, repo parsing, effective-config merging;ci_timeoutparsing (finite / unlimited keywords / non-positive);Solution and test project references wired up;
VERTICAL_SLICES.mdanddotnet/README.mdupdated.Verification
dotnet build/dotnet testin the authoring environment — the .NET SDK download host is blocked by the network policy and no SDK is preinstalled. NuGet is reachable, so CI (Dockerfile.dotnet, which restores from themcr.microsoft.combase + nuget.org) should build normally.To compensate, verified without a compiler:
auto, ci_timeout168h, auto-fix3/3/0/3/3/3).Please rely on CI's
dotnet test dotnet/no-mistakes.slnfor the authoritative green check.Notes
ResolveAgent) is intentionally deferred to the native-agent slice (14); it needs the process-launch layer.🤖 Generated with Claude Code
https://claude.ai/code/session_01VGneFMSR4ZXgoAjrN3yeQp
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests