Migrate checkpoints to per-commit granularity with API proxy transcript capture

[james-in-a-box]

Summary

Migrates checkpoint system from per-push to per-commit granularity and replaces Claude Code JSONL transcript extraction with Anthropic API proxy instrumentation.

Key changes:

• Per-commit checkpoints: Each commit in a push gets its own checkpoint, enabling precise traceability between code changes and agent sessions
• API proxy transcript capture: Transcripts are now extracted from API request/response pairs captured at the gateway proxy layer, providing a stable format independent of Claude Code internals
• Buffer rotation: 10MB per-session buffer with rotation to handle long sessions
• Graceful degradation: Checkpoint creation continues even if transcript extraction fails

Architecture

gateway.py (push) → checkpoint_handler → transcript_extractor → proxy buffer
                          ↓
               checkpoint branch (egg/checkpoints/v1)

The proxy buffer (/tmp/egg-transcripts/{container_id}.jsonl) captures:

• API request/response pairs (messages, tools, content)
• Token usage per request
• Tool calls and their results
• Both streaming and non-streaming responses

Files Changed

New files:

• gateway/transcript_buffer.py - TranscriptBuffer class for API traffic capture
• gateway/tests/test_transcript_buffer.py - 19 tests
• gateway/tests/test_checkpoint_handler.py - 15 tests
• tests/shared/egg_contracts/test_transcript_extractor.py - 17 tests

Modified:

• gateway/gateway.py - Instrumentation hooks for API proxy
• gateway/checkpoint_handler.py - Per-commit iteration, proxy buffer integration
• gateway/session_manager.py - Buffer cleanup on session end
• shared/egg_contracts/transcript_extractor.py - Rewritten for proxy buffer only
• shared/egg_contracts/checkpoint_cli.py - Updated help text

Issue: closes #509

Test plan

• All 944 tests pass
• Per-commit iteration tested with mocked git rev-list
• Buffer rotation tested at size limits
• Transcript extraction tested for messages, tool calls, token usage
• SSE streaming response parsing tested
• Concurrent buffer writes tested for thread safety

Authored-by: egg

[james-in-a-box]

Review: PR #517 - Migrate checkpoints to per-commit granularity with API proxy transcript capture

This is a significant architectural change with several issues that need to be addressed before merging.

---

Critical Issues

1. Race condition in buffer rotation (gateway/transcript_buffer.py:310-369)

The _rotate_buffer() method reads the entire buffer file, truncates, and rewrites it. While there's a thread lock (self._lock), the file locking in _append_entry() uses fcntl.flock() which only locks during the write, not during rotation. This creates a TOCTOU race between rotation check and file operations:

def _maybe_rotate(self) -> None:
    if not self.buffer_path.exists():  # Check
        return
    current_size = self.buffer_path.stat().st_size  # Check  
    if current_size >= self._max_size:
        self._rotate_buffer()  # Act - reads file, truncates, writes

def _append_entry(self, entry_bytes: bytes) -> None:
    with open(self.buffer_path, "ab") as f:
        fcntl.flock(f.fileno(), fcntl.LOCK_EX)  # Only locks this write
        f.write(entry_bytes)  # Can interleave with rotation

If another process writes between rotation's read and write, data will be lost. Fix: Hold the exclusive lock during the entire rotation operation, or use atomic file replacement.

2. Memory accumulation for streaming responses (gateway/gateway.py:2980-2996)

All streaming chunks are accumulated in memory before capture:

collected_chunks: list[bytes] = []

def generate() -> Any:
    try:
        for chunk in upstream.iter_bytes():
            collected_chunks.append(chunk)  # Accumulates all chunks
            yield chunk
    finally:
        _capture_streaming_response(..., chunks=collected_chunks, ...)

For long-running streaming responses with large content, this could consume significant memory. The proxy should work with a memory budget since it handles many concurrent requests. Consider capping the collected size and truncating if exceeded.

3. Missing error handling for failed API responses (gateway/gateway.py:2738-2779)

_capture_non_streaming_response() only captures when the response body is valid JSON. For 4xx/5xx errors or malformed responses, nothing is captured:

try:
    response_json = json.loads(response_body)
except (json.JSONDecodeError, TypeError):
    logger.debug("Could not parse response body for transcript capture")
    return  # Silently drops capture for all non-JSON responses

This loses visibility into failed API calls, which are often the most important for debugging. Consider capturing error responses with at least the status code and error type.

---

Correctness Issues

4. Incomplete SSE parsing (gateway/gateway.py:2833-2920)

The _parse_sse_response() function doesn't handle all Anthropic SSE event types correctly:

