Skip to content

feat: add bounded call history recorder - #220

Draft
Justinabox wants to merge 1 commit into
mainfrom
agent/call-history-recorder-20260706
Draft

feat: add bounded call history recorder#220
Justinabox wants to merge 1 commit into
mainfrom
agent/call-history-recorder-20260706

Conversation

@Justinabox

Copy link
Copy Markdown
Owner

Summary

  • Adds callstack.voice.history with a frozen CallRecord, process-local CallHistoryRecorder, and JSON-ready render_call_record() helper.
  • Enforces bounded retained history while preserving active calls until they can be finalized.
  • Adds regression coverage for lifecycle finalization, duplicate IDs, terminal status validation, timezone-aware timestamps, redacted rendering, and repr privacy.

Linked issue: #219

Tests

  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/test_call_history.py -q
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/test_call_history.py tests/test_call_service.py tests/test_api_auth.py -q
  • git diff --check
  • git diff --cached --check
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q

Risks / notes

  • This is a narrow foundation slice for Add PII-safe call history records for PBX workflows #219 only. It does not yet wire the recorder into CallService lifecycle hooks or expose GET /calls/history.
  • Active calls are not evicted by max_records; if active calls outnumber the limit, retained in-memory records can temporarily exceed the configured bound until calls finalize.

@Justinabox

Copy link
Copy Markdown
Owner Author

Review-manager classification: needs-fix before landing.

What passed locally on the PR head:

  • Diff inspected: callstack/voice/history.py, tests/test_call_history.py.
  • git diff --check
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/test_call_history.py -q9 passed
  • PYTHONPATH=. uv run --no-project --with pytest --with pytest-asyncio --with pytest-aiohttp --with pyserial-asyncio --with aiosqlite pytest tests/ -q829 passed
  • GitHub status checks: none reported.

Blockers to resolve or explicitly narrow before this can be marked safe-to-land:

  1. render_call_record() is documented as public/PII-safe by default, but termination_reason and voicemail_id are arbitrary strings and are emitted unchanged. Please either sanitize/redact phone-like/private identifiers in those rendered fields or constrain them to safe enum/opaque-safe values, with regression tests proving full phone-like values do not appear in the rendered dict/string.
  2. CallDirection is only a type hint; start_call(direction=...) currently accepts and renders arbitrary strings. Please add runtime validation for the public shape, with tests for invalid directions failing closed before mutating history.
  3. The current active-call bound behavior is acceptable only if intentional: active calls can temporarily make recent() exceed max_records because active records are not evicted. Please document that contract in the helper docstring and keep/extend the existing test that active calls remain finalizable under the bound.

No product/security human decision is needed yet; this looks like a narrow helper slice, but the public-renderer privacy contract should be tightened before merge.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-human.

This is a clean, well-scoped foundation, but it establishes a new call-history data/retention contract and privacy-facing representation without wiring it into a user-facing lifecycle yet. Retention semantics and the intended public integration should be explicitly confirmed before autonomous landing. No behavior was approved or merged in this pass.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release-captain review: needs-fix before landing.

This helper-only foundation is not yet the scoped #219 feature: no production call lifecycle or authenticated history API uses the recorder. In addition, active calls can make the purportedly bounded recorder grow without limit, and render_call_record() returns arbitrary termination_reason / voicemail_id strings without privacy sanitization. Runtime direction values are also not validated. Please either narrow/reframe the issue and PR explicitly to a safe internal helper, or complete the intended lifecycle/API integration with strict bound, privacy, and runtime-validation regressions. Re-run the required full suite on the rebased head.

No human product decision is required for these correctness/privacy fixes.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix; not ready to land.

Two independent reviews found that the proposed CallHistoryRecorder does not meet its bounded/public-safe contract:

  • with max_records=1, arbitrarily many active start_call() records remain retained, so long-running processes can grow without bound;
  • render_call_record() forwards untrusted termination_reason and voicemail_id verbatim, and its redact=False path exposes raw identifiers despite the public-safe framing.

Please use fresh RED tests to define a hard active-record bound (or explicit rejection/finalization policy) and an allowlisted/redacted public rendering contract for all free-text metadata. Do not silently patch production code first. After the focused and full gates pass on a rebase of current main, request a fresh privacy/quality review.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix.

The helper is not bounded under active-call load (max_records=1 can retain multiple active records), and the default renderer can expose arbitrary termination_reason/voicemail_id metadata even with redaction enabled. Runtime direction and timestamp chronology inputs also need validation before mutation.

Required TDD follow-up: define/enforce a strict active-record bound; make the default renderer safe for every rendered field; validate direction/type and lifecycle chronology before state changes. This PR is only a helper and is not yet wired into call lifecycle/API paths, so keep feature claims scoped accordingly. Rebase on current main and rerun required gates after the regressions are green.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix. The proposed recorder can exceed max_records without bound when calls remain active, accepts invalid runtime direction values despite its public typed shape, and its claimed public-safe renderer forwards arbitrary termination_reason/voicemail_id text. Please define and enforce a strict active-record capacity policy, validate public enum-like fields before mutation, and constrain/redact opaque termination and voicemail values with regressions. This remains a helper-only foundation until lifecycle/API wiring ships. Rebase on current main and rerun the required full gate.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review classification: needs-human. This introduces or documents a product-level capability/data-retention/deployment decision beyond a narrow behavior-preserving fix. It will not be auto-merged in this lane. Please retain the branch for Korgo’s product/operational decision, then request a focused follow-up review against current main.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release-lane classification: needs fix/revalidation before merge.

This draft was not independently reviewed or integration-tested in this pass and its GitHub mergeability is stale/unknown after current-main advances. Please rebase or otherwise validate it against current main, run the required full local gate, and request a fresh release review with exact head SHA and gate evidence. Do not treat absence of hosted checks as approval.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release classification: needs-human.

This is a material product/API/data-lifecycle feature rather than a narrow corrective maintenance patch. It needs Korgo’s decision on product scope, compatibility, and operational/privacy semantics before autonomous landing. Keep it draft; after a decision, rebase/repackage over current main and run the prescribed gates.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release review: needs-fix. render_call_record(..., redact=True) claims public-safe output but delegates to redact_phone_number(), which preserves nonempty no-digit values. Modem-controlled alphanumeric caller/dialed IDs could be rendered verbatim. Add a fresh failing no-digit-sentinel rendering test, make rendering fail closed, rebase on current origin/main, and rerun required gates.

@Justinabox

Copy link
Copy Markdown
Owner Author

Release classification: needs-human decision.

This is a new externally visible product capability (call-history foundation/replay semantics). The scheduled release lane requires product confirmation of retention, privacy, exposure, and integration priority before landing.

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.

1 participant