noteflow/docs/spec.md

NoteFlow Spec Validation (2025-12-31)

This document validates the previous spec review against the current repository state and adds concrete evidence with file locations and excerpts. Locations are given as path:line within this repo.

Corrections vs prior spec (validated)

• Background tasks: diarization jobs are already tracked and cancelled on shutdown; the untracked task issue is specific to integration sync tasks.
• Webhook executor already uses per-request timeouts and truncates response bodies; gaps are delivery-id tracking and connection limits, not retry logic itself.
• Outlook adapter error handling is synchronous-safe with response.text, but lacks explicit timeouts, pagination, and bounded error body logging.

---

High-impact findings (confirmed/updated)

1) Timestamp representations are inconsistent across the gRPC schema

Status: Confirmed.

Evidence:

• src/noteflow/grpc/proto/noteflow.proto:217

// Creation timestamp (Unix epoch seconds)
double created_at = 4;

• src/noteflow/grpc/proto/noteflow.proto:745

// Start time (Unix timestamp seconds)
int64 start_time = 3;

• src/noteflow/grpc/proto/noteflow.proto:1203

// Start timestamp (ISO 8601)
string started_at = 7;

• src/noteflow/grpc/proto/noteflow.proto:149

// Server-side processing timestamp
double server_timestamp = 5;

Why it matters:

• Multiple time encodings (double seconds, int64 seconds, ISO strings) force per-field conversions and increase client/server mismatch risk.

Recommendations:

• Standardize absolute time to google.protobuf.Timestamp and durations to google.protobuf.Duration in new fields or v2 messages.
• Keep legacy fields for backward compatibility and deprecate them in comments.
• Provide helper conversions in src/noteflow/grpc/_mixins/converters.py to reduce repeated ad-hoc conversions.

---

2) UpdateAnnotation uses sentinel defaults with no presence tracking

Status: Confirmed.

Evidence:

• src/noteflow/grpc/proto/noteflow.proto:502

message UpdateAnnotationRequest {
  // Updated type (optional, keeps existing if not set)
  AnnotationType annotation_type = 2;

  // Updated text (optional, keeps existing if empty)
  string text = 3;

  // Updated start time (optional, keeps existing if 0)
  double start_time = 4;

  // Updated end time (optional, keeps existing if 0)
  double end_time = 5;

  // Updated segment IDs (replaces existing)
  repeated int32 segment_ids = 6;
}

• src/noteflow/grpc/_mixins/annotation.py:127

# Update fields if provided
if request.annotation_type != noteflow_pb2.ANNOTATION_TYPE_UNSPECIFIED:
    annotation.annotation_type = proto_to_annotation_type(request.annotation_type)
if request.text:
    annotation.text = request.text
if request.start_time > 0:
    annotation.start_time = request.start_time
if request.end_time > 0:
    annotation.end_time = request.end_time
if request.segment_ids:
    annotation.segment_ids = list(request.segment_ids)

• Contrast: presence-aware optional fields already exist elsewhere: src/noteflow/grpc/proto/noteflow.proto:973

message UpdateWebhookRequest {
  // Updated URL (optional)
  optional string url = 2;
  // Updated name (optional)
  optional string name = 4;
  // Updated enabled status (optional)
  optional bool enabled = 6;
}

Why it matters:

• You cannot clear text to an empty string or set a time to 0 intentionally.
• segment_ids cannot be cleared because an empty list is treated as "no update".

Recommendations:

• Introduce a patch-style request with google.protobuf.FieldMask (or optional fields) and keep the legacy fields for backward compatibility.
• If you keep legacy fields, add explicit clear_* flags for fields that need clearing.

---

3) TranscriptUpdate payload is ambiguous without oneof

Status: Confirmed.

Evidence:

• src/noteflow/grpc/proto/noteflow.proto:136

message TranscriptUpdate {
  string meeting_id = 1;
  UpdateType update_type = 2;
  string partial_text = 3;
  FinalSegment segment = 4;
  double server_timestamp = 5;
}

Why it matters:

• The schema allows both partial_text and segment or neither, even when update_type implies one payload. Clients must defensively branch.

