Skip to content

feat(requestrouter): let request routers report download progress - #27

Merged
Quick104 merged 1 commit into
mainfrom
feat/request-download-progress
Sep 29, 2026
Merged

Quick104 merged 1 commit into
mainfrom
feat/request-download-progress

Conversation

@Quick104

@Quick104 Quick104 commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Closes #26
Related issue: Silo-Server/silo-server#1643
Validation tasks: none

Request-router plugins can tell Silo whether a request's download is queued, downloading, completed or failed, but not how far along it is, so Silo shows "Processing" for as long as a title downloads. Silo-Server/silo-server#1643 adds download progress to requests; this is the plugin contract it needs.

Approach

  • TargetStatus.progress (field 6) is a new DownloadProgress with:

    • a phase
    • bytes total and bytes left, summed over the target's distinct downloads, so a season pack counts once
    • the latest estimated completion
    • the number of downloads

    Plugins set it only while a target is queued or downloading and the downstream service has something in its queue for it.

  • RequestRouterDescriptor.reports_download_progress (field 2) declares that a plugin fills progress. The host reads it from the stored manifest and refreshes only declaring plugins every minute. It ignores progress from any other plugin.

  • The phase vocabulary is open: queued, downloading, paused, stalled, importing, import_blocked. Hosts read an unknown value as downloading.

  • bytes_total is 0 whenever any download's size is unknown, so a host shows no percentage rather than an overstated one.

  • The README and docs/compatibility.md state these rules. The convert and manifest tests cover the new flag, including manifests written before it existed.

Compatibility and release

The change is additive: one new message and new fields, with no RPC changes. buf breaking against main reports nothing. Plugins that never set progress, and hosts that never read it, behave as before.

Release it as v0.19.0 after merge. Three changes depend on it, and each is pinned to this branch's commit until the tag exists:

Validation

  • go test ./..., go vet ./... and the three example builds pass.
  • gofmt -l . lists only pkg/pluginsdk/runtime/scan_source_test.go, which is unchanged from main.
  • buf generate with the pinned generators reproduces main's generated code. After this change, only request_router.pb.go changes.
  • buf breaking --against '.git#branch=origin/main' reports nothing.
  • I did not run make proto, because it requires protoc, which the build host lacks. I ran buf generate directly, which is how the committed code was generated (its header reads protoc (unknown)).
  • End to end: I ran a disposable Silo sandbox with both request-router plugins built against this branch and stub Sonarr, Radarr and Seerr servers. Progress reached the server, the web app, and the Android and Apple apps.

Risks

None identified.

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

AI Disclosure

  • Harness: T3 Code (Claude Code agent harness)
  • Tool(s): Claude Code, including workflow subagents; GitHub CLI; buf
  • Model(s): claude-opus-5-5
  • Involvement: Fully AI-generated at the maintainer's request. The agent wrote the change and ran the checks.
  • Adversarial review: Two rounds of independent review by Claude subagents, each in a fresh context, covered proto compatibility, presence semantics, and whether the docs match what the host does. They found three documentation gaps: a set progress must carry a phase; the one-minute refresh applies only to declaring plugins; and the docs did not say what bytes_total means when only some sizes are known. The comments and docs now cover all three.

🤖 Generated with Claude Code

Add an optional DownloadProgress to TargetStatus: a phase, bytes total and
left summed over the target's distinct downloads, the latest estimated
completion, and the number of downloads. A plugin declares that it fills it
with request_router.reports_download_progress, which the host reads from the
manifest to decide which targets to refresh every minute.

The phase vocabulary is open, and bytes_total is 0 whenever any download's
size is unknown, so hosts show no percentage rather than an overstated one.

Refs #26

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@kody-ai

kody-ai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Code Review Completed! 🔥

The code review was successfully completed based on your current configurations.

Kody Guide: Usage and Configuration
Interacting with Kody
  • Request a Review: Ask Kody to review your PR manually by adding a comment with the `@kody start-review` command at the root of your PR.

  • Provide Feedback: Help Kody learn and improve by reacting to its comments with a 👍 for helpful suggestions or a 👎 if improvements are needed.

Providing Context (Files & MCPs)

