fix(sdp): unblock CI — fuzz OOM + crash-log visibility + base SAST remediation - #34
Conversation
The bounded Atheris job wrote a crashing input to a crash-*/oom-*/timeout-* file and only uploaded it as a workflow artifact, so diagnosing a fuzz failure required downloading that artifact — infeasible when artifact access is restricted, and it hides the failure cause from the run itself. On a target crash, run_atheris.sh now prints the newly-produced reproducer to the log as base64 with its size, sha256, and the exact replay command, attributed to the crashing target (snapshotting pre-existing reproducer files first so cross-target files are not misattributed). The crash-* artifact upload is unchanged; the base64 in the log decodes byte-for-byte to the same input. Verified: `bash -n` passes; a simulated crash correctly distinguishes the new reproducer from a pre-existing one and the logged base64 round-trips to the original bytes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughJWKS URL에 HTTP 및 HTTPS 스킴 제한을 추가했다. 퍼징 상태 초기화와 실패 재현 정보 출력을 보강했다. 인증, 카탈로그, 정책, 커넥터 및 플랫폼 동작에 대한 회귀 테스트를 추가했다. Changes보안 및 회귀 검증
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
The Atheris execute_query harness aborts with an out-of-memory exit (libFuzzer rc=71, an empty `oom-*` reproducer — a cumulative-growth OOM, not a single input). Root cause: `execute_query`/`draft_sql` record a policy decision and an audit event on every call into the module-level append-only lists `evidence._POLICY_DECISION_LOG` and `catalog._AUDIT_LOG`. The pytest suite resets these via an autouse isolation fixture; the fuzz harnesses have none, so across a bounded-time run (hundreds of thousands of iterations) the lists grow until the process exceeds libFuzzer's rss limit. Add `invariants.reset_in_memory_state()` and call it in the two evidence- recording checks so each iteration starts from a bounded state (seeded catalog data is preserved). This is a harness state-accumulation fix; it does not change the code under test. Verified: with the reset, 8000 execute_query iterations leave `_POLICY_DECISION_LOG`/`_AUDIT_LOG` at 1 entry each; without it, 2000 iterations grow the policy log to 2000 (the unbounded growth behind the OOM). The Hypothesis property suite (`tests/fuzz/test_fuzz_properties.py`) still passes. Surfaced by the crash-reproducer log output added in the previous commit, which attributed the OOM to the execute_query target. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
The central "Semgrep (multi-language SAST)" gate (config p/default, repo-wide
like trivy-fs) reports 3 Medium+ findings on the base tree, so every PR fails
it regardless of its diff. Remediate at the source:
- authz.py (dynamic-urllib, file:// LFI): `_load_jwks_from_url` passed the
operator-configured SDP_OIDC_JWKS_URL straight to urlopen, which honours
file://. Add an http(s) scheme allow-list so a misconfigured JWKS URL can no
longer read a local file, then suppress the audit-only rule with rationale
(the rule flags any dynamic urlopen regardless of the new guard).
- observability.py (dynamic-urllib): false positive — the scheme is already
allow-listed to {http,https} immediately above the urlopen (other schemes
raise). Scoped `# nosemgrep` with rationale.
- graph_store.py (sqlalchemy-execute-raw-query): false positive — the AGE
cypher statement is a psycopg `sql.Composed` of `sql.Literal(...)` values
with the AS-column declaration chosen from a closed allow-list map, and the
params bind as a positional driver parameter; it is not raw string SQL.
Scoped `# nosemgrep` with rationale.
Suppressions use the exact check-ids and are counted as suppressed (not
findings) by the gate. Verified locally: the scheme guard blocks
file://ftp://gopher:// and allows http(s); a local semgrep run with the exact
check-ids confirms all three lines move to `suppressions` (0 active findings).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
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 head82911bd7b0493a1570f8dc8ddd35696ead96d04d. -
Head SHA:
82911bd7b0493a1570f8dc8ddd35696ead96d04d -
Workflow run: 30506150408
-
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 (3 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (3 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (3 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (3 files)"]
R2 --> V2["targeted test run"]
OpenCode Review Overview
--> Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (5 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (5 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Test (15 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test (15 files)"]
R2 --> V2["targeted test run"]
|
The OpenCode coverage-evidence gate requires the changed Python lines to be covered. This PR added a scheme allow-list in `_load_jwks_from_url`, but the existing tests exercise OIDC verification by passing `jwks` directly, so the new fetch/guard lines were uncovered. Add tests/test_authz.py covering `_load_jwks_from_url`: - rejects non-http(s) schemes (file://, ftp://, gopher://, empty) with ValueError (the file:// LFI guard), - fetches and parses JSON over https/http with urlopen monkeypatched, - honours the SDP_OIDC_JWKS_TIMEOUT_SECONDS override. Verified locally: the previously-uncovered lines 123-129 (scheme parse, reject, timeout, urlopen, json parse) are now covered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
Extend tests/test_authz.py to cover every guard branch in the security-critical OIDC verification path, satisfying the changed-file coverage-evidence gate and hardening the module (authz.py 75% -> 100%, 0 missing lines): - _claim_values: str, list, None, and non-str/non-list scalar shapes - load_oidc_role_map: default map, JSON override, and non-object rejection - validate_oidc_claim_shape: missing subject / tenant / exp, invalid exp type, and expired-token branches - _select_jwk: missing kid, non-list keys, and no-matching-key branches - verify_oidc_jwks_token: missing issuer, missing audience, jwks-None-without-URL, unsupported-algorithm rejection, and the env-URL JWKS load path Verified locally: `pytest tests/test_api.py tests/test_authz.py --cov=sdp.authz` reports 100% (0 missing). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
Replace the leftover `_jwt.encode(...) if False else _jwt.encode(...)` with a direct HS256 token construction. Same behavior (unsupported-alg rejection path after the env-URL JWKS load), no unreachable branch. authz.py stays at 100%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
sdp.policy.evaluate is the single choke point every data-access route passes through, and its deny branches are the governance security boundary — but they were only exercised indirectly. Add tests/test_policy.py (5 tests) pinning them directly so a refactor cannot silently downgrade a deny to an allow: - nonexistent resource -> deny (not silent allow) - critical-sensitivity asset -> deny for non-admin (redact obligated), allow for admin (role-gated, not blanket) - publish/patch/deprecate -> deny for a non-admin reader (admin required) - is_mutable reflects allow/deny - every evaluation records exactly one PolicyDecision to the evidence log policy.py 88% -> 98% (the remaining line 109 is an unreachable defensive branch: with the fixed subject table, any tenant-passing subject already holds a reader role). A module-scoped autouse fixture snapshot/restores catalog._DATA and evidence._POLICY_DECISION_LOG for isolation. Test-only; no production change. Verified: tests/test_policy.py 5 passed. (The 3 test_graph_security failures in this sandbox are a pre-existing environmental ModuleNotFoundError: psycopg — CI installs it via requirements-dev.txt; unrelated to this diff.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
…e.py 85% -> 100%) browse.preview / browse.schema are the policy-gated data-access surface. The deny/validation branches were uncovered. Add tests/test_browse.py (8 tests): - preview rejects out-of-range pagination (limit<1, limit>100, offset<0) before any data access - preview / schema propagate a policy denial as PermissionError (cross-tenant subject), and the schema denial is audited - schema on an unknown dataset raises KeyError (404 at the route) - PII columns obligated by policy are masked to '***' in returned rows, and apply_mask is a no-op when nothing is obligated masked (security regression guard) browse.py 85% -> 100%. A module-scoped autouse fixture snapshot/restores catalog._DATA, catalog._AUDIT_LOG, and evidence._POLICY_DECISION_LOG for isolation. Test-only; no production change. Verified: tests/test_browse.py 8 passed; full suite (excl. the pre-existing psycopg-missing graph_security env failures) green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
…> 99%) observability.py is the request-telemetry seam (structured logs + /metrics). Its sink/export paths were undertested. Add tests/test_observability.py (12 tests, no network — mocked urlopen + temp file): - _header_value: default for empty headers, direct + case-insensitive lookup, and the AttributeError fallback for a non-mapping headers object - _file_sink_path: netloc-only and netloc+path forms - _sink_status: unconfigured (memory), https (target=netloc), other scheme - _export_to_sink: file write, http(s) POST invocation (SSRF-adjacent path, asserted via a fake urlopen), and rejection of an unsupported scheme - record_observability_export_error: str and dict inputs (timestamp stamped) - record_request_observation: export=False skips the sink; a sink failure is swallowed and captured as an export error rather than raised observability.py 80% -> 99% (the remaining line 57 is the os.name=='nt' Windows-only path, unreachable on Linux CI). Test-only; no production change. Verified: 12 passed; full suite green (excl. pre-existing psycopg-missing graph_security env failures). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
…100%) config.py is the config-loading seam (org "KV, not env at runtime" rule). _load_from_kv_table (reads application config from the config_entries table) was uncovered. Add tests/test_config.py (6 tests, fake SQLAlchemy engine — no live DB): - no DSN -> None (falls back to bundled defaults) - row parsing: JSON-string decoded, non-string passthrough, invalid-JSON raw fallback - DB/connection failure is swallowed -> None (fail-soft) - AppConfig.from_mapping applies KV overrides over defaults and rejects an out-of-domain graph_backend - default_config_seed returns an independent copy of the defaults config.py 77% -> 100%. Test-only; no production change. Verified: 6 passed; full suite green (excl. pre-existing psycopg-missing graph_security env failures). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
… -> 98%) orchestrator.validate_sql_query is the SELECT-only / single-statement / no-comment / no-literal / no-boolean / no-forbidden-keyword / source-table allowlist guard (the Atheris fuzz target); draft_sql/execute_query are the policy+schema-gated query paths. Add tests/test_orchestrator.py (18 tests): - validate_sql_query: accepts a clean single SELECT; flags each unsafe class (non-SELECT, multi-statement, comments incl. /* */, string literal, AND/OR, forbidden keyword, missing source table, unauthorized table reference) - draft_sql reject branches: unpublished dataset, missing schema, forbidden keyword in the question, unknown columns - draft_sql build branches: explicit-column SELECT + PII-in-analysis assumption, date group-by assumption, and the default count(*) query - execute_query dry-run -> SUCCEEDED with row_count 0 orchestrator.py 89% -> 98%. The 3 remaining lines are unreachable defensive code: invalid_row_limit/invalid_timeout are pydantic-guarded at request construction (row_limit>=1, timeout range), and the "*"+extra-columns branch is rejected earlier by column validation. Test-only; no production change. Verified: 18 passed; full suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
The demo source connectors (sql/rdf/file_lake/rest) must refuse work they should not do. Add tests/test_connectors.py (22 tests, parametrized over the connectors): - get_source_connector rejects an unknown connector id - inspect_schema: KeyError on a missing dataset, ValueError on a wrong source scheme, and success on the matching scheme - preview: KeyError on missing dataset, ValueError on wrong scheme, and PermissionError (audited) when policy denies the connector's 'analyst' subject (exercised with a critical-sensitivity dataset) connectors.py 84% -> 100%. Test-only; no production change. Verified: 22 passed; full sdp suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
…tion (->100%) Take three remaining small sdp modules to 100% with focused, dependency-free tests: - embeddings.py 89%->100%: empty text -> zero vector, L2-normalisation, cosine-similarity overlap ranking, and degenerate inputs (empty / length mismatch / zero vector) -> 0.0 - credentials.py 96%->100%: connector_secret_ref format + prefix override; connector_secret_status presence signal (present/absent) without exposing the raw secret value; and the unsupported-vault-provider ValueError guard - semantic_validation.py 84%->100%: missing-dataset KeyError, and an incomplete dataset (cleared metadata / no approved mapping / no terms) tripping the DatasetShape + BusinessMappingShape violation branches and the terms warning Test-only; no production change. Verified: 9 passed; full sdp suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
Add tests/test_catalog.py and tests/test_demo_smoke.py to meet the org's 100% coverage standard: - catalog.py 89%->100%: version-bump malformed fallback; term-score empty-token skip; join-candidate skips (unpublished, zero-overlap); every search filter continue branch (owner/sensitivity/status/license/min_freshness/inactive completeness gate); list_facet_counts (unsupported field, query scoring + zero-score skip, list-valued field counting); get_dataset_or_404 / schema-history / schema-diff not-found errors; register auto-id + already- exists; patch none-collection skip + schema-version bump; publish idempotent no-op audit; audit-events and related-datasets accessors. - demo_smoke.py 86%->100%: main() exercised directly for its int exit code; the `if __name__ == "__main__"` CLI entry marked `# pragma: no cover` (only reachable on direct execution, not by import). Full suite: 256 passed, 8 skipped; both modules 100%. Tests only (plus the one demo_smoke pragma comment). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head bounded evidence and found no blocking issues.
Findings
No blocking findings.
Summary
PR #34 (fix(sdp): unblock CI - fuzz OOM + crash-log visibility + base SAST remediation) reviewed at head 6263c0c. Changed files inspected as changed-file evidence: src/sdp/authz.py, src/sdp/demo_smoke.py, src/sdp/graph_store.py, src/sdp/observability.py, tests/fuzz/README.md, tests/fuzz/invariants.py, tests/fuzz/run_atheris.sh plus the 12 new tests/test_.py modules (inlined current-head hunks and diff stat: 19 files, 1358 insertions, 5 deletions). Approval sufficiency: the PR intent (unblock security/fuzz gates) is affirmatively supported - the JWKS fetch is restricted to http(s) via a ValueError guard before urlopen (file:// local-file-disclosure remediation), the log-sink urlopen is reachable only for http(s), the three nosemgrep suppressions are each justified by an allow-list guard directly above the flagged call, fuzz memory growth is bounded by reset_in_memory_state(), crash reproducers (base64 + sha256 + replay command) are printed to the run log per README and the run_atheris.sh diff stat (+27), and the coverage tests bring catalog/demo_smoke/authz/observability/config/connectors/credentials/embeddings/orchestrator/policy/semantic-validation/browse to 100% line coverage per changed-file history. Verification posture: Coverage execution evidence: PASS - supported repository test suites passed (test contract python3 -m pytest tests; coverage contract python3 -m coverage run -m pytest tests && python3 -m coverage report --show-missing --fail-under=100); configured repository docstring gates passed or were advisory; Failed GitHub Check evidence: none (no completed failed checks at head); Other unresolved review thread evidence: none; the historical opencode-agent CHANGES_REQUESTED (coverage evidence at older head 82911bd) is reconciled and resolved on this head by the Coverage PASS and head commit 6263c0c (test(sdp): bring catalog + demo_smoke to 100% line coverage); the github-code-quality and coderabbitai comments carried no substantive current-head claims. Linter/static: nosemgrep suppressions with per-site justifications at src/sdp/authz.py, src/sdp/graph_store.py, src/sdp/observability.py; security contracts (bandit, pip_audit, trivy, hadolint) declared in pyproject/workflows; no lint or security failures reported. TDD/regression: 12 new test modules pin previously untested branches (JWKS scheme guard cc6dbeb, log-sink export aec9570, PII masking + policy-deny 23909c7, SQL-safety guards a504523, governance deny-paths d1691c1, connector contract guards 529aa03, DB-backed KV config cbaf7c8, embeddings/connector-secret/semantic validation a72033a); fuzz invariants extended with reset_in_memory_state() while the property-test contract remains enforced. Coverage: Coverage execution evidence: PASS - supported repository test suites passed; head commit and sibling commits raise modules to 100% line coverage per changed-file history. Docstring coverage: Coverage execution evidence reports configured repository docstring gates passed or were advisory; new docstrings (reset_in_memory_state, check_draft_sql, authz guard comment) are behavior-accurate and concrete. DAG: CodeGraph/source-backed flowchart (rendered in prose below) maps the authz scheme guard to OIDC actor resolution and the API auth boundary, tests/test_.py to the pytest suite, and tests/fuzz/* to run_atheris.sh and fuzz.yml; it reflects the base-to-head changed flow (base: unguarded urlopen + unbounded append-only accumulators; head: guarded fetches + bounded fuzz state). PoC/execution: no OPENCODE_EXECUTION_RECEIPT runtime receipts exist in the bounded evidence, so adversarial probes cite trusted source traces, the current-head diff, changed-file history and the Coverage execution evidence PASS; Atheris fuzz runs remain CI-bounded (60s PR / 300s nightly per fuzz.yml). DDD/domain: authz/OIDC boundary, catalog/evidence append-only stores and observability sinks stay consistent with the domain model; only guards and tests changed. CDD/context: scheme allow-lists match the operator-config trust boundary (SDP_OIDC_JWKS_URL, SDP_LOG_SINK_URL); no request-input trust confusion introduced. Similar issues: prior hardening commits in this repo follow the same guard-plus-justified-suppression pattern (145b8cf neutralizes cypher/SQL injection, 99722ec locks supply chain, c2632c0 resolves CodeQL alerts). Claim/concept check: README claims (reproducer base64 + sha256 + replay command in log; crash fails job and uploads artifact) match PR intent and the run_atheris.sh diff stat; no doc/code contradiction found in the inlined hunks. Standards search: SSRF/local-file-disclosure class mitigated by URL scheme allow-lists; psycopg sql.Literal quoting follows SQL-injection-safe parameterization practice; introduced identifiers (ALLOWED_JWKS_SCHEMES, reset_in_memory_state, the pragma comment) are multi-word, idiomatic, non-reserved; no sequential-id exposure or enumeration risk in changed surfaces. Compatibility/convention: no public API/schema/config-key changes; the guard only changes behavior for non-http(s) JWKS URLs (previously an implicit urllib fetch), which is the intended remediation; test naming follows the repo test.py convention; the pragma: no cover comment aligns with the 100% coverage gate. Breaking-change/backcompat: http(s) JWKS and log-sink URLs behave exactly as before; file:// JWKS URLs now fail fast with a clear ValueError instead of performing local reads. Implementation completeness: no placeholder bodies, TODO-only branches or fake returns introduced; reset_in_memory_state and the scheme guards are fully implemented and exercised by tests. Performance: reset bounds append-only accumulator growth across fuzz iterations (the documented oom- root cause) while preserving seeded catalog data; production hot paths add at most one urlsplit per JWKS load. Developer experience: DX surface classified = fuzz CI output and test authoring; crash reproducers (base64 + sha256 + replay command) in run logs make failure diagnosis artifact-free, the README documents the new output, and 12 focused test modules localize failures. User experience: UX surface classified = CLI/log surfaces (demo_smoke.py CLI and fuzz runner output); demo_smoke main() exit-code contract unchanged (0 ready / 1 not); no web UI surfaces changed (web_app_review_requirements empty). Visual/DOM: non-web interaction surface reviewed (CLI/API/logs/docs/workflow output only); no DOM/rendering changes. Accessibility/i18n: no UI strings, layout, scrolling, or motion changes; the prefers-reduced-motion contract is not implicated; Korean test fixtures in tests/test_api.py are unaffected. Supply-chain/license: zero dependency changes; fuzz requirements already pinned with --require-hashes per README; pip_audit/bandit/trivy contracts unchanged. Packaging: pyproject.toml present (python >=3.10, workflows run 3.12); unpackaged_source_surfaces empty; run_atheris.sh invoked with PYTHONPATH=src per README; no packaging gaps introduced. Security/privacy: SSRF/file-disclosure guards on both urlopen call sites, the cypher query is literal-quoted, fuzz crash payloads are synthetic fuzz inputs (no secrets) printed as base64 to CI logs; no new secrets, authz, tenant-isolation, or identifier-exposure surfaces introduced.
Approval sufficiency: bounded evidence supplied affirmative approval evidence for changed files, coverage/docstring posture, risk surfaces, and current-head verification; approval is not based merely on the absence of known blockers.
Verification posture: CodeGraph evidence was initialized and bounded current-head evidence reviewed for changed-file evidence including src/sdp/authz.py, src/sdp/demo_smoke.py, src/sdp/graph_store.py, src/sdp/observability.py, tests/fuzz/README.md, and 14 more.
Linter/static: workflow/static review evidence is bounded by the current-head GitHub Checks gate and changed-file evidence.
TDD/regression: coverage execution evidence and focused changed hunks were reviewed from bounded-review-evidence.md.
Coverage: coverage execution evidence reports supported repository test suites passed.
Docstring coverage: coverage execution evidence reports configured repository docstring gates passed or docstring coverage was advisory.
DAG: CodeGraph/source-backed behavior map connects src/sdp/authz.py to the affected review, runtime, or workflow path and required checks.
PoC/execution: coverage-evidence job executed on the current head and reported PASS.
DDD/domain: workflow and repository-governance invariants were reviewed against changed files in bounded evidence.
CDD/context: CodeGraph evidence, changed-file history, and focused hunks were reviewed from bounded-review-evidence.md.
Similar issues: changed-file history evidence was reviewed for comparable local precedents.
Claim/concept check: bounded evidence, repository source, current-head workflow evidence, and, where numeric, scientific, statistical, or literature-backed claims are affected, original-paper/formula evidence and parameter-recovery expectations were used for claims.
Standards search: standards and external-source claims require trusted bounded source evidence prepared outside the isolated model process; no evidence-backed standards blocker is present in bounded evidence.
Compatibility/convention: changed workflow/script conventions, object naming, and reserved-word safety for schema/API/config/code surfaces were checked in bounded evidence.
Breaking-change/backcompat: deployment evidence and changed-file history were checked for backward-compatibility risk.
Performance: changed surfaces were checked for performance risk in bounded evidence.
Developer experience: changed automation, review, test, setup, and maintenance surfaces were checked for helpful or obstructive DX impact in bounded evidence.
User experience: connected user, operator, API, CLI, documentation, review-comment, status-check, rendering, and workflow-reader behavior was checked for contradictions against code, docs, and tests in bounded evidence.
Visual/DOM: deterministic repair does not infer browser runtime execution; source-backed DOM/UI evidence and trusted workflow receipts were reviewed when present, and non-web surfaces used API/CLI/log/docs/workflow evidence instead.
Accessibility/i18n: accessibility, localization, and human-readable text surfaces were checked where UI, CLI, API message, docs, logs, or review text changed.
Supply-chain/license: dependency, package, model, container, and external-tool changes were checked in bounded evidence.
Packaging: package, build, test, lint, and security contracts were checked in bounded evidence.
Security/privacy: workflow-token, review-gate, and repository-automation security/privacy boundaries were checked in bounded evidence.
Adversarial validation
{"status":"passed","probes":[{"path":"src/sdp/authz.py","line":127,"hypothesis":"The new JWKS scheme allow-list can be bypassed so a file:// JWKS URL still reaches urlopen and discloses local files.","attack_or_counterexample":"Set SDP_OIDC_JWKS_URL=file:///etc/passwd (or another non-http(s) scheme such as gopher://) and call _load_jwks_from_url; urllib honours file://, so without the guard an operator misconfiguration becomes local file disclosure.","evidence":"Trusted source trace at src/sdp/authz.py:127 observed the bounded scheme allow-list branch reject the counterexample: the current-head diff adds scheme = urlsplit(jwks_url).scheme.lower() and raises ValueError('OIDC JWKS URL must use the http or https scheme') for any scheme outside _ALLOWED_JWKS_SCHEMES = frozenset({'https', 'http'}), so the 'file' scheme is rejected before the urlopen call at this line can execute; tests/test_authz.py (added, 165 lines) covers the OIDC/JWKS module and the scheme guard per changed-file history (cc6dbeb2 test(authz): cover JWKS URL scheme guard), and Coverage execution evidence reports PASS (supported repository test suites passed). source-line-sha256=36c8c70dcb2fdbbe740ed506f1bf3c19fe7bf6219acf2334dc394cf89e042e99","outcome":"falsified"},{"path":"src/sdp/graph_store.py","line":478,"hypothesis":"The nosemgrep suppression masks an injectable raw-SQL execute where a crafted cypher query or graph name reaches Postgres unquoted.","attack_or_counterexample":"Pass a query containing a statement terminator and DROP TABLE, and a graph_name containing a quote, through the cypher executor; if unquoted, the payload would break out of the cypher literal and execute a second statement.","evidence":"Trusted source trace at src/sdp/graph_store.py:478 observed the injection counterexample fail: the statement is built as pg_sql.SQL('SELECT * FROM cypher({}, {}, %s) AS ({})').format(pg_sql.Literal(self.graph_name), pg_sql.Literal(query), result_columns), so the hostile query and graph_name are quoted by psycopg sql.Literal and cannot escape the cypher string literal; result_columns is selected from a closed allow-list dict whose None check raises ValueError for unknown declarations; params are passed as a bound positional driver parameter (json.dumps(params),); the prior hardening commit 145b8cfb (fix(security): neutralize cypher/SQL injection + authz graph endpoints) corroborates this boundary, no raw string SQL is executed, and Coverage execution evidence reports PASS. source-line-sha256=89dcfc5a4ca23859d6ebf246eba562f785ec8c5b998f2dc6c951a736a62f3afe","outcome":"falsified"},{"path":"src/sdp/observability.py","line":139,"hypothesis":"The nosemgrep suppression on the log-sink urlopen lets a non-http(s) SDP_LOG_SINK_URL reach urlopen, enabling local file disclosure.","attack_or_counterexample":"Set SDP_LOG_SINK_URL=file:///etc/passwd and trigger an observability export; if the scheme guard is not in force, urlopen would read the local file.","evidence":"Trusted source trace at src/sdp/observability.py:139 observed the sink scheme bypass counterexample fail: _export_to_sink branches on the pre-computed scheme, the file branch writes via path.open() and returns, the http/https branch contains the urlopen call, and every other scheme falls through to raise ValueError('unsupported SDP_LOG_SINK_URL scheme: ...'), so the 'file' scheme cannot reach urlopen; tests/test_observability.py (added, 150 lines) covers log-sink export per changed-file history (aec9570f test(observability): cover log-sink export + header extraction), and Coverage execution evidence reports PASS. source-line-sha256=34578c2b7df85bbbf2d6b1328cf96c184c2ebe16916bc9520c7a67c524249695","outcome":"falsified"},{"path":"tests/fuzz/invariants.py","line":13,"hypothesis":"reset_in_memory_state() clears the seeded catalog datasets (catalog._DATA), so fuzz iterations lose the data the code under test operates on and the OOM fix degrades the harness to vacuous passes.","attack_or_counterexample":"Run fuzz iterations that depend on the seeded catalog rows (for example the search invariant over the seeded datasets) immediately after the reset and check whether the data under test survives.","evidence":"Trusted current-head diff trace at tests/fuzz/invariants.py:13 (the new import line adding catalog, evidence, observability) shows the added reset_in_memory_state() clears only the append-only accumulators catalog._AUDIT_LOG, catalog._SCHEMA_HISTORY, evidence._POLICY_DECISION_LOG and calls observability.reset_request_observability(); the function docstring explicitly states the seeded catalog datasets (catalog._DATA) are left intact so the code under test still has data to operate on, so the seeded-data-loss hypothesis fails; the oom-* exit class it fixes (libFuzzer rss limit on unbounded list growth) is documented in the same docstring, and Coverage execution evidence reports PASS. source-line-sha256=b1496fb1fefd21ebfdef12cd708802a0240130fec5564c227af20bbb5f26f337","outcome":"falsified"}],"residual_risk":"Bounded residual risk: (1) http (non-TLS) JWKS and log-sink URLs remain accepted - a deliberate operator-config trade-off; deployments should prefer https. (2) The tail of tests/fuzz/invariants.py and the full run_atheris.sh hunk were truncated in the evidence packet, so the check_execute_query reset placement and the reproducer-logging tail were verified via the diff stat, README, changed-file history and receipts rather than full text. (3) No in-repo runtime receipt exists for an executed Atheris run; fuzz execution remains CI-bounded (fuzz.yml 60s/300s)."}- Result: APPROVE
- Reason: PR #34 is a coherent CI-unblock PR: the JWKS fetch and log-sink urlopen calls are now gated by explicit scheme allow-lists (SSRF/local-file-disclosure remediation), the three nosemgrep suppressions each sit directly under a guard that justifies them, fuzz memory growth is bounded by reset_in_memory_state(), crash reproducers are surfaced in the run log, and 12 new test modules plus a pragma change bring previously untested branches to full line coverage. Coverage execution evidence reports PASS (supported repository test suites passed; docstring gates passed or advisory), no failed GitHub Checks and no unresolved review threads exist at head 6263c0c, and four adversarial probes (JWKS scheme bypass, raw-SQL injection via the cypher execute, log-sink scheme bypass, seeded-data loss in the fuzz reset) were all falsified by trusted source traces and the passing suite.
- Head SHA:
6263c0ccb5677df5ac009aae859d183db3c23a75 - Workflow run: 30713547086
- Workflow attempt: 1
Superseded automated OpenCode change request from a previous head; exact current head 6263c0c has a later OpenCode approval.
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head bounded evidence and found no blocking issues.
Findings
No blocking findings.
Summary
Pull request overview: PR #34 'fix(sdp): unblock CI — fuzz OOM + crash-log visibility + base SAST remediation' (head 6263c0c, base 9a7088e). Three workstreams: (1) base-tree SAST remediation — src/sdp/authz.py adds an http(s) scheme allow-list in _load_jwks_from_url that raises ValueError for file:// and other schemes before urlopen, and nosemgrep suppressions at src/sdp/graph_store.py:478 and src/sdp/observability.py:139 whose justifications match the code (allow-listed psycopg sql.Composed with Literal-wrapped identifiers and bound params; scheme-guarded urlopen); (2) fuzz OOM fix — tests/fuzz/invariants.py adds reset_in_memory_state() clearing catalog._AUDIT_LOG/_SCHEMA_HISTORY and evidence._POLICY_DECISION_LOG plus observability.reset_request_observability() at the top of check_draft_sql so bounded Atheris runs no longer exceed libFuzzer's rss limit; (3) crash-log visibility — tests/fuzz/run_atheris.sh prints base64 + sha256 + replay command on failure, documented in tests/fuzz/README.md; plus src/sdp/demo_smoke.py marks the main guard pragma:no-cover and 12 new test files (test_authz.py through test_semantic_validation.py, +1358 lines) bring modules to the 100% coverage gate. Findings: no blocking findings (none P0/P1/P2). Current-head evidence: Coverage execution evidence Result: PASS (supported repository test suites passed); Failed GitHub Check evidence: no completed failed checks; Other unresolved review thread evidence: none. Historical CHANGES_REQUESTED from opencode-agent (coverage evidence gap) is superseded by the current-head Coverage PASS and the later opencode-agent APPROVED review; the code-quality bot comment was addressed by commit 38ab65f; CodeRabbit 'review limit reached' carried no substantive claims. mergeStateStatus 'blocked' is branch policy, not DIRTY/CONFLICTING, so no merge-conflict repair is required.
Labels:
- Approval sufficiency: affirmative evidence supports PR intent — guard implemented and tested, OOM reset verified by green suite, docs updated, no failed checks.
- Verification posture: trusted Coverage execution evidence (Result: PASS) for 'python3 -m coverage run -m pytest tests && python3 -m coverage report --show-missing --fail-under=100'; no OPENCODE_EXECUTION_RECEIPT browser/E2E receipts apply (non-web PR).
- Linter/static: nosemgrep suppressions in src/sdp/authz.py, src/sdp/graph_store.py, src/sdp/observability.py are source-consistent with the guarded code; hadolint lint contract targets the Dockerfile, which is unchanged.
- TDD/regression: 12 new test files cover OIDC/JWKS scheme guard, browse PII/policy-deny, catalog+demo_smoke, config KV loader, connector contract guards, credentials, embeddings, observability export, orchestrator SQL-safety, policy deny-paths, semantic validation; regression direction: python3 -m pytest tests.
- Coverage: Coverage execution evidence Result: PASS — supported repository test suites passed on head 6263c0c.
- Docstring coverage: configured repository docstring gates passed or docstring coverage was advisory (Coverage execution evidence); docstring_commands list is empty.
- DAG: Mermaid flowchart below reflects head flow (base-to-head changes: JWKS scheme guard, fuzz reset, reproducer logging); CodeGraph index (65 files, 1181 nodes, 2560 edges) is up to date at head.
- PoC/execution: Coverage execution evidence is the only trusted execution evidence; bounded fuzz CI (fuzz.yml 60s/target PR) shows no failed checks on this head.
- DDD/domain: JWKS guard sits on the OIDC trust boundary; the fuzz reset isolates harness state from domain logic (append-only logs are testing artifacts, not domain behavior).
- CDD/context: demo/enterprise context split preserved; guard applies to operator-config inputs; tests are harness-scoped.
- Similar issues: prior opencode-agent CHANGES_REQUESTED (coverage evidence) resolved at current head; code-quality bot finding addressed by 38ab65f; CodeRabbit carried no substantive claims.
- Claim/concept check: JWKS guard matches the stated threat model (misconfigured file:// disclosure) and is corroborated by tests/test_authz.py; the fuzz OOM diagnosis (unbounded list accumulators) is documented in the invariants.py docstring and matches the reset.
- Standards search: no new external standards introduced; fuzz README continues to cite the in-repo Miller 1990 paper; semgrep rule-path IDs used for suppressions.
- Compatibility/convention: naming reviewed — ALLOWED_JWKS_SCHEMES and reset_in_memory_state are multi-word snake_case; no new DB/API/config identifiers or sequential-id exposure; test files follow tests/test*.py convention.
- Breaking-change/backcompat: _load_jwks_from_url now rejects non-http(s) schemes with ValueError (previously a file:// URL would have been fetched); no public API signature changes; behavior change only for invalid operator config.
- Performance: reset_in_memory_state() clears O(1) lists per fuzz iteration, bounding RSS growth; no runtime hot-path changes.
- Developer experience: DX surface classified = fuzz failure diagnostics; base64+sha256+replay command printed in CI log (tests/fuzz/README.md), removing artifact-download friction.
- User experience: UX surface classified = demo_smoke CLI exit-code contract (unchanged; pragma no-cover only) and fuzz-runner log output (new).
- Visual/DOM: no web UI changes; non-web interaction surface reviewed = CLI/demo smoke and fuzz-runner log output.
- Accessibility/i18n: no UI changes; no new i18n surface.
- Supply-chain/license: no dependency or lockfile changes in this PR.
- Packaging: no packaging changes; pyproject.toml contract (python >=3.10, tests/coverage/security commands) unchanged and coverage evidence passed.
- Security/privacy: JWKS file:// disclosure closed by the allow-list; both urlopen paths are scheme-guarded; no new secrets, authz boundaries, or exposed identifiers; fuzz logs print synthetic crash inputs only.
Changed-file evidence inspected: src/sdp/authz.py, src/sdp/demo_smoke.py, src/sdp/graph_store.py, src/sdp/observability.py, tests/fuzz/invariants.py, tests/fuzz/README.md (focused current-head hunks).
Mermaid DAG (head flow):
flowchart LR
A['src/sdp/authz.py JWKS scheme allow-list'] --> B['_load_jwks_from_url http(s) only']
A --> C['file:// SDP_OIDC_JWKS_URL rejected with ValueError']
B --> I['OIDC actor context for token verification paths']
D['tests/fuzz/invariants.py reset_in_memory_state'] --> E['audit and policy logs bounded per iteration']
E --> F['Atheris oom-* exit prevented']
G['tests/fuzz/run_atheris.sh reproducer logging'] --> H['base64 + sha256 + replay command in CI log']
C --> J['verification: python3 -m pytest tests (coverage PASS)']
E --> J
H --> J
Approval sufficiency: bounded evidence supplied affirmative approval evidence for changed files, coverage/docstring posture, risk surfaces, and current-head verification; approval is not based merely on the absence of known blockers.
Verification posture: CodeGraph evidence was initialized and bounded current-head evidence reviewed for changed-file evidence including src/sdp/authz.py, src/sdp/demo_smoke.py, src/sdp/graph_store.py, src/sdp/observability.py, tests/fuzz/README.md, and 14 more.
Linter/static: workflow/static review evidence is bounded by the current-head GitHub Checks gate and changed-file evidence.
TDD/regression: coverage execution evidence and focused changed hunks were reviewed from bounded-review-evidence.md.
Coverage: coverage execution evidence reports supported repository test suites passed.
Docstring coverage: coverage execution evidence reports configured repository docstring gates passed or docstring coverage was advisory.
DAG: CodeGraph/source-backed behavior map connects src/sdp/authz.py to the affected review, runtime, or workflow path and required checks.
PoC/execution: coverage-evidence job executed on the current head and reported PASS.
DDD/domain: workflow and repository-governance invariants were reviewed against changed files in bounded evidence.
CDD/context: CodeGraph evidence, changed-file history, and focused hunks were reviewed from bounded-review-evidence.md.
Similar issues: changed-file history evidence was reviewed for comparable local precedents.
Claim/concept check: bounded evidence, repository source, current-head workflow evidence, and, where numeric, scientific, statistical, or literature-backed claims are affected, original-paper/formula evidence and parameter-recovery expectations were used for claims.
Standards search: standards and external-source claims require trusted bounded source evidence prepared outside the isolated model process; no evidence-backed standards blocker is present in bounded evidence.
Compatibility/convention: changed workflow/script conventions, object naming, and reserved-word safety for schema/API/config/code surfaces were checked in bounded evidence.
Breaking-change/backcompat: deployment evidence and changed-file history were checked for backward-compatibility risk.
Performance: changed surfaces were checked for performance risk in bounded evidence.
Developer experience: changed automation, review, test, setup, and maintenance surfaces were checked for helpful or obstructive DX impact in bounded evidence.
User experience: connected user, operator, API, CLI, documentation, review-comment, status-check, rendering, and workflow-reader behavior was checked for contradictions against code, docs, and tests in bounded evidence.
Visual/DOM: deterministic repair does not infer browser runtime execution; source-backed DOM/UI evidence and trusted workflow receipts were reviewed when present, and non-web surfaces used API/CLI/log/docs/workflow evidence instead.
Accessibility/i18n: accessibility, localization, and human-readable text surfaces were checked where UI, CLI, API message, docs, logs, or review text changed.
Supply-chain/license: dependency, package, model, container, and external-tool changes were checked in bounded evidence.
Packaging: package, build, test, lint, and security contracts were checked in bounded evidence.
Security/privacy: workflow-token, review-gate, and repository-automation security/privacy boundaries were checked in bounded evidence.
Adversarial validation
{"status":"passed","probes":[{"path":"src/sdp/authz.py","line":127,"hypothesis":"SDP_OIDC_JWKS_URL pointing at file:// (or another non-http(s) scheme) bypasses the new allow-list guard and _load_jwks_from_url reads a local file via urllib, defeating the stated remediation.","attack_or_counterexample":"Set SDP_OIDC_JWKS_URL=file:///etc/passwd (the operator misconfiguration the guard claims to block) and call the OIDC token-verification path.","evidence":"Trusted source trace at src/sdp/authz.py:127: the added guard computes scheme = urlsplit(jwks_url).scheme.lower() and raises ValueError for any scheme outside {https, http} before urlopen is reached; tests/test_authz.py was added with commit cc6dbeb2 'test(authz): cover JWKS URL scheme guard (unblock coverage-evidence)' asserting this rejection, and the trusted Coverage execution evidence reports Result: PASS for the supported repository test suites on the head; source-line-sha256=36c8c70dcb2fdbbe740ed506f1bf3c19fe7bf6219acf2334dc394cf89e042e99","outcome":"falsified"},{"path":"tests/fuzz/invariants.py","line":129,"hypothesis":"reset_in_memory_state() references module attributes that do not exist (catalog._AUDIT_LOG, catalog._SCHEMA_HISTORY, evidence._POLICY_DECISION_LOG, observability.reset_request_observability), or clearing them breaks subsequent fuzz/property iterations, so the fuzz/property suite fails on the head.","attack_or_counterexample":"Run the Hypothesis property tests / Atheris harness that import tests/fuzz/invariants.py and call check_draft_sql repeatedly across iterations.","evidence":"Trusted source trace at tests/fuzz/invariants.py:129: the reset runs at the top of check_draft_sql and mirrors the pytest autouse isolation fixture described in its docstring; the trusted Coverage execution evidence reports Result: PASS for the supported repository test suites on head 6263c0ccb5677df5ac009aae859d183db3c23a75, and the Failed GitHub Check evidence states 'No completed failed GitHub Checks were present'; source-line-sha256=a01245fc69d13d41c6780b2a30e6319f3f5dc4c23c689af31cd4e7b584408a3a","outcome":"falsified"},{"path":"src/sdp/graph_store.py","line":478,"hypothesis":"The nosemgrep suppression hides raw SQL injection: attacker-controlled columns or query text reaches cursor.execute unquoted.","attack_or_counterexample":"Pass an unknown AGE result declaration (columns not in the allow-list map) or a malicious query string through the graph execute path.","evidence":"Trusted source trace at src/sdp/graph_store.py:478: result_columns comes from a closed allow-list dict and any unknown value raises ValueError('unsupported AGE result declaration'); graph_name and query are wrapped in pg_sql.Literal and params are bound as a positional driver parameter (json.dumps(params)), so the suppression comment accurately describes a psycopg sql.Composed statement rather than raw string SQL; the change is comment-only versus base and the trusted Coverage execution evidence reports Result: PASS; source-line-sha256=89dcfc5a4ca23859d6ebf246eba562f785ec8c5b998f2dc6c951a736a62f3afe","outcome":"falsified"},{"path":"src/sdp/observability.py","line":139,"hypothesis":"_export_to_sink can reach urlopen with a non-http(s) scheme (e.g., file://), enabling local file read via SDP_LOG_SINK_URL.","attack_or_counterexample":"Set SDP_LOG_SINK_URL=file:///etc/passwd and export an observation through the log-sink path.","evidence":"Trusted source trace at src/sdp/observability.py:139: urlopen is only reachable inside the if scheme in {'http','https'} branch and every other scheme falls through to raise ValueError('unsupported SDP_LOG_SINK_URL scheme'); tests/test_observability.py (commit aec9570f 'test(observability): cover log-sink export + header extraction') covers the export branches, and the trusted Coverage execution evidence reports Result: PASS on the head; source-line-sha256=34578c2b7df85bbbf2d6b1328cf96c184c2ebe16916bc9520c7a67c524249695","outcome":"falsified"}],"residual_risk":"Bounded. urllib redirect-following from an already-compromised http(s) JWKS/log-sink endpoint could in principle traverse to another scheme, but both URLs are operator-controlled configuration rather than request input, so the guard achieves its stated misconfiguration threat model (an attacker able to alter the configured endpoint could already mint JWKS keys); http (not https-only) remains allow-listed as a deliberate operator compatibility choice — a TLS hardening note, not a regression introduced here. The tail of tests/fuzz/run_atheris.sh could not be fully re-read from the truncated hunks, but commit b4254a90 and the README changes corroborate the documented base64+sha256+replay logging, and no failed checks or unresolved threads remain on the head."}- Result: APPROVE
- Reason: CI-unblock PR verified end-to-end: JWKS URL scheme allow-list closes file:// disclosure, fuzz OOM reset bounds append-only logs, nosemgrep suppressions are source-consistent (allow-listed psycopg Composed + bound params), coverage evidence PASS on the head, no failed checks, no unresolved threads.
- Head SHA:
6263c0ccb5677df5ac009aae859d183db3c23a75 - Workflow run: 30717979266
- Workflow attempt: 1
Superseded automated OpenCode approval whose explicit review evidence does not match exact current head 03f0225; a fresh current-head review is required.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tests/test_browse.py (1)
70-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
customer_email컬럼이 없을 때 다른 검증으로 조용히 전환됩니다.Line 75에서
customer_email컬럼이 fixture 스키마에 없으면, 테스트는browse.preview를 호출하지 않고apply_mask를 직접 호출하는 대체 경로로 전환됩니다(Line 76-81). 이 대체 경로는browse.preview의 마스킹 통합 동작을 검증하지 않으므로, fixture 데이터가 변경되면 이 테스트는 통합 경로를 검증하지 않은 채 조용히 통과합니다.컬럼 존재를 사전 조건으로 assert하거나
pytest.skip으로 명시하여, 통합 경로가 실제로 실행되는지 항상 확인하십시오.♻️ 제안된 수정
def test_preview_masks_pii_columns() -> None: """Any column policy obligates as masked is redacted to '***' in returned rows.""" # Mark an email-bearing column PII so policy obligates masking for the preview rows. base = catalog._DATA["crm-customer-master"] schema = [c.model_copy(update={"pii": (c.name == "customer_email")}) for c in base.schema] - if not any(c.name == "customer_email" for c in schema): - # Fall back to masking apply_mask directly if the fixture lacks the column. - assert browse.apply_mask({"customer_email": "x@y.z", "keep": "v"}, ["customer_email"]) == { - "customer_email": "***", - "keep": "v", - } - return + assert any(c.name == "customer_email" for c in schema), ( + "fixture must include customer_email to exercise the preview masking integration" + ) catalog._DATA["crm-customer-master"] = base.model_copy(update={"schema": schema}) result = browse.preview("crm-customer-master", user="admin", purpose="analysis", limit=2) assert "customer_email" in result["masking_summary"]["masked_columns"] assert all(row["customer_email"] == "***" for row in result["rows"])🤖 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_browse.py` around lines 70 - 86, Update test_preview_masks_pii_columns so a missing customer_email fixture column is handled explicitly with a prerequisite assertion or pytest.skip, rather than calling apply_mask directly and returning. Ensure the test always either executes browse.preview for the integration path or clearly reports that the fixture cannot support it.tests/test_semantic_validation.py (1)
22-35: 📐 Maintainability & Code Quality | 🔵 Trivial
_isolate_state픽스처가 여러 파일에 중복되어 있습니다.
tests/test_browse.py,tests/test_catalog.py,tests/test_connectors.py,tests/test_orchestrator.py,tests/test_policy.py에도 유사한 픽스처가 있습니다. 하단의 통합 코멘트를 참고하십시오.🤖 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_semantic_validation.py` around lines 22 - 35, 중복된 _isolate_state 픽스처를 공용 테스트 설정으로 통합하십시오. catalog._DATA, catalog._AUDIT_LOG, evidence._POLICY_DECISION_LOG의 상태를 저장·복원하는 구현을 tests/conftest.py의 단일 autouse 픽스처로 옮기고, test_semantic_validation.py 및 언급된 다른 테스트 파일의 중복 정의를 제거하십시오.tests/test_catalog.py (1)
26-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win동일한
_isolate_state픽스처가 6개 파일에 중복되어 있습니다.각 파일이
catalog._DATA,catalog._AUDIT_LOG,evidence._POLICY_DECISION_LOG(그리고test_catalog.py의 경우catalog._SCHEMA_HISTORY)를 스냅샷/복원하는 거의 동일한 autouse 픽스처를 독립적으로 정의합니다. 공유tests/conftest.py로 추출하면 격리 계약을 한 곳에서 관리할 수 있고, 향후 새 인메모리 저장소가 추가될 때 6개 파일을 모두 수정할 필요가 없습니다.
tests/test_catalog.py#L26-L44: 이 버전(가장 완전한 형태,_SCHEMA_HISTORY포함)을tests/conftest.py의 공유 픽스처로 승격하십시오.tests/test_browse.py#L23-L38: 로컬 정의를 제거하고 공유 픽스처를 사용하십시오.tests/test_connectors.py#L32-L46: 로컬 정의를 제거하고 공유 픽스처를 사용하십시오.tests/test_orchestrator.py#L27-L41: 로컬 정의를 제거하고 공유 픽스처를 사용하십시오.tests/test_policy.py#L23-L34: 로컬 정의를 제거하고 공유 픽스처를 사용하십시오(이 파일은_AUDIT_LOG복원이 빠져 있으므로, 공유 픽스처로 통합 시 일관성도 확보됩니다).tests/test_semantic_validation.py#L22-L36: 로컬 정의를 제거하고 공유 픽스처를 사용하십시오.🤖 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_catalog.py` around lines 26 - 44, 중복된 _isolate_state autouse 픽스처를 공유 테스트 픽스처로 통합하십시오. tests/test_catalog.py#L26-L44의 전체 catalog._DATA, catalog._AUDIT_LOG, catalog._SCHEMA_HISTORY 및 evidence._POLICY_DECISION_LOG 스냅샷/복원 구현을 tests/conftest.py에 승격하고, tests/test_browse.py#L23-L38, tests/test_connectors.py#L32-L46, tests/test_orchestrator.py#L27-L41, tests/test_policy.py#L23-L34, tests/test_semantic_validation.py#L22-L36의 로컬 _isolate_state 정의를 제거하여 공유 픽스처를 사용하게 하십시오. 특히 test_policy.py의 누락된 _AUDIT_LOG 복원도 공유 구현으로 일관되게 처리하십시오.
🤖 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/fuzz/README.md`:
- Around line 52-61: Update the README’s reproducer and artifact documentation
to cover crash-*, oom-*, and timeout-* files consistently. Adjust the run output
and CI artifact descriptions in the relevant sections, including
tests/fuzz/run_atheris.sh collection behavior and the workflow’s uploaded
patterns, without changing the fuzzing implementation.
In `@tests/fuzz/run_atheris.sh`:
- Line 56: Update the reproduction command emitted by run_atheris.sh to use
REPO_ROOT-derived absolute paths for PYTHONPATH and the harness, and
shell-escape the harness path so it remains a single argument when the checkout
path contains spaces. Preserve the existing base64 heredoc reproduction flow.
In `@tests/test_authz.py`:
- Around line 42-47: Update the test around _load_jwks_from_url to remove
SDP_OIDC_JWKS_TIMEOUT_SECONDS from the environment before invoking it, ensuring
the captured timeout assertion validates the default 2.0 value regardless of CI
configuration.
- Around line 12-17: Update test_load_jwks_from_url_rejects_non_http_schemes to
mock the network urlopen call with a failing stub, then assert it was not called
for each invalid URL while preserving the existing ValueError assertions. Ensure
the test verifies rejection occurs before any network or local-file access.
In `@tests/test_catalog.py`:
- Around line 240-253: Update the DatasetPatchRequest model to declare the
optional schema field using the same schema type expected by patch_dataset, so
DatasetPatchRequest(**payload) preserves schema updates. Then revise
test_patch_dataset_schema_bumps_schema_version to instantiate
DatasetPatchRequest directly instead of the local _SchemaPatch subclass, while
retaining the existing schema_version and column-name assertions.
In `@tests/test_credentials.py`:
- Around line 28-39: Update
test_secret_status_reports_presence_without_exposing_value so the absent-secret
assertion directly verifies that the public_dict() representation does not
expose the token string; remove the always-true "secret_ref" alternative from
the or condition while preserving the existing presence and raw-secret checks.
---
Nitpick comments:
In `@tests/test_browse.py`:
- Around line 70-86: Update test_preview_masks_pii_columns so a missing
customer_email fixture column is handled explicitly with a prerequisite
assertion or pytest.skip, rather than calling apply_mask directly and returning.
Ensure the test always either executes browse.preview for the integration path
or clearly reports that the fixture cannot support it.
In `@tests/test_catalog.py`:
- Around line 26-44: 중복된 _isolate_state autouse 픽스처를 공유 테스트 픽스처로 통합하십시오.
tests/test_catalog.py#L26-L44의 전체 catalog._DATA, catalog._AUDIT_LOG,
catalog._SCHEMA_HISTORY 및 evidence._POLICY_DECISION_LOG 스냅샷/복원 구현을
tests/conftest.py에 승격하고, tests/test_browse.py#L23-L38,
tests/test_connectors.py#L32-L46, tests/test_orchestrator.py#L27-L41,
tests/test_policy.py#L23-L34, tests/test_semantic_validation.py#L22-L36의 로컬
_isolate_state 정의를 제거하여 공유 픽스처를 사용하게 하십시오. 특히 test_policy.py의 누락된 _AUDIT_LOG 복원도
공유 구현으로 일관되게 처리하십시오.
In `@tests/test_semantic_validation.py`:
- Around line 22-35: 중복된 _isolate_state 픽스처를 공용 테스트 설정으로 통합하십시오. catalog._DATA,
catalog._AUDIT_LOG, evidence._POLICY_DECISION_LOG의 상태를 저장·복원하는 구현을
tests/conftest.py의 단일 autouse 픽스처로 옮기고, test_semantic_validation.py 및 언급된 다른 테스트
파일의 중복 정의를 제거하십시오.
🪄 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: d929feff-e7e0-458f-8473-46ae9d0b4fe7
📒 Files selected for processing (19)
src/sdp/authz.pysrc/sdp/demo_smoke.pysrc/sdp/graph_store.pysrc/sdp/observability.pytests/fuzz/README.mdtests/fuzz/invariants.pytests/fuzz/run_atheris.shtests/test_authz.pytests/test_browse.pytests/test_catalog.pytests/test_config.pytests/test_connectors.pytests/test_credentials.pytests/test_demo_smoke.pytests/test_embeddings.pytests/test_observability.pytests/test_orchestrator.pytests/test_policy.pytests/test_semantic_validation.py
Empty commit; no content change. Fires the synchronize event so the required review workflows re-run and the OpenCode review is re-dispatched for this head after the review-backend outage earlier today. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QpbqAeggRho3fQA1gn34UY
Empty commit; no content change. The previous synchronize event was emitted during the org-wide Actions stall and may not have been processed; this re-queues the required review pipeline for this head. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QpbqAeggRho3fQA1gn34UY
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head bounded evidence and found no blocking issues.
Findings
No blocking findings.
Summary
APPROVE for PR #34 (fix(sdp): unblock CI - fuzz OOM + crash-log visibility + base SAST remediation). Changed files inspected: src/sdp/authz.py, src/sdp_core/contracts.py, src/sdp/observability.py, src/sdp/graph_store.py, src/sdp/demo_smoke.py, tests/fuzz/invariants.py, tests/fuzz/run_atheris.sh, tests/fuzz/README.md and 12 new tests/test_*.py files (reviewed via the Focused changed hunks, CodeGraph evidence, per-file history, and check/coverage receipts; direct PR-head reads were denied by the sandbox and are cited as a source limitation). Approval sufficiency: the only functional trust-boundary change (JWKS URL http/https allow-list at src/sdp/authz.py) closes file:// local-file disclosure through urlopen and is pinned by tests/test_authz.py (per-file history 492ea6e prove rejected JWKS schemes never fetch). Verification posture: Coverage execution evidence reports PASS for python3 -m pytest tests; Failed GitHub Check evidence records no failed checks for head 2ba30b9; mergeStateStatus blocked is branch policy, not a conflict (not DIRTY/CONFLICTING). Linter/static: nosemgrep suppressions at src/sdp/authz.py:127, src/sdp/graph_store.py:478 and src/sdp/observability.py:139 carry audited rule IDs with guard-verified justifications; no lint failure evidence. TDD/regression: tests/test_catalog.py:290 exercises DatasetPatchRequest.schema through the public PATCH contract (per-file history c257ae4); authz scheme rejection is asserted at tests/test_authz.py:175. Coverage: Coverage execution evidence PASS - supported repository test suites passed. Docstring coverage: configured repository docstring gates passed or docstring coverage was advisory per Coverage execution evidence. DAG: head-flow Mermaid diagram in the review body maps the _load_jwks_from_url allow-list guard to urlopen, DatasetPatchRequest.schema to the PATCH handler, reset_in_memory_state to the fuzz loop, and run_atheris.sh to log-visible reproducers; base-to-head security-positive delta. PoC/execution: only trusted workflow execution receipts exist (coverage PASS); no browser tool receipt applies to this non-web CLI/API repository. DDD/domain: the schema patch field keeps the dataset domain contract aligned with the resource shape; additive and optional. CDD/context: patch schema mirrors the GET dataset schema surface; no context drift found. Similar issues: current-head evidence reports no unresolved non-outdated review threads from any reviewer. Claim/concept check: the JWKS rationale (urllib honors non-http schemes such as file://) matches stdlib urlopen semantics; the fuzz OOM root cause (unbounded catalog._AUDIT_LOG / evidence._POLICY_DECISION_LOG accumulation) matches the reset_in_memory_state docstring at tests/fuzz/invariants.py:13. Standards search: the OIDC JWKS fetch is operator config restricted to http/https; no external standard claim is contradicted. Compatibility/convention: the new contract field is Optional with default None so existing PATCH consumers remain backward compatible; naming review flags schema as a single-word SQL-reserved name (P3 nit only; a two-word rename would diverge from the dataset resource field it mirrors). Breaking-change/backcompat: no public field removed or renamed. Implementation completeness: no placeholder bodies introduced; reset_in_memory_state is a concrete documented implementation. Performance: fuzz-loop memory growth is bounded by the per-iteration reset; the production hot path adds one urlsplit call. Developer experience: run_atheris.sh now prints base64 payload, SHA-256 digest and replay command for crash/oom/timeout artifacts, documented in tests/fuzz/README.md. User experience: non-web surface classified - CLI/API/log surfaces: JWKS misconfiguration now raises ValueError with the scheme hint and fuzz failures are diagnosable from logs. Visual/DOM: non-web change; the reviewed interaction surface is CLI/API/log/docs output rather than DOM. Accessibility/i18n: no UI or motion change; no a11y/i18n surface introduced. Supply-chain/license: no new dependencies (urllib.parse urlsplit is stdlib); no lockfile changes. Packaging: pyproject requires-python >=3.10 contract unchanged; fuzz tooling documented. Security/privacy: JWKS allow-list prevents file:// and other non-http(s) fetches; sink urlopen is reachable only behind the http/https guard; no secrets or enumerable identifiers are exposed by this PR.
Approval sufficiency: bounded evidence supplied affirmative approval evidence for changed files, coverage/docstring posture, risk surfaces, and current-head verification; approval is not based merely on the absence of known blockers.
Verification posture: CodeGraph evidence was initialized and bounded current-head evidence reviewed for changed-file evidence including src/sdp/authz.py, src/sdp/demo_smoke.py, src/sdp/graph_store.py, src/sdp/observability.py, src/sdp_core/contracts.py, and 15 more.
Linter/static: workflow/static review evidence is bounded by the current-head GitHub Checks gate and changed-file evidence.
TDD/regression: coverage execution evidence and focused changed hunks were reviewed from bounded-review-evidence.md.
Coverage: coverage execution evidence reports supported repository test suites passed.
Docstring coverage: coverage execution evidence reports configured repository docstring gates passed or docstring coverage was advisory.
DAG: CodeGraph/source-backed behavior map connects src/sdp/authz.py to the affected review, runtime, or workflow path and required checks.
PoC/execution: coverage-evidence job executed on the current head and reported PASS.
DDD/domain: workflow and repository-governance invariants were reviewed against changed files in bounded evidence.
CDD/context: CodeGraph evidence, changed-file history, and focused hunks were reviewed from bounded-review-evidence.md.
Similar issues: changed-file history evidence was reviewed for comparable local precedents.
Claim/concept check: bounded evidence, repository source, current-head workflow evidence, and, where numeric, scientific, statistical, or literature-backed claims are affected, original-paper/formula evidence and parameter-recovery expectations were used for claims.
Standards search: standards and external-source claims require trusted bounded source evidence prepared outside the isolated model process; no evidence-backed standards blocker is present in bounded evidence.
Compatibility/convention: changed workflow/script conventions, object naming, and reserved-word safety for schema/API/config/code surfaces were checked in bounded evidence.
Breaking-change/backcompat: deployment evidence and changed-file history were checked for backward-compatibility risk.
Performance: changed surfaces were checked for performance risk in bounded evidence.
Developer experience: changed automation, review, test, setup, and maintenance surfaces were checked for helpful or obstructive DX impact in bounded evidence.
User experience: connected user, operator, API, CLI, documentation, review-comment, status-check, rendering, and workflow-reader behavior was checked for contradictions against code, docs, and tests in bounded evidence.
Visual/DOM: deterministic repair does not infer browser runtime execution; source-backed DOM/UI evidence and trusted workflow receipts were reviewed when present, and non-web surfaces used API/CLI/log/docs/workflow evidence instead.
Accessibility/i18n: accessibility, localization, and human-readable text surfaces were checked where UI, CLI, API message, docs, logs, or review text changed.
Supply-chain/license: dependency, package, model, container, and external-tool changes were checked in bounded evidence.
Packaging: package, build, test, lint, and security contracts were checked in bounded evidence.
Security/privacy: workflow-token, review-gate, and repository-automation security/privacy boundaries were checked in bounded evidence.
Adversarial validation
{"status":"passed","probes":[{"path":"src/sdp/authz.py","line":127,"hypothesis":"A non-HTTP(S) JWKS URL such as file:///etc/passwd could reach urlopen and disclose local file contents through the OIDC JWKS fetch.","attack_or_counterexample":"Set SDP_OIDC_JWKS_URL=file:///etc/passwd and call _load_jwks_from_url to attempt a local-file read.","evidence":"Trusted focused hunk at src/sdp/authz.py:127 shows the http/https allow-list guard raising ValueError before any urlopen call; tests/test_authz.py:175 (per-file history 492ea6e5 test(authz): prove rejected JWKS schemes never fetch) asserts rejected schemes perform no fetch, and Coverage execution evidence reports PASS for python3 -m pytest tests; source-line-sha256=36c8c70dcb2fdbbe740ed506f1bf3c19fe7bf6219acf2334dc394cf89e042e99","outcome":"falsified"},{"path":"src/sdp_core/contracts.py","line":254,"hypothesis":"Adding schema: Optional[list[ColumnMetadata]] = None to DatasetPatchRequest could break the public PATCH contract (annotation/validation failure or schema payload dropped by the handler).","attack_or_counterexample":"Send PATCH /catalog/datasets/crm-customer-master with a schema list of ColumnMetadata and verify the columns persist after the update.","evidence":"Trusted focused hunk at src/sdp_core/contracts.py:254 adds the schema field; tests/test_catalog.py:290 (per-file history c257ae43 test(catalog): exercise schema patch through public contract) drives the schema patch through the public endpoint, and Coverage execution evidence PASS confirms the suite exited 0; source-line-sha256=d839763450787c11164b18d2cea740aed447a5e7e437f5bada1b571023848da3","outcome":"falsified"},{"path":"src/sdp/observability.py","line":139,"hypothesis":"The log-sink urlopen could be reached with a non-http(s) scheme, turning SDP_LOG_SINK_URL into a file:// read or an unsupported-scheme execution path.","attack_or_counterexample":"Set SDP_LOG_SINK_URL=file:///etc/passwd and attempt to export an observation through _export_to_sink.","evidence":"Trusted focused hunk at src/sdp/observability.py:139 places urlopen strictly after the file-scheme branch and the scheme in {http, https} guard, with every other scheme falling through to raise ValueError(unsupported SDP_LOG_SINK_URL scheme); tests/test_observability.py:150 (per-file history aec9570f test(observability): cover log-sink export + header extraction) exercised the sink paths, and Coverage execution evidence PASS confirms the suite ran green; source-line-sha256=34578c2b7df85bbbf2d6b1328cf96c184c2ebe16916bc9520c7a67c524249695","outcome":"falsified"}],"residual_risk":"reset_in_memory_state in tests/fuzz/invariants.py references private module attributes (catalog._AUDIT_LOG, catalog._SCHEMA_HISTORY, evidence._POLICY_DECISION_LOG, observability.reset_request_observability) that are exercised by the Atheris harness path rather than a pytest unit test; no failed fuzz checks were recorded on head 2ba30b9b, but a future module refactor could silently break the reset. The DatasetPatchRequest field name schema is a SQL-reserved single word (P3 naming nit). The nosemgrep suppressions depend on the audited Semgrep rule IDs remaining accurate. Direct PR-head file reads were unavailable in this run, so module-level assertions rely on the inlined current-head hunks, CodeGraph evidence, per-file history, and the trusted check/coverage receipts."}- Result: APPROVE
- Reason: All current-head trusted evidence is green (Coverage execution evidence PASS, no failed GitHub checks, no unresolved review threads) and the JWKS scheme allow-list, the additive DatasetPatchRequest.schema contract field, and the fuzz OOM/diagnostics fixes are source-backed with focused tests; no blocking finding could be confirmed from any adversarial probe.
- Head SHA:
2ba30b9b9792a62ab85a5e92b1bfa08dc428f488 - Workflow run: 30842342734
- Workflow attempt: 1
Superseded automated OpenCode approval whose explicit review evidence does not match exact current head 8130be1; a fresh current-head review is required.
Adopt the #34 versions of authz.py / graph_store.py / observability.py (JWKS scheme allow-list + justified nosemgrep suppressions) wholesale; this branch keeps only the orchestrator comma-join source-table fix and its regression tests as its own diff once #34 lands on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adopt the #34 versions of authz.py / graph_store.py / observability.py (JWKS scheme allow-list + justified nosemgrep suppressions) and drop the now-redundant test_load_jwks_rejects_non_http_scheme (superseded by the mocked-urlopen JWKS suite in tests/test_authz.py from #34). This branch keeps the SELECT..INTO / volatile-function gate in orchestrator.py and its regression tests as its own diff once #34 lands on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head bounded evidence and requested changes before merge.
Findings
1. P1 tests/fuzz/invariants.py:13 - reset_in_memory_state() is defined but never invoked — the fuzz-OOM fix is not wired into any harness
- Problem: PR #34 claims to unblock the fuzz gate by bounding in-memory evidence growth, but the only mechanism in the diff — reset_in_memory_state() at tests/fuzz/invariants.py lines 27-51 — has no call site at head. The authoritative current-head changed-files list contains no tests/fuzz/atheris/* harness and no tests/fuzz/test_fuzz_properties.py change, and base files cannot reference a name introduced by this PR, so catalog._AUDIT_LOG, catalog._SCHEMA_HISTORY, evidence._POLICY_DECISION_LOG, and the request-observability buffer keep growing across fuzz iterations and the libFuzzer-rss-limit oom-* abort the PR says it fixes is not actually fixed.
- Root cause: The harness wiring step is missing: the reset function was added to the invariants module with a docstring but no consumer and no unit test.
- Fix: Call invariants.reset_in_memory_state() once per fuzzer iteration inside each Atheris harness (tests/fuzz/atheris/fuzz_execute_query.py, tests/fuzz/atheris/fuzz_draft_sql.py, and the other targets) before invoking the code under test; add a unit test asserting catalog._AUDIT_LOG, catalog._SCHEMA_HISTORY, evidence._POLICY_DECISION_LOG, and the observability buffer are empty after reset.
- Regression test: python3 -m pytest tests/fuzz/test_fuzz_properties.py (plus a new reset assertion test), then PYTHONPATH=src FUZZ_SECONDS=60 tests/fuzz/run_atheris.sh
- Suggested diff: posted in this finding's inline review thread.
2. P2 tests/fuzz/README.md:63 - README claims oom-/timeout- files are uploaded as workflow artifacts, but .github/workflows/fuzz.yml is unchanged in this PR
- Problem: The README diff replaces 'A crash fails the job and uploads the crash-* artifact' with '...uploads matching crash-, oom-, and timeout-* files as workflow artifacts', while .github/workflows/fuzz.yml is absent from the authoritative changed-files list. Artifact upload is performed by the workflow via actions/upload-artifact, not by the shell runner, so the new upload claim cannot take effect unless the unchanged workflow already globs oom-* and timeout-*.
- Root cause: The documented CI artifact contract was updated without a matching workflow change.
- Fix: Add crash-, oom-, and timeout-* paths to the fuzz.yml upload-artifact step, or trim the README claim (tests/fuzz/README.md lines 61-63) to the run-log printing that tests/fuzz/run_atheris.sh actually implements.
- Regression test: Inspect .github/workflows/fuzz.yml upload-artifact patterns at head and reconcile; run a bounded fuzz job and confirm the uploaded artifact set matches the README.
- Suggested diff: posted in this finding's inline review thread.
Summary
PR #34 (head 8130be1) unblocks CI via SAST remediation + fuzz diagnostics. Verified: src/sdp/authz.py JWKS scheme allow-list blocks file:// disclosure (falsified); src/sdp_core/contracts.py:254 adds schema: Optional[list[ColumnMetadata]] with public-contract test in tests/test_catalog.py (falsified). Confirmed blocker: tests/fuzz/invariants.py:13 adds reset_in_memory_state() but the authoritative changed-files list contains no tests/fuzz/atheris/* harness or test_fuzz_properties.py change, so no execution path invokes the new name and catalog._AUDIT_LOG/evidence._POLICY_DECISION_LOG growth (the oom-* abort the PR fixes) is unaddressed. Approval sufficiency: no — confirmed implementation gap. Verification posture: Coverage execution evidence PASS (python3 -m pytest tests; coverage --fail-under=100); no failed GitHub checks at head; no OPENCODE_EXECUTION_RECEIPT for fuzz runtime. Linter/static: nosemgrep suppressions in authz.py/graph_store.py/observability.py backed by scheme allow-lists. TDD/regression: 12 new test files; missing reset-wiring test. Coverage: PASS per Coverage execution evidence. Docstring coverage: advisory (no docstring gate configured). DAG: flowchart base-to-head in review body. PoC/execution: none for the fuzz OOM path (source limitation). DDD/domain: no domain-model change beyond the additive DatasetPatchRequest schema field. CDD/context: fuzz/CI-gate context. Similar issues: historical opencode-agent approvals were dismissed; coderabbit comments historical and not current-head unresolved. Claim/concept check: PR fuzz-OOM claim not evidenced end-to-end at head. Standards search: scheme allow-listing matches file-disclosure remediation. Compatibility/convention: no new single-word identifiers at API boundaries; schema field mirrors the Dataset contract. Breaking-change/backcompat: patch contract additive. Performance: memory-bounding intended but inert. Developer experience: reproducer logging documented in README. User experience: non-web (CLI/CI/docs). Visual/DOM: N/A non-web. Accessibility/i18n: N/A. Supply-chain/license: no dependency changes. Packaging: pyproject requires-python>=3.10; workflows pin 3.12. Security/privacy: JWKS guard sound; SSRF posture limited to operator config.
Adversarial validation
{"status":"failed","probes":[{"path":"tests/fuzz/invariants.py","line":13,"hypothesis":"The new reset_in_memory_state() has no caller at head, so the fuzz-OOM fix claimed by PR #34 does not take effect and the audit/policy-decision accumulators keep growing until libFuzzer's rss limit aborts the run with an oom-* exit.","attack_or_counterexample":"A bounded/nightly Atheris run (tests/fuzz/run_atheris.sh, FUZZ_SECONDS=60 or 300) drives execute_query/draft_sql for hundreds of thousands of iterations; with no per-iteration reset, catalog._AUDIT_LOG, catalog._SCHEMA_HISTORY, evidence._POLICY_DECISION_LOG and the request-observability buffer grow unbounded — the exact OOM the PR says it fixes.","evidence":"Trusted current-head source trace: the focused diff adds only the import at tests/fuzz/invariants.py:13 plus the reset_in_memory_state() definition (lines ~27-51), and the authoritative current-head changed-files list contains no tests/fuzz/atheris/* harness and no tests/fuzz/test_fuzz_properties.py change; base files cannot reference a name introduced in this PR, so no execution path invokes the reset. source-line-sha256=b1496fb1fefd21ebfdef12cd708802a0240130fec5564c227af20bbb5f26f337","outcome":"confirmed"},{"path":"src/sdp/authz.py","line":127,"hypothesis":"A misconfigured SDP_OIDC_JWKS_URL=file:///etc/passwd (or any non-http(s) scheme) still reaches urllib and discloses local file contents.","attack_or_counterexample":"Set SDP_OIDC_JWKS_URL=file:///etc/passwd and call _load_jwks_from_url(); without a scheme guard urllib's FileHandler would open the local file.","evidence":"Trusted source trace at src/sdp/authz.py:127: urlopen is only reachable after the urlsplit(jwks_url).scheme.lower() allow-list check (_ALLOWED_JWKS_SCHEMES={https,http}) raises ValueError for scheme 'file'; current-head tests/test_authz.py (commit 492ea6e5 'test(authz): prove rejected JWKS schemes never fetch') and the Coverage execution evidence PASS (full suite, --fail-under=100) observed the rejection. source-line-sha256=36c8c70dcb2fdbbe740ed506f1bf3c19fe7bf6219acf2334dc394cf89e042e99","outcome":"falsified"},{"path":"src/sdp_core/contracts.py","line":254,"hypothesis":"PATCH /catalog/datasets/{dataset_id} with a schema payload is rejected or silently drops the column metadata because DatasetPatchRequest lacks the field.","attack_or_counterexample":"Send a PATCH with a schema list of ColumnMetadata through the public catalog API and check the stored/returned dataset preserves it.","evidence":"Trusted source line src/sdp_core/contracts.py:254 adds schema: Optional[list[ColumnMetadata]] = None to DatasetPatchRequest, and current-head tests/test_catalog.py (commit c257ae43 'test(catalog): exercise schema patch through public contract') exercises the schema patch through the public contract; the Coverage execution evidence PASS observed the full suite passing. source-line-sha256=d839763450787c11164b18d2cea740aed447a5e7e437f5bada1b571023848da3","outcome":"falsified"}],"residual_risk":"The run_atheris.sh body and .github/workflows/fuzz.yml were not in the inlined evidence (evidence truncation), so the exact upload glob and reproducer-printing implementation were not independently re-read; the confirmed blocker (unwired OOM reset) holds regardless, and the README artifact-upload claim needs reconciliation against fuzz.yml at head."}-
Result: REQUEST_CHANGES
-
Reason: P1: tests/fuzz/invariants.py adds reset_in_memory_state() with no caller in the changed set, so the fuzz-OOM fix claimed by the PR is not wired into any harness; P2: README artifact-upload claim is not backed by any fuzz.yml change.
-
Head SHA:
8130be134af77763f769bbea4262fad2e7269ea4 -
Workflow run: 31003016166
-
Workflow attempt: 1
Changed-File Evidence Map
flowchart LR
Evidence["OpenCode evidence"] --> Review["Current PR review path"]
Review --> Verify["Required checks"]
|
Exact-head verification for
Exact-head Tests |
|
@opencode-agent @cwl-noema-review @coderabbitai review Please re-review exact head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Exact HEAD =='
git rev-parse HEAD
git status --short
printf '%s\n' '== PR changed files =='
git diff --name-status "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --name-status HEAD^ HEAD
printf '%s\n' '== Diff whitespace check =='
git diff --check "$(git merge-base HEAD origin/main)" HEAD 2>/dev/null || git diff --check HEAD^ HEAD
printf '%s\n' '== Relevant file outlines =='
ast-grep outline tests/fuzz/invariants.py --items all 2>/dev/null || true
ast-grep outline src/sdp/authz.py --items all 2>/dev/null || true
printf '%s\n' '== Fuzz reset call sites and harness invocation =='
rg -n -C 5 '\b(reset_in_memory_state|check_draft_sql|check_execute_query)\b' tests/fuzz
rg -n -C 5 '\bcheck_execute_query\s*\(' tests/fuzz/atheris
printf '%s\n' '== Failure artifact upload configuration =='
rg -n -C 8 '(upload-artifact|crash-\*|oom-\*|timeout-\*|failure\(\))' .github/workflows/fuzz.yml tests/fuzz/run_atheris.sh tests/fuzz/README.md
printf '%s\n' '== JWKS scheme guard =='
rg -n -C 8 '(urlsplit|jwks|JWKS|http|https)' src/sdp/authz.py tests/test_authz.py
printf '%s\n' '== GitHub PR state and checks =='
gh pr view 34 --repo ContextualWisdomLab/semantic-data-portal \
--json headRefOid,mergeStateStatus,reviewDecision,statusCheckRollup,isDraft \
--jq '{headRefOid, mergeStateStatus, reviewDecision, isDraft, checks: [.statusCheckRollup[] | {name, status, conclusion, workflowName}]}'Length of output: 33222 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/main 2>/dev/null || git rev-parse HEAD^)"
printf '%s\n' '== Complete production and fuzz-runner diff =='
git diff --unified=80 "$base" HEAD -- \
src/sdp/authz.py \
src/sdp/demo_smoke.py \
src/sdp/graph_store.py \
src/sdp/observability.py \
src/sdp_core/contracts.py \
tests/fuzz/invariants.py \
tests/fuzz/run_atheris.sh \
tests/fuzz/README.md
printf '%s\n' '== In-memory state owners and reset API usage =='
rg -n -C 6 '(^[A-Z_][A-Z0-9_]*\s*[:=].*(\[\]|\{\})|def (audit|policy|schema|reset|clear)|append\(|\.clear\(\))' \
src/sdp tests/fuzz/invariants.py
printf '%s\n' '== Full reset function and runner failure block =='
sed -n '1,155p' tests/fuzz/invariants.py
sed -n '1,150p' tests/fuzz/run_atheris.sh
printf '%s\n' '== Changed test module structure =='
for file in \
tests/test_authz.py tests/test_browse.py tests/test_catalog.py tests/test_config.py \
tests/test_connectors.py tests/test_credentials.py tests/test_demo_smoke.py \
tests/test_embeddings.py tests/test_observability.py tests/test_orchestrator.py \
tests/test_policy.py tests/test_semantic_validation.py
do
ast-grep outline "$file" --items all 2>/dev/null || true
done
printf '%s\n' '== PR reviews and unresolved review threads =='
gh pr view 34 --repo ContextualWisdomLab/semantic-data-portal \
--json reviews,latestReviews,reviewRequests \
--jq '{reviews: [.reviews[] | {author: .author.login, state, submittedAt, body}], latestReviews: [.latestReviews[] | {author: .author.login, state, submittedAt}], reviewRequests}'Length of output: 161955
I confirmed both disputed findings are satisfied by the complete current tree:
I also reviewed the production-code changes, fuzz runner, state-reset interfaces, and changed test structure. I found no blocking issue. The exact-head checks are successful, including tests, bounded Atheris fuzzing, Semgrep, CodeQL, dependency review, OSV, Trivy, and required review workflows. The remaining [approve] You are interacting with an AI system. |
|
@opencode-agent review Please re-review exact current head |
Dismissed as factually superseded by exact-head evidence at 8130be1. The claimed unwired reset is invoked directly at the start of both check_draft_sql() and check_execute_query(), the state-mutating invariant entry points exercised by the fuzz/property harnesses; exact-head fuzz run 30849899169 is successful. The second finding is also false on the exact head: .github/workflows/fuzz.yml already uploads crash-*, oom-*, and timeout-*. Both inline threads were resolved after source-level revalidation. This dismissal removes incorrect feedback only; it is not an approval and does not bypass the independent-review requirement.
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head bounded evidence and found no blocking issues.
Findings
No blocking findings.
Summary
PR #34 (fix(sdp): unblock CI — fuzz OOM + crash-log visibility + base SAST remediation). Changed-file evidence inspected from inlined current-head hunks: src/sdp/authz.py, src/sdp_core/contracts.py, src/sdp/demo_smoke.py, src/sdp/graph_store.py, src/sdp/observability.py, tests/fuzz/README.md, tests/fuzz/invariants.py (plus 11 added tests/.py files and tests/fuzz/run_atheris.sh per Changed files). Findings: none blocking. Approval sufficiency: positive evidence only — Coverage execution evidence Result PASS (supported repository test suites passed; docstring gates passed or advisory), Failed GitHub Check evidence empty at head 8130be1, Other unresolved review thread evidence empty, and mergeStateStatus=blocked is branch-policy state, not a DIRTY/CONFLICTING merge conflict. Verification posture: source-trace + coverage-based; no OPENCODE_EXECUTION_RECEIPT runtime-tool receipts exist in bounded evidence (non-web Python repo; stated as a limitation, not a repository fact). Linter/static: bandit/pip_audit/trivy and hadolint are the configured commands; three nosemgrep suppressions were added at src/sdp/authz.py:126, src/sdp/graph_store.py:478, src/sdp/observability.py:139 with source-accurate justifications (http(s)/file scheme allow-list present before urlopen; graph_store statement is a psycopg sql.Composed of sql.Literal(graph_name)/sql.Literal(query) with the AS-column decl from a closed allow-list map and a positional bound param). TDD/regression: 11 new test files (1,380 insertions, 20 changed files) with per-file coverage-raising commits in Changed file history evidence — test_authz.py 175 lines incl. 492ea6e 'prove rejected JWKS schemes never fetch', test_catalog.py 290 lines incl. c257ae4 'exercise schema patch through public contract', plus browse masking, config KV loader, orchestrator SQL-safety, observability sink and policy deny-path tests. Coverage: Coverage execution evidence Result PASS — supported repository test suites passed; per-file history shows coverage gates reached 100% on the touched modules. Docstring coverage: configured repository docstring gates passed or were advisory per Coverage execution evidence. DAG: source-backed Mermaid flowchart in this summary mapping every changed surface to its execution path and verification path; it reflects the base-to-head changed flow (base 9a7088e -> head 8130be1). PoC/execution: no trusted execution receipts supplied, so runtime behavior rests on current-head diff/source traces plus the passing coverage evidence; this is a review source limitation, not an observed failure. DDD/domain: the OIDC/JWKS guard stays inside the sdp.authz domain module and the catalog PATCH contract change stays in sdp_core/contracts; fuzz state reset is isolated to tests/fuzz/invariants.py and never touches production paths. CDD/context: authz (JWKS fetch) and catalog (DatasetPatchRequest) contexts changed consistently with their consumers; the fuzz-reset context is harness-only. Similar issues: none in changed scope; CodeGraph blast radius shows no dependents on the changed surfaces (the flagged design_tokens.py SEMANTIC symbol is pre-existing and not part of this PR). Claim/concept check: urllib does honor file://, so the scheme allow-list is the correct, fail-closed remediation (an empty scheme also fails the allow-list and raises ValueError); the invariants.py docstring root-cause (unbounded append-only catalog._AUDIT_LOG / evidence._POLICY_DECISION_LOG growth aborting libFuzzer with an oom- exit) is consistent with the code it clears; tests/fuzz/README.md reproducer claims (crash-/oom-/timeout- files, base64 payload, SHA-256 digest, replay command printed to the log) align with the run_atheris.sh commit history (b4254a9, a71f4b4). Standards search: JWKS over http(s) is operator configuration, and the fuzz README cites Miller et al. 1990 as its oracle contract. Compatibility/convention: DatasetPatchRequest change is purely additive (Optional[list[ColumnMetadata]] = None, no field removed or retyped); the single-word API field schema matches the dataset's existing serialized contract per commit 88e4f39 'fix(catalog): preserve schema fields in patch requests', so no rename is warranted; Python snake_case and repository conventions are followed on all new identifiers (reset_in_memory_state, _ALLOWED_JWKS_SCHEMES). Breaking-change/backcompat: none — additive field only; the JWKS guard rejects only schemes that were previously unsafe; graph_store/observability runtime behavior is unchanged (comment-only edits). Implementation completeness: reset_in_memory_state() and the JWKS scheme guard are fully implemented; no pass/NotImplementedError/TODO-only branches or constant returns appear in the changed hunks. Performance: the fuzz reset bounds append-only memory growth (the OOM fix), and the JWKS guard is O(1) per fetch. Developer experience: fuzz runner now surfaces base64 payload + SHA-256 digest + shell-safe replay command for crash/oom/timeout reproducers (README + run_atheris.sh history), making CI failures diagnosable without artifact downloads; the pragma no cover comment documents the CLI entry in src/sdp/demo_smoke.py:86. User experience: CLI smoke entry (src/sdp/demo_smoke.py main()) keeps its 0/1 exit semantics; the catalog PATCH contract gains schema round-trip; no web UI surfaces changed in this PR. Visual/DOM: non-web change; the reviewed interaction surfaces are CLI exit behavior, fuzz runner log output, API JSON contract serialization, and the fuzz docs. Accessibility/i18n: no DOM/motion changes exist; demo_smoke keeps ensure_ascii=False (i18n-preserving) and there is no a11y surface to regress. Supply-chain/license: no dependency, lockfile, or manifest changes in this PR; pip_audit and trivy remain the configured supply-chain gates. Packaging: pyproject.toml is untouched, unpackaged_source_surfaces is empty, and the python test contract (python3 -m pytest tests, Python >=3.10) is present. Security/privacy: the JWKS scheme allow-list closes the urllib file:// local-disclosure vector at src/sdp/authz.py:127; the observability sink and graph_store suppressions are source-justified (allow-listed schemes/columns, bound params); no secrets, credentials, or identifier-enumeration exposure changes were introduced. Mermaid DAG: flowchart TD A["src/sdp/authz.py JWKS scheme guard"] --> B["_load_jwks_from_url urlsplit allow-list"] --> C["urlopen http(s) only, file:// rejected"] --> D["tests/test_authz.py rejected-scheme tests"]; E["src/sdp_core/contracts.py DatasetPatchRequest.schema"] --> F["PATCH /catalog/datasets/{id} handler"] --> G["tests/test_catalog.py public-contract schema patch"]; H["tests/fuzz/invariants.py reset_in_memory_state"] --> I["fuzz harnesses check_draft_sql / check_execute_query"] --> J["bounded RSS, no fuzz OOM"]; K["tests/fuzz/run_atheris.sh reproducer logging"] --> L["crash-/oom-/timeout- artifact + base64 + SHA-256 + replay command"].
Approval sufficiency: bounded evidence supplied affirmative approval evidence for changed files, coverage/docstring posture, risk surfaces, and current-head verification; approval is not based merely on the absence of known blockers.
Verification posture: CodeGraph evidence was initialized and bounded current-head evidence reviewed for changed-file evidence including src/sdp/authz.py, src/sdp/demo_smoke.py, src/sdp/graph_store.py, src/sdp/observability.py, src/sdp_core/contracts.py, and 15 more.
Linter/static: workflow/static review evidence is bounded by the current-head GitHub Checks gate and changed-file evidence.
TDD/regression: coverage execution evidence and focused changed hunks were reviewed from bounded-review-evidence.md.
Coverage: coverage execution evidence reports supported repository test suites passed.
Docstring coverage: coverage execution evidence reports configured repository docstring gates passed or docstring coverage was advisory.
DAG: CodeGraph/source-backed behavior map connects src/sdp/authz.py to the affected review, runtime, or workflow path and required checks.
PoC/execution: coverage-evidence job executed on the current head and reported PASS.
DDD/domain: workflow and repository-governance invariants were reviewed against changed files in bounded evidence.
CDD/context: CodeGraph evidence, changed-file history, and focused hunks were reviewed from bounded-review-evidence.md.
Similar issues: changed-file history evidence was reviewed for comparable local precedents.
Claim/concept check: bounded evidence, repository source, current-head workflow evidence, and, where numeric, scientific, statistical, or literature-backed claims are affected, original-paper/formula evidence and parameter-recovery expectations were used for claims.
Standards search: standards and external-source claims require trusted bounded source evidence prepared outside the isolated model process; no evidence-backed standards blocker is present in bounded evidence.
Compatibility/convention: changed workflow/script conventions, object naming, and reserved-word safety for schema/API/config/code surfaces were checked in bounded evidence.
Breaking-change/backcompat: deployment evidence and changed-file history were checked for backward-compatibility risk.
Performance: changed surfaces were checked for performance risk in bounded evidence.
Developer experience: changed automation, review, test, setup, and maintenance surfaces were checked for helpful or obstructive DX impact in bounded evidence.
User experience: connected user, operator, API, CLI, documentation, review-comment, status-check, rendering, and workflow-reader behavior was checked for contradictions against code, docs, and tests in bounded evidence.
Visual/DOM: deterministic repair does not infer browser runtime execution; source-backed DOM/UI evidence and trusted workflow receipts were reviewed when present, and non-web surfaces used API/CLI/log/docs/workflow evidence instead.
Accessibility/i18n: accessibility, localization, and human-readable text surfaces were checked where UI, CLI, API message, docs, logs, or review text changed.
Supply-chain/license: dependency, package, model, container, and external-tool changes were checked in bounded evidence.
Packaging: package, build, test, lint, and security contracts were checked in bounded evidence.
Security/privacy: workflow-token, review-gate, and repository-automation security/privacy boundaries were checked in bounded evidence.
Adversarial validation
{"status":"passed","probes":[{"path":"src/sdp/authz.py","line":127,"hypothesis":"A misconfigured SDP_OIDC_JWKS_URL using a file:// or other non-HTTP(S) scheme would be passed straight to urllib's urlopen and disclose local file contents (local file disclosure).","attack_or_counterexample":"SDP_OIDC_JWKS_URL=file:///etc/passwd and empty-scheme variants fed to _load_jwks_from_url.","evidence":"Trusted current-head diff at src/sdp/authz.py:127 shows the urlopen invocation is now reachable only after the new allow-list guard (scheme = urlsplit(jwks_url).scheme.lower(); raise ValueError when not in {http, https}) so file:// is rejected before any fetch; tests/test_authz.py (175 added lines, commit 492ea6e5 'test(authz): prove rejected JWKS schemes never fetch') and Coverage execution evidence Result PASS (supported repository test suites passed) corroborate the rejection. source-line-sha256=36c8c70dcb2fdbbe740ed506f1bf3c19fe7bf6219acf2334dc394cf89e042e99","outcome":"falsified"},{"path":"src/sdp_core/contracts.py","line":254,"hypothesis":"Adding `schema` to DatasetPatchRequest would break the PATCH contract (serialization/name-shadowing) or be silently ignored by the catalog PATCH handler, so the documented schema patch would not persist.","attack_or_counterexample":"PATCH /catalog/datasets/crm-customer-master with a body containing a schema array of ColumnMetadata objects.","evidence":"Trusted current-head diff at src/sdp_core/contracts.py:254 adds schema: Optional[list[ColumnMetadata]] = None as a purely additive optional field (no existing field removed or retyped, so the request model stays backward compatible); commit c257ae43 'test(catalog): exercise schema patch through public contract' added tests/test_catalog.py (290 lines) exercising the schema patch through the public contract, and Coverage execution evidence Result PASS (supported repository test suites passed) shows the field parses and the PATCH path round-trips without rejection or silent drop. source-line-sha256=d839763450787c11164b18d2cea740aed447a5e7e437f5bada1b571023848da3","outcome":"falsified"}],"residual_risk":"The inlined evidence packet truncated the focused hunks for the tail of tests/fuzz/invariants.py, tests/fuzz/run_atheris.sh, and the 11 added test files, and direct source reads were denied by the sandbox, so the call-site wiring of reset_in_memory_state() inside check_draft_sql()/check_execute_query() and the exact run_atheris.sh logging lines were established via current-head Changed file history and the maintainer's exact-head verification (which the workflow receipts at tests/fuzz/invariants.py:13 and :129 corroborate as existing current-head lines) rather than direct hunks; the previous DISMISSED 'never invoked' thread is not unresolved at current head. No failed GitHub checks, no unresolved review threads, and no merge conflict exist at head 8130be134af77763f769bbea4262fad2e7269ea4."}- Result: APPROVE
- Reason: Approved — the JWKS fetch scheme allow-list closes urllib file:// local-disclosure, DatasetPatchRequest gains a purely additive optional schema field with public-contract tests, the fuzz OOM reset and reproducer logging are in place, and current-head Coverage execution evidence is PASS with no failed checks and no unresolved review threads.
- Head SHA:
8130be134af77763f769bbea4262fad2e7269ea4 - Workflow run: 31325677453
- Workflow attempt: 1
Unblocks semantic-data-portal's required security/fuzz gates, which were failing on the base tree independent of any diff, and adds the diagnostics that surfaced the root cause. Three related commits:
1. Surface Atheris crash reproducers in the run log
run_atheris.shwrote crashing inputs tocrash-*/oom-*/timeout-*files and only uploaded them as a workflow artifact, so a fuzz failure was undiagnosable without downloading that artifact. Now, on a target crash, the runner prints the new reproducer (base64 + size + sha256 + replay command, attributed to the crashing target) to the log. Verified:bash -nclean; a simulated crash emits only the new reproducer and the base64 round-trips byte-for-byte.This immediately paid off — it revealed the long-standing fuzz failure was an OOM in the
execute_querytarget (exit 71, emptyoom-*reproducer = cumulative growth), not thevalidate_sql_querylogic I'd first suspected.2. Fix the
execute_queryOOM (bound in-memory evidence growth)execute_query/draft_sqlrecord a policy decision + audit event on every call into the module-level append-only listsevidence._POLICY_DECISION_LOG/catalog._AUDIT_LOG. The pytest suite resets these via an autouse fixture; the fuzz harnesses had none, so a bounded-time run (100k+ iterations) grew them past libFuzzer's rss limit. Addedinvariants.reset_in_memory_state()(called in the two evidence-recording checks), preserving seeded catalog data. Verified: with the reset, 8000 iterations leave the logs at 1 entry each; without it, 2000 iterations grow the policy log to 2000. Hypothesis property suite still passes (10/10).3. Remediate base-branch Semgrep SAST findings (Medium+)
The repo-wide
Semgrep (multi-language SAST)gate reported 3 findings on the base tree:_load_jwks_from_urlpassed the operator-configuredSDP_OIDC_JWKS_URLstraight tourlopen(honoursfile://). Added an http(s) scheme allow-list (real hardening) + scoped# nosemgrepfor the audit-only rule.{http,https}immediately above theurlopen. Scoped, documented# nosemgrep.sql.Composedofsql.Literal(...)values with the AS-column decl from a closed allow-list map and params bound positionally — not raw string SQL. Scoped, documented# nosemgrep.Verified: scheme guard blocks
file:///ftp:///gopher://, allows http(s); a local Semgrep run with the exact check-ids confirms all 3 lines move tosuppressions(0 active findings), which the gate counts as suppressed rather than failing.Net effect
Both the Atheris (OOM) and Semgrep (SAST) gates — which block every sdp PR — turn green, and the fuzz harness now self-diagnoses future crashes in the log.
Summary by CodeRabbit
보안 개선
http및https스킴만 허용해 안전하지 않은 URL 사용을 차단했습니다.테스트 및 안정성