1. Missing input_tokens from message_start: The message_start event contains usage.input_tokens, but only message_delta.usage is captured (which only has output_tokens). This means input_tokens may be missing or zero for streaming responses.

2. Error events not handled: The error event type from the streaming API is not captured, meaning API errors during streaming are lost.

3. Partial JSON parsing failure drops entire tool_use: If the accumulated partial_json fails to parse (line 2917-2919), the input is silently set to {}, but the tool_use block is still included. This could lead to confusing transcripts where tools appear to be called with empty input.

5. Tool result matching is order-dependent (shared/egg_contracts/transcript_extractor.py:282-311)

The tool result matching in extract_tool_calls_from_proxy_buffer() assumes tool results appear in a later entry than the corresponding tool_use:

tool_use_map: dict[str, ToolCall] = {}  # Populated from response content

for entry in entries:
    # ... populate tool_use_map from response ...
    
    # Then look for results in request messages
    for msg in req_messages:
        if block.get("type") == "tool_result":
            tool_use_id = block.get("tool_use_id", "")
            if tool_use_id in tool_use_map:  # Must exist from earlier entry

If entries are processed out of order (e.g., due to concurrent writes), results won't match. The code should be resilient to entry ordering.

6. Unsafe path construction (shared/egg_contracts/transcript_extractor.py:433-435)

def get_proxy_buffer_path(container_id: str) -> Path:
    """Get the default proxy buffer path for a container ID."""
    return Path("/tmp/egg-transcripts") / f"{container_id}.jsonl"

No validation on container_id. A malicious or malformed container ID like ../../../etc/passwd could construct paths outside the intended directory. While this is defense-in-depth (container IDs come from session manager), it should still be validated.

---

Design Issues

7. Force push handling creates single checkpoint for entire history

get_commits_in_push() correctly falls back to [new_sha] for force pushes:

if result.returncode == 0 and result.stdout.strip():
    commits = result.stdout.strip().split("\n")
    return [c for c in commits if c]

# Force push falls back to new_sha only
return [new_sha]

However, on force push, git rev-list old..new returns nothing because old_sha may not be an ancestor of new_sha. This means force pushes only create a single checkpoint for the tip, regardless of how many commits are being force-pushed. This may be intentional, but should be documented.

8. Shared transcript for all commits in push

All commits in a push get the same transcript since the proxy buffer is shared:

for commit_sha in commits:
    checkpoint = handler.capture_checkpoint(...)  # Same buffer for all

This means if the session made 3 commits, all 3 checkpoints will contain the full session transcript. While this is acknowledged in the design, it's a significant storage overhead for multi-commit pushes. Consider at minimum deduplicating the transcript content.

9. No integration test for end-to-end per-commit flow

The test plan mentions an integration test test_checkpoint_e2e.py, but this file is not in the PR. The existing tests mock the internal functions but don't verify the actual git-push-to-checkpoint flow works correctly.

---

Minor Issues

10. Import inside function (gateway/gateway.py:2755, 2790)

def _capture_non_streaming_response(...):
    import time  # Repeated import

The time module is already imported at module level in gateway.py. These local imports are unnecessary.

11. Buffer directory permissions not set

TranscriptBuffer.__init__ creates the directory but doesn't ensure secure permissions:

self._buffer_dir.mkdir(parents=True, exist_ok=True)

/tmp/egg-transcripts will be world-readable by default. Since this contains API request/response data, it should have restricted permissions (e.g., mode=0o700).

12. Inconsistent container_id parameter

In extract_transcript_from_proxy_buffer(), if container_id is not provided, it's inferred from the filename:

if container_id is None:
    container_id = buffer_path.stem  # Filename without extension

But in checkpoint_handler.py, it's always passed explicitly. The fallback logic should either be removed or documented as intentional.

---

Summary

The architecture is sound and the per-commit granularity is a good improvement. However, there are several correctness and robustness issues that should be fixed:

1. Must fix: Race condition in buffer rotation (critical for data integrity)
2. Must fix: Memory accumulation for streaming (resource exhaustion risk)
3. Should fix: Missing input_tokens capture for streaming responses (affects token usage accuracy)
4. Should fix: Path validation for container_id (defense in depth)
5. Consider: Adding the mentioned integration test before merging

Request changes until at least items 1-2 are addressed.

— Authored by egg

[james-in-a-box]

Re-review: PR #517 - Address review feedback on transcript capture

This re-review examines commit f65a0cd which addresses feedback from the previous review at 0c7da209.

---

Verification of Previous Issues

✅ Issue #1 (Critical): Race condition in buffer rotation - FIXED

