Repository navigation
feat(conformance): expand session and audio fixtures - #49
harshsaver merged 2 commits into
Conversation
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
|
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.
Please use an own-property-safe construction such as |
harshsaver
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
@harshsaver can you please approve the workflow, need to run the CI before merge. |
nexuslinkproductions
left a comment
There was a problem hiding this comment.
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, usesStrEnum):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). Theexpect: reject/rejectLinemechanism 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
unsafeand 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 requiresStrEnum(3.11+) andrequires-pythonis correctly bumped to>=3.11— good. But note the repo-levelMakefile check-pythonusespython3 -m unittest, while the SDK test is pytest-style with subtests; both happen to pass here, but if a contributor'spython3is 3.10 the collection fails with a confusingImportError: cannot import name 'StrEnum'. Consider having the Makefile/CI pin or check the interpreter version explicitly. Not blocking.⚠️ Second-eyes (non-blocking):dart pub getreports "8 packages have newer versions incompatible with dependency constraints" — pre-existing constraint pinning, not introduced here, but worth adart pub outdatedpass 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.
| "CAPTURE_MODE_UNSPECIFIED", "CAPTURE_MODE_TAP_TO_SPEAK", | ||
| "CAPTURE_MODE_HOLD_TO_TALK", "CAPTURE_MODE_HANDS_FREE", | ||
| "CAPTURE_MODE_WAKE_PHRASE", | ||
| } |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
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