feat(orchestrator): forward cost-attribution to contextual-orchestrator - #482
feat(orchestrator): forward cost-attribution to contextual-orchestrator#482seonghobae wants to merge 1 commit into
Conversation
scopeweave's AI briefing (POST /api/projects/:id/ai/brief) has always
called contextual-orchestrator with only {model, messages} -- the client's
own comment explained this as caution about the orchestrator's strict
unknown-field rejection. Checked the actual contract
(ALLOWED_CHAT_KEYS/ATTRIBUTION_DIMENSIONS in
contextual-orchestrator/server.py + cost_ledger.py): attribution is an
explicitly allowed field (account/service/upstream_api/model_name/team/
group/company, plus a provider alias), so every scopeweave call has been
landing in the orchestrator's cost ledger as unattributed spend for no
real reason -- a real, buyer-visible gap for a repo that positions itself
as an org-wide LLM cost-review hub.
chat(messages, attribution) now accepts an optional second argument,
sanitizes it against the real allowed-dimension set (drops unknown keys
and null/empty values, coerces survivors to strings, omits the field
entirely rather than sending an empty object when nothing valid remains),
and forwards it. The ai/brief call site now sends
{ service: 'scopeweave', account: p.org_id }.
Fully backward compatible: chat(messages) with no second argument sends
exactly the same body as before (verified in the new test).
New tests/unit/orchestrator.test.mjs (registered in package.json's
test:unit script) exercises the real non-mock path by spawning a fresh
process with ORCHESTRATOR_URL set and a stubbed global fetch, matching
the existing tests/api/auth-secret.test.mjs pattern for env-var-gated
module state. Covers: known dimensions forwarded verbatim, unknown/empty
values dropped, attribution omitted when nothing survives sanitization,
and the no-argument case being byte-identical to prior behavior.
Verified: npm run test:unit (17/17 passed) and npm run test:api (3/3
passed, including the ai/brief mock-path smoke test) both green.
📝 WalkthroughWalkthroughAI 브리핑 호출에 ChangesAttribution 전달
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The change forwards sanitized cost-attribution metadata while preserving existing no-argument behavior; no actionable merge-blocking risk remains, and the PR is merge-ready after normal checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/orchestrator.test.mjs (1)
30-36: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win허용된 값의 문자열 변환을 직접 검증하세요.
현재 테스트는 모든 유지 값을 문자열로 전달합니다. 따라서
String(value)변환이 제거되어도 테스트가 통과합니다.account: 123과 같은 값을 전달하고account: '123'을 기대값으로 검증하세요.수정 예시
- const { url, body } = chatRequestBody({ service: 'scopeweave', account: 'org-123' }); + const { url, body } = chatRequestBody({ service: 'scopeweave', account: 123 }); assert.equal(url, 'http://orchestrator.test/v1/chat/completions'); assert.equal(body.model, 'contextual-orchestrator'); - assert.deepEqual(body.attribution, { service: 'scopeweave', account: 'org-123' }); + assert.deepEqual(body.attribution, { service: 'scopeweave', account: '123' });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/orchestrator.test.mjs` around lines 30 - 36, Update the known-dimensions test around chatRequestBody to pass a non-string account value such as 123, while continuing to assert the other request fields, and expect attribution.account to be the string "123" so the String(value) coercion is directly verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/orchestrator.test.mjs`:
- Around line 30-36: Update the known-dimensions test around chatRequestBody to
pass a non-string account value such as 123, while continuing to assert the
other request fields, and expect attribution.account to be the string "123" so
the String(value) coercion is directly verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 095d8a72-f49e-4a1d-a5e5-a6e18938d8c4
📒 Files selected for processing (5)
CHANGELOG.mdpackage.jsonserver/app.mjsserver/orchestrator.mjstests/unit/orchestrator.test.mjs
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headc40e77336f59b093701b48e57a4f0341ecf2dff7. -
Head SHA:
c40e77336f59b093701b48e57a4f0341ecf2dff7 -
Workflow run: 31701453934
-
Workflow attempt: 1
Coverage evidence
Coverage Decision
- Result: FAIL
- Test evidence: not proven passing
- Docstring evidence: not proven passing when configured
- Failure count: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: orchestrator.test.mjs"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: orchestrator.test.mjs"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage Decision
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (4 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (4 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test: orchestrator.test.mjs"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: orchestrator.test.mjs"]
R2 --> V2["targeted test run"]
|
|
@opencode-agent @cwl-noema-review Repair this as a draft on the live protected |
1 participant
Superseded by the hardened orchestrator integration order: #458 first, then issue #494.
Fresh reconciliation against the current ScopeWeave and contextual-orchestrator contracts found that this PR's unique value is still valid — forwarding
service=scopeweaveand the authenticated organization as theaccountcost-attribution dimension — but this branch is based on pre-hardeningdevelop@74a5e99d...and itsserver/orchestrator.mjsdiverges from the current production-hardening owner #458. Merging this branch would reintroduce the older unbounded/less strict transport instead of preserving #458's authenticated HTTPS-only endpoint validation, message/response bounds, timeout, fail-closed provider parsing, sanitized errors, and development-only deterministic adapter.The unique semantic behavior and verification contract are preserved in #494 using fresh read-only contextual-orchestrator evidence (
ALLOWED_CHAT_KEYSincludesattribution; cost dimensions include account/service/upstream_api/provider/model_name/team/group/company). #494 explicitly requires rebuilding only the attribution delta after #458 integrates and forbids copying this PR's pre-hardening orchestrator substrate.Closing this stale implementation removes an unsafe divergent PR without discarding its buyer-visible cost-observability requirement.