Repository navigation
fix(forge): classify a forge by the brand in the hostname, not by DNS label - #3673
Conversation
…el classification ## The gap Exact-label classification correctly rejects `evil-github.life-white.uk` and `dev.azure.com.attacker.example`. It also drops self-hosted instances whose hostname carries the forge name only inside a label — `gitlab-internal.company.com`, `github-enterprise.acme.com`, `mygithub.life-white.uk`. Those lose CI status in `wt list --full`, `wt switch --prs`, and `repo.provider` in `wt list --format=json`. The remedy is one line of `forge.platform`, and nothing said so. One shape did announce itself. `LegacyForgeAlias::from_url` required an SSH URL **and** a dotless host **and** a `github-`/`gitlab-`/`gitea-` prefix, so it caught the branded multi-account alias (`git@github-personal:owner/repo.git`) and nothing else. A dotted hyphenated host got silence, which reads as a bug rather than a configuration gap. ## The change Detection is now the rule that preceded the exact-label boundary: a host that names a forge anywhere and does not classify gets the `forge.platform` hint. The SSH alias falls out as one instance of that, so the prefix table, the transport check, and the dotless-host check are gone — three special cases replaced by the general one they were approximating. Classification is untouched. The hint is an opt-in the user has to take, and `LegacyForgeHost` still never feeds provider dispatch. `wt list --full`, `wt switch --prs`, and `wt config show --full` keep the one emit path they already shared. The type is renamed `LegacyForgeAlias` → `LegacyForgeHost`, and the message drops its shape-specific lead-in: ``` ▲ Remote host gitlab-internal.company.com is not auto-detected as a forge; enable CI status and wt switch --prs with forge.platform = "gitlab" @ .config/wt.toml ``` One wording covers an SSH alias and a hostname alike — both are the host of the primary remote, and both take the same remedy. ## Azure DevOps stays out The issue asks whether the widening should also cover a host containing but not ending in `dev.azure.com` / `visualstudio.com`. It shouldn't, and the asymmetry is the point. A brand name is a naming convention. Companies do run `github-enterprise.acme.com`, and no rule tells that from `evil-github.life-white.uk`, so classification declines both and this hint offers both the same opt-in. The two Azure service domains are matched by suffix, which is an ownership check. A host containing one without ending in it is outside those domains by construction, and Azure DevOps Server — the self-hosted edition — runs on corporate hostnames carrying neither string. The set the widening would add there is therefore exactly the lookalike set: it would reach no real installation while telling the owner of a lookalike to configure it as a forge. `branded_forge_name` records this where someone would otherwise go to fix the asymmetry. ## Tests `test_legacy_forge_host_covers_every_shape_the_substring_rule_matched` pins the host string the message prints and the platform inferred, for each shape — alias, hyphenated self-hosted host, embedded name, mixed case with a port, and a dotted lookalike — and asserts `ForgeKind::from_host` still rejects every one, so dispatch is provably unchanged. Its counterpart pins silence for hosts that classify, hosts naming no forge, the userinfo lookalike, the two Azure cases, and a local path. The `--prs` bail test moves to `https://gitlab-internal.company.com`, the newly covered case; the SSH alias keeps its end-to-end coverage in `switch_picker_dry_run`. The three existing inline snapshots carry the new wording. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
The classifier change reads cleanly: from_host (which drives dispatch) stays exact-label, branded_forge_name is diagnostic-only and opt-in, and the test matrix pins both the widened hint set and the unchanged dispatch boundary. No concerns there.
One stale doc, outside this diff: the unreleased ## 0.71.0 CHANGELOG entry for #3662 describes the behavior this PR replaces. Its third paragraph still reads "Only one case announces itself — a single-label SSH alias of the github-personal form … A self-hosted instance at a hyphenated hostname gets no such message; it simply stops reporting CI," and quotes the old SSH host alias github-personal is not auto-detected… wording. After this change every branded host announces itself, a hyphenated self-hosted instance does get the message, and the lead-in is now Remote host …. Since #3662 and #3673 land in the same unreleased release, that paragraph would ship describing behavior that never existed — worth updating that entry (or folding a note into it) before marking ready. Happy to push that CHANGELOG edit if you'd like.
… label Reverts the branded half of the exact-label classifier and deletes the diagnostic built to explain it. `github-enterprise.acme.com`, `mygithub.life-white.uk`, `gitlab-internal.company.com`, and the `github-personal` SSH alias resolve to their forge again, so CI status, `wt switch --prs`, and `repo.provider` work with no config. ## Why the label boundary goes It looked like an ownership check and wasn't one. An attacker controls their own DNS, so `github.attacker.example` has the exact label `github` and classified fine; `gitlab.evil.co.uk` likewise. What the rule actually excluded was the self-hoster who put the brand in a hyphenated name. It failed open for the adversary and closed for the customer. The residual case for it doesn't survive either. The hostname comes out of the user's own `.git/config`, and whoever can put a host there can put code there too — the trust decision happens at clone time, and by the time worktrunk reads the remote the user is already building from it. All the classification decides is which forge CLI (`gh`, `glab`, `tea`, `az`) runs against it. So the rule is recall-first: any host carrying `github`, `gitlab`, or `gitea` matches, first match winning. The cost is a host that merely sounds like a forge getting a forge CLI run at it, which surfaces as that CLI's error rather than as silence — the better of the two failures, and `forge.platform` overrides it. ## What stays Azure DevOps keeps suffix matching on its two service domains, and for a reason unrelated to security: those are service domains rather than a brand in the host, so every real hosted instance already matches, and the on-prem edition carries neither string. `dev.azure.com.attacker.example` and `evil-visualstudio.com` are outside the domains and carry no brand to fall back on, so they stay unclassified. Userinfo still resolves to the network host, so `https://github.com@attacker.example/…` is `attacker.example`. ## What goes `LegacyForgeAlias`, `Repository::legacy_forge_alias`, `legacy_forge_alias_diagnostic`, and its three emit sites in `wt list`, `wt switch --prs`, and `wt config show --full`. Every host the diagnostic fired on now classifies, so it could only ever return `None`. A host with no brand at all still reaches the existing generic hint, which is the right message there — there is no platform to infer. The end-to-end warning-dedup test goes with it, since no warning is raised from both the collect and `--prs` threads any more; `stash_warning_preserves_order` keeps the mechanism covered. ## Docs `## Forge platform` in `src/cli/mod.rs` described the override as being for SSH aliases and self-hosted instances, which now detect on their own. It states the rule and scopes the override to hosts carrying no brand — a Forgejo instance at `forge.example.com`. Mirrors, `dev/wt.example.toml`, and the two `config` help snapshots regenerate from it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk-bot
left a comment
There was a problem hiding this comment.
Reviewing as a draft — flagging anything that looks worth a quick fix. Mark ready for a full review.
The revert reads cleanly: from_host is now a straight substring test with Azure kept on suffix matching, LegacyForgeAlias and its three emit sites are fully removed with no dangling references, and the tests pin both the widened classification and the Azure boundary. Docs and help snapshots are regenerated, and the test suite is green. No concerns in the code.
One thing to reconcile before this lands — and it supersedes the CHANGELOG note on my previous (draft) review, which is now stale on two counts.
Since that review, v0.71.0 shipped — it's tagged at this PR's base commit (aac20ee), and the Forge hosts are classified by exact DNS label entry under ## 0.71.0 is in the released changelog. So that entry is now history describing what users actually got in 0.71.0; it shouldn't be rewritten. This PR reverts that shipped, breaking behavior (gitlab-internal.company.com, github-mirror.example.com, mygithub.life-white.uk, and the github-personal alias all classify again with no config), but adds no CHANGELOG entry. Worth a new ## 0.72.0 section documenting the reversal, so a user who read the 0.71.0 breaking-change note sees it rolled back.
My earlier suggestion to fold a note into the #3662 entry no longer applies — that was written when 0.71.0 was still unreleased and the approach was to widen the diagnostic rather than remove it.
|
Right that v0.71.0 shipped — it's tagged at Not adding a The concern behind the suggestion is right and worth carrying forward: a user who read 0.71.0's breaking-change note should see it rolled back, so the 0.72.0 entry needs to name the reversal rather than describe the new rule from scratch.
|
Release v0.72.0 — 55 commits since v0.71.0. Minor bump: `cargo semver-checks` reports 5 breaking library changes, so patch is disallowed pre-1.0. ## Headline changes - **`wt merge` / `wt step push` no longer autostash the target worktree** (#3703). Both strategies now advance the target through one `advance_target` — a compare-and-swap `update-ref`, then `read-tree -m -u` in the target worktree — so `refs/stash` is never entered and staged changes stay staged. - **Forge classification returns to brand-in-hostname** (#3673), reverting the exact-DNS-label rule 0.71.0 shipped. `github-enterprise.acme.com` and friends resolve again with no config. - **`[projects."…"]` keys match by `*` pattern and carry forge settings** (#3701), so one user-config entry covers every repository on a self-hosted host. - **A published JSON Schema for `wt list --format=json` schema 2** (#3747), plus machine-readable approval state and `branch_outcome` (#3710). Full detail in `CHANGELOG.md`. ## One fix made during the release cut The release audit surfaced a gap this release's own `advance_target` rewrite introduced, fixed here rather than deferred: **`wt merge` / `wt step push` now refuse a target worktree parked mid-operation.** The target sync is a two-tree merge, which refuses an unmerged index but *not* a stopped cherry-pick or rebase whose conflict has already been staged. A target paused between steps could therefore have the push range written into it, and the user's `--continue` would commit the synced tree as the step's result. The old fast-forward path got this check for free from `receive.denyCurrentBranch=updateInstead`, which refused any unclean target outright; both strategies now ask directly, and the refusal names the worktree holding the operation. `test_push_refuses_target_mid_operation` covers it in both shapes a stopped operation can take, and both are mutation-verified. With the gate disabled, the push succeeds and writes `feature.txt` into the mid-cherry-pick worktree. The rebase case was added in response to review feedback on this PR, and pins a second dependency. A rebase detaches HEAD, so `git worktree list --porcelain` reports the target with no branch and `worktree_for_branch` finds it only because `finalize_worktree` backfills from `rebase-merge/head-name`. That makes the rebase arm the one place this guarantee rests on a helper of ours rather than on git — the fast-forward path it replaced got the refusal from `find_shared_symref`. With the backfill disabled, `wt step push` succeeds against a worktree parked mid-rebase while the cherry-pick case still passes, so the gap was real. ## Validation - Local gate green: `cargo run -- hook pre-merge --yes` — 4570 tests, clippy, fmt, doc sync. - Cross-platform nightly green on the cut-from tip `3817df079` (run 31133551751): full nextest matrix on linux/macOS/Windows, feature-powerset, all three release triples, nix-flake, minimal-versions, unused-deps, crate-build, link-check. - Changelog verified entry-by-entry against the diffs by an independent pass; every one of the 55 commits either maps to an entry or is a documented skip. - `main` advanced during the CI wait. #3762 ships in this release and now has a changelog entry; the other two commits that landed (#3758, #3749) touch only `.github/`. - Data-loss surface reviewed by four independent finders over the cumulative diff. One further finding — a pre-0.72 `approvals.toml` key containing `*` being reinterpreted as a wildcard on upgrade — was reviewed and accepted as out of scope for this release. > _This was written by Claude Code on behalf of max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
worktrunk 0.72.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## Release Notes ### Improved - **A host carrying a forge's name anywhere resolves to that forge again**: 0.71.0 required `github`, `gitlab`, or `gitea` as a whole dot-separated label, which read as an ownership check but wasn't one — an attacker controls their own DNS — while shutting out self-hosters with hyphenated names. `github-enterprise.acme.com`, `mygithub.life-white.uk`, and the `github-personal` SSH alias classify again, so CI status, `wt switch --prs`, and `repo.provider` work with no config. ([#3673](max-sixty/worktrunk#3673)) - **One `[projects."…"]` entry can cover every repository on a host, and can set the forge**: A key containing `*` matches any run of characters, `/` included, so `[projects."git.company.example/*"]` covers a whole host; every matching entry applies, least- to most-specific. The table also gained `forge.platform` and `forge.hostname`, so a self-hosted host needs one entry here rather than a block in every repo. [Docs](https://worktrunk.dev/config/#user-project-specific-settings) ([#3701](max-sixty/worktrunk#3701), thanks @chrishas35 for the request and @witt-bit for the workspace-scoped case it partly serves) - **`wt merge` and `wt step push` leave the target worktree's uncommitted changes in place**: The autostash that held a dirty target's changes restored them as unstaged; both now advance the target with a compare-and-swap `update-ref` and `read-tree -m -u`, which leaves uncommitted work untouched. (Breaking: the fast-forward path no longer runs `git push`, so `pre-push` and the receive-side hooks no longer fire, and a failed sync errors with the ref rolled back.) ([#3703](max-sixty/worktrunk#3703), [#3684](max-sixty/worktrunk#3684), [#3693](max-sixty/worktrunk#3693), thanks @gubasso for reporting) - **Approval state and branch-removal outcomes are machine-readable**: `wt config approvals list --format=json` reports whether a non-interactive run would stop for approval. `wt remove` and `wt step prune` replace `branch_deleted` with `branch_outcome`: `deleted`, `deferred`, `not_attempted`, `retained_unmerged`, `retained_checked_out`, `retained_raced`, `retained_failed`. (Breaking.) ([#3710](max-sixty/worktrunk#3710), thanks @NathanaelRea for the requests) - **Every commit hash worktrunk prints follows `core.abbrev`**: The `wt list` table, `wt switch --prs`'s `log` tab, and `wt config state`'s CI cache table sliced to 8 characters while `--format=json` carried git's `%h`. All now ask git how wide it abbreviates in this repo. ([#3676](max-sixty/worktrunk#3676), [#3677](max-sixty/worktrunk#3677)) - **`wt list --format=json` schema 2 has a published JSON Schema**: [worktrunk.dev/schema/list-v2.json](https://worktrunk.dev/schema/list-v2.json) holds the contract, and `wt list --print-schema` prints the same document. Four fields that were bare strings are now enumerated vocabularies; the emitted JSON is unchanged. ([#3747](max-sixty/worktrunk#3747)) - **A detached worktree is named by its commit, not `-`**: The Branch cell hardcoded `-`, which reads as missing data rather than a state; it now carries the row's abbreviated HEAD, and the picker and statusline name the worktree the same way. ([#3675](max-sixty/worktrunk#3675)) ### Fixed - **Piped output is plain and no longer panics**: `wt list | head -3` exited 101 with `failed printing to stdout: Broken pipe`, and `wt list` wrote ANSI to a pipe unconditionally. Every stdout surface now exits cleanly, and the human-read ones are plain when piped unless `CLICOLOR_FORCE=1`. ([#3746](max-sixty/worktrunk#3746), [#3766](max-sixty/worktrunk#3766)) - **A CI check that hasn't finished no longer reads as passed**: Each forge's status parser missed documented values, so a GitHub PR parked on an approval gate showed green, and GitLab's `canceling` and Azure DevOps's `postponed` read as no CI at all. ([#3741](max-sixty/worktrunk#3741), [#3740](max-sixty/worktrunk#3740)) - **The shell wrappers survive an `rm` alias and a failing `--execute`**: Aliases bake into the wrapper at parse time, so `alias rm='rm -v'` reached its cleanup — noise on zsh and bash, and on nushell an abort that leaked three temp files, as a failing `--execute` body also did. ([#3714](max-sixty/worktrunk#3714), thanks @Ar4l), ([#3732](max-sixty/worktrunk#3732), [#3734](max-sixty/worktrunk#3734)) - **An alias or hook wrapping `wt switch` or `wt remove` keeps your subdirectory**: The user's position came from the `wt` process's cwd, which inside an alias body is the worktree root, so an aliased `wt remove` from `feature/apps/gateway` landed at the primary worktree's root. Fixes [#3723](max-sixty/worktrunk#3723). ([#3724](max-sixty/worktrunk#3724), thanks @vivienm for reporting) - **`wt merge` and `wt step push` refuse a target worktree parked mid-operation**: The two-tree sync refuses an unmerged index but not a stopped cherry-pick or rebase whose conflict was already staged, so the push range could land in a paused target and be committed by `--continue`. ([#3759](max-sixty/worktrunk#3759)) - **The Claude plugin's worktree-remove hook resolves against the worktree path**: The hook anchored at `CLAUDE_PROJECT_DIR`, which the `claude agents` view routinely leaves outside any repository, so `wt remove` died with `not a git repository` and the session became undeletable. Its guard now also requires a `.git` entry. ([#3754](max-sixty/worktrunk#3754), [#3767](max-sixty/worktrunk#3767), thanks @judewang for the fix and the report) - **A Gitea API error is reported as one, not as a parse failure**: `tea api` exits 0 whatever the HTTP status, so both call sites guessed from the body's shape and blamed an API change for an API error. `--include` surfaces the status instead. ([#3713](max-sixty/worktrunk#3713), [#3600](max-sixty/worktrunk#3600)) - **`--print-schema` and the doc-generation help flags name the right command**: All three found the subcommand by scanning `argv` for a `/wt` suffix, which never matches `wt.exe` under a backslash path, so on Windows they read the binary's own path as the command. ([#3762](max-sixty/worktrunk#3762)) - **A multibyte shell name no longer panics**: `extract_filename_from_path` sliced at `len() - 4` to test for `.exe` with no char-boundary check: `SHELL=/bin/日本語 wt config show` panicked, and on macOS every process name goes through it during shell detection. ([#3727](max-sixty/worktrunk#3727)) - **`wt` installed under a dotted name generates shell integration for that name**: `binary_name` used `file_stem`, which cuts at the last dot, so `wt config shell init bash` under `wt.old` emitted a wrapper for `wt`. It now strips only the executable suffix. ([#3719](max-sixty/worktrunk#3719)) - **Concurrent `wt step prune` removals no longer race the worktree registry**: `git worktree remove` reads every sibling under `.git/worktrees/`, so two overlapping removals could have one read a sibling mid-teardown. Registry-mutating removals now serialize behind a second lock. ([#3692](max-sixty/worktrunk#3692)) - **The `wt switch` first-run offer previews the legacy files it removes**: Accepting "Install shell integration?" could delete a deprecated worktrunk-managed wrapper the prompt never named. What gets removed is unchanged. ([#3656](max-sixty/worktrunk#3656)) - **`wt config create --project` writes a resolvable link**: The comment it writes into `.config/wt.toml` carried a raw Zola target, because the link-conversion regex stopped at the first `]` — here the one closing a nested code span. ([#3731](max-sixty/worktrunk#3731)) - **`wt list --branches` counts a local branch containing `/` as local**: The summary tally classified branch-only rows by `branch.contains('/')`, so a local `feature/login` counted under "N remote branches". ([#3687](max-sixty/worktrunk#3687)) - **`wt step relocate`'s human summary counts template-error branches as skipped**: `--format=json` already folded them into `skipped`; the human tally undercounted by the number of branches whose `worktree-path` template failed to expand. ([#3688](max-sixty/worktrunk#3688)) ### Documentation - **SignPath attribution appears with the artifacts it describes**: The code-signing notice and a route to the policy now sit in the install section's Windows block on the README and the docs landing page, as SignPath Foundation's OSS program requires. ([#3709](max-sixty/worktrunk#3709)) - **`wt step copy-ignored`'s `--require-include` example renders as a terminal block**: It was the only `console` block in the command's long help missing the `$ ` prefix. ([#3706](max-sixty/worktrunk#3706)) ### Internal - **Library API rework** (Breaking library API): `cargo-semver-checks` fails five lints — `LegacyForgeAlias` and `Repository::legacy_forge_alias` removed with the forge-classification revert, `Repository::forge_platform_override` removed for one shared resolver, `stage_worktree_removal` gained two parameters, `UserProjectOverrides` gained a `forge` field, and the `real-repo-benches` feature was removed. ([#3673](max-sixty/worktrunk#3673), [#3694](htt See merge request: Harmonybrew/homebrew-core!16275
Reverts the branded half of the exact-label classifier and deletes the
diagnostic built to explain it.
github-enterprise.acme.com,mygithub.life-white.uk,gitlab-internal.company.com, and thegithub-personalSSH alias resolve totheir forge again, so CI status,
wt switch --prs, andrepo.providerworkwith no config.
Why the label boundary goes
It looked like an ownership check and wasn't one. An attacker controls their own
DNS, so
github.attacker.examplehas the exact labelgithuband classifiedfine;
gitlab.evil.co.uklikewise. What the rule actually excluded was theself-hoster who put the brand in a hyphenated name. It failed open for the
adversary and closed for the customer.
The residual case for it doesn't survive either. The hostname comes out of the
user's own
.git/config, and whoever can put a host there can put code theretoo — the trust decision happens at clone time, and by the time worktrunk reads
the remote the user is already building from it. All the classification decides
is which forge CLI (
gh,glab,tea,az) runs against it.So the rule is recall-first: any host carrying
github,gitlab, orgiteamatches, first match winning. The cost is a host that merely sounds like a forge
getting a forge CLI run at it, which surfaces as that CLI's error rather than as
silence — the better of the two failures, and
forge.platformoverrides it.What stays
Azure DevOps keeps suffix matching on its two service domains, and for a reason
unrelated to security: those are service domains rather than a brand in the
host, so every real hosted instance already matches, and the on-prem edition
carries neither string.
dev.azure.com.attacker.exampleandevil-visualstudio.comare outside the domains and carry no brand to fall backon, so they stay unclassified. Userinfo still resolves to the network host, so
https://github.com@attacker.example/…isattacker.example.What goes
LegacyForgeAlias,Repository::legacy_forge_alias,legacy_forge_alias_diagnostic, and its three emit sites inwt list,wt switch --prs, andwt config show --full. Every host the diagnostic firedon now classifies, so it could only ever return
None. A host with no brand atall still reaches the existing generic hint, which is the right message there —
there is no platform to infer.
The end-to-end warning-dedup test goes with it, since no warning is raised from
both the collect and
--prsthreads any more;stash_warning_preserves_orderkeeps the mechanism covered.
Docs
## Forge platforminsrc/cli/mod.rsdescribed the override as being for SSHaliases and self-hosted instances, which now detect on their own. It states the
rule and scopes the override to hosts carrying no brand — a Forgejo instance at
forge.example.com. Mirrors,dev/wt.example.toml, and the twoconfighelpsnapshots regenerate from it.
The branch's history has a false start — the first commit widened the diagnostic, the second deletes it in favour of relaxing classification — plus a merge of
mainafter v0.71.0 shipped. The net diff is the second approach; it all squashes on merge.Also supersedes #3672, the triage bot's PR for the same issue — it widens the diagnostic rather than removing the need for one, so it should be closed too.
Closes #3671