Recommendations:

• Add a new TranscriptUpdateV2 with oneof payload { PartialTranscript partial = 4; FinalSegment segment = 5; } and a new RPC (e.g., StreamTranscriptionV2) to avoid breaking existing clients.
• Prefer google.protobuf.Timestamp for server_timestamp in the v2 message.

---

4) Background task tracking is inconsistent

Status: Partially confirmed.

Evidence (tracked + cancelled diarization tasks):

• src/noteflow/grpc/_mixins/diarization/_jobs.py:130

# Create background task and store reference for potential cancellation
task = asyncio.create_task(self._run_diarization_job(job_id, num_speakers))
self._diarization_tasks[job_id] = task

• src/noteflow/grpc/service.py:445

for job_id, task in list(self._diarization_tasks.items()):
    if not task.done():
        task.cancel()
        with contextlib.suppress(asyncio.CancelledError):
            await task

Evidence (untracked sync tasks):

• src/noteflow/grpc/_mixins/sync.py:109

sync_task = asyncio.create_task(
    self._perform_sync(integration_id, sync_run.id, str(provider)),
    name=f"sync-{sync_run.id}",
)
# Add callback to clean up on completion
sync_task.add_done_callback(lambda _: None)

Why it matters:

• Sync tasks are not stored for cancellation on shutdown and exceptions are not centrally observed (even if _perform_sync handles most errors).

Recommendations:

• Add a shared background-task registry (or a TaskGroup) in the servicer and register sync tasks so they can be cancelled on shutdown.
• Use a done-callback that logs uncaught exceptions and removes the task from the registry.

---

5) Segmenter leading buffer uses O(n) pop(0) in a hot path

Status: Confirmed.

Evidence:

• src/noteflow/infrastructure/asr/segmenter.py:233

while total_duration > self.config.leading_buffer and self._leading_buffer:
    removed = self._leading_buffer.pop(0)
    self._leading_buffer_samples -= len(removed)

Why it matters:

• pop(0) shifts the list each time, causing O(n) behavior under sustained audio streaming.

Recommendations:

• Replace the list with collections.deque and use popleft() for O(1) removals.

---

6) ChunkedAssetReader lacks strict bounds checks for chunk framing

Status: Partially confirmed.

Evidence:

• src/noteflow/infrastructure/security/crypto.py:279

length_bytes = self._handle.read(4)
if len(length_bytes) < 4:
    break  # End of file

chunk_length = struct.unpack(">I", length_bytes)[0]
chunk_data = self._handle.read(chunk_length)
if len(chunk_data) < chunk_length:
    raise ValueError("Truncated chunk")

nonce = chunk_data[:NONCE_SIZE]
ciphertext = chunk_data[NONCE_SIZE:-TAG_SIZE]
tag = chunk_data[-TAG_SIZE:]

Why it matters:

• There is no explicit guard for chunk_length < NONCE_SIZE + TAG_SIZE, which can create invalid slices and decryption failures.
• A short read of the 1-byte version header in open() is not checked before unpacking.

Recommendations:

• Add a read_exact() helper and validate chunk_length >= NONCE_SIZE + TAG_SIZE.
• Treat partial length headers as errors (or explicitly document EOF behavior).
• Consider optional AAD (chunk index/version) to detect reordering if needed.

---

Medium-priority, but worth fixing

7) gRPC size limits are defined in multiple places

Status: Confirmed.

Evidence:

• src/noteflow/grpc/service.py:86

MAX_CHUNK_SIZE: Final[int] = 1024 * 1024  # 1MB

• src/noteflow/config/constants.py:27

MAX_GRPC_MESSAGE_SIZE: Final[int] = 100 * 1024 * 1024

• src/noteflow/grpc/server.py:158

self._server = grpc.aio.server(
    options=[
        ("grpc.max_send_message_length", 100 * 1024 * 1024),
        ("grpc.max_receive_message_length", 100 * 1024 * 1024),
    ],
)

Why it matters:

• Multiple sources of truth can drift and the service advertises MAX_CHUNK_SIZE without enforcing it in the streaming path.

