mirror of
https://github.com/max-sixty/worktrunk.git
synced 2026-09-14 20:00:38 +08:00
codex/remove-codex-cloud-specific-tests
6 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Maximilian Roos
|
dd453304ed |
test: fold the mock stub into the wt binary (#3712)
`cargo test --test integration` neither built nor rebuilt `mock-stub`: a
target filter deselects the dummy test that pulled it in, so a fresh
tree panicked ("mock-stub binary not found") and a warm one could run a
stale stub. The chain that compensated — the separate helper package,
its dummy `builds.rs`, the `default-members` entry, nextest's
experimental `build-bins` setup script, `workspace_bin()` — existed only
because cargo-dist ships every `[[bin]]`, and carried a TODO to collapse
once that changed. dist 0.30.2 does support per-binary exclusion now
(`[dist.binaries]`, since 0.29.0; verified with `dist plan`), but the
TODO's plan has a hole it predates: `cargo install worktrunk` installs
every feature-satisfied `[[bin]]`, and dist config doesn't govern
crates.io installs.
So the mock commands are now the `wt` binary itself. `mock_commands`
links `wt` under the mock's name (`gh`, `glab`, …), and `main()`
dispatches to the ported playback (`testing::mock_stub`) when
`WORKTRUNK_TEST_MOCK_CONFIG_DIR` is set, argv[0] is a foreign name, and
the config dir holds `<argv0>.json` for it. The existence check keeps wt
under a foreign argv[0] *without* a config being wt — the
argv0-validation security test symlinks it as `wt;touch` and must reach
wt's own rejection. The shipped binary already compiles the whole
`testing` module unconditionally, so this adds no new category of test
code to it. Windows links `wt.exe` with `hard_link` (copy fallback for
cross-drive dirs); the debug binary is ~67 MB, so per-mock copies stay
the fallback.
Every runner is now safe by construction — cargo rebuilds a package's
own binaries whenever its integration tests build, so there is no
separate artifact to go missing or stale. Deleted: the helper package,
the setup script plus `experimental = ["setup-scripts"]`, the
`default-members` trick, `workspace_bin()`, and `wt_bin()`'s dead
compile-time branch (unit-test targets get neither the runtime variable
nor the `option_env!` value, so the runtime resolution is the one
mechanism).
Validated locally: the pre-merge gate's full suite passes (4542/4542;
its doc step also caught unescaped `argv[0]` intra-doc links in the new
comments, fixed and `cargo doc -Dwarnings` re-verified). With all stub
artifacts purged from `target/`, plain `cargo test --test integration`
on mock-dependent tests builds them and passes — the command that used
to hit the trap. Two integration tests pin the dispatch's argv[0] edges
(an empty argv[0], and a non-UTF8 one), alongside the existing
`wt;touch` carve-out test.
The first commit is the investigation that preceded the fix: it verified
the `wt` binary itself was never subject to the staleness the mock-stub
was, and documented that in `tests/CLAUDE.md`; the fix then narrows that
paragraph further, since the gap it scoped no longer exists.
A two-reviewer subagent round (one prosecuting the diff against the
failure modes documented in the repo's own mock history — the #401/#407
Windows era, #547, #654, #127, #2544, #2730, #2744 — the other
adversarial) then hardened the dispatch. The reserved-name guard is
case-insensitive, matching the config probe, which goes through a
filesystem that equates `WT.json` with `wt.json` on macOS and Windows;
`command_name()` reads `args_os` — `env::args()` panics on a non-Unicode
argument, and this runs inside `main()` on every invocation (caught by
the tend review) — and returns `None` for a degenerate argv[0] instead
of panicking; the `.exe` suffix is stripped explicitly rather than via
`file_stem`, so a dotted mock name (`python3.11`) resolves identically
on every platform; and `copy_mock_binary` is now private —
`MockConfig::write` writes `<name>.json` before linking and is the only
way to create a mock, so a link cannot exist without its config, and the
dispatch's missing-config fall-through can only mean "wt under a foreign
name" (the argv0-validation tests' `wt;touch`), never a half-configured
mock that silently runs real wt with the mocked tool's arguments. The
`Option<&str>` mock helpers whose `None` arm produced exactly such
configless links lost the arm (every caller passed `Some`), and 25
redundant standalone link calls went with it.
The review also surfaced the one remaining spawn-a-stale-binary path
outside the suite: `wt-perf timeline` resolved a sibling `wt` by path,
checked only existence, and told the user to build it manually — so
`cargo run -p wt-perf -- timeline` after a `src/` edit silently measured
stale code. It now builds `wt` first and takes the artifact path from
cargo's `--message-format=json` report rather than deriving a sibling
location, so target-dir and profile overrides can't divert the build
away from where it's resolved; a release wt-perf builds and measures a
release wt. The build runs before the timeline's wall-clock measurement
starts, cargo's progress streams on stderr, and stdout keeps the
`--chrome` JSON contract.
> _This was written by Claude Code on behalf of max-sixty_
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
1eded27943 |
Simplify forge, shell integration, and removal internals (#3662)
This consolidates cross-cutting models that had accumulated parallel representations, while preserving the CLI and config interfaces. ## What changed - Forge identity now flows through one `ForgeKind`, with boundary-safe network-host classification shared by CI, remote references, and structured repository metadata. Branded SSH aliases such as `github-personal` remain outside provider dispatch and receive an actionable `forge.platform` diagnostic when forge data is requested. - Worktree removal now uses one owned target type and rechecks uncached worktree topology immediately before compare-and-swap branch deletion, retaining branches that gained a live or locked checkout. - Shell integration no longer implements the retired single-file directive writer. Stale wrappers receive repair guidance, execution fails closed, and child processes cannot inherit the retired sourceable-file capability. - Zsh and Git-version probes are shared, while dead mocks, redundant dependency edges, serializer detours, pass-through types, and duplicate tests are removed. The removal guard deliberately distinguishes live, stale-prunable, and locked registrations. The detached cleanup path performs the same record-aware check before deleting a branch ref. ## Testing `cargo run -- hook pre-merge --yes` passed after merging current `origin/main`: 4,489 tests, Clippy, formatting, lockfile checks, docs, doctests, and snapshot review. > _This was written by Claude Code on behalf of max_. |
||
|
Maximilian Roos
|
a285ed308d |
refactor(tests): bring the mock-stub env vars under the WORKTRUNK_TEST_ prefix (#3621)
`MOCK_CONFIG_DIR` and `MOCK_CALL_LOG_DIR` were the only two worktrunk-invented environment variables without the `WORKTRUNK_` prefix. That is not just a naming inconsistency: `isolate_subprocess_env` scrubs the parent environment by prefix — `GIT_*` and `WORKTRUNK_*` — so an unprefixed name is the one thing a test child inherits from whoever ran the suite. Renaming them brings them under that scrub. - `MOCK_CONFIG_DIR` → `WORKTRUNK_TEST_MOCK_CONFIG_DIR` - `MOCK_CALL_LOG_DIR` → `WORKTRUNK_TEST_MOCK_CALL_LOG_DIR` `TEST` rather than a bare `WORKTRUNK_` because both are read only by `tests/helpers/mock-stub` — they are the protocol between the harness and its helper binary, and `wt` itself never reads either one. That matches the ~15 existing `WORKTRUNK_TEST_*` knobs. ## The snapshot half Mechanical but not a substitution: the `env:` block is byte-sorted by key, so the renamed entry moves position within it and a `sed` in place would leave it where the old name sorted. All 971 affected blocks were rewritten by dropping the old line, inserting the new one, and re-sorting — the aggregate diff is exactly one removed and one added line per file: ``` 971 files changed, 971 insertions(+), 971 deletions(-) - MOCK_CONFIG_DIR: "[MOCK_CONFIG_DIR]" + WORKTRUNK_TEST_MOCK_CONFIG_DIR: "[TEST_MOCK_CONFIG]" ``` The redaction placeholder follows its neighbours' convention in `add_standard_env_redactions` (`WORKTRUNK_TEST_NU_VENDOR_AUTOLOAD_DIR` → `[TEST_NU_VENDOR_AUTOLOAD]`), so it now reads `[TEST_MOCK_CONFIG]` rather than repeating the full key. `MOCK_CALL_LOG_DIR` appears in no snapshot — its two call sites are `.output()` assertion tests — so it needs no redaction. ## Keeping it from regressing `tests/CLAUDE.md` gains the rule under "Where a new environment variable goes": name it `WORKTRUNK_TEST_*`, and the rule covers the harness↔helper-binary protocol, not just knobs `wt` itself reads. Without it the next helper-binary variable gets named `MOCK_*` again and the hermeticity hole reopens. ## Verification `cargo run -- hook pre-merge --yes` passes (exit 0) on the merged tree: 4607 tests including `--features shell-integration-tests`, `pre-commit run --all-files`, clippy, doctests. No pending snapshots. A repo-wide sweep finds no remaining unprefixed spelling. ## Merged main #3620 landed while this was in flight and regenerated several `for_each` snapshots that still carried the old key, so main is merged in here. It resolved with no conflicts, and the result is what you'd want rather than what git happened to produce: those blocks now carry #3620's new keys (`GIT_ALLOW_PROTOCOL`, `CLAUDE_CONFIG_DIR`, `WORKTRUNK_TEST_PARENT_SHELL`) *and* the renamed key, each in sorted position. The sweep and the gate above both ran after the merge. #3620 also rewrote the "Where a new environment variable goes" section this branch adds to — three layers became four. Both edits survived; the new naming paragraph follows the updated layer list. The two advisory `affected tests` checks are red for the same reason, and merging clears them: `cargo affected` errors on `git diff stdout was not valid UTF-8`, because the diff from this PR's base contains 16 binary files — the `tests/fixtures/standard/` git objects and index files that #3620 deleted. This branch's own commit contributes none. A PR based after #3620 won't see them. 🤖 Generated with [Claude Code](https://claude.com/claude-code) > _This was written by Claude Code on behalf of Maximilian Roos_ Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Maximilian Roos
|
5cbc5b4614 |
fix: argv panic, format-flag, state-write enforcement, deprecated-config preservation (#2788)
## Summary Four standalone correctness fixes, each independent of the others, each with a regression test. Originally this branch was a large approval-boundary hardening PR (the "Group B" work); since then #2806 landed on `main` and structurally closed that entire TOCTOU class, deleting the point-fix scaffolding this branch had built. The branch has been rebaselined onto current `main` and reduced to the leftover orthogonal fixes that #2806 doesn't cover. ## What's in this PR | Fix | Why | |---|---| | Non-Unicode argv panic (`src/help.rs`, `src/main.rs`, `src/invocation.rs`) | `std::env::args()` panics on non-UTF-8 argv; the help renderer, command-line logger, and the three `invocation.rs` accessors (`binary_name`, `invocation_path`, `was_invoked_with_explicit_path`) all used it. Switched to `args_os()` + `to_string_lossy()`, closing the class. Regression test in `tests/integration_tests/custom.rs`. | | Format-flag rejection on write state actions (`src/main.rs`) | `guard_format_on_write` called `clap::Error::exit()`, killing the process before the verbose diagnostic and output epilogue ran. Now returns a `Result` so the conflict error flows through normal error handling. Parameterised regression test (flag before/after, all four state kinds) in `tests/integration_tests/config_state.rs`. | | `StateWrite` compiler-enforced format-on-write guard (`src/cli/config.rs`, `src/cli/mod.rs`, `src/main.rs`) | Follows the above. The hand-placed `guard_format_on_write(...)` at 5 write-arm sites was opt-in — a future write variant could silently accept `--format`. Replaced with a `StateWrite` trait co-located with the action enums; exhaustive (no-wildcard) `write_verb()` impls make the classification a compile-time obligation. Behaviour byte-identical. | | Don't drop deprecated config when the canonical parent is scalar (`src/config/deprecation.rs`) | Three data-loss edges in `compute_migrated_content`: `commit = "x"` + `[commit-generation]` had nowhere to insert `[commit.generation]`, same shape for `switch = "x"` + `[select]`, and `args = [1, "--ok"]` silently filtered the non-string element while removing `args`. Added a `can_host_subtable` guard and an all-strings check. Regression tests for all three. | ## Why the diff is small now The big "approval-boundary hardening" Group B that gave this PR its original title was a point-fix-per-call-site approach to the post-approval config-reload TOCTOU. After surfacing the structural alternative, a separate session built it as #2806, which landed on `main` and made the point-fixes redundant (it deletes most of the scaffolding they added). Force-pushed to rebaseline rather than carry the dead weight; the previous draft's review threads are on the prior history (`0fd7d4db8`) which is no longer the branch HEAD. The "human security review of Group B" gate that kept this PR a draft is now moot — Group B's RCE-class code is on `main` via #2806's own review, not here. ## Test plan - [x] `cargo build` + `cargo clippy --bins --features shell-integration-tests` clean - [x] Survivor tests pass on the new base (argv, format-flag, StateWrite, deprecation — 119 config_state + 150 deprecation tests, plus the new regression cases) - [x] Required CI green: `test (linux/macos/windows)`; `codecov/patch` 100% > _This was written by Claude Code on behalf of Maximilian Roos_ --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Worktrunk Bot
|
be5a96270e | refactor(test): use ansi_str::AnsiStr for stripping in custom tests (#2669) | ||
|
Maximilian Roos
|
016eb030e7 |
docs(extending): rename "external subcommand" to "custom subcommand" (#2270)
## Summary Renames the git-style `wt-<name>` dispatch feature to "custom subcommand" across user-facing docs and internal code. Two motivations: - **Avoid overloading "external."** The codebase already uses "external command" for the `shell_exec` concept (subprocesses like `git` and `gh`). Using the same word for the `wt-foo` dispatch feature is ambiguous. - **Cargo uses "custom command" / "external subcommand" interchangeably.** kubectl calls theirs "plugins," gh calls theirs "extensions"; git doesn't have a settled term. "Custom subcommand" reads naturally in prose and matches cargo's user-facing phrasing. ## Changes **User-facing docs** — `docs/content/extending.md` (section heading, description, comparison table) and `docs/content/faq.md` (link + anchor). Skill references auto-sync. **Internal code** — `src/commands/external.rs` → `custom.rs`, `Commands::External` → `Commands::Custom`, `handle_external_command` → `handle_custom_command`, plus matching renames in `src/completion.rs` (inject/discover/forward functions) and corresponding tests. Also renames `tests/integration_tests/external.rs` → `custom.rs`. **Kept intact** — clap's `#[command(external_subcommand)]` attribute and `.allow_external_subcommands(true)` are clap's own vocabulary, not ours. CHANGELOG is historical and left unchanged. ## Test plan - [x] `cargo build` clean - [x] `cargo clippy --all-targets --all-features` clean - [x] `cargo test --lib --bins` — all pass - [x] `cargo test --test integration` — all pass (1476) - [x] `pre-commit run --all-files` clean > _This was written by Claude Code on behalf of Maximilian_ --------- Co-authored-by: Claude <noreply@anthropic.com> |