# 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`
```proto
// Creation timestamp (Unix epoch seconds)
double created_at = 4;
```
- `src/noteflow/grpc/proto/noteflow.proto:745`
```proto
// Start time (Unix timestamp seconds)
int64 start_time = 3;
```
- `src/noteflow/grpc/proto/noteflow.proto:1203`
```proto
// Start timestamp (ISO 8601)
string started_at = 7;
```
- `src/noteflow/grpc/proto/noteflow.proto:149`
```proto
// 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`
```proto
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`
```python
# 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`
```proto
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`
```proto
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`
```python
# 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`
```python
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`
```python
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`
```python
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`
```python
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`
```python
MAX_CHUNK_SIZE: Final[int] = 1024 * 1024  # 1MB
```
- `src/noteflow/config/constants.py:27`
```python
MAX_GRPC_MESSAGE_SIZE: Final[int] = 100 * 1024 * 1024
```
- `src/noteflow/grpc/server.py:158`
```python
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`
```python
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`
```python
delivery_id = str(uuid4())
headers = {
    HTTP_HEADER_WEBHOOK_DELIVERY: delivery_id,
    HTTP_HEADER_WEBHOOK_TIMESTAMP: timestamp,
}
```
- `src/noteflow/infrastructure/webhooks/executor.py:306`
```python
return WebhookDelivery(
    id=uuid4(),
    webhook_id=config.id,
    event_type=event_type,
    ...
)
```
- Client is initialized without limits:
  `src/noteflow/infrastructure/webhooks/executor.py:103`
```python
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`
```python
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`
```proto
// Status: "running", "success", "error"
string status = 3;
```
- `src/noteflow/grpc/proto/noteflow.proto:856`
```proto
// 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.

