Skip to content

feat(conformance): expand session and audio fixtures - #49

Merged
harshsaver merged 2 commits into
october-dev:mainfrom
Pushkraj-Space:issue-22-cross-language-fixtures
Sep 11, 2026
Merged

harshsaver merged 2 commits into
october-dev:mainfrom
Pushkraj-Space:issue-22-cross-language-fixtures

Conversation

@Pushkraj-Space

Copy link
Copy Markdown
Contributor

Introduce the manifest-driven 15-set conformance corpus for runtime events, session control, audio frames, and voice source discovery. Add forward-compatibility coverage, strict rejection fixtures, and deterministic synthetic PCM payloads.

Align Dart, TypeScript, Python, and Rust parsing and serialization around the Murmur profile: supported protocol majors, bounded integer handling, known enum names, exact oneof selection, source defaults, and base64 grammar. Move Dart uint64 fields to BigInt and make Rust flattened oneofs unambiguous.

Replace hard-coded SDK fixture tests with shared manifest runners and document the wire/profile/policy boundaries, rejection vocabulary, ordering behavior, and audio generation contract.

Closes #22

Introduce the manifest-driven 15-set conformance corpus for runtime events, session control, audio frames, and voice source discovery. Add forward-compatibility coverage, strict rejection fixtures, and deterministic synthetic PCM payloads.

Align Dart, TypeScript, Python, and Rust parsing and serialization around the Murmur profile: supported protocol majors, bounded integer handling, known enum names, exact oneof selection, source defaults, and base64 grammar. Move Dart uint64 fields to BigInt and make Rust flattened oneofs unambiguous.

Replace hard-coded SDK fixture tests with shared manifest runners and document the wire/profile/policy boundaries, rejection vocabulary, ordering behavior, and audio generation contract.

Closes october-dev#22
@harshsaver

Copy link
Copy Markdown
Member

The existing fixture suites pass locally (protocol checker, Python, TypeScript, and Dart), but the red-team review reproduced a cross-language round-trip bug in the new TypeScript source parser.

parseVoiceSource builds metadata with {} and assigns metadata[key] = value. For a JSON-parsed metadata object {"__proto__":"value","normal":"kept"}, the __proto__ assignment hits the inherited setter and silently drops the valid string entry. voiceSourceToJson(parseVoiceSource(input)) returns only {"normal":"kept"}. The metadata contract allows arbitrary string keys, and the other languages' map representations preserve this entry.

Please use an own-property-safe construction such as Object.fromEntries and add this input to the shared conformance corpus so every SDK checks the same round trip. Leaving open for the reproduced data loss; unavailable/pending CI is not the blocker.

@harshsaver harshsaver left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rechecked the unchanged head on September 8. The TypeScript metadata-loss blocker still reproduces in sdks/typescript/src/index.ts:205-209: parseVoiceSource constructs metadata with {} and writes metadata[key] = value, so a valid JSON-parsed proto string key hits the inherited setter instead of becoming an own property.

Reproduction from the PR root:

import { parseVoiceSource, voiceSourceToJson } from './sdks/typescript/src/index.ts';
const input = JSON.parse('{"sourceId":"synthetic","displayName":"Synthetic source","transport":"SOURCE_TRANSPORT_SYNTHETIC","metadata":{"__proto__":"value","normal":"kept"}}');
console.log(voiceSourceToJson(parseVoiceSource(input)).metadata);
// Actual: { normal: 'kept' }
// Expected: both original string entries retained.

Please use own-property-safe map construction (for example Object.fromEntries after validating values), and add this case to the shared source-discovery conformance corpus so all SDKs protect the same round trip. Do not reject a valid metadata key or add a compatibility fallback to mask the loss.

The current shared protocol checker (15 sets / 43 lines), TypeScript tests, and Python tests all passed locally, but none detects this regression. Those passing suites do not make this merge-ready. The fix and shared regression fixture are still required.

@harshsaver harshsaver left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-audit: the previous metadata-key blocker is fixed. Object.fromEntries preserves proto as an own string-valued key, and the new shared source-discovery fixture exercises this through structural round trips. Local conformance checker (15 sets/44 lines), TypeScript typecheck/tests, and Python tests passed. No new blocker found in the reviewed changes. Not merging yet: the current GitHub workflow is action_required rather than successful; Rust/Dart were unavailable locally. Please obtain workflow approval and a green complete CI run, including Rust and Dart/Flutter integration, on this exact head before merging.

@Pushkraj-Space

Copy link
Copy Markdown
Contributor Author

@harshsaver can you please approve the workflow, need to run the CI before merge.

@nexuslinkproductions nexuslinkproductions 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.