The fix correctly addresses the race condition:

• Rotation now holds exclusive file lock during the entire operation (gateway/transcript_buffer.py:339-377)
• Uses atomic os.replace() to swap temp file with original
• Temp file cleanup on error is properly handled

Minor note: The lock is released after os.replace() which is correct since the replacement is atomic. The file descriptor is now stale after replace, but the unlock is still valid.

✅ Issue #2 (Critical): Memory accumulation for streaming - FIXED

The fix caps collected chunks at 10MB (gateway/gateway.py:3031-3052):

• MAX_CAPTURE_SIZE = 10 * 1024 * 1024 is defined
• Size tracking with collected_size variable
• capture_truncated flag prevents further accumulation
• Appropriate debug logging when truncation occurs

✅ Issue #3: Missing error handling for failed API responses - FIXED

Non-streaming error responses are now captured (gateway/gateway.py:2760-2773):

• 4xx/5xx responses with non-JSON bodies are captured with status code and truncated message
• Structured error responses are captured as error content blocks (gateway/gateway.py:2779-2789)
• status_code parameter added to _capture_non_streaming_response()

✅ Issue #4a: Missing input_tokens from message_start - FIXED

Input tokens are now extracted from message_start event (gateway/gateway.py:2897-2908):

• input_tokens, cache_read_input_tokens, cache_creation_input_tokens captured
• Comments clarify that message_delta only contains output_tokens

✅ Issue #4b: Error events not handled in SSE parsing - FIXED

Error events from streaming API are now captured (gateway/gateway.py:2910-2918):

• error event type handled
• Error info added as content block with type "error"
• stop_reason set to "error"

✅ Issue #6: Unsafe path construction - FIXED

Path traversal validation added (shared/egg_contracts/transcript_extractor.py:447-459):

• Rejects container_ids containing /, \, .., or empty strings
• Additional resolve().relative_to() check ensures path stays within base directory
• Raises ValueError with descriptive message

✅ Minor #10: Import inside function - FIXED

import time moved to module level (gateway/gateway.py:33).

✅ Minor #11: Buffer directory permissions - FIXED

Buffer directory now created with mode=0o700 (gateway/transcript_buffer.py:98), with additional chmod call to fix existing directories.

---

New Issues in the Fix Commit

1. Partial input_parse_error handling is incomplete (Minor)

In _parse_sse_response() (gateway/gateway.py:2963-2970), when JSON parsing fails for tool_use input:

except json.JSONDecodeError:
    logger.debug(
        "Failed to parse tool_use input JSON",
        tool_id=block.get("id"),
    )
    block["input"] = {}
    block["input_parse_error"] = True

The input_parse_error flag is added but never consumed by downstream code. The transcript will contain a tool_use block with empty input and this flag, but there's no handling in extract_tool_calls_from_proxy_buffer() to surface this to the checkpoint. Consider either:

• Adding a comment explaining this flag is for future debugging
• Or including the raw partial_input in an error field for debugging

This is minor since the transcript is still captured, just with potentially confusing empty input.

2. Rotation temp file not flushed before rename (Minor)

In _rotate_buffer() (gateway/transcript_buffer.py:367-374):

temp_path = self.buffer_path.with_suffix(".jsonl.tmp")
with open(temp_path, "w") as tmp:
    for entry in entries[-keep_count:]:
        tmp.write(entry + "\n")

# Atomic replace - still holding lock on original file
os.replace(temp_path, self.buffer_path)

The temp file is not explicitly flushed (tmp.flush()) and synced (os.fsync(tmp.fileno())) before os.replace(). While Python's context manager will flush on close, the os.replace() happens after the with block closes, so this is actually fine. However, there's no fsync which means on system crash, the replacement could be incomplete on some filesystems. For a buffer in /tmp that's cleared on restart anyway, this is acceptable.

---

Remaining Unaddressed Issues from Previous Review

Issue #5: Tool result matching is order-dependent

This issue from the previous review was not addressed. In extract_tool_calls_from_proxy_buffer() (shared/egg_contracts/transcript_extractor.py:282-311), tool results are matched to tool_use blocks assuming sequential ordering of buffer entries. If entries are processed out of order, results won't match. However, since the buffer is append-only and read sequentially, this may be acceptable in practice.

Issue #7: Force push handling creates single checkpoint for entire history

This was acknowledged as possibly intentional but should be documented. Not addressed in this commit.

Issue #8: Shared transcript for all commits in push

Storage overhead for multi-commit pushes where all checkpoints get the same transcript. Not addressed in this commit (acknowledged as known limitation).

Issue #9: No integration test for end-to-end per-commit flow

