fix(ecc): stop recording failed RCX extraction as Success - #263
Merged
Merged
Conversation
Emin017
force-pushed
the
emin/fix-rcx-false-success
branch
from
September 7, 2026 09:37
336d2be to
70b2431
Compare
Signed-off-by: Emin <me@emin.chat>
A failed iRCX run could still be recorded as a successful flow step: native init_rcx()/run_rcx() return values were dropped, SPEF publication silently no-op'd, and the RCX output verdict treated an empty spef list as success. - run_rcx propagates native init_rcx()/run_rcx() False returns and marks the run_rcx substep Incomplete, mirroring the run_sta failure paths - destroy_rcx() is attempted on every path once init_rcx() is entered - stale spef_writer/*.spef artifacts are wiped before extraction so a rerun after a past success cannot glob stale outputs - copy_rcx_spef_outputs returns a boolean and publishes all-or-nothing: no fresh SPEF, a missing spef_writer dir, or a missing/zero-byte published SPEF fails - check_step_result requires a non-empty spef list with every entry an existing, non-zero-byte file Refs #229
Require every selected SPEF source to exist and be non-empty before copying anything to the step output, so a partial set or a stale destination from an earlier run can no longer pass as fresh extraction output. Treat any run_step() return other than True or StateEnum.Success as a step failure instead of recording Success.
execute_tool_step only logged the run_step() return value, so a runner that returned False (or None or any non-success StateEnum) after publishing valid outputs was still recorded as Success - the RCX save-data failure paths hit exactly this hole. - execute_tool_step sets step_error unless run_step returns True or StateEnum.Success (the ecc_sizer runner returns StateEnum), naming the raw returned value; the existing flow terminal path then records the step as Imcomplete - copy_rcx_spef_outputs now validates the whole selected source set (regular, non-zero-byte) before creating or overwriting any destination, so a missing or empty source can no longer be masked by a stale destination from an earlier run, and publication is all-or-nothing - engine tests: parametrized over all seven rejected return values plus True/StateEnum.Success positives (incl. the sizer Timing optimization contract) using real output files, the real verdict, persisted ledger assertions, and the exact on_step_completed error - runner tests: mixed valid/missing and valid/empty source sets fail without partial copies, stale destinations are left untouched, and preflight failure preserves the live SPEF list Refs #229
A copy2 failure mid-publication left previously written destinations on disk, so a failed publish could still leave a partial SPEF set visible as current extraction output. Publication now removes every destination written by the failed pass before reporting failure, following the all-or-nothing policy used by STA artifact publication. Refs #229
Destination validation ran after the copy loop's failure handler, so a publication whose copies succeeded but whose result was missing or zero-byte returned False without removing the destinations written by the pass. Validation now raises inside the same failure-handling block, and every destination written by this invocation is unlinked whenever publication fails for any reason. Refs #229
copy2 can create or truncate its destination and then raise (for example on ENOSPC), and the destination was only registered for cleanup after copy2 returned, so the failed copy's partial SPEF stayed visible. Destinations are now registered for cleanup before the copy runs, and the copy-failure regression covers partial writes (empty and truncated) followed by OSError(ENOSPC). Refs #229
Publishing straight onto destinations meant a failed pass could delete or truncate the last known-good SPEF it had already overwritten. Publication now moves any existing destination to a .prev backup before replacing it through a .tmp temp file, and a failed pass restores every backup, removes destinations it created, and drops partial temp copies, leaving the step outputs exactly as before the pass. Refs #229
Per the repository's decomposition rule, path resolution, stale spef_writer cleanup, and SPEF publication move from the oversized ecc runner into chipcompiler/tools/ecc/rcx_artifacts.py, with their tests in test/tools/ecc/test_rcx_artifacts.py; run_rcx keeps orchestrating extraction. Publication now also removes the .prev backups after the output set passes validation, so a successful rerun does not accumulate a second SPEF set on disk; backup-removal failures are reported without failing the already-published step. A successful-rerun regression asserts fresh content and no temp/backup leftovers. Refs #229
Finalize-phase cleanup, functionality-equivalent only: the RCX verdict loop collapses to the guard + all() idiom used elsewhere in the codebase, and the publication selection consolidates the duplicated destination comprehension. Refs #229
Emin017
force-pushed
the
emin/fix-rcx-false-success
branch
from
September 8, 2026 09:59
70b2431 to
d5f0542
Compare
Yell-walkalone
approved these changes
Sep 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Changed
run_step()returnsTrueorStateEnum.Success, preventing failed RCX runs from being recorded asSuccess.chipcompiler/tools/ecc/rcx_artifacts.pyand add focused regressions for runner, artifact publication, and flow-state behavior..zcode/workspace directory.Refs #229
Scope
Select the areas touched by this PR:
uv.lock, or release artifacts.Runtime And Packaging Impact
ecc-toolsorecc-dreamplacedependency changedNotes:
Validation
List the commands you ran. Mark checks that are not applicable as N/A.
uv run pytest test/uv run ruff check chipcompiler testuv run ruff format --check chipcompiler testecc --help,ecc --version,ecc version --jsonnix run .#cli -- --help.venv/bin/pytest test/tools/ecc/test_runner.py test/test_engine_flow.py(72 passed);git diff --check origin/main...HEADSkipped checks and reason:
Checklist