Add these hints in your PR description (or a comment) to unlock deeper checks:

  • Ticket / Acceptance Criteria: `Refs: ABC-123` (Linear/Jira/Asana/ClickUp/Trello) or a direct ticket link.
  • Bugfix Validation: a Sentry/Datadog/Bugsnag event link (or paste the stack trace/error message).
  • Endpoint Risk: mention the route (e.g., `POST /api/payments`) or controller/action name.
  • Attach a repo file as context: use an explicit marker like `@file:docs/guide.mdx#L10-L50` (replace with your real path).
  • API Contract Docs: include `@file:openapi.yaml` or `@file:swagger.json` when changing routes/schemas.
  • Definition of Done / Standards: include `@file:DOD.md` or `@file:CONTRIBUTING.md` if your repo has them.
  • Design System Source of Truth: include `@file:ui/index.ts` (replace with your DS entrypoint path).
  • Feature Flags: include the flag key/name and `@file:flags.ts` / `@file:config.json` (and optionally the PostHog flag name).
  • Edge/CDN Rules: link the Cloudflare rule/zone or describe the intended redirect/header behavior.
  • Attach an MCP tool output: use `@mcp<provider|tool>` (replace with an installed MCP provider + tool, e.g., `@mcp<sentry|events.search>`).
Current Kody Configuration
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug ✅
Performance ✅
Security ✅
Business Logic ❌

Access your configuration settings here.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-28T19:51:58.749660Z 1abd582 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 91be17e6-e6fe-40b5-ae8d-c90e5a39212f

📥 Commits

Reviewing files that changed from the base of the PR and between 6c1da00 and 1abd582.

⛔ Files ignored due to path filters (1)
  • pkg/pluginproto/silo/plugin/v1/request_router.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (5)
  • README.md
  • docs/compatibility.md
  • pkg/pluginsdk/convert/request_router_test.go
  • pkg/pluginsdk/manifest/request_router_test.go
  • proto/silo/plugin/v1/request_router.proto

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The request-router protocol adds a declaration flag and a progress message for target status. Documentation describes progress values and host polling behavior. SDK tests cover manifest loading, validation, and metadata conversion for the new flag.

Changes

Request-router progress contract

Layer / File(s) Summary
Define progress reporting
proto/silo/plugin/v1/request_router.proto, README.md, docs/compatibility.md
The protocol adds reports_download_progress and TargetStatus.progress, with fields for phase, byte totals, estimated completion, and download count. The documentation describes aggregation, phase handling, and the host’s progress polling and clearing rules.

SDK manifest and metadata handling

Layer / File(s) Summary
Validate manifest and metadata handling
pkg/pluginsdk/manifest/request_router_test.go, pkg/pluginsdk/convert/request_router_test.go
Tests cover manifest loading with and without the flag, validation for scheduled-task capabilities, descriptor round trips, and metadata compatibility with unknown flags.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Feature

Merge Risk: ⚪ Minimal · up to 1abd5

The additive progress contract and compatibility coverage present no identified issue that needs resolution before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 1abd5

The new fields are additive, and older plugins remain outside progress reporting. No security issue is established in this PR, but the host behavior that will enforce the contract is not included, so its safeguards remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — A plugin's reported progress can affect its target's displayed status and documented refresh cadence if the host accepts it. The supplied evidence does not establish broader authority, credential access, or cross-tenant reach from these fields.

Trust Boundaries and Controls

  • observed — Progress originates with a plugin, while the contract directs hosts to check the manifest declaration before using it. Conversion tests exercise metadata handling rather than an authorization or host acceptance boundary.

Resilience and Maintainability Implications

  • inferred — The specified clear-and-revert behavior limits how long stale progress should drive accelerated checks, but per-target ownership and recovery after failed or interrupted checks remain unverified without host implementation evidence.

Hardening Proposals

  • proposed — When the host adopts this contract, verify acceptance against its stored manifest, bind progress to the owning target, and exercise clearing and cadence recovery across completion, failure, repeated checks, and interruption.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR meets the coding requirements in directly linked issue #26. The proto adds DownloadProgress, TargetStatus.progress = 6, and RequestRouterDescriptor.reports_download_progress = 2. The cont…
Out of Scope Changes check ✅ Passed The changes stay within issue #26. The proto and generated-code changes implement the contract. The README and compatibility documentation explain the contract. The added tests verify the new flags an…
Title check ✅ Passed The title clearly and concisely describes the main change: allowing request-router plugins to report download progress.
Description check ✅ Passed The description directly explains the problem, implementation, compatibility, validation, and release plan for the download-progress contract.
Full details: Docstring Coverage

Explanation

Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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.

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.

Let request routers report download progress in CheckStatus

1 participant