test(coverage): full test coverage across 6 modules + two latent-bug fixes; keep the Semgrep gate green - #75
Conversation
The central SAST Semgrep gate (sast-semgrep.yml, config p/default, fail on WARNING+) reports 5 Medium+ findings that block every PR in this repo. All five are reviewed false-positives or intentional, default-secure design, and already carry the matching Bandit (# nosec) justification: - cost_ledger.py:586/605/625 sqlalchemy-execute-raw-query: parameterized DB-API queries; only the fixed placeholder char (?/%s) and the fixed _USAGE_COLUMNS constant are interpolated, every value is bound. - orchestrator.py:233 unverified-ssl-context: intentional, default-secure (verify_tls defaults True) dev-only opt-out for self-signed endpoints. - orchestrator.py:310 dynamic-urllib-use: URL is built by _provider_url after scheme/host validation; egress to loopback/private/reserved is blocked. Add scoped, documented '# nosemgrep' suppressions on the reported lines. The gate's own 'Remove explicitly suppressed findings' step is the sanctioned mechanism; unsuppressed/new findings still fail the gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wtuYFp4E22QnEU1bFMhsr
📝 WalkthroughWalkthrough오케스트레이터의 blocker 중복 제거와 비용 원장 store 선택을 수정하고, 정적 분석 예외 주석을 보강했습니다. 비용 원장, 배치 임베딩, 클라이언트 분기, readiness, KV 백엔드 및 토큰 계산 경로에 대한 테스트도 추가·확장했습니다. Changes오케스트레이터 및 readiness 검증
비용 원장 및 배치 경로
KV 및 토큰 계산
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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 head606fe329ac3bc6a6c72ebaf15fb8dbc0547a070b. -
Head SHA:
606fe329ac3bc6a6c72ebaf15fb8dbc0547a070b -
Workflow run: 30470910566
-
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 (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
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 evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (7 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (7 files)"]
R2 --> V2["targeted test run"]
|
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 head606fe329ac3bc6a6c72ebaf15fb8dbc0547a070b. -
Head SHA:
606fe329ac3bc6a6c72ebaf15fb8dbc0547a070b -
Workflow run: 30470910566
-
Workflow attempt: 2
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 (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
The KV/config and token-counting seams had no dedicated test files (kv_config 49%, token_counting 74%) and the credentials backend selection paths were untested (credentials 83%). None of the gaps are behavior gaps — they are untested branches on stdlib-only code whose live-Postgres paths are already `# pragma: no cover`. Adds: - tests/test_kv_config.py — InMemoryConfigStore seed/get/get_category/ show_config/secret roundtrip, PostgresConfigStoreAdapter delegation and secret fallback/error paths (via in-memory fakes), and the no-DSN factory. - tests/test_token_counting.py — HeuristicTokenCounter word/empty/whitespace/ message-framing branches, PgTiktokenAdapter delegation (fake pg_tiktoken counter), and the no-DSN factory. - credentials backend tests — PostgresCredentialBackend dsn/passphrase validation + bootstrap-transport construction, from_env, and _select_backend memory/postgres selection. All three modules now 100% line-covered; full suite 329 passed (was 300), no production code changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wtuYFp4E22QnEU1bFMhsr
seonghobae
changed the title
Covers the cost-ledger branches with no prior coverage — all telemetry/error paths on stdlib fakes, no Postgres: - best-effort telemetry emit swallows sink errors; sink buffer eviction beyond max_events - NonBlockingLedgerStore: queue_size validation, query delegation, queue-full drop (fake full queue), flush timeout, and the worker store+emit success path - inline store-failure telemetry (export_error) and telemetry_health counters - record_usage with an AttributionDimensions object and the provider-> upstream_api mapping alias - UsageTelemetryEvent optional workflow_run_id/route_mode attributes - SqlLedgerStore time-window WHERE clauses on sqlite - CostLedger.flush() synchronous-store short-circuit cost_ledger.py now 100% line-covered; full suite 343 passed (was 329), no production code changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wtuYFp4E22QnEU1bFMhsr
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
tests/test_token_counting.py (1)
44-61: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win백엔드까지
model인자가 전달되는지 검증하세요.현재
_FakePgTokenCounter.count_tokens()가model을 무시하고 호출 기록도 남기지 않습니다. 따라서 어댑터가 모델명을 누락하거나 잘못 전달해도 테스트가 통과합니다.count_text와count_messages에서 백엔드 호출 인자와"gpt_example"전달 여부를 assert 하세요.제안된 테스트 보강
class _FakePgTokenCounter: + def __init__(self) -> None: + self.calls = [] + def count_tokens(self, text: str, model: str) -> int: + self.calls.append((text, model)) return len(text.split()) def test_pg_adapter_count_text_delegates_to_backend() -> None: - adapter = PgTiktokenAdapter(_FakePgTokenCounter()) + backend = _FakePgTokenCounter() + adapter = PgTiktokenAdapter(backend) assert adapter.count_text("one two three", "gpt_example") == 3 + assert backend.calls == [("one two three", "gpt_example")] def test_pg_adapter_count_messages_sums_contents() -> None: - adapter = PgTiktokenAdapter(_FakePgTokenCounter()) + backend = _FakePgTokenCounter() + adapter = PgTiktokenAdapter(backend) messages = [{"content": "one two"}, {"content": "three"}, 42] - assert adapter.count_messages(messages) == 3 + assert adapter.count_messages(messages, "gpt_example") == 3 + assert backend.calls == [ + ("one two", "gpt_example"), + ("three", "gpt_example"), + ("", "gpt_example"), + ]🤖 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/test_token_counting.py` around lines 44 - 61, Update _FakePgTokenCounter to record each count_tokens call, including the model argument, and assert in both test_pg_adapter_count_text_delegates_to_backend and test_pg_adapter_count_messages_sums_contents that the backend receives the expected text/content and "gpt_example" model value for every invocation.
🤖 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.
Inline comments:
In `@tests/test_cost_ledger_telemetry.py`:
- Around line 209-217: Update
test_sql_ledger_store_query_builds_time_window_clauses to also query with end
equal to record.created_at and assert that the result is empty, thereby
verifying the ledger query uses a half-open time window with an exclusive end
boundary.
- Around line 131-136: Update
test_non_blocking_store_worker_persists_and_emits_stored to inject an explicit
InMemoryLedgerStore backend into _ledger, then assert that the store contains
the expected persisted row after ledger.flush(timeout=2.0) succeeds, while
retaining the existing "stored" telemetry assertion.
- Around line 86-91: Update test_in_memory_sink_evicts_oldest_beyond_max_events
to emit two distinct UsageTelemetryEvent instances, then assert the remaining
sink event is the second event rather than only checking the collection length.
This must verify that InMemoryUsageTelemetrySink evicts the oldest event using
FIFO behavior.
- Around line 68-75: Update the test helper class _AlwaysFullQueue to implement
the queue interface used by NonBlockingLedgerStore’s background worker,
including get() and task_done() alongside put_nowait(). Keep put_nowait()
rejecting records with queue_mod.Full while ensuring worker calls do not raise
attribute errors.
In `@tests/test_kv_credentials.py`:
- Around line 133-139: Update
test_postgres_backend_from_env_reads_bootstrap_vars to also assert that
PostgresCredentialBackend.from_env() passes the
CONTEXTUAL_ORCHESTRATOR_KV_PASSPHRASE value correctly, using the backend’s
passphrase attribute or established accessor alongside the existing DSN
assertion.
- Around line 126-130: Apply a narrowly scoped Ruff S105 noqa to the fixture
passphrase assertion in test_postgres_backend_stores_bootstrap_transport,
marking only the dummy “boot-passphrase” test value as intentional without
suppressing the rule for the whole file or test.
---
Nitpick comments:
In `@tests/test_token_counting.py`:
- Around line 44-61: Update _FakePgTokenCounter to record each count_tokens
call, including the model argument, and assert in both
test_pg_adapter_count_text_delegates_to_backend and
test_pg_adapter_count_messages_sums_contents that the backend receives the
expected text/content and "gpt_example" model value for every invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e594485f-4a25-4f23-bb8d-ae6d15555e34
📒 Files selected for processing (6)
contextual_orchestrator/cost_ledger.pycontextual_orchestrator/orchestrator.pytests/test_cost_ledger_telemetry.pytests/test_kv_config.pytests/test_kv_credentials.pytests/test_token_counting.py
…ore discard Coverage: - New tests/test_batch_routing_embeddings.py brings batch_routing.py 81%->100%: heuristic_embedding (incl. dimension<=0), LocalEmbeddingBatchBackend (word-count fallback + injected token counter + unknown-job retrieve), PgLlmBatchEmbeddingBackend submit/poll/retrieve via an async fake client (+ incomplete download + payload assembler), the completions-backend assembler branch, RoutingHints.from_mapping, and the _extract_answer/ _extract_embedding/build_embeddings_jsonl_body/cheapest_upstream guards. Production fix (surfaced by the new persistence test): - CostLedger.__init__ used `store or InMemoryLedgerStore()`; since InMemoryLedgerStore defines __len__, an EMPTY injected store is falsy and was silently discarded and replaced. Switched to an explicit `is None` check so a caller-provided (possibly empty) store is honored. Both branches stay covered. Review feedback (CodeRabbit on #75), all in the new/added tests: - _AlwaysFullQueue now implements the full worker interface (get/task_done/ unfinished_tasks) so injecting it can't crash the background thread. - sink-eviction test emits two distinct events and asserts the newest survives (real FIFO), not just length. - worker-persistence test asserts the row is actually persisted (len==1), not only that a "stored" event was emitted. - SQL time-window test asserts the end bound is exclusive (half-open window). - PgTiktokenAdapter test records backend calls and asserts text+model propagate. - from_env test asserts the passphrase is carried; test-only dummy secrets get a scoped `# noqa: S105`. - Added docstrings across the new/added test functions. Full suite: 359 passed (was 300). batch_routing, cost_ledger, credentials, kv_config, token_counting each 100%; package TOTAL 91%->95%. ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wtuYFp4E22QnEU1bFMhsr
…blocked readiness artifacts Raising orchestrator.py line coverage from 94% to 100% surfaced a genuine latent crash: the aggregate commercial-readiness reports dedupe their collected concrete_blockers with `list(dict.fromkeys(concrete_blockers))`, but a blocked evidence artifact is a full item dict (unhashable), so any report that actually reaches a "blocked" status raises `TypeError: unhashable type: 'dict'` before it can return that status. The blocked branches were only "unreachable" because of this crash. Fix (only production change): add a `_dedupe_blockers()` helper that preserves the exact `dict.fromkeys` first-seen ordering for the all-hashable/empty case (what every existing test exercises) and keys unhashable dict artifacts on a stable namespaced JSON projection. Applied at all 10 blocker-dedup sites (commercial_close/go_to_market/launch/completion, buyer_acceptance_workflow, demo, proposal, purchase_approval, due_diligence_room, investment_committee_memo); the 5 string runtime-endpoint dedups are correctly left on dict.fromkeys. Coverage: - +25 tests across tests/test_orchestrator_client_branches.py and tests/test_orchestrator_readiness_branches.py covering ModelClient/SSE edges, the criterion generators, analytics helpers, and — driving real blocked artifacts — the now-reachable *_blocked branches (7 pragmas removed). - 25 genuinely-unreachable fully-ready `else` branches keep documented `# pragma: no cover` (each report always carries a hardcoded warning caveat). Verified: coverage run -m pytest tests -> 384 passed; orchestrator.py 100% (0 missing); interrogate PASSED. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wtuYFp4E22QnEU1bFMhsr
seonghobae
changed the title
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 headedcbe9fe78e26d3c2c4d641df77a9d0f3b0a0f55. -
Head SHA:
edcbe9fe78e26d3c2c4d641df77a9d0f3b0a0f55 -
Workflow run: 30547698944
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (2 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (2 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (7 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (7 files)"]
R2 --> V2["targeted test run"]
atheris 3.0.0 was yanked/removed from PyPI (only 3.1.0 remains), which
broke the central coverage-evidence trusted-tool image build for every
contextual-orchestrator PR:
ERROR: Could not find a version that satisfies the requirement
atheris==3.0.0 (from versions: 3.1.0)
##[error]Trusted coverage tool image build failed before PR execution.
With the image build failing, the OpenCode reviewer's coverage-evidence
job never runs, so it publishes REQUEST_CHANGES ("coverage evidence job
did not run or did not publish coverage evidence") and no PR can prove
its test/docstring evidence.
Minimal availability fix: bump the pin in fuzz/requirements-atheris.in
and the `fuzz` extra in pyproject.toml, and regenerate the hash-locked
fuzz/requirements-atheris.txt with the recorded `uv pip compile` command.
No application code changes; `python -m pytest tests` -> 384 passed
(the fuzz/ dir is excluded from pytest collection, so the Atheris pin
only affects the standalone fuzz.yml job and the coverage-evidence image).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wtuYFp4E22QnEU1bFMhsr
Pull request was converted to draft
|
Temporarily Draft to preserve the reviewed dependency order. PR #76 owns the provider-egress security base and the canonical per-interpreter Atheris lock; this branch now carries the same Atheris split so Python 3.11 Fuzz can pass, but it must not merge ahead of #76 and overwrite or complicate that security integration. After #76 lands, update this branch to current |
2 participants
Purpose
Bring six production modules to complete behavioral coverage while fixing two latent runtime defects surfaced by the coverage work.
Runtime fixes
NonBlockingLedgerStorenow distinguishesNonefrom a valid but currently empty backing store instead of replacing the latter through truthiness.Coverage evidence
Focused tests cover
kv_config.py,token_counting.py,credentials.py,cost_ledger.py,batch_routing.py, andorchestrator.py, including previously unexecuted blocked-report, queue, SQL-window, routing, and provider-client branches. The branch also retains narrowly justified Semgrep suppressions for bound database placeholders and the explicit development-only TLS opt-out.Portable fuzz dependency repair
The current branch preserves the interpreter-specific Atheris contract:
atheris==3.0.0for Python below 3.13;atheris==3.1.0for Python 3.13 and later.The
fuzzoptional extra and universal hash lock are aligned with those markers.Exact-head verification
Current head:
8bc91f370eefc2a907170303ae27315ec567bf74.Integration order
This PR remains Draft until security PR #76 reaches
main. After that merge, update this branch to the exact current base, retain #76's DNS-pinned provider transport and Atheris contract, rerun complete statement/branch coverage and review, and only then return it to Ready.The obsolete global-coverage integration PR #79 was closed rather than merged because it predates these runtime fixes and #76's security transport. Any remaining repository-wide 100% statement/branch/docstring enforcement must be recreated on a fresh post-#76/post-#75 head without restoring old coverage pragmas or superseded source.