Conversation
|
Warning Review limit reached
More reviews will be available in 34 minutes and 10 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds the Resgrid.Audio.Voice library, refactors Resgrid API clients to injected instances, adds relay engine dispatch/radio/record modes, and updates Docker, CI, and tooling files. ChangesLiveKit PTT Voice Subsystem
Developer tooling
Sequence Diagram(s)LiveKit session lifecycle sequenceDiagram
participant VoiceRoomManager
participant LiveKitVoiceTransport
participant LiveKitRoomSession
participant LiveKitAudioPublisher
VoiceRoomManager->>LiveKitVoiceTransport: CreateSession(VoiceChannel)
VoiceRoomManager->>LiveKitRoomSession: ConnectAsync()
LiveKitRoomSession->>LiveKitRoomSession: TrackReadLoop for subscribed tracks
LiveKitRoomSession->>LiveKitAudioPublisher: CreatePublisherAsync(trackName)
LiveKitAudioPublisher->>LiveKitAudioPublisher: WriteAsync / FlushAsync
Dispatch announcement flow sequenceDiagram
participant DispatchVoiceMode
participant IResgridCallsApi
participant VoiceRoomManager
participant IVoiceRoomSession
participant DispatchToneOutService
participant IAudioPublisher
DispatchVoiceMode->>IResgridCallsApi: GetActiveCallsAsync(departmentId)
DispatchVoiceMode->>VoiceRoomManager: JoinAsync(VoiceChannel)
VoiceRoomManager-->>DispatchVoiceMode: IVoiceRoomSession
DispatchVoiceMode->>IVoiceRoomSession: CreatePublisherAsync(trackName)
DispatchVoiceMode->>DispatchToneOutService: AnnounceAsync(publisher, announcement)
DispatchToneOutService->>IAudioPublisher: WriteAsync / FlushAsync
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (4)
Resgrid.Audio.Voice/Dsp/CtcssDetector.cs (1)
40-49: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffHot-path allocations in
Process.Per-sample
List<short>.Add, plusGetRange(...).ToArray()andRemoveRange(0, n)(each an O(n) buffer shift) per block, run on the live receive path. A ring buffer /Span-based sliding window would avoid the per-block allocation and copy. Optional, but this code runs continuously on RF audio.🤖 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 `@Resgrid.Audio.Voice/Dsp/CtcssDetector.cs` around lines 40 - 49, The Process hot path in CtcssDetector is doing per-sample List<short>.Add plus per-block GetRange(...).ToArray() and RemoveRange(...) shifts, which creates allocations and copies on the live audio path. Refactor the sliding window logic in Process to use a ring buffer or Span-based window over pcm/_windowSamples so block extraction and 50% overlap reuse storage without allocating or shifting the underlying buffer. Keep the existing Goertzel.NormalizedStrength and _present update behavior, but change how the block data is buffered and read.Resgrid.Audio.Voice/Dsp/Mdc1200Decoder.cs (1)
36-53: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
Processruns a full-window copy + multi-phase decode every frame once buffered.After the buffer reaches
_windowSamples, every subsequent frame triggers_buffer.ToArray()and a fullTryDecode(which itself iterates all bit phases). Combined with per-sampleAddand O(n)RemoveRange, this is costly on a continuous RF stream. Consider a ring buffer and/or only attempting decode on a decimated cadence. Functionally fine; optional.🤖 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 `@Resgrid.Audio.Voice/Dsp/Mdc1200Decoder.cs` around lines 36 - 53, `Mdc1200Decoder.Process` is doing an expensive full-buffer copy and full `TryDecode` on every frame once `_buffer` is populated, plus per-sample `Add` and O(n) `RemoveRange`. Optimize the hot path by switching the backing storage to a ring buffer or similar sliding window, and consider decoding only on a decimated cadence instead of every call. Keep the existing behavior of retaining `_tailSamples` for boundary-spanning packets, but reduce repeated allocations and scans in `Process` and `Mdc1200Codec.TryDecode`.Resgrid.Audio.Voice/Recording/SqliteTransmissionLog.cs (1)
68-91: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDispose the
SqliteCommand.
cmd(Line 68) is created without ausing, so the command (and its parameter collection) isn't deterministically disposed on each append. Wrap it inawait using/usingto release native handles promptly on the hot recording path.🔧 Proposed fix
- var cmd = conn.CreateCommand(); + await using var cmd = conn.CreateCommand();🤖 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 `@Resgrid.Audio.Voice/Recording/SqliteTransmissionLog.cs` around lines 68 - 91, The SqliteCommand created in SqliteTransmissionLog.Append/record persistence is never deterministically disposed, leaving command and parameter resources alive on the hot path. Update the command lifecycle in the method that builds cmd from conn.CreateCommand() so it is wrapped in using or await using, and keep the existing parameter setup and ExecuteNonQueryAsync call inside that scope to release native handles promptly.Dockerfile (1)
54-57: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePrefer
chownto the runtime user overchmod 0777.World-writable runtime dirs are broader than needed. If the non-root UID of the DHI base is known,
chown-ing the dirs (or0770) grants the same write access without making them writable by every account in the container.🤖 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 `@Dockerfile` around lines 54 - 57, The runtime directory setup is too permissive because it uses world-writable permissions for /app/publish/data and /app/publish/recordings. Update the Dockerfile’s directory-creation step to grant write access by changing ownership to the DHI runtime user (or, if appropriate, use tighter group-based permissions like 0770) instead of chmod 0777, so the non-root container user can write without exposing the dirs to every account.
🤖 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 @.github/workflows/dotnet.yml:
- Around line 126-136: The Add-Ffi helper should fail clearly when the extracted
library is missing, and it should verify the downloaded archive before using it.
Update Add-Ffi to check the Get-ChildItem result before Copy-Item and throw a
descriptive error if no matching FFI library is found, using the Add-Ffi symbol
to locate the change. Also add an integrity check for the Invoke-WebRequest
download step, similar to the ffi-download stage, so the asset.zip contents are
validated before Expand-Archive and Copy-Item run.
In @.gitignore:
- Line 115: The .mcp.json ignore rule is too broad and will match nested files
anywhere in the tree. Update the ignore entry in .gitignore to be anchored to
the repository root so it only applies to the workspace-local config, using the
existing .mcp.json rule location as the reference.
In `@Dockerfile`:
- Around line 87-103: The ffi-download stage currently downloads and installs
liblivekit_ffi.so without verifying integrity, unlike the other pinned downloads
in this repository. Update the ffi-download logic to use a per-arch SHA-256
check before the install step, and keep the existing TARGETARCH and asset
selection flow in sync with the expected checksum values. Locate the change in
the ffi-download stage by the asset selection/curl/unzip/install sequence and
add verification immediately after download and before install.
- Around line 43-44: The Dockerfile currently downloads the generic
docker-compose-wait asset via the ADD in the build stage, which pulls the x86_64
binary and breaks arm64 images. Update the wait download logic to use the
architecture-specific asset based on TARGETARCH, and keep ENTRYPOINT using the
matching binary name so the container works on both amd64 and arm64; use the
existing ffi-download TARGETARCH handling as the pattern and adjust the
wait-related build steps accordingly.
In `@Resgrid.Audio.Relay.Console/Voice/DispatchVoiceMode.cs`:
- Around line 66-83: The dispatch tone-out loop in DispatchVoiceMode currently
adds each CallId to seen before AnnounceAsync succeeds, so a failed announcement
is treated as processed and never retried. Update the logic around
VoiceModeRuntime.FormatCallAnnouncement and service.AnnounceAsync so a call is
only recorded in seen after a successful announce, or explicitly remove the
CallId from seen when AnnounceAsync throws. If needed, handle failures per call
inside the foreach so one bad announcement does not prevent later calls in the
same batch from being processed.
In `@Resgrid.Audio.Relay.Console/Voice/RadioMode.cs`:
- Around line 51-84: The NAudioRadioDevice instance in RadioMode is only
manually disposed at the end, so it can leak if an exception is thrown before
that point. Update the RadioMode flow to scope `device` with a `using` so it is
cleaned up alongside `ptt`, `carrier`, and `bridge`, and remove the trailing
manual `device.Dispose()`; keep the change localized around the
`NAudioRadioDevice`, `RadioBridge`, and `StartAsync` setup path.
- Around line 122-123: The RadioMode configuration parsing ignores the return
value from Enum.TryParse, so invalid values for o.PttMethod and o.CarrierDetect
silently fall back to default enum values. Update the parsing logic in the
RadioMode setup to check the TryParse results for both PttKeyingMethod and
CarrierDetectSource, and when parsing fails, surface a clear warning or error
that includes the invalid setting value and the relevant property name before
continuing or failing fast.
- Around line 147-178: `RadioMode.CreatePtt` and `RadioMode.CreateCarrier` are
always constructing `SerialPttController` and `SerialCarrierDetector` with only
`settings.SerialPort`, which prevents them from sharing the same COM port.
Update these helpers to accept and pass through a shared `SerialPort` instance
(or equivalent shared-port parameter) when serial PTT and serial carrier detect
target the same port, while keeping the existing `settings`, `useDtr`/`useDsr`,
and `logger` wiring intact.
In `@Resgrid.Audio.Relay.Console/Voice/RecordMode.cs`:
- Around line 42-62: Move the setup/wait logic in RecordMode.Execute (the
JoinAsync loop, TransmissionRecorder.Start, and
VoiceModeRuntime.WaitForCancellationAsync) into a try block and put disposal of
recorders, log, and disposableStores in a finally so partial failures or
cancellation still clean up. Use the existing recorders, log, and
disposableStores variables to ensure any successfully created
TransmissionRecorder instances, the optional log, and all disposable store
objects are disposed even if JoinAsync throws mid-loop. Keep the return path
unchanged, but make teardown exception-safe by always running it in the finally.
In `@Resgrid.Audio.Voice/Audio/Resampler.cs`:
- Around line 20-26: Add explicit sample-rate validation in the Resampler
resampling path before computing outLen or step, since inRate and outRate are
currently used in division in Resample without being checked. Update the
Resampler.Resample method to reject zero or negative inRate/outRate values up
front, returning an error or empty result consistently with the existing
behavior, so the later length calculation and step computation cannot divide by
zero.
- Around line 20-25: The Resampler logic currently computes outLen as a long,
but short array allocation requires an int, so update the allocation path in
Resampler to validate outLen is within int.MaxValue before creating the output
buffer. Use the existing outLen calculation in Resampler and, after guarding for
nonpositive values, cast to int only after the bounds check so new short[...]
compiles safely.
In `@Resgrid.Audio.Voice/LiveKit/LiveKitAudioPublisher.cs`:
- Around line 118-138: `LiveKitAudioPublisher.DisposeAsync` currently marks the
publisher as disposed before calling `FlushAsync`, which causes `FlushAsync` to
return immediately and drop any buffered residual audio. Update `DisposeAsync`
to flush pending audio before setting `_disposed` (or perform the flush inline),
and keep the existing `_logger` error handling around the flush and unpublish
steps so the final partial frame is preserved during teardown.
In `@Resgrid.Audio.Voice/LiveKit/LiveKitRoomSession.cs`:
- Around line 48-57: The LiveKitRoomSession connection flow is firing duplicate
“connected” notifications because RaiseConnection(true, "connected") is
triggered both by the _room.Connected event handler and again after ConnectAsync
in LiveKitRoomSession; remove the redundant explicit raise (or the event
handler) so subscribers see a single connected transition, keeping the event
wiring and the ConnectAsync success path consistent.
In `@Resgrid.Audio.Voice/Recording/TransmissionRecorder.cs`:
- Around line 61-69: Guard against a null TrackSid in OnAudioFrame: the current
_segments.GetOrAdd(frame.TrackSid, ...) call will throw if the key is null. Add
an early return near the existing PCM null/empty check in OnAudioFrame so frames
with a null TrackSid are ignored before touching the ConcurrentDictionary.
In `@Resgrid.Audio.Voice/ToneOut/DispatchToneOutService.cs`:
- Around line 70-73: The per-channel publish loop in DispatchToneOutService is
swallowing cancellation by catching Exception and logging it as an error. Update
the catch block in the tone-out publish path to let cancellation flow normally
by handling OperationCanceledException separately (or rethrowing it) and only
logging unexpected failures in the existing _logger.Error path, so normal
shutdown isn’t treated as an error.
- Around line 49-51: Guard the preview logging in DispatchToneOutService so a
null text argument cannot crash the method. In the tone-out path where
_logger?.Information builds the preview from text.Length/text.Substring,
normalize text once at the start of the method (or use a safe fallback string)
and then use that normalized value for both the length check and preview
generation. Update the logging statement and any nearby logic in
DispatchToneOutService to reference the safe variable instead of the raw text
parameter.
In `@Resgrid.Audio.Voice/ToneOut/IDepartmentVoiceResolver.cs`:
- Around line 28-31: The CustomerDepartmentVoiceResolver constructor should
validate its IVoiceChannelProvider dependency immediately instead of allowing a
null to be stored and fail later. Update the constructor in
CustomerDepartmentVoiceResolver to perform a null check on the channels
parameter and throw an appropriate argument exception before assigning it to
_channels, so misconfigured DI is caught at construction time.
In `@Resgrid.Audio.Voice/ToneOut/ToneGenerator.cs`:
- Around line 35-36: Clamp the generated PCM sample before converting it to
short in ToneGenerator.ToneOut.ToneGenerator, where the sample is computed from
value and assigned to samples[i]. The current cast can wrap when
ToneProfile.Amplitude drives the signal beyond the short range, so add a clamp
to the valid short bounds before the assignment. Keep the fix localized to the
sample generation path so the tone output clips safely instead of flipping sign.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 54-57: The runtime directory setup is too permissive because it
uses world-writable permissions for /app/publish/data and
/app/publish/recordings. Update the Dockerfile’s directory-creation step to
grant write access by changing ownership to the DHI runtime user (or, if
appropriate, use tighter group-based permissions like 0770) instead of chmod
0777, so the non-root container user can write without exposing the dirs to
every account.
In `@Resgrid.Audio.Voice/Dsp/CtcssDetector.cs`:
- Around line 40-49: The Process hot path in CtcssDetector is doing per-sample
List<short>.Add plus per-block GetRange(...).ToArray() and RemoveRange(...)
shifts, which creates allocations and copies on the live audio path. Refactor
the sliding window logic in Process to use a ring buffer or Span-based window
over pcm/_windowSamples so block extraction and 50% overlap reuse storage
without allocating or shifting the underlying buffer. Keep the existing
Goertzel.NormalizedStrength and _present update behavior, but change how the
block data is buffered and read.
In `@Resgrid.Audio.Voice/Dsp/Mdc1200Decoder.cs`:
- Around line 36-53: `Mdc1200Decoder.Process` is doing an expensive full-buffer
copy and full `TryDecode` on every frame once `_buffer` is populated, plus
per-sample `Add` and O(n) `RemoveRange`. Optimize the hot path by switching the
backing storage to a ring buffer or similar sliding window, and consider
decoding only on a decimated cadence instead of every call. Keep the existing
behavior of retaining `_tailSamples` for boundary-spanning packets, but reduce
repeated allocations and scans in `Process` and `Mdc1200Codec.TryDecode`.
In `@Resgrid.Audio.Voice/Recording/SqliteTransmissionLog.cs`:
- Around line 68-91: The SqliteCommand created in
SqliteTransmissionLog.Append/record persistence is never deterministically
disposed, leaving command and parameter resources alive on the hot path. Update
the command lifecycle in the method that builds cmd from conn.CreateCommand() so
it is wrapped in using or await using, and keep the existing parameter setup and
ExecuteNonQueryAsync call inside that scope to release native handles promptly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 892445a7-d7d1-4540-bbc9-5b2c322fe7b2
📒 Files selected for processing (74)
.github/workflows/dotnet.yml.gitignoreCLAUDE.mdDockerfileProviders/Resgrid.Providers.ApiClient/V4/CallsApi.csProviders/Resgrid.Providers.ApiClient/V4/Models/ActiveCallsModels.csProviders/Resgrid.Providers.ApiClient/V4/Models/VoiceModels.csProviders/Resgrid.Providers.ApiClient/V4/VoiceApi.csResgrid Audio.slnResgrid.Audio.Core/Radio/CarrierDetector.csResgrid.Audio.Core/Radio/IPttController.csResgrid.Audio.Core/Radio/IRadioDevice.csResgrid.Audio.Core/Radio/NAudioRadioDevice.csResgrid.Audio.Core/Radio/PttControllers.csResgrid.Audio.Core/Radio/RadioBridge.csResgrid.Audio.Core/Radio/RadioSettings.csResgrid.Audio.Core/Resgrid.Audio.Core.csprojResgrid.Audio.Relay.Console/Configuration/RelayHostOptions.csResgrid.Audio.Relay.Console/Configuration/RelayModeOptions.csResgrid.Audio.Relay.Console/LoclxTunnel.csResgrid.Audio.Relay.Console/Program.csResgrid.Audio.Relay.Console/Resgrid.Audio.Relay.Console.csprojResgrid.Audio.Relay.Console/Voice/DispatchVoiceMode.csResgrid.Audio.Relay.Console/Voice/RadioMode.csResgrid.Audio.Relay.Console/Voice/RecordMode.csResgrid.Audio.Relay.Console/Voice/VoiceModeRuntime.csResgrid.Audio.Tests/Resgrid.Audio.Tests.csprojResgrid.Audio.Voice.Tests/AudioTests.csResgrid.Audio.Voice.Tests/DspTests.csResgrid.Audio.Voice.Tests/RecordingTests.csResgrid.Audio.Voice.Tests/Resgrid.Audio.Voice.Tests.csprojResgrid.Audio.Voice/Abstractions/AudioFormat.csResgrid.Audio.Voice/Abstractions/IAudioPublisher.csResgrid.Audio.Voice/Abstractions/IVoiceRoomSession.csResgrid.Audio.Voice/Abstractions/IVoiceTransport.csResgrid.Audio.Voice/Abstractions/VoiceAudioFrame.csResgrid.Audio.Voice/Abstractions/VoiceChannel.csResgrid.Audio.Voice/Abstractions/VoiceParticipant.csResgrid.Audio.Voice/Audio/MuLaw.csResgrid.Audio.Voice/Audio/Resampler.csResgrid.Audio.Voice/Audio/WavIo.csResgrid.Audio.Voice/Connection/IVoiceChannelProvider.csResgrid.Audio.Voice/Connection/ResgridVoiceChannelProvider.csResgrid.Audio.Voice/Dsp/AudioFilters.csResgrid.Audio.Voice/Dsp/CtcssDetector.csResgrid.Audio.Voice/Dsp/EmergencyToneDetector.csResgrid.Audio.Voice/Dsp/Goertzel.csResgrid.Audio.Voice/Dsp/IEmergencyAlertSink.csResgrid.Audio.Voice/Dsp/Mdc1200.csResgrid.Audio.Voice/Dsp/Mdc1200Decoder.csResgrid.Audio.Voice/Dsp/SquelchGate.csResgrid.Audio.Voice/LiveKit/LiveKitAudioPublisher.csResgrid.Audio.Voice/LiveKit/LiveKitRoomSession.csResgrid.Audio.Voice/LiveKit/LiveKitVoiceTransport.csResgrid.Audio.Voice/Recording/ITransmissionStore.csResgrid.Audio.Voice/Recording/JsonlTransmissionLog.csResgrid.Audio.Voice/Recording/LocalFileTransmissionStore.csResgrid.Audio.Voice/Recording/RecorderSettings.csResgrid.Audio.Voice/Recording/S3TransmissionStore.csResgrid.Audio.Voice/Recording/SqliteTransmissionLog.csResgrid.Audio.Voice/Recording/TransmissionRecord.csResgrid.Audio.Voice/Recording/TransmissionRecorder.csResgrid.Audio.Voice/Resgrid.Audio.Voice.csprojResgrid.Audio.Voice/ToneOut/DispatchToneOutService.csResgrid.Audio.Voice/ToneOut/HostedDepartmentVoiceResolver.csResgrid.Audio.Voice/ToneOut/IDepartmentVoiceResolver.csResgrid.Audio.Voice/ToneOut/ITextToSpeech.csResgrid.Audio.Voice/ToneOut/ResgridTtsClient.csResgrid.Audio.Voice/ToneOut/ToneGenerator.csResgrid.Audio.Voice/ToneOut/ToneProfile.csResgrid.Audio.Voice/ToneOut/TtsSettings.csResgrid.Audio.Voice/VoiceRoomManager.csdocker-entrypoint.shscripts/download-livekit-ffi.sh
💤 Files with no reviewable changes (1)
- docker-entrypoint.sh
| .vs/ | ||
| /.idea | ||
| /.dual-graph-pro | ||
| .mcp.json |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Anchor the .mcp.json ignore rule if this is meant to be workspace-local.
As written, it will ignore any .mcp.json anywhere in the tree, which can hide nested fixtures or examples unintentionally. If you only mean the repo-root config, use /.mcp.json.
🔧 Proposed fix
-.mcp.json
+/.mcp.json📝 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.
Suggested change
| .mcp.json | |
| /.mcp.json |
🤖 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 @.gitignore at line 115, The .mcp.json ignore rule is too broad and will
match nested files anywhere in the tree. Update the ignore entry in .gitignore
to be anchored to the repository root so it only applies to the workspace-local
config, using the existing .mcp.json rule location as the reference.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Dockerfile (1)
53-54: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid world-writable token and recording directories.
./datacontains the persisted Resgrid token cache, sochmod -R 0777lets any container user read or replace tokens and recordings. Prefer setting ownership to the DHI runtime UID/GID and narrowing this to0700/0770.Also applies to: 164-164
🤖 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 `@Dockerfile` around lines 53 - 54, The Dockerfile is making the persisted token cache and recordings world-writable, which is too permissive for sensitive runtime data. Update the setup around the publish data/recordings directories to assign ownership to the DHI runtime UID/GID instead of relying on chmod -R 0777, and restrict permissions to 0700 for the token cache and 0770 for shared recordings. Apply the same permission hardening in the other matching Dockerfile section referenced by the review, using the existing directory setup steps as the place to change this.Resgrid.Relay.Engine/LoclxTunnel.cs (2)
31-39: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn
nullwhen tunnel startup fails.This catch block logs the failure but still returns a non-null
LoclxTunnel, so callers can't distinguish “disabled” from “enabled but failed to start.” That makes tunnel startup look successful when no child process is running.Suggested fix
catch (Exception ex) { Cli.Error.WriteLine($"[localxpose] Failed to start tunnel: {ex.Message}"); + tunnel.Dispose(); + return null; } return tunnel;🤖 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 `@Resgrid.Relay.Engine/LoclxTunnel.cs` around lines 31 - 39, The LoclxTunnel startup path logs the exception but still returns the tunnel instance, which makes a failed start look successful. Update the try/catch around tunnel.Start(options) in the LoclxTunnel constructor/path so that a startup failure returns null instead of the partially initialized tunnel. Use the existing Cli.Error.WriteLine logging, and make sure callers can distinguish a disabled tunnel from one that failed to launch.
52-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't ignore
loclx auth logintimeouts or failures.
WaitForExit(15000)returnsfalseon timeout, and disposingauthdoes not kill the hung subprocess. Right now a stuck or failed auth step can leave an orphanedloclx auth loginprocess behind and still continue into tunnel startup as if authentication succeeded.Suggested fix
Environment.SetEnvironmentVariable("LX_ACCESS_TOKEN", token); using var auth = StartLoclx("auth login"); - auth?.WaitForExit(15000); + if (auth == null || !auth.WaitForExit(15000)) + { + auth?.Kill(entireProcessTree: true); + throw new TimeoutException("loclx auth login timed out."); + } + + if (auth.ExitCode != 0) + throw new InvalidOperationException($"loclx auth login failed with exit code {auth.ExitCode}."); }🤖 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 `@Resgrid.Relay.Engine/LoclxTunnel.cs` around lines 52 - 53, The auth step in StartLoclx("auth login") is being ignored when WaitForExit(15000) times out or fails, so LoclxTunnel can continue as if login succeeded. Update the auth flow in LoclxTunnel to check the WaitForExit result (and exit code if available), and treat timeout/failure as a hard error instead of proceeding. If auth does not complete successfully, explicitly terminate the auth process before disposing it and abort tunnel startup.Resgrid.Relay.Engine/Voice/RecordMode.cs (1)
81-88: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
options.S3before readingBucket.When
Storeiss3orbothand theS3section is missing, Line 84 throws aNullReferenceExceptioninstead of the intended configuration error.Suggested change
if (kind == "s3" || kind == "both") { var s3 = options.S3; + if (s3 == null) + throw new InvalidOperationException("Recorder S3 store selected but RESGRID__RELAY__Recorder__S3 is not configured."); if (string.IsNullOrWhiteSpace(s3.Bucket)) throw new InvalidOperationException("Recorder S3 store selected but RESGRID__RELAY__Recorder__S3__Bucket is not set.");🤖 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 `@Resgrid.Relay.Engine/Voice/RecordMode.cs` around lines 81 - 88, Guard options.S3 before accessing its properties in RecordMode so the configuration path fails cleanly instead of throwing a NullReferenceException. In the branch that handles kind == "s3" || kind == "both", add an explicit null/empty check for options.S3 before reading Bucket, and throw the existing InvalidOperationException with the same configuration message if the S3 section is missing. Keep the fix localized to the RecordMode logic around S3TransmissionStore.Create and the S3 settings validation.Resgrid.Relay.Engine/Voice/RadioMode.cs (1)
93-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
RunTuneAsyncis silent withLogger.None.All of the meter and startup messages on Lines 112-117 are dropped, so tune mode no longer shows the live level display or operator guidance. Pass a real
ILoggerintoRunTuneAsyncor write these messages directly to stdout.🤖 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 `@Resgrid.Relay.Engine/Voice/RadioMode.cs` around lines 93 - 121, RunTuneAsync is using Logger.None, which suppresses the meter and startup guidance emitted from the SamplesReceived handler and the logger.Information calls. Update RadioMode.RunTuneAsync to accept or resolve a real ILogger/Serilog logger, and use that logger for the live level display and tuning instructions so tune mode remains visible to the operator.
🧹 Nitpick comments (2)
Resgrid.Relay.Engine/Configuration/RelayModeOptions.cs (1)
53-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer enums or binding-time validation for closed-set options.
PttMethod,CarrierDetect,Store, andLogare all documented as finite value sets, but they're modeled as free-form strings. A config typo here will bind cleanly and only fail much later in the mode runtime. Tightening these to enums now would make the new config surface a lot safer.Example refactor
+public enum PttMethod +{ + Vox, + SerialRts, + SerialDtr, + Cm108 +} + +public enum CarrierDetectMode +{ + None, + SerialCts, + SerialDsr, + Cm108Gpio +} + public sealed class RadioModeOptions { - public string PttMethod { get; set; } = "Vox"; + public PttMethod PttMethod { get; set; } = PttMethod.Vox; ... - public string CarrierDetect { get; set; } = "None"; + public CarrierDetectMode CarrierDetect { get; set; } = CarrierDetectMode.None; }Also applies to: 102-107
🤖 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 `@Resgrid.Relay.Engine/Configuration/RelayModeOptions.cs` around lines 53 - 63, The closed-set relay config fields in RelayModeOptions are still modeled as free-form strings, so typos will bind silently and fail later in runtime. Tighten PttMethod and CarrierDetect here, and also the related Store and Log options mentioned in the same config surface, by switching them to enums or adding binding-time validation. Update the corresponding configuration model and any consumers that read these properties so the allowed values are enforced when options are bound.Resgrid.Relay.Engine/Voice/DispatchVoiceMode.cs (1)
53-58: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrune
seento the current active-call set.
seenonly grows, so a long-running relay retains every historicalCallIdfor the life of the process. Since each poll already fetches the full active set, intersectingseenwith the current ids before announcing avoids unbounded growth.Suggested change
while (!cancellationToken.IsCancellationRequested) { try { var calls = await callsApi.GetActiveCallsAsync(deptId, cancellationToken).ConfigureAwait(false); + var activeIds = new HashSet<string>( + calls.Where(c => !string.IsNullOrWhiteSpace(c.CallId)).Select(c => c.CallId), + StringComparer.Ordinal); + seen.IntersectWith(activeIds); + foreach (var call in calls) { if (string.IsNullOrWhiteSpace(call.CallId) || seen.Contains(call.CallId)) continue;Also applies to: 66-79
🤖 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 `@Resgrid.Relay.Engine/Voice/DispatchVoiceMode.cs` around lines 53 - 58, `seen` in DispatchVoiceMode only accumulates CallId values and can grow without bound in a long-running relay. Update the polling flow in DispatchVoiceMode to prune `seen` against the current result of GetActiveCallsAsync before announcing new calls, so it only retains ids that are still active. Use the existing `seen` HashSet and the active-call enumeration in the main polling loop (including the announcement path around the later block referenced by the review) to remove stale ids after each poll.
🤖 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 `@Dockerfile`:
- Around line 121-130: The Dockerfile download step for /wait should require a
checksum on arm64 and fail on HTTP errors. Update the WAIT_SHA256_ARM64 handling
in the RUN block that selects asset/sha so arm64 cannot proceed without a
populated hash, and change the curl invocation in that same block to use
fail-fast behavior. Keep the integrity check tied to the existing sha256sum
validation path so both amd64 and arm64 downloads are always verified.
In `@Providers/Resgrid.Providers.ApiClient/V4/ResgridV4ApiClient.cs`:
- Around line 49-56: `ResgridV4ApiClient.CurrentUserId` is incorrectly falling
back to `DepartmentId` in `SystemApiKey` mode, which causes `ComService` to pass
a non-user value into `SaveCallFileInput.UserId`. Update `CurrentUserId` in
`ResgridV4ApiClient` so it only returns the token-backed user identifier from
`_tokenState` (the `sub` claim) and never substitutes the department id; if
department-scoped uploads are needed, add a separate path or property for that
behavior and keep the user id flow unchanged.
In `@Resgrid.Audio.Core/ComService.cs`:
- Around line 27-33: ComService currently accepts required dependencies without
validation, so wiring mistakes can surface later as NullReferenceException. Add
constructor null guards in ComService for logger, audioProcessor, apiClient,
healthApi, and callsApi so the constructor fails immediately with clear
ArgumentNullException behavior before any of these fields are used.
In `@Resgrid.Relay.Engine/Smtp/SmtpRelayInfrastructure.cs`:
- Around line 123-130: Reject invalid constructor input combinations in
RelayMessageStore before creating CachedLookupsService: if callsClient is
provided without apiClient, and if both apiClient and callsClient are null,
throw an ArgumentException/ArgumentNullException up front. Update the
RelayMessageStore constructor in SmtpRelayInfrastructure to validate these
prerequisites before assigning _callsClient, _lookupCache, and _lookupService so
ResolveDispatchCodes never wires a null lookupsApi path that later fails inside
CachedLookupsService.
In `@Resgrid.Relay.Engine/Voice/RadioMode.cs`:
- Around line 63-64: The RadioMode session setup is disposing the same
RadioBridge instance twice because await using var bridge already handles async
cleanup at scope exit, and the explicit DisposeAsync call later repeats
teardown. Update RadioMode to rely on a single disposal path by removing the
manual DisposeAsync call and keeping the await using scope around the bridge
lifecycle so StartAsync still runs with the same bridge instance.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 53-54: The Dockerfile is making the persisted token cache and
recordings world-writable, which is too permissive for sensitive runtime data.
Update the setup around the publish data/recordings directories to assign
ownership to the DHI runtime UID/GID instead of relying on chmod -R 0777, and
restrict permissions to 0700 for the token cache and 0770 for shared recordings.
Apply the same permission hardening in the other matching Dockerfile section
referenced by the review, using the existing directory setup steps as the place
to change this.
In `@Resgrid.Relay.Engine/LoclxTunnel.cs`:
- Around line 31-39: The LoclxTunnel startup path logs the exception but still
returns the tunnel instance, which makes a failed start look successful. Update
the try/catch around tunnel.Start(options) in the LoclxTunnel constructor/path
so that a startup failure returns null instead of the partially initialized
tunnel. Use the existing Cli.Error.WriteLine logging, and make sure callers can
distinguish a disabled tunnel from one that failed to launch.
- Around line 52-53: The auth step in StartLoclx("auth login") is being ignored
when WaitForExit(15000) times out or fails, so LoclxTunnel can continue as if
login succeeded. Update the auth flow in LoclxTunnel to check the WaitForExit
result (and exit code if available), and treat timeout/failure as a hard error
instead of proceeding. If auth does not complete successfully, explicitly
terminate the auth process before disposing it and abort tunnel startup.
In `@Resgrid.Relay.Engine/Voice/RadioMode.cs`:
- Around line 93-121: RunTuneAsync is using Logger.None, which suppresses the
meter and startup guidance emitted from the SamplesReceived handler and the
logger.Information calls. Update RadioMode.RunTuneAsync to accept or resolve a
real ILogger/Serilog logger, and use that logger for the live level display and
tuning instructions so tune mode remains visible to the operator.
In `@Resgrid.Relay.Engine/Voice/RecordMode.cs`:
- Around line 81-88: Guard options.S3 before accessing its properties in
RecordMode so the configuration path fails cleanly instead of throwing a
NullReferenceException. In the branch that handles kind == "s3" || kind ==
"both", add an explicit null/empty check for options.S3 before reading Bucket,
and throw the existing InvalidOperationException with the same configuration
message if the S3 section is missing. Keep the fix localized to the RecordMode
logic around S3TransmissionStore.Create and the S3 settings validation.
---
Nitpick comments:
In `@Resgrid.Relay.Engine/Configuration/RelayModeOptions.cs`:
- Around line 53-63: The closed-set relay config fields in RelayModeOptions are
still modeled as free-form strings, so typos will bind silently and fail later
in runtime. Tighten PttMethod and CarrierDetect here, and also the related Store
and Log options mentioned in the same config surface, by switching them to enums
or adding binding-time validation. Update the corresponding configuration model
and any consumers that read these properties so the allowed values are enforced
when options are bound.
In `@Resgrid.Relay.Engine/Voice/DispatchVoiceMode.cs`:
- Around line 53-58: `seen` in DispatchVoiceMode only accumulates CallId values
and can grow without bound in a long-running relay. Update the polling flow in
DispatchVoiceMode to prune `seen` against the current result of
GetActiveCallsAsync before announcing new calls, so it only retains ids that are
still active. Use the existing `seen` HashSet and the active-call enumeration in
the main polling loop (including the announcement path around the later block
referenced by the review) to remove stale ids after each poll.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cbd57a9-37f9-4ddb-879f-e25b73ba605e
📒 Files selected for processing (45)
.github/workflows/dotnet.ymlDockerfileProviders/Resgrid.Providers.ApiClient/V4/CallsApi.csProviders/Resgrid.Providers.ApiClient/V4/HealthApi.csProviders/Resgrid.Providers.ApiClient/V4/IResgridApiClient.csProviders/Resgrid.Providers.ApiClient/V4/LookupsApi.csProviders/Resgrid.Providers.ApiClient/V4/ResgridApiClientOptions.csProviders/Resgrid.Providers.ApiClient/V4/ResgridV4ApiClient.csProviders/Resgrid.Providers.ApiClient/V4/VoiceApi.csResgrid Audio.slnResgrid.Audio.Core/ComService.csResgrid.Audio.Relay.Console/Program.csResgrid.Audio.Relay.Console/Resgrid.Audio.Relay.Console.csprojResgrid.Audio.Tests/Resgrid.Audio.Tests.csprojResgrid.Audio.Tests/SmtpDispatchAddressParserTests.csResgrid.Audio.Tests/SmtpRelayTelemetryTests.csResgrid.Audio.Voice/Audio/Resampler.csResgrid.Audio.Voice/Connection/ResgridVoiceChannelProvider.csResgrid.Audio.Voice/Dsp/IEmergencyAlertSink.csResgrid.Audio.Voice/LiveKit/LiveKitAudioPublisher.csResgrid.Audio.Voice/LiveKit/LiveKitRoomSession.csResgrid.Audio.Voice/Recording/TransmissionRecorder.csResgrid.Audio.Voice/ToneOut/DispatchToneOutService.csResgrid.Audio.Voice/ToneOut/IDepartmentVoiceResolver.csResgrid.Audio.Voice/ToneOut/ToneGenerator.csResgrid.Relay.Engine/Abstractions/ConnectionState.csResgrid.Relay.Engine/Abstractions/IRelayService.csResgrid.Relay.Engine/Abstractions/IRelayStatus.csResgrid.Relay.Engine/Abstractions/RelayServiceState.csResgrid.Relay.Engine/Abstractions/RelayStateChangedEventArgs.csResgrid.Relay.Engine/Configuration/RelayHostOptions.csResgrid.Relay.Engine/Configuration/RelayModeOptions.csResgrid.Relay.Engine/LoclxTunnel.csResgrid.Relay.Engine/Resgrid.Relay.Engine.csprojResgrid.Relay.Engine/Smtp/CachedLookupsService.csResgrid.Relay.Engine/Smtp/IDispatchLookupCache.csResgrid.Relay.Engine/Smtp/NullDispatchLookupCache.csResgrid.Relay.Engine/Smtp/RedisDispatchLookupCache.csResgrid.Relay.Engine/Smtp/SmtpDispatchAddressParser.csResgrid.Relay.Engine/Smtp/SmtpRelayInfrastructure.csResgrid.Relay.Engine/Smtp/SmtpTelemetry.csResgrid.Relay.Engine/Voice/DispatchVoiceMode.csResgrid.Relay.Engine/Voice/RadioMode.csResgrid.Relay.Engine/Voice/RecordMode.csResgrid.Relay.Engine/Voice/VoiceModeRuntime.cs
✅ Files skipped from review due to trivial changes (5)
- Resgrid.Relay.Engine/Abstractions/ConnectionState.cs
- Resgrid.Relay.Engine/Abstractions/RelayServiceState.cs
- Resgrid.Audio.Tests/SmtpDispatchAddressParserTests.cs
- Resgrid.Relay.Engine/Smtp/IDispatchLookupCache.cs
- Resgrid.Relay.Engine/Abstractions/IRelayStatus.cs
🚧 Files skipped from review as they are similar to previous changes (14)
- Resgrid.Audio.Tests/Resgrid.Audio.Tests.csproj
- Resgrid.Audio.Voice/ToneOut/IDepartmentVoiceResolver.cs
- .github/workflows/dotnet.yml
- Resgrid.Audio.Voice/ToneOut/ToneGenerator.cs
- Resgrid.Audio.Relay.Console/Resgrid.Audio.Relay.Console.csproj
- Resgrid.Audio.Voice/Connection/ResgridVoiceChannelProvider.cs
- Resgrid.Audio.Voice/Dsp/IEmergencyAlertSink.cs
- Resgrid.Audio.Voice/ToneOut/DispatchToneOutService.cs
- Resgrid.Audio.Voice/Audio/Resampler.cs
- Resgrid.Audio.Voice/LiveKit/LiveKitAudioPublisher.cs
- Resgrid.Audio.Voice/Recording/TransmissionRecorder.cs
- Resgrid.Audio.Relay.Console/Program.cs
- Resgrid.Audio.Voice/LiveKit/LiveKitRoomSession.cs
- Resgrid Audio.sln
Comment on lines
+121
to
+130
| ARG WAIT_SHA256_ARM64= | ||
| RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates \ | ||
| && case "${TARGETARCH}" in \ | ||
| amd64) asset="wait"; sha="${WAIT_SHA256_AMD64}" ;; \ | ||
| arm64) asset="wait_aarch64"; sha="${WAIT_SHA256_ARM64}" ;; \ | ||
| *) echo "Unsupported arch: ${TARGETARCH}" >&2 ; exit 1 ;; \ | ||
| esac \ | ||
| && curl -sSL -o /wait \ | ||
| "https://github.com/ufoscout/docker-compose-wait/releases/download/${WAIT_VERSION}/${asset}" \ | ||
| && if [ -n "${sha}" ]; then echo "${sha} /wait" | sha256sum -c - ; fi \ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL -o "$tmp/wait_aarch64" \
"https://github.com/ufoscout/docker-compose-wait/releases/download/2.12.1/wait_aarch64"
sha256sum "$tmp/wait_aarch64"Repository: Resgrid/Relay
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the Dockerfile section around the reported lines.
git ls-files Dockerfile
wc -l Dockerfile
sed -n '110,140p' Dockerfile
# Search for related checksum arguments/usages.
rg -n "WAIT_SHA256|WAIT_VERSION|docker-compose-wait|curl -sSL|curl -fsSL" DockerfileRepository: Resgrid/Relay
Length of output: 2836
Require the arm64 checksum and fail on HTTP errors.
Dockerfile:121-130 leaves WAIT_SHA256_ARM64 empty, so arm64 downloads can skip integrity checking, and curl -sSL won’t fail on HTTP error responses. Make the arm64 hash mandatory and switch to curl -fsSL so the downloaded /wait is always validated.
🤖 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 `@Dockerfile` around lines 121 - 130, The Dockerfile download step for /wait
should require a checksum on arm64 and fail on HTTP errors. Update the
WAIT_SHA256_ARM64 handling in the RUN block that selects asset/sha so arm64
cannot proceed without a populated hash, and change the curl invocation in that
same block to use fail-fast behavior. Keep the integrity check tied to the
existing sha256sum validation path so both amd64 and arm64 downloads are always
verified.
Comment on lines
+49
to
+56
| public string CurrentUserId | ||
| { | ||
| get | ||
| { | ||
| if (_options.GrantType == ResgridAuthGrantType.SystemApiKey && !String.IsNullOrWhiteSpace(_options.DepartmentId)) | ||
| return _options.DepartmentId; | ||
|
|
||
| return _tokenState?.UserId; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don’t substitute DepartmentId for CurrentUserId.
Line 53 makes CurrentUserId return a department id in SystemApiKey mode, but ComService passes this value into SaveCallFileInput.UserId. That can misattribute or fail file uploads because a department id is not a user id. Keep CurrentUserId limited to the token sub claim, and model the system-key/department-scoped upload path separately.
As per coding guidelines, "ResgridV4ApiClient handles ... extraction of CurrentUserId from the access token sub claim for file uploads."
🤖 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 `@Providers/Resgrid.Providers.ApiClient/V4/ResgridV4ApiClient.cs` around lines
49 - 56, `ResgridV4ApiClient.CurrentUserId` is incorrectly falling back to
`DepartmentId` in `SystemApiKey` mode, which causes `ComService` to pass a
non-user value into `SaveCallFileInput.UserId`. Update `CurrentUserId` in
`ResgridV4ApiClient` so it only returns the token-backed user identifier from
`_tokenState` (the `sub` claim) and never substitutes the department id; if
department-scoped uploads are needed, add a separate path or property for that
behavior and keep the user id flow unchanged.
Source: Coding guidelines
Comment on lines
+27
to
+33
| public ComService(Logger logger, AudioProcessor audioProcessor, IResgridApiClient apiClient, IResgridHealthApi healthApi, IResgridCallsApi callsApi) | ||
| { | ||
| _logger = logger; | ||
| _audioProcessor = audioProcessor; | ||
| _apiClient = apiClient; | ||
| _healthApi = healthApi; | ||
| _callsApi = callsApi; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Fail fast on missing injected dependencies.
These new required dependencies are dereferenced later; add constructor null guards so wiring errors fail with clear ArgumentNullExceptions instead of later NullReferenceExceptions.
Proposed fix
- _logger = logger;
- _audioProcessor = audioProcessor;
- _apiClient = apiClient;
- _healthApi = healthApi;
- _callsApi = callsApi;
+ _logger = logger ?? throw new ArgumentNullException(nameof(logger));
+ _audioProcessor = audioProcessor ?? throw new ArgumentNullException(nameof(audioProcessor));
+ _apiClient = apiClient ?? throw new ArgumentNullException(nameof(apiClient));
+ _healthApi = healthApi ?? throw new ArgumentNullException(nameof(healthApi));
+ _callsApi = callsApi ?? throw new ArgumentNullException(nameof(callsApi));📝 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.
Suggested change
| public ComService(Logger logger, AudioProcessor audioProcessor, IResgridApiClient apiClient, IResgridHealthApi healthApi, IResgridCallsApi callsApi) | |
| { | |
| _logger = logger; | |
| _audioProcessor = audioProcessor; | |
| _apiClient = apiClient; | |
| _healthApi = healthApi; | |
| _callsApi = callsApi; | |
| public ComService(Logger logger, AudioProcessor audioProcessor, IResgridApiClient apiClient, IResgridHealthApi healthApi, IResgridCallsApi callsApi) | |
| { | |
| _logger = logger ?? throw new ArgumentNullException(nameof(logger)); | |
| _audioProcessor = audioProcessor ?? throw new ArgumentNullException(nameof(audioProcessor)); | |
| _apiClient = apiClient ?? throw new ArgumentNullException(nameof(apiClient)); | |
| _healthApi = healthApi ?? throw new ArgumentNullException(nameof(healthApi)); | |
| _callsApi = callsApi ?? throw new ArgumentNullException(nameof(callsApi)); |
🤖 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 `@Resgrid.Audio.Core/ComService.cs` around lines 27 - 33, ComService currently
accepts required dependencies without validation, so wiring mistakes can surface
later as NullReferenceException. Add constructor null guards in ComService for
logger, audioProcessor, apiClient, healthApi, and callsApi so the constructor
fails immediately with clear ArgumentNullException behavior before any of these
fields are used.
Comment on lines
+123
to
+130
| public RelayMessageStore(SmtpRelayOptions options, ISmtpTelemetry telemetry, IResgridApiClient apiClient = null, IResgridCallsClient callsClient = null, IDispatchLookupCache lookupCache = null, IResgridLookupsApi lookupsApi = null) | ||
| { | ||
| _options = options ?? throw new ArgumentNullException(nameof(options)); | ||
| _telemetry = telemetry ?? throw new ArgumentNullException(nameof(telemetry)); | ||
| _callsClient = callsClient ?? new ResgridCallsClient(); | ||
| _callsClient = callsClient ?? new ResgridCallsClient(apiClient); | ||
| _lookupCache = lookupCache ?? CreateLookupCache(options); | ||
| _lookupService = new CachedLookupsService(_lookupCache); | ||
| lookupsApi ??= apiClient != null ? new LookupsApi(apiClient) : null; | ||
| _lookupService = new CachedLookupsService(_lookupCache, lookupsApi); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the constructor prerequisites before wiring the lookup service.
This overload allows callsClient-only construction, but still creates CachedLookupsService with lookupsApi == null. If ResolveDispatchCodes is enabled, the first cache miss will throw from _lookupsApi.Lookup*Async(...). The other invalid combination (apiClient == null && callsClient == null) also fails indirectly in new ResgridCallsClient(apiClient), so both states should be rejected up front.
Suggested fix
public RelayMessageStore(SmtpRelayOptions options, ISmtpTelemetry telemetry, IResgridApiClient apiClient = null, IResgridCallsClient callsClient = null, IDispatchLookupCache lookupCache = null, IResgridLookupsApi lookupsApi = null)
{
_options = options ?? throw new ArgumentNullException(nameof(options));
_telemetry = telemetry ?? throw new ArgumentNullException(nameof(telemetry));
+
+ if (apiClient == null && callsClient == null)
+ throw new ArgumentNullException(nameof(apiClient), "Provide either an apiClient or a callsClient.");
+
+ if (options.ResolveDispatchCodes && apiClient == null && lookupsApi == null)
+ throw new ArgumentException("ResolveDispatchCodes requires either an apiClient or a lookupsApi.", nameof(lookupsApi));
+
_callsClient = callsClient ?? new ResgridCallsClient(apiClient);
_lookupCache = lookupCache ?? CreateLookupCache(options);
lookupsApi ??= apiClient != null ? new LookupsApi(apiClient) : null;
_lookupService = new CachedLookupsService(_lookupCache, lookupsApi);
_dispatchAddressParser = new SmtpDispatchAddressParser(options);📝 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.
Suggested change
| public RelayMessageStore(SmtpRelayOptions options, ISmtpTelemetry telemetry, IResgridApiClient apiClient = null, IResgridCallsClient callsClient = null, IDispatchLookupCache lookupCache = null, IResgridLookupsApi lookupsApi = null) | |
| { | |
| _options = options ?? throw new ArgumentNullException(nameof(options)); | |
| _telemetry = telemetry ?? throw new ArgumentNullException(nameof(telemetry)); | |
| _callsClient = callsClient ?? new ResgridCallsClient(); | |
| _callsClient = callsClient ?? new ResgridCallsClient(apiClient); | |
| _lookupCache = lookupCache ?? CreateLookupCache(options); | |
| _lookupService = new CachedLookupsService(_lookupCache); | |
| lookupsApi ??= apiClient != null ? new LookupsApi(apiClient) : null; | |
| _lookupService = new CachedLookupsService(_lookupCache, lookupsApi); | |
| public RelayMessageStore(SmtpRelayOptions options, ISmtpTelemetry telemetry, IResgridApiClient apiClient = null, IResgridCallsClient callsClient = null, IDispatchLookupCache lookupCache = null, IResgridLookupsApi lookupsApi = null) | |
| { | |
| _options = options ?? throw new ArgumentNullException(nameof(options)); | |
| _telemetry = telemetry ?? throw new ArgumentNullException(nameof(telemetry)); | |
| if (apiClient == null && callsClient == null) | |
| throw new ArgumentNullException(nameof(apiClient), "Provide either an apiClient or a callsClient."); | |
| if (options.ResolveDispatchCodes && apiClient == null && lookupsApi == null) | |
| throw new ArgumentException("ResolveDispatchCodes requires either an apiClient or a lookupsApi.", nameof(lookupsApi)); | |
| _callsClient = callsClient ?? new ResgridCallsClient(apiClient); | |
| _lookupCache = lookupCache ?? CreateLookupCache(options); | |
| lookupsApi ??= apiClient != null ? new LookupsApi(apiClient) : null; | |
| _lookupService = new CachedLookupsService(_lookupCache, lookupsApi); |
🤖 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 `@Resgrid.Relay.Engine/Smtp/SmtpRelayInfrastructure.cs` around lines 123 - 130,
Reject invalid constructor input combinations in RelayMessageStore before
creating CachedLookupsService: if callsClient is provided without apiClient, and
if both apiClient and callsClient are null, throw an
ArgumentException/ArgumentNullException up front. Update the RelayMessageStore
constructor in SmtpRelayInfrastructure to validate these prerequisites before
assigning _callsClient, _lookupCache, and _lookupService so ResolveDispatchCodes
never wires a null lookupsApi path that later fails inside CachedLookupsService.
Comment on lines
+63
to
+64
| await using var bridge = new RadioBridge(device, ptt, carrier, radioSettings, logger, mdc, emergency, alertSink); | ||
| await bridge.StartAsync(session, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
bridge is disposed twice.
await using var bridge already schedules an async dispose at scope exit, so the explicit DisposeAsync on Line 88 runs a second teardown. If RadioBridge.DisposeAsync is not idempotent, shutdown can fail or double-release resources.
Suggested change
- await bridge.DisposeAsync().ConfigureAwait(false);
return 0;Also applies to: 88-88
🤖 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 `@Resgrid.Relay.Engine/Voice/RadioMode.cs` around lines 63 - 64, The RadioMode
session setup is disposing the same RadioBridge instance twice because await
using var bridge already handles async cleanup at scope exit, and the explicit
DisposeAsync call later repeats teardown. Update RadioMode to rely on a single
disposal path by removing the manual DisposeAsync call and keeping the await
using scope around the bridge lifecycle so StartAsync still runs with the same
bridge instance.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes