Skip to content

feat: show download progress for PV artifacts#291

Merged
munezaclovis merged 5 commits into
mainfrom
agent/download-progress-events
Jul 12, 2026
Merged

feat: show download progress for PV artifacts#291
munezaclovis merged 5 commits into
mainfrom
agent/download-progress-events

Conversation

@munezaclovis

@munezaclovis munezaclovis commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds byte-level download progress reporting for PV-managed resource artifacts and PV self-update downloads.

  • emits download_progress events from daemon-owned Managed Resource update jobs over the existing NDJSON socket stream
  • adds a shared CLI indicatif renderer for daemon events, direct foreground resource commands, and PV app binary downloads
  • bumps the daemon protocol version to 3 because the job stream contract now includes a new event type
  • covers resource download progress, command-level progress propagation, protocol serialization, daemon stream writes, client parsing, and update command behavior

Validation

  • cargo fmt --all
  • cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
  • cargo nextest run -p resources -E 'test(artifact_downloader_reports_download_progress_events) | test(managed_resource_commands_install_reports_download_progress) | test(managed_resource_commands_update_reports_download_progress) | test(managed_resource_commands_install_php_pair_reports_download_progress)'
  • cargo nextest run -p protocol -E 'test(download_progress_event_serializes_resource_artifact_progress) | test(managed_resource_update_check_command_bumps_protocol_version)'
  • cargo nextest run -p daemon -E 'test(parse_job_event_reports_download_progress_events) | test(streamed_job_writes_download_progress_events) | test(streamed_job_completion_wins_when_download_progress_write_blocks)'
  • cargo nextest run -p cli --test update
  • cargo nextest run -p daemon --test daemon_foundation --test project_env_reconciliation
  • cargo nextest run -p cli --test dns --test setup

Summary by CodeRabbit

  • New Features
    • Added terminal-aware download progress bars and byte counters for app binary downloads, managed resource installs/updates, PHP track operations, and Composer operations.
    • Progress is streamed during long-running daemon jobs via a new download_progress event.
  • Bug Fixes
    • Improved update/install visibility by showing progress continuously rather than only end-of-operation output.
    • Non-terminal output remains unchanged.
  • Chores
    • Bumped protocol version to 3 and updated protocol documentation/examples.
    • Added a progress UI dependency to support richer terminal rendering.

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c7b3d832-69ee-4f38-884a-09a2d92cda80

📥 Commits

Reviewing files that changed from the base of the PR and between b4d5cbe and f777389.

📒 Files selected for processing (4)
  • DESIGN.md
  • crates/cli/src/progress.rs
  • crates/daemon/src/jobs.rs
  • crates/resources/src/download.rs
🚧 Files skipped from review as they are similar to previous changes (4)
  • DESIGN.md
  • crates/resources/src/download.rs
  • crates/cli/src/progress.rs
  • crates/daemon/src/jobs.rs

📝 Walkthrough

Walkthrough

This PR adds end-to-end download progress reporting across artifact downloads, managed-resource operations, daemon job events, the versioned protocol, and CLI progress rendering.

Changes

Download Progress Feature

