mirror of
https://github.com/max-sixty/worktrunk.git
synced 2026-09-14 20:00:38 +08:00
codex/remove-codex-cloud-tests-and-fix-environment
5 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Worktrunk Bot
|
06a0315efb |
chore(clawpatch): file the FailureStrategy invariant against the code that enforces it (#3823)
Nightly survey finding. The signal-handling clawpatch spec lists `src/commands/command_approval.rs` as a context file with the reason "FailureStrategy::Warn must not swallow signal-derived errors" — but that file contains no reference to `FailureStrategy`, signals, or interrupts. `grep -n "signal\|Interrupt\|FailureStrategy" src/commands/command_approval.rs` returns nothing, and `git log -S FailureStrategy -- src/commands/command_approval.rs` is empty, so the entry has been wrong since the spec was added in #2859 rather than having drifted. `FailureStrategy` is declared and consumed entirely in `src/commands/command_executor.rs`, which the spec already lists as an owned file. So this drops the misfiled context entry and folds its claim — the one detail the owned-file reason didn't carry, that the interrupt check runs *before* any `FailureStrategy` branch — into that entry, matching how CLAUDE.md → Signal Handling states it. Nothing is lost from the spec; it just points at the file that implements the invariant. I checked the rest of the specs the same way: every `path` in `entrypoints` / `ownedFiles` / `contextFiles` / `tests` across all 14 `.config/clawpatch/features/*.json` resolves, every `entrypoints[].symbol` appears in its named file, and this was the only reason string naming a symbol its file doesn't contain. No regression test — the change is metadata prose in a file no test reads, and the mismatch isn't expressible as an assertion without inventing a schema check for `reason` strings, which would be a larger and more speculative change than the fix. Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com> |
||
|
Worktrunk Bot
|
05a301250f |
chore(clawpatch): point two stale entrypoint symbols at the live code (#3765)
Nightly sweep finding: two `.config/clawpatch/features/*.json`
entrypoints name symbols that no longer exist.
**`feat_custom_template_expansion.json`.** `expand_command_template` was
deleted by #3635 ("derive the hook preview from the execution path"),
which folded the preview render into `render_template_preview` in
`src/commands/command_executor.rs` and left `expand_template` in
`src/config/expansion.rs` as the single runtime render. The feature file
kept pointing at the old name, so the one thing it exists to do — send a
reader to the code that renders a project-supplied template before the
approval gate sees it — lands nowhere.
- **Entrypoint** → `src/config/expansion.rs::expand_template`. The
module doc calls it "a single generic function" for template rendering,
and it's what the feature's own summary is about ("expansion happens
before the approval gate sees the final string").
- **`ownedFiles` reordered and re-reasoned.** `expansion.rs` moves first
and its reason now names the render functions. `command_executor.rs` is
added — that's where #3635 put `render_template_preview` and the
foreground step pipeline, so the file the code moved *into* was missing
from a file list that still described where it came from.
`hook_commands.rs` stays, with its reason updated to what it actually
holds now (`run_hook`, the `hook show --expanded` rows).
**`feat_custom_switch_resolve.json`.** Entrypoint `handle_switch` →
`handle_switch_command`. The function was renamed when #3049 moved
switch/remove orchestration out of `main.rs`; the definition today is
`pub fn handle_switch_command` at `src/commands/worktree/switch.rs`.
This second one is why the sweep's original audit reported only one
stale symbol: it grepped each `entrypoints[].symbol` as a plain
substring, and `handle_switch` matches inside `handle_switch_command`
(and `handle_switch_output`, `handle_switch_created_output`, …), so a
renamed symbol whose new name merely extends the old one reads as
present. Re-run anchored on a definition —
`\b(fn|struct|enum|trait|type|const|static|mod)\s+<symbol>\b` inside the
declared `path` — all 14 features now resolve, and no other entrypoint
has this shape.
Following the convention of the last edit to this directory (
|
||
|
Worktrunk Bot
|
3554f4979a |
fix(rebase): surface Ctrl-C during rebase as interrupt, not conflict (#3539)
Surfaced by the nightly code-quality survey; the fix layer was corrected
after review, and the review discussion then drove the `Interrupted`
rendering work (see below).
## Problem
When a `git rebase` child is killed by a signal (SIGINT/SIGTERM) but
leaves the worktree in `REBASING` state, `handle_rebase` classified the
failure by `worktree_state()` alone (now `REBASING`) and returned
`GitError::RebaseConflict`, printing conflict-resolution guidance and a
non-130 exit code instead of a clean interrupt. This violates the
signal-handling policy in `CLAUDE.md`: signal-derived child exits must
be detected via the structured signal channel **before** any other
classification.
The original attempt added an interrupt check to
`classify_rebase_failure`, but it was **inert on the real path**:
`handle_rebase` runs `repo.run_command(["rebase", …])`, which is
`Cmd::run()` capture mode. On failure that returns a `CommandError` (via
`from_failed_output`), never a `WorktrunkError::ChildProcessExited`, and
the interrupt detector only matched `ChildProcessExited`. Worse,
`from_failed_output` stored only `output.status.code()` (which is `None`
for a signal kill) and dropped `output.status.signal()` entirely, so the
signal number wasn't recoverable downstream at all.
A second problem fell out of the review discussion: every interrupt
classification funneled into `AlreadyDisplayed { exit_code: 128 + sig
}`, erasing the signal identity — so a SIGTERM-killed `wt` exited
**silently**. Because wt traps SIGINT/SIGTERM to forward them to
children, the shell never sees wt die and never prints its own
`Terminated` line, so the standard shell convention was suppressed with
nothing in its place. And an interrupt that left the worktree mid-rebase
said nothing about the `REBASING` state it left behind (git ran in
capture mode, so none of its output was ever shown).
## Fix
The capture layer preserves the signal:
- `CommandError` gains a `signal: Option<i32>` field, populated in
`from_failed_output` from `output.status.signal()` on Unix (`None`
elsewhere). A signal-killed child has no exit code, so the display
summary names the signal instead: `git rebase failed (signal 11)`.
- `ErrorExt::interrupt_signal()` (né `interrupt_exit_code()`) walks the
anyhow chain for a `CommandError` carrying SIGINT or SIGTERM, so it
works through `run_command`'s `.context("Failed to execute: git …")`
wrapper.
Only SIGINT/SIGTERM classify as interrupts in capture mode. Capture
children get no signal forwarding or escalation, so a death by any other
signal (SIGSEGV, an OOM SIGKILL) is never wt's own doing, and a silent
exit would discard the captured stderr the user never saw. Those stay on
the visible error path: mid-rebase, the `REBASING` state classifies them
as `RebaseConflict`, whose rendering carries the `--abort` hint;
otherwise `GitError::Other`. Stream mode's `ChildProcessExited` branch
counts any signal: its output already streamed to the terminal, and
user-initiated kills are normalized upstream to the originating
SIGINT/SIGTERM (the `seen_signal` branch in `shell_exec`, the concurrent
runner's originating-signal override).
Interrupt exits are now typed and announced. A new
`WorktrunkError::Interrupted { signal, hint }` replaces
`AlreadyDisplayed { exit_code: 128 + sig }` at every
interrupt-classification site (`handle_command_error`, `for_each`,
`classify_rebase_failure`, `run_custom`'s signal branch). It still exits
`128 + signal`, but renders once, at exit, per shell convention:
- **SIGINT**: silent — the terminal already echoed `^C`, and shells
print nothing for an interrupted job.
- **SIGTERM**: `✗ Terminated` — the line the shell would have printed
had wt not trapped the signal.
- **Any other signal** (reachable from stream mode): `✗ Killed by signal
N`.
`hint` carries an optional recovery line rendered under the message.
`classify_rebase_failure` uses it for the state an interrupt leaves
behind:
```
◎ Rebasing onto main...
✗ Terminated
↳ Rebase onto main left in progress; to abort, run git rebase --abort
```
(For SIGINT the hint stands alone under the user's own `^C`.)
## Reachability caveat
The common terminal Ctrl-C case is subtler than the original description
implied: on paths where wt has not installed its signal handler, a
terminal Ctrl-C is delivered to the whole foreground group and
terminates `wt` itself at 130 before classification runs. The
capture-mode fix is reachable, and correct, when the signal reaches only
the `git` child while `wt` survives (e.g. within `wt merge` after the
handler is installed, a targeted kill, or a supervisor signalling the
child), and it also closes the latent, broader gap where capture-mode
signal deaths were classified without consulting the structured signal
channel at all.
## Tests
- `capture_mode_interrupt_is_not_classified_as_conflict` feeds
`classify_rebase_failure` the real `CommandError`-with-signal shape
(wrapped in the `.context(...)` layer `run_command` adds) and asserts
`Interrupted { signal: 2, hint: Some(…) }` with the `git rebase --abort`
hint, not `RebaseConflict`.
- `capture_mode_crash_signal_stays_a_visible_error` asserts a SIGSEGV
death mid-rebase classifies as `RebaseConflict` and a SIGKILL death
outside `REBASING` as `Other`, never a silent interrupt.
- `interrupt_signal_capture_mode_narrows_to_sigint_sigterm` pins the
capture-mode signal set: 2 and 15 classify, 9 and 11 don't.
- `from_failed_output_preserves_signal_and_interrupt_signal_recovers_it`
pins the capture layer directly: a signal-killed `Output` yields
`signal: Some(2)` / `exit_code: None`, recovered through a
`.context(...)` wrapper.
- `interrupted_renders_by_signal_and_exits_128_plus_signal` pins the
exit-time rendering: silent SIGINT, `✗ Terminated` for SIGTERM, `✗
Killed by signal 9` otherwise, hint placement, and the `128 + signal`
exit codes.
- `test_interrupt_signal` covers the stream-mode channel plus
idempotence (an `Interrupted` error re-checked by an outer loop stays an
interrupt).
- Existing integration tests drive the paths end-to-end:
`test_pre_merge_pipeline_aborts_on_signal_exit` (hook pipeline),
`test_for_each_aborts_on_signal_exit` (worktree loop),
`run_custom_propagates_signal_exit_code` (custom subcommands) — all now
flowing through `Interrupted`.
---------
Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
Co-authored-by: Maximilian Roos <m@maxroos.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
||
|
Maximilian Roos
|
94dece43c7 |
chore(clawpatch): add threat-model specs for review surfaces (#2859)
Adds eight clawpatch feature specs and refines feat_custom_network_boundary to be activity-based (matching the Network Access reword in #2850). Config-only — no code change. |
||
|
Maximilian Roos
|
ac958c2326 |
chore(clawpatch): track hand-authored threat models under .config/clawpatch (#2822)
## Summary [clawpatch](https://www.npmjs.com/package/clawpatch) is the automated Codex-driven code reviewer we used for a security/data-safety review pass over worktrunk. It keyed its state to a gitignored `.clawpatch/` directory — which means the work vanishes when the worktree is removed. This relocates that state to `.config/clawpatch/`, alongside the repo's other tool configs (`insta.yaml`, `nextest.toml`, `wt.toml`, …), and tracks only the durable, reusable inputs. The auto-mapper is near-blind to worktrunk's layout (it seeds features from `src/main.rs`, `src/bin/*`, `tests/*` only — not `src/commands/`, `src/git/`, `src/config/`), so every high-value finding came from six **hand-authored** feature definitions. Each is a threat model mapping a worktrunk security/data-safety invariant to the exact files implementing it — `approval_gate`, `merge_safety`, `remove`, `config_deprecation`, `repo_cache`, `switch_resolve`. That mapping is the actual review IP and is worth keeping in-repo: model- and tool-agnostic, equally usable by a future review pass. ## What's tracked vs not Tracked (8 files): the 6 `feat_custom_*.json` threat models, the tuned `config.json`, and a `.gitignore` allowlist block. Everything else stays gitignored — derived, machine-specific, or transient: - `findings/` — regenerated by `clawpatch review`, and tied to old branch state - `project.json` — regenerated by `clawpatch init`, contains an absolute `rootPath` - the auto-mapped `feat_cli-command` feature — deterministic from `clawpatch map` - `runs/` `reports/` `locks/` `patches/` — per-run transient The `.gitignore` uses an allowlist (`ignore .config/clawpatch/*`, then re-include `config.json` and `features/feat_custom_*.json`) so a future `clawpatch map`/`review` cannot accidentally start tracking regenerated state. ## Usage note Run with `--state-dir .config/clawpatch` or `CLAWPATCH_STATE_DIR=.config/clawpatch`. The in-file `stateDir` alone is not enough: clawpatch's config discovery only honors the relocated directory when the state dir is set via flag or env. `config.json` also adds `.config/clawpatch/**` to `exclude` so clawpatch never reviews its own state. ## Test plan No code change — config and gitignore only. Verified `clawpatch status --state-dir .config/clawpatch` reads the relocated state (7 features, 31 findings) and `git check-ignore` confirms the allowlist tracks exactly `config.json` + the 6 `feat_custom_*.json`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com> |