Structured review — murmur #49 (second-pair-of-eyes pass — PR is +2154/−297)

Scope: expand the shared conformance corpus (session + audio fixtures) and align all four SDKs (Dart, Python, Rust, TypeScript) plus the language-agnostic checker. From fork Pushkraj-Space. Refs #22.

Tests/lint actually run by reviewer: checked out pr-49 (head a69c02534c16). Installed Python 3.11, Node 22, Rust 1.88, and Dart 3.13 to run every SDK's conformance suite against the new corpus:

  • Language-agnostic checker: python3 tool/check_conformance.py → "fixtures and connector manifests are consistent (15 sets, 44 lines)".
  • Python SDK (requires-python >=3.11, uses StrEnum): pytest tests/test_conformance.py → 1 passed, 44 subtests passed.
  • TypeScript SDK: npm ci && npm test (node --test) → pass; npm run check (tsc --noEmit) → clean.
  • Rust SDK: cargo test → 1 passed (passes_every_manifest_conformance_set).
  • Dart SDK: dart pub get && dart analyze → No issues found; dart test → All tests passed.

Findings:

  • ✅ Correctness: the checker (tool/check_conformance.py) is the real win — it validates uint32/uint64-as-string, base64 (std + url-safe, padding rules), protocol major/minor, oneof arm ambiguity, enum membership, and sequence ordering, and each invalid fixture asserts a specific rejection reason. All four SDKs consume the same manifest + fixtures and agree (verified by running them). The expect: reject / rejectLine mechanism correctly distinguishes "must parse" from "must reject at line N".
  • ✅ Security: no secrets/PII in the diff (scanned for keys/tokens/private blocks). The Rust addition introduces no unsafe and no network (grep confirms). Fixtures are synthetic. The checker treats fixture content as untrusted data (bounded validation, no eval/exec of fixture content).
  • ✅ Cross-language parity: the four SDKs were updated in lockstep (same new arms/enums: session-control, audio-frames, source-discovery, forward-compatible-events) and all pass the identical corpus — this is exactly what a conformance suite is for, and it's done right.
  • ⚠️ Second-eyes (non-blocking): the Python SDK now requires StrEnum (3.11+) and requires-python is correctly bumped to >=3.11 — good. But note the repo-level Makefile check-python uses python3 -m unittest, while the SDK test is pytest-style with subtests; both happen to pass here, but if a contributor's python3 is 3.10 the collection fails with a confusing ImportError: cannot import name 'StrEnum'. Consider having the Makefile/CI pin or check the interpreter version explicitly. Not blocking.
  • ⚠️ Second-eyes (non-blocking): dart pub get reports "8 packages have newer versions incompatible with dependency constraints" — pre-existing constraint pinning, not introduced here, but worth a dart pub outdated pass at some point.

Verdict: APPROVE — all four SDK conformance suites + the standalone checker run green against the new corpus, no unsafe/network/secrets introduced, cross-language parity holds. The two notes are non-blocking. Solid conformance work.

Comment thread tool/check_conformance.py
"CAPTURE_MODE_UNSPECIFIED", "CAPTURE_MODE_TAP_TO_SPEAK",
"CAPTURE_MODE_HOLD_TO_TALK", "CAPTURE_MODE_HANDS_FREE",
"CAPTURE_MODE_WAKE_PHRASE",
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The validation here is thorough — uint64-as-string with range check, base64 with correct padding rules (std + url-safe), oneof ambiguity detection, and per-fixture rejection reasons. Ran it: '15 sets, 44 lines consistent'. This is what makes the cross-SDK corpus trustworthy.

import json
import re
from dataclasses import dataclass, field
from enum import StrEnum

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

StrEnum requires Python 3.11+, and requires-python is correctly bumped to >=3.11. Non-blocking note: the repo Makefile check-python invokes python3 -m unittest, so a contributor on 3.10 gets a confusing ImportError: cannot import name 'StrEnum' at collection. Consider an explicit interpreter-version check in the Makefile/CI.

@harshsaver harshsaver left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Current head remains unchanged from the last audit: metadata-key preservation fix and shared regression are good; local available conformance checks previously passed. Still waiting for approved, successful full GitHub CI, particularly Dart/Flutter and Rust. No merge while workflow status remains action_required.

@harshsaver harshsaver left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rechecked the audited head a69c025 after explicitly approving its pending workflow. All five checks now pass, including Rust and Dart/Flutter. The Object.fromEntries implementation fixes the proto collision without a compatibility layer, and the shared fixture exercises the protocol edge case. Ready to merge.

@harshsaver
harshsaver merged commit 8691da4 into october-dev:main Sep 11, 2026
5 checks passed
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.

[Conformance] Expand cross-language session and audio fixtures

3 participants