Layer / File(s) Summary
Artifact download progress primitives
crates/resources/src/download.rs, crates/resources/src/lib.rs, crates/resources/tests/*
Adds progress events, callback interfaces, byte-counting writers, compatibility wrappers, exports, and tests.
Progress-aware resource commands
crates/resources/src/command.rs, crates/resources/tests/managed_resource_commands.rs
Adds _with_progress install/update variants across resource, PHP-pair, Composer, and backing-resource flows.
Daemon reconciliation plumbing
crates/daemon/src/managed_resources/*, crates/daemon/src/project_env.rs
Threads progress through managed-resource updates, runtime catalogs, and project/system reconciliation.
Protocol and daemon streaming
crates/protocol/*, crates/daemon/src/client.rs, crates/daemon/src/jobs.rs, DESIGN.md
Adds download_progress, bumps protocol version to 3, parses and forwards events, and streams them with job heartbeats.
CLI rendering and integration
crates/cli/src/progress.rs, crates/cli/src/commands/*, Cargo.toml, crates/cli/Cargo.toml
Adds indicatif progress bars and wires them into resource, setup, and application update commands.

Estimated code review effort: 4 (Complex) | ~75 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant DaemonClient
  participant DaemonJobs
  participant ArtifactDownloader
  CLI->>DaemonClient: run_job_with_events_blocking(renderer)
  DaemonClient->>DaemonJobs: stream update job
  DaemonJobs->>ArtifactDownloader: download_with_progress(progress)
  ArtifactDownloader-->>DaemonJobs: download_progress event
  DaemonJobs-->>DaemonClient: download_progress NDJSON
  DaemonClient-->>CLI: JobDownloadProgress
Loading

Possibly related PRs

  • prvious/pv#239: Provides the daemon protocol foundation extended here.
  • prvious/pv#242: Provides the artifact download pipeline extended with progress reporting.
  • prvious/pv#244: Provides the resource command flows extended with _with_progress variants.

Poem

A rabbit hops with byte-sized glee, 🐇
Watching bars fill, one, two, three!
From daemon socket to terminal screen,
Progress flows where none has been.
Hop along, download complete!
Indicatif makes it neat!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 19.05% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding download progress for PV artifacts.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch agent/download-progress-events

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@munezaclovis
munezaclovis marked this pull request as ready for review July 8, 2026 04:38
@codspeed-hq

codspeed-hq Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 7 untouched benchmarks


Comparing agent/download-progress-events (f777389) with main (32e4a2c)

Open in CodSpeed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b771f260d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/daemon/src/jobs.rs Outdated
Comment on lines +536 to +537
if let Ok(result) = result {
result?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Do not abort updates on progress write errors

When the foreground client disconnects during a download (for example Ctrl-C or a closed terminal), write_foreground_job_event can return a broken-pipe error and this result? exits complete_streamed_job_with_heartbeat_and_events before complete_update_job_with_progress records completion/failure or runs the post-update reconciliation. The heartbeat path intentionally ignores foreground write failures so the job can finish; progress writes should not cancel the update task either.

Useful? React with 👍 / 👎.

Comment thread crates/daemon/src/jobs.rs Outdated
Comment on lines +530 to +531
event = events.recv() => {
if let Some(event) = event {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle the closed progress channel

In the production update path, the only SocketDownloadProgress sender is moved into the spawn_blocking download phase, so after downloads finish it can be dropped while the completion future is still awaiting post-update reconciliation. At that point events.recv() returns None immediately forever, and this branch just loops without disabling the receiver, causing a busy spin and preventing heartbeat events until reconciliation completes.

Useful? React with 👍 / 👎.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The daemon progress bridge can fail or overload foreground update streams when the client is slow or disconnected.

Reviewed changes — This PR adds byte-level download progress for managed resource artifact downloads and PV self-update downloads across the resource, daemon protocol, and CLI rendering layers.

  • Add resource download progress callbacksresources now reports started, advanced, and finished download events and threads callbacks through direct install/update command paths.
  • Stream daemon-owned update downloadsdaemon maps resource progress into download_progress NDJSON events and the client parses them through JobEventHandler.
  • Render CLI progress barscli adds a shared DownloadProgressRenderer for direct resource commands, daemon-streamed updates, and app binary downloads.
  • Bump the socket protocolprotocol::PROTOCOL_VERSION moves to 3, with docs, snapshots, and serialization coverage updated.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using GPT𝕏

Comment thread crates/daemon/src/jobs.rs Outdated
Comment thread crates/daemon/src/jobs.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/cli/src/commands/composer.rs (1)

32-38: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Clear progress bars before printing summaries.

Drop the renderer immediately after the download operation so final command output is printed after progress cleanup.

Proposed fix
     let installed = with_resource_http_client(environment, |client| {
         commands.install_composer_with_php_pair_and_progress(selector, client, &progress)
     })?;
+    drop(progress);
     let php_pair = installed.php_pair();
     let composer = installed.composer();
     let mut output = Output::new(stdout, OutputMode::plain());

@@
     let updated = with_resource_http_client(environment, |client| {
         commands.update_composer_with_progress(client, &progress)
     })?;
+    drop(progress);
     let mut output = Output::new(stdout, OutputMode::plain());

Also applies to: 60-64

🤖 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 `@crates/cli/src/commands/composer.rs` around lines 32 - 38, The progress
renderer created in composer command handling is still alive when the final
summary output is printed, so clear it right after the download/install call
completes. In the composer flow around DownloadProgressRenderer::new and
install_composer_with_php_pair_and_progress, explicitly drop or end the renderer
before creating Output and printing the summary, and apply the same cleanup in
the other affected block referenced by the review so progress bars never overlap
final command output.
🧹 Nitpick comments (1)
crates/cli/src/progress.rs (1)

22-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Add enabled early return to update_app_progress for consistency.

update_resource_progress checks self.enabled before allocating key/label strings, but update_app_progress does not — it builds progress_key and format! strings unconditionally before update_progress discards them when disabled. Adding the same guard keeps the two paths consistent and avoids wasted allocations.

♻️ Suggested refactor
     pub(crate) fn update_app_progress(
         &self,
         version: &str,
         downloaded_bytes: u64,
         total_bytes: u64,
     ) {
+        if !self.enabled {
+            return;
+        }
+
         self.update_progress(
             progress_key("pv", "app", version),
             format!("Downloading PV {version}"),
             downloaded_bytes,
             total_bytes,
         );
     }
🤖 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 `@crates/cli/src/progress.rs` around lines 22 - 34, Add an early return in
update_app_progress when self.enabled is false, matching the guard used by
update_resource_progress. Update the update_app_progress method in progress.rs
so it checks enabled before calling progress_key and format!, and then only
forwards to update_progress when progress reporting is active. Keep the behavior
consistent with the existing progress methods and avoid allocating the key/label
strings when disabled.
🤖 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 `@crates/cli/src/commands/artifact_resource.rs`:
- Around line 43-47: The success/warning output in the resource install flow is
being written while DownloadProgressRenderer is still alive, so explicitly drop
the progress renderer right after the
with_resource_http_client/commands.install_with_progress work completes and
before creating or using Output. Apply the same cleanup in the other matching
install path as well, using the DownloadProgressRenderer and Output setup in
artifact_resource.rs as the reference points.

In `@crates/cli/src/commands/php.rs`:
- Around line 34-40: The PHP command flow keeps DownloadProgressRenderer alive
while Output writes the final install/update text, so the progress cleanup
happens too late. In the php command handler, drop the DownloadProgressRenderer
before creating or using Output, and apply the same change in the install/update
paths referenced by the php command helpers so the terminal is cleared before
any final status lines are printed.

In `@crates/cli/src/commands/update.rs`:
- Around line 102-106: The managed-resource update flow keeps
DownloadProgressRenderer alive while later output is written, which leaves
progress bars on screen during the summary and follow-up work. In the update
command path around daemon::run_job_with_events_blocking and
write_managed_resource_update_summary, explicitly end the renderer’s scope or
drop the DownloadProgressRenderer immediately after the job/download completes
and before creating Output or printing the summary. Apply the same cleanup in
the related app-update path referenced by the comment so both flows clear the
renderer before subsequent output.

In `@crates/daemon/src/jobs.rs`:
- Around line 16-17: `ProgressWriter` currently sends every progress update into
an unbounded channel, which lets stale events accumulate and delay completion
delivery. Update the progress path in `jobs.rs` to either use a bounded channel
with `try_send` or coalesce/drop excess progress events, and make sure the
completion flow in the job sender/receiver logic returns `JobCompleted` or
`JobFailed` immediately instead of draining queued progress first.

---

Outside diff comments:
In `@crates/cli/src/commands/composer.rs`:
- Around line 32-38: The progress renderer created in composer command handling
is still alive when the final summary output is printed, so clear it right after
the download/install call completes. In the composer flow around
DownloadProgressRenderer::new and install_composer_with_php_pair_and_progress,
explicitly drop or end the renderer before creating Output and printing the
summary, and apply the same cleanup in the other affected block referenced by
the review so progress bars never overlap final command output.

---

Nitpick comments:
In `@crates/cli/src/progress.rs`:
- Around line 22-34: Add an early return in update_app_progress when
self.enabled is false, matching the guard used by update_resource_progress.
Update the update_app_progress method in progress.rs so it checks enabled before
calling progress_key and format!, and then only forwards to update_progress when
progress reporting is active. Keep the behavior consistent with the existing
progress methods and avoid allocating the key/label strings when disabled.
🪄 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 Plus

Run ID: 34fffa70-434c-4704-9d84-1a12d1f373e0

📥 Commits

Reviewing files that changed from the base of the PR and between 32e4a2c and b771f26.

⛔ Files ignored due to path filters (42)
  • Cargo.lock is excluded by !**/*.lock
  • crates/cli/tests/snapshots/dns__dns_install_writes_prepared_and_system_resolver_config.snap is excluded by !**/*.snap
  • crates/cli/tests/snapshots/setup__setup_no_path_configures_system_integrations_and_waits_for_reconciliation.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__idle_client_without_newline_does_not_block_health_requests.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__invalid_reconciliation_scope_reports_scope_parse_failure.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__malformed_request_does_not_stop_accepting_connections.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__managed_resource_update_check_returns_success_response.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__protocol_mismatch_returns_restart_guidance_without_creating_a_job.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__socket_protocol_streams_job_progress_and_persists_final_status.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__start_removes_stale_socket_before_binding.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__unsupported_job_streams_failure_event_and_persists_failed_status.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__update_lock_rejects_mutating_jobs_but_keeps_health_available.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__update_lock_rejects_update_jobs_before_manifest_refresh.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/daemon_foundation__valid_reconciliation_scopes_stream_stub_completion.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__config_declared_hostnames_are_persisted_during_reconciliation.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__duplicate_rendered_env_key_leaves_resource_state_unchanged.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__duplicate_user_owned_key_writes_block_and_records_warning.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__existing_allocation_name_survives_primary_hostname_change.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__first_allocation_reconciliation_records_desired_state_before_context_failure.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__generated_allocation_name_too_long_leaves_resource_state_unchanged.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__invalid_config_failure_rolls_back_resource_state_mutations.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__latest_resource_track_resolves_default_track_before_state_and_dotenv_writes.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__latest_resource_track_reuses_stored_track_when_manifest_default_changes.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__legacy_url_placeholder_failure_preserves_last_valid_desired_state.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__malformed_pv_block_leaves_dotenv_unchanged_and_records_failure.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__malformed_pv_block_preflight_preserves_resource_and_hostname_state.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__missing_context_leaves_dotenv_unchanged_and_records_failure.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__missing_dotenv_is_created_with_private_permissions.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__multiple_managed_dotenv_blocks_fold_to_one_and_preserve_permissions.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__no_mappings_do_not_touch_existing_dotenv_and_record_noop_success.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__omitted_resource_track_resolves_manifest_default_track.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__omitted_resource_track_reuses_stored_track_when_manifest_default_changes.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__omitted_resource_track_without_mappings_updates_state_without_dotenv.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__project_env_reconciliation_uses_project_root_not_config_path_for_dotenv.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__resources_and_empty_allocations_without_env_mappings_update_state_without_dotenv.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__root_env_with_resource_waits_for_resource_context_before_dotenv.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__root_only_env_rendering_writes_dotenv_and_records_rendered_state.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/project_env_reconciliation__seeded_resource_and_allocation_contexts_render_dotenv.snap is excluded by !**/*.snap
  • crates/resources/tests/snapshots/artifact_cache_download__artifact_downloader_reports_download_progress_events.snap is excluded by !**/*.snap
  • crates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_install_php_pair_reports_download_progress.snap is excluded by !**/*.snap
  • crates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_install_reports_download_progress.snap is excluded by !**/*.snap
  • crates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_update_reports_download_progress.snap is excluded by !**/*.snap
