Remote configuration improvements

[helq]

On top of #415

Commits 7f947df and 82864af.

Two highly correlated changes:

Support map-format deserialization: allow custom_remotes in Settings.toml to be specified as either a TOML array ([[...]]) or as indexed environment variables (CUSTOM_REMOTES__0__URL), matching the pattern used by the config crate.

Add branch_patterns for selective fetching: when a custom remote has thousands of branches, fetching all refs downloads millions of objects and saturates the pod for 30 minutes. branch_patterns accepts glob patterns (e.g., /6.18, 14) so git fetch only downloads matching refs. The same patterns filter baseline branch candidates.

[rgushchin]

🤖 AI Code Review (Sashiko / Jetski)

Here is a review of PR #416 (remote-config):

---

📋 Summary

The PR introduces map-format deserialization for custom_remotes and adds branch_patterns for selective refspec fetching. While the deserialization pattern and glob-based branch candidate filtering are solid ideas, there are critical regression and performance risks that should be addressed before merging.

---

🚨 1. Critical Regression Risks

1.1 Complete Removal of git fetch Timeout (src/git_ops.rs)

• Issue: The original implementation wrapped the git fetch invocation (and retry) in tokio::time::timeout(timeout_duration, fetch_future) (30 min for initial, 5 min for routine updates). In PR Remote configuration improvements #416, tokio::time::timeout was removed entirely from ensure_remote_with_refspecs.
• Impact: If an external remote hangs, stalls on SSL/SSH negotiation, or suffers network partitioning, fetch_cmd.output().await will block indefinitely. Because ensure_remote is called during server boot (main.rs), candidate baseline resolution (baseline.rs), and periodic sync (sync.rs), any unresponsive remote will hang the entire server or worker loop indefinitely.
• Fix: Re-introduce tokio::time::timeout around fetch_cmd.output().await and retry_cmd.output().await.

1.2 Fetch Caching Permanently Bypassed for Selective Remotes (src/git_ops.rs)

• Issue: In ensure_remote_with_refspecs, should_fetch is determined by:

  let head_exists = Command::new("git").args(["show-ref", "--verify", "-q", &head_ref])...;
  let should_fetch = if just_added || !head_exists || force_fetch { true } else { ... age check ... };

  When selective refspecs are fetched (or when HEAD is not among the fetched refs), refs/remotes/<name>/HEAD is never created, and git remote set-head <name> --auto fails.
• Impact: head_exists evaluates to false on every invocation. As a result, the 1-hour timestamp freshness check is completely bypassed, forcing a synchronous network git fetch on every call to BaselineRegistry::get_candidates for every patchset processed.
• Fix: Avoid treating !head_exists as a mandatory trigger to fetch when a valid, fresh timestamp file exists in .sashiko/fetch_timestamps/.

---

⚠️ 2. Architectural & Functional Concerns

2.1 Unfiltered Full Fetches on Startup and in Sync Worker

• Startup (src/main.rs:856): Server startup iterates over custom_remotes and calls ensure_remote(&repo_path, &remote.name, &remote.url, false) without passing branch_patterns (passing refspecs = None). This runs git fetch --prune --no-tags <remote>, downloading all thousands of branches on every server restart.
• Sync Worker (src/worker/sync.rs:77): GitSyncWorker lists remotes via git remote and calls ensure_remote with refspecs = None, triggering an unfiltered fetch of all branches every hour.
• Reviewer Setup (src/reviewer.rs:826): On initial remote setup in Reviewer, ensure_remote is called with refspecs = None.
• Impact: The primary goal—preventing the download of thousands of branches and millions of objects—is defeated on server boot and hourly sync.
• Fix: Thread branch_patterns (or refspecs) into main.rs, GitSyncWorker, and Reviewer when calling ensure_remote.

2.2 Git Refspec Incompatibility with Custom Glob Syntax (src/git_ops.rs vs src/baseline.rs)

• Issue: glob_match supports ? and arbitrary multi-* patterns. However, BaselineRegistry converts patterns directly to refspecs:

  format!("+refs/heads/{}:refs/remotes/{}/{}", p, remote.name, p)

• Impact: Git refspecs do not support ? or multiple * wildcards. If a user sets a pattern like v?.? or *6.18*, git fetch fails immediately with fatal: invalid refspec, causing ensure_remote_with_refspecs to fail and branches to be ignored.
• Fix: Validate that branch_patterns contains valid Git refspec patterns (single * wildcard only, no ?), or document and sanitize the pattern input.

---

💡 3. Code Quality & Maintainability

3.1 Deduplicate Custom Deserializer Visitors (src/settings.rs)

• There are now three nearly identical custom visitors for indexed maps vs sequences:
  • OptVecVisitor (deserialize_optional_string_vec)
  • CustomRemotesVisitor (deserialize_custom_remotes)
  • IndexedVecVisitor<T> (deserialize_indexed_vec)
• Since IndexedVecVisitor<T> is generic over T: Deserialize, it can be reused or adapted to handle Option<Vec<T>>, eliminating ~100 lines of boilerplate.

3.2 Update Documentation

• Consider updating docs/configuration.md to document the new branch_patterns configuration option under [[git.custom_remotes]] along with examples and supported pattern syntax.

[helq]

After a couple of rounds of AI reviews and subsequent fixes, I present to you the new code. It has been hardened and tested in production. Thanks for the review

[rgushchin]

🤖 AI Code Review (Sashiko / Jetski)

Here is a summary of issues that should be addressed before PR #416 is merged:

🚨 Critical Issues & Regressions

1. Fetch Cache Bypass on Every Patchset (src/git_ops.rs)

   • In ensure_remote_with_refspecs, should_fetch triggers if !head_exists. When selective refspecs are fetched, refs/remotes/<name>/HEAD is not created, causing head_exists to always evaluate to false.
   • Impact: Bypasses the 1-hour timestamp freshness check and triggers a synchronous network git fetch on every incoming patchset during baseline candidate search.
   • Fix: Check timestamp age even when !head_exists, or record initial fetch completion in .sashiko/fetch_timestamps/.

2. Unfiltered Full Fetches on Boot and Hourly Sync (src/main.rs, src/worker/sync.rs)

   • main.rs (startup remote initialization) and GitSyncWorker (hourly sync) invoke ensure_remote with refspecs = None.
   • Impact: Fetches all branches and objects for custom remotes on every server start and hourly sync, defeating the pod bandwidth/resource savings.
   • Fix: Pass branch_patterns / refspecs into main.rs and GitSyncWorker.

3. Git Refspec Syntax Incompatibility (src/git_ops.rs vs src/baseline.rs)

   • glob_match supports ? and multiple * wildcards, but Git refspecs reject ? and multiple * (e.g. *6.18*, */*, v?.?).
   • Impact: Configuring such patterns causes git fetch to fail with fatal: invalid refspec, marking the remote as failed and triggering a 1-hour backoff.
   • Fix: Validate/restrict branch_patterns to valid Git refspec syntax (single *, no ?) or document pattern constraints.

4. only_branches Ignored When branch_patterns is Configured (src/baseline.rs)

   • In BaselineRegistry::get_candidates, refspecs is constructed exclusively from branch_patterns.
   • Impact: If a remote defines both only_branches and branch_patterns, only_branches entries are excluded from fetch refspecs.
   • Fix: Merge only_branches into the generated refspecs list.

---

Note: This review was automatically generated by AI.