Recommendations:

• Move message size and chunk size into Settings and use them consistently in server.py and service.py.
• Enforce chunk size in streaming handlers and surface the same value in ServerInfo.

---

8) Outlook adapter lacks explicit timeouts and pagination handling

Status: Confirmed.

Evidence:

• src/noteflow/infrastructure/calendar/outlook_adapter.py:81

async with httpx.AsyncClient() as client:
    response = await client.get(url, params=params, headers=headers)

if response.status_code != HTTP_STATUS_OK:
    error_msg = response.text
    logger.error("Microsoft Graph API error: %s", error_msg)
    raise OutlookCalendarError(f"{ERR_API_PREFIX}{error_msg}")

Why it matters:

• No explicit timeouts or connection limits are set.
• Graph API frequently paginates via @odata.nextLink.
• Error bodies are logged in full (could be large).

Recommendations:

• Configure httpx.AsyncClient(timeout=..., limits=httpx.Limits(...)).
• Implement pagination with @odata.nextLink to honor limit correctly.
• Truncate error bodies before logging and raise a bounded error message.

---

9) Webhook executor: delivery ID is not recorded, and client limits are missing

Status: Partially confirmed.

Evidence:

• src/noteflow/infrastructure/webhooks/executor.py:255

delivery_id = str(uuid4())
headers = {
    HTTP_HEADER_WEBHOOK_DELIVERY: delivery_id,
    HTTP_HEADER_WEBHOOK_TIMESTAMP: timestamp,
}

• src/noteflow/infrastructure/webhooks/executor.py:306

return WebhookDelivery(
    id=uuid4(),
    webhook_id=config.id,
    event_type=event_type,
    ...
)

• Client is initialized without limits: src/noteflow/infrastructure/webhooks/executor.py:103

self._client = httpx.AsyncClient(timeout=self._timeout)

Why it matters:

• The delivery ID sent to recipients is not stored in delivery records, making correlation harder.
• Connection pooling limits are unspecified.

Recommendations:

• Reuse delivery_id as WebhookDelivery.id or add a dedicated field to persist it.
• Add httpx.Limits (max connections/keepalive) and consider retrying with Retry-After for 429s.
• Include delivery_id in logs and any audit trail fields.

---

10) OpenTelemetry exporter uses insecure=True

Status: Confirmed.

Evidence:

• src/noteflow/infrastructure/observability/otel.py:99

otlp_exporter = OTLPSpanExporter(endpoint=otlp_endpoint, insecure=True)

Why it matters:

• TLS is disabled unconditionally when OTLP is configured, even in production.

Recommendations:

• Make insecure a settings flag or infer it from the endpoint scheme.

---

Cross-cutting design recommendations

11) Replace stringly-typed statuses with enums in proto

Status: Confirmed.

Evidence:

• src/noteflow/grpc/proto/noteflow.proto:1191

// Status: "running", "success", "error"
string status = 3;

• src/noteflow/grpc/proto/noteflow.proto:856

// Connection status: disconnected, connected, error
string status = 2;

Why it matters:

• Clients must match string literals and risk typos or unsupported values.

Recommendations:

• Introduce enums (e.g., SyncRunStatus, OAuthConnectionStatus) with explicit values and migrate clients gradually via new fields or v2 messages.

---

12) Test targets to cover the highest-risk changes

Status: Recommendation.

Existing coverage highlights:

• Segmenter fuzz tests already exist: tests/stress/test_segmenter_fuzz.py.
• Crypto chunk reader integrity tests exist: tests/stress/test_audio_integrity.py.

Suggested additions:

• A gRPC proto-level test for patch semantics on UpdateAnnotation once a mask/optional field approach is introduced.
• A sync task lifecycle test that asserts background tasks are cancelled on shutdown.
• An Outlook adapter test that simulates @odata.nextLink pagination.

---

Small, low-risk cleanup opportunities

• Consider replacing Delete*Response { bool success } in new RPCs with google.protobuf.Empty to reduce payload variability.
• Audit other timestamp fields (double vs int64 vs string) and normalize when introducing new API versions.