📒 Files selected for processing (21)
  • Cargo.toml
  • DESIGN.md
  • crates/cli/Cargo.toml
  • crates/cli/src/commands/artifact_resource.rs
  • crates/cli/src/commands/composer.rs
  • crates/cli/src/commands/php.rs
  • crates/cli/src/commands/update.rs
  • crates/cli/src/lib.rs
  • crates/cli/src/progress.rs
  • crates/daemon/src/client.rs
  • crates/daemon/src/jobs.rs
  • crates/daemon/src/lib.rs
  • crates/daemon/src/managed_resources/mod.rs
  • crates/protocol/src/event.rs
  • crates/protocol/src/lib.rs
  • crates/protocol/src/tests.rs
  • crates/resources/src/command.rs
  • crates/resources/src/download.rs
  • crates/resources/src/lib.rs
  • crates/resources/tests/artifact_cache_download.rs
  • crates/resources/tests/managed_resource_commands.rs

Comment thread crates/cli/src/commands/artifact_resource.rs
Comment thread crates/cli/src/commands/php.rs
Comment thread crates/cli/src/commands/update.rs
Comment thread crates/daemon/src/jobs.rs Outdated

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — I reviewed the 0438883 delta that hardens progress cleanup and daemon progress-event delivery after the prior Pullfrog review.

  • Cleared terminal progress before final output — Direct resource, Composer, PHP, and app-update command paths now drop DownloadProgressRenderer before creating Output or printing summaries and warnings.
  • Bounded daemon progress delivery — Foreground update progress now uses a bounded channel with non-blocking try_send, so slow readers cannot create unbounded progress-event backlog.
  • Made progress writes best-effort — Streamed progress-event writes now match heartbeat behavior by ignoring write failures/timeouts and returning the real update completion outcome as soon as it is ready.
  • Covered stream hardening — Daemon tests were added or updated for progress event delivery, completion winning over blocked progress writes, and ignored progress write errors.

