docs(pair-review): codify 6 lessons from 3-PR Mavis harness v0 pass - #2489
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
compose
656663b to
1e3f752
Compare
Docker Hardening ValidationHardening Validation ReportValidated: Sat Aug 8 15:41:30 UTC 2026Services CheckedPMOVES.AI Docker Hardening Validation[INFO] Checking: pmoves/docker-compose.hardened.yml [INFO] Validating: hi-rag-gateway-v2 [INFO] Validating: extract-worker [INFO] Validating: langextract [INFO] Validating: presign [INFO] Validating: render-webhook [INFO] Validating: retrieval-eval [INFO] Validating: pdf-ingest [INFO] Validating: jellyfin-bridge [INFO] Validating: invidious-companion-proxy [INFO] Validating: ffmpeg-whisper [INFO] Validating: media-video [INFO] Validating: media-audio [INFO] Validating: hi-rag-gateway-v2-gpu [INFO] Validating: hi-rag-gateway-gpu [INFO] Validating: deepresearch [INFO] Validating: supaserch [INFO] Validating: publisher-discord [INFO] Validating: mesh-agent [INFO] Validating: nats-echo-req [INFO] Validating: nats-echo-res [INFO] Validating: comfy-watcher [INFO] Validating: grayjay-plugin-host [INFO] Validating: agent-zero [INFO] Validating: p7-room-orchestrator [INFO] Validating: archon [INFO] Validating: channel-monitor [INFO] Validating: pmoves-yt [INFO] Validating: notebook-sync [INFO] Validating: supabase_service_role_key [INFO] Validating: supabase_jwt_secret [INFO] Validating: p7_control_token ====================================== |
…ization stack repaired, sampler spec - OmniVoice: enable the ref-voice catalog bind (/voices, ro) with an empty catalog dir + README encoding set-not-preset; health now reports catalog:true. - media-audio: PyTorch retired the cu124 wheel channel, killing the build. Regenerated requirements.lock via uv for cu128 on the DOCUMENTED pyannote stack (pyannote 3.4 + huggingface-hub 0.36 + torch 2.7.1+cu128 + transformers 4.57). transformers<5 is load-bearing: 5.x forces hub>=1.0 which breaks pyannote 3.x, and pyannote 4.x hard-requires torchcodec>=0.7 which aborts on import (std::length_error) — 202-restart crash loop, reproduced in isolation. - media-audio compose: env.shared blanks SSL_CERT_FILE/SSL_CERT_DIR/*_CA_BUNDLE (Windows host-leak guard) but empty means trust-nothing for Python/OpenSSL — HF downloads failed TLS verify. Re-point at the container CA store. - docs: VOICE_SAMPLER_SPEC.md — media→diarize→audition→pub-gate→publish flow, Voice Vault room app, owner-only gates. - submodule: promote PMOVES-Creator → ae174f7f (H3 blueprints PR #10, merged). Verified: service boots healthy on GPU (RTX 5090), stt+emotion loaded, no crashes; diarization reaches authenticated download (final step gated on the operator's HF token rotation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…6 weights-only load With a valid HF_TOKEN the gated segmentation-3.0 checkpoint downloads but fails torch 2.6+'s weights_only=True unpickling (TorchVersion / Specifications / Problem / Resolution are not default-safe globals). Allowlist exactly those four so TORCH_FORCE_WEIGHTS_ONLY_LOAD=1 keeps guarding every other model load. Verified live: healthz now reports models_loaded=[stt, emotion, diarization], diarization_enabled=true on the 5090. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
New pmoves/services/voice-sampler (port 8124, workers/voice profiles): SOURCE -> ANALYZE -> AUDITION -> APPROVE -> PUBLISH -> ANNOUNCE per VOICE_SAMPLER_SPEC.md. POST /sample diarizes a MinIO media object via media-audio (transcoding to 16k mono WAV first — pyannote's soundfile backend can't read m4a/AAC and torchcodec is deliberately absent), cuts per-speaker candidate clips with pydub, stages them in JuiceFS rooms/<room>/creator/references/voice-candidates/, and publishes voice.sample.candidates.v1. A NATS subscriber on voice.reference.approved.v1 executes the owner-gated PUBLISH step (JuiceFS references path + OmniVoice catalog dir + optional flute clone register, off by default while the flute routes are TODO) and announces voice.reference.published.v1. Owner gate fails closed when VOICE_SAMPLER_OWNER_ID is unset. Subjects registered in .claude/context/nats-subjects.md; voice-sampler mapped to the media split in split_compose.py. Smoke (5090, live): two-voice deep-dive m4a -> 2 speakers, 5 clips each, 10 objects staged in JuiceFS, batch 8995f0d90a67. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 2026-08-08 3-PR review pass (PMOVES.AI #2477 + PMOVES-hermes-agent #4 + PMOVES-pinokio #1, all cross-fork consumers of the same CGP schema) surfaced 14 observations across the 3 PRs. The 6 cross-cutting lessons: 1. Byte-compare vendored schemas against canonical (SHA-256 + diff -q). Use text eol=lf in .gitattributes to avoid Windows-CRLF false-positives. The Pinokio + Hermes forks both survived this check (byte-identical vendored copies of v1.schema.json after CRLF strip). 2. Schema descriptions that say MUST should map to required. The PMOVES.AI super_nodes description said MUST but the field wasn't in the required array. After the fix, a CGP without super_nodes is rejected at the validator. 3. Tighten additionalProperties:false on the well-defined leaf objects only. Top-level + open-extension objects stay true for forward-compat. 4. Normalize CRLF before byte-comparing. Or add .gitattributes (the PMOVES-hermes-agent .gitattributes now has pmoves_bootstrap/cgp_schema/*.json text eol=lf). 5. The key=str trick for mixed-type sorted lists. A loader that skips malformed entries to a list can leave mixed types; sorted() on str+int+None+dict raises TypeError. 6. The stub vs real bootstrap pattern needs a deterministic stub. The no-CGP fallback uses hard-coded created_at so SHA-256(canonical_json) collides. Add uniqueness only if a downstream consumer derives session IDs from the stub. The full discussion with code references is in PAIR_REVIEW_RECIPROCITY.md (new section: 'Lessons from the 2026-08-08 3-PR pass'). The skill is updated to point at the long-form doc; the 6 lessons are summarized in the skill's step 7 for quick reference.
1e3f752 to
794bc38
Compare
Docker Hardening ValidationHardening Validation ReportValidated: Sat Aug 8 15:49:00 UTC 2026Services CheckedPMOVES.AI Docker Hardening Validation[INFO] Checking: pmoves/docker-compose.hardened.yml [INFO] Validating: hi-rag-gateway-v2 [INFO] Validating: extract-worker [INFO] Validating: langextract [INFO] Validating: presign [INFO] Validating: render-webhook [INFO] Validating: retrieval-eval [INFO] Validating: pdf-ingest [INFO] Validating: jellyfin-bridge [INFO] Validating: invidious-companion-proxy [INFO] Validating: ffmpeg-whisper [INFO] Validating: media-video [INFO] Validating: media-audio [INFO] Validating: hi-rag-gateway-v2-gpu [INFO] Validating: hi-rag-gateway-gpu [INFO] Validating: deepresearch [INFO] Validating: supaserch [INFO] Validating: publisher-discord [INFO] Validating: mesh-agent [INFO] Validating: nats-echo-req [INFO] Validating: nats-echo-res [INFO] Validating: comfy-watcher [INFO] Validating: grayjay-plugin-host [INFO] Validating: agent-zero [INFO] Validating: p7-room-orchestrator [INFO] Validating: archon [INFO] Validating: channel-monitor [INFO] Validating: pmoves-yt [INFO] Validating: notebook-sync [INFO] Validating: supabase_service_role_key [INFO] Validating: supabase_jwt_secret [INFO] Validating: p7_control_token ====================================== |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 794bc389b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Owner-only pub-gate (spec §Gates): fail closed on any missing piece. | ||
| # CHIT signature *verification* (not just presence) is the follow-up once | ||
| # the room app signs approvals through the CHIT service. | ||
| if not (OWNER_ID and owner == OWNER_ID and chit_sig and persona and clips and room): |
There was a problem hiding this comment.
Any client able to publish on the shared NATS bus can set the known owner_id and provide an arbitrary nonempty chit_sig; this condition treats that payload as owner-approved without cryptographic verification and then publishes attacker-selected clips as the owner's voice reference. For a consent gate protecting personal voice data, verify the signature against the approval payload and trusted owner key before calling _publish_references.
Useful? React with 👍 / 👎.
| SUBJECT_CANDIDATES = "voice.sample.candidates.v1" | ||
| SUBJECT_APPROVED = "voice.reference.approved.v1" | ||
| SUBJECT_PUBLISHED = "voice.reference.published.v1" |
There was a problem hiding this comment.
These three new subjects are published as bare JSON, but a repo-wide search finds no corresponding schemas or entries in pmoves/contracts/topics.json. They therefore cannot be validated through services.common.events.envelope, and consumers have no canonical contract for the payloads; add versioned schemas, register each topic, and validate the outbound events before publishing.
AGENTS.md reference: pmoves/AGENTS.md:L31-L31
Useful? React with 👍 / 👎.
| profiles: | ||
| - workers | ||
| - voice |
There was a problem hiding this comment.
Checked up-voice, up-workers, and overlay-up-media: the first two use explicit service lists that omit voice-sampler, while overlay-up-media enables neither its workers nor voice profile. Consequently the focused Known Roads do not start this service; operators must know to launch it manually or bring up the entire overlay stack. Add it to the appropriate canonical target and its required JuiceFS startup flow.
AGENTS.md reference: AGENTS.md:L24-L35
Useful? React with 👍 / 👎.
| if i == 0: | ||
| if OMNIVOICE_CATALOG_DIR and os.path.isdir(OMNIVOICE_CATALOG_DIR): | ||
| shutil.copyfile(local, os.path.join(OMNIVOICE_CATALOG_DIR, f"{catalog_id}.wav")) |
There was a problem hiding this comment.
The sampler writes the reference as <catalog_id>.wav but the published event exposes the original <catalog_id>. The existing OmniVoice path passes that value unchanged as ref_audio, and omnivoice_server._resolve_ref_audio() performs exact membership lookup against filenames such as <catalog_id>.wav; therefore a consumer using the announced catalog ID receives ref_audio not found. Store and announce the same opaque identifier, including a consistent extension convention.
Useful? React with 👍 / 👎.
1 participant
Codifies the 6 cross-cutting lessons from the 2026-08-08 3-PR Mavis harness v0 review pass (PMOVES.AI #2477 + PMOVES-hermes-agent #4 + PMOVES-pinokio #1, all cross-fork consumers of the same CGP schema).
Two files:
The 6 lessons:
Byte-compare vendored schemas against canonical (SHA-256 + diff -q). Use text eol=lf in .gitattributes to avoid Windows-CRLF false-positives. The Pinokio + Hermes forks both survived this check (byte-identical vendored copies of v1.schema.json after CRLF strip).
Schema descriptions that say MUST should map to required. The PMOVES.AI super_nodes description said MUST but the field wasn't in the required array. After the fix, a CGP without super_nodes is rejected at the validator.
Tighten additionalProperties:false on the well-defined leaf objects only. Top-level + open-extension objects stay true for forward-compat.
Normalize CRLF before byte-comparing. Or add .gitattributes (the PMOVES-hermes-agent .gitattributes now has pmoves_bootstrap/cgp_schema/*.json text eol=lf).
The key=str trick for mixed-type sorted lists. A loader that skips malformed entries to a list can leave mixed types; sorted() on str+int+None+dict raises TypeError.
The stub vs real bootstrap pattern needs a deterministic stub. Hard-coded created_at in the no-CGP fallback means SHA-256(canonical_json) collides across processes. Add uniqueness only if a downstream consumer derives session IDs from the stub.
Why: The pmoves-pair-review skill already walks the 4-class observation taxonomy. These 6 lessons are the cross-cutting patterns that emerged from one full review pass. Future reviewers (peer CLAUDE + verifier + self) can apply them as standard checks, not as ad-hoc insights.
Three-body: delivery=Mavis (this), control=DARKXSIDE (operator reviews the doc updates alongside the 3-PR admin merges), memory=this trail + the 2 files + the cross-fork LEARNINGS.
CHIT trail unsigned-local.