Repository navigation
test(zcode): automate CLI acceptance and skill behavior evaluation - #1850
Conversation
| run: | | ||
| uv run --locked --no-sync python -m evaluation.zcode_guidance.pin --check | ||
| if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } | ||
| uv run --locked --no-sync python -m pytest evaluation/zcode_guidance/tests -q |
There was a problem hiding this comment.
[P2] 新增门禁未覆盖 evaluation.zcode_guidance.run，跨树导入失效不会被任何检查发现
本步骤名为「Validate the ZCode Skill pin and behavior evaluation harness」，但只运行 pin --check 与 pytest evaluation/zcode_guidance/tests。evaluation/zcode_guidance/tests/test_guidance.py:25-27 只导入 fixture / pin / report，从不导入 run；而 evaluation/zcode_guidance/run.py:32-33 依赖两个跨树符号：tests.e2e.zcode_acceptance.host.NativeHost、tests.e2e.zcode_acceptance.runner.{error_code, serve}。
pyproject.toml:193 已把 evaluation 排除出 ty check 作用域（实测：无参数 ty check 通过，显式 ty check evaluation 报 357 条诊断），ruff 也不做跨模块符号解析。因此 tests/e2e/zcode_acceptance/ 内的重命名会让 run.py 静默失效：既不会被新增的本 workflow 发现，也不会被 make quality（Makefile:32 的 uv run ty check）发现。而 run.py 正是 evaluation/zcode_guidance/README.md 中产出 live 报告的入口，report.py 的 replay() 又要求 model_mode=live 的产物。
建议：在本步骤追加一行导入冒烟，例如
uv run --locked --no-sync python -c "import evaluation.zcode_guidance.run"，
或在 test_guidance.py 中加一条 importlib.import_module("evaluation.zcode_guidance.run") 的测试。
| complete = not any( | ||
| failure in incomplete | ||
| or re.fullmatch( | ||
| r"turn_\d+_(incomplete|response_unverified|model_unobserved|native_wire_mismatch)", failure |
There was a problem hiding this comment.
[P1] The four regex-matched failure codes have no test coverage, so a future refactor can silently turn "execution incomplete" into "execution failed"
complete is decided by two paths: a fixed set membership test and a regex over the failure name.
complete = not any(
failure in incomplete
or re.fullmatch(
r"turn_\d+_(incomplete|response_unverified|model_unobserved|native_wire_mismatch)", failure
)
for failure in failures
)All four codes are reachable from grade() — turn_{index+1}_incomplete (:75), _response_unverified (:77), _model_unobserved (:87), _native_wire_mismatch (:102) — and none of them appear in the incomplete set (:284-291), so the regex is the only thing standing between them and complete=True.
I removed each alternative from that pattern one at a time and reran the suite:
[incomplete] -> 20 passed
[response_unverified] -> 20 passed
[model_unobserved] -> 20 passed
[native_wire_mismatch] -> 20 passed
The gate itself does not flip: qualified still ends up False, because :317's all(item["execution_complete"] ...) covers both arms and native_wire_mismatch (which is also in the incomplete set) still fails that arm independently. So this is not a "gate can be bypassed today" finding.
What is a problem is the reporting consequence. Once any one of those four is weakened, the affected outcome's status becomes failed instead of incomplete (:304), while README.md states that an incomplete baseline execution fails the gate. A reader of the report can then read "the run executed completely and failed" when the truth is that it never finished — which is precisely the distinction a grader exists to draw. The contrast with the neighbouring set path is what makes it look like an oversight: deleting execution_failed from incomplete does fail test_completed_turns_do_not_hide_execution_teardown_failure, so that path is guarded and this one is not.
Suggested direction: add one offline test per alternative — construct a case whose only failure is turn_1_model_unobserved, then assert results[i]["status"] == "incomplete" and qualified is False. A more durable fix is to have grade() return structured failures with a kind field instead of encoding the class in a string prefix, so replay() switches on a value rather than a pattern.
There was a problem hiding this comment.
Confirmed fixed, independently rather than taken from the commit message.
Re-ran the same mutation against de18005d — dropping each alternative from the execution_complete pattern one at a time and running the suite:
drop incomplete -> 1 failed, 35 passed
drop response_unverified -> 1 failed, 35 passed
drop model_unobserved -> 1 failed, 35 passed
drop native_wire_mismatch -> 10 failed, 26 passed
At 4df02e32 all four mutations passed 20 passed. The assertion that catches it is exactly the one this review asked for:
> assert result["status"] == "incomplete"
E AssertionError: assert 'failed' == 'incomplete'
FAILED .../test_guidance.py::test_baseline_execution_evidence_failures_keep_replay_incomplete[empty-search-model_unobserved-turn_2_model_unobserved]
So the report can no longer present an incomplete execution as a complete one that merely failed, and the parametrization covers each code individually.
Also noted in a228b1c2: rejected_inputs() now separates a pinned-host input-schema rejection from an unfinished execution, and the wire comparison skips rejected calls so a request the host refused before the handler no longer becomes native_wire_mismatch. The condition len(observed) != 1 (a started handler, a second terminal event, or contradictory evidence all keep the wire requirement) looks like the right conservatism. @frf12 reported this against report.py:296 in discussion_r4181281010; I have not re-derived that one independently beyond reading the branch, but this thread's own finding is closed.
|
@Teingi please review again. thx. |
hidb4ai
left a comment
hidb4ai
left a comment
There was a problem hiding this comment.
One remaining P2 in the handling of native input-schema rejections; details inline.
| if not identity or not isinstance(payload.get("input"), dict) or not payload.get("toolName"): | ||
| raise ValueError("invalid_native_tool_event") |
There was a problem hiding this comment.
[P2] Preserve non-object inputs until native rejection classification
A completed host-side schema rejection still aborts the entire report when the tool input is an array. Reproduced at ccf79c13 using native_rejected_search() in an otherwise complete archive, changing only its streamed input to [] in the without_skill arm. Despite the matching Tool input failed inputSchema validation event and turn.completed, replay() raises ValueError("invalid_native_tool_event") here before rejected_inputs() runs. The pinned ZCode host preserves array inputs and rejects them before invoking the MCP handler, so no wire call is expected.
This also prevents the live runner from writing report.json. The documented outcome is a completed behavior failure that does not disqualify an otherwise passing Skill arm, as already happens for {}. Preserve non-object arguments long enough to correlate the rejection, including the equivalent check below, while keeping missing or contradictory rejection evidence incomplete. Please add the array-input case to the replay regression.
Which issue or RFC does this PR close?
Related to #1751; follows the ZCode workflows and acceptance work in #1814.
Rationale for this change
Keep the existing ZCode acceptance scenarios reproducible as PowerContext contracts and the supported host evolve. Add a separate live-model Skill evaluation that checks tool routing and authorization boundaries without confusing controlled MCP replies with backend qualification.
What changes are included in this PR?
29628c9acdb81b703bbd4080c207a0e7ce5e276e(CLI 0.16.9), run all plugin Node tests and two independent controlled-model acceptances against a real PowerContext Server.evaluation/skills/skill-up. Cover ordinary coding without tools, explicit-save success, empty search without query expansion, rejected writes without a success claim, and stale candidate approval without an unauthorized retry.20583250; retain legacy live results as historical evidence, without claiming current-pin live qualification.Are there any user-facing changes?
New contributor validation commands and documentation. Public APIs, persisted formats and installed plugin behavior are unchanged.
CI uses controlled inference with a real Server. Skill model runs use live inference with controlled MCP replies; they do not establish backend persistence, memory quality or official Windows desktop acceptance. Live Skill evaluation remains an explicit run with an already configured model.
How was this change tested?
20583250, preserving the current evaluation index and the ZCode suite entry. The merge also includes the current Atomic Memory API and Skill changes.python -m pytest evaluation/zcode_guidance/tests -q: 61 passed on Python 3.11 and 3.12.python -m pytest tests/test_zcode_acceptance_protocol.py tests/test_zcode_acceptance_host.py evaluation/zcode_guidance/tests -q: 69 passed on Python 3.11 and 3.12. This includes native schema-rejection, incomplete-evidence, subprocess EOF/privacy, capture-response gating, and current structured extraction/reconciliation regressions.node --test integrations/zcode/plugins/powercontext/tests/*.test.mjs: 22 passed, including actual CLI plugin discovery.python -m evaluation.zcode_guidance.pin --check: current packaged Skill pin20583250verified.python -m evaluation.zcode_guidance.run --help: live runner entry point and shared imports passed without calling a model.git diff --check, andactionlint .github/workflows/zcode-acceptance.ymlpassed. Checked the artifact collector against completed local runs to confirm its allowlist excludes private files.5cf66be6292eba4ad6e05a4e24e409ebfe1da22aand the legacy collection/entry Memory contract. Both passing arms establish no improvement claim. Original reports and source snapshots are retained privately; the current grader rejects legacy Memory archives withunsupported_memory_contract.AI usage statement
GPT-6 AI assistance was used for implementation, test development and documentation. The changes were reviewed and validated with focused automated checks and retained native host/MCP evidence.