Repository navigation
test(conformance): enable shared compaction scenario - #5123
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
9ecef00 to
a4c50c5
Compare
svcnvidia-nemo-ci
left a comment
There was a problem hiding this comment.
SHIP WITH CARE — two RISK findings in reporting error paths.
Reviewed all 11 changed files and their affected evidence-checking, provider/runtime, gating, and CI-rendering paths. No BLOCKER found. This was a static review: tests were inspected, not executed, and the reported native OpenCode rollout was not independently reproduced.
Review generated by codex. This was a full review. Request a full review with /review reset=true.
| result = inspect_episode(scenario, directory, execution) | ||
| except (OSError, ValueError, TypeError, KeyError, AttributeError, RecursionError) as exc: | ||
| execution_error = True | ||
| execution_error |= scenario.tier <= gate_tier |
There was a problem hiding this comment.
RISK — A non-gating P1 checker error invalidates the retained P0 measurement.
If the eight P0 scenarios pass but inspecting context_compaction raises ValueError—for example, from malformed rollout JSON—the new tier condition permits exit 0 under the default gate. However, the fallback scenario lacks checks. validate_row unconditionally reads that field, so render_ci marks the harness's otherwise valid P0 measurement Unavailable. Expected: retain P0 Passed and report P1 failure separately.
Could you make the fallback dictionary renderable by adding the following entry beside checker_error?
"checks": [],This restores the invariant that every serialized scenario has a check collection. An empty collection honestly records that inspection produced no checks; the fallback verdict still fails P1, and the matrix renderer already displays absent check results as failures. Please verify this suggested change before adopting it.
Proposed acceptance checks: simulate an inspection exception only for compaction in an otherwise passing full suite. Default gating should return 0, and rendering its summary plus CI result should show Passed | 8/9 | Failed, not Unavailable. Preserve exit 2/unavailable for a P0 checker error and for the same compaction checker error with gate_tier="P1". Retain the all-harness regression test's distinction between ordinary missing compaction evidence: code 0 under P0 versus code 1 under P1. These checks were not executed.
| ] | ||
| lines.append(f"| `{harness}` | {label} | {count} |") | ||
| p1 = (row or {}).get("tier_verdicts", {}).get("P1") | ||
| p1_label = {"fulfilled": "Passed", "not_fulfilled": "Failed", None: "Not measured"}[p1] |
There was a problem hiding this comment.
RISK — Malformed tier metadata aborts publication of every harness measurement.
A retained summary containing tier_verdicts={"P1":"invalid"} passes validate_row, then raises KeyError here. tier_verdicts=null or a list raises AttributeError on the preceding line. These reads occur after render_ci's per-harness exception boundary, so one malformed artifact prevents the entire table from being published instead of isolating that harness as unavailable.
Could you validate the optional metadata in validate_row before display lookups? A small addition is:
tiers = row.get("tier_verdicts", {})
if not isinstance(tiers, dict) or any(
key not in ("P0", "P1", "P2")
or value not in ("fulfilled", "not_fulfilled")
for key, value in tiers.items()
):
raise ValueError("invalid tier verdicts in documentation results")Also move render_table's existing row-validation loop before its display loop. This preserves the invariant that malformed tier metadata is rejected before consumption and lets render_ci's existing ValueError handling isolate the affected row. Please verify this suggested code before adopting it.
Proposed acceptance checks: give one harness an invalid P1 string, null, and a list as tier_verdicts; each should render only that harness as unavailable while retaining another valid harness's measurements. Preserve summaries omitting the optional field (P1: Not measured) and the existing compaction-report test's Passed | 8/9 | Failed assertion. These checks were not executed.
a4c50c5 to
e576e72
Compare
e576e72 to
fc23626
Compare
fc23626 to
9618409
Compare
9618409 to
40562f8
Compare
Scenario continuation: #5439. Reordering the stack made this PR's head reachable from its former Pi base, so GitHub automatically marked it merged. The scenario did not land on main. Prior discussion is retained here; the original implementation stack now ends with #5439, and #5435 → #5436 form a separate Pi follow-up.
Register one shared
context_compactionscenario after the collector, shared checker/provider support and harness fixes. This remains the last PR in the stack, following the shared fixture correction #5435 and Pi fix #5436. Earlier layers retain the eight-scenario CI workload. This completes the scenario introduced by #5307; no separate issue is needed.The task runs fourteen real shell actions. Small context/threshold settings exercise each harness's automatic compaction; Pi additionally receives enough history and a controlled context-limit error during the task to exercise compact-and-retry. Every selected harness faces the same five checks: completed event, summary reuse in the next policy request, helper references and ownership, exclusion from policy turns, and retained usage. SimpleAgent remains unchanged.
Validation on the assembled stack:
pytest -q scripts/harness_conformance/tests tests/unit_tests/harness_capabilities: 439 passed, 2 skipped.pre-commit run --all-files: passed.--gate-tier P1. Its compaction rollout retains 17 captured calls, 15 policy turns and one helper, with summary reuse and complete usage.Reproduce with the selected harness dependencies installed:
Default CI reports all nine scenarios and retains the eight-scenario P0 gate. Explicit P1 gating also requires compaction, so SimpleAgent fails accurately. Usage is synthetic fixture input; compression quality, before/after counts and boundary references are outside these five checks. Runs use real pinned runtimes and a controlled local provider; remote model/sandbox qualification and full repository CI were not run.