qa: walk gate tri-state verdicts (GREEN/RED/ERROR), provenance stamps, door-cross camera re-assert#1613
Conversation
…, door-cross camera re-assert
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 38 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: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughWalk testing now distinguishes walkability failures from harness defects, records provenance and diagnostics, captures door-cross pose observations, returns tri-state exit codes, and maps harness-only errors to skipped room results. Confidence: 97%. ChangesWalkability verdict classification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant run_gate
participant DebugEndpoint
participant classify_verdict
participant stage_walk
run_gate->>DebugEndpoint: request /debug during walk checks
DebugEndpoint-->>run_gate: pose data or harness error
run_gate->>classify_verdict: classify collected report
classify_verdict-->>run_gate: GREEN, RED, or ERROR
stage_walk->>stage_walk: convert harness ERROR to SKIP
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
evaOS review status: completedPR: #1613 - qa: walk gate tri-state verdicts (GREEN/RED/ERROR), provenance stamps, door-cross camera re-assert evaOS review completed for this PR head. Automation note: agents should wait for this comment to reach PR URL: #1613 Review URL: #1613 (review) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@qa/room_pipeline.py`:
- Around line 93-97: Add a full-mode regression test in the room pipeline test
suite covering coherence=GREEN with walk=SKIP, and assert that the result is not
shippable or certified. Ensure the test exercises the ERROR-to-SKIP mapping in
the report handling flow so an unavailable walk gate cannot certify the room.
In `@qa/walk_test.py`:
- Around line 564-567: Update qa/walk_test.py lines 564-567 in _drive_and_check
to return the existing drive-error sentinel when every post-click combat-surface
status poll fails, while preserving normal timeout handling when any poll
responds. At lines 583-587, retain and use the sentinel skip path so unavailable
polling cannot pass impassable checks. At lines 600-605, make failed door or
return-leg polling produce explicit harness-error details rather than an
ordinary door result, and add tests distinguishing responsive timeouts from
complete transport outages.
- Around line 402-405: Update the pose-capture logic around crossed and the
return loop so destination snapshots are recorded only after confirming the
party is at target, not merely because it left home. Likewise, record the home
pose only after confirmed return; a return timeout must produce a door failure
instead of comparing the stranded room. Add mocked coverage for wrong-target
arrival and return timeout.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: ce61bea2-935f-4830-b1e4-7e96a623d768
📒 Files selected for processing (3)
qa/room_pipeline.pyqa/test_walk_test.pyqa/walk_test.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: viewer-tests
- GitHub Check: test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: viewer-tests
- GitHub Check: test
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Treat/Users/lume/WorldOSas the canonical local Mac app/private-art checkout for WorldOS GUI and native-app testing.
Use/Volumes/LEXAR/Codexfor Codex artifacts, scratch files, screenshots, reports, and downloaded CI/VM artifacts.
Before install, build, or test commands, verifypwd; GUI/native app runs must use/Users/lume/WorldOSor an approved same-disk worktree configuration.
Use thecodex/prefix for new branches unless instructed otherwise.
Files:
qa/room_pipeline.pyqa/walk_test.pyqa/test_walk_test.py
🪛 ast-grep (0.44.1)
qa/walk_test.py
[error] 307-308: Command coming from incoming request
Context: subprocess.run(["git", "-C", str(REPO), "rev-parse", "--short", "HEAD"],
capture_output=True, text=True, timeout=10)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
qa/test_walk_test.py
[warning] 214-214: Do not make http calls without encryption
Context: "http://engine:8766"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 214-214: Do not make http calls without encryption
Context: "http://qa:8971"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 217-217: Do not make http calls without encryption
Context: "http://engine:8766"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
[warning] 217-217: Do not make http calls without encryption
Context: "http://qa:8971"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🪛 Ruff (0.15.21)
qa/walk_test.py
[warning] 305-305: Missing return type annotation for private function _repo_sha
(ANN202)
[error] 308-308: subprocess call: check for execution of untrusted input
(S603)
[warning] 308-308: subprocess.run without explicit check argument
Add explicit check=False
(PLW1510)
[error] 308-308: Starting a process with a partial executable path
(S607)
[warning] 315-315: Missing return type annotation for private function _manifest_sha256
(ANN202)
[warning] 377-377: Missing return type annotation for private function _check_door_cross
(ANN202)
[error] 419-420: try-except-pass detected, consider logging the exception
(S110)
[warning] 446-446: Missing return type annotation for private function _room_ortho_opt
(ANN202)
qa/test_walk_test.py
[warning] 157-157: Missing return type annotation for private function _base_report
(ANN202)
[warning] 157-157: Missing type annotation for **over
(ANN003)
[warning] 218-218: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 219-219: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 220-220: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 221-221: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 222-222: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 231-231: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 240-240: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 245-245: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 252-252: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🔇 Additional comments (2)
qa/walk_test.py (1)
245-348: LGTM!Also applies to: 368-374, 446-456, 530-542, 650-650, 734-750
qa/test_walk_test.py (1)
154-261: LGTM!
![evaos-code-review-bot[bot]](https://avatars.githubusercontent.com/in/4184532?s=60&v=4){kind=small}
There was a problem hiding this comment.
Walkthrough
PR: #1613 - qa: walk gate tri-state verdicts (GREEN/RED/ERROR), provenance stamps, door-cross camera re-assert
Head: aca6e162541af14600742055d95ffdb13c5a2d75 into main. Review event: COMMENT.
Provider: GLM/Z.ai through ZCode (zcode-glm, zcode, model GLM-5.2).
Estimated review effort: 3/5 (~36 min)
Changed Files
| File | Status | Churn | Purpose | Risk |
|---|---|---|---|---|
qa/room_pipeline.py |
modified | +8/-0 | Changed file | Moderate: validated P2 finding |
qa/test_walk_test.py |
modified | +108/-0 | Test coverage | Low |
qa/walk_test.py |
modified | +185/-20 | Changed file | Elevated: validated P1 finding |
Review Signal
Validated inline findings: 3 (P0: 0, P1: 1, P2: 1, P3: 1).
Dropped findings before posting: 1. High-severity findings: 1.
Risk Taxonomy
- Runtime correctness: 3
Validation and Proof
1 required validation/proof recommendation(s) selected from changed files.
- required: Unity editor or Play Mode smoke - WorldOS repo profile implies Unity runtime risk. Proof: Unity editor smoke; Play Mode log; scene/prefab screenshot or recording.
Proof status: missing - 1 required validation/proof recommendation(s) missing from PR metadata.
Profile validation hints: Prefer correctness, persistence, CI, release, and regression findings over style-only feedback.
Profile proof expectations: Look for Unity editor, play-mode, fixture, or focused smoke evidence when runtime behavior changes.
Related Context
Related issues/PRs: #1607.
Suggested labels: bug, tests.
Suggested reviewers: none from current metadata.
Review Settings Preview
- Profile: assertive
- Enabled sections: Review summary (inline_review); Walkthrough (inline_review); Changed-files table (walkthrough); Effort estimate (walkthrough); Related issues/PRs (walkthrough); Suggested labels (suggestion_only); Review status comment (sticky_status)
- Path instructions:
Assets/**- Prioritize scene, prefab, save-state, asset-reference, and gameplay regressions. - Path instructions:
ProjectSettings/**- Treat build, platform, input, graphics, and release behavior changes as high risk. - Label suggestions: unity, gameplay, regression-hardening
- Reviewer suggestions: none
- Suggestion behavior: suggestions only; labels and reviewers are not auto-applied.
- Roadmap-only settings: auto-apply labels; auto-request reviewers; required status checks
Pre-merge checklist
- Inline comments target current RIGHT-side diff lines.
- No secret-like content survived into posted inline comments.
- REQUEST_CHANGES is only used when eligible P0/P1 findings survive validation.
- Required behavior proof is present or not applicable.
- Labels and reviewers are suggestions only; the bot did not auto-apply them.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aca6e16254
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… pose, poll-outage harness, .get()-safe verdict, fire-VFX blob masking
Additional fix (live-run finding, no review comment id): animated-fire-VFX blob masking (#1525)The first walk gates on the generated Additive fix in
🤖 Addressed by Claude Code |
evaOS review status: closed or merged before reviewPR: #1613 - qa: walk gate tri-state verdicts (GREEN/RED/ERROR), provenance stamps, door-cross camera re-assert evaOS review stopped because the PR closed or merged before this queued head could be reviewed. Automation note: agents should wait for this comment to reach PR URL: #1613 Details: state=closed |
All 3 findings fixed + verified at 92edf8d (50 tests): _is_shippable requires a GREEN walk verdict (SKIP/ERROR can never certify; resume re-runs cached SKIP); door-cross poses captured only on confirmed legs; total poll outages return the harness sentinel. Threads resolved with per-fix replies.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92edf8d78c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(detail.get("error"), str) and detail["error"].startswith("click:"): | ||
| report["harness_errors"].append(f"door {list(cell)}: {detail['error']}") | ||
| continue |
There was a problem hiding this comment.
When the engine drops after a door click, _check_door_cross() still swallows /combat-surface polling exceptions and returns ok=False with crossed_to=None, not a click: error. This new harness branch only catches click exceptions, so the following door counter records a walkability failure and classify_verdict() emits RED even though the tri-state contract says player/engine outages should be ERROR; propagate poll failures from the door-cross path into harness_errors.
Useful? React with 👍 / 👎.
| or report.get("doors", {}).get("fail") | ||
| or report.get("orphans") | ||
| or report.get("path", {}).get("fail") | ||
| or report.get("visual", {}).get("fail") |
There was a problem hiding this comment.
When --visual N is enabled and the QA visual harness fails (for example /health or /shot is unavailable, or the visual move returns drive-error:), _visual_registration() produces no cases or ok=False cases and run_gate() increments report['visual']['fail'] without adding a harness error. Because this line treats every visual failure as a walkability failure, screenshot/QA outages can be reported as RED room defects instead of ERROR; split visual transport/capture failures into harness_errors before counting genuine registration misses.
Useful? React with 👍 / 👎.
| # fire-anchor cells (brazier/hearth VFX) from the room geometry and/or the surface props — used to | ||
| # mask animated-flame diff blobs in visual mode and to prefer fire-distant visual sample cells. | ||
| fire_cells = fire_anchor_cells(_load_room_geometry(room)) | fire_anchor_cells(surf) | ||
| report = _init_report(room, ortho, scene, mask, engine, qa) |
There was a problem hiding this comment.
When the engine is down before the first /combat-surface read, run_gate() raises before this report is initialized, so qa/walk_test.py never reaches classify_verdict() or writes an ERROR/2 report. The pipeline wrapper converts that exception to SKIP, but the documented walk gate CLI still exits via traceback for a harness outage; catch the initial surface probe and add it to harness_errors so direct gate runs get the new tri-state semantics.
Useful? React with 👍 / 👎.
| if p.get("kind") in FIRE_KINDS: | ||
| for cr in p.get("cells", []): | ||
| out.add((int(cr[0]), int(cr[1]))) |
There was a problem hiding this comment.
For rooms like camp_clearing_night, the animated fire is declared as a manifest effects entry and the live scene-grid prop kind is campfire_pit; other existing scene grids use kinds such as stone_hearth / brazier_r. This exact-kind check therefore returns no fire cells for those current fire sources, so --visual still samples/matches against flame flicker and can report the false visual RED this masking path was added to prevent; include the manifest effects and/or match the existing fire-bearing kind variants.
Useful? React with 👍 / 👎.
1 participant
Why
An adversarial review of the walkability gate (
qa/walk_test.py) flagged one real risk class: a harness/infrastructure defect must never read as a walkability verdict — in either direction. A player/engine outage was scoring as a RED room defect (blaming a healthy room), and a missing/debugcamera extension was folded into the same hard-fail bucket as a genuine pose mismatch. Both directions are wrong: infra noise that reads as RED wastes a re-authoring cycle on a good room; and — the more dangerous inverse the gate must structurally prevent — anything that lets a broken room slip through green.This change is additive: a healthy run's GREEN behavior is byte-for-byte unchanged (no drive-errors, no harness gaps → identical counts → GREEN/0). Every reclassification only fires on a path that a healthy run never takes.
What
1. Tri-state semantics.
classify_verdict(report) -> (verdict, exit_code)is now the single pure decision point:drive-error:in_drive_and_check)/debugmissing camera fields entirely / unreachable/clickthrew; door-cross/debugunreachablereport["harness_errors"]: list[str]collects the harness class. Exit codes: 0 GREEN / 1 RED / 2 ERROR. The visual fail-on-zero-measurable-cases and impassable timeout-counts-as-fail semantics stay walkability (they guard vacuous greens); only actual exceptions become harness. Where a category was ambiguous, it is classified so it can never certify a broken room green.2. Provenance stamps in the report (→
walk_report.json):schema_version: 1,repo_sha,manifest_sha256,ts(UTC ISO8601),engine_url,qa_url. Certification (#1607) pins artifact shas at cert level; the report self-describing its provenance closes the traceability loop. Factored into_init_report(...)for offline unit-testing.3. Door-cross camera re-assert.
_check_door_crossre-fetches/debugafter cross-arrival and re-runscheck_camera_poseagainst the DESTINATION room's pinned ortho, and again against HOME's ortho after the return leg. A pose mismatch there = RED (the camera-contract regression the gate exists to catch);/debugunreachable there = harness. Existing door pass/fail counting preserved; pose results added into the door case records.qa/room_pipeline.py—stage_walkmaps anERRORverdict to statusSKIPwith detail"harness error — retry/investigate (never a verdict): <first errors>". SKIP already blocks shippable (correct: a broken harness never reads as RED and never certifies the room).Tests
qa/test_walk_test.pyextended (+21 tests, 30 total, offline/no live services): tri-state verdict + exit classification incl. the real-fail-wins-over-harness case; provenance keys via_init_report; and the pure pose-mismatch-vs-harness classifiers (classify_camera_fails,classify_pose_observation,is_drive_error).Run:
uv run --directory servers/engine --group dev python -m pytest ../../qa/test_walk_test.py -q -p no:xdist→ 30 passed.Do NOT merge — for orchestrator review.