fix: review-1 findings — legacy-fleet compat + self-referential REAL_GIT foot-gun#2
Merged
Merged
Conversation
…GIT foot-gun Adversarial review #1 (fugu, codex backend) REJECTED the extraction with three findings; all fixed with regression coverage: 1. Heartbeat trailer drift: commit_is_empty_heartbeat only accepted the renamed Agentic-* trailer keys, so a legacy fleet's hooks (which write Agend-*) would silently stop having their heartbeats recognized by init-pile cleanup. Now tolerates BOTH trailer generations. 2. Self-referential REAL_GIT: README's original env ordering captured the shim itself into AGENTIC_GIT_REAL_GIT (command -v git after the PATH prepend), which resolve_real_git trusted verbatim -> self-exec loop killed only by the #1504 depth cap (exit 70). resolve_real_git now canonicalize-compares the env value against current_exe and falls back to the self-excluding PATH search when they match; README ordering fixed and annotated. 3. prepare-commit-msg.ps1 read only the AGENTIC_GIT_* env — no legacy fallback, breaking the zero-daemon-change adoption claim on Windows. Now mirrors the bash hook's AGEND_INSTANCE_NAME/AGEND_HOME fallbacks. Considered and rejected: dual-accepting Agentic-*/Agend-* in the bash hook's idempotence grep — hooks are installed per-home by exactly one orchestrator generation, so mixed-generation trailers in one commit do not occur in either deployment. cargo test --workspace: 85 unit + 13 phase2 (both +1 review1 regression) all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
suzuke
added a commit
that referenced
this pull request
Jul 4, 2026
* fix: make agentic-git publishable + close 3 run bugs + smoke tests
Adversarial code review (fugu) of main found 3 real defects; all fixed with
regression coverage, plus a CI publish gate and end-to-end smoke tests.
1. **agentic-git crate was not publishable** (the real cause of the failed
release run). `cli.rs`/`shim_phase1.rs` did `include_str!("../../../assets/
hooks/…")` — workspace-ROOT assets that `cargo package` does not put in the
crate tarball, so the verify build failed. Move `assets/hooks/` INTO the
crate (`crates/agentic-git/assets/`), fix the 6 include paths. `cargo
package -p agentic-git` now verifies clean.
2. **`run` could launch UNGUARDED.** `spawn_agent`'s child PATH used
`join_paths(...).unwrap_or(path_env)`; a home path containing ':' made
join fail → silent fallback to the real PATH → the agent's git bypassed the
shim entirely. Now a hard EX_CONFIG refusal.
3. **`.agend-managed` defeated skip-when-clean.** The provisioned marker is an
untracked file, so every fresh session's tree looked dirty and clean-tree
destructive ops still snapshotted. `is_clean` now treats a tree whose only
untracked entry is the marker as clean.
Also: **smoke tests** (`tests/smoke.rs`) drive the shipped binary through the
real `agentic-git run` flow (routing→trailer→snapshot→recover→deny) + pin #2/#3;
**CI `cargo package` gate** (both crates, verify build) so #1 can't regress;
**release.yml `set +e`** so `bash -e` no longer aborts the idempotent publish
on an already-uploaded crate.
cargo test --workspace: 150 green (+3 smoke); clippy clean; both crates package.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix: allow the documented snapshot restore through the shim (fugu hunt #4)
fugu verified the 3 packaging/run fixes, then found a real usability
blocker: the recovery layer's OWN documented restore —
`git checkout <snapshot-ref> -- .` — was DENIED by the shim. The
cross-branch guard reads args[1] as a branch target, so a snapshot ref
tripped 'cross-branch — cannot switch to refs/agentic-git/snapshots/...',
leaving snapshots un-restorable unless the user knew to set
AGENTIC_GIT_BYPASS=1. The recovery feature was effectively broken for a
docs-following user.
Fix: a `checkout <tree-ish> -- <pathspec>` is a working-tree path restore,
not a branch switch (the bound branch is unchanged), so the cross-branch
guard must not deny it. Recognize the `--` pathspec form and skip the
cross-branch check for checkout.
+smoke test run_session_documented_restore_works_without_bypass_smoke: full
real-session round-trip (dirty+untracked → reset --hard snapshot → delete
untracked → `git checkout <snap> -- .` through the shim, NO bypass → both
recovered).
Also harden legacy_env_adoption's bypass assertion: check the deny SIGNATURE
('cross-branch'), not the loose word 'denied' (the #2158 bypass audit dumps
process ancestry to stderr, which a parent command's text could false-trip).
cargo test --workspace: green; clippy clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(snapshot): force a self-contained identity in commit-tree (Linux CI)
The new smoke tests + Linux CI caught a real cross-platform bug invisible on
macOS: on a runner/container/fresh machine with NO git user.name/user.email
(ubuntu-latest has none; macOS derives a default), the snapshot's
`git commit-tree` — which REQUIRES an author+committer — silently failed,
so a real session's destructive op created NO snapshot (SNAPS=0, fail-open),
i.e. the recovery layer didn't work exactly where it's supposed to. And the
agent's own `git commit` failed (TRAILER=no).
Fix: the snapshot forces a fixed GIT_AUTHOR/COMMITTER_NAME+EMAIL
('agentic-git <agentic-git@localhost>') alongside the dates it already
forces, making snapshots environment-independent. The smoke session also
sets an identity so the agent's own commits succeed on bare runners.
Verified by reproducing the failure on macOS with GIT_CONFIG_GLOBAL=/dev/null
+ empty HOME: smoke now passes (previously SNAP=NONE, as on ubuntu CI).
cargo test --workspace: green; smoke green under a no-git-identity env.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
suzuke
added a commit
that referenced
this pull request
Jul 8, 2026
…embedder P1a) (#24) * feat(core): consolidate signing/verify primitives + scheme envelope (embedder P1a) P1a moves the security contract into agentic-git-core so signer and verifier share ONE source (no drift). Isolated to the agentic-git repo — the agend-terminal daemon wiring + daemon signer swap is P1b. - Move `ensure_key` (lock-free first-writer-wins `hard_link` provisioning) + its `open_new_0600` helper verbatim from the run CLI into core as the single key primitive; core gains a `getrandom` dep. The run CLI now calls `core::ensure_key` / `core::sign_binding` (its local copies removed). - Add `sign_binding` = ensure_key + sign — closes the reviewer4 hole where the low-level `sign` panicked on a fresh home. - `verify` becomes a typed `Result<(), VerifyError{MissingKey | MalformedTag | MacMismatch | UnsupportedScheme{..}}>` (was `bool`). It parses a self-describing tag ENVELOPE (`ag-hmac-sha256:v<algo>:<key-format>:<hex>`) BEFORE the MAC check, so an HMAC scheme skew is diagnosable — the shim maps `UnsupportedScheme` to a LOUD deny (`verify_sidecar`) instead of a silent "unbound". - Add `BINDING_FORMAT_VERSION` / `HMAC_ALGO_VERSION` consts and `envelope_tag` (the "can produce" capability). ORDERING INVARIANT (fleet-safety — the emphasized landmine): the DEFAULT emit stays the BARE hex tag (implicit scheme v1), byte-identical to the pre-P1a signer, so an unswapped shim and every on-disk sidecar keep verifying. The envelope is a capability core can parse + produce, NOT the default output (that flips in P2). RED-first proven: making `sign_binding` emit the envelope fails the byte-identical test against an independent python-HMAC oracle. reviewer4 acceptance conditions (RED-first): #2 a malformed `ag-hmac-sha256:` prefixed tag NEVER falls back to bare-hex legacy (extra colon / non-`v` algo / non-hex / unknown key-format -> MalformedTag/UnsupportedScheme); #3 a newer scheme or any tamper NEVER authenticates; #4 bare-hex legacy accepted (the migration window). (#1 lockfile-single-source is P1b — agend-terminal.) Regression: the run CLI's concurrent first-writer-wins is preserved — the existing session_mode race tests (driving the real `run` binary) stay green through the core reroute, plus a direct core-level N-thread race test. cargo test (workspace) 0-failed; clippy --workspace --all-targets clean; cargo package -p agentic-git-core green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com> * fix(release): bump agentic-git-core to 0.2.0 + package interdependent crates together (reviewer4 D1) P1a added new public API (`VerifyError`, `ensure_key`, `sign_binding`) + a breaking `verify` bool->Result change to agentic-git-core WITHOUT bumping its version, so the version/package contract was incoherent: `cargo package -p agentic-git` resolves `agentic-git-core = "0.1.0"` from crates.io (the OLD published API) and fails to compile the new call sites — the PR's ubuntu CI failure. A released binary depending on published core 0.1.0 could not build with the new code. - Bump the workspace (core) version 0.1.0 -> 0.2.0 — a 0.x minor bump for the new + breaking API, exactly what the workspace comment prescribes ("core bumps when its API changes"). The shim keeps its own independent 0.2.2. - Update the binary's dep requirement to `agentic-git-core { path, version = "0.2.0" }`. - CI: package the two interdependent crates in ONE `cargo package -p agentic-git-core -p agentic-git --allow-dirty` so the binary's verify build resolves core from the just-packaged sibling (build-from-workspace), NOT the older published registry version. No crates.io publish happens (the release-time publish stays manual / operator-gated). dev3 VERIFIED the crypto surface and reviewer4 confirmed no crypto fail-open — this is purely the package/version contract fix; no core logic changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com> --------- Signed-off-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com> Co-authored-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the three findings from adversarial review #1 (REJECTED):
commit_is_empty_heartbeatnow tolerates bothAgentic-*and legacyAgend-*trailer generations (main.rsheartbeat scan). Regression:empty_heartbeat_with_legacy_agend_trailers_detected_review1.REAL_GIT—resolve_real_gitcanonicalize-compares the env value againstcurrent_exeand ignores it when they match (falls back to the self-excluding PATH search). README "Trying it" ordering fixed + annotated. Regression:self_referential_real_git_env_ignored_review1(repro was exit 70 recursion-guard FATAL; now exit 0).prepare-commit-msg.ps1now falls back toAGEND_INSTANCE_NAME/AGEND_HOMElike the bash hook.Review board: t-20260704140553348828-14440-9 · re-review dispatched to fugu.
🤖 Generated with Claude Code