Still no test_checkpoint_e2e.py in this PR.

Issue #12: Inconsistent container_id parameter fallback

The fallback logic in extract_transcript_from_proxy_buffer() where container_id can be inferred from filename stem was not addressed.

---

Summary

The critical and high-priority issues from the previous review have been properly addressed:

1. ✅ Race condition in buffer rotation (Critical #1) - Fixed with proper locking and atomic replace
2. ✅ Memory accumulation for streaming (Critical #2) - Fixed with 10MB cap
3. ✅ Missing error handling for failed API responses (Critical #3) - Fixed
4. ✅ Missing input_tokens from message_start (Correctness #4a) - Fixed
5. ✅ Error events not handled in SSE parsing (Correctness #4b) - Fixed
6. ✅ Unsafe path construction (Correctness #6) - Fixed with validation
7. ✅ Minor issues #10 and #11 - Fixed

The remaining issues (#5, #7, #8, #9, #12) are either design decisions, documentation, or test coverage - acceptable to address in follow-up work.

No blocking issues remain. The fixes are sound and address the critical concerns from the previous review.

— Authored by egg

[james-in-a-box]

Re-review: PR #517 - Address re-review feedback (commit edbcad0)

This commit addresses the outstanding items from the previous re-review. The changes are minor: documentation improvements and one debugging enhancement.

---

Verification of Previous Issues

✅ Re-review Issue #1: input_parse_error handling incomplete - ADDRESSED

The new code adds raw_partial_input field when JSON parsing fails (gateway/gateway.py:2974):

block["raw_partial_input"] = partial_input[:1000] if len(partial_input) > 1000 else partial_input

This preserves the raw input (truncated to 1KB) for debugging incomplete streaming responses. The truncation logic is correct and prevents buffer bloat.

✅ Original Issue #5: Tool result order dependency - DOCUMENTED

Added clear docstring in transcript_extractor.py:218-223 explaining that order-dependency is expected because the buffer is append-only and read sequentially.

✅ Original Issue #7: Force push checkpoint behavior - DOCUMENTED

Added note in checkpoint_handler.py:126-131 explaining that force pushes intentionally create only a single checkpoint for the tip since they represent history rewrites.

✅ Original Issue #8: Shared transcript for multi-commit pushes - DOCUMENTED

Added note in checkpoint_handler.py:743-748 explaining that transcript sharing is intentional as it captures the full session reasoning.

✅ Original Issue #12: container_id fallback from filename - DOCUMENTED

Added explanation in transcript_extractor.py:379-383 that the filename-based fallback supports standalone testing and ad-hoc extraction.

---

Import Reordering

The import statements in gateway.py (lines 55-149) were reorganized alphabetically by ruff auto-fix. This is a no-op change that improves code consistency.

---

Minor Observations (Non-blocking)

1. raw_partial_input not consumed downstream: The new raw_partial_input field is written to the buffer but not consumed by extract_tool_calls_from_proxy_buffer() or displayed anywhere. This is fine for debugging purposes - developers can inspect the raw buffer when investigating streaming issues. However, consider adding a log line when this field is present during extraction to surface the issue more visibly.

2. Truncation threshold is arbitrary: The 1000-char limit is reasonable for debugging but could be made a constant for consistency with MAX_CAPTURE_SIZE. Low priority.

---

Summary

All feedback from the previous re-review has been addressed appropriately:

• Documentation clarifies the design decisions for order-dependency, force push handling, shared transcripts, and container_id fallback
• The raw_partial_input field provides useful debugging context for streaming parse failures

No blocking issues. The PR is ready for human review and merge.

— Authored by egg

[james-in-a-box]

Re-review: PR #517 - Address minor observations (commit fb0ea31)

This commit addresses the two minor observations from the previous review.

---

Verification of Previous Observations

✅ Observation #1: Truncation threshold constant - ADDRESSED

A RAW_INPUT_TRUNCATE_SIZE = 1000 constant is now defined at gateway/gateway.py:2675 with a clear docstring explaining its purpose.

✅ Observation #2: raw_partial_input logging - ADDRESSED

Warning log added at transcript_extractor.py:250-258 that surfaces streaming parse failures during checkpoint extraction:

if block.get("input_parse_error"):
    raw_input = block.get("raw_partial_input", "")
    logger.warning(
        "Tool call has incomplete input due to streaming parse failure: "
        "tool=%s id=%s raw_input_preview=%s",
        tool_name,
        tool_use_id,
        raw_input[:100] + "..." if len(raw_input) > 100 else raw_input,
    )

The implementation correctly:

• Uses block.get("raw_partial_input", "") with a default empty string
• Truncates the preview to 100 chars for log readability
• Uses standard Python logging format strings for lazy evaluation

---

New Observations (Non-blocking)

1. No test coverage for input_parse_error path

Neither test_anthropic_proxy.py nor test_transcript_extractor.py contain tests for the input_parse_error / raw_partial_input handling path. While the logic is simple and correct, adding a test would:

• Document the expected behavior for incomplete streaming responses
• Prevent regressions if this code path is modified

This is acceptable to address in follow-up work since the code path is defensive and rarely triggered.

---

Summary

The two minor observations from the previous review have been properly addressed:

1. ✅ Constant defined for truncation threshold
2. ✅ Warning log surfaces parse failures during extraction

No blocking issues. The PR is ready for human review and merge.

— Authored by egg

[james-in-a-box]

Re-review: PR #517 - Add test coverage for input_parse_error handling (commit 8cf9961)

This commit adds test coverage for the input_parse_error and raw_partial_input handling that was implemented in previous commits to handle incomplete streaming JSON responses.

---

Verification of Previous Observation

✅ Non-blocking observation addressed: Test coverage for input_parse_error path

The new tests properly exercise the defensive code path:

TestParseSSEResponse (tests/gateway/test_anthropic_proxy.py:807-861):

• test_parse_tool_use_with_incomplete_json: Verifies that incomplete JSON sets input_parse_error=True and preserves raw_partial_input
• test_parse_tool_use_with_long_incomplete_json_truncates: Verifies that raw_partial_input is truncated to RAW_INPUT_TRUNCATE_SIZE (1000 chars)

TestExtractToolCallsFromProxyBuffer (tests/shared/egg_contracts/test_transcript_extractor.py:252-329):

• test_extract_tool_use_with_input_parse_error: Verifies extraction logs warning when input_parse_error is present
• test_extract_tool_use_with_input_parse_error_long_raw_input: Verifies warning log truncates long raw input preview to 100 chars

All tests pass and properly verify the behavior.

---

New Tests Overview

The commit adds 61 tests across two files. Beyond the input_parse_error tests, it includes comprehensive coverage for:

• SSE response parsing (_parse_sse_response)
• Streaming request detection (_is_streaming_request)
• Credential injection (_inject_anthropic_credentials)
• Header filtering (_get_forwarded_headers, _filter_response_headers)
• Tool blocking in private mode (_filter_blocked_tools)
• Transcript capture functions (_capture_non_streaming_response, _capture_streaming_response)
• Transcript extraction (metadata, messages, tool calls, token usage)

The tests use appropriate fixtures, mocking, and assertions.

---

Minor Observation (Non-blocking)

1. Test file location inconsistency

The test file tests/gateway/test_anthropic_proxy.py is placed in tests/gateway/ rather than following the pattern of gateway/tests/test_*.py used by other gateway tests like gateway/tests/test_checkpoint_handler.py and gateway/tests/test_transcript_buffer.py.

This is a minor inconsistency in test organization. Not blocking since both locations work with pytest, but worth noting for future cleanup.

---

Summary

The commit properly addresses the non-blocking observation from my previous review by adding comprehensive test coverage for the input_parse_error handling path. The tests verify:

1. ✅ Incomplete JSON sets input_parse_error flag
2. ✅ raw_partial_input is preserved (with truncation)
3. ✅ Warning is logged during extraction
4. ✅ Warning log preview is truncated for long inputs

The PR has already been approved by the repository owner. The new changes are sound and add valuable test coverage.

No blocking issues. The PR is ready for merge.

— Authored by egg

[jwbron]

@james-in-a-box the release build failed for this pr, let's open a new PR fixing the issue: https://github.com/jwbron/egg/actions/runs/21891853356/job/63199377341#step:11:17

[james-in-a-box]

Build Failure Fix

I've investigated the release build failure and created a fix.

Root Cause:
The Docker build failed at step 4/24 when installing Python 3.11 because libexpat1 package version 2.4.7-1ubuntu0.7 no longer exists on Ubuntu's mirrors (404 error). This happened because Docker cached the apt package index from an earlier layer, but Ubuntu has since updated libexpat1 to a newer version.

E: Failed to fetch http://security.ubuntu.com/ubuntu/pool/main/e/expat/libexpat1_2.4.7-1ubuntu0.7_amd64.deb  404  Not Found

Fix:
Added apt-get update before adding the deadsnakes PPA to refresh the base Ubuntu package lists. This ensures the package index is current before installing Python 3.11 and its dependencies (including the required libexpat1).

PR: #521

— Authored by egg