chore(cli): cleanup docs and deadcode - #269
Merged
Merged
Conversation
Emin017
force-pushed
the
emin/update-cli-cleanup-review
branch
from
September 8, 2026 03:59
df6fb2e to
4a4650c
Compare
Emin017
force-pushed
the
emin/update-cli-docs
branch
from
September 8, 2026 04:00
1f5842a to
ecfd5cd
Compare
Emin017
force-pushed
the
emin/update-cli-cleanup-review
branch
from
September 8, 2026 04:00
4a4650c to
4d3c0c5
Compare
Emin017
added
bug
Yell-walkalone
previously approved these changes
Sep 8, 2026
auto-merge was automatically disabled
September 8, 2026 09:16
Pull Request is not mergeable
auto-merge was automatically disabled
September 8, 2026 09:19
Pull Request is not mergeable
Emin017
force-pushed
the
emin/update-cli-docs
branch
from
September 8, 2026 09:25
ecfd5cd to
e13fbd4
Compare
Emin017
force-pushed
the
emin/update-cli-cleanup-review
branch
from
September 8, 2026 09:25
c2e768a to
bd03705
Compare
auto-merge was automatically disabled
September 8, 2026 09:37
Pull Request is not mergeable
auto-merge was automatically disabled
September 8, 2026 09:38
Pull Request is not mergeable
Emin017
dismissed
Yell-walkalone’s stale review
September 8, 2026 09:40
The base branch was changed.
The record model keeps exactly one machine-readable surface: --plain (key=value). --json/--jsonl had no remaining consumer after the desktop app moved to rpc serve; ecc version keeps --json as a hidden flag for the desktop appInfoService call. Tests now assert structured results through --plain via the plain_records fixture instead of parsing JSON stdout; pure json/jsonl-surface tests are removed.
Drop --json/--jsonl from all command synopses and examples in the user guide, config reference, tutorial, and dev guide; the dev guide records the hidden ecc version --json flag reserved for the desktop app. Status and config examples now show real --plain output.
Drop the removed --json/--jsonl modes from the design spec's goals, structured-output section, capability tables, and phase checklists, and from the development quickstart and both READMEs; disclosure examples now show the inspect hints the CLI actually emits. The hidden ecc version --json desktop contract stays documented.
set_pdk_root interpolated the path straight into a TOML basic string, so directories containing a double quote or backslash produced invalid TOML. Serialize through format_toml_value and pin both replacement and table-creation branches with tomllib round-trips.
The param, pdk, and project handlers rewrote ecc.toml with a truncating
open('w'), so an interruption or write failure after truncation could
destroy the project config. Add write_text_atomic (sibling temp file,
flush/fsync, os.replace) next to the shared text editors and route all
five rewrite sites through it.
apply_config_overrides validated and persisted in the same loop, so a valid override was already written when a later unknown target or malformed patch raised. Validate and stage every merged document first, then commit the staged files.
manifest.py crossed the 700-line review threshold after the flow-range and registration work. Extract the write/mutation/registration half (build_manifest_document, write_manifest_if_absent, update_manifest, write_back_workspace_status, manifest_range_for_flow, pre_register_workspace) into chipcompiler.cli.project.manifest_write, keeping manifest.py on loading, normalization, and classification, and move the write-side tests and monkeypatch targets with the code.
The styled doc pager test left Rich's independent NO_COLOR and TERM inputs untouched, so it failed on dumb-terminal hosts even with supports_color mocked to true. The pdk setup tests mocked subprocess but not shutil.which, so hosts without make exited through missing_tool before exercising the clone/retry/proxy behavior. Pin both inputs and command discovery locally, add a missing-tool branch test, and fix a formatting drift in records.py.
Warning is a terminal state (a non-blocking synthesis LEC that did not prove equivalence lets the flow continue), but resume selection, run_only, reconciliation, and run_step skipping all compared against Success alone, so a completed flow with a warned LEC re-ran the LEC and its whole physical suffix on every bare run. Introduce one canonical finished-state predicate in data.step and use it across selectors, reconciliation, and skipping.
The step->directory mapping existed as four hand-maintained copies (checklist, signoff package collector, report extract, QoR). Move it to data.step_dirs, next to the step enums, and derive the consumers from it; report-specific GUI labels stay local to the QoR reporter.
…ing honestly Any nonempty ledger without a failure counted as a complete flow, so running or partially progressed workspaces could score Green while the status mapping's blocked branches were unreachable. Classify not_started/running/in_progress/failed/complete explicitly. Corner timing with no measured slack is now unknown instead of pass, and endpoint-count rollups stay unmeasured instead of summing absent values as zero, so missing evidence is never rendered as success.
… toml scanning Move write_text_atomic to utility.file as the canonical writer and route report output through it: a failed report write no longer destroys the existing file and surfaces as report_write_failed instead of a traceback. pdk setup now validates the candidate root with the project's configured pdk overrides resolved against that root, so a valid custom layout is not rejected and overridden paths are actually checked. The TOML editor's bracket scanner skips strings and comments, so a value like "alu[rev" no longer looks multiline and swallows following keys through the next table header.
Fresh flow ranges skipped environment preflight solely because they have no named preset, so missing tools surfaced only after manifest registration and workspace creation had started; derive the probe set from build_flow_range and preflight before any mutation. Overwrite deleted the previous workspace before building its replacement, so any mid-creation failure permanently lost the old artifacts (workspace refresh made this the primary path). Rename the old tree to a sibling backup under the lock, restore it when creation or step-workspace construction fails, and discard the backup only once the replacement is verified; a refresh whose construction stopped early now reports failure instead of an unconditional refreshed.
Workspace seeding (provenance and flow-target writes) joined the rollback-protected construction phase with its save result checked, and the running marker is written only after the replacement is committed, so no failure path strands an inconsistent manifest or a partial target. pdk setup runs make unzip only inside a git checkout, refusing unrecognized non-empty directories with a structured error. The parameter transaction aborts before mutating when a snapshot read fails, instead of rolling back to a deletion. Config overrides use strict JSON reads (a corrupt target aborts the set instead of being replaced by the patch) and commit atomically with rollback. Preflight components derive from an explicit tool-to-runtime mapping, so a Yosys-only chain preflights Yosys without requiring unrelated components. Golden-netlist persistence rejects generated-name collisions, report timing no longer invents zero violations from met slack or misread constraint aliases, and missing slack renders as unknown rather than VIOLATION.
…honest evidence The TOML value tokenizer skips inline comments inside unterminated bracket collections, set_pdk_root reuses the same multiline value-range logic, and format_toml_value escapes control characters. Workspace seeding writes run inside the recovery boundary so a failed write restores the renamed-aside previous workspace. Parameter transactions abort before mutating when a snapshotted file cannot be read, and the golden netlist is persisted after the filelist bulk copy so a generated name collision is refused instead of overwritten. QoR parsing rejects NaN/Infinity metric values, missing slack renders as unknown instead of VIOLATION, and ecc.spec documents libfontconfig as a verified host prerequisite of the bundle.
…diting Config overrides reject JSON documents whose root is not an object instead of silently overwriting them with only the patch. Report filenames sanitize design-derived names into safe basenames so a crafted design value cannot escape the signoff directory. Gate status requires every gate step before issuing a pass: partial ledgers report incomplete. pdk setup verifies checkout provenance through the git config remote before running make unzip. The clock skew extractor unpacks the source key from MetricStore.query correctly, so explicit _ns keys keep their units. Table-header discovery runs on masked text so bracket text inside multiline strings is never mistaken for a [table] header, workspace resolution rejects malformed manifests instead of falling through to <project>/default, and the run-input capability check uses isinstance instead of a class-name literal.
…llbacks A synthesis LEC that runs to completion and reports inequivalence is a non-blocking warning; infrastructure failures (missing binary, spawn error, exception) now stay Incomplete and block the flow, and the fatal error is cleared only on the warning path. When an overwrite run against an undeclared workspace fails before construction, the fresh manifest entry this invocation created is removed instead of shadowing the restored previous workspace. Key assignments in ecc.toml are located on string-masked text so key-like content inside multiline strings is never edited, inline-table keys are escaped and TOML-untypeable values are rejected before any write, and pdk root writes map I/O failures to a structured config_error instead of a traceback.
Only a terminal Warning state continues a flow: the Incomplete + non-blocking-step fallbacks in run_steps, rerun selection, and the progress renderer are gone, so a missing tool or process failure can no longer produce an overall successful run. Workspace run failures write the terminal manifest status while still holding the workspace lock. Overwrite rollback is now checked: removal/restore problems are surfaced in the error reason with the stranded backup path. Config override rollback restores via atomic replaces instead of truncating write_bytes. The coverage policy module moved out of the runtime package into the single test that consumes it, and reconciliation tests derive the canonical chain from the real builder.
The one-click installer (--with-toolchain) now provisions the PDK, so the clone + make unzip orchestration in pdk setup is a duplicate acquisition path — and the only CLI command doing network downloads. Attaching a PDK stays with ecc pdk set-root, CHIPCOMPILER_ICS55_PDK_ROOT, or the repo default; show/unset are unchanged. Also drop the pdk_root keyword from resolve_pdk_overrides: setup was its only caller.
ecc-cli-dev.{en,cn}.md document CLI extension development — contributor
material, not end-user reference. Move them back to docs/ (linked from
development.md), out of the wheel and the PyInstaller bundle, and remove
the dev topic from ecc doc.
Also restore the devcontainer CHIPCOMPILER_ICS55_PDK_ROOT env line and
clean up stale pdk setup references in README, development.md, and
cli-design.md that review of ae03f4b surfaced.
Signed-off-by: Emin <me@emin.chat>
…dition
The repo-only ecc-cli-dev.{en,cn}.md become the 'Extending The CLI'
section of the development guide; conventions already covered by
CLAUDE.md and review-guidelines.md are linked instead of repeated
(progressive disclosure). development.cn.md is the merged Chinese
counterpart, so contributor docs now have language parity with the
user guides.
The PyInstaller bundle build instructions move to the README
build-from-source section (user-facing), and the stale ~3.6G bundle
size is corrected to the measured ~0.9G.
Also drop test_workspace_cli_removed.py: it asserted on removed
workspace subcommands, which the test guidelines rule out (no negative
tests for deleted logic; refresh itself is covered by
test_workspace_refresh.py).
Follow-through on the docs cleanup: remove the stale architecture.md, drop its CLAUDE.md pointer, and point all workspace-cli.md references (including the garbled rpc-guide.md.md paths) at docs/rpc-guide.md.
Relative ../../ links in the bundled guides are dead weight for the primary audience: ecc doc renders them unclickable in a terminal, and in wheel/PyInstaller installs the targets do not exist. Links to repository files now use absolute github.com/openecos-projects/ecc URLs (pointing at the post-merge main layout), while guide-to-guide references keep their relative links for GitHub and gain an ecc doc <topic> hint for terminal readers.
ecc-cli-config -> ecc-config-ref, ecc-cli-ug -> ecc-user-guide, ecc-cli-tutorial -> ecc-tutorial. ecc doc topics stay config/ug/tutorial (short to type); the command help and the user guide now spell out that ug means user guide.
Typer lists commands in registration order; registering the doc command right after the two root-level commands puts it at position 3 instead of buried mid-list. The ug guide's help listing is updated to match.
Add the missing doc/project/workspace rows, note that the guides ship with the CLI and work offline (ecc doc ug/config/tutorial), and drop the cli-design.md spec links — the design specification is contributor material, not user-facing documentation.
…at CONTRIBUTING.md prek install alone already registers both hook stages via the config's default_install_hook_types; the explicit --hook-type ... --overwrite form repairs an existing or partial install. README contributing sections now lead with CONTRIBUTING.md (rules and review expectations) and keep the development guide for setup.
…hints Codex review follow-ups: directory links need tree/main (blob is for files), the ecc doc hints in the Chinese docs must pass --lang cn or they open the English guides, and the Chinese tutorial's development guide link now points at development.cn.md.
…esults The pre-route LEC checklist item was downgraded to policy=warn so a failed or stale proof only surfaced as an export warning. Drop the downgrade: both LEC steps now record unproven results as failed with policy=block, which blocks signoff package export like any other required check.
Remove the Warning terminal state and its non-blocking machinery (StateEnum.Warning, is_non_blocking_step, warning rendering, warning run status): an unproven synthesis LEC is persisted as Incomplete and stops the flow like any other failed step. Persisted legacy Warning states are normalized to Unstart on resume and re-selected by rerun selectors, so existing workspaces re-run the LEC instead of skipping it.
Codex review of the branch surfaced four defects:
- The post-route LEC freshness/requirement checks bound the proof to
filler_ecc output, but the canonical chain wires postRouteLec's gate
input to the LVS output netlist, so a proven proof was judged stale
and blocked export. Bind the checklist, collector, and discovery
helpers to lvs_ecc/output/<design>_lvs.v.gz and require the LVS step
instead of filler for the LEC requirement.
- The checklist text report dropped every evidence entry because it
only kept plain strings while schema-v3 producers store {kind, path}
objects; extract the path from dict entries.
- The signoff review grouper matched step 'Route' although the
canonical step name is 'route', mis-routing routing checklist items
into the Reports group.
- Two comments still claimed a warned LEC is finished and skipped,
contradicting the Warning-state removal.
…ract Codex review round 2 surfaced three documentation defects: - The installer docs promised a bare glibc 2.34+ host, but the bundle now filters out libfontconfig (ecc.spec filter_host_fontconfig), so DreamPlace placement needs the host's fontconfig; state the prerequisite in the READMEs and tutorials. - The development guides' tool-interface example used signatures the loader never calls (build_step(workspace, step), run_step(workspace_step) -> StateEnum) and referenced the nonexistent StateEnum.Incomplete; rewrite both examples to the real contract (keyword-expanded build_step, run_step(workspace, step, ecc_module=None) -> bool). - development.md described create_flow_json as fabricating runs/<id>/home/flow.json although the fixture writes home/flow.json beneath the given workspace directory.
… rework Codex review round 3 surfaced five defects: - Reconcile rejected every pre-reorder ledger (DRC/LVS before filler, RCX/STA/Harden as preset suffixes) as flow_mismatch; migrate them onto the canonical chain by step name instead, keeping the identical Synthesis..route prefix and restarting the post-route suffix. - The rcx/harden presets removed with the chain fold-in broke persisted configs; resolve them as legacy ranges (through sta / through Harden). - load_workspace inferred netlist roles from the golden_ filename prefix, flipping a primary netlist named golden_* to golden on reload; the golden path is now persisted in the flow ledger at creation/first run and trusted on reload, with the filename convention kept only for legacy ledgers. - QoR flow order followed STEP_DIRECTORIES insertion order, which predated the chain rework; the table now follows the canonical chain so area scoring picks the latest scored step in execution order. - README quick-start configs still carried the removed [flow].run key.
Codex review round 4 surfaced seven defects: - toml_edit searched unmasked text for the next table header and matched table/assignment keys by raw spelling: header-like text inside multiline strings could become an insertion point, and quoted keys ([params."place"], "timeout" = ...) were duplicated instead of edited. Header and assignment matching now compare semantic keys on the original text at masked-candidate positions. - The reordered-ledger migration's rollback was keyed on 'appended', which is empty when a complete legacy ledger is only reordered; key the rollback on the flow_data snapshot instead. - run_prepare's generic exception path discarded rollback diagnostics and skipped the manifest-registration rollback; it now shares failed_workspace's registration cleanup and reports rollback problems in the failure reason. - QoR scoring consumed analysis metrics for steps no longer successful (invalidation keeps outputs on disk); only Success steps score now. - param set/unset let read/write failures escape as tracebacks; they now return structured config_error records. - The signoff step reconstructor did not apply the persisted info.spef projection for STA-entry workspaces, so refresh and execution saw different step models; it mirrors the execution-side projection now, and the signoff LEC golden precedence matches the engine (synthesis output, then declared golden, then origin RTL).
Codex review round 5 surfaced five defects: - STA-entry workspaces declare design.spef, but collect_sta_signoff_items only read RCX_ecc output, so a declared parasitics file never reached the STA/Harden runs; flows without an RCX step now use the declared SPEF for every corner. - The user guides claimed release v0.1.0-alpha.11 ships the new command groups; that tag does not exist and alpha.9 predates them, so both guides now state the actual coverage. - The Sizer env-root probe accepted a non-executable file, shadowing a working PATH install; the override now requires the executable bit. - The user guides still claimed ecc check skips single-RTL-source existence; check validates every declared source now. - The test conftest kept PDK skip mappings for the deleted test_harden_flow.py/test_rcx_flow.py modules.
…h alpha.12 The user guides and tutorials now say the ecc doc guides, the doctor/signoff/report command groups, and the run workspace/range selectors become available with release v0.1.0-alpha.12, with the run-from-source path called out until then.
Codex review round 6 surfaced five defects; four are fixed here: - Explicit step execution (ecc run --only lec) initialized a native ECC DB engine for yosys_lec, unlike the guarded batch path; the netlist-only LEC step now skips DB creation there too. - ecc migrate rebased home.json and PDK-config pointers but not the absolute origin/ paths persisted in flow.json step info (golden_verilog, spef), leaving moved workspaces pointing at the old runs/ location; both directions now rebase them. - The tutorials claimed existing-workspace selectors reject aliases, but _require_step_index normalizes them; the notes, examples, and troubleshooting rows now match the code. - The user guides' run-status vocabulary omitted 'partial'. Also: the legacy rcx/harden preset aliases added earlier only resolved in the workspace layer — ecc check/run validate against the builder set and still rejected them, so the validators now include the alias ranges. Deferred on request: runtime/requests.py workspace.create not accepting golden_verilog/spef inputs.
Emin017
force-pushed
the
emin/update-cli-cleanup-review
branch
from
September 8, 2026 09:45
bd03705 to
f1f783b
Compare
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
--plainas the public machine-readable output for ECC subcommands. Remove--jsonand--jsonlfrom those commands while retaining the hiddenecc version --jsondesktop contract.ecc.toml, config override, report, parameter, manifest, workspace creation, overwrite, and refresh updates atomic or rollback-aware. Failed mutations preserve the previous config and workspace data.Warningflow state. An unproven synthesis LEC now stops the flow and blocks signoff export; legacyWarningstates are reset for rerun.ecc pdk setupand theecc doc devtopic. Rename the six shipped guides, keep contributor docs in the repository, add a Chinese development guide, and repair README and guide links.Scope
Select the areas touched by this PR:
uv.lock, or release artifacts.Runtime And Packaging Impact
ecc-toolsorecc-dreamplacedependency changedNotes:
emin/update-cli-docs.Validation
List the commands you ran. Mark checks that are not applicable as N/A.
uv run pytest test/(GitHub Actions uses--no-syncand excludestest/examples/test_soc.py.)uv run ruff check chipcompiler testuv run ruff format --check chipcompiler testecc --help,ecc --version,ecc version --jsonnix run .#cli -- --helpSkipped checks and reason:
Checklist