feat: keys rotate, status Codex/Gemini checks, remote developer health (#20 #21 #22)#26
Conversation
AgentDiff ReportSummary
Review Context
Files To Review First
Trace details
|
Greptile SummaryThis PR replaces the old
Confidence Score: 4/5Safe to merge after addressing the non-atomic write in the re-sign path. The key archiving and verify fallback logic are sound. The one concrete defect is in resign_last_local_traces: it uses std::fs::write directly on the live trace file, which truncates before writing — a disk-full or I/O error mid-write silently destroys all trace lines, including those not being re-signed. An atomic write (write to a .tmp sibling, then rename) would eliminate this risk. src/commands/keys.rs — the resign_last_local_traces write path. Important Files Changed
Reviews (2): Last reviewed commit: "fix: address Greptile review bugs in arc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- keys.rs: remove redundant pub_path.exists() guard after private key rename — public key existence is already ensured before the rename, so re-checking creates a silent partial-archive path that leaves old traces unverifiable after rotation - status.rs: fix misleading ok() prefix in Gemini (true,true) arm when tools.enableHooks is not set; consolidate into a single warn() line Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
codeprakhar25
changed the title
| let raw = std::fs::read_to_string(&path) | ||
| .with_context(|| format!("reading {}", path.display()))?; | ||
| let mut lines: Vec<String> = raw.lines().map(String::from).collect(); | ||
| while lines.last().map(|l| l.trim().is_empty()).unwrap_or(false) { | ||
| lines.pop(); | ||
| } | ||
| anyhow::ensure!(!lines.is_empty(), "local trace buffer is empty"); | ||
|
|
||
| let take = n.min(lines.len()); | ||
| let start = lines.len() - take; | ||
| for i in start..lines.len() { | ||
| let mut val: serde_json::Value = serde_json::from_str(&lines[i]) | ||
| .with_context(|| format!("parsing trace line {}", i + 1))?; | ||
| if let Some(obj) = val.as_object_mut() { | ||
| obj.remove("sig"); | ||
| } | ||
| let sig = keys::sign_record(&val)?; | ||
| val.as_object_mut() | ||
| .context("trace entry must be a JSON object")? | ||
| .insert("sig".to_string(), serde_json::to_value(&sig)?); | ||
| lines[i] = serde_json::to_string(&val)?; | ||
| } | ||
|
|
||
| std::fs::write(&path, lines.join("\n") + "\n") | ||
| .with_context(|| format!("writing {}", path.display()))?; |
There was a problem hiding this comment.
std::fs::write opens the file with O_TRUNC, zeroing its contents before writing the new data. If the write is interrupted (e.g. disk-full mid-write, process killed, I/O error), the file is left truncated and partially written — all original trace lines, including the ones not being re-signed, are lost. Because this is an audit-trail store, a silently half-written file is worse than a failed write.
The fix is to write to a .tmp sibling and atomically replace the original via std::fs::rename.
1 participant
What does this PR do?
Closes #20,
Closes #21,
Closes #22.
Issue #20 —
agentdiff keys rotatekeys::archive_current_keypair: moves the active keypair into a timestamped folder under~/.agentdiff/keys/archive/with anarchive.tomlmetadata file (key_id, archived_at, optional expires_at)keys::try_load_archived_verifying_key: letsagentdiff verifyfall back to the local archive when a key ID is absent from the git registry — ensures traces signed by rotated keys remain verifiable offlinekeys rotate --resign-last N: re-signs the last N entries of the current branch's local trace buffer with the newly generated keyIssue #21 —
agentdiff statusCodex/Gemini enablement flags~/.codex/config.toml, additionally checksfeatures.codex_hooks = truevia TOML parse — warns with re-run hint if missingcapture-antigravityis registered insettings.json, additionally checkstools.enableHooks = truevia JSON parse — warns if the flag is absent (without it, Gemini ignores hooks even when entries are present)Issue #22 — Per-developer health in
agentdiff status --remoteREMOTE TRACE BRANCHEStable: one row perrefs/agentdiff/traces/*ref with trace count, age, and stale (>7d) warningDEVELOPERStable: groups traces byAgentdiffMetadata.author, shows trace count and last-active age; stale developers highlighted in yellow--since DURATIONflag (e.g.7,7d,48h) filters both tables to the active windowType of change
Greptile bugs addressed
archive_current_keypair(src/keys.rs): removed redundantif pub_path.exists()re-check after the private key has already been renamed — the public key existence is guaranteed by theanyhow::ensure!above, so the re-check created a silent partial-archive path that would leave old-key traces unverifiable after rotationstatus --remoteGemini arm (src/commands/status.rs): the(true, true)else branch was printing anok()-prefixed line before the actualwarn()— consolidated into a singlewarn()line so the output is not misleadingTesting
cargo checkpasses (no errors)agentdiff statusruns correctly (verified via post-commit hook output)Checklist
tomlandserde_jsonwere already in Cargo.toml)