[Mikola Lysenko (mikolalysenko)]

LLM Description written by Claude Code:claude-opus-5-5

This PR is an architecture refactor sweep of crates/socket-patch-core/src/api/{blob_fetcher,date,retry,types}.rs. It works through the audit notes for those rows in coverage/files.tsv on arch-audit/ledger (discussion #1830, living document Part 4 "structure" and Part 5 "tests"). Behavior, output bytes, JSON and exit codes are unchanged.

Changes

blob_fetcher.rs (prod +22/−70, test +39/−253)

• Inlined format_fetch_result_for into format_fetch_result, its only caller. No other code called it.

• Removed about 40 comment lines: history (#571), section banners, a 16-line justification of stage+rename cut to 3 lines, a 9-line doc on blob_hash_matches cut to 2, and a # Arguments block that restated the signature.

• Replaced 11 format tests with 3. Each new test compares the whole output:

  • format_fetch_result_lists_successes_then_failures
  • format_fetch_result_without_failures: present, downloaded-only, all-skipped and mixed results
  • a_failure_without_an_error_reads_unknown_error

  The deleted tests duplicated display_id_abbreviates_only_long_blob_hashes, failures_are_listed_in_sorted_order_regardless_of_input_order or successes_and_failures_split_cleanly, or used .contains. The two blob_hash_matches tests only exercised eq_ignore_ascii_case. The real path is covered by blob_fetcher_edges_e2e's uppercase-hash and mismatch tests.

date.rs (prod +3/−10, test +4/−15)

• parsed_order_is_chronological_in_both_formats asserted secs > prev_2822, which compares the loop's own counter with itself. I deleted that assert and renamed the test formatted_instants_parse_back_exactly_in_both_formats, which is what its remaining assert_eqs check.
• Replaced a match with map_or.
• Removed a comment that repeated apply_zone's doc, 4 test banners, and a stale "pinned by … below" note.

retry.rs (prod +19/−45, test +2/−4)

• Made is_retryable_status, is_retryable_transport and parse_retry_after pub(crate). Only client.rs uses them.
• Made MAX_RETRIES_CEILING private. The env doc now says "clamped to 10", and max_retries_env_parsing asserts Some(10), so the documented value is pinned.
• Deleted API_CONNECT_TIMEOUT and API_READ_TIMEOUT. They were pub only so that api_timeout_e2e could assert const == literal. Their values moved into ApiTimeouts::default, and their docs moved onto the fields. The e2e test is now default_bounds_are_10s_connect_60s_read.
• Shortened the module doc from 34 lines to 6. It restated the ApiRetryPolicy field docs and the delay doc.

types.rs (test +41/−227)

• Deleted 10 tests: serialize-side .contains checks and round trips. The payload-deserialize tests already pin the same field names.
• scan --json writes BatchPackagePatches verbatim, so I replaced their serialize checks with one structured test, batch_packages_serialize_with_the_wire_keys, which compares the whole to_value.
• published_at_is_per_patch_not_per_package asserted facts about its own fixture literals. It is now deserializes_a_live_by_package_payload and keeps only the asserts on the parsed values.

Evidence

Each mutation below turns the named test red:

Mutation	Test that fails
format_fetch_result returns "" when nothing happened	format_fetch_result_without_failures
ApiTimeouts::default().read set to 61 s	default_bounds_are_10s_connect_60s_read
rename_all = "camelCase" dropped from BatchPatchInfo	batch_packages_serialize_with_the_wire_keys

Other checks:

• cargo clippy --workspace --all-features -- -D warnings is clean.
• cargo test -p socket-patch-core --lib: 6,976 pass. The 4 that fail are the known root-sandbox failures, which pass in CI: copy_tree symlinked root, vlt_heal unremovable lock, and the pypi_poetry and pypi_requirements wire-failure tests.
• blob_fetcher_edges_e2e, api_timeout_e2e and api_retry_e2e pass.

Totals

git diff --stat: 5 files, +139/−633.

• Production: +44/−125.
• Tests: +95/−508.

Not done here

Each of these would change lines that open PRs also change:

• Dropping the dead on_progress/OnProgress parameter. All production callers pass None, but the call sites are in fetch_stage.rs (Sweep bun_preflight, composer_hints, context, fetch_stage #1855, next to a line it changes), repair.rs (Sweep remove.rs and repair.rs #1875) and rollback.rs (Sweep hosted_unwind, lock_cli, list, commands/mod (+ rollback dead branch) #1864).
• Deriving BlobFetchResult.success from error. Its literals are in fetch_stage.rs tests (Sweep bun_preflight, composer_hints, context, fetch_stage #1855).
• The OrganizationInfo and api/client.rs notes. That file is in One serve GET and one vendor retry loop in api/client.rs (#1869) #1870 and Build: share one test env guard in core (#1889) #1891.

/code-review high findings

8 findings: 4 fixed, 4 not.

Fixed:

• A private intra-doc link.
• Lost serialize coverage for scan --json's batch packages.
• The ceiling doc drifting from the code.
• A clumsy test literal.

Not fixed:

• Narrowing pub items of the published core crate. Nothing outside the workspace is a supported consumer of these internals, and earlier sweeps (Sweep core constants.rs and utils (concurrent, durability, …) #1907, Sweep core utils http, mod and hatch #1914, Sweep json_envelope, update_notifier, main, path_scope #1967) narrowed core pub items the same way.
• The deleted fixture-only asserts. The assert_ne! on two publishedAt literals checked the fixture, not the code. The published_at doc keeps the semantics.
• The removed apply_zone inline comment. The function's doc comment says the same thing (RFC 2822 §4.3 → -0000).
• Timeout literals in the test. The test pins the documented defaults, which is the point of it.

Behavior change

None.

Risk

Low. The only non-test code changes are visibility, inlined constants and one inlined formatter. No cfg(windows)/cfg(unix) code changed.

🤖 Generated with Claude Code

https://claude.ai/code/session_011f9aeQX98fpZ3NhqMfqsjr

---

Note

Low Risk
Changes are mostly docs, test consolidation, and crate-internal visibility; production logic paths are unchanged aside from inlining a formatter and moving timeout literals into Default.

Overview
This PR cleans up four socket-patch-core API modules (blob_fetcher, date, retry, types) plus the timeout e2e test—no intended behavior, JSON, or CLI output changes.

In blob_fetcher, format_fetch_result_for is inlined into format_fetch_result (its only caller), verbose comments are trimmed, and many overlapping format tests are replaced with a few whole-string assertions.

In retry, retry helpers (is_retryable_status, is_retryable_transport, parse_retry_after) become pub(crate), MAX_RETRIES_CEILING is private (env doc still says clamp to 10), and the public API_CONNECT_TIMEOUT / API_READ_TIMEOUT constants are removed in favor of literals on ApiTimeouts::default; api_timeout_e2e now asserts those defaults directly. Module docs are shortened.

In types, redundant serde round-trip / .contains tests are dropped in favor of payload-shaped deserialize tests and one structured batch_packages_serialize_with_the_wire_keys test for scan --json wire keys.

date.rs gets small comment/test tidy-ups (e.g. RFC2822 prefix via map_or, a renamed round-trip test without a bogus monotonic assert).

^{Reviewed by Cursor Bugbot for commit 0b99f82. Configure here.}

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

BugBot review

---

Generated by Claude Code

[cursor]

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 0b99f82. Configure here.}

[Mikola Lysenko (mikolalysenko)]

Burn-down agent: labeled Ready for review.

• Head: 0b99f82a77f510751288620cde69b277d939e0bb
• CI: 82/82 checks passed or skipped on this head, no failures
• Bugbot: reviewed this head, no new issues
• Mergeable, no CHANGELOG.md changes
• For reviewers: cleanup-only sweep of socket-patch-core/src/api/{blob_fetcher,date,retry,types}.rs plus tests/api_timeout_e2e.rs. Check that the tests removed as fake or duplicate don't drop real coverage.

---

Generated by Claude Code

[Mikola Lysenko (mikolalysenko)]

Final review brief

What it does: A cleanup of four socket-patch-core API modules (blob_fetcher, date, retry, types) that is meant not to change behavior. It inlines a one-caller formatter, moves the timeout constants into ApiTimeouts::default, makes the retry helpers pub(crate), cuts restating comments, and replaces many overlapping or .contains tests with a few whole-value assertions.

Risk: Low. Production code changes only in visibility, inlined constants and one inlined formatter, and the inlined formatter gives the same output (BLOB.many is "blobs"). The only external effect: API_CONNECT_TIMEOUT, API_READ_TIMEOUT and MAX_RETRIES_CEILING leave the public API of the core crate, and three retry helpers become pub(crate). Nothing in the workspace uses them outside api/.

Look here:

• retry.rs:39 and retry.rs:139: ceiling made private, and timeouts inlined into Default
• retry.rs:164: retry helpers narrowed to pub(crate) (only client.rs calls them)
• blob_fetcher.rs:187: format_fetch_result_for inlined
• types.rs:306: one structured serialize test in place of the .contains checks (the scan --json wire keys)
• api_timeout_e2e.rs: the defaults are now pinned as literals

Verified: I read the full diff (5 files, +139/−633). I grepped the workspace for every removed or narrowed item and found no outside users. I checked that the inlined formatter is the same code. cargo test -p socket-patch-core --lib api:: passes locally: 245/245. The branch is up to date with main, CHANGELOG.md is untouched, and there are no unresolved threads. CI on the head: 32 passed, 50 skipped, 0 failed, with ci-ok and clippy green. Bugbot reviewed this commit (Low Risk) and left no findings.

Changes I made: None.

Open questions: Removing public items from socket-patch-core is a semver break for anyone who uses the crate directly. The PR treats those internals as unsupported, as earlier sweeps did (#1907, #1914, #1967). Approve only if you agree.

Auto-merge is armed, so approving sends this PR straight to the merge queue.

---

Generated by Claude Code