Fix clone_repo to Always Clone from Remote URL

[Trecek]

Summary

clone_repo previously fell back to the local filesystem path as the clone source in two bug scenarios inside _resolve_clone_source: when a branch was not found on the remote (ls-remote exit code 2) and when a network timeout occurred during ls-remote. When a remote URL is configured, the clone source must always be the remote — never the local filesystem — to preserve the isolation guarantee.

The fix simplifies _resolve_clone_source to a single-expression function: remove the branch parameter and the ls-remote network check, and always return the remote URL when one is available. The no-remote fallback to local path is retained as the only legitimate use of local as a clone source.

Architecture Impact

Process Flow Diagram

%%{init: {'flowchart': {'nodeSpacing': 50, 'rankSpacing': 60, 'curve': 'basis'}}}%%
flowchart TB
    %% CLASS DEFINITIONS %%
    classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff;
    classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff;
    classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff;
    classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff;
    classDef newComponent fill:#2e7d32,stroke:#81c784,stroke-width:2px,color:#fff;

    %% TERMINALS %%
    START([clone_repo called])
    SUCCESS([return clone_path + remote_url])
    WARN_UNCOMMITTED([return uncommitted_changes dict])
    WARN_UNPUBLISHED([return unpublished_branch dict])
    WARN_REWRITE([return remote_url_rewrite_failed dict])
    ERROR_VAL([raise ValueError])
    ERROR_RT([raise RuntimeError])

    subgraph Resolution ["Source Resolution"]
        direction TB
        AutoDetect{"source_dir<br/>━━━━━━━━━━<br/>empty?"}
        CwdDetect["detect_source_dir(cwd)<br/>━━━━━━━━━━<br/>git rev-parse --show-toplevel"]
        ValidateDir{"Path.is_dir()?"}
        BranchDetect{"branch<br/>━━━━━━━━━━<br/>empty?"}
        DetectBranch["detect_branch(source)<br/>━━━━━━━━━━<br/>git rev-parse --abbrev-ref HEAD"]
    end

    subgraph Guards ["Strategy Guards"]
        direction TB
        StratCheck{"strategy<br/>━━━━━━━━━━<br/>== ''?"}
        UncommittedGuard["detect_uncommitted_changes<br/>━━━━━━━━━━<br/>git status --porcelain"]
        DirtyCheck{"changes<br/>━━━━━━━━━━<br/>found?"}
        UnpublishedGuard["detect_unpublished_branch<br/>━━━━━━━━━━<br/>git ls-remote origin"]
        UnpubCheck{"branch missing<br/>━━━━━━━━━━<br/>on remote?"}
    end

    subgraph CloneSetup ["Clone Setup"]
        direction TB
        TimestampPath["compute clone_path<br/>━━━━━━━━━━<br/>runs_parent / run_name-timestamp"]
        GetRemoteURL["git remote get-url origin<br/>━━━━━━━━━━<br/>detected_url from source<br/>timeout=30s"]
    end

    subgraph StrategyBranch ["Strategy Branch"]
        direction TB
        StrategyCheck{"● strategy<br/>━━━━━━━━━━<br/>== clone_local?"}
        LocalCopy["shutil.copytree<br/>━━━━━━━━━━<br/>copy working tree"]
        ResolveSource["● _resolve_clone_source<br/>━━━━━━━━━━<br/>detected_url truthy?"]
        UseRemote["Use remote URL<br/>━━━━━━━━━━<br/>always when URL known<br/>— no ls-remote check"]
        UseLocal["Use local path<br/>━━━━━━━━━━<br/>only when no remote<br/>URL configured"]
        GitClone["git clone [--branch B]<br/>━━━━━━━━━━<br/>clone_source → clone_path"]
        CloneOK{"returncode<br/>━━━━━━━━━━<br/>== 0?"}
    end

    subgraph PostClone ["Post-Clone"]
        direction TB
        EffectiveURL["effective_url<br/>━━━━━━━━━━<br/>remote_url arg or detected_url"]
        RewriteOrigin["git remote set-url origin<br/>━━━━━━━━━━<br/>enforce effective_url"]
        RewriteOK{"rewrite<br/>━━━━━━━━━━<br/>succeeded?"}
        Decontam["decontaminate<br/>━━━━━━━━━━<br/>untrack + delete<br/>generated files"]
    end

    %% FLOW: Source Resolution %%
    START --> AutoDetect
    AutoDetect -->|"yes"| CwdDetect
    AutoDetect -->|"no"| ValidateDir
    CwdDetect --> ValidateDir
    ValidateDir -->|"false"| ERROR_VAL
    ValidateDir -->|"true"| BranchDetect
    BranchDetect -->|"yes"| DetectBranch
    BranchDetect -->|"no"| StratCheck
    DetectBranch --> StratCheck

    %% FLOW: Guards %%
    StratCheck -->|"yes"| UncommittedGuard
    StratCheck -->|"no (proceed / clone_local)"| TimestampPath
    UncommittedGuard --> DirtyCheck
    DirtyCheck -->|"yes"| WARN_UNCOMMITTED
    DirtyCheck -->|"no"| UnpublishedGuard
    UnpublishedGuard --> UnpubCheck
    UnpubCheck -->|"yes"| WARN_UNPUBLISHED
    UnpubCheck -->|"no"| TimestampPath

    %% FLOW: Clone Setup %%
    TimestampPath --> GetRemoteURL
    GetRemoteURL --> StrategyCheck

    %% FLOW: Strategy Branch %%
    StrategyCheck -->|"yes"| LocalCopy
    StrategyCheck -->|"no"| ResolveSource
    ResolveSource -->|"URL configured"| UseRemote
    ResolveSource -->|"no URL"| UseLocal
    UseRemote --> GitClone
    UseLocal --> GitClone
    LocalCopy --> EffectiveURL
    GitClone --> CloneOK
    CloneOK -->|"no"| ERROR_RT
    CloneOK -->|"yes"| EffectiveURL

    %% FLOW: Post-Clone %%
    EffectiveURL --> RewriteOrigin
    RewriteOrigin --> RewriteOK
    RewriteOK -->|"failed + caller supplied remote_url"| WARN_REWRITE
    RewriteOK -->|"ok or auto-detected"| Decontam
    Decontam --> SUCCESS

    %% CLASS ASSIGNMENTS %%
    class START,SUCCESS,WARN_UNCOMMITTED,WARN_UNPUBLISHED,WARN_REWRITE,ERROR_VAL,ERROR_RT terminal;
    class AutoDetect,BranchDetect,StratCheck,DirtyCheck,UnpubCheck,StrategyCheck,CloneOK,RewriteOK stateNode;
    class CwdDetect,DetectBranch,TimestampPath,GetRemoteURL,GitClone,LocalCopy,EffectiveURL,RewriteOrigin,Decontam handler;
    class UncommittedGuard,UnpublishedGuard detector;
    class ResolveSource stateNode;
    class UseRemote,UseLocal phase;

