refactor(network): standardize HTTP operations on async reqwest (#496) - #548
Merged
Merged
Conversation
Convert reqwest::blocking to async reqwest across the network layer: - update_check.rs: async fetch_latest_version with UpdateCheckError carrying timeout/connection/status context; spawn via tokio instead of std thread - skills/provider.rs: resolve_via_search via bridge pattern (Handle::try_current + Runtime::new fallback), mirroring install.rs - test_catalog_integrity.rs: #[tokio::test] with async client - Cargo.toml: remove blocking feature from reqwest Closes #496
Contributor
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe refactor replaces blocking Reqwest calls with asynchronous clients. It adds Tokio runtime bridging, structured update-check errors, asynchronous retries and tests, removes the ChangesAsync HTTP refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant TokioRuntime
participant ReqwestClient
participant HTTPServer
CLI->>TokioRuntime: start async operation
TokioRuntime->>ReqwestClient: send timed request
ReqwestClient->>HTTPServer: HTTP request
HTTPServer-->>ReqwestClient: response or failure
ReqwestClient-->>TokioRuntime: parsed result or contextual error
TokioRuntime-->>CLI: continue synchronously or log failure
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 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
`@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/archive-report.md`:
- Around line 32-38: Add blank lines before and after the affected Markdown
headings in
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/archive-report.md
(lines 32-38); add blank lines after ## Identity, ## Sources of Truth, and ##
Target and Environment in
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/qa-report.md
(lines 3-18); and add a blank line after ### Implementation Order in
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.md
(lines 19-20) to satisfy MD022.
- Around line 27-35: Correct the archived update-check documentation to match
the implementation: in the version-check change summary and
specs/version-check/spec.md, describe the detached background thread using
std::thread::Builder, a created tokio runtime, and block_on. Remove claims that
it uses tokio::spawn or implicit naming, while preserving the other async HTTP
changes.
In `@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/design.md`:
- Line 89: Resolve the Markdown-lint findings across the documentation: in
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/design.md:89,
specify a language on the data-flow fenced block; in exploration.md:1, make the
opening heading top-level; and in proposal.md:9, add the required spacing after
the heading marker.
- Around line 39-63: Align all version-check specifications with the dedicated
Tokio Runtime created inside the detached thread by update_check::spawn, using
Runtime::block_on rather than tokio::spawn, and document runtime ownership, task
lifetime, cancellation, and shutdown behavior. Update
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/design.md:39-63,
exploration.md:174-180, proposal.md:25-31, specs/version-check/spec.md:50-72,
and openspec/specs/version-check/spec.md:61-83; remove contradictory spawn-model
requirements and state that spawning occurs after Cli::parse as implemented by
main.rs.
- Around line 65-85: Complete structured HTTP error handling around
UpdateCheckError, fetch_latest_version, and resolve_via_search_http: map
redirect errors before generic connection errors, include the request URL in
ParseError, and call error_for_status() before JSON parsing. In
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/design.md
lines 65-85 and 120-142, document URL, duration, reason, and status fields plus
redirect and parse scenarios. Apply the corresponding documentation updates in
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/version-check/spec.md
lines 5-13 and openspec/specs/version-check/spec.md lines 156-193.
In
`@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/exploration.md`:
- Around line 91-97: Update the Tokio dependency feature list to include time,
preserving the existing rt-multi-thread, macros, and fs features so tests such
as test_catalog_integrity.rs can use tokio::time::sleep.
In
`@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/proposal.md`:
- Around line 96-104: Update the Success Criteria command for the E2E test to
explicitly set RUN_E2E=1 when invoking cargo test, ensuring the gated catalog
integrity test executes rather than being skipped.
In
`@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/qa-report.md`:
- Line 47: Update the QA-7 row in the QA report so its evidence describes the
resolve_via_search_http error context without unescaped pipe characters, either
by rewording the closure reference or escaping both pipes; preserve the table’s
five-cell structure.
In
`@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/state.yaml`:
- Around line 19-33: Update state.yaml to reflect the completed archived change:
add Cargo.toml to scope, and rewrite the summary and Affected entries for
update_check.rs, skills/provider.rs, test_catalog_integrity.rs, and Cargo.toml
in past/completed tense, removing descriptions of the old blocking
implementation and pending “must be removed” work.
In `@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.md`:
- Around line 24-35: Update task 1.1 so its listed UpdateCheckError variants and
payloads match the final contract documented in verify-report.md, including
Connection with url and reason, HttpStatus with url and status, and Timeout with
url and duration_secs; alternatively, explicitly document the intentional
deviation if the implementation deliberately retains the current reqwest-based
variants.
In
`@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/verify-report.md`:
- Around line 103-112: Remove the stale State.yaml current_phase finding from
the correctness table and Issues section in the verification report, updating
both to reflect current_phase: archive with sdd-verify and sdd-archive
completed.
- Around line 41-46: Update the success scenario in the verification report to
reference a test that exercises fetch_latest_version_async and asserts the
expected version from a successful response, rather than
test_fetch_latest_version_timeout. Add that success-path test if none exists,
and keep the timeout test cited only for the timeout scenario.
- Around line 62-70: The e2e-testing table in the verification report
incorrectly marks unexecuted scenarios as passing. Update the five scenario
statuses to untested or conditional unless live execution was performed, and
only mark them PASS when accompanied by output from the test_catalog_integrity
command with RUN_E2E=1.
In `@src/skills/provider.rs`:
- Around line 294-309: Remove the active-runtime Handle::block_on path from
SkillsShProvider::resolve: add separate async and synchronous resolution entry
points, have async callers await the async path, and let the synchronous path
create a temporary runtime only when no runtime is active. Add a #[tokio::test]
covering public resolution, and update src/skills/provider.rs:294-309,
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/skill-recommendations/spec.md:7-10,
and openspec/specs/skill-recommendations/spec.md:905-910 to remove the
handle.block_on requirement and document both paths.
- Around line 323-330: Update the skills.sh request flow in the visible
SearchResponse fetch to call error_for_status() on the response before JSON
parsing, preserving contextual error handling for failed requests. Add a test
covering 4xx or 5xx responses and verify the resulting error reflects the HTTP
failure rather than a JSON parsing or not-found error.
In `@src/update_check.rs`:
- Around line 146-149: Update the response parsing in the update-check request
around the `response.json().await` call to map errors where `e.is_timeout()` is
true to `UpdateCheckError::Timeout`, while retaining `ParseError` for other
failures. Add a test that sends response headers, stalls the body, and verifies
the configured client timeout produces `UpdateCheckError::Timeout`.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: ab8e046e-a1c1-4020-a553-64ed4749c3f6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/archive-report.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/design.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/exploration.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/proposal.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/qa-report.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/dependency-management/spec.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/e2e-testing/spec.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/skill-recommendations/spec.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/version-check/spec.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/state.yamlopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/verify-report.mdopenspec/specs/dependency-management/spec.mdopenspec/specs/e2e-testing/spec.mdopenspec/specs/skill-recommendations/spec.mdopenspec/specs/version-check/spec.mdsrc/skills/provider.rssrc/update_check.rstests/test_catalog_integrity.rs
Comment on lines
+32
to
+38
| ### version-check — Changes Applied | ||
| - `crates.io API Query`: MODIFIED — async client replaces blocking, HTTP errors carry diagnostic context | ||
| - `Detached Background Thread`: MODIFIED — Tokio spawn replaces std::thread, implicit naming | ||
| - `Synchronous Path Documentation`: ADDED (new requirement) — cache I/O documented with `// Note: sync path` | ||
|
|
||
| ### skill-recommendations — Changes Applied | ||
| - `Provider Skill Resolution Uses Async HTTP`: ADDED — resolve_via_search uses async reqwest with bridge pattern, all HTTP errors carry context |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines around the affected Markdown headings.
markdownlint reports MD022 in all three files.
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/archive-report.md#L32-L38: add blank lines before and after the affected headings.openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/qa-report.md#L3-L18: add blank lines after## Identity,## Sources of Truth, and## Target and Environment.openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.md#L19-L20: add a blank line after### Implementation Order.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 32-32: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 37-37: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
📍 Affects 3 files
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/archive-report.md#L32-L38(this comment)openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/qa-report.md#L3-L18openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.md#L19-L20
🤖 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
`@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/archive-report.md`
around lines 32 - 38, Add blank lines before and after the affected Markdown
headings in
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/archive-report.md
(lines 32-38); add blank lines after ## Identity, ## Sources of Truth, and ##
Target and Environment in
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/qa-report.md
(lines 3-18); and add a blank line after ### Implementation Order in
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.md
(lines 19-20) to satisfy MD022.
Source: Linters/SAST tools
Comment on lines
+62
to
+70
| ### e2e-testing (5 scenarios) | ||
|
|
||
| | Scenario | Covered By | Status | | ||
| |----------|-----------|--------| | ||
| | All curated entries reachable — async client | `test_catalog_integrity.rs` uses `#[tokio::test]` + `reqwest::Client` | ✅ PASS | | ||
| | Retry on transient failure with async client | `send_request().await` + `tokio::time::sleep` retry logic | ✅ PASS | | ||
| | Timeout on slow endpoint with context | `client.builder().timeout(Duration::from_secs(15))` + failures list with context | ✅ PASS | | ||
| | Non-200 HTTP response carries status context | failure message includes `r.status()` | ✅ PASS | | ||
| | Network error carries diagnostic context | failure message includes `e` (error Debug impl) | ✅ PASS | |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
fd -i 'qa-report.md|verify-report.md|state.yaml|test_catalog_integrity.rs' . || true
printf '%s\n' '--- relevant report references ---'
rg -n -C 5 'e2e-testing|catalog integrity|RUN_E2E|untested|not selected|PASS|current_phase|archive|propose' \
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor \
--glob 'qa-report.md' --glob 'verify-report.md' --glob 'state.yaml' || true
printf '%s\n' '--- test implementation and invocation references ---'
rg -n -C 5 'test_catalog_integrity|RUN_E2E|reqwest::Client|send_request|tokio::time::sleep|timeout|GITHUB_TOKEN' \
. --glob '*.rs' --glob '*.md' --glob '*.yaml' --glob '*.yml' || trueRepository: dallay/agentsync
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
for f in \
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/qa-report.md \
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/verify-report.md \
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/state.yaml
do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f"
fi
done
printf '\n--- test files ---\n'
fd -i 'test_catalog_integrity.rs' . -x sh -c 'echo "--- $1"; cat -n "$1"' sh {}Repository: dallay/agentsync
Length of output: 24851
🏁 Script executed:
#!/bin/bash
set -eu
find openspec/changes/archive/2026-08-11-issue-496-async-http-refactor -maxdepth 1 -type f -print
printf '\n--- qa report ---\n'
sed -n '1,220p' openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/qa-report.md
printf '\n--- verify report ---\n'
sed -n '1,140p' openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/verify-report.mdRepository: dallay/agentsync
Length of output: 17422
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- E2E execution evidence and commands ---'
rg -n -C 3 \
'RUN_E2E|test_catalog_integrity|catalog_dallay_skill_urls_are_reachable|--ignored|running [0-9]+ tests|test result' \
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor \
README.md \
tests/test_catalog_integrity.rs
printf '%s\n' '--- source-derived test attributes ---'
python3 - <<'PY'
from pathlib import Path
p = Path("tests/test_catalog_integrity.rs")
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if "#[test]" in line or "#[tokio::test]" in line or "#[ignore]" in line or "RUN_E2E" in line:
print(f"{i}: {line}")
PYRepository: dallay/agentsync
Length of output: 39586
Do not mark unexecuted E2E scenarios as PASS.
qa-report.md marks live catalog-integrity execution as not selected and untested. verify-report.md provides no E2E execution output and marks all five scenarios PASS based on source references. If the test was not run, mark these scenarios untested or conditional. Otherwise, include output from RUN_E2E=1 GITHUB_TOKEN=... cargo test --test test_catalog_integrity -- --nocapture.
🤖 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
`@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/verify-report.md`
around lines 62 - 70, The e2e-testing table in the verification report
incorrectly marks unexecuted scenarios as passing. Update the five scenario
statuses to untested or conditional unless live execution was performed, and
only mark them PASS when accompanied by output from the test_catalog_integrity
command with RUN_E2E=1.
Comment on lines
+294
to
+309
| match tokio::runtime::Handle::try_current() { | ||
| Ok(handle) => handle.block_on(resolve_via_search_http( | ||
| &url, | ||
| std::time::Duration::from_secs(10), | ||
| id, | ||
| )), | ||
| Err(_) => { | ||
| let rt = tokio::runtime::Runtime::new() | ||
| .map_err(|e| anyhow::anyhow!("failed to create runtime: {}", e))?; | ||
| rt.block_on(resolve_via_search_http( | ||
| &url, | ||
| std::time::Duration::from_secs(10), | ||
| id, | ||
| )) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- provider.rs relevant symbols and call sites ---'
rg -n -C 8 'resolve_via_search|Handle::try_current|block_on|resolve' src/skills/provider.rs
printf '%s\n' '--- related provider-resolution call sites ---'
rg -n -C 5 'resolve_via_search|resolve_provider|provider.*resolve|skill.*provider' src tests openspec 2>/dev/null | head -n 500
printf '%s\n' '--- specification sections ---'
sed -n '1,30p' openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/skill-recommendations/spec.md
sed -n '895,920p' openspec/specs/skill-recommendations/spec.mdRepository: dallay/agentsync
Length of output: 46779
🌐 Web query:
Tokio Handle::block_on panics called within asynchronous execution context documentation current behavior
💡 Result:
Calling Tokio's Handle::block_on from within an asynchronous execution context causes the thread to panic [1][2][3]. This behavior is explicitly documented as a safety mechanism to prevent blocking an executor thread, which could lead to deadlocks or runtime instability [4]. According to the official documentation, the panic occurs if the method is called from within an asynchronous context, such as inside another Runtime::block_on, Handle::block_on, or from a function annotated with #[tokio::main] [1][2][5]. The runtime enforces this to ensure that asynchronous tasks do not block the worker threads responsible for driving other asynchronous operations [4]. If you need to execute blocking code or block the current thread to wait for a future within an asynchronous context, the recommended alternative is to use tokio::task::block_in_place [6][7]. This function informs the executor that the current task is about to block, allowing the executor to potentially hand off remaining tasks to other worker threads while the current thread is occupied [7]. Once inside a block_in_place closure, it is safe to use Handle::block_on to re-enter the asynchronous context if necessary [7]. Top results: [1], [2], [7], [4], [3]
Citations:
- 1: https://docs.rs/tokio/latest/src/tokio/runtime/handle.rs.html
- 2: https://docs.rs/tokio/latest/tokio/%20runtime/struct.Handle.html?search=std%3A%3Avec
- 3: https://docs.rs/tokio/latest/tokio/runtime/struct.Handle.html
- 4: https://github.com/tokio-rs/tokio/blob/306ed1c3/tokio/src/future/block_on.rs
- 5: https://docs.rs/tokio/latest/aarch64-unknown-linux-gnu/tokio/runtime/struct.Handle.html
- 6: docs:
block_onshould mentionblock_in_placetokio-rs/tokio#7387 - 7: https://docs.rs/tokio/latest/tokio/task/fn.block_in_place.html
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Cargo Tokio configuration ---'
rg -n -C 3 'tokio|reqwest' Cargo.toml Cargo.lock
printf '%s\n' '--- full SkillsShProvider implementation and public provider API ---'
sed -n '230,410p' src/skills/provider.rs
printf '%s\n' '--- provider tests around all resolution paths ---'
sed -n '1,260p' tests/unit/provider.rs
sed -n '410,520p' src/skills/provider.rs
printf '%s\n' '--- async contexts calling synchronous recommendation/install APIs ---'
rg -n -C 8 '#\[tokio::test\]|async fn|install_selected_with|install_all|provider\.resolve\(' src tests -g '*.rs' | head -n 700
printf '%s\n' '--- archived and active specification references ---'
rg -n -C 4 'bridge pattern|handle\.block_on|try_current|Provider Skill Resolution Uses Async HTTP|resolve_via_search' openspecRepository: dallay/agentsync
Length of output: 50373
Remove the active-runtime Handle::block_on() path.
SkillsShProvider::resolve() is synchronous, but its search path calls Handle::block_on() when Handle::try_current() succeeds. Tokio panics when Handle::block_on() runs inside an asynchronous execution context. Split resolution into async and synchronous entry points. Async callers must await the async path. The synchronous path may create and use a temporary runtime only when no runtime is active. Add a #[tokio::test] for the public resolution path, and update both specifications to remove the handle.block_on() requirement and define the async and synchronous paths.
📍 Affects 3 files
src/skills/provider.rs#L294-L309(this comment)openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/skill-recommendations/spec.md#L7-L10openspec/specs/skill-recommendations/spec.md#L905-L910
🤖 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/skills/provider.rs` around lines 294 - 309, Remove the active-runtime
Handle::block_on path from SkillsShProvider::resolve: add separate async and
synchronous resolution entry points, have async callers await the async path,
and let the synchronous path create a temporary runtime only when no runtime is
active. Add a #[tokio::test] covering public resolution, and update
src/skills/provider.rs:294-309,
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/skill-recommendations/spec.md:7-10,
and openspec/specs/skill-recommendations/spec.md:905-910 to remove the
handle.block_on requirement and document both paths.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.md (1)
30-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the
HttpStatustest contract.Line 32 uses the obsolete tuple-style variant. The recorded contract on line 26 and the implementation use
UpdateCheckError::HttpStatus { url, status }.Proposed fix
-- [x] 2.3 RED: Add test — `test_fetch_latest_version_404` returns HTTP 404, expects `UpdateCheckError::HttpStatus(404)` +- [x] 2.3 RED: Add test — `test_fetch_latest_version_404` returns HTTP 404, expects `UpdateCheckError::HttpStatus { status: 404, .. }`🤖 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 `@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.md` around lines 30 - 33, Update the `test_fetch_latest_version_404` expectation to use the struct-style `UpdateCheckError::HttpStatus { url, status }` variant, matching the contract and implementation. Preserve the existing 404 status assertion and provide the expected request URL in the pattern.
🤖 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 @.atl/skill-registry.md:
- Around line 31-32: Update the registry entries for HTTP, Tokio, and relevant
code paths to reflect the async implementation: remove stale reqwest::blocking
references, add Tokio’s time feature, and document the runtime bridges used by
the async flows.
In
`@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/proposal.md`:
- Around line 57-59: Update the `Cargo.toml` section in the proposal by adding a
blank line immediately after the `#### 4. Cargo.toml` heading and another blank
line before the TOML code fence.
---
Duplicate comments:
In `@openspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.md`:
- Around line 30-33: Update the `test_fetch_latest_version_404` expectation to
use the struct-style `UpdateCheckError::HttpStatus { url, status }` variant,
matching the contract and implementation. Preserve the existing 404 status
assertion and provide the expected request URL in the pattern.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 11795914-ea1a-48c7-b0ee-079e331c27bf
📒 Files selected for processing (16)
.atl/skill-registry.mdCargo.tomlopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/archive-report.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/design.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/exploration.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/proposal.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/qa-report.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/dependency-management/spec.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/specs/version-check/spec.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/state.yamlopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/tasks.mdopenspec/changes/archive/2026-08-11-issue-496-async-http-refactor/verify-report.mdopenspec/specs/dependency-management/spec.mdopenspec/specs/version-check/spec.mdsrc/skills/provider.rssrc/update_check.rs
Comment on lines
+31
to
+32
| | HTTP | reqwest 0.13 (blocking feature present) | | ||
| | Async runtime | Tokio (rt-multi-thread, macros, fs) | |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'reqwest\s*=|tokio\s*=' Cargo.toml
rg -n 'reqwest::blocking|reqwest::Client|tokio::time::sleep|Handle::try_current|Runtime::new' src tests
rg -n 'blocking feature present|reqwest::blocking|Tokio \(' .atl/skill-registry.mdRepository: dallay/agentsync
Length of output: 1675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- registry ---'
cat -n .atl/skill-registry.md | sed -n '24,60p'
printf '%s\n' '--- update_check ---'
cat -n src/update_check.rs | sed -n '90,135p;190,225p'
printf '%s\n' '--- provider ---'
cat -n src/skills/provider.rs | sed -n '275,335p'
printf '%s\n' '--- catalog test ---'
cat -n tests/test_catalog_integrity.rs | sed -n '1,95p'
printf '%s\n' '--- all relevant dependency/API references ---'
rg -n 'reqwest::blocking|reqwest::Client|tokio::time::sleep|Handle::try_current|Runtime::new|blocking feature present|Tokio \(' Cargo.toml src tests .atl/skill-registry.mdRepository: dallay/agentsync
Length of output: 12930
Update the registry to match the async implementation.
Change the HTTP, Tokio, and relevant code-path entries to remove stale reqwest::blocking references, document the Tokio time feature, and describe the runtime bridges.
🤖 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 @.atl/skill-registry.md around lines 31 - 32, Update the registry entries for
HTTP, Tokio, and relevant code paths to reflect the async implementation: remove
stale reqwest::blocking references, add Tokio’s time feature, and document the
runtime bridges used by the async flows.
|
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.
Description
Standardizes all HTTP operations on async reqwest, eliminating
reqwest::blockingfrom the codebase. This removes blocking HTTP from the Tokio runtime, improving scheduler efficiency and aligning with the existing bridge pattern ininstall.rs.Fixes #496
Changes
src/update_check.rs:fetch_latest_version()converted to async with newUpdateCheckErrorenum carrying useful context (timeout duration, URL, HTTP status). Background spawn now usestokio::spawninstead ofstd::thread.src/skills/provider.rs:resolve_via_search()now uses asyncreqwest::Clientvia the bridge pattern (Handle::try_current()+Runtime::new()fallback), mirroringinstall.rs:244-253.tests/test_catalog_integrity.rs: Converted to#[tokio::test]with async client.Cargo.toml: Removedblockingfeature from reqwest.Acceptance Criteria
Type of change
How Has This Been Tested?
cargo test --all-features— 575 lib tests + 185 integration tests passcargo clippy --all-targets --all-features -- -D warnings— cleancargo fmt --all -- --check— cleanTest Configuration:
cargo test --all-featuresChecklist: