Fix repo tagging for git worktrees and non-standard clone directories

[winjer]

User description

Derives the repo: tag from the origin remote URL instead of the show-toplevel directory basename, with a show-toplevel fallback when no remote exists or the URL is unparseable. Fixes misleading repo tags for git worktrees and ticket-named clone directories.

Adds repo_from_remote_url (sibling to the existing org_from_remote_url) plus unit and integration tests.

Design: design.md on this branch

---

PR Type

Bug fix, Tests, Documentation

---

Description

• Derive repo: tag from origin remote URL instead of show-toplevel basename

• Add repo_from_remote_url function mirroring existing org_from_remote_url

• Fall back to show-toplevel basename when no remote or unparseable URL

• Add unit and integration tests plus design/implementation documentation

---

Diagram Walkthrough

flowchart LR
  A["gather_env_tags"] --> B["git remote get-url origin"]
  B --> C{"URL present?"}
  C -- "Yes" --> D["repo_from_remote_url"]
  D --> E{"Parseable?"}
  E -- "Yes" --> F["Push repo: from URL"]
  E -- "No" --> G["Fallback: show-toplevel basename"]
  C -- "No" --> G
  B --> H["org_from_remote_url"]
  H --> I["Push org: tag"]

Loading

File Walkthrough

	Relevant files
Bug fix	tags.rs Add repo_from_remote_url and restructure tag derivation    src/tags.rs Added repo_from_remote_url function to extract repo name from remote URL (SCP, ssh://, https://) Restructured gather_env_tags to derive repo: from origin remote URL with show-toplevel fallback via closure Added 6 unit tests for repo_from_remote_url covering various URL forms and local path Added 2 integration tests for gather_env_tags repo tag derivation with and without remotes +155/-7
Documentation	design.md Design document for repo tag fix                                                  planflow/changes/fix-repo-tag-worktree/design.md Design document describing the repo tag fix for worktrees and non-standard clone directories Covers architecture, fallback ordering, implementation phases, and acceptance criteria +140/-0  implementation.md Implementation plan for repo tag fix                                          planflow/changes/fix-repo-tag-worktree/implementation.md Implementation plan with 5 tasks (T1–T5) and dependency graph Includes code snippets, verification commands, and acceptance-criteria traceability +395/-0  spec.md Spec delta for repo tag requirements                                          planflow/changes/fix-repo-tag-worktree/specs/tags/spec.md Spec delta adding requirements for repo tag from remote URL and fallback behavior Includes scenarios for SSH, HTTPS, ssh://, GitLab subgroups, and local path fallback +104/-0  work.md Work plan and task allocation                                                        planflow/changes/fix-repo-tag-worktree/work.md Work plan with task table, dependency graph, and allocation All tasks agent-owned with human checkpoint after T5 +43/-0
Miscellaneous	housekeeping.md Housekeeping note for formatting issue                                      planflow/housekeeping.md Added housekeeping note about pre-existing rustfmt line width issue in gather_env_tags +3/-0

---

[isotoma-pr-agent]

PR Reviewer Guide 🔍

(Review updated until commit fb42c8f)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ No major issues detected

[isotoma-pr-agent]

Persistent review updated to latest commit fb42c8f

[winjer]

Review round 1 feedback:

• Applied (fb42c8f): extracted the duplicated show-toplevel fallback into a push_repo_from_toplevel closure so both fallback paths (unparseable URL / no remote) stay in sync.
• Skipped: nothing else raised — review reported no major issues and no security concerns.

All suites green locally (219 passed, 0 failed; cargo clippy -- -D warnings clean) and CI is green on fb42c8f.

[winjer]

/improve

[isotoma-pr-agent]

PR Code Suggestions ✨

Latest suggestions up to fb42c8f

Category	Suggestion	Impact
Possible issue	Fix closure borrow conflict preventing compilation The closure push_repo_from_toplevel captures &mut tags, which prevents any direct use of tags (e.g., tags.push(format!("repo:{repo}")) and tags.push(format!("org:{org}"))) while the closure value is still in scope. This will fail to compile due to the borrow checker. Replace the closure with inlined blocks to avoid holding a mutable borrow across the entire if/else. src/tags.rs [117-135] -let mut push_repo_from_toplevel = || { - if let Some(toplevel) = git_cmd(&["rev-parse", "--show-toplevel"], cwd) { - if let Some(name) = std::path::Path::new(&toplevel).file_name() { - tags.push(format!("repo:{}", name.to_string_lossy())); - } - } -}; if let Some(url) = git_cmd(&["remote", "get-url", "origin"], cwd) { match repo_from_remote_url(&url) { Some(repo) => tags.push(format!("repo:{repo}")), - None => push_repo_from_toplevel(), + None => { + if let Some(toplevel) = git_cmd(&["rev-parse", "--show-toplevel"], cwd) { + if let Some(name) = std::path::Path::new(&toplevel).file_name() { + tags.push(format!("repo:{}", name.to_string_lossy())); + } + } + } } if let Some(org) = org_from_remote_url(&url) { tags.push(format!("org:{org}")); } } else { // No origin remote: fall back to show-toplevel basename for repo. - push_repo_from_toplevel(); + if let Some(toplevel) = git_cmd(&["rev-parse", "--show-toplevel"], cwd) { + if let Some(name) = std::path::Path::new(&toplevel).file_name() { + tags.push(format!("repo:{}", name.to_string_lossy())); + } + } } Suggestion importance[1-10]: 9 __ Why: The closure push_repo_from_toplevel captures &mut tags, and since it is still in scope when tags.push(format!("repo:{repo}")) and tags.push(format!("org:{org}")) are called directly, the Rust borrow checker will reject this code with a compilation error. Inlining the blocks as suggested resolves the mutable borrow conflict and is a critical fix for the PR to compile.	High

---

Previous suggestions

Suggestions up to commit e64f58e

Category	Suggestion	Impact
General	Extract duplicated fallback logic into a closure The show-toplevel fallback logic is duplicated verbatim in both the None arm of the match and the else block. Extract it into a closure so both fallback paths stay in sync and future changes only need to be made in one place. src/tags.rs [118-138] -match repo_from_remote_url(&url) { - Some(repo) => tags.push(format!("repo:{repo}")), - None => { - if let Some(toplevel) = git_cmd(&["rev-parse", "--show-toplevel"], cwd) { - if let Some(name) = std::path::Path::new(&toplevel).file_name() { - tags.push(format!("repo:{}", name.to_string_lossy())); - } - } - } -} -if let Some(org) = org_from_remote_url(&url) { - tags.push(format!("org:{org}")); -} -} else { - // No origin remote: fall back to show-toplevel basename for repo. +let push_repo_from_toplevel = || { if let Some(toplevel) = git_cmd(&["rev-parse", "--show-toplevel"], cwd) { if let Some(name) = std::path::Path::new(&toplevel).file_name() { tags.push(format!("repo:{}", name.to_string_lossy())); } } +}; + +if let Some(url) = git_cmd(&["remote", "get-url", "origin"], cwd) { + match repo_from_remote_url(&url) { + Some(repo) => tags.push(format!("repo:{repo}")), + None => push_repo_from_toplevel(), + } + if let Some(org) = org_from_remote_url(&url) { + tags.push(format!("org:{org}")); + } +} else { + push_repo_from_toplevel(); } Suggestion importance[1-10]: 6 __ Why: The show-toplevel fallback logic is genuinely duplicated in both the None arm and the else block, and extracting it into a closure improves maintainability. The improved_code correctly reflects the change and the closure borrow semantics are valid for this context.	Low

[winjer]

Round 2 feedback — not applied: the borrow-conflict claim is contradicted by the checkable state. Commit fb42c8f compiles and CI is green on it: the Actions test job runs cargo test and cargo clippy -- -D warnings, both passed on fb42c8f. Under NLL, the closure's mutable borrow of tags is control-flow sensitive: on the Some path, no closure call occurs afterwards, so the borrow is dead by the time tags.push(...) runs directly. pr-agent's own round-1 analysis stated the closure's "borrow semantics are valid for this context".

[winjer]

Reviewer summary for the human:

Review process notes (things worth a sanity-check rather than taking on my certainty):

• The PR went through planflow batch reviews driven to zero issues (including two rounds of Minor fixes: asserting git-setup success in the new tests, then reusing the git_cmd helper), plus full local verification (219 tests passed, cargo clippy -- -D warnings clean) and green CI on every pushed head.
• pr-agent round 1 suggested extracting the duplicated show-toplevel fallback into a closure — applied in fb42c8f.
• pr-agent round 2 then claimed that closure fails to compile with a borrow conflict. Rebutted with evidence rather than fixed: fb42c8f compiles and CI (which runs cargo test + clippy -- -D warnings) is green on that exact commit, and NLL makes the borrow path-sensitive. Left unresolved-as-thread-free; if you disagree, reverting fb42c8f to the inlined blocks is safe and behaviourally identical.
• One housekeeping item was filed in planflow/housekeeping.md (rides this merge): cargo fmt --check fails repo-wide, partly pre-existing (tests/*), partly one line from this change (gather_env_tags signature width, src/tags.rs:102). CI does not enforce fmt, so this is debt, not a breakage.