Skip to content

CI: test the failure modes, not just the happy paths#39

Merged
leogdion merged 2 commits into
v1.0.0-integrationfrom
ci-coverage
Jul 25, 2026
Merged

CI: test the failure modes, not just the happy paths#39
leogdion merged 2 commits into
v1.0.0-integrationfrom
ci-coverage

Conversation

@leogdion

@leogdion leogdion commented Jul 25, 2026

Copy link
Copy Markdown
Member

Merge order: 5 of 6 — merge this after #35, #36 and #37.

⚠️ CI on this branch is red by design. It is cut from the same commit as
main, so until #35/#36/#37 merge into v1.0.0-integration it contains none
of the fixes it tests. 17 assertions fail, identically on
ubuntu-latest and macos-latest
, and every one of them is a bug fixed by #36
or #37: the init rollback (4), the missing-value hangs (3), the add name
validation (3), --no-push (6), and the list --json backslash (1). Nothing
else fails, on either runner. Once #35/#36/#37 land it is green — see
Verified below, where the whole suite passes against a real merge of all four
branches.

Closes #27.

Why

The old smoke block was genuinely useful — it pinned both documented regressions
and ran on Linux and macOS — but it asserted almost nothing about invalid input,
and init and root were untested entirely. None of the blockers in the
pre-1.0 review would have been caught by it.

And ShellCheck is not the safety net it looks like: it passes clean on code
containing both the argument-parsing hang and the _seed_agents precedence
bug. Linting is not coverage.

What

Extracts the inline block to tests/smoke.sh, run by CI and by contributors
with the same command. This does not violate the single-file constraint — that
rule governs git-trees, not the test harness, and AGENTS.md now says so.

The suite builds its own fixtures under mktemp -d from file:// remotes: no
network, no jq, nothing written outside the temp dir. It uses set -uo pipefail without -e, matching the tool's own style, so one run reports every
failure instead of stopping at the first.

#27 gap Now
Missing --host / --dir value exits nonzero ✅ bounded
init at all — works offline via file:// ✅ incl. failed-fetch rollback + retry
root, root --agents, _seed_agents overwrite refusal ✅ all four seed cases
list --json with a backslash in a path ✅ round-trips through json.load
Failed track/push makes add exit nonzero
add brandnew asserted loosely ✅ tightened to == origin/brandnew
--older-than abc, clean --gone default branch, clean --apply n/a — #35 removes clean

Plus add . / .. / 'has space', and --no-push / TREES_NO_PUSH from #37.

Two assertions deserve their comments:

  • The missing-value checks are bounded. The regression is an infinite loop,
    so a plain assertion would hang the runner until the job timeout rather than
    fail. timeout isn't installed on macOS, hence the background-PID + kill -0
    helper.
  • add brandnew asserts the exact upstream. The old != origin/main also
    passed on an empty or otherwise wrong upstream — the same bug in a different
    hat.

AGENTS.md § Testing claimed automated coverage that did not exist. Rewritten to
describe what the suite actually checks.

Verified

Against unfixed main — 17 failures locally, and the same 17 on both CI
runners. They are exactly the right ones, and nothing else fails, so the suite
is not flaky against the current tool.

Portability — the first CI run caught a real bug in this PR, since fixed:
in_dir is only ever passed as an argument, and ShellCheck reports its body as
unreachable under a code that differs by version (SC2329 in 0.11 via Homebrew on
macOS, SC2317 in 0.9 via apt on ubuntu). The directive covered only half the
matrix, so the ubuntu job failed at lint before running anything. Both codes are
listed now, and ubuntu executes the full suite.

Against a real merge of remove-clean + script-bugs + add-no-push +
docs-and-install
smoke tests passed, all sections green including
--no-push.

That merge also answers the conflict question: #35, #36 and #37 merge into each
other cleanly.
The only conflict in the whole set is #38 vs #35 in README.md
(the quickstart).

shellcheck -s bash git-trees install.sh tests/smoke.sh clean; env -C avoided
throughout since macOS env has no such flag.

Conflict resolution for ci.yml

#36 and #37 append assertions to the inline block that this PR deletes. Their
coverage is not lost
— it is already reproduced inside tests/smoke.sh
(the add name-validation, missing-value, JSON-escaping and --no-push
sections). Resolve by keeping tests/smoke.sh and the two-line ci.yml, and
dropping the inline block entirely.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 602710bb-55a6-4233-ad44-4dd8afc22e11

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci-coverage

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@leogdion
leogdion changed the base branch from main to v1.0.0-integration July 25, 2026 17:43
CI and others added 2 commits July 25, 2026 15:07
The old smoke block asserted almost nothing about invalid input, and `init` and
`root` were untested entirely. None of the blockers in the pre-1.0 review would
have been caught by it, and ShellCheck passes clean on code containing both the
argument-parsing hang and the `_seed_agents` precedence bug — linting is not
coverage.

