Devana fixes#17
Merged
Merged
Conversation
The embedded Apple Speech helper path was cached in a OnceLock after first materialization, so later transcriptions returned the cached path without re-running helper_needs_refresh(). A same-user attacker could replace the on-disk helper at its predictable temp path and have Muninn execute the tampered binary on the next dictation. Remove the OnceLock cache and re-validate the on-disk bytes against the embedded helper on every call, rewriting atomically when they differ. The HELPER_INIT mutex still serializes concurrent refreshes. Ref: .devana/...-P0-apple-speech-helper-toctou.md
…ty grant (P1) should_abort_injection aborted whenever Accessibility was requested during the interaction, even when the refreshed preflight already reported it granted. That returned Ok(()) without injecting, then process_and_inject deleted the temp WAV — discarding a fully transcribed utterance with no retry path, forcing re-dictation. Since the refreshed preflight confirms AXIsProcessTrusted is now true, injection will succeed, so proceed instead of aborting. Genuine denials still abort. Update the encoding test accordingly and add a still-denied abort case. Ref: .devana/...-P1-accessibility-grant-loses-transcript.md
Replay persistence is enqueued to a background worker via try_send, but the runtime deleted the temp WAV synchronously right after injection. When replay_retain_audio was enabled the worker frequently found the source file already gone and silently skipped audio retention, defeating full-debug replay. Transfer WAV ownership to the worker when the request is accepted: enqueue now returns whether the worker took ownership, the worker deletes the WAV only after persist (and thus after audio retention), and the runtime deletes it itself only when the request was not enqueued (replay disabled, queue full, or disconnected). The service's Drop already joins the worker, so shutdown still persists+cleans up. Ref: .devana/...-P1-replay-audio-deleted-before-persist.md
README documents that full-debug replay redacts prompt fields, and the config and envelope snapshots already strip system_prompt, but replay_refine_context wrote the verbatim transcript.system_prompt into record.json under refine_context.system_prompt. That leaked private vocabulary, names, and project hints into replay directories that may be shared or backed up. Store the [redacted] marker instead of the prompt text, keeping the field present so the artifact still records that a refine prompt was configured. Update the regression test to assert redaction. Ref: .devana/...-P1-replay-prompt-leak-refine-context.md
…njection (P2) non_empty_text (orchestrator + scoring) and inject_checked only rejected zero-length strings, so a whitespace-only output.final_text was preferred over a usable transcript.raw_text and typed as blank output into the focused app. STT and refine paths already use trim-based emptiness, so behavior was inconsistent. Align all three with trim-based emptiness: whitespace-only text counts as absent for routing/scoring (falling back to raw_text) and inject_checked rejects it. Add orchestrator regression tests for whitespace fallback and no-injection. Ref: .devana/...-P2-whitespace-only-text-injected.md
…cording_enabled (P2) The README implied external 'toggle' starts recording when idle without the start_recording_enabled opt-in, but the code (correctly) gates it like 'start'. Leaving toggle ungated would let any external agent bypass the microphone opt-in by sending 'toggle' instead of 'start', defeating the flag's security purpose, so the gate is intentional and the docs were wrong. Update the README and the action.rs doc comments to state that starting capture from idle is gated for both Start and Toggle (the tray path forces the gate on because a local human action is trusted). Add a regression assertion that an idle external toggle is Disabled without the opt-in. Ref: .devana/...-P2-external-toggle-start-gate.md
…nch (P2) The muninn:// Apple Event handler was installed once at bootstrap when url_scheme_enabled was true, and dispatch forwarded every parsed verb without rechecking the flag. Disabling url_scheme_enabled via live config reload mutated in-memory config but left the handler active, so stop/toggle/cancel URLs from any local app still drove the runtime after an operator believed they were disabled. Gate URL dispatch on a runtime AtomicBool seeded from the launch config and refreshed on every ConfigReloaded event, and always install the handler so the flag is a real bidirectional runtime control surface. When disabled, parsed URLs are dropped with a log instead of reaching the worker. Ref: .devana/...-P2-url-scheme-disable-ignored-reload.md
…st the first (P2) finish() broke out of the post-commit read loop as soon as the first completed transcription segment arrived, so multi-segment/multi-part transcripts were truncated to the first segment and an empty-then-real completion sequence dropped the real text. The accumulator already joins all completed entries, but finish() never consumed the rest of the stream. Drain segments instead: wait (bounded by the upstream finish timeout) for the first completion, then briefly drain additional segments within a short grace before finalizing. Reading until socket close is not viable here because the realtime session is persistent and the upstream finish timeout would abort and discard the transcript. Add regression tests for multi-segment accumulation and empty-first-completion recovery. Ref: .devana/...-P2-openai-streaming-truncates-segments.md
…ently (P2) run_command wrote the full stdin payload and awaited shutdown before spawning the stdout/stderr drains and before arming timeout(timeout_budget, child.wait()). A child that emits output while reading (fills its stdout pipe and stops reading stdin) or that never reads stdin to EOF blocked the parent's write_all forever, and the timeout safety net was never reached — freezing the dictation pipeline. Spawn the stdout/stderr readers before writing stdin so the child stays unblocked, and drive the write/shutdown together with child.wait() under one timeout_budget (WritePhaseError maps the failing phase to its CommandErrorKind). Add regression tests for the large-stdin round-trip and the never-reads-stdin timeout. Mark the devana report fixed. Ref: .devana/...-P2-external-step-stdin-write-deadlock.md
… first (P2) extract_google_transcript_text flattened results[]->alternatives[]->transcript and took .next(), returning only the first segment's first alternative. Google's speech:recognize returns one results entry per consecutive audio segment for longer audio, so longer dictations were silently truncated to roughly the first segment with no error. Aggregate over all results, taking each result's first alternative and concatenating them (segments carry their own leading spaces), then trim — matching the Whisper and Deepgram adapters. Add regression tests for multi-segment concatenation and first-alternative selection. Mark the devana report fixed. Ref: .devana/...-P2-google-batch-multisegment-truncation.md
…ures (P3) Both the Deepgram and OpenAI streaming backends finalized with 'Message::Close(_) => break' and then returned into_outcome(), which builds an empty *success* outcome when no final text was seen. A server-initiated error close (auth rejected, quota/credits exhausted, policy/internal fault) was therefore reported to the user as 'no speech detected' instead of the real error, hiding actionable failures like an expired API key. Inspect the close frame: a non-benign code (not 1000 Normal / 1001 Going Away, and a frame is present) is captured as StreamingTranscriptionError::failed with the code and reason. The error is returned only when no usable final text was produced, so a transcript completed before a trailing error close still succeeds. Add regression tests for both backends. Mark the devana report fixed. Ref: .devana/...-P3-streaming-error-close-as-empty.md
The P0/P1/P2 reports whose fixes did not modify the report file itself; the three reports with an Agent Handoff section were committed alongside their fixes with status updated to fixed.
📝 WalkthroughWalkthroughThe PR adds documentation across runtime, config, and support modules, and changes several control flows: external-control URL handling and start-recording gating, accessibility and whitespace handling in injection, replay enqueue/cleanup ownership, runner subprocess cleanup, and streaming transcription finalization. ChangesRuntime and pipeline updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
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 |
🤖 Augment PR SummarySummary: This PR is a “Devana fixes” pass that tightens macOS runtime behavior and greatly expands inline documentation across Muninn. Changes:
Technical Notes: Several helpers now treat whitespace-only values as empty (e.g., injection routing and secret resolution) to avoid propagating blank output. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| .and_then(|alternative| alternative.get("transcript")) | ||
| .and_then(Value::as_str) | ||
| }) | ||
| .collect::<String>(); |
There was a problem hiding this comment.
src/stt_google_tool.rs:622 — collect::<String>() concatenates per-result transcripts without inserting separators, so multi-result responses that don't include leading/trailing whitespace can produce merged words (e.g., "foo" + "bar" -> "foobar"). Consider verifying this against real Google results[] payloads where segments often don't carry spacing.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| /// Queue replay persistence for one utterance. | ||
| /// | ||
| /// Returns `true` when the worker accepted the request and will delete the | ||
| /// temporary WAV after writing. Returns `false` when replay is disabled, the |
There was a problem hiding this comment.
src/replay_dispatch.rs:57 — The docstring says enqueue returns false when replay is disabled, but the implementation doesn't check logging.replay_enabled and can still return true if the request was accepted into the queue. This could mislead callers that use the boolean to decide who owns WAV cleanup.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| //! macOS TCC permission refresh before recording and injection. | ||
| //! | ||
| //! Hotkey recording requires Input Monitoring; tray and external controls use a | ||
| //! lighter preflight. Prompting during a user gesture may grant access but still |
There was a problem hiding this comment.
src/runtime_permissions.rs:4 — The module docs say a permission prompt during a user gesture “still aborts that gesture”, but should_abort_injection now proceeds when Accessibility was granted during the prompt. It may be worth narrowing this doc to recording-start behavior (or updating it to reflect injection’s new behavior).
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
src/lib.rs (1)
414-443: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake temporary indicator updates a required capability.
These new defaults only forward to
set_state, so any adapter that does not override them will latchCancelled/Outputinstead of reverting.src/runtime_flow.rsandsrc/runtime_pipeline.rsalready call these helpers as temporary transitions, so this weakens the cross-file contract in a user-visible way.Suggested fix
#[async_trait] pub trait IndicatorAdapter: Send + Sync { @@ - async fn set_temporary_state( - &mut self, - state: IndicatorState, - min_duration: Duration, - fallback_state: IndicatorState, - ) -> MacosAdapterResult<()> { - let _ = min_duration; - let _ = fallback_state; - self.set_state(state).await - } + async fn set_temporary_state( + &mut self, + state: IndicatorState, + min_duration: Duration, + fallback_state: IndicatorState, + ) -> MacosAdapterResult<()>; @@ - async fn set_temporary_state_with_glyph( - &mut self, - state: IndicatorState, - glyph: Option<char>, - min_duration: Duration, - fallback_state: IndicatorState, - fallback_glyph: Option<char>, - ) -> MacosAdapterResult<()> { - let _ = glyph; - let _ = fallback_glyph; - self.set_temporary_state(state, min_duration, fallback_state) - .await - } + async fn set_temporary_state_with_glyph( + &mut self, + state: IndicatorState, + glyph: Option<char>, + min_duration: Duration, + fallback_state: IndicatorState, + fallback_glyph: Option<char>, + ) -> MacosAdapterResult<()>; }🤖 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 `@src/lib.rs` around lines 414 - 443, Make temporary indicator updates a required adapter capability instead of a no-op default. Update the IndicatorAdapter trait methods set_temporary_state and set_temporary_state_with_glyph in src/lib.rs so they are no longer silently implemented by delegating to set_state; either make them required to override or provide behavior that truly reverts after min_duration. Ensure the runtime call sites in runtime_flow and runtime_pipeline still rely on these methods as temporary transitions, and adjust any concrete adapter implementations accordingly.src/runner/transport.rs (1)
142-180: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake timeout cleanup bounded too.
This branch times out
write_and_wait, but then immediately awaitscleanup_after_command_failure, which drains stdout/stderr with no deadline. If a timed-out step leaves a descendant holding either pipe open,run_commandcan still block indefinitely andPipelineRunner::runno longer honors the step/global timeout budget.🤖 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 `@src/runner/transport.rs` around lines 142 - 180, The timeout handling in `run_command` only bounds `write_and_wait`, but the `Err(_)` timeout branch still awaits `cleanup_after_command_failure` without any deadline, so the command can block indefinitely while draining pipes. Update the timeout path in `src/runner/transport.rs` so cleanup is also bounded by the remaining timeout budget, using the same `timeout_budget`/deadline logic around `cleanup_after_command_failure` and preserving the existing `CommandError` construction in `run_command`.
🧹 Nitpick comments (1)
src/replay_dispatch.rs (1)
54-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for the WAV ownership contract.
enqueue()now decides whethersrc/runtime_pipeline.rsdeletes the temp recording locally or leaves it to the worker. A focused test for accepted, full, and disconnected queue outcomes would make leaks and double-deletes much easier to catch on this cross-file 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 `@src/replay_dispatch.rs` around lines 54 - 102, Add a regression test around ReplayDispatch::enqueue to verify the WAV ownership contract across the src/runtime_pipeline.rs handoff. Cover the accepted, queue-full, and disconnected paths so the test asserts when enqueue returns true the worker owns cleanup, and when it returns false the caller still deletes the temp recording locally. Use the enqueue method and ReplayPersistRequest/Sender try_send behavior to locate the logic under test.
🤖 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 `@src/error.rs`:
- Around line 50-52: The MissingPermissions error variant is still formatting
permission names with debug output instead of the stable labels used elsewhere.
Update the Display implementation for MissingPermissions in error.rs so it
renders each PermissionKind via as_str() rather than using {:?}, keeping %error
logs consistent with the snake-case permission identifiers introduced in this
file.
In `@src/runner.rs`:
- Around line 337-338: Update the rustdoc on exit_status in RunnerTrace to match
the broader trace contract: it is not only for external child processes, since
run_in_process_step also sets Some(0) for successful builtin steps and
propagates InProcessStepError.exit_status on in-process failures. Adjust the
comment to describe exit_status as the step exit code when available, with None
only for timeout, and keep the wording aligned with run_in_process_step and
InProcessStepError.
In `@src/runtime_permissions.rs`:
- Around line 1-6: Update the module docs in runtime_permissions to match the
current permission flow: the top comment should no longer say prompt-driven
gestures always abort, since should_abort_injection() now continues when
Accessibility is granted in the same interaction. Also revise the comments
around should_abort_injection() and the Input Monitoring preflight so they only
describe Input Monitoring for RecordingStartSource::Hotkey, not tray/external
controls.
In `@src/runtime_shell.rs`:
- Around line 238-241: The MCP start_recording gating is still using the
startup-captured state after config reload, so update the server path to reflect
reloaded config. In runtime_shell::handle_config_reload (or the nearby reload
handling that calls set_url_scheme_enabled), make sure the MCP server state is
refreshed too—either by updating the stored start_recording_enabled flag or
recreating the MCP server instance—so both external_control and MCP honor the
new config consistently.
In `@src/stt_google_tool.rs`:
- Around line 614-623: The transcript assembly in the result-processing chain is
concatenating per-result segments directly, causing adjacent phrases to merge
without spaces. Update the logic around the filter_map/collect flow that builds
the transcript so it gathers each normalized segment separately and joins them
with a space instead of collecting into a single String. Keep the fix localized
to the transcript-building code in stt_google_tool.rs, near the `transcript`
construction and `raw_text` assignment.
---
Outside diff comments:
In `@src/lib.rs`:
- Around line 414-443: Make temporary indicator updates a required adapter
capability instead of a no-op default. Update the IndicatorAdapter trait methods
set_temporary_state and set_temporary_state_with_glyph in src/lib.rs so they are
no longer silently implemented by delegating to set_state; either make them
required to override or provide behavior that truly reverts after min_duration.
Ensure the runtime call sites in runtime_flow and runtime_pipeline still rely on
these methods as temporary transitions, and adjust any concrete adapter
implementations accordingly.
In `@src/runner/transport.rs`:
- Around line 142-180: The timeout handling in `run_command` only bounds
`write_and_wait`, but the `Err(_)` timeout branch still awaits
`cleanup_after_command_failure` without any deadline, so the command can block
indefinitely while draining pipes. Update the timeout path in
`src/runner/transport.rs` so cleanup is also bounded by the remaining timeout
budget, using the same `timeout_budget`/deadline logic around
`cleanup_after_command_failure` and preserving the existing `CommandError`
construction in `run_command`.
---
Nitpick comments:
In `@src/replay_dispatch.rs`:
- Around line 54-102: Add a regression test around ReplayDispatch::enqueue to
verify the WAV ownership contract across the src/runtime_pipeline.rs handoff.
Cover the accepted, queue-full, and disconnected paths so the test asserts when
enqueue returns true the worker owns cleanup, and when it returns false the
caller still deletes the temp recording locally. Use the enqueue method and
ReplayPersistRequest/Sender try_send behavior to locate the logic under test.
🪄 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: 30259651-e2ff-4f90-b817-42b8c3292c32
📒 Files selected for processing (51)
README.mdsrc/audio.rssrc/audio/render.rssrc/autostart.rssrc/config.rssrc/config/logging.rssrc/config_watch.rssrc/envelope.rssrc/error.rssrc/external_control.rssrc/external_control/action.rssrc/external_control/mcp.rssrc/external_control/status.rssrc/external_control/url_scheme.rssrc/hotkeys.rssrc/injector.rssrc/internal_tools.rssrc/lib.rssrc/logging.rssrc/main.rssrc/mock.rssrc/orchestrator.rssrc/permissions.rssrc/platform.rssrc/refine.rssrc/replay.rssrc/replay_dispatch.rssrc/runner.rssrc/runner/codec.rssrc/runner/execution.rssrc/runner/transport.rssrc/runtime_flow.rssrc/runtime_permissions.rssrc/runtime_pipeline.rssrc/runtime_shell.rssrc/runtime_tray.rssrc/runtime_worker.rssrc/scoring.rssrc/secrets.rssrc/state.rssrc/streaming_transcription.rssrc/streaming_transcription/deepgram.rssrc/streaming_transcription/google.rssrc/streaming_transcription/openai.rssrc/stt_apple_speech_tool.rssrc/stt_deepgram_tool.rssrc/stt_google_tool.rssrc/stt_openai_tool.rssrc/stt_whisper_cpp_tool.rssrc/target_context.rssrc/transcription.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/runner/transport.rs`:
- Around line 316-324: In the bounded cleanup path in
drain_reader_until_cleanup_deadline usage, the current ordering drains stdout
before stderr even though only stderr is returned, which can waste the remaining
deadline if stdout stays open. Update the cleanup sequence in the transport
runner so stderr is drained first and its output/notes are preserved, then drain
stdout as best-effort discarded cleanup, keeping the existing use of
drain_reader_until_cleanup_deadline and the final stderr.output return behavior
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: 0a7059ef-cf37-4865-abb7-bf081a5f55f9
📒 Files selected for processing (19)
src/config/logging.rssrc/config_watch.rssrc/envelope.rssrc/error.rssrc/external_control/mcp.rssrc/lib.rssrc/logging.rssrc/mock.rssrc/orchestrator.rssrc/platform.rssrc/replay_dispatch.rssrc/runner.rssrc/runner/transport.rssrc/runtime_permissions.rssrc/runtime_shell.rssrc/secrets.rssrc/state.rssrc/stt_google_tool.rssrc/target_context.rs
✅ Files skipped from review due to trivial changes (9)
- src/target_context.rs
- src/platform.rs
- src/state.rs
- src/config/logging.rs
- src/envelope.rs
- src/config_watch.rs
- src/secrets.rs
- src/logging.rs
- src/mock.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/runtime_permissions.rs
- src/runner.rs
- src/error.rs
- src/orchestrator.rs
- src/stt_google_tool.rs
Comment on lines
+316
to
+324
| let stdout_drain = | ||
| drain_reader_until_cleanup_deadline(&mut stdout_reader, cleanup_deadline, "stdout").await; | ||
| cleanup_notes.extend(stdout_drain.notes); | ||
|
|
||
| let stderr = match drain_reader(stderr_reader).await { | ||
| Ok(stderr) => stderr, | ||
| Err(source) => { | ||
| cleanup_notes.push(format!("stderr drain during cleanup failed: {source}")); | ||
| CapturedOutput::default() | ||
| } | ||
| let stderr = | ||
| drain_reader_until_cleanup_deadline(&mut stderr_reader, cleanup_deadline, "stderr").await; | ||
| cleanup_notes.extend(stderr.notes); | ||
|
|
||
| (stderr.output.unwrap_or_default(), cleanup_notes) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Drain stderr before discarded stdout during bounded cleanup.
Line 316 waits on stdout first, but this function only returns stderr. If a descendant keeps stdout open until the deadline, stderr gets no remaining budget and useful failure output is lost.
Proposed fix
- let stdout_drain =
- drain_reader_until_cleanup_deadline(&mut stdout_reader, cleanup_deadline, "stdout").await;
- cleanup_notes.extend(stdout_drain.notes);
-
let stderr =
drain_reader_until_cleanup_deadline(&mut stderr_reader, cleanup_deadline, "stderr").await;
cleanup_notes.extend(stderr.notes);
+
+ let stdout_drain =
+ drain_reader_until_cleanup_deadline(&mut stdout_reader, cleanup_deadline, "stdout").await;
+ cleanup_notes.extend(stdout_drain.notes);
(stderr.output.unwrap_or_default(), cleanup_notes)📝 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
| let stdout_drain = | |
| drain_reader_until_cleanup_deadline(&mut stdout_reader, cleanup_deadline, "stdout").await; | |
| cleanup_notes.extend(stdout_drain.notes); | |
| let stderr = match drain_reader(stderr_reader).await { | |
| Ok(stderr) => stderr, | |
| Err(source) => { | |
| cleanup_notes.push(format!("stderr drain during cleanup failed: {source}")); | |
| CapturedOutput::default() | |
| } | |
| let stderr = | |
| drain_reader_until_cleanup_deadline(&mut stderr_reader, cleanup_deadline, "stderr").await; | |
| cleanup_notes.extend(stderr.notes); | |
| (stderr.output.unwrap_or_default(), cleanup_notes) | |
| let stderr = | |
| drain_reader_until_cleanup_deadline(&mut stderr_reader, cleanup_deadline, "stderr").await; | |
| cleanup_notes.extend(stderr.notes); | |
| let stdout_drain = | |
| drain_reader_until_cleanup_deadline(&mut stdout_reader, cleanup_deadline, "stdout").await; | |
| cleanup_notes.extend(stdout_drain.notes); | |
| (stderr.output.unwrap_or_default(), cleanup_notes) |
🤖 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 `@src/runner/transport.rs` around lines 316 - 324, In the bounded cleanup path
in drain_reader_until_cleanup_deadline usage, the current ordering drains stdout
before stderr even though only stderr is returned, which can waste the remaining
deadline if stdout stays open. Update the cleanup sequence in the transport
runner so stderr is drained first and its output/notes are preserved, then drain
stdout as best-effort discarded cleanup, keeping the existing use of
drain_reader_until_cleanup_deadline and the final stderr.output return behavior
intact.
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
muninn://URL handling.Bug Fixes
Improvements