ci(noema): keep sanitized sidecar evidence, including discovery timing, on successful runs - #2326
seonghobae wants to merge 2 commits into
Conversation
The noema-sidecar-evidence upload ran only under failure(), so every successful Noema run discarded the sanitized sidecar stderr that already carries the DEBUG http_request/discovery_complete lines from pin 767e67fb. Match Strix's always() gate; files, pin, name and retention are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LeLbwEgYXzCzpaFMTHDcBS
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSidecar 증거 업로드 조건이 ChangesSidecar 증거 및 discovery 타이밍
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The sanitizer dropped model_discovery.py's discovery_result (DEBUG, per-provider elapsed_ms) and discovery_complete (INFO) lines, so the Noema sidecar artifact could not split pre-healthz startup into discovery and preflight. Allowlist both with the timestamp, keeping only the provider name and bounded numbers, and correct the CHANGELOG claim that discovery_complete already reached the sanitized file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LeLbwEgYXzCzpaFMTHDcBS
seonghobae
changed the title
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
[central CI evidence contract — exact a403d73b034e3eeaf7b89dd4487ad32f1fa4761c]
failure()→always() 방향은 성공 실행의 진단 증거를 보존한다는 목적과 맞습니다. 다만 이 exact head는 아직 “successful Noema run keeps sidecar evidence”를 fail-closed contract로 만들지 못했습니다.
핵심은 upload step이 계속 if-no-files-found: ignore라는 점입니다. sidecar stderr/preflight 생성 경로가 rename/path/logging regression으로 조용히 사라져도 Noema verdict가 성공하면 upload step도 성공하고 artifact가 0개인 채 required workflow가 GREEN이 될 수 있습니다. 현재 test도 YAML에 always()가 있는지만 확인하므로 이 failure mode를 잡지 못합니다. 이번 PR의 buyer-visible 목적 자체가 evidence retention인 만큼, 성공 verdict + admitted sidecar 경로에서는 두 파일(또는 명시된 required subset)의 존재/비어 있지 않음/expected schema를 먼저 검증하고 누락 시 evidence gate를 실패시켜야 합니다. sidecar가 애초에 실행되지 않는 명시적 context가 있다면 그 context만 별도 branch로 ignore를 허용하십시오.
Realistic RED: otherwise-successful Noema fixture에서 stderr 또는 preflight 파일을 하나씩 제거/잘못된 경로로 이동시키고 현재 workflow가 녹색으로 지나가는 것을 재현 → GREEN에서 evidence-presence contract가 nonzero로 막아야 합니다. Hosted acceptance도 이 exact generation의 실제 successful run에서 noema-sidecar-evidence artifact가 존재하고 두 payload를 포함하는지 확인해야 합니다. 단순 unit/YAML assertion으로는 ‘kept’가 증명되지 않습니다.
또한 body의 “Exposure is unchanged”는 data category 관점에서는 Strix/failure path와 같을 수 있지만 exposure frequency는 failure-only에서 every outcome으로 넓어집니다. 따라서 sanitizer allowlist test에 gateway/provider secret, Authorization/header, URL/query, prompt/user payload, arbitrary exception text 같은 negative controls를 유지하고, artifact visibility/retention(5 days)을 SECURITY/THREAT_MODEL 또는 해당 workflow contract에 명시해 주세요. request_id/provider account label을 보존하는 것이 의도된 telemetry인지도 contract에 고정하는 편이 좋습니다.
현재 hosted exact-head runs(35611933746, 35611934408, 35611934414, 35611933749, 35611933715)은 pending/queued라 release evidence도 아직 없습니다.
판정: direction PASS candidate / success-path evidence-presence FAIL / exposure-doctoring PENDING / exact-head hosted acceptance PENDING.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head product diff. Coverage is a separate gate.
Changed files
.github/workflows/noema-review.yml— GitHub Actions review jobCHANGELOG.md— repository behaviorscripts/ci/sanitize_contextual_orchestrator_sidecar_stream.py— review and security gate shell pathtests/test_contextual_orchestrator_review_runtime_preflight.py— regression suitetests/test_noema_orchestrator_workflow_contract.py— regression suite
Changed behavior
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: noema-review.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: noema-review.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Repository file: CHANGELOG.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: CHANGELOG.md"]
R2 --> V2["required checks"]
Evidence --> S3["CI script: sanitize_contextual_orchestrator_sidecar_stream.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: sanitize_contextual_orchestrator_sidecar_stream.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_contextual_orchestrator_review_runtime_preflight.py (2 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_contextual_orchestrator_review_runtime_preflight.py (2 files)"]
R4 --> V4["targeted test run"]
Findings
No source-backed product finding is synthesized from the coverage gate. A coverage miss belongs in the status comment.
- Head SHA:
a403d73b034e3eeaf7b89dd4487ad32f1fa4761c - Workflow run: 35663552145
- Workflow attempt: 1
- Coverage gate:
failure
Review outcome
Coverage is a gate, not the review. This body reviews the changed product files.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Workflow: noema-review.yml"]
S1 --> I1["GitHub Actions review job"]
I1 --> R1["Review risk: Workflow: noema-review.yml"]
R1 --> V1["actionlint plus required checks"]
Evidence --> S2["Repository file: CHANGELOG.md"]
S2 --> I2["repository behavior"]
I2 --> R2["Review risk: Repository file: CHANGELOG.md"]
R2 --> V2["required checks"]
Evidence --> S3["CI script: sanitize_contextual_orchestrator_sidecar_stream.py"]
S3 --> I3["review and security gate shell path"]
I3 --> R3["Review risk: CI script: sanitize_contextual_orchestrator_sidecar_stream.py"]
R3 --> V3["bash -n plus Strix self-test"]
Evidence --> S4["Test: test_contextual_orchestrator_review_runtime_preflight.py (2 files)"]
S4 --> I4["regression suite"]
I4 --> R4["Review risk: Test: test_contextual_orchestrator_review_runtime_preflight.py (2 files)"]
R4 --> V4["targeted test run"]
OpenCode Review Overview
Coverage evidence did not pass, so approval is blocked. The formal pull-request review is the source-backed diff review, not this status comment. |
|
Admission correction — exact current head |
This branch has not been deployed
1 participant
Problem
In
noema-review.yml, the "Upload contextual-orchestrator sidecar evidence" step ran only underif: failure() && env.PR_NUMBER != ''. Every successful Noema run therefore discarded the sanitized sidecar stderr. The vendored pin767e67fbandscripts/ci/contextual_orchestrator_review_launcher.py(_configure_sidecar_logging, DEBUG by default) already emit thehttp_requestanddiscovery_completelines, so the missing piece is the upload. No CO pin bump or--log-levelchange is needed. contextual-orchestrator#1218 does not change the deployed Noema path.Change
a403d73balso allowlists two sanitizer lines:discovery_result account=<provider> model_count=N elapsed_ms=X(DEBUG) anddiscovery_complete providers=N models=N errors=N(INFO) frommodel_discovery.pyat pin 767e67fb. They are kept with the timestamp, and only the provider name and bounded numbers survive, so pre-healthz startup can be split into discovery and preflight. Scope was agreed with the contextual-orchestrator lead, who is reviewing independently.if: always() && env.PR_NUMBER != '', the same gate Strix uses forstrix-reports. The step name drops "on failure".strix_runs/contextual-orchestrator-sidecar.stderr.log,…-preflight.json), artifact namenoema-sidecar-evidence, the pinnedactions/upload-artifact@043fb46d…,if-no-files-found: ignore, 5-day retention, and the step position (after "Prepare Noema model verdict", before the publication token refresh).test_noema_review_uploads_sidecar_evidence_on_every_outcome, assertsalways(), and asserts thatfailure()no longer appears. One CHANGELOG entry is added.Evidence
pytest tests/test_noema_orchestrator_workflow_contract.py: 12 passed.noema-review.yml(runner image, required inventory, ruleset audit, queue contract, docs-only admission, token lifetime, gate, and more): 293 passed, 2 skipped.tests/test_pr_review_merge_scheduler.py -k noema: 1 passed.test_sidecar_stream_sanitizer_admits_discovery_timing_events: fails on the previous sanitizer, passes now.tests/test_contextual_orchestrator_review_runtime_preflight.py: 136 passed. The 13 test files that reference the sanitizer ornoema-review.yml: 773 passed, 2 skipped. Sanitizer module coverage and interrogate are both 100%.actions_queue_health*,noema_review_document, …). Hosted CI decides.Conflicts
29443f41) touches onlyscripts/ci/contextual_orchestrator_review_sidecar.shand its contract test (git diff --stat origin/main...29443f41). There is no file overlap with this PR.Trust boundary
This is a
pull_request_targetrequired workflow, so the new condition takes effect for consumer PRs only after merge, on the base branch.🤖 Generated with Claude Code
https://claude.ai/code/session_01LeLbwEgYXzCzpaFMTHDcBS
Summary by CodeRabbit
개선 사항
문서