Repository navigation
test(zcode): automate CLI acceptance and skill behavior evaluation - #1850
jackie-cqz wants to merge 5 commits into
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") 的测试。
Teingi
left a comment
There was a problem hiding this comment.
One P2 in the baseline qualification logic.
| 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.
[P2] Treat confirmed baseline rejections as behavior failures
A no-Skill turn can finish normally after ZCode rejects an invalid search_memory({}) request before execution. scheduled() includes that structured request, but there is correctly no corresponding MCP call. The resulting native_wire_mismatch is classified as execution-incomplete here, so the entire run becomes unqualified even when every with-Skill case passes. This contradicts the documented rule that baseline behavior failures do not fail the gate.
I reproduced this with a complete synthetic paired archive: adding only the baseline's rejected request and native error event, while retaining turn.completed, changes qualified from true to false. The pinned ZCode executor returns input-validation failures to the model before invoking the handler; this was a replay test, not a live-model run. Please distinguish a recorded native rejection from missing execution evidence, and classify the former as a completed behavior failure.
There was a problem hiding this comment.
At a228b1c2, the original {} rejection case is fixed, but this P2 remains for non-object arguments.
For a native search_memory request with input: [], the pinned ZCode host preserves the array in model.streaming and returns an input-schema rejection. Even with that matching error event and turn.completed, scheduled() rejects the input before rejected_inputs() can inspect the rejection.
I reproduced this with a complete synthetic paired archive: {} yields status=failed, execution_complete=true, and qualified=true; replacing only the input with [] or a string raises ValueError: invalid_native_tool_event. This aborts report generation instead of recording a completed baseline behavior failure. The event shape was checked against the pinned host source; this is a replay reproduction, not a live-model run.
Please allow non-object arguments with a confirmed native schema rejection to reach the behavior-failure classification, while continuing to reject missing or contradictory execution evidence. Both the scheduled-event and stream-only paths currently enforce the dictionary check.
| 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. |
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/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.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?
node --test integrations/zcode/plugins/powercontext/tests/*.test.mjs: 22 passed, including actual CLI plugin discovery.python -m evaluation.zcode_guidance.pin --check: packaged Skill pin verified.python -m evaluation.zcode_guidance.run --help: live runner entry-point and shared import smoke check passed.python -m pytest evaluation/zcode_guidance/tests -q: 36 passed on both 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: 42 passed on Python 3.11 and 3.12. These include explicit response-gate and real subprocess EOF/privacy regressions.git diff --check, andactionlint .github/workflows/zcode-acceptance.ymlpassed. Checked the artifact collector against completed local runs to confirm its allowlist excludes private files.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.