Skip to content

fix(git): propagate exit codes in push/pull/fetch/stash/worktree - #234

Merged
pszymkowiak merged 1 commit into
rtk-ai:masterfrom
polaminggkub-debug:fix/git-exit-code-propagation
Mar 2, 2026
Merged

pszymkowiak merged 1 commit into
rtk-ai:masterfrom
polaminggkub-debug:fix/git-exit-code-propagation

Conversation

@polaminggkub-debug

Copy link
Copy Markdown
Contributor

Problem

`rtk git push`, `rtk git pull`, `rtk git fetch`, `rtk git stash` (pop/apply/drop/push/default), and `rtk git worktree` all print a `FAILED:` message on error but return exit code 0 to the caller.

This breaks CI/CD pipelines and shell scripts that rely on exit code semantics:

```bash
rtk git push || handle_error # handle_error never called
rtk git pull && deploy # deploy runs even on pull failure
```

Root cause

The failure branches in these functions print the error but fall through to `Ok(())` instead of propagating git's exit code via `std::process::exit()`.

Fix

Add `std::process::exit(output.status.code().unwrap_or(1))` in each failure branch, matching the pattern already used correctly in `run_log`, `run_diff`, `run_add`, and `run_branch`.

Per the Rust docs, `status.code()` returns `Option` — `None` when the process was killed by a signal (Unix). The `.unwrap_or(1)` fallback handles that case correctly.

Functions fixed

Function Issue
`run_push` Falls through to `Ok(())` on failure
`run_pull` Falls through to `Ok(())` on failure
`run_fetch` Explicit `return Ok(())` on failure
`run_stash` (pop/apply/drop/push) Falls through to `Ok(())` after timer
`run_stash` (default) Falls through to `Ok(())` after timer
`run_worktree` `return Ok(())` on both success and failure

Testing

All 412 existing tests pass. No new behavior on the success path.

🤖 Generated with Claude Code

run_push, run_pull, run_fetch, run_stash (pop/apply/drop/push and
default), and run_worktree all printed FAILED messages on error but
returned Ok(()) instead of propagating git's exit code.

This breaks CI/CD pipelines and shell scripts that rely on exit code
semantics (e.g. `git push || handle_error`).

Fix: call std::process::exit(output.status.code().unwrap_or(1)) in
each failure branch, matching the pattern already used in run_log,
run_diff, run_add, and run_branch.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@pszymkowiak

Copy link
Copy Markdown
Collaborator

Hi,

Thanks for the fix!

LGTM

@pszymkowiak
pszymkowiak merged commit 5cfaecc into rtk-ai:master Mar 2, 2026
2 of 3 checks passed
maxkulish added a commit to maxkulish/rtk that referenced this pull request Mar 18, 2026
…og limits

Port 5 critical bug fixes from upstream rtk-ai/rtk (v0.29.0):

