fix(rater): preserve generic v1 and add criterion-bound wrapper - #917
Merged
Conversation
|
Warning Review limit reachedNext included review available in 19 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (18)
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 |
seonghobae
pushed a commit
that referenced
this pull request
Aug 30, 2026
… TTL Two unresolved CodeRabbit findings on this PR's current head, fixed narrowly without touching the stale full-history merge: - CostRoutingCoordinator.__init__'s readiness-recovery loop indexed self._readiness_jobs[readiness_job_id] right after listing its keys. On a durable (Valkey/Redis) registry the document can expire between the list and the lookup, raising KeyError out of __init__ and failing server construction. Now skips missing/non-dict entries instead. - _run_provider_readiness_job did the same unchecked self._readiness_jobs[job_id] lookup before entering its try block, so the same TTL race would kill the worker thread with an unhandled KeyError, leaving the job stuck queued/running forever. Now returns quietly when the document is already gone (nothing left to advance). - tests/test_naruon_ecosystem_connector.py called urllib.request.urlopen(req) with no timeout, unlike every other HTTP test in this file (timeout=10); added it so a non-responding local server cannot hang CI indefinitely. Verified with the Rust _token_packer extension built locally (maturin develop --release): tests/test_cost_router.py + tests/test_cost_router_boundaries.py + test_naruon_ecosystem_connector.py = 54 passed. Full suite: 2748 passed, 1 skipped, 1 failed (the failure is tests/test_psychometric_routing.py needing the private fast-mlsirm package, unrelated and unreachable in this sandbox per PR #917's same documented blocker). This PR (#857) remains too diverged from main (165 files / ~13.9k lines) to merge-resolve safely in one pass; left as-is otherwise. Of the three findings flagged for re-verification this cycle, the submit() concurrency and chat() deadline-propagation ones were already resolved on this head, and zdr_only does not exist in this PR's diff at all (that finding belongs to PR #911 instead). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
pushed a commit
that referenced
this pull request
Aug 30, 2026
Two of this PR's own tests hardcoded assumptions that main invalidated during the merge above (32 commits, base was e7618a3 -> 5f2753a): - tests/test_nim_benchmark_workflow_contract.py read .github/workflows/tests.yml, which main renamed to ci.yml in 9b0a356 ("use conventional workflow filename"); the nim_benchmark_quality job content the tests check for is present and intact under the new name. Point both reads at ci.yml. - tests/test_nim_benchmark_release_acceptance.py:: test_budgeted_client_fallback_and_transport_errors matched the old error string "provider .* request failed". Main's contextual_orchestrator/provider_errors.py (PR #879, now on main) reclassifies provider HTTP failures through ProviderUpstreamError (still a RuntimeError subclass) with the new fixed message "provider rejected the request with HTTP {status}"; updated the match regex to the new message. One more failure, tests/test_nim_benchmark_release_acceptance.py:: test_smoke_manifest_cannot_authorize_production_routing, is NOT caused by this merge: it fails identically (same 1280-vs-1283 token-budget numbers on task trick_arithmetic_lily_pads/conduct_bounded) on this PR's own unmerged head b0167b0, contradicting the PR description's claimed "112 passed" NIM-focused run. Left untouched rather than loosening the equal-budget assertion or the 30/0.9 evidence thresholds without the author's input on why token usage grew by 3 tokens for that one locked task; flagged in the gap baseline for follow-up. Full suite after both fixes: 2797 passed, 2 failed (the pre-existing token-budget gap above, plus tests/test_psychometric_routing.py needing the private fast-mlsirm package that is unreachable in this sandbox, same documented blocker as PR #917). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
seonghobae
pushed a commit
that referenced
this pull request
Aug 30, 2026
… streaming _validate_responses_seed and _validate_responses_logprobs left a raw blank string in body on their omit branch instead of popping the key, so _responses_virtual_requires_provider_path's body.get(...) is not None checks saw a truthy "" and wrongly rejected virtual streaming (400 invalid_stream) for orchestrator/auto and orchestrator/free even though seed/top_logprobs were semantically omitted. Both validators now pop the key on omit, matching the existing _validate_chat_logprobs_surface convention. Verified the other CodeRabbit findings on this head are already resolved: zdr_only is already stripped via _ORCHESTRATION_ONLY_KEYS at every upstream payload construction site, text.format is already restricted to json_object/json_schema, and record_stream_usage failures are already decoupled from SSE completion via their own try/except. Regression test added; full suite 2684 passed, 1 failed (fast_mlsirm unreachable-package sandbox gap, same pre-existing blocker as PR #917). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
…n-bounded-context # Conflicts: # docs/planning/adrs/0038-streamed-responses-usage-boundary.md # docs/planning/adrs/0039-streamed-responses-usage-boundary.md # docs/planning/adrs/0040-streamed-responses-usage-boundary.md
Contributor
Author
|
설계 결정: 추상화는 사용하지 않습니다. 와 는 같은 endpoint를 쓰더라도 서로 다른 모델 catalog를 노출할 수 있는 독립 credential/account boundary이므로 각각 discovery·보존·실패 판정을 수행해야 합니다. 모델 간 동적 선택과 latency/quality evidence 공유는 provider 이름 추정이 아니라 오직 명시적 경계에서만 수행하십시오. 이 PR이 해당 전제를 포함하면 제거하거나 현재 head에서 재검증해 주세요. |
Contributor
Author
|
일반화: 앞 코멘트의 NVIDIA는 예시일 뿐입니다. 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor/endpoint/model ID라도 서로 다른 key의 모델·권한·가격·privacy·가용성·실패 상태가 같다고 추정하거나 dedupe하지 마십시오. 개념은 제거하며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 으로만 성립합니다. |
Contributor
Author
|
최종 일반 계약: 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor, endpoint, model ID라도 서로 다른 key의 모델, 권한, 가격, privacy, 가용성, 실패 상태를 같다고 추정하거나 dedupe하지 않습니다. provider family는 없으며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 model_group으로만 성립합니다. |
4 tasks
seonghobae
marked this pull request as ready for review
August 31, 2026 16:57
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
seonghobae
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Comment on lines
+142
to
+155
| ## References | ||
|
|
||
| Evans, E. (2003). *Domain-driven design: Tackling complexity in the heart of | ||
| software*. Addison-Wesley. https://www.pearson.com/en-gb/subject-catalog/p/domain-driven-design-tackling-complexity-in-the-heart-of-software/P200000009375 | ||
|
|
||
| Vernon, V. (2013). *Implementing domain-driven design*. Addison-Wesley. | ||
| https://www.informit.com/store/implementing-domain-driven-design-0133039897 | ||
|
|
||
| These are copyrighted commercial books, so no redistributable full-text PDF is | ||
| committed. The official publisher pages above are linked instead. Evans grounds | ||
| the bounded-context, ubiquitous-language, and Anti-Corruption Layer choices; | ||
| Vernon grounds the small aggregate and invariant boundary used here. An official | ||
| publisher excerpt on aggregates is also available at | ||
| https://www.informit.com/articles/article.aspx?p=2020371. |
Contributor
There was a problem hiding this comment.
🔍 Research grounding needs follow-up
The ADR cites commercial books but records no relevant academic-paper search or artifact. Repository governance requires that grounding for substantive feature changes.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
added
priority: high
status: draft
type: feature
ChatGPT Codex Connector
Comment on lines
+558
to
+564
| evaluation_run_snapshot_ref=payload["evaluation_run_snapshot_ref"], | ||
| item_instance_ref=payload["item_instance_ref"], | ||
| task_revision_ref=payload["task_revision_ref"], | ||
| rubric_revision_ref=payload["rubric_revision_ref"], | ||
| criterion_set=trusted, | ||
| response_evidence_ref=payload["response_evidence_ref"], | ||
| observations=parsed_observations, |
Contributor
There was a problem hiding this comment.
🔴 Provider output controls evaluation identity
With only expected_criterion_set, provider output supplies every run, item, task, evidence, and rater identity without an independent match. Another evaluation's result can be accepted and attributed to the wrong item.
Prompt for agents
Extend the RaterInvocation admission boundary in contextual_orchestrator/rater_observation.py so the owning workflow separately supplies the expected invocation context, not only the criterion set. Bind and compare the invocation, evaluation-run snapshot, item instance, task revision, response evidence, and rater configuration identities before constructing the aggregate. Keep rubric and criterion-set checks, add stable mismatch errors, and add tests proving that each individually substituted provider field is rejected.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+569
to
+572
| try: | ||
| self.criterion_set._assert_integrity() | ||
| except EvaluationCriterionBindingError as exc: | ||
| raise RaterObservationError(exc.code, str(exc)) from exc |
Contributor
There was a problem hiding this comment.
🟡 Post-admission mutation escapes integrity checks
After admission, to_payload verifies only the criterion set; configuration, invocation, and observation fields remain unchecked. Mutated evidence can therefore serialize as valid.
Prompt for agents
Apply the same sealed-construction and replayed-integrity model used by evaluation_criterion_binding.py to the observation aggregate in contextual_orchestrator/rater_observation.py. Ensure RaterConfigurationIdentity, CriterionObservation, and every non-criterion RaterInvocation field are immutable or fingerprinted and validated before export. Add regressions that mutate each child and aggregate with object.__setattr__ or __dict__, then require to_payload to reject the altered evidence.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+471
to
+479
| def exercise_rater_observation(value: Any) -> None: | ||
| """Drive arbitrary JSON through the governed rater observation boundary.""" | ||
| try: | ||
| invocation = RaterInvocation.from_mapping(value) | ||
| except RaterObservationError: | ||
| return | ||
| payload = invocation.to_payload() | ||
| assert 1 <= len(payload["observations"]) <= MAX_RATER_OBSERVATIONS | ||
| assert RaterInvocation.from_mapping(payload).to_payload() == payload |
Contributor
There was a problem hiding this comment.
🔍 Duplicate fuzz target is unreachable
The fuzz.targets variant omits the required trusted set, so every input exits immediately. Keeping two divergent invariants invites silent coverage loss.
Was this helpful? React with 👍 or 👎 to provide feedback.
seonghobae
changed the title
seonghobae
pushed a commit
that referenced
this pull request
Sep 2, 2026
Merging origin/main brought in ADR 0126-rater-contract-module-exception.md (from #917), which collided with this PR's own new 0126-openai-chat-responses-shape-translation.md -- both files auto-merged cleanly (different filenames) but tripped tests/test_planning_adr_identifiers.py's uniqueness contract. Per CLAUDE.md, "a same-number collision is a rename, not a redesign": renumber this PR's ADR to the next free id (0127) and update its front-matter id plus every in-repo reference (provider_api_version.py, chat_responses_shape.py, orchestrator.py, CHANGELOG.md). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
3 tasks
seonghobae
pushed a commit
that referenced
this pull request
Sep 2, 2026
main already carries an unrelated ADR at docs/planning/adrs/0125-domain-neutral-rater-observation-context.md (from #917). Merging main into this branch left two different ADRs both numbered 0125, which is a same-number collision per this repo's convention ("a rename, not a redesign") and fails the executable uniqueness contract in tests/test_planning_adr_identifiers.py. Renumber this branch's ADR to the next free number, 0127, and update its self-reference and the matching contract test's path/title reference. No content change beyond the identifier. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
seonghobae
pushed a commit
that referenced
this pull request
Sep 2, 2026
…ate-controls Bring PR #983 (stateless candidate routing controls) up to date with main, which had advanced past the PR's stale base sha. Resolved one real conflict in fuzz/targets.py: both this PR and main (#917, governed rater observation) independently added a new "11." item to the module docstring's numbered list of fuzzed surfaces (this PR's server._validate_routing, main's rater_observation.RaterInvocation). Kept both, renumbered sequentially as 11 and 12 -- purely additive documentation, no logic conflict. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
This was referenced Sep 2, 2026
seonghobae
pushed a commit
that referenced
this pull request
Sep 2, 2026
) PR #1012 and PR #1020 both independently renumbered their new ADR to 0127 after each collided separately with main's #917. That left both PRs claiming 0127, which would collide again on merge. Since main currently tops out at 0126, 0127 stays genuinely free and PR #1020 keeps it; this PR moves to 0128, the next free number, verified against current main and cross-checked against PR #1020's own branch for any other numbers it introduces. Renames docs/planning/adrs/0127-openai-chat-responses-shape-translation.md to 0128-openai-chat-responses-shape-translation.md and updates every in-repo reference (ADR front matter id, provider_api_version.py and chat_responses_shape.py docstrings, two orchestrator.py comments, and the CHANGELOG entry). Verified tests/test_planning_adr_identifiers.py passes standalone and also passes with PR #1020's 0127 ADR file temporarily copied in alongside this branch's 0128 file (coexistence simulation only, not merged). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BV96rXhqoR3tYZ9AeAVur4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root-cause repair
The earlier branch made
criterion_setmandatory insidecwl_governed_rater_observation/v1. That was not a compatible extension: it changed an already published generic invocation shape and coupled provider-neutral observation transport directly to criterion governance.This head separates the contracts:
cwl_governed_rater_observation/v1again accepts and round-trips the existing generic payload without a criterion set;cwl_criterion_bound_rater_invocation/v1wraps one generic invocation with one complete, non-empty, content-addressed criterion set;The criterion set carries intended use, construct, population/language/domain scope, rubric revision, criterion definitions, admissible-evidence and exclusion rules, response semantics, category definitions, abstention rules, and not-observable rules. A fixed item bank or promoted-anchor set may be empty at cold start; substantive criteria may not.
Authority boundary
contextual-orchestrator records provider/model/human execution and observation evidence. It does not author criterion truth, score respondents, adjudicate, validate, calibrate, activate instruments, promote anchors, certify, or make employment/product decisions.
TDD lineage
A compatibility regression was added before the production repair. It reproduces both failures of the previous design: old generic v1 payloads were rejected, and no separate criterion-bound wrapper existed. Existing criterion-binding tests were then moved to the wrapper contract before production was changed.
Merge boundary
Keep this PR non-merge evidence until the unchanged current head has terminal repository/security/coverage checks and any qualifying independent review required by live protection. Queued, absent, predecessor, local-only, or model-only evidence is not merge evidence. No self-approval, force-push, bypass, gate weakening, fabricated score, or fabricated anchor is requested.