Fix remote template sync against local Hub storage

[mfreeman451]

Summary

• fall back to Hub template file APIs when a remote Hub returns local-storage file:// URLs
• use the same fallback in start template resolution updates, not just templates sync
• skip non-git workspace bootstrap for the synthetic global grove so scion start --global does not walk the caller home directory

Validation

• go test ./cmd -run 'TestRunTemplateSync_|TestPullTemplateFromHubMatch_|TestUpdateHubTemplate_|TestStartAgentViaHub_GlobalGroveSkipsWorkspaceBootstrap|TestStartAgentViaHub_EnvGatherFailureCleansUp' -count=1
• go test ./pkg/templatecache -run 'TestHydrateSuccess' -count=1
• live repro: scion start --global --upload-template ... no longer fails with /home/scion permission denied and successfully starts a hosted agent in namespace scion

[mfreeman451]

lgtm

[mfreeman451]

Cleaned up the best-effort hub availability handling without broadening the branch.

Changes:

• replaced repeated hubCtx, _ = CheckHubAvailabilityWithOptions(...) call-site patterns with a small checkHubAvailabilityBestEffort(...) helper
• behavior stays best-effort for the read/list/show/delete/clone paths
• failures are now debug-logged instead of being silently discarded

Current head: 16a7b409

[ptone]

PR #121 Review: Fix remote template sync against local Hub storage

Executive Summary

This PR adds a fallback mechanism so that when a remote Hub returns local-storage file:// URLs (unusable by the CLI client), template upload/download operations transparently route through Hub-side file APIs instead. The change is low-to-medium risk -- the refactoring is clean and well-tested, though there are two issues worth addressing around memory safety for large templates and a missing templateID sanitization concern.

---

Critical Issues

1. UploadFilesMultipart buffers entire template payload in memory

File: pkg/hubclient/templates.go:332-358

All files are read into a single bytes.Buffer before sending the HTTP request. For templates with large assets, this could cause OOM conditions. The existing signed-URL path streams files individually, so this is a regression in the fallback path.

// Current (lines 333-343)
var body bytes.Buffer
writer := multipart.NewWriter(&body)
for _, file := range files {
    if err := appendMultipartFile(writer, file); err != nil {
        return err
    }
}

Suggested Fix: Use io.Pipe to stream the multipart body, or batch files into smaller groups. At minimum, add a size check before buffering:

func (s *templateService) UploadFilesMultipart(ctx context.Context, templateID string, files []FileInfo) error {
    pr, pw := io.Pipe()
    writer := multipart.NewWriter(pw)

    go func() {
        for _, file := range files {
            if err := appendMultipartFile(writer, file); err != nil {
                pw.CloseWithError(err)
                return
            }
        }
        pw.CloseWithError(writer.Close())
    }()

    req, err := http.NewRequestWithContext(ctx, http.MethodPost,
        s.c.transport.BaseURL+"/api/v1/templates/"+templateID+"/files", pr)
    // ...
}

Severity: Medium -- not blocking but should be addressed before this path handles real-world templates of arbitrary size.

2. templateID is not sanitized in URL construction

File: pkg/hubclient/templates.go:347, 362

templateID is concatenated directly into URL paths. While templateID likely comes from the Hub server and is server-controlled, if a crafted ID contained path-traversal characters (e.g. ../), it could alter the request target. This is consistent with existing code patterns in the file (lines 237, 255, 264, etc.), so this is not a new vulnerability introduced by this PR -- but the new UploadFilesMultipart constructs the URL manually via string concatenation rather than going through s.c.transport.Post(), which may apply different validation.

req, err := http.NewRequestWithContext(ctx, http.MethodPost,
    s.c.transport.BaseURL+"/api/v1/templates/"+templateID+"/files", &body)

Suggested Fix: Use url.PathEscape(templateID) for consistency:

s.c.transport.BaseURL+"/api/v1/templates/"+url.PathEscape(templateID)+"/files"

Severity: Low -- the templateID is server-sourced, but defense-in-depth is prudent, especially since UploadFilesMultipart bypasses the normal transport helper methods.

---

Observations

3. hasLocalSignedURLs / hasLocalDownloadURLs are nearly identical

Files: cmd/templates.go:34-44

These two functions have identical logic with different input types. Consider a generic helper or consolidating on a shared URL-extraction pattern to reduce duplication:

func hasLocalSignedURLs(urls []hubclient.UploadURLInfo) bool { ... }
func hasLocalDownloadURLs(urls []hubclient.DownloadURLInfo) bool { ... }

This is minor -- Go generics could unify them, but it's a style preference rather than a bug.

4. resp.Body not explicitly closed in UploadFilesMultipart

File: pkg/hubclient/templates.go:353-357

resp, err := s.c.transport.Do(ctx, req)
if err != nil {
    return err
}
return apiclient.CheckResponse(resp)

Verify that apiclient.CheckResponse or s.c.transport.Do closes resp.Body. If CheckResponse returns nil, the body may leak. This is likely handled by the existing transport layer, but worth confirming.

5. Duplicate switch case in mock server

File: cmd/templates_test.go:524-531 and cmd/templates_test.go:568-578

The mock newMockHubServerForLocalStorageTemplateFallback has two cases matching /api/v1/templates/local-storage-tpl-id/download with GET. The second one (line 568) is unreachable because the switch will always match the first. This doesn't affect correctness since the test only hits it once, but it's dead code.

6. Error message string matching for retry logic is fragile

File: cmd/templates.go:1157

if !strings.Contains(err.Error(), "file not found") {

This already existed but is now exercised by the new retry path. Matching on error substrings is brittle -- a server-side message change will silently break the retry. Not introduced by this PR, so not blocking.

---

Positive Feedback

• Clean refactoring: The extraction of upload/download logic into cmd/template_transfer.go with clear function boundaries (uploadTemplateFiles, uploadTemplateFilesThroughHubAPI, uploadTemplateFilesBySignedURL) is well-structured and reduces code duplication across both syncTemplateToHub and updateHubTemplate.
• Good test coverage: The PR includes 5 new test functions covering the happy path for local-storage fallback in sync, retry, pull, and update flows. The mock servers are thorough.
• The IsGlobal fix is surgical and correct: Adding && !hubCtx.IsGlobal to skip workspace bootstrap for the global grove prevents the dangerous CollectFiles call on the user's home directory -- this was a real security/reliability bug.
• escapePathSegments is correctly implemented for the ReadFile path -- it splits on / and escapes each segment individually, preserving directory separators while encoding special characters.

---

Final Verdict

Approve with minor suggestions. The core logic is correct and well-tested. The memory buffering concern (#1) is worth a follow-up but is not blocking for typical template sizes. The IsGlobal workspace bootstrap fix is an important correctness improvement. Ship it.

[ptone]

non-blocking review - but I'll leave for a bit in case you want to tune up

[mfreeman451]

This branch is empty against current main after the recent upstream merges, so I am closing it as superseded.