Pullfrog  | View workflow run | Using GPT𝕏

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0438883a73

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Cargo.lock
"percent-encoding",
"pin-project-lite",
"socket2 0.5.10",
"socket2 0.6.3",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Revert unrelated socket2 lockfile bumps

This changes existing hyper-util/quinn dependency resolution from socket2 0.5.10 to socket2 0.6.3, which is unrelated to adding indicatif for progress rendering. The repo's AGENTS.md explicitly says to never update all dependencies and to use precise lockfile changes; leaving this network-stack churn in the progress UI PR makes the dependency change harder to audit and can alter runtime behavior outside this feature, so please regenerate the lockfile with only the required new dependency closure.

Useful? React with 👍 / 👎.

Comment thread crates/daemon/src/jobs.rs Outdated
Comment on lines +315 to +318
let update_result = if stream_is_open && started_stream_result.is_ok() {
complete_streamed_job_with_heartbeat(
let (event_sender, event_receiver) = channel(FOREGROUND_JOB_PROGRESS_BUFFER);
let progress = SocketDownloadProgress::new(event_sender);
complete_streamed_job_with_heartbeat_and_events(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Wire progress through setup reconciliation downloads

This only attaches SocketDownloadProgress to foreground update jobs, but first-time pv setup still waits on a reconcile job (crates/cli/src/commands/setup.rs:132) and the daemon reconciliation install path still calls the non-progress install APIs (crates/daemon/src/managed_resources/mod.rs:582, :587, :871). In the setup path that downloads the default Managed Resources, no download_progress events are emitted or rendered, so users still see only heartbeat messages during the largest initial artifact downloads.

Useful? React with 👍 / 👎.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new reconciliation progress plumbing misses one foreground pv update path.

Reviewed changes — I reviewed the b4d5cbe delta that extends daemon download-progress events from update jobs into setup and foreground reconciliation jobs.

  • Streamed setup reconciliation progresspv setup now uses daemon::run_job_with_events_blocking with DownloadProgressRenderer so setup-owned resource downloads can render progress bars.
  • Threaded DaemonDownloadProgress through reconciliation — Foreground system, project, and managed-resource reconciliation paths now pass a daemon progress bridge into missing resource installation and project-env reconciliation.
  • Covered setup and project reconciliation events — Daemon tests now assert foreground reconciliation emits download_progress events for setup defaults and project-required resources.

Note: 1 inline comment(s) dropped because they did not anchor to lines inside the PR diff:

  • crates/daemon/src/jobs.rs:692 (RIGHT) — line 692 (RIGHT) is not inside a diff hunk

Pullfrog  | Fix it ➔View workflow run | Using GPT𝕏

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/daemon/src/jobs.rs (1)

692-692: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Thread progress through the post-update reconciliation path. reconcile_system_projects_and_resources_with_progress(...) already exists and emits download events for resource installs, so this call should use it too. In the spawn_blocking branch, clone progress for the update step so it stays available for reconciliation.

🤖 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 `@crates/daemon/src/jobs.rs` at line 692, The post-update reconciliation
currently drops progress reporting. In the update flow around
reconcile_system_projects_and_resources, clone progress before entering the
spawn_blocking branch and pass that clone to
reconcile_system_projects_and_resources_with_progress, preserving the original
progress handle for subsequent reconciliation steps.
🤖 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.

Outside diff comments:
In `@crates/daemon/src/jobs.rs`:
- Line 692: The post-update reconciliation currently drops progress reporting.
In the update flow around reconcile_system_projects_and_resources, clone
progress before entering the spawn_blocking branch and pass that clone to
reconcile_system_projects_and_resources_with_progress, preserving the original
progress handle for subsequent reconciliation steps.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0c709559-0f53-4c09-ad78-69520886bb79

📥 Commits

Reviewing files that changed from the base of the PR and between b771f26 and b4d5cbe.

📒 Files selected for processing (10)
  • crates/cli/src/commands/artifact_resource.rs
  • crates/cli/src/commands/composer.rs
  • crates/cli/src/commands/php.rs
  • crates/cli/src/commands/setup.rs
  • crates/cli/src/commands/update.rs
  • crates/cli/src/progress.rs
  • crates/daemon/src/jobs.rs
  • crates/daemon/src/managed_resources/mod.rs
  • crates/daemon/src/managed_resources/tests.rs
  • crates/daemon/src/project_env.rs
🚧 Files skipped from review as they are similar to previous changes (5)
  • crates/cli/src/commands/artifact_resource.rs
  • crates/cli/src/commands/composer.rs
  • crates/cli/src/commands/php.rs
  • crates/cli/src/progress.rs
  • crates/cli/src/commands/update.rs

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — I reviewed the f777389 delta that preserves foreground daemon progress delivery while fixing the remaining pv update reconciliation progress gap.

  • Threaded update progress through follow-up reconciliationcomplete_update_job_inner now reuses DaemonDownloadProgress for post-update system/project resource reconciliation, so foreground pv update can stream downloads triggered after artifact updates.
  • Preserved job outcomes after stream write failures — Foreground progress and heartbeat write failures now close the stream path without overwriting the underlying persisted job result.
  • Covered update reconciliation progress — Daemon tests now assert that foreground update streams include download_progress for missing desired resources installed during follow-up reconciliation.

Pullfrog  | View workflow run | Using GPT𝕏

@munezaclovis
munezaclovis merged commit aee5b2b into main Jul 12, 2026
4 checks passed
@munezaclovis
munezaclovis deleted the agent/download-progress-events branch July 12, 2026 03:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant