Skip to content

fix(observability): meter checkpoint git commands so their dashboards stop reading empty - #129

Merged
yordis merged 1 commit into
mainfrom
yordis/fix-meter-checkpoint-git-commands
Oct 21, 2026
Merged

yordis merged 1 commit into
mainfrom
yordis/fix-meter-checkpoint-git-commands

Conversation

@yordis

@yordis yordis commented Oct 21, 2026 •

Copy link
Copy Markdown
Member
  • Checkpoint capture, restore and diff run git through a path that never recorded the git command metrics, so the checkpoint git operations and failures panels had nothing to show even while checkpoints ran constantly.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

… stop reading empty

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@cursor

cursor Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
Change is instrumentation-only around existing git execution; behavior should be unchanged aside from metric recording.

Overview
Routes every Git VCS process invocation through shared observability so checkpoint and other git work shows up on command count, failure, and duration dashboards.

GitVcsDriver adds a runGitProcess helper that wraps VcsProcess.run with withMetrics (gitCommandsTotal, gitCommandDuration, labeled by operation). The main gitCommand path uses it, and checkpoint capture’s streamed ls-files call is switched from a bare vcsProcess.run so those subprocesses are metered too.

New effect tests assert per-operation success/failure counts (including allowed non-zero exits), duration samples, and that a full checkpoint capture increments metrics once per git process, including streamed stdout handling.

Reviewed by Cursor Bugbot for commit dbe982d. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions github-actions Bot added size:S vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Oct 21, 2026
@coderabbitai

coderabbitai Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

📝 Walkthrough

The Git VCS driver now records command counts and durations by operation for shared Git commands and checkpoint index inspection. Tests cover successful commands, allowed non-zero exits, process failures, and streamed processes.

Changes: Git command metrics

Layer / File(s) Summary
Instrument Git process execution
apps/server/src/vcs/GitVcsDriver.ts, apps/server/src/vcs/GitVcsDriver.test.ts
A shared process runner records command counts and durations by operation. The shared command path and checkpoint index inspection use the runner. Tests check success, failure, allowed non-zero exits, and streamed processes.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: 🔵 Low · up to dbe98

Checkpoint diff failures may not appear in Git command failure counts. Clarifying the intended metric boundary is advisable, but the evidence does not establish a merge blocker.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description states the problem but omits the required Problem, Change, Scope and approval, and Verification sections. It does not explain the implementation, approval or issue context, or test res… Rewrite the description using all required headings. Add the expected behavior and reproduction context under Problem; explain the metric instrumentation under Change; provide the linked issue or approval rationale under Scope and approval;…
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly identifies the main change: adding Git command metrics for checkpoint operations. It is specific and uses a conventional commit format.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description states the problem but omits the required Problem, Change, Scope and approval, and Verification sections. It does not explain the implementation, approval or issue context, or test results.

Resolution

Rewrite the description using all required headings. Add the expected behavior and reproduction context under Problem; explain the metric instrumentation under Change; provide the linked issue or approval rationale under Scope and approval; and list focused tests, observed results, and any checks not run under Verification.

✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔇 Additional comments: 1
apps/server/src/vcs/GitVcsDriver.ts-512-516: 1

512-516: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Count rejected non-zero Git exits as failures.

withMetrics labels the exit of process.run, not the final Git operation. In diffCheckpoints, allowNonZeroExit: true lets a non-zero exit reach this wrapper as a success. The caller then raises VcsProcessExitError. The failure counter therefore misses that checkpoint diff failure. Preserve expected non-zero results, but classify exits that callers reject as failures in the command metric. (effect.website)


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: c7ad88f2-43cb-43c0-839d-211311e5cb93
📥 Commits

Reviewing files that changed from the base of the PR and between 89c063a and dbe982d.

📒 Files selected for processing: 2
  • apps/server/src/vcs/GitVcsDriver.test.ts
  • apps/server/src/vcs/GitVcsDriver.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

@github-actions

Copy link
Copy Markdown

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider Metric Main baseline This PR Impact PR ceiling
Codex Total thread wire 5.0 KiB 5.0 KiB 0 B (0.0%) 6.8 KiB ✅
Codex Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Codex Live turn WebSocket wire 1.2 KiB 1.2 KiB 0 B (0.0%) 2.0 KiB ✅
Codex Live turn WebSocket decoded 20.9 KiB 20.9 KiB 0 B (0.0%) 29.3 KiB ✅
Codex Live turn messages 2 2 0 (0.0%) 8 ✅
Claude Total thread wire 5.0 KiB 5.0 KiB −24 B (−0.5%) 6.8 KiB ✅
Claude Thread snapshot wire 3.8 KiB 3.8 KiB 0 B (0.0%) 4.9 KiB ✅
Claude Live turn WebSocket wire 1.2 KiB 1.2 KiB −24 B (−2.0%) 2.0 KiB ✅
Claude Live turn WebSocket decoded 21.2 KiB 21.2 KiB −41 B (−0.2%) 29.3 KiB ✅
Claude Live turn messages 2 1 −1 (−50.0%) 8 ✅

Baseline: 89c063a · PR result: dbe982d · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

  • Codex decoded thread snapshot: 108.5 KiB
  • Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

@yordis
yordis merged commit 3d23485 into main Oct 21, 2026
28 of 29 checks passed
@yordis
yordis deleted the yordis/fix-meter-checkpoint-git-commands branch October 21, 2026 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant