Port slice 7: daemon IPC and run lifecycle to .NET - #9
Conversation
|
Warning Warning! 1 of 11 rules had issues (11 of 31 ran) in 1m 12s. Found 32 issues that should be addressed. cc @devboxerhub Please review the inline comments below. Rule outcomes: 9 passed, 2 violated, 20 skipped 📋 Rule Results (click to expand)
|
WalkthroughThis PR adds .NET daemon and IPC projects, implements Unix-socket JSON-RPC transport and run orchestration, extends AXI query/drive/respond/abort CLI flows, and updates related core models, solution wiring, tests, and porting notes. ChangesDaemon/IPC/CLI Port
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Rule-Based Code ReviewFound 31 issue(s) that need attention:
AI-Powered Code ReviewReviewed 46 code sections. This review was performed automatically using custom rules and AI analysis. Please review the feedback and use your judgment. 31 finding(s)These could not be attached to specific lines in the diff, so they are summarized here:
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Review Process.Start inputs for command injection; prefer explicit executable and arguments. - [ai-reviewer-dotnet-process-start] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
dotnet/src/NoMistakes.Daemon/DaemonHost.cs (1)
125-139: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRecovery failures are swallowed with no actual logging.
The comment says "Go logs 'failed to recover stale runs' and keeps starting," but this
catchblock performs no logging at all — a recovery failure during startup becomes completely invisible to operators.♻️ Suggested fix
try { db.RecoverStaleRuns(CrashRecoveryError); } - catch (Exception) + catch (Exception ex) { - // Go logs "failed to recover stale runs" and keeps starting. + Console.Error.WriteLine($"failed to recover stale runs: {ex}"); }🤖 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.Daemon/DaemonHost.cs` around lines 125 - 139, The startup recovery path in RecoverOnStartup currently swallows all exceptions from db.RecoverStaleRuns(CrashRecoveryError) without any log output, making failures invisible. Update the catch block in DaemonHost.RecoverOnStartup to log a clear warning/error when recovery fails, including the exception details, while still allowing startup to continue as intended.dotnet/src/NoMistakes.Ipc/WireTypes.cs (1)
108-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing
FixReviewconstant despiteDifffield referencing it.
Diff's doc comment says "Unified diff for fix_review events." butEventTypeshas noFixReviewconstant — callers wiring this field will have to hardcode the string"fix_review", losing the type-safety this class exists to provide.♻️ Proposed fix
public static class EventTypes { public const string RunCreated = "run_created"; public const string RunUpdated = "run_updated"; public const string RunCompleted = "run_completed"; public const string StepStarted = "step_started"; public const string StepCompleted = "step_completed"; public const string LogChunk = "log_chunk"; + public const string FixReview = "fix_review"; }Also applies to: 150-155
🤖 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.Ipc/WireTypes.cs` around lines 108 - 116, EventTypes is missing the FixReview event name even though Diff is documented for fix_review events, so add a FixReview constant alongside the other event strings in EventTypes and use it wherever the Diff field is wired so callers do not need to hardcode "fix_review". Keep the naming and placement consistent with RunCreated, RunUpdated, and LogChunk to preserve the existing type-safe event mapping.dotnet/src/NoMistakes.Daemon/RunManager.cs (1)
235-265: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
CancelActiveRunsAsyncscans every historical run for the repo on each push.
db.GetRunsByRepo(repoId)(Line 238) is called on everyStartRunAsyncand appears to return the repo's full run history, filtered in-memory by branch/status. For repos with a long push history this becomes an ever-growing hot-path scan on every push. Consider a DB-level query scoped to active (Pending/Running) runs for the repo+branch instead of fetching all rows and filtering client-side.🤖 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.Daemon/RunManager.cs` around lines 235 - 265, CancelActiveRunsAsync is doing an in-memory scan over the repo’s full run history via db.GetRunsByRepo(repoId), which makes StartRunAsync increasingly expensive on long-lived repos. Update RunManager.CancelActiveRunsAsync to query only active runs for the given repoId and branch at the DB level, scoped to Pending/Running statuses, instead of filtering all rows client-side. Keep the existing cancellation flow with TryCancel, but feed it a much smaller result set from the repository/query layer.dotnet/tests/NoMistakes.Tests/AxiAbortByRunIdTests.cs (1)
120-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate daemon test-harness helper across test files.
StartDaemonAsync/DaemonHandlehere are duplicated verbatim inDaemonNotifyPushTests.cs(Lines 361-380). Consider extracting to a shared test-support helper (e.g., alongsideDataTestSupport) to avoid drift between the two copies.cc
@tinycode-labsAs per path instructions, informing "
@tinycode-labs" per file per review feedback.🤖 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/AxiAbortByRunIdTests.cs` around lines 120 - 139, The StartDaemonAsync and DaemonHandle test-harness helpers are duplicated in this test class and DaemonNotifyPushTests, so move them into a shared test-support utility near DataTestSupport and update both tests to use that single implementation. Keep the behavior of StartDaemonAsync, RunIpcHandlers.Register, and DaemonHandle.DisposeAsync the same, but remove the local copy here so the helper cannot drift between files.dotnet/src/NoMistakes.Daemon/RunIpcHandlers.cs (1)
36-45: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueN+1 query pattern in
GetRuns.Each run triggers a separate
db.GetStepsByRun(run.Id)call inside the loop. For repos with many historical runs this is an O(n) round-trip pattern; consider a batch-fetch API onDatabasekeyed by repo/run-id set if this becomes a hot path.🤖 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.Daemon/RunIpcHandlers.cs` around lines 36 - 45, The GetRuns handler in RunIpcHandlers is doing an N+1 query by calling db.GetStepsByRun(run.Id) inside the loop over db.GetRunsByRepo(p.RepoId). Update the handler to fetch steps in bulk for all returned runs, ideally by adding a batch API on Database keyed by the run IDs or repo scope, and then pass the preloaded step data into RunInfoMapper.RunToInfo instead of querying per run.dotnet/src/NoMistakes.Daemon/RunInfoMapper.cs (1)
70-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad exception swallowing hides real failures.
Both enrichment reads catch bare
Exceptionand discard it entirely, matching Go's ignored-error pattern, but silently losing signal for programming bugs (e.g., NullReferenceException) alongside expected decorative-read failures. Consider at least logging the exception before falling back to defaults.🤖 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.Daemon/RunInfoMapper.cs` around lines 70 - 91, The two best-effort enrichment reads in RunInfoMapper currently swallow all exceptions, which hides real failures. Update the try/catch blocks around StepFindingStats and StepFixSummaries to keep the fallback behavior but log the caught exception before leaving the default values in place, using the existing RunInfoMapper context so issues are visible without changing the enrichment flow.dotnet/src/NoMistakes.Cli/AxiAbort.cs (1)
41-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAvoid matching cancel errors by message text.
dotnet/src/NoMistakes.Cli/AxiAbort.cs:41-46—ex.Message.Contains("no active run")couples the client to daemon wording; add a dedicated IPC error code for inactive/unknown run IDs and branch on that instead.@tinycode-labs🤖 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.Cli/AxiAbort.cs` around lines 41 - 46, The AxiAbort exception handling currently relies on matching IpcRpcException.Message text for “no active run,” which is brittle. Update AxiAbort to branch on a dedicated IPC error code for inactive/unknown run IDs instead of using ex.Message.Contains, and adjust the daemon/client contract so the abort path can detect that condition via a stable symbol on IpcRpcException or its payload. Keep the existing no-op behavior in the AxiAbort catch flow, but only after checking the new error code rather than message wording.
🤖 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.Cli/AxiAbort.cs`:
- Around line 30-46: The abort flow in AxiAbort has a TOCTOU gap between
DaemonStatus.IsRunningAsync and the IPC work, so a daemon exit after the status
check can make IpcClient.DialAsync or CallAsync throw outside the current
IpcRpcException handler. Update AxiAbort so connection/setup failures from
IpcClient.DialAsync and the cancel call are treated like the existing “daemon
not running”/“no active run” no-op cases, keeping the abort operation idempotent
even if the daemon disappears mid-request.
In `@dotnet/src/NoMistakes.Cli/DaemonNotifyPush.cs`:
- Around line 41-49: The IOException wrapping in DaemonNotifyPush’s connect path
drops the original exception details, so update the catch block around
IpcClient.DialAsync to rethrow a new IOException that preserves ex as the inner
exception. Keep the existing context message, but include the caught exception
when constructing the new IOException so the original stack trace and
diagnostics are retained.
In `@dotnet/src/NoMistakes.Daemon/RunManager.cs`:
- Around line 316-326: `TrySetRunError` in `RunManager` should mirror the
terminal-status protection used by `MarkCancelled` so it does not overwrite runs
that have already reached a final state. Update the `ExecuteRunAsync` catch-path
helper to first check the current run status (via the same db lookup/state check
pattern used in `MarkCancelled`) and only call `db.UpdateRunErrorStatus(...)`
when the run is still `Pending` or `Running`. Keep the best-effort exception
handling, but ensure `TrySetRunError` skips updates for already
`Completed`/`Failed` runs.
- Around line 63-95: StartRunAsync can still insert and launch a run after
shutdown has begun because shuttingDown is only checked before taking
branchLocks and before the active registration path. Update
RunManager.StartRunAsync to re-check the shutdown state under the same
synchronization used for active registration (the gate lock), and if shutdown is
in progress, avoid calling db.InsertRun or launching ExecuteRunAsync; instead
cancel/mark the would-be run as stopped so Shutdown and run creation are
serialized consistently.
In `@dotnet/src/NoMistakes.Ipc/Client.cs`:
- Around line 64-99: In CallAsync<TResult>, the timeout currently only stops
waiting via WaitAsync while the underlying stream.ReadAsync<Response> keeps
running, which can leave a stale read active after mutex is released. Update the
read path to use a linked CancellationTokenSource with CancelAfter(CallTimeout)
so the in-flight ReadAsync is actually canceled, and when that timeout occurs
treat the connection as broken by throwing an IOException (or equivalent) from
the timeout catch; keep the change localized to Client.CallAsync and its
response-read logic.
In `@dotnet/src/NoMistakes.Ipc/Server.cs`:
- Around line 61-79: The Unix socket permissions are being tightened too late in
ServeAsync, leaving a brief window where the socket can be accessible with the
process umask permissions. Update the ServeAsync flow in Server.cs so the
permission-setting step associated with File.SetUnixFileMode runs immediately
after Bind on the socket path and before ln.Listen(16), keeping the existing
Unix-only guard and the 0700-style mode intact.
---
Nitpick comments:
In `@dotnet/src/NoMistakes.Cli/AxiAbort.cs`:
- Around line 41-46: The AxiAbort exception handling currently relies on
matching IpcRpcException.Message text for “no active run,” which is brittle.
Update AxiAbort to branch on a dedicated IPC error code for inactive/unknown run
IDs instead of using ex.Message.Contains, and adjust the daemon/client contract
so the abort path can detect that condition via a stable symbol on
IpcRpcException or its payload. Keep the existing no-op behavior in the AxiAbort
catch flow, but only after checking the new error code rather than message
wording.
In `@dotnet/src/NoMistakes.Daemon/DaemonHost.cs`:
- Around line 125-139: The startup recovery path in RecoverOnStartup currently
swallows all exceptions from db.RecoverStaleRuns(CrashRecoveryError) without any
log output, making failures invisible. Update the catch block in
DaemonHost.RecoverOnStartup to log a clear warning/error when recovery fails,
including the exception details, while still allowing startup to continue as
intended.
In `@dotnet/src/NoMistakes.Daemon/RunInfoMapper.cs`:
- Around line 70-91: The two best-effort enrichment reads in RunInfoMapper
currently swallow all exceptions, which hides real failures. Update the
try/catch blocks around StepFindingStats and StepFixSummaries to keep the
fallback behavior but log the caught exception before leaving the default values
in place, using the existing RunInfoMapper context so issues are visible without
changing the enrichment flow.
In `@dotnet/src/NoMistakes.Daemon/RunIpcHandlers.cs`:
- Around line 36-45: The GetRuns handler in RunIpcHandlers is doing an N+1 query
by calling db.GetStepsByRun(run.Id) inside the loop over
db.GetRunsByRepo(p.RepoId). Update the handler to fetch steps in bulk for all
returned runs, ideally by adding a batch API on Database keyed by the run IDs or
repo scope, and then pass the preloaded step data into RunInfoMapper.RunToInfo
instead of querying per run.
In `@dotnet/src/NoMistakes.Daemon/RunManager.cs`:
- Around line 235-265: CancelActiveRunsAsync is doing an in-memory scan over the
repo’s full run history via db.GetRunsByRepo(repoId), which makes StartRunAsync
increasingly expensive on long-lived repos. Update
RunManager.CancelActiveRunsAsync to query only active runs for the given repoId
and branch at the DB level, scoped to Pending/Running statuses, instead of
filtering all rows client-side. Keep the existing cancellation flow with
TryCancel, but feed it a much smaller result set from the repository/query
layer.
In `@dotnet/src/NoMistakes.Ipc/WireTypes.cs`:
- Around line 108-116: EventTypes is missing the FixReview event name even
though Diff is documented for fix_review events, so add a FixReview constant
alongside the other event strings in EventTypes and use it wherever the Diff
field is wired so callers do not need to hardcode "fix_review". Keep the naming
and placement consistent with RunCreated, RunUpdated, and LogChunk to preserve
the existing type-safe event mapping.
In `@dotnet/tests/NoMistakes.Tests/AxiAbortByRunIdTests.cs`:
- Around line 120-139: The StartDaemonAsync and DaemonHandle test-harness
helpers are duplicated in this test class and DaemonNotifyPushTests, so move
them into a shared test-support utility near DataTestSupport and update both
tests to use that single implementation. Keep the behavior of StartDaemonAsync,
RunIpcHandlers.Register, and DaemonHandle.DisposeAsync the same, but remove the
local copy here so the helper cannot drift between files.
🪄 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: b4a98d5a-f49e-4e15-82c3-225466e95d3d
⛔ Files ignored due to path filters (10)
logs/iter-0013-20260709-001851.logis excluded by!**/*.loglogs/iter-0014-20260709-002924.logis excluded by!**/*.loglogs/iter-0015-20260709-003021.logis excluded by!**/*.loglogs/iter-0016-20260709-003846.logis excluded by!**/*.loglogs/iter-0017-20260709-003939.logis excluded by!**/*.loglogs/iter-0018-20260709-004825.logis excluded by!**/*.loglogs/iter-0019-20260709-004946.logis excluded by!**/*.loglogs/iter-0020-20260709-005531.logis excluded by!**/*.loglogs/iter-0021-20260709-005643.logis excluded by!**/*.loglogs/iter-0022-20260709-010249.logis excluded by!**/*.log
📒 Files selected for processing (36)
NOTES.mdSTEPS.mdVERTICAL_SLICES.mddotnet/no-mistakes.slndotnet/src/NoMistakes.Cli/AxiAbort.csdotnet/src/NoMistakes.Cli/CliApp.csdotnet/src/NoMistakes.Cli/DaemonNotifyPush.csdotnet/src/NoMistakes.Cli/NoMistakes.Cli.csprojdotnet/src/NoMistakes.Core/Finding.csdotnet/src/NoMistakes.Core/RunCancelReason.csdotnet/src/NoMistakes.Daemon/DaemonHost.csdotnet/src/NoMistakes.Daemon/DaemonPidRecord.csdotnet/src/NoMistakes.Daemon/DaemonStatus.csdotnet/src/NoMistakes.Daemon/NoMistakes.Daemon.csprojdotnet/src/NoMistakes.Daemon/RunInfoMapper.csdotnet/src/NoMistakes.Daemon/RunIpcHandlers.csdotnet/src/NoMistakes.Daemon/RunManager.csdotnet/src/NoMistakes.Ipc/Client.csdotnet/src/NoMistakes.Ipc/IpcJson.csdotnet/src/NoMistakes.Ipc/JsonLineStream.csdotnet/src/NoMistakes.Ipc/Messages.csdotnet/src/NoMistakes.Ipc/NoMistakes.Ipc.csprojdotnet/src/NoMistakes.Ipc/Protocol.csdotnet/src/NoMistakes.Ipc/Server.csdotnet/src/NoMistakes.Ipc/WireTypes.csdotnet/tests/NoMistakes.Tests/AxiAbortByRunIdTests.csdotnet/tests/NoMistakes.Tests/DaemonLifecycleTests.csdotnet/tests/NoMistakes.Tests/DaemonNotifyPushTests.csdotnet/tests/NoMistakes.Tests/DaemonPidRecordTests.csdotnet/tests/NoMistakes.Tests/IpcProtocolTests.csdotnet/tests/NoMistakes.Tests/IpcSocketRoundTripTests.csdotnet/tests/NoMistakes.Tests/NoMistakes.Tests.csprojdotnet/tests/NoMistakes.Tests/RunInfoMapperTests.csdotnet/tests/NoMistakes.Tests/RunIpcHandlerTests.csdotnet/tests/NoMistakes.Tests/RunManagerTests.csdotnet/tests/NoMistakes.Tests/RunTests.cs
| if (!await DaemonStatus.IsRunningAsync(paths, ct).ConfigureAwait(false)) | ||
| { | ||
| return new AxiAbortOutcome(false, runId, "daemon not running, so no active run to cancel (no-op)"); | ||
| } | ||
|
|
||
| using var client = await IpcClient.DialAsync(paths.Socket, ct).ConfigureAwait(false); | ||
| try | ||
| { | ||
| await client.CallAsync<CancelRunResult>(Methods.CancelRun, new CancelRunParams { RunId = runId }, ct) | ||
| .ConfigureAwait(false); | ||
| } | ||
| catch (IpcRpcException ex) when (ex.Message.Contains("no active run")) | ||
| { | ||
| // The daemon reports an unknown/inactive run id as "no active run | ||
| // <id>". Treat that as an idempotent no-op: the run is already gone. | ||
| return new AxiAbortOutcome(false, runId, "no active run with that id (no-op)"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
TOCTOU gap between liveness check and IPC dial/call.
DaemonStatus.IsRunningAsync is checked once, then IpcClient.DialAsync/CallAsync run afterward. If the daemon exits in that window, DialAsync or CallAsync will throw an exception type other than IpcRpcException with "no active run" (e.g., a socket connection failure), which isn't caught here — breaking the documented "no-op when daemon isn't running" contract and instead propagating an unhandled exception.
🔧 Proposed fix: treat connection failures as the same no-op path
- using var client = await IpcClient.DialAsync(paths.Socket, ct).ConfigureAwait(false);
- try
- {
- await client.CallAsync<CancelRunResult>(Methods.CancelRun, new CancelRunParams { RunId = runId }, ct)
- .ConfigureAwait(false);
- }
- catch (IpcRpcException ex) when (ex.Message.Contains("no active run"))
- {
- // The daemon reports an unknown/inactive run id as "no active run
- // <id>". Treat that as an idempotent no-op: the run is already gone.
- return new AxiAbortOutcome(false, runId, "no active run with that id (no-op)");
- }
- return new AxiAbortOutcome(true, runId, null);
+ try
+ {
+ using var client = await IpcClient.DialAsync(paths.Socket, ct).ConfigureAwait(false);
+ await client.CallAsync<CancelRunResult>(Methods.CancelRun, new CancelRunParams { RunId = runId }, ct)
+ .ConfigureAwait(false);
+ }
+ catch (IpcRpcException ex) when (ex.Message.Contains("no active run"))
+ {
+ // The daemon reports an unknown/inactive run id as "no active run
+ // <id>". Treat that as an idempotent no-op: the run is already gone.
+ return new AxiAbortOutcome(false, runId, "no active run with that id (no-op)");
+ }
+ catch (Exception) when (ct.IsCancellationRequested == false)
+ {
+ // Daemon disappeared between the liveness check and the dial/call:
+ // desired end state is already reached, so treat as a no-op too.
+ return new AxiAbortOutcome(false, runId, "daemon not running, so no active run to cancel (no-op)");
+ }
+ return new AxiAbortOutcome(true, runId, null);📝 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.
| if (!await DaemonStatus.IsRunningAsync(paths, ct).ConfigureAwait(false)) | |
| { | |
| return new AxiAbortOutcome(false, runId, "daemon not running, so no active run to cancel (no-op)"); | |
| } | |
| using var client = await IpcClient.DialAsync(paths.Socket, ct).ConfigureAwait(false); | |
| try | |
| { | |
| await client.CallAsync<CancelRunResult>(Methods.CancelRun, new CancelRunParams { RunId = runId }, ct) | |
| .ConfigureAwait(false); | |
| } | |
| catch (IpcRpcException ex) when (ex.Message.Contains("no active run")) | |
| { | |
| // The daemon reports an unknown/inactive run id as "no active run | |
| // <id>". Treat that as an idempotent no-op: the run is already gone. | |
| return new AxiAbortOutcome(false, runId, "no active run with that id (no-op)"); | |
| } | |
| if (!await DaemonStatus.IsRunningAsync(paths, ct).ConfigureAwait(false)) | |
| { | |
| return new AxiAbortOutcome(false, runId, "daemon not running, so no active run to cancel (no-op)"); | |
| } | |
| try | |
| { | |
| using var client = await IpcClient.DialAsync(paths.Socket, ct).ConfigureAwait(false); | |
| await client.CallAsync<CancelRunResult>(Methods.CancelRun, new CancelRunParams { RunId = runId }, ct) | |
| .ConfigureAwait(false); | |
| } | |
| catch (IpcRpcException ex) when (ex.Message.Contains("no active run")) | |
| { | |
| // The daemon reports an unknown/inactive run id as "no active run | |
| // <id>". Treat that as an idempotent no-op: the run is already gone. | |
| return new AxiAbortOutcome(false, runId, "no active run with that id (no-op)"); | |
| } | |
| catch (Exception) when (ct.IsCancellationRequested == false) | |
| { | |
| // Daemon disappeared between the liveness check and the dial/call: | |
| // desired end state is already reached, so treat as a no-op too. | |
| return new AxiAbortOutcome(false, runId, "daemon not running, so no active run to cancel (no-op)"); | |
| } | |
| return new AxiAbortOutcome(true, runId, null); |
🤖 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.Cli/AxiAbort.cs` around lines 30 - 46, The abort flow
in AxiAbort has a TOCTOU gap between DaemonStatus.IsRunningAsync and the IPC
work, so a daemon exit after the status check can make IpcClient.DialAsync or
CallAsync throw outside the current IpcRpcException handler. Update AxiAbort so
connection/setup failures from IpcClient.DialAsync and the cancel call are
treated like the existing “daemon not running”/“no active run” no-op cases,
keeping the abort operation idempotent even if the daemon disappears
mid-request.
| IpcClient client; | ||
| try | ||
| { | ||
| client = await IpcClient.DialAsync(paths.Socket, ct).ConfigureAwait(false); | ||
| } | ||
| catch (IOException ex) | ||
| { | ||
| throw new IOException($"connect to daemon: {ex.Message}"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Rewrapped IOException drops the original exception/stack trace.
Re-throwing new IOException($"connect to daemon: {ex.Message}") without passing ex as the inner exception loses the original stack trace and exception details, making connection failures harder to diagnose in production logs.
🔧 Proposed fix: preserve the inner exception
catch (IOException ex)
{
- throw new IOException($"connect to daemon: {ex.Message}");
+ throw new IOException($"connect to daemon: {ex.Message}", ex);
}cc @tinycode-labs
As per path instructions, **/*.* requires informing "@tinycode-labs" per file for review feedback.
📝 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.
| IpcClient client; | |
| try | |
| { | |
| client = await IpcClient.DialAsync(paths.Socket, ct).ConfigureAwait(false); | |
| } | |
| catch (IOException ex) | |
| { | |
| throw new IOException($"connect to daemon: {ex.Message}"); | |
| } | |
| IpcClient client; | |
| try | |
| { | |
| client = await IpcClient.DialAsync(paths.Socket, ct).ConfigureAwait(false); | |
| } | |
| catch (IOException ex) | |
| { | |
| throw new IOException($"connect to daemon: {ex.Message}", ex); | |
| } |
🤖 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.Cli/DaemonNotifyPush.cs` around lines 41 - 49, The
IOException wrapping in DaemonNotifyPush’s connect path drops the original
exception details, so update the catch block around IpcClient.DialAsync to
rethrow a new IOException that preserves ex as the inner exception. Keep the
existing context message, but include the caught exception when constructing the
new IOException so the original stack trace and diagnostics are retained.
| public async Task<string> StartRunAsync(Repo repo, string branch, string headSha, string baseSha) | ||
| { | ||
| if (shuttingDown) | ||
| { | ||
| throw new InvalidOperationException("daemon is shutting down"); | ||
| } | ||
|
|
||
| // Serialize per repo+branch so two concurrent pushes cannot both pass | ||
| // the supersede check and create duplicate pipelines (Go branchLocks). | ||
| var branchLock = branchLocks.GetOrAdd(repo.Id + "/" + branch, _ => new SemaphoreSlim(1, 1)); | ||
| await branchLock.WaitAsync().ConfigureAwait(false); | ||
| try | ||
| { | ||
| await CancelActiveRunsAsync(repo.Id, branch).ConfigureAwait(false); | ||
|
|
||
| var run = db.InsertRun(repo.Id, branch, headSha, baseSha); | ||
| var entry = new ActiveRun | ||
| { | ||
| Cts = new CancellationTokenSource(), | ||
| Done = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously), | ||
| }; | ||
| lock (gate) | ||
| { | ||
| active[run.Id] = entry; | ||
| } | ||
| _ = Task.Run(() => ExecuteRunAsync(run, repo, entry)); | ||
| return run.Id; | ||
| } | ||
| finally | ||
| { | ||
| branchLock.Release(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant symbols/lines.
git ls-files 'dotnet/src/NoMistakes.Daemon/RunManager.cs' 'dotnet/src/NoMistakes.Daemon/*' | sed -n '1,120p'
echo '--- AST outline ---'
ast-grep outline dotnet/src/NoMistakes.Daemon/RunManager.cs --view expanded || true
echo '--- Line numbers around StartRunAsync and Shutdown ---'
nl -ba dotnet/src/NoMistakes.Daemon/RunManager.cs | sed -n '1,260p'Repository: leonj1/no-mistakes
Length of output: 2292
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If needed, inspect for shutdown state usage and any lock ordering around active runs.
rg -n "shuttingDown|active\\[|CancelActiveRunsAsync|Shutdown\\(" dotnet/src/NoMistakes.Daemon/RunManager.csRepository: leonj1/no-mistakes
Length of output: 497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '60,120p;170,260p' dotnet/src/NoMistakes.Daemon/RunManager.cs | cat -nRepository: leonj1/no-mistakes
Length of output: 6324
Prevent StartRunAsync from registering after shutdown begins RunManager.cs:63-95 @tinycode-labs
shuttingDown is checked before branchLock/gate, so Shutdown() can set the flag, snapshot active, and return before a new run is inserted and launched. Re-check under gate right before registration, or make shutdown and run creation share the same synchronization, and cancel/mark the new row if shutdown already started.
🤖 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.Daemon/RunManager.cs` around lines 63 - 95,
StartRunAsync can still insert and launch a run after shutdown has begun because
shuttingDown is only checked before taking branchLocks and before the active
registration path. Update RunManager.StartRunAsync to re-check the shutdown
state under the same synchronization used for active registration (the gate
lock), and if shutdown is in progress, avoid calling db.InsertRun or launching
ExecuteRunAsync; instead cancel/mark the would-be run as stopped so Shutdown and
run creation are serialized consistently.
| private void TrySetRunError(string runId, string message) | ||
| { | ||
| try | ||
| { | ||
| db.UpdateRunErrorStatus(runId, message, RunStatus.Failed); | ||
| } | ||
| catch (Exception) | ||
| { | ||
| // Best-effort, like Go logging a failed post-panic DB update. | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
TrySetRunError lacks the terminal-status guard that MarkCancelled has.
MarkCancelled (Lines 299-314) only writes when the run is still Pending/Running, so a pipeline that already committed its own terminal status is never overwritten. TrySetRunError, invoked from the generic catch (Exception ex) branch in ExecuteRunAsync (Line 281), has no such guard — if the pipeline throws after already marking the run Completed/Failed (e.g., an exception during post-completion cleanup), this will silently stomp that terminal state with Failed/"internal panic: ...". The doc comment on ExecuteRunAsync's catch block itself implies the panic path is meant for runs that would otherwise be stuck "pending or running forever," which matches MarkCancelled's guard but isn't enforced here.
🔧 Proposed fix: guard on non-terminal status like `MarkCancelled`
private void TrySetRunError(string runId, string message)
{
try
{
- db.UpdateRunErrorStatus(runId, message, RunStatus.Failed);
+ var fresh = db.GetRun(runId);
+ if (fresh is { Status: RunStatus.Pending or RunStatus.Running })
+ {
+ db.UpdateRunErrorStatus(runId, message, RunStatus.Failed);
+ }
}
catch (Exception)
{
// Best-effort, like Go logging a failed post-panic DB update.
}
}📝 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.
| private void TrySetRunError(string runId, string message) | |
| { | |
| try | |
| { | |
| db.UpdateRunErrorStatus(runId, message, RunStatus.Failed); | |
| } | |
| catch (Exception) | |
| { | |
| // Best-effort, like Go logging a failed post-panic DB update. | |
| } | |
| } | |
| private void TrySetRunError(string runId, string message) | |
| { | |
| try | |
| { | |
| var fresh = db.GetRun(runId); | |
| if (fresh is { Status: RunStatus.Pending or RunStatus.Running }) | |
| { | |
| db.UpdateRunErrorStatus(runId, message, RunStatus.Failed); | |
| } | |
| } | |
| catch (Exception) | |
| { | |
| // Best-effort, like Go logging a failed post-panic DB update. | |
| } | |
| } |
🤖 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.Daemon/RunManager.cs` around lines 316 - 326,
`TrySetRunError` in `RunManager` should mirror the terminal-status protection
used by `MarkCancelled` so it does not overwrite runs that have already reached
a final state. Update the `ExecuteRunAsync` catch-path helper to first check the
current run status (via the same db lookup/state check pattern used in
`MarkCancelled`) and only call `db.UpdateRunErrorStatus(...)` when the run is
still `Pending` or `Running`. Keep the best-effort exception handling, but
ensure `TrySetRunError` skips updates for already `Completed`/`Failed` runs.
| public async Task<TResult?> CallAsync<TResult>(string method, object? parameters, CancellationToken ct = default) | ||
| { | ||
| await mutex.WaitAsync(ct).ConfigureAwait(false); | ||
| try | ||
| { | ||
| var request = Protocol.NewRequest(method, parameters); | ||
| await stream.WriteAsync(request, ct).ConfigureAwait(false); | ||
|
|
||
| Response? response; | ||
| try | ||
| { | ||
| response = await stream.ReadAsync<Response>(ct).WaitAsync(CallTimeout, ct).ConfigureAwait(false); | ||
| } | ||
| catch (TimeoutException ex) | ||
| { | ||
| throw new IOException("read response: timed out", ex); | ||
| } | ||
| if (response == null) | ||
| { | ||
| throw new IOException("read response: connection closed"); | ||
| } | ||
| if (response.Error != null) | ||
| { | ||
| throw new IpcRpcException(response.Error.Code, response.Error.Message); | ||
| } | ||
| if (response.Result is { } result) | ||
| { | ||
| return IpcJson.Deserialize<TResult>(result); | ||
| } | ||
| return default; | ||
| } | ||
| finally | ||
| { | ||
| mutex.Release(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files dotnet/src/NoMistakes.Ipc | sed -n '1,120p'
printf '\n== Client.cs (lines around CallAsync) ==\n'
nl -ba dotnet/src/NoMistakes.Ipc/Client.cs | sed -n '1,180p'
printf '\n== Search for JsonLineStream ==\n'
rg -n "class JsonLineStream|ReadAsync<|WriteAsync|not safe for concurrent readers|concurrent readers|WaitAsync\\(" dotnet/src/NoMistakes.Ipc -S
printf '\n== Candidate files containing JsonLineStream ==\n'
fd -a "JsonLineStream*" dotnet/src/NoMistakes.Ipc || true
fd -a "*Stream*" dotnet/src/NoMistakes.Ipc || trueRepository: leonj1/no-mistakes
Length of output: 571
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the stream implementation and timeout-related code paths.
for f in $(rg -l "class JsonLineStream|WaitAsync\\(|ReadAsync<|WriteAsync" dotnet/src/NoMistakes.Ipc -S); do
echo "===== $f ====="
wc -l "$f"
done
# Show the relevant implementation slices if present.
for f in $(rg -l "class JsonLineStream|ReadAsync<|WriteAsync" dotnet/src/NoMistakes.Ipc -S); do
echo
echo "----- $f (selected lines) -----"
nl -ba "$f" | sed -n '1,260p'
doneRepository: leonj1/no-mistakes
Length of output: 543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused behavioral probe: understand the semantics of Task.WaitAsync with a non-linked cancellation token.
python3 - <<'PY'
import asyncio, textwrap, sys
print("This repo is .NET; semantic check will be done by reading source only if runtime isn't available.")
PYRepository: leonj1/no-mistakes
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any explicit documentation about timeouts, cancellation, or stream reuse.
rg -n "timeout|cancel|cancellation|reuse|concurrent|single reader|mutex" dotnet/src/NoMistakes.Ipc -SRepository: leonj1/no-mistakes
Length of output: 2949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Client.cs =="
sed -n '1,140p' dotnet/src/NoMistakes.Ipc/Client.cs
echo
echo "== JsonLineStream.cs =="
sed -n '1,140p' dotnet/src/NoMistakes.Ipc/JsonLineStream.csRepository: leonj1/no-mistakes
Length of output: 7332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in ["dotnet/src/NoMistakes.Ipc/Client.cs", "dotnet/src/NoMistakes.Ipc/JsonLineStream.cs"]:
print(f"===== {p} =====")
text = Path(p).read_text()
for i, line in enumerate(text.splitlines(), 1):
if p.endswith("Client.cs") and 55 <= i <= 100:
print(f"{i:4d}: {line}")
if p.endswith("JsonLineStream.cs") and 1 <= i <= 120:
print(f"{i:4d}: {line}")
print()
PYRepository: leonj1/no-mistakes
Length of output: 6283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path("dotnet/src/NoMistakes.Ipc/Client.cs").read_text().splitlines()
for i, line in enumerate(text, 1):
if 1 <= i <= 70:
print(f"{i:4d}: {line}")
PYRepository: leonj1/no-mistakes
Length of output: 3204
Timeout should cancel the in-flight read, not just stop waiting. @tinycode-labs
WaitAsync(CallTimeout, ct) only stops awaiting; the underlying ReadAsync keeps running. Once the finally releases mutex, the next call can reuse the same JsonLineStream while that stale read is still consuming bytes, which can corrupt framing or route a late response to the wrong RPC. Use a linked CancellationTokenSource with CancelAfter(CallTimeout) for the read, and treat a timeout as a broken connection.
🤖 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.Ipc/Client.cs` around lines 64 - 99, In
CallAsync<TResult>, the timeout currently only stops waiting via WaitAsync while
the underlying stream.ReadAsync<Response> keeps running, which can leave a stale
read active after mutex is released. Update the read path to use a linked
CancellationTokenSource with CancelAfter(CallTimeout) so the in-flight ReadAsync
is actually canceled, and when that timeout occurs treat the connection as
broken by throwing an IOException (or equivalent) from the timeout catch; keep
the change localized to Client.CallAsync and its response-read logic.
| public async Task ServeAsync(string socketPath) | ||
| { | ||
| var ln = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified); | ||
| try | ||
| { | ||
| // A previous daemon may have crashed without removing its socket | ||
| // file; bind fails on the stale leftover, so clear it first | ||
| // (Go transport_unix.go does the same os.Remove before listen). | ||
| File.Delete(socketPath); | ||
| ln.Bind(new UnixDomainSocketEndPoint(socketPath)); | ||
| ln.Listen(16); | ||
| if (!OperatingSystem.IsWindows()) | ||
| { | ||
| // Go listens under umask 0o077 so only the owner can connect. | ||
| File.SetUnixFileMode( | ||
| socketPath, | ||
| UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the server implementation and nearby context.
git ls-files dotnet/src/NoMistakes.Ipc/Server.cs dotnet/src/NoMistakes.Ipc -z | tr '\0' '\n'
echo "----"
ast-grep outline dotnet/src/NoMistakes.Ipc/Server.cs --view expanded || true
echo "----"
cat -n dotnet/src/NoMistakes.Ipc/Server.cs | sed -n '1,220p'
echo "----"
rg -n "SetUnixFileMode|UnixDomainSocketEndPoint|socketPath|Listen\\(" dotnet/src/NoMistakes.Ipc -SRepository: leonj1/no-mistakes
Length of output: 11133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any umask handling or other socket permission setup in the repo.
rg -n "umask|SetUnixFileMode|UnixFileMode|socket permissions|UnixDomainSocketEndPoint" . -S
echo "----"
# Read the project README or docs for IPC/socket setup if present.
fd -a -t f 'README.md|*.md' . | head -50Repository: leonj1/no-mistakes
Length of output: 2878
🌐 Web query:
Unix domain socket connect before listen bind only connectable after listen permissions checked at connect time
💡 Result:
For Unix domain stream sockets, a connection can only be established after the server has successfully called bind to associate the socket with a path and listen to mark the socket as passive [1][2][3]. If a client attempts to connect before the server has called listen, the connect call will typically fail with ECONNREFUSED or ENOENT [1][4][5]. Regarding permissions, for Unix domain sockets visible in the filesystem, access control is enforced at the time of the connect call [6][4]. On Linux, connecting to a stream socket requires write permission on the socket file itself [6][5]. The permission checking for connecting to a Unix domain socket is equivalent to calling open with write-only access to the socket pathname [4]. In summary: 1. Bind and Listen: The server must bind the socket to a path and then listen to enable the OS to queue incoming connection requests [2][3][7]. 2. Connect Sequence: A client's connect call will fail if the target is not currently listening [1][4][5]. 3. Permissions: The kernel checks if the connecting process has the necessary permissions (write access to the socket file) at the moment connect is invoked [6][4][5]. Note that this permission behavior is Linux-specific; many BSD-derived systems ignore socket file permissions, so portable applications should not rely on them for security [6][5].
Citations:
- 1: https://stackoverflow.com/questions/74192727/af-unix-socket-behaviour-on-client-side-before-the-server-calls-accept
- 2: https://beej.us/guide/bgipc/html/split/unixsock.html
- 3: https://www.engineermaxxing.com/parminces/books/beej-ipc/ch11-unix-sockets.html
- 4: https://litux.nl/mirror/unixnetworkprogramming/0131411551_ch15lev1sec4.html
- 5: https://linux.die.net/man/7/unix
- 6: https://manpages.debian.org/bookworm/manpages/unix.7.en.html
- 7: https://docs.rs/tokio/latest/tokio/net/struct.UnixSocket.html
Set the Unix socket mode before listening. Bind already creates the socket path, so File.SetUnixFileMode(...0700) can run before ln.Listen(16). As written, the socket is briefly exposed with whatever mode the process umask produced; if that umask is permissive, a local process can race the chmod and connect before the restriction lands. @tinycode-labs
🤖 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.Ipc/Server.cs` around lines 61 - 79, The Unix socket
permissions are being tightened too late in ServeAsync, leaving a brief window
where the socket can be accessible with the process umask permissions. Update
the ServeAsync flow in Server.cs so the permission-setting step associated with
File.SetUnixFileMode runs immediately after Bind on the socket path and before
ln.Listen(16), keeping the existing Unix-only guard and the 0700-style mode
intact.
Rule-Based Code ReviewFound 31 issue(s) that need attention:
AI-Powered Code ReviewReviewed 58 code sections. This review was performed automatically using custom rules and AI analysis. Please review the feedback and use your judgment. 31 finding(s)These could not be attached to specific lines in the diff, so they are summarized here:
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Review Process.Start inputs for command injection; prefer explicit executable and arguments. - [ai-reviewer-dotnet-process-start] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH |
Rule-Based Code ReviewFound 32 issue(s) that need attention:
AI-Powered Code ReviewReviewed 78 code sections. This review was performed automatically using custom rules and AI analysis. Please review the feedback and use your judgment. 32 finding(s)These could not be attached to specific lines in the diff, so they are summarized here:
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Review Process.Start inputs for command injection; prefer explicit executable and arguments. - [ai-reviewer-dotnet-process-start] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dotnet/src/NoMistakes.Daemon/RunManager.cs (1)
143-168: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
@tinycode-labsRunManager.cs:143 reject non-branch refs before starting a run.BranchFromRefpasses through anyrefs/*input that is notrefs/heads/*, so a tag push likerefs/tags/v1still creates a run keyed on the tag ref. Filter non-refs/heads/*refs beforeStartRunAsync.🤖 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.Daemon/RunManager.cs` around lines 143 - 168, The ref handling in RunManager.BranchFromRef currently lets any non-refs/heads/* value pass through, so non-branch refs can still reach StartRunAsync. Update the ref parsing in RunManager and/or the caller that invokes StartRunAsync so only refs/heads/* are accepted for run creation, and reject refs/tags/* and other refs before a run is started. Use the existing BranchFromRef and StartRunAsync flow to locate the fix.
🧹 Nitpick comments (8)
dotnet/src/NoMistakes.Core/ApprovalAction.cs (1)
1-12: 📐 Maintainability & Code Quality | 🔵 TrivialPer path instructions, routing 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.Core/ApprovalAction.cs` around lines 1 - 12, This file needs a path-instruction routing update so review feedback is directed to `@tinycode-labs` for the whole file, not just a section. Update the handling around ApprovalAction so any code review feedback for this file consistently mentions `@tinycode-labs` at the file level.Source: Path instructions
dotnet/tests/NoMistakes.Tests/AxiDriveTests.cs (1)
1-237: 📐 Maintainability & Code Quality | 🔵 TrivialPer path instructions, routing 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/AxiDriveTests.cs` around lines 1 - 237, This file needs the review feedback routed to `@tinycode-labs` once at the file level, not repeated per section. Update the relevant review/comment handling around AxiDriveTests so the path-instruction behavior sends a single `@tinycode-labs` mention for this file, and keep the existing test logic and symbols like AxiDriveTests unchanged.Source: Path instructions
dotnet/src/NoMistakes.Cli/AxiRender.cs (1)
1-382: 📐 Maintainability & Code Quality | 🔵 TrivialPer path instructions, routing 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.Cli/AxiRender.cs` around lines 1 - 382, The review-routing note in AxiRender should be updated so the file-level instruction consistently directs all code review feedback to `@tinycode-labs`, rather than mentioning it only for a specific section or implying mixed routing. Adjust the relevant comment text in AxiRender to make the target explicit and file-wide, keeping the wording aligned with the existing path-based review guidance.Source: Path instructions
dotnet/src/NoMistakes.Core/Finding.cs (1)
10-179: 📐 Maintainability & Code Quality | 🔵 TrivialPer path instructions, routing 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.Core/Finding.cs` around lines 10 - 179, Route review feedback at the file level for this change so all comments for Finding.cs go to `@tinycode-labs` once per file, not once per section. Update the review-dispatch logic that handles this file’s path instructions to apply the coderabbitai → `@tinycode-labs` mapping at file scope, and make sure no section-level splits create duplicate or inconsistent routing.Source: Path instructions
dotnet/src/NoMistakes.Git/Gate.cs (1)
1-15: 📐 Maintainability & Code Quality | 🔵 TrivialPer path instructions, routing 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.Git/Gate.cs` around lines 1 - 15, The review routing for this file needs to be applied at the file level, not per section. Update the handling around the Gate namespace so any code review feedback for this file is directed to `@tinycode-labs` once per file, using the Gate class as the unique anchor for this path-based rule.Source: Path instructions
dotnet/src/NoMistakes.Cli/NoMistakes.Cli.csproj (1)
7-16: 📐 Maintainability & Code Quality | 🔵 TrivialPer path instructions, routing 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.Cli/NoMistakes.Cli.csproj` around lines 7 - 16, The review routing for NoMistakes.Cli.csproj is being applied at the section level instead of the file level. Update the handling around the project file entry so the coderabbitai feedback is routed once per file to `@tinycode-labs`, using the csproj file’s top-level review mapping rather than per ItemGroup or per section.Source: Path instructions
dotnet/tests/NoMistakes.Tests/AxiRespondTests.cs (1)
1-780: 📐 Maintainability & Code Quality | 🔵 TrivialPer path instructions, routing 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/AxiRespondTests.cs` around lines 1 - 780, The review feedback routing in AxiRespondTests should be updated to target `@tinycode-labs` at the file level, not per section. Adjust the existing file-level instructions/commenting mechanism around AxiRespondTests so coderabbitai feedback is attributed once for the whole file and not repeated for individual blocks or methods. Use the AxiRespondTests class as the anchor when making the change.Source: Path instructions
dotnet/src/NoMistakes.Cli/Toon.cs (1)
426-433: 📐 Maintainability & Code Quality | 🔵 Trivial
EncodeKeyskips the control-character validation applied to values.
FormatPrimitivecallsValidateCharactersbefore quoting a string value (Line 288), butEncodeKeycallsQuoteStringdirectly without it. A key containing a control character (e.g.\u0001) is silently quoted/escaped instead of raisingToonEncodingException, unlike the identical content used as a value.♻️ Proposed fix
private static string EncodeKey(string key) { + ValidateCharacters(key); if (key.Length == 0 || !IsValidUnquotedKey(key)) { return QuoteString(key); } return key; }🤖 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.Cli/Toon.cs` around lines 426 - 433, EncodeKey currently bypasses the same control-character check that FormatPrimitive applies to string values, so invalid keys can be quoted instead of failing. Update EncodeKey in Toon to validate the key with ValidateCharacters before calling QuoteString, mirroring the value path. Keep the existing IsValidUnquotedKey and QuoteString flow, but ensure any control characters trigger ToonEncodingException consistently for keys and values.
🤖 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.Cli/AxiDrive.cs`:
- Line 91: The active-run lookup in ActiveRunIdAsync is currently treating
lookup failures as if no active run exists, which allows AxiDrive to continue
into TriggerRunAsync incorrectly. Update the ActiveRunIdAsync call path and its
error handling so daemon/IPC failures are surfaced to the caller instead of
returning a null/empty run id, and ensure the logic in AxiDrive uses that error
to stop rather than proceeding with push/rerun. Keep the fix centered on
ActiveRunIdAsync and the related handling around the lookup result.
In `@dotnet/src/NoMistakes.Cli/AxiQuery.cs`:
- Around line 335-352: In ExecutablePath, the ResolveLinkTarget fallback only
handles IOException, so restricted environments can still throw
UnauthorizedAccessException and break the exe fallback. Update the try/catch
around new FileInfo(exe).ResolveLinkTarget(...) to also catch
UnauthorizedAccessException, and return exe in that case just like the existing
IOException path. Keep the fallback behavior unchanged for ExecutablePath and
its callers such as axi home.
In `@dotnet/src/NoMistakes.Pipeline/ApprovalGate.cs`:
- Around line 104-110: The gate setup in ApprovalGate must not leave
waiting/pending armed if db.SetRunAwaitingAgent fails. In the gate-open path
that sets waiting, waitingStep, and pending under mu, wrap the database marker
call so any exception immediately clears the in-memory state (or otherwise
invokes the same cleanup used by the finally path) before rethrowing; use
ApprovalGate, waiting, pending, and db.SetRunAwaitingAgent to locate the fix.
---
Outside diff comments:
In `@dotnet/src/NoMistakes.Daemon/RunManager.cs`:
- Around line 143-168: The ref handling in RunManager.BranchFromRef currently
lets any non-refs/heads/* value pass through, so non-branch refs can still reach
StartRunAsync. Update the ref parsing in RunManager and/or the caller that
invokes StartRunAsync so only refs/heads/* are accepted for run creation, and
reject refs/tags/* and other refs before a run is started. Use the existing
BranchFromRef and StartRunAsync flow to locate the fix.
---
Nitpick comments:
In `@dotnet/src/NoMistakes.Cli/AxiRender.cs`:
- Around line 1-382: The review-routing note in AxiRender should be updated so
the file-level instruction consistently directs all code review feedback to
`@tinycode-labs`, rather than mentioning it only for a specific section or
implying mixed routing. Adjust the relevant comment text in AxiRender to make
the target explicit and file-wide, keeping the wording aligned with the existing
path-based review guidance.
In `@dotnet/src/NoMistakes.Cli/NoMistakes.Cli.csproj`:
- Around line 7-16: The review routing for NoMistakes.Cli.csproj is being
applied at the section level instead of the file level. Update the handling
around the project file entry so the coderabbitai feedback is routed once per
file to `@tinycode-labs`, using the csproj file’s top-level review mapping rather
than per ItemGroup or per section.
In `@dotnet/src/NoMistakes.Cli/Toon.cs`:
- Around line 426-433: EncodeKey currently bypasses the same control-character
check that FormatPrimitive applies to string values, so invalid keys can be
quoted instead of failing. Update EncodeKey in Toon to validate the key with
ValidateCharacters before calling QuoteString, mirroring the value path. Keep
the existing IsValidUnquotedKey and QuoteString flow, but ensure any control
characters trigger ToonEncodingException consistently for keys and values.
In `@dotnet/src/NoMistakes.Core/ApprovalAction.cs`:
- Around line 1-12: This file needs a path-instruction routing update so review
feedback is directed to `@tinycode-labs` for the whole file, not just a section.
Update the handling around ApprovalAction so any code review feedback for this
file consistently mentions `@tinycode-labs` at the file level.
In `@dotnet/src/NoMistakes.Core/Finding.cs`:
- Around line 10-179: Route review feedback at the file level for this change so
all comments for Finding.cs go to `@tinycode-labs` once per file, not once per
section. Update the review-dispatch logic that handles this file’s path
instructions to apply the coderabbitai → `@tinycode-labs` mapping at file scope,
and make sure no section-level splits create duplicate or inconsistent routing.
In `@dotnet/src/NoMistakes.Git/Gate.cs`:
- Around line 1-15: The review routing for this file needs to be applied at the
file level, not per section. Update the handling around the Gate namespace so
any code review feedback for this file is directed to `@tinycode-labs` once per
file, using the Gate class as the unique anchor for this path-based rule.
In `@dotnet/tests/NoMistakes.Tests/AxiDriveTests.cs`:
- Around line 1-237: This file needs the review feedback routed to
`@tinycode-labs` once at the file level, not repeated per section. Update the
relevant review/comment handling around AxiDriveTests so the path-instruction
behavior sends a single `@tinycode-labs` mention for this file, and keep the
existing test logic and symbols like AxiDriveTests unchanged.
In `@dotnet/tests/NoMistakes.Tests/AxiRespondTests.cs`:
- Around line 1-780: The review feedback routing in AxiRespondTests should be
updated to target `@tinycode-labs` at the file level, not per section. Adjust the
existing file-level instructions/commenting mechanism around AxiRespondTests so
coderabbitai feedback is attributed once for the whole file and not repeated for
individual blocks or methods. Use the AxiRespondTests class as the anchor when
making the change.
🪄 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: c17263f0-5680-4872-81a2-4225c23962f3
⛔ Files ignored due to path filters (18)
logs/iter-0023-20260709-010356.logis excluded by!**/*.loglogs/iter-0024-20260709-011540.logis excluded by!**/*.loglogs/iter-0025-20260709-011652.logis excluded by!**/*.loglogs/iter-0026-20260709-012748.logis excluded by!**/*.loglogs/iter-0027-20260709-012911.logis excluded by!**/*.loglogs/iter-0028-20260709-014411.logis excluded by!**/*.loglogs/iter-0029-20260709-014723.logis excluded by!**/*.loglogs/iter-0030-20260709-014843.logis excluded by!**/*.loglogs/iter-0031-20260709-020343.logis excluded by!**/*.loglogs/iter-0032-20260709-020938.logis excluded by!**/*.loglogs/iter-0033-20260709-021140.logis excluded by!**/*.loglogs/iter-0034-20260709-022206.logis excluded by!**/*.loglogs/iter-0035-20260709-022255.logis excluded by!**/*.loglogs/iter-0036-20260709-022831.logis excluded by!**/*.loglogs/iter-0037-20260709-022952.logis excluded by!**/*.loglogs/iter-0038-20260709-024452.logis excluded by!**/*.loglogs/iter-0039-20260709-025624.logis excluded by!**/*.loglogs/iter-0040-20260709-031124.logis excluded by!**/*.log
📒 Files selected for processing (27)
NOTES.mdPLAN.mdSTEPS.mddotnet/no-mistakes.slndotnet/src/NoMistakes.Cli/AxiDrive.csdotnet/src/NoMistakes.Cli/AxiEnv.csdotnet/src/NoMistakes.Cli/AxiQuery.csdotnet/src/NoMistakes.Cli/AxiRender.csdotnet/src/NoMistakes.Cli/CliApp.csdotnet/src/NoMistakes.Cli/NoMistakes.Cli.csprojdotnet/src/NoMistakes.Cli/Toon.csdotnet/src/NoMistakes.Core/ApprovalAction.csdotnet/src/NoMistakes.Core/Finding.csdotnet/src/NoMistakes.Daemon/RunIpcHandlers.csdotnet/src/NoMistakes.Daemon/RunManager.csdotnet/src/NoMistakes.Git/Gate.csdotnet/src/NoMistakes.Pipeline/ApprovalGate.csdotnet/src/NoMistakes.Pipeline/NoMistakes.Pipeline.csprojdotnet/tests/NoMistakes.Tests/ApprovalGateTests.csdotnet/tests/NoMistakes.Tests/AxiDriveCliTests.csdotnet/tests/NoMistakes.Tests/AxiDriveTests.csdotnet/tests/NoMistakes.Tests/AxiQueryCliTests.csdotnet/tests/NoMistakes.Tests/AxiQueryTests.csdotnet/tests/NoMistakes.Tests/AxiRenderTests.csdotnet/tests/NoMistakes.Tests/AxiRespondTests.csdotnet/tests/NoMistakes.Tests/NoMistakes.Tests.csprojdotnet/tests/NoMistakes.Tests/ToonTests.cs
✅ Files skipped from review due to trivial changes (3)
- dotnet/src/NoMistakes.Pipeline/NoMistakes.Pipeline.csproj
- PLAN.md
- STEPS.md
🚧 Files skipped from review as they are similar to previous changes (3)
- dotnet/tests/NoMistakes.Tests/NoMistakes.Tests.csproj
- dotnet/src/NoMistakes.Daemon/RunIpcHandlers.cs
- dotnet/src/NoMistakes.Cli/CliApp.cs
| return AxiOutput.Error(1, $"get current HEAD: {ex.Message}"); | ||
| } | ||
|
|
||
| var runId = await ActiveRunIdAsync(client, env.Repo.Id, branch, headSha, ct).ConfigureAwait(false); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not turn active-run lookup failures into “no active run”.
Lines 150-153 hide daemon/IPC errors, so Line 91 can proceed into TriggerRunAsync and push/rerun while an active run may already exist. Surface the lookup error instead.
Proposed fix
- var runId = await ActiveRunIdAsync(client, env.Repo.Id, branch, headSha, ct).ConfigureAwait(false);
+ string runId;
+ try
+ {
+ runId = await ActiveRunIdAsync(client, env.Repo.Id, branch, headSha, ct).ConfigureAwait(false);
+ }
+ catch (Exception ex) when (ex is not OperationCanceledException)
+ {
+ return AxiOutput.Error(1, $"get active run: {ex.Message}");
+ }
@@
- GetActiveRunResult active;
- try
- {
- active = await client.CallAsync<GetActiveRunResult>(
- Methods.GetActiveRun, new GetActiveRunParams { RepoId = repoId, Branch = branch }, ct)
- .ConfigureAwait(false) ?? new GetActiveRunResult();
- }
- catch (Exception ex) when (ex is not OperationCanceledException)
- {
- return string.Empty;
- }
+ var active = await client.CallAsync<GetActiveRunResult>(
+ Methods.GetActiveRun, new GetActiveRunParams { RepoId = repoId, Branch = branch }, ct)
+ .ConfigureAwait(false) ?? new GetActiveRunResult();
return ActiveRunInfoForHead(active.Run, headSha)?.Id ?? string.Empty;Also applies to: 140-153
🤖 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.Cli/AxiDrive.cs` at line 91, The active-run lookup in
ActiveRunIdAsync is currently treating lookup failures as if no active run
exists, which allows AxiDrive to continue into TriggerRunAsync incorrectly.
Update the ActiveRunIdAsync call path and its error handling so daemon/IPC
failures are surfaced to the caller instead of returning a null/empty run id,
and ensure the logic in AxiDrive uses that error to stop rather than proceeding
with push/rerun. Keep the fix centered on ActiveRunIdAsync and the related
handling around the lookup result.
| internal static string ExecutablePath() | ||
| { | ||
| var exe = Environment.ProcessPath; | ||
| if (string.IsNullOrEmpty(exe)) | ||
| { | ||
| var args = Environment.GetCommandLineArgs(); | ||
| return args.Length > 0 ? args[0] : "no-mistakes"; | ||
| } | ||
| try | ||
| { | ||
| var resolved = new FileInfo(exe).ResolveLinkTarget(returnFinalTarget: true); | ||
| return resolved?.FullName ?? exe; | ||
| } | ||
| catch (IOException) | ||
| { | ||
| return exe; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the relevant method.
FILE="dotnet/src/NoMistakes.Cli/AxiQuery.cs"
wc -l "$FILE"
sed -n '320,360p' "$FILE"
# Search for usages of ExecutablePath to understand impact scope.
rg -n "ExecutablePath\(\)" dotnet/src/NoMistakes.Cli -SRepository: leonj1/no-mistakes
Length of output: 1822
🌐 Web query:
FileSystemInfo.ResolveLinkTarget UnauthorizedAccessException permissions .NET
💡 Result:
The System.UnauthorizedAccessException in.NET's FileSystemInfo.ResolveLinkTarget method occurs when the operating system denies access to the path, typically due to security restrictions or I/O-related issues [1]. While the API is designed to resolve symbolic links and junctions [2][3], it does not have a "try" variant that avoids exceptions, meaning it can throw various IO-related exceptions during execution [4]. Common causes and scenarios include: 1. Permission Restrictions: The process lacks the necessary filesystem permissions to read the link or access the target path [1]. 2. Race Conditions: The file or link may cease to exist, be deleted, or be modified between the time the application verifies the link (e.g., checking LinkTarget) and the time it calls ResolveLinkTarget [5][6]. 3. Unsupported Reparse Points: The method may throw exceptions when encountering certain types of reparse points that are not standard symbolic links or junctions, such as AppExecLinks [7]. 4. Transient I/O Errors: Brief network interruptions or filesystem locks can cause the underlying Windows API calls (which ResolveLinkTarget wraps) to fail [1][5]. Because ResolveLinkTarget is known to throw exceptions rather than returning null for failures, the recommended practice is to wrap calls in a try-catch block [4][8][6]. Standard patterns include catching UnauthorizedAccessException specifically, or broader IOException types, to handle scenarios where the link or target is temporarily inaccessible or deleted [5][6]. If you frequently encounter this, check that your application has adequate privileges for the target directories and implement robust error handling that assumes the filesystem state may change between checks [5]. [4][6]
Citations:
- 1: https://learn.microsoft.com/en-us/dotnet/api/system.unauthorizedaccessexception?view=net-9.0
- 2: https://learn.microsoft.com/en-us/dotnet/api/system.io.filesysteminfo.resolvelinktarget?view=net-10.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.io.file.resolvelinktarget?view=net-9.0
- 4: [API Proposal]: Add *Try*ResolveLinkTarget to FileSystemInfo dotnet/runtime#56256
- 5: Microsoft.Extensions.FileProviders.Physical.PhysicalFilesWatcher.RaiseChangeEvents takes down process on an IO exception dotnet/runtime#71003
- 6: https://source.dot.net/Microsoft.Extensions.FileProviders.Physical/Internal/FileSystemInfoHelper.cs.html
- 7: [release/6.0] Fix detection of unsupported links dotnet/runtime#58399
- 8: Don't throw exception when trying to resolve a possible link path PowerShell/PowerShell#16310
Broaden the ResolveLinkTarget catch to include permission failures. UnauthorizedAccessException can escape in restricted environments, so axi home can fail instead of falling back to exe. Catch it alongside IOException. @tinycode-labs
🤖 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.Cli/AxiQuery.cs` around lines 335 - 352, In
ExecutablePath, the ResolveLinkTarget fallback only handles IOException, so
restricted environments can still throw UnauthorizedAccessException and break
the exe fallback. Update the try/catch around new
FileInfo(exe).ResolveLinkTarget(...) to also catch UnauthorizedAccessException,
and return exe in that case just like the existing IOException path. Keep the
fallback behavior unchanged for ExecutablePath and its callers such as axi home.
Scaffold NoMistakes.Ipc and port internal/ipc/protocol.go: JSON-RPC 2.0 request/response/error types, method and error-code constants (including cancel_run and the notify-push push_received message), params/results, RunInfo/StepResultInfo/IpcEvent wire types, and newline-delimited JSON framing with Go's 1 MiB message cap. Step names normalize "babysit" to "ci" on read. Round-trip tests run over a unix-domain socket pair. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…col.** Scaffold the `NoMistakes.Ipc` project and port the IPC request/response protocol, including the `CancelRun` and `notify-push` message types, with serialization round-trip tests over a socket pair.
Scaffold NoMistakes.Daemon: DaemonHost lifecycle (socket via Paths, atomic PID file with ownership-guarded cleanup, server-PID dir from Paths) plus the unix-socket IpcServer port in NoMistakes.Ipc with health/shutdown dispatch. Tests cover start, stop (IPC + method), PID-file lifecycle, stale-socket replacement, and bind-failure startup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… startup/shutdown.** Scaffold the `NoMistakes.Daemon` project and port daemon startup and shutdown: socket creation, PID file, and server-PID location from `Paths`. Tests cover start, stop, and PID-file lifecycle.
Port RunManager (run creation with per-repo+branch serialization, supersede-on-push, HandleCancel, shutdown) behind a PipelineRunner seam for the slice-9 executor, runToInfo/stepToInfo mapping including the awaiting-agent fields, and the get_run/get_runs/get_active_run/cancel_run IPC handlers. 253 tests pass in Docker (232 baseline + 21 new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… run creation, `HandleCancel`, run status, and `runToInfo` including the awaiting-agent fields. Tests cover run create, cancel, and status/info mapping.
Database.RecoverStaleRuns was already ported with the data layer; this adds the daemon-startup wiring (DaemonHost optional Database, recovery before PID write/serve, best-effort like Go) plus the missing coverage: transactional run+step recovery, never-reported-as-parked at the RunInfoMapper wire level, and a port of Go TestRecoverStaleRunsOnStartup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…StaleRuns`: clears awaiting-agent and fails stale runs plus their steps in one transaction. Tests cover the transactional recovery and that a recovered run is never reported as parked.
…ck (slice 7e.1) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…run <id>` working outside a worktree (needs only `NM_HOME` plus the daemon), with unknown/inactive targets and a stopped daemon as idempotent no-ops (`aborted: false`). Tests cover abort-by-id success and each no-op case (unknown id, inactive run, stopped daemon).
…IPC (slice 7e.2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he slice-4 `notify-push` hook command invoked by the post-receive hook, routed over the slice-7a IPC surface to the daemon. Tests cover the hook invocation reaching the daemon. Mark slice 7 Done in `VERTICAL_SLICES.md` and open the PR.
Hand-port of toon-go's encoder subset (ordered objects, inline primitive arrays, tabular row arrays, full quoting rules) plus the axi render layer: run object, gate object, and findings table with Go-identical field order. Adds RiskLevel to Core findings parsing for the gate risk field. No significance column - the Go findings table has none. Docker verification: 319 tests passed (279 baseline + 40 new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ng with stable field order in `NoMistakes.Cli`: the run object, the gate object, and the findings table (including the `significance` column if merged). Tests assert output shape and field order.
Add AxiQuery (pure TOON document builders for axi home, status, and logs over the slice-8a render layer), AxiEnv (paths/db/repo environment resolution with worktree fallback), and CliApp axi dispatch with cobra-shaped flag parsing and exit codes (2 usage, 1 environment/lookup). Logs validates --step before opening any environment, matching Go's order. Docker verification: 347 tests passed (319 baseline + 28 new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e read-only `axi` commands: home, status, and logs, rendering via the slice-8a TOON layer. Tests cover each command's output against the rendered shapes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… the `axi run` and `axi abort` commands (worktree/branch-scoped abort; abort-by-id already landed in slice 7e). Tests cover run submission and scoped abort.
RunManager responder registry routes IPC respond to the parked gate's consumer (slice-9 executor seam); RunIpcHandlers registers respond; AxiDrive.RespondAsync ports runAxiRespond minus the finding/targeting flags, with action validation before env open. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ort the `axi respond` command with the three response verbs (approve/fix/skip) resolving a parked gate, without the finding or targeting flags. Tests cover each verb's gate-resolution semantics.
Add --findings, --instructions, and --add-finding to axi respond: SplitCsv + ParseAddFinding ports, per-finding instructions fan-out, and exit-2 usage errors for malformed --add-finding payloads. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ort the `--findings` and `--add-finding` flags on `axi respond`, including finding-payload parsing and validation errors for malformed payloads. Tests cover each flag's payload path and the malformed-payload errors.
… and `--yes` default.** Port the `--step` flag (targeting a specific parked step) and the `--yes` default behavior on `axi respond`. Tests cover step targeting, targeting a non-parked step, and the `--yes` default path.
Add NoMistakes.Pipeline project with ApprovalGate: the approval-gate half of Go's pipeline executor (waiting-step state, buffered approval hand-off, awaiting-agent marker lifecycle). Includes ApprovalGateTests and PLAN.md restating VERTICAL_SLICES.md as the port plan. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3f45a09 to
d99fbbd
Compare
Rule-Based Code ReviewFound 32 issue(s) that need attention:
AI-Powered Code ReviewReviewed 78 code sections. This review was performed automatically using custom rules and AI analysis. Please review the feedback and use your judgment. 32 finding(s)These could not be attached to specific lines in the diff, so they are summarized here:
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Review Process.Start inputs for command injection; prefer explicit executable and arguments. - [ai-reviewer-dotnet-process-start] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH
[Dotnet Csharp Best Practices Filesystem Rules] [Dotnet Csharp Best Practices Filesystem Rules] - Avoid BinaryFormatter-style deserialization of untrusted data. - [ai-reviewer-dotnet-binaryformatter-deserialize] - [DB Rule ID: 82120ebc-59c8-54b8-adc2-e842d1f3f7c8] Category: RuleCategory.SECURITY | Priority: RulePriority.RECOMMENDED | Severity: RuleSeverity.HIGH |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@NOTES.md`:
- Around line 383-389: Update the DaemonHost recovery note to match the actual
bootstrap split and avoid implying that 7e.2 wires Database/RunManager into
DaemonHost yet. In the NOTES.md section describing DaemonHost, keep the mention
of the optional Database? db parameter and RecoverOnStartup behavior, but revise
the guidance so it reflects that the real Database/RunManager hookup is deferred
to a later slice; make sure the wording around DaemonHost, RunAsync, and
RecoverOnStartup does not suggest implementation work that is not present.
In `@STEPS.md`:
- Around line 41-42: Update the Slice 8a note in STEPS.md to remove the stale
significance-column reference so the roadmap matches the final TOON output
shape; edit the Slice 8a bullet text and its “Done when” note to describe the
run object, gate object, and findings table as rendered by NoMistakes.Cli
without mentioning a significance column, keeping the stable field-order/test
expectations intact.
🪄 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: d3dd4d3e-dbc1-4928-aadd-c97450d07529
⛔ Files ignored due to path filters (28)
logs/iter-0013-20260709-001851.logis excluded by!**/*.loglogs/iter-0014-20260709-002924.logis excluded by!**/*.loglogs/iter-0015-20260709-003021.logis excluded by!**/*.loglogs/iter-0016-20260709-003846.logis excluded by!**/*.loglogs/iter-0017-20260709-003939.logis excluded by!**/*.loglogs/iter-0018-20260709-004825.logis excluded by!**/*.loglogs/iter-0019-20260709-004946.logis excluded by!**/*.loglogs/iter-0020-20260709-005531.logis excluded by!**/*.loglogs/iter-0021-20260709-005643.logis excluded by!**/*.loglogs/iter-0022-20260709-010249.logis excluded by!**/*.loglogs/iter-0023-20260709-010356.logis excluded by!**/*.loglogs/iter-0024-20260709-011540.logis excluded by!**/*.loglogs/iter-0025-20260709-011652.logis excluded by!**/*.loglogs/iter-0026-20260709-012748.logis excluded by!**/*.loglogs/iter-0027-20260709-012911.logis excluded by!**/*.loglogs/iter-0028-20260709-014411.logis excluded by!**/*.loglogs/iter-0029-20260709-014723.logis excluded by!**/*.loglogs/iter-0030-20260709-014843.logis excluded by!**/*.loglogs/iter-0031-20260709-020343.logis excluded by!**/*.loglogs/iter-0032-20260709-020938.logis excluded by!**/*.loglogs/iter-0033-20260709-021140.logis excluded by!**/*.loglogs/iter-0034-20260709-022206.logis excluded by!**/*.loglogs/iter-0035-20260709-022255.logis excluded by!**/*.loglogs/iter-0036-20260709-022831.logis excluded by!**/*.loglogs/iter-0037-20260709-022952.logis excluded by!**/*.loglogs/iter-0038-20260709-024452.logis excluded by!**/*.loglogs/iter-0039-20260709-025624.logis excluded by!**/*.loglogs/iter-0040-20260709-031124.logis excluded by!**/*.log
📒 Files selected for processing (54)
NOTES.mdPLAN.mdSTEPS.mdVERTICAL_SLICES.mddotnet/no-mistakes.slndotnet/src/NoMistakes.Cli/AxiAbort.csdotnet/src/NoMistakes.Cli/AxiDrive.csdotnet/src/NoMistakes.Cli/AxiEnv.csdotnet/src/NoMistakes.Cli/AxiQuery.csdotnet/src/NoMistakes.Cli/AxiRender.csdotnet/src/NoMistakes.Cli/CliApp.csdotnet/src/NoMistakes.Cli/DaemonNotifyPush.csdotnet/src/NoMistakes.Cli/NoMistakes.Cli.csprojdotnet/src/NoMistakes.Cli/Toon.csdotnet/src/NoMistakes.Core/ApprovalAction.csdotnet/src/NoMistakes.Core/Finding.csdotnet/src/NoMistakes.Core/RunCancelReason.csdotnet/src/NoMistakes.Daemon/DaemonHost.csdotnet/src/NoMistakes.Daemon/DaemonPidRecord.csdotnet/src/NoMistakes.Daemon/DaemonStatus.csdotnet/src/NoMistakes.Daemon/NoMistakes.Daemon.csprojdotnet/src/NoMistakes.Daemon/RunInfoMapper.csdotnet/src/NoMistakes.Daemon/RunIpcHandlers.csdotnet/src/NoMistakes.Daemon/RunManager.csdotnet/src/NoMistakes.Git/Gate.csdotnet/src/NoMistakes.Ipc/Client.csdotnet/src/NoMistakes.Ipc/IpcJson.csdotnet/src/NoMistakes.Ipc/JsonLineStream.csdotnet/src/NoMistakes.Ipc/Messages.csdotnet/src/NoMistakes.Ipc/NoMistakes.Ipc.csprojdotnet/src/NoMistakes.Ipc/Protocol.csdotnet/src/NoMistakes.Ipc/Server.csdotnet/src/NoMistakes.Ipc/WireTypes.csdotnet/src/NoMistakes.Pipeline/ApprovalGate.csdotnet/src/NoMistakes.Pipeline/NoMistakes.Pipeline.csprojdotnet/tests/NoMistakes.Tests/ApprovalGateTests.csdotnet/tests/NoMistakes.Tests/AxiAbortByRunIdTests.csdotnet/tests/NoMistakes.Tests/AxiDriveCliTests.csdotnet/tests/NoMistakes.Tests/AxiDriveTests.csdotnet/tests/NoMistakes.Tests/AxiQueryCliTests.csdotnet/tests/NoMistakes.Tests/AxiQueryTests.csdotnet/tests/NoMistakes.Tests/AxiRenderTests.csdotnet/tests/NoMistakes.Tests/AxiRespondTests.csdotnet/tests/NoMistakes.Tests/DaemonLifecycleTests.csdotnet/tests/NoMistakes.Tests/DaemonNotifyPushTests.csdotnet/tests/NoMistakes.Tests/DaemonPidRecordTests.csdotnet/tests/NoMistakes.Tests/IpcProtocolTests.csdotnet/tests/NoMistakes.Tests/IpcSocketRoundTripTests.csdotnet/tests/NoMistakes.Tests/NoMistakes.Tests.csprojdotnet/tests/NoMistakes.Tests/RunInfoMapperTests.csdotnet/tests/NoMistakes.Tests/RunIpcHandlerTests.csdotnet/tests/NoMistakes.Tests/RunManagerTests.csdotnet/tests/NoMistakes.Tests/RunTests.csdotnet/tests/NoMistakes.Tests/ToonTests.cs
✅ Files skipped from review due to trivial changes (5)
- dotnet/src/NoMistakes.Daemon/NoMistakes.Daemon.csproj
- VERTICAL_SLICES.md
- dotnet/src/NoMistakes.Pipeline/NoMistakes.Pipeline.csproj
- dotnet/tests/NoMistakes.Tests/NoMistakes.Tests.csproj
- PLAN.md
🚧 Files skipped from review as they are similar to previous changes (47)
- dotnet/src/NoMistakes.Ipc/NoMistakes.Ipc.csproj
- dotnet/src/NoMistakes.Git/Gate.cs
- dotnet/src/NoMistakes.Core/ApprovalAction.cs
- dotnet/tests/NoMistakes.Tests/RunTests.cs
- dotnet/src/NoMistakes.Daemon/DaemonStatus.cs
- dotnet/src/NoMistakes.Cli/NoMistakes.Cli.csproj
- dotnet/src/NoMistakes.Core/RunCancelReason.cs
- dotnet/src/NoMistakes.Cli/AxiAbort.cs
- dotnet/src/NoMistakes.Ipc/WireTypes.cs
- dotnet/src/NoMistakes.Daemon/RunInfoMapper.cs
- dotnet/src/NoMistakes.Daemon/RunIpcHandlers.cs
- dotnet/tests/NoMistakes.Tests/AxiQueryCliTests.cs
- dotnet/no-mistakes.sln
- dotnet/tests/NoMistakes.Tests/RunIpcHandlerTests.cs
- dotnet/tests/NoMistakes.Tests/AxiAbortByRunIdTests.cs
- dotnet/tests/NoMistakes.Tests/DaemonPidRecordTests.cs
- dotnet/src/NoMistakes.Cli/DaemonNotifyPush.cs
- dotnet/src/NoMistakes.Ipc/Protocol.cs
- dotnet/tests/NoMistakes.Tests/IpcSocketRoundTripTests.cs
- dotnet/tests/NoMistakes.Tests/ToonTests.cs
- dotnet/src/NoMistakes.Ipc/IpcJson.cs
- dotnet/tests/NoMistakes.Tests/ApprovalGateTests.cs
- dotnet/src/NoMistakes.Cli/AxiEnv.cs
- dotnet/src/NoMistakes.Pipeline/ApprovalGate.cs
- dotnet/src/NoMistakes.Daemon/DaemonHost.cs
- dotnet/tests/NoMistakes.Tests/RunManagerTests.cs
- dotnet/src/NoMistakes.Core/Finding.cs
- dotnet/src/NoMistakes.Ipc/JsonLineStream.cs
- dotnet/tests/NoMistakes.Tests/AxiQueryTests.cs
- dotnet/src/NoMistakes.Ipc/Messages.cs
- dotnet/tests/NoMistakes.Tests/AxiDriveTests.cs
- dotnet/tests/NoMistakes.Tests/RunInfoMapperTests.cs
- dotnet/tests/NoMistakes.Tests/IpcProtocolTests.cs
- dotnet/tests/NoMistakes.Tests/DaemonNotifyPushTests.cs
- dotnet/tests/NoMistakes.Tests/AxiDriveCliTests.cs
- dotnet/src/NoMistakes.Cli/CliApp.cs
- dotnet/tests/NoMistakes.Tests/DaemonLifecycleTests.cs
- dotnet/src/NoMistakes.Daemon/RunManager.cs
- dotnet/src/NoMistakes.Ipc/Client.cs
- dotnet/src/NoMistakes.Daemon/DaemonPidRecord.cs
- dotnet/src/NoMistakes.Ipc/Server.cs
- dotnet/src/NoMistakes.Cli/AxiRender.cs
- dotnet/src/NoMistakes.Cli/AxiQuery.cs
- dotnet/src/NoMistakes.Cli/Toon.cs
- dotnet/tests/NoMistakes.Tests/AxiRenderTests.cs
- dotnet/tests/NoMistakes.Tests/AxiRespondTests.cs
- dotnet/src/NoMistakes.Cli/AxiDrive.cs
| - Daemon wiring: `DaemonHost` ctor gained an optional `Database? db` param; | ||
| `RunAsync` calls private `RecoverOnStartup()` right after `EnsureDirs`, | ||
| BEFORE the PID file is written and the socket serves (Go recoverOnStartup | ||
| order). Recovery is best-effort (exception swallowed, daemon keeps | ||
| starting, like Go's slog.Error path). With `db == null` (all pre-7d | ||
| callers/tests) it is a no-op — 7e.2 should pass the real Database when it | ||
| constructs Database+RunManager in the daemon. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reconcile the 7d recovery note with the actual bootstrap split.
This says 7e.2 should pass the real Database/RunManager into DaemonHost, but the later 7e.2 note explicitly says that wiring was deferred. Please update this note so the next slice does not look for bootstrap code that does not exist yet.
♻️ Suggested doc fix
- `RunAsync` calls private `RecoverOnStartup()` right after `EnsureDirs`, BEFORE the PID file is written and the socket serves (Go recoverOnStartup order). Recovery is best-effort (exception swallowed, daemon keeps starting, like Go's slog.Error path). With `db == null` (all pre-7d callers/tests) it is a no-op — 7e.2 should pass the real Database when it constructs Database+RunManager in the daemon.
+ `RunAsync` calls private `RecoverOnStartup()` right after `EnsureDirs`, BEFORE the PID file is written and the socket serves (Go recoverOnStartup order). Recovery is best-effort (exception swallowed, daemon keeps starting, like Go's slog.Error path). With `db == null` (all pre-7d callers/tests) it is a no-op — the real `Database`/`RunManager` daemon bootstrap is still deferred to a later slice.As per path instructions, **/*.*: for code review feedback by coderabbitai then inform @tinycode-labs per file, not per section of file.
📝 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.
| - Daemon wiring: `DaemonHost` ctor gained an optional `Database? db` param; | |
| `RunAsync` calls private `RecoverOnStartup()` right after `EnsureDirs`, | |
| BEFORE the PID file is written and the socket serves (Go recoverOnStartup | |
| order). Recovery is best-effort (exception swallowed, daemon keeps | |
| starting, like Go's slog.Error path). With `db == null` (all pre-7d | |
| callers/tests) it is a no-op — 7e.2 should pass the real Database when it | |
| constructs Database+RunManager in the daemon. | |
| - Daemon wiring: `DaemonHost` ctor gained an optional `Database? db` param; | |
| `RunAsync` calls private `RecoverOnStartup()` right after `EnsureDirs`, | |
| BEFORE the PID file is written and the socket serves (Go recoverOnStartup | |
| order). Recovery is best-effort (exception swallowed, daemon keeps | |
| starting, like Go's slog.Error path). With `db == null` (all pre-7d | |
| callers/tests) it is a no-op — the real `Database`/`RunManager` daemon bootstrap is still deferred to a later slice. |
🧰 Tools
🪛 LanguageTool
[style] ~386-~386: To elevate your writing, try using more formal phrasing here.
Context: ...est-effort (exception swallowed, daemon keeps starting, like Go's slog.Error path). With `db =...
(CONTINUE_TO_VB)
🤖 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 `@NOTES.md` around lines 383 - 389, Update the DaemonHost recovery note to
match the actual bootstrap split and avoid implying that 7e.2 wires
Database/RunManager into DaemonHost yet. In the NOTES.md section describing
DaemonHost, keep the mention of the optional Database? db parameter and
RecoverOnStartup behavior, but revise the guidance so it reflects that the real
Database/RunManager hookup is deferred to a later slice; make sure the wording
around DaemonHost, RunAsync, and RecoverOnStartup does not suggest
implementation work that is not present.
Source: Path instructions
| - [x] **Slice 8a — TOON rendering.** Port TOON rendering with stable field order in `NoMistakes.Cli`: the run object, the gate object, and the findings table (including the `significance` column if merged). Tests assert output shape and field order. | ||
| Done when: `docker build -f Dockerfile.test.dotnet .` succeeds with the TOON output-shape/field-order tests passing. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the stale significance-column reference.
Slice 8a is already marked done, and the slice notes say the findings table intentionally has no significance column. Please update this entry so the roadmap matches the final rendered shape.
♻️ Suggested doc fix
-- [x] **Slice 8a — TOON rendering.** Port TOON rendering with stable field order in `NoMistakes.Cli`: the run object, the gate object, and the findings table (including the `significance` column if merged). Tests assert output shape and field order.
++ [x] **Slice 8a — TOON rendering.** Port TOON rendering with stable field order in `NoMistakes.Cli`: the run object, the gate object, and the findings table. Tests assert output shape and field order.As per path instructions, **/*.*: for code review feedback by coderabbitai then inform @tinycode-labs per file, not per section of file.
📝 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.
| - [x] **Slice 8a — TOON rendering.** Port TOON rendering with stable field order in `NoMistakes.Cli`: the run object, the gate object, and the findings table (including the `significance` column if merged). Tests assert output shape and field order. | |
| Done when: `docker build -f Dockerfile.test.dotnet .` succeeds with the TOON output-shape/field-order tests passing. | |
| - [x] **Slice 8a — TOON rendering.** Port TOON rendering with stable field order in `NoMistakes.Cli`: the run object, the gate object, and the findings table. Tests assert output shape and field order. |
🤖 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 `@STEPS.md` around lines 41 - 42, Update the Slice 8a note in STEPS.md to
remove the stale significance-column reference so the roadmap matches the final
TOON output shape; edit the Slice 8a bullet text and its “Done when” note to
describe the run object, gate object, and findings table as rendered by
NoMistakes.Cli without mentioning a significance column, keeping the stable
field-order/test expectations intact.
Source: Path instructions
1 participant
Summary
Ports vertical slice 7 (daemon IPC and run lifecycle) to the .NET codebase, completed across steps 7a–7e.2:
NoMistakes.Ipc): request/response protocol, wire types, newline-delimited JSON framing with the 1 MiB cap, step-name normalization ("babysit" → "ci").IpcServer(stale-socket replacement, 0700 socket),DaemonHostlifecycle with atomic PID-file ownership semantics.RunInfomapping including awaiting-agent fields, run IPC handlers.IpcClient, daemon liveness check, and the abort-by-id op with idempotent no-ops for unknown/inactive runs and a stopped daemon.RunManager.HandlePushReceivedAsync+ thepush_receivedhandler on the daemon, and the hiddendaemon notify-pushCLI command the slice-4 post-receive hook invokes, routed over IPC. An end-to-end test runs the real hook script against the real CLI binary and asserts the push reaches a live daemon and starts a run.Slice 7 is marked Done in
VERTICAL_SLICES.md; porting notes for later slices are inNOTES.md.Test plan
docker build -f Dockerfile.test.dotnet .— 279 tests pass (185 on main + 94 added by this branch), including the notify-push hook-invocation e2e test.🤖 Generated with Claude Code
Summary by CodeRabbit
daemon notify-pushworkflow for post-receive hooks.