Loading

Color Legend:

Color	Category	Description
Dark Blue	Terminal	Start, success, warning returns, and error states
Teal	State	Decision and routing nodes (● = modified)
Orange	Handler	Processing, git command execution, path computation
Red	Detector	Uncommitted/unpublished validation guards
Purple	Phase	Clone source options (remote URL vs local path)

Closes #367

Implementation Plan

Plan file: /home/talon/projects/autoskillit-runs/clone-fix-20260312-173207/temp/make-plan/clone_remote_fix_plan_2026-03-12_173652.md

Token Usage Summary

No token data accumulated for this pipeline run.

🤖 Generated with Claude Code via AutoSkillit

[Trecek]

AutoSkillit PR Review — Verdict: changes_requested (REQUEST_CHANGES unavailable on own PR; posting as COMMENT)

[Trecek]

[warning] tests: test_clone_uses_remote_when_branch_not_on_remote substantially overlaps with test_strategy_proceed_with_unpublished_branch_fails_from_remote (same fixture, branch, expected RuntimeError). The only unique value — asserting clone source — is undermined by the fragile index noted separately.

[Trecek]

[warning] tests: Fragile positional index clone_calls[0][0][0][-2] extracts clone source without validating command list length; silently returns the wrong element if git clone gains/loses arguments (e.g. --no-hardlinks, --depth). Use a named extraction relative to known flags instead.

[Trecek]

[info] tests: test_url_returned_regardless_of_branch_existence is redundant with test_returns_url_when_configured: both pass a non-empty URL and assert it is returned. _resolve_clone_source no longer accepts a branch parameter, so there is nothing to distinguish the two tests.

[Trecek]

[info] cohesion: TestResolveCloneSource imports _resolve_clone_source inside each individual test method instead of at module scope, inconsistent with the import-locality pattern used elsewhere in the test suite.

[Trecek]

[info] cohesion: test_url_returned_regardless_of_branch_existence name implies branch-parameter testing, but the function no longer accepts a branch parameter after simplification; the name is misleading.

[Trecek]

[info] slop: Docstring for _resolve_clone_source is over-verbose for a one-liner function; four sentences including a redundant note about clone_local that duplicates the module docstring.

[Trecek]

[info] slop: Docstring contains historical 'Previously... After the fix' narrative; tests should document the current invariant, not a changelog.

[Trecek]

[info] slop: Docstring contains historical 'Previously... After the fix' narrative; only the current invariant belongs in a test docstring.