**P1.1a: Exit code propagation (rtk-ai#234)**
- Add std::process::exit(code) to push/pull/fetch/stash/worktree/commit
- Fixes CI/CD pipelines that rely on exit code semantics
- Pattern: eprintln error, then exit with git's actual exit code

**P1.1b: git commit -am, --amend support (rtk-ai#327/rtk-ai#360)**
- Change Commit enum from {messages: Vec<String>} to unit variant
- Update main.rs GitCommands::Commit to use trailing_var_arg
- Update build_commit_command() to accept all args, not just -m messages
- Now supports: -a, -am, --amend, --no-edit, --allow-empty, etc.
- Add tests for --amend, -am flags

**P1.1c: git branch creation fix (rtk-ai#194)**
- Already implemented in our fork (lines 961-1019)
- Detects positional args without list flags → routes to passthrough
- Prevents `rtk git branch newbranch` from silently becoming a list

**P1.1d: git log limit injection fix (rtk-ai#461/rtk-ai#478)**
- Add parse_user_limit() to parse -N, -n N, --max-count=N/N formats
- Update has_limit_flag detection to handle all limit formats
- Fix: don't inject -10 when user provides their own limit
- Fix: when user provides --oneline without limit, use -50 not -10
- Add 7 tests for parse_user_limit() edge cases

**P1.1e: Multiple -m flags (#c18553a)**
- Already supported by new trailing_var_arg implementation
- Tests verify `git commit -m "title" -m "body"` works

All tests pass (510 passed, 2 ignored). Zero clippy warnings.

Upstream commits ported:
- 5cfaecc (exit codes)
- 409aed6 (commit flags)
- 88dc752 (branch creation - already had)
- c18553a (multiple -m - already had)
- Related: rtk-ai#461, rtk-ai#478 (log limit)
thehoff pushed a commit to thehoff/contextcrawler that referenced this pull request May 14, 2026
…-ai#234)

run_push, run_pull, run_fetch, run_stash (pop/apply/drop/push and
default), and run_worktree all printed FAILED messages on error but
returned Ok(()) instead of propagating git's exit code.

This breaks CI/CD pipelines and shell scripts that rely on exit code
semantics (e.g. `git push || handle_error`).

Fix: call std::process::exit(output.status.code().unwrap_or(1)) in
each failure branch, matching the pattern already used in run_log,
run_diff, run_add, and run_branch.

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
thehoff added a commit to thehoff/contextcrawler that referenced this pull request Jul 14, 2026
… coexist (rtk-ai#234)

P0 regression from 0.4.4 (rtk-ai#219, 65cf208). The runtime integrity gate treated
ANY non-contextcrawler PreToolUse Bash hook as tampering and refused to run,
disabling Tirith + supply-chain gating for anyone with a real multi-hook
settings.json (git-hygiene, lab-repo-guard, council-review-gate, ...).

Posture A (own-boundary): contextcrawler validates ITS registration and flags
entries that masquerade as ours; it ignores unrelated third-party hooks — that
is Claude Code's settings-trust boundary, not ours.

- inspect_registration_surface: only a contextcrawler/rtk-hook-shaped command
  that FAILS is_expected_hook_command is `unexpected` (repoint/masquerade).
  Unrelated + malformed sibling entries are ignored.
- A valid, non-masquerading registration with no baseline now RUNS instead of
  hard-bailing (bricked every upgrade + re-bricked on each hook edit). Removed
  the now-unreachable BinaryHookStatus::NoBaseline.
- registration_surface_hash covers only contextcrawler-owned entries, so a
  legitimate edit to some other tool's hook never invalidates our baseline.

Preserved: closed-argv validation, trusted-install-path for the absolute form,
masquerade/repoint detection, hash change-detection when a baseline exists.
Verified against the real settings.json (5 sibling hooks): clean auto-allow,
empty stderr. Reverses three rtk-ai#219 posture tests (-> council).

Refs: rtk-ai#234
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K3SAimSBwPUp4MkRXaYXSU
thehoff added a commit to thehoff/contextcrawler that referenced this pull request Jul 14, 2026
…#234)

Addresses council findings on the round-1 posture change:

- Bind the matcher into the identity hash (council blocker): moving our hook
  from Bash to another matcher (dropping Bash gating) with an unchanged command
  is now caught as Tampered. Sibling entries stay excluded so unrelated hook
  edits don't invalidate the baseline.
- Ownership by parsed argv[0] basename, not substring (council medium): a
  sibling like ~/.claude/hooks/smartkit.sh (contains "rtk" + path has "hook")
  is no longer mis-flagged as tampering. Genuine repoints keeping our binary
  name are still caught.
- Trust-on-first-use baseline (council blocker): a valid registration with no
  identity is RegisteredNoBaseline (non-fatal) — the runtime gate records the
  identity on first clean run, so a later swap of our own binding to any other
  valid form/matcher is then caught as Tampered. Replaces the old fatal
  NoBaseline (which bricked upgrades) without leaving the no-baseline case
  unprotected.

Rejected (Law-2): council blocker "a sibling with ; or && passes
is_expected_hook_command" — false; that validator forbids ;|&<>`$ newline and
requires exactly `<exe> hook claude` (integrity.rs is_expected_hook_command).

5 new/updated regression tests (matcher-move, smartkit FP, TOFU record+enforce,
parsed-substring). Full lib suite 3252 pass/0 fail; clippy clean.

Refs: rtk-ai#234
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K3SAimSBwPUp4MkRXaYXSU
thehoff added a commit to thehoff/contextcrawler that referenced this pull request Jul 14, 2026
…rtk-ai#234)

Second-round council raised two real correctness gaps in the round-2 hash:

- Hash only `type == "command"` owned hooks: a `command` -> `prompt` type swap
  on our entry (command string unchanged) now drops it from the identity set
  and is caught as Tampered, even when a second owned entry keeps has_expected.
- Serialise the sorted (matcher, command) tuples as JSON instead of a manual
  NUL-join: the previous encoding was forgeable via NUL/newline inside a
  matcher (two distinct registrations could collide). Structured serialization
  is unambiguous.
- Surface a stderr warning (never stdout — hook JSON protocol) when the TOFU
  baseline record fails, so degraded tamper-detection is not silent.

Rejected (Law-2, with evidence): "absolute paths outside trusted prefixes not
flagged" — already Tampered via is_expected_hook_command + trusted-path check
(test_binary_hook_foreign_absolute_path_is_tampered); "$VAR shell-expansion
bypass" — is_expected_hook_command forbids `$`, such forms are ignored and can
only ever run our genuine binary. init records the identity at install
(init.rs), so the TOFU pre-first-run window is a narrow upgrade-without-init
residual, not a fatal gap.

2 new tests (type-swap, adversarial-matcher). Full lib suite 3254 pass/0 fail;
clippy clean.

Refs: rtk-ai#234
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K3SAimSBwPUp4MkRXaYXSU
thehoff added a commit to thehoff/contextcrawler that referenced this pull request Jul 14, 2026
…self-disable)

0.4.4's rtk-ai#219 integrity gate disabled contextcrawler (Tirith + supply-chain
included) whenever any sibling PreToolUse hook coexisted. Posture A own-boundary
fix + 2 council rounds (parsed-basename ownership, matcher+type-bound identity
hash, TOFU baseline). Refs: rtk-ai#234
thehoff added a commit to thehoff/contextcrawler that referenced this pull request Jul 14, 2026
P0 hotfix: rtk-ai#234 — the 0.4.4 hook-integrity gate (rtk-ai#219) disabled ContextCrawler
(Tirith + supply-chain gating included) whenever any sibling PreToolUse hook
coexisted. Own-boundary fix, hardened over two council rounds. See CHANGELOG.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K3SAimSBwPUp4MkRXaYXSU
thehoff added a commit to thehoff/contextcrawler that referenced this pull request Jul 14, 2026
…ts` run

The security releases (0.4.3-0.4.7) verified with `cargo test --lib`, which
skips integration tests in tests/ and #[ignore] tests — so these were red
undetected. All test/infra only; no production behaviour change.

- branding_lint: add a rule for the match-arm binary-name alternation
  `Some("contextcrawler" | "rtk")`. The lint had been RED since 0.4.4 (rtk-ai#219's
  is_trusted_install_path added this form with no allowlist rule).
- integrity_cargo_bin: compute the registration identity hash the rtk-ai#234 way
  (sorted owned (matcher, command) tuples), not the old whole-surface hash, so
  the cargo-bin acceptance test matches the shipped verifier.
- read.rs / git.rs ignored tests: the debug binary is `contextcrawler`, not the
  pre-rebrand `ctxcrl` — fix the hard-coded path.
- gain sigpipe test: seed the temp DB UNDER $HOME. The tracking DB path is
  confined to $HOME (#111), so a /tmp DB was refused (exit 1) before the
  broken-pipe path ran; the broken-pipe behaviour itself was already correct.

Full `cargo test --all-targets`: 3406 pass / 0 fail; `--ignored`: 7/7.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K3SAimSBwPUp4MkRXaYXSU
thehoff added a commit to thehoff/contextcrawler that referenced this pull request Oct 12, 2026
…g criterion

The gate's correctness has been judged by "can a reviewer imagine a
bypass?" That question is unbounded, which is why rtk#2286 took four
rounds, the supply-chain cluster three, rtk-ai#234 three, and the rtk-ai#191
false-positive suppressor eight. Every round was found by council; none
by the test suite, because the tests are written from the same
enumeration as the code and inherit its blind spot.

Nothing measured whether a round helped. It turns out rtk-ai#191 worked well —
organic downgrades fell 17.0/day to 7.4/day, pipe_to_interpreter 526 to
76, private_network_access 62 to 0 — and nobody knew, because nothing
reported it.

downgrades.jsonl records (cmd, tirith_json) pairs, which are exactly the
inputs to should_downgrade_for_command. Capturing the verdict JSON makes
replay hermetic: no Tirith binary needed in CI.

Measured today: 335/713 (47.0%) of historical downgrades would be
recognised as false positives by current code.

Two guards, because either alone is gameable:

- suppression_rate_does_not_regress pins the floor. Catches suppression
  quietly breaking.
- genuinely_dangerous_commands_still_downgrade pins sentinels that must
  never be suppressed, including the rtk-ai#210 regression shape
  `printf evil | sh; python3 -m json.tool x.json`. Catches the gate
  going inert.

Verified both fire, and that each catches what the other misses:
breaking pipe_to_interpreter suppression fails only the floor test;
suppressing everything scores 100% on the floor and fails only the
sentinel test [ctxcrlRules, CTX-006].

The raw log is not committable — 2,009 records named the author's home
directory, 683 internal hostnames, 354 RFC1918 addresses, 71
credential-shaped literals that core::secret_redact::redact does not
cover (it handles PEM blocks, URL userinfo, Authorization headers, GH
tokens and secret *paths*, not inline token=<value>). The extractor
scrubs these and drops any record that still trips a leak check.

Two scrubber defects were found by reading the output rather than by the
leak checks, which is why the script tells you to:
- the private-IP check matched `10.` + digits and discarded 30 good
  records containing timing decimals like 10.209643;
- the username survived deeper in paths (`Personas/<user>/`, mangled
  scratch dirs) after the home prefix was replaced.

cargo test --all: 20 suites, 3572 passed, 0 failed, 7 ignored.

Refs: rtk-ai#243
thehoff pushed a commit to thehoff/contextcrawler that referenced this pull request Oct 12, 2026
The runtime hook gate now tolerates genuine Claude absence (settings.json
missing) without failing closed. The modern Claude PreToolUse check is
gated on host presence; stat errors still propagate as Unsafe so a
hostile directory cannot hide behind the absence fast-path.

The canonical data dir is renamed from 'ctxcrl' to 'contextcrawler' to
match the binary name and give users one folder per app. The path
migration learns the intermediate rtk→ctxcrl→contextcrawler chain:
- 'rtk' (orig)      → 'contextcrawler'
- 'ctxcrl' (prev)   → 'contextcrawler'  (intermediate state)

When both legacy dirs coexist, the intermediate ('ctxcrl') state wins
because it represents the user's intent after the rtk→ctxcrl upgrade.
The migration's first-mover wins (no overwrite), so call order is the
policy. Two regression tests pin the order: one for prefs, one for
project-local.

Project-local dirs now ignored in .gitignore alongside the existing
'.ctxcrl/' entry, so the migration's new directory doesn't pollute
git status.

Verified on the user's host:
- ~/.local/share/ctxcrl/ → ~/.local/share/contextcrawler/ (95MB history.db)
- ~/.local/share/rtk/ archived to ~/.contextcrawler-rollback/2026-08-19/
- ~/.config/{ctxcrl,rtk}/ removed; canonical /contextcrawler/ is the only dir
- 153,985 commands in the canonical DB; writes verified post-install
- New binary installed via 'cargo install --path . --force' (exit 0)

refs: rtk-ai#245
Refs: rtk-ai#234
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.

2 participants