Repository navigation
Require stream-outage coverage and simplify credential test assertions - #3880
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. What this changesThis PR requires the existing stream-outage regression in CI and shares equivalent deferred-promise and refused-receipt test helpers. Example: The case “exchange bounds unavailable stream policy checks without extending authorization” disappears from the peer suite.
Review scores
ProductKind: Maintenance · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep open: this maintainer-authored PR closes a concrete CI coverage gap and preserves existing test assertions and scheduling; no actionable defect was found. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe CI manifest selects credential-proxy integration tests, and the runner compares their reported outcomes with required case names; shared helpers coordinate tests and check refused receipts. flowchart TB
Manifest[CI expected cases] --> Runner[CI test runner]
Runner --> Tests[Credential integration tests]
Helpers[Shared test helpers] --> Tests
Tests --> Results[Test outcomes]
Results --> Gate[Required case validation]
Technical reviewBest possible solution: Land the focused CI expectation and behavior-preserving helper extraction through the normal review process. Do we have a high-confidence way to reproduce the issue? Source inspection confirms that current main omits the existing outage case from expectedTests and that the runner enforces named expectations; no target code was executed. Is this the best way to solve the issue? The existing CI manifest is the correct owner for required-case enforcement, and the extracted helpers preserve assertion strength and asynchronous scheduling. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against dde0d51badfa. Provenance checked
TestingProof path: in-process harness. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes:
Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment Reviewed October 27, 2026, 1:12 PM ET / 17:12 UTC. |
freeqaz
left a comment
There was a problem hiding this comment.
Free's clanker here (an AI agent working on Free's behalf). Comment-only review of 5a71f19af2.
Summary: test-only, and coverage is not weaker. Nothing blocks landing.
- Expected tests:
exchange bounds unavailable stream policy checks without extending authorizationexists atcredential-proxy-peer-pin.test.ts:274. It is now inchecks-baseline-1'sexpectedTests, so the suite fails if that test is skipped or missing. This adds coverage. assertRefusedReceipt: all four replaced blocks assert the same three things as before:authorization === null,settled === true, and a deep-equalrefused/dispatched: falseoutcome. Each call site still passes its own error.tests/helpers/deferred.ts: the code is identical to the two local copies it replaces. It is a helper, not a test file, so it needs no suite mapping.- Verification:
credential-proxy-peer-pin.test.tspassed 154/154 with 0 skipped on a remote test VM. CI Required is green on this head (the PostgreSQL file runs there). The PR merges cleanly with main and with #3879, which also editschecks-baseline-1.json.
The peer suite ran the test that bounds stream renewal during a policy outage, but CI’s expected-test list omitted it. CI now checks that the named case runs and passes. The PostgreSQL cases share their receipt assertions; the peer and PostgreSQL tests also share a small deferred-promise helper. The original awaits and case-specific expected errors remain at the call sites.
All 154 peer cases and 35 PostgreSQL cases passed without skips. The CI runner’s targeted test also verified that it rejects a missing or skipped expected case.