refactor: migrate buyer report callers while preserving aliases - #963
Conversation
…report aliases TaskOrchestrator.buyer_evidence_manifest_report() and buyer_handoff_bundle_report() were docstring-labeled "deprecated ... alias for existing Python consumers", but no such consumer exists. Confirmed via grep: their only callers anywhere were their own dedicated test files. The real deprecated HTTP routes (server.py /api/v1/buyer_evidence_manifests/latest and /api/v1/buyer_handoff_bundles/latest) already bypass them and call commercial_evidence_manifest_report()/commercial_handoff_bundle_report() directly, since the resource-name rename in 73d0412 ("use commercial readiness resource names") repointed the routes at the new commercial_* methods while leaving these two orphaned wrapper methods behind. Remove both dead aliases. Their dedicated tests (test_buyer_evidence_manifest.py, test_buyer_handoff_bundle.py) keep their HTTP-route/OpenAPI/docs contract coverage but now exercise commercial_evidence_manifest_report() / commercial_handoff_bundle_report() directly instead of the removed aliases; the alias-payload-preservation tests are dropped since there is no longer an alias to preserve payload for. test_commercial_blocked_evidence.py's parametrized blocked-degradation coverage is repointed at the same two commercial_* methods so no coverage is lost. Full test suite: 2849 passed, 1 skipped (test_psychometric_routing.py, unrelated — needs Python 3.12 + a fast-mlsirm fetch this sandbox's egress policy blocks). interrogate docstring gate: 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 7 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 (4)
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 |
|
Cross-PR integration contract: routing identity is provider-neutral |
|
Checked this PR's diff against #971's for the cross-PR contract above. Both touch Generated by Claude Code |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. |
|
Contextual-Orchestrator와 관계한 것들을 같이 손보든 어쩌든 해결하세요. Bypass merge 필요하면 가능 (chicken and eggs 상황이라면) + NVIDIA NIM 만 쓰는 건 허용하지 않아요. Contextual-Orchestrator를 쓰세요. Timeout은 적어도 3시간으로 잡으세요. 120초 같은 건 당황스럽군요. Strix가 6시간 이상 동작해서 취약점 잡는 것도 본 일이 있습니다. Opencode와 Noema 는 Coderabbitai 및 Devin 수준으로 실제로 리뷰를 하게 하시오. Strix도 보안 리뷰를 꼼꼼하게 하도록 하시오. 특히 보안 리뷰는 전체 코드로 수행하는 것입니다. Contextual-Orchestrator는 실시간으로 빠르면서 능력이 좋은 모델에 요청을 보내어 시간을 당기시오. |
|
A fix already exists but hasn't merged yet: Generated by Claude Code |
There was a problem hiding this comment.
Noema LLM review
PR #963 migrates internal/test callers from the deprecated buyer_* report methods to the canonical commercial_* methods while preserving the buyer_* aliases for external consumers. The diff is limited to CHANGELOG documentation and test renames; no production code changes are present. The prior review thread confirms the aliases are retained and covered by dedicated payload-equivalence tests. The test renames are consistent, the blocked-evidence parametrization now targets the canonical methods, and the CHANGELOG entry accurately describes the deprecation. No correctness, security, or maintainability regressions identified.
Reviewed changed lines
CHANGELOG.md:13 (RIGHT): Adds a Deprecated section under [0.2.0] - Unreleased accurately stating that internal callers use commercial_* methods while buyer_* aliases and HTTP compatibility routes remain available. Matches the PR intent and prior review conclusions.tests/test_buyer_evidence_manifest.py:65 (RIGHT): Renames the primary test to call commercial_evidence_manifest_report, aligning with the canonical method. The test still exercises the same runtime evidence and assertions, and the main block is updated accordingly.tests/test_buyer_handoff_bundle.py:65 (RIGHT): Renames the primary test to call commercial_handoff_bundle_report, consistent with the canonical method. The main block is updated, and the deprecated alias test remains intact.tests/test_commercial_blocked_evidence.py:63 (RIGHT): Updates the REPORT_NAMES list to use commercial_evidence_manifest_report and commercial_handoff_bundle_report, ensuring the blocked-evidence degradation test covers the canonical methods. The list already contained these names, so the change is consistent.
Adversarial validation
tests/test_buyer_evidence_manifest.py:65 (RIGHT)falsified: The commercial_evidence_manifest_report method might not exist, causing the renamed test to fail with AttributeError. — The test calls orchestrator.commercial_evidence_manifest_report(...) and the prior review thread reports 25 focused tests passed, including this renamed test. Additionally, tests/test_commercial_blocked_evidence.py uses getattr(orchestrator, 'commercial_evidence_manifest_report') in a parametrized test, which would also fail if the method were absent.tests/test_buyer_handoff_bundle.py:65 (RIGHT)falsified: The deprecated buyer_handoff_bundle_report alias might have been removed, breaking external consumers and the alias-preservation test. — The test file still contains test_deprecated_python_handoff_alias_preserves_payload (visible in the file context), and the prior review thread confirms the aliases are retained and covered. The CHANGELOG explicitly states the aliases remain available.tests/test_commercial_blocked_evidence.py:63 (RIGHT)falsified: The commercial_* methods might not degrade to blocked when repository evidence is missing, causing the parametrized test to fail. — The test is parametrized over the updated REPORT_NAMES and the prior review thread reports the full suite passed. The methods are designed to fail closed, and the test explicitly checks for blocked/not_ready/do_not_recommend statuses.- Residual risk: No residual risk identified. The diff is test-only and documentation; the production aliases are confirmed present by the prior review thread and the dedicated alias-preservation tests.
Findings
-
No blocking findings.
-
Result: APPROVE
-
Head SHA:
3ad9417c69bffaac769e2e2d014312eace0a4598 -
Reviewer credential:
noema-review-github-app -
Actor:
cwl-noema-review[bot]
Summary
Org-wide hollow-path audit finding (
빈 깡통 경로 너무 많다..):TaskOrchestrator.buyer_evidence_manifest_report()/buyer_handoff_bundle_report()incontextual_orchestrator/orchestrator.pywere docstring-labeled"deprecated ... alias for existing Python consumers", but no such consumer exists.Independently re-verified against current
origin/main(c6c3a0c9) before touching anything:grep -rn "buyer_evidence_manifest_report\|buyer_handoff_bundle_report"across the whole tree (excluding historicaldocs/superpowers/plans/*planning docs) turned up exactly one caller each — their own dedicated test files (tests/test_buyer_evidence_manifest.py,tests/test_buyer_handoff_bundle.py), plus a generic parametrizedgetattr(...)sweep intests/test_commercial_blocked_evidence.py.server.py's deprecated routes (/api/v1/buyer_evidence_manifests/latest,/api/v1/buyer_handoff_bundles/latest) callcommercial_evidence_manifest_report()/commercial_handoff_bundle_report()directly — they never touch thebuyer_*aliases.git log -Straced the history:73d04124("feat: use commercial readiness resource names") renamed the real implementations frombuyer_evidence_manifest_report/buyer_handoff_bundle_reporttocommercial_evidence_manifest_report/commercial_handoff_bundle_report, repointed both the canonical and the legacy HTTP routes at the newcommercial_*names, and then added back two thinbuyer_*wrapper methods labeled as a "Python consumer" alias — but nothing was ever wired to call them.commercial_go_to_market_readiness_report()(the one place the alias's own historical plan doc claimed as a consumer) also callscommercial_handoff_bundle_report()directly, confirming the orphaning is total.Finding confirmed real. Applied the suggested "remove" direction (the routes already bypass the aliases, so keeping them would mean inventing a call site that was never intended).
Changes
contextual_orchestrator/orchestrator.py: removed both dead alias methods.tests/test_buyer_evidence_manifest.py/tests/test_buyer_handoff_bundle.py: kept the still-live HTTP-route/OpenAPI/admin/docs contract test (the deprecated routes and theirDeprecation/Linkheaders are real, tested behavior); the direct-call structural assertions test now callcommercial_evidence_manifest_report()/commercial_handoff_bundle_report()— the methods they were actually meant to exercise — instead of the removed aliases; dropped the now-meaningless "alias preserves payload" tests.tests/test_commercial_blocked_evidence.py: repointed its parametrized blocked-degradation sweep from the two removed alias names tocommercial_evidence_manifest_report/commercial_handoff_bundle_report, so no blocked-branch coverage is lost.CHANGELOG.md:### Removedentry under[0.2.0] - Unreleased.No production HTTP behavior changes — the deprecated
/api/v1/buyer_evidence_manifests/latestand/api/v1/buyer_handoff_bundles/latestroutes are untouched and keep working exactly as before, since they never called the removed aliases.Test plan
python -m interrogate -c pyproject.toml contextual_orchestrator→PASSED (minimum: 100.0%, actual: 100.0%)python tests/test_buyer_evidence_manifest.py→okpython tests/test_buyer_handoff_bundle.py→okpython -m pytest -q tests/test_commercial_blocked_evidence.py→19 passedpython -m pytest tests -q --ignore=tests/test_psychometric_routing.py→2849 passed, 1 skipped in 780.60stest_psychometric_routing.pywas excluded because it needs Python ≥3.12 plus afast-mlsirmfetch from a GitHub release tarball that this sandbox's egress policy blocks (403, confirmed via directcurl) — unrelated to this change (psychometric routing / MLSIRM, not the buyer/commercial reports touched here).make test(uv-locked) hits the same network restriction resolving that same optional dependency.🤖 Generated with Claude Code
https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Generated by Claude Code