Extracts the inline block to `tests/smoke.sh` so contributors run exactly what
CI runs. This does not violate the single-file constraint: that rule governs
`git-trees`, not the test harness. The suite builds its own fixtures under
`mktemp -d` from `file://` remotes — no network, no `jq` — and keeps running
after a failure so one run reports every problem.

New coverage, each assertion commented with the bug it pins:

- `init` end to end, plus rollback when the post-clone fetch fails and the
  retry that rollback makes possible
- `--host` / `--dir` with a missing value, bounded so the infinite-loop
  regression fails fast instead of hanging the runner until the job timeout
  (`timeout` is not installed on macOS, hence the background-PID dance)
- `root`, bare-container adoption, and all four `_seed_agents` cases including
  the broken symlink that `-e` misses and `-L` catches
- `add` rejecting `.`, `..`, and `'has space'` as bad branch names rather than
  directory collisions, and before `Preparing worktree` is printed
- `list --json` for a worktree path containing a backslash and a quote
- `add --no-push`, `TREES_NO_PUSH`, and that the default still pushes

`add brandnew` now asserts the upstream is exactly `origin/brandnew`. The old
`!= origin/main` form also passed on an empty or otherwise wrong upstream —
the same bug wearing a different hat.

`AGENTS.md` § Testing is rewritten: it claimed automated coverage that did not
exist, and now describes what the suite actually checks.

Closes #27.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`in_dir` is only ever passed as an argument to the assert_* helpers, so
ShellCheck reports its body as unreachable. The code differs by version — 0.11
(local, and Homebrew on the macOS runner) says SC2329, 0.9 (apt on
ubuntu-latest) says SC2317 — so the existing directive covered only half the
matrix and the ubuntu job failed at lint before it ever ran the suite.

List both codes.
@leogdion

Copy link
Copy Markdown
Member Author

Rebased onto the current v1.0.0-integration (now carrying #35#38). Conflicted in both files it touches, and the resolution was more than mechanical, so worth spelling out.

.github/workflows/ci.yml — took this branch's side (run: tests/smoke.sh ./git-trees); replacing the inline block is the point of the PR. But the inline block had grown since this branch was cut: #35 added the clean assertions and #36 added the slug ones. Taking our side wholesale would have silently deleted that coverage, so I ported every one of them into tests/smoke.sh rather than letting the merge drop them. I mapped each inline assertion to an equivalent before committing; nothing was lost, and add brandnew is stronger here (asserts the upstream is exactly origin/brandnew, where the inline version only checked != origin/main — which also passes on an empty upstream, i.e. the same bug wearing a different hat).

tests/smoke.sh — this branch predates #36, so it asserted the opposite of shipped behavior:

assert_fail "add rejects branch names containing /" bash "$T" add feature/x
assert_contains "slash rejection names the problem" "$out" "must not contain '/'"

That is now a test that git-trees has a bug. Removed, and replaced with an add — slugged directories section covering the slug, the ref keeping its slash, origin/slug/one as upstream, the multi-component case, and the collision refusal naming the owning branch. It runs in its own container so slug-one cannot be mistaken for a directory an earlier test made. I added one assertion the inline block did not have: that list and add agree about where a slash branch lives — those two disagreed before #36, with list advertising branches add refused to serve, and nothing was pinning it.

Also added the clean-is-unrecognized checks to the help/dispatch section, and a fixture branch slug/one on the origin so the "slash branch already exists on the remote" path is exercised.

AGENTS.md — the manual "things worth checking" checklist and this branch's "what the suite covers" list were both editing the same region. Kept the suite list, since it supersedes the checklist, and folded #36's slug items into it as their own bullet.

Verified locally on macOS before pushing: bash -n, shellcheck -s bash git-trees install.sh tests/smoke.sh, and the full suite green. CI now green on ubuntu and macOS.

One note for whoever merges: the comment justifying grep -qw clean claimed the usage text contains "clean/dirty". It does not — the only clean left in the script is the dirty-state label list prints. Reworded to say that.

@leogdion
leogdion merged commit c58e263 into v1.0.0-integration Jul 25, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI covers happy paths but none of the failure modes

1 participant