docs(refork): Phase 0 inventory and v29 seam analysis#378
Conversation
PR Template CheckMissing required sections:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Phase 0 documentation and tooling for reforking via-core onto zksync-era core-v29.19.2. It includes comprehensive architecture mapping across seven distinct integration seams, a specification for a differential evaluation rig, and the Smithers automated port-unit workflow. Additionally, a Python script is provided to generate the port inventory. The review feedback highlights a critical bug in the inventory generation script where compact rename paths from git diff are not correctly resolved, causing renamed files to report zero line changes. A fix has been suggested to resolve these paths properly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| loc = {} | ||
| for line in numstat.splitlines(): | ||
| add, dele, rest = line.split("\t", 2) | ||
| if "=>" in rest: # rename: "old => new" possibly with {…} braces | ||
| rest = rest.split("\t")[-1] | ||
| loc[rest] = (add, dele) |
There was a problem hiding this comment.
There is a bug in how renamed files are handled when parsing the git diff --numstat output. Git formats renamed paths in --numstat using a compact representation (e.g., core/lib/{utils => multivm/...}/bytecode.rs or old => new).
Because the script currently does not resolve these compact rename paths, loc is populated with keys containing { and =>. When the script later queries loc using the plain destination path (from --name-status), the lookup fails and defaults to ("0", "0"). This causes all renamed files to incorrectly report 0 lines added and 0 lines deleted in the generated inventory.
We can fix this by adding a helper function to resolve the compact rename path into the final destination path before populating loc.
def resolve_new_path(p):
if " => " not in p:
return p
if "{" in p and "}" in p:
prefix, rest = p.split("{", 1)
rename_part, suffix = rest.split("}", 1)
_, new = rename_part.split(" => ", 1)
return f"{prefix}{new}{suffix}".replace("//", "/")
return p.split(" => ", 1)[1]
loc = {}
for line in numstat.splitlines():
add, dele, rest = line.split("\t", 2)
loc[resolve_new_path(rest)] = (add, dele)There was a problem hiding this comment.
Pull request overview
This PR adds “Phase 0” refork planning artifacts for re-basing via-core onto upstream zksync-era tag core-v29.19.2, including a reproducible inventory of diverged paths and seam-by-seam porting guidance.
Changes:
- Add
etc/refork/build_inventory.py(and checked-ininventory.csv) to classify all diverged paths vs the fork point and annotate each with a v29 “fate”. - Add refork documentation (
docs/refork/*) covering port inventory, seam analyses (01–07), Smithers orchestration workflow, and differential evaluation specs. - Add a Phase 0 README tying the artifacts together and describing how to regenerate the inventory.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| etc/refork/build_inventory.py | Generates etc/refork/inventory.csv by diffing HEAD vs merge base and joining against upstream pin to compute categories and v29 fate. |
| etc/refork/inventory.csv | Checked-in generated inventory output (diverged paths + classifications + fates). |
| docs/refork/README.md | Entry point for Phase 0 artifacts and reproduction instructions. |
| docs/refork/port-inventory.md | Summary stats, category meanings, and dependency-wave port order for port units. |
| docs/refork/smithers-workflow.md | Spec for the Smithers per-unit port pipeline, gates, and cross-run memory usage. |
| docs/refork/differential-evals.md | Spec for parity/differential evaluation topology and scorers. |
| docs/refork/seams/01-node-framework.md | Seam mapping for node framework composition changes in v29. |
| docs/refork/seams/02-settlement-btc-sender.md | Seam mapping/decision on keeping BTC sender stack parallel to ETH-shaped settlement abstraction. |
| docs/refork/seams/03-config.md | Seam mapping for config system shift to smart_config derives and schema registration. |
| docs/refork/seams/04-state-keeper-fee-model.md | Seam mapping for state keeper IO and fee model trait seams and port approach. |
| docs/refork/seams/05-da.md | Seam mapping for DA trait changes and upstream Celestia client availability. |
| docs/refork/seams/06-dal-migrations.md | Seam mapping and regenesis-driven migrations/DAL port strategy. |
| docs/refork/seams/07-watchers-reorg-storage-init.md | Grouped small-unit seam mapping and per-unit port recipe. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| v29_blobs = tree_blobs(UPSTREAM_PIN) | ||
| head_blobs = tree_blobs("HEAD") | ||
| v29_tree = set(v29_blobs) | ||
| v29_by_base = defaultdict(list) | ||
| for p in v29_tree: | ||
| v29_by_base[os.path.basename(p)].append(p) | ||
|
|
| loc = {} | ||
| for line in numstat.splitlines(): | ||
| add, dele, rest = line.split("\t", 2) | ||
| if "=>" in rest: # rename: "old => new" possibly with {…} braces | ||
| rest = rest.split("\t")[-1] | ||
| loc[rest] = (add, dele) | ||
|
|
| with open(OUT_CSV, "w", newline="") as fh: | ||
| w = csv.DictWriter(fh, fieldnames=list(rows[0].keys())) | ||
| w.writeheader() | ||
| w.writerows(rows) |
|
Warning Review limit reached
More reviews will be available in 42 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
WalkthroughThis PR adds Phase 0 refork documentation and tooling pinned to core-v29.20.0: refork README, a port-inventory report, a Python inventory generator, a differential-eval rig spec, seven seam porting guides, and the Smithers port-unit workflow. ChangesRefork Phase 0: Documentation and inventory
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@etc/refork/build_inventory.py`:
- Around line 112-115: The current move-detection builds v29_by_base from
set(v29_blobs) so cands[0] is non-deterministic and basename-only matches
produce false positives (e.g., mod.rs); update the logic around v29_tree /
v29_by_base that selects cands[0] (and the analogous block at the later 132-134
region) to be deterministic and stricter: sort the candidate list before picking
a candidate, and only mark v29_fate as "moved?" when the basename is unique
among v29_by_base (or alternatively replace with a stronger path-similarity
check); otherwise leave the row as "gone" so reviewers can triage manually.
- Around line 118-145: When parsing numstat into loc, normalize git's
brace-compressed rename tokens so the same key will be found later: when you
detect a rename rest containing "=>", expand/normalize brace forms (the value in
rest produced in the numstat loop) into the canonical "old_path => new_path"
string (and/or also store the unbraced new_path key) so that the later lookup
loc.get(path, loc.get(f"{old_path} => {path}", ("0","0"))) finds the correct
entry; update the numstat parsing logic that sets rest and loc[rest] and/or add
a small helper used by fate/name_status handling to canonicalize rename syntax
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bf952e6-1a6f-4883-a149-7e77e249c8e1
⛔ Files ignored due to path filters (1)
etc/refork/inventory.csvis excluded by!**/*.csv
📒 Files selected for processing (12)
docs/refork/README.mddocs/refork/differential-evals.mddocs/refork/port-inventory.mddocs/refork/seams/01-node-framework.mddocs/refork/seams/02-settlement-btc-sender.mddocs/refork/seams/03-config.mddocs/refork/seams/04-state-keeper-fee-model.mddocs/refork/seams/05-da.mddocs/refork/seams/06-dal-migrations.mddocs/refork/seams/07-watchers-reorg-storage-init.mddocs/refork/smithers-workflow.mdetc/refork/build_inventory.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build core images / Prepare contracts
- GitHub Check: copilot-pull-request-reviewer
🧰 Additional context used
📓 Path-based instructions (2)
docs/**
⚙️ CodeRabbit configuration file
docs/**: Focus on technical accuracy, stale commands, unsafe operator guidance, and source-vs-live or deployment-boundary claims.
Avoid prose churn.
Files:
docs/refork/seams/03-config.mddocs/refork/seams/04-state-keeper-fee-model.mddocs/refork/seams/07-watchers-reorg-storage-init.mddocs/refork/smithers-workflow.mddocs/refork/seams/05-da.mddocs/refork/README.mddocs/refork/differential-evals.mddocs/refork/seams/02-settlement-btc-sender.mddocs/refork/seams/06-dal-migrations.mddocs/refork/port-inventory.mddocs/refork/seams/01-node-framework.md
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdScope
Applies to this repository and all descendants unless a nested
AGENTS.mdadds more specific instructions.Purpose
This repository owns Via source and runtime behavior. It does not own live or desired-state deployment.
Read first
Before changing non-trivial runtime behavior, read the relevant Via guide and the nearest crate README or config.
Important paths
- Runtime:
core/- Verifier:
via_verifier/- Prover / Indexer:
prover/,via_indexer/- Bitcoin:
core/lib/via_btc_client/,core/node/via_btc_*, and verifier BTC paths- DA / Reorg:
core/lib/via_da_clients/,core/node/via_main_node_reorg_detector/, andvia_verifier/node/via_reorg_detector/. (When touching one, check the other — see Reuse and duplication discipline.)Source-of-truth rules
- Source code describes behavior; it does not prove deployment.
- Prefer Via-specific
via_modules when extending fork behavior.- Follow the call graph into upstream (non-
via_*) code when necessary, and explain why a Via extension was insufficient.- For Bitcoin work, remember that txids are byte-reversed.
Safety rules
- Never commit secrets or live credentials.
- Do not run migrations or deploy from this repo without explicit approval.
- Keep local agent scratch directories (
.gitnexus/,.agents/, etc.) out of commits.Reuse and duplication discipline
Apply this section before writing implementation code. The PR template’s Reuse & Duplication and Performance, Complexity, and Resource Impact sections cannot be filled in honestly after the fact — they exist to surface the reasoning required here.
Production code is permanent audit cost. Every new function, detector, client, poller, watcher, or worker adds ongoing review and maintenance burden. Production LOC is measured as net delta (lines added minus lines removed, excluding comments, documentation, tests, and generated files). Before writing new code, identify the existing function...
Files:
docs/refork/seams/03-config.mddocs/refork/seams/04-state-keeper-fee-model.mddocs/refork/seams/07-watchers-reorg-storage-init.mddocs/refork/smithers-workflow.mddocs/refork/seams/05-da.mddocs/refork/README.mddocs/refork/differential-evals.mddocs/refork/seams/02-settlement-btc-sender.mddocs/refork/seams/06-dal-migrations.mddocs/refork/port-inventory.mddocs/refork/seams/01-node-framework.mdetc/refork/build_inventory.py
🪛 ast-grep (0.43.0)
etc/refork/build_inventory.py
[error] 33-35: Use of unsanitized data to create processes
Context: subprocess.run(
["git", *args], check=True, capture_output=True, text=True
)
Note: [CWE-78].
(os-system-unsanitized-data)
[error] 33-35: Command coming from incoming request
Context: subprocess.run(
["git", *args], check=True, capture_output=True, text=True
)
Note: [CWE-20].
(subprocess-from-request)
🪛 LanguageTool
docs/refork/seams/02-settlement-btc-sender.md
[style] ~31-~31: Consider an alternative for the overused word “exactly”.
Context: ..._sender`), which the inventory shows is exactly how via-core is already built — nothing...
(EXACTLY_PRECISELY)
docs/refork/seams/06-dal-migrations.md
[style] ~8-~8: Consider an alternative for the overused word “exactly”.
Context: ... interleave with upstream's, which is exactly what makes a merge impossible. - ~2,649...
(EXACTLY_PRECISELY)
🪛 markdownlint-cli2 (0.22.1)
docs/refork/smithers-workflow.md
[warning] 20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/refork/differential-evals.md
[warning] 10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.15.15)
etc/refork/build_inventory.py
[warning] 33-33: Missing type annotation for *args
(ANN002)
[error] 34-34: subprocess call: check for execution of untrusted input
(S603)
[error] 35-35: Starting a process with a partial executable path
(S607)
[warning] 59-59: Too many return statements (9 > 6)
(PLR0911)
[warning] 81-81: Too many return statements (7 > 6)
(PLR0911)
[warning] 98-98: Too many branches (16 > 12)
(PLR0912)
[warning] 98-98: Too many statements (63 > 50)
(PLR0915)
[warning] 103-103: Missing return type annotation for private function tree_blobs
(ANN202)
[warning] 124-124: Missing return type annotation for private function fate
Add return type annotation: str
(ANN202)
[warning] 172-174: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
🔇 Additional comments (2)
docs/refork/seams/05-da.md (1)
42-44: ⚡ Quick winClarify the sequencing dependency language at line 43.
The phrase "only depends on config + btc_client (for none of it)" is ambiguous. Does it mean DA depends on no other units, or that other units do not depend on DA? Reword to remove the parenthetical and state the dependency direction explicitly (e.g., "Wave 1–2; depends only on [unit X] and [unit Y]" or "Wave 1–2; no other units depend on DA completion").
docs/refork/smithers-workflow.md (1)
20-32: ⚡ Quick winAdd language specification to fenced code block at line 20.
The ASCII diagram block opens with triple backticks but no language specifier, which may cause markdown renderers to apply incorrect syntax highlighting. Use
```textor```bashto declare the block type.-``` +```text pick unit ──► agent in worktree ──► cargo build -p <crate> ──► cargo test -p <crate>Source: Linters/SAST tools
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 941fae1684
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def main(): | ||
| os.makedirs(os.path.dirname(OUT_CSV), exist_ok=True) | ||
| rng = f"{MERGE_BASE}..HEAD" |
There was a problem hiding this comment.
When someone follows the documented reproduction command after this commit lands, this range includes the 13 Phase 0 files added by the commit itself, including etc/refork/inventory.csv. That makes the script rewrite the inventory with its own docs/tooling classified as new port work and changes the checked-in 3,457-path result, so the inventory is no longer reproducible from the committed instructions. Please pin the analyzed via revision to the pre-artifact commit or filter docs/refork/ and etc/refork/ from the diff.
Useful? React with 👍 / 👎.
| if status == "A" and path.endswith(".sql") and "/migrations/" in path: | ||
| cat, sub = "D", "via-migration" |
There was a problem hiding this comment.
This catches every added SQL migration as via-migration, but the generated D rows include non-Via upstream backports such as core/lib/dal/migrations/20240910112120_unsealed_batches_in_db.*.sql and 20240925103531_gateway_upgrade.*.sql. Because seam 06 then tells the port to renumber all via-migration rows after the v29 pin, those upstream schema changes would be ported again instead of treated as already present/squashed in v29; only Via-owned migrations should enter this bucket, while other added migrations need the same v29 fate/backport check as other additions.
Useful? React with 👍 / 👎.
| if "=>" in rest: # rename: "old => new" possibly with {…} braces | ||
| rest = rest.split("\t")[-1] | ||
| loc[rest] = (add, dele) |
There was a problem hiding this comment.
For modified renames, git diff --numstat -M (with -M rename detection, per git diff -h) emits brace-compressed paths like {old => new}/file.txt, while --name-status -M emits separate old/new fields. This normalization leaves the brace path as the key, so the lookup below falls back to 0/0; the checked-in CSV already shows R054/R080 rows such as core/lib/basic_types/src/pubdata_da.rs with zero LOC, which undercounts deltas and classifies many renamed files as wiring.
Useful? React with 👍 / 👎.
| cands = v29_by_base.get(os.path.basename(path)) | ||
| if cands: | ||
| return f"moved?:{cands[0]}" |
There was a problem hiding this comment.
This marks any missing path as moved? when v29 contains the same basename, even for common names like mod.rs, lib.rs, or config.rs; the checked-in CSV already maps deleted files such as core/lib/default_da_clients/src/no_da/mod.rs to unrelated multivm tracer modules. Those false candidates make the port inventory understate files that are actually gone and send porting work toward arbitrary locations, so this should require a stronger similarity/content check or leave ambiguous basename-only matches as gone/unresolved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
etc/refork/build_inventory.py (2)
139-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck
new_pathbefore falling back to old-path lineage infate().For
R*rows,path = old_path or new_pathmakes thepresentbranch ignore the renamed path. If via renamed a file and v29 has thatnew_pathwith different content, this gets labeledgone/moved?instead ofpresent, which feeds the wrongv29_fateinto the downstream queue.Suggested fix
def fate(old_path, new_path): # via's final content already equals upstream v29 -> the diff was a # backport of upstream work; nothing to port after the refork. if v29_blobs.get(new_path) is not None and v29_blobs.get(new_path) == head_blobs.get(new_path): return "identical-to-v29" + if new_path in v29_tree: + return "present" path = old_path or new_path if path in v29_tree: return "present"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@etc/refork/build_inventory.py` around lines 139 - 156, The fate() logic should check the post-rename path in v29 first instead of immediately falling back to old_path; modify the function (fate) so it tests if new_path is non-empty and in v29_tree and returns "present" in that case, and only then fall back to using old_path or new_path for the subsequent blob/base checks (i.e., change the path resolution to prefer new_path and keep the rest of the moves/basename logic unchanged).
48-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
verifier_dalto the same unit key used by the workflow docs.
via_crate_root()returnsverifier_dalforvia_verifier/lib/verifier_dal/..., but the dependency waves and workflow guidance refer to that unit asvia_verifier_dal. That splits one crate across two identifiers at the CSV/docs boundary and can break wave ordering or ownership checks.Suggested fix
def via_crate_root(path): """Return the via crate directory a path belongs to, or None.""" parts = path.split("/") for i, seg in enumerate(parts[:-1]): if seg.startswith(("via_", "via-")) and seg not in ("via_guides",): # top-level workspaces keep their inner crate as the unit if seg in ("via_verifier", "via_indexer") and len(parts) > i + 2: - return parts[i + 2] + inner = parts[i + 2] + if seg == "via_verifier" and inner == "verifier_dal": + return "via_verifier_dal" + return inner return seg return NoneAlso applies to: 60-79
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@etc/refork/build_inventory.py` around lines 48 - 57, via_crate_root currently returns "verifier_dal" for paths like "via_verifier/lib/verifier_dal/..." which mismatches the workflow/docs unit key "via_verifier_dal"; update via_crate_root so when a top-level workspace segment equals "via_verifier" and the inner crate name equals "verifier_dal" it returns the normalized unit key "via_verifier_dal" (i.e., detect the pattern in via_crate_root and return "via_verifier_dal" instead of "verifier_dal"); apply the same normalization logic to the analogous checks later in the file where inner crate names under top-level workspaces are mapped (the code around the other workspace-handling branches).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/refork/seams/02-settlement-btc-sender.md`:
- Around line 39-43: Remove the static fallback
SettlementLayer::L1(via_chain_id) from the port plan and perform an explicit
audit: run git grep -l 'SettlementLayer' (and 'SettlementLayerClient') to
enumerate all consumers, and for each consumer record whether the port omits the
layer (preferred) or requires a via-specific adapter / an explicit invariant
proving the path is inert; update the docs to reflect that no L1 placeholder is
allowed and note which ports need adapters or invariant proofs (reference
symbols: SettlementLayer, SettlementLayerClient, SettlementLayer::L1(...) and
via_chain_id).
---
Outside diff comments:
In `@etc/refork/build_inventory.py`:
- Around line 139-156: The fate() logic should check the post-rename path in v29
first instead of immediately falling back to old_path; modify the function
(fate) so it tests if new_path is non-empty and in v29_tree and returns
"present" in that case, and only then fall back to using old_path or new_path
for the subsequent blob/base checks (i.e., change the path resolution to prefer
new_path and keep the rest of the moves/basename logic unchanged).
- Around line 48-57: via_crate_root currently returns "verifier_dal" for paths
like "via_verifier/lib/verifier_dal/..." which mismatches the workflow/docs unit
key "via_verifier_dal"; update via_crate_root so when a top-level workspace
segment equals "via_verifier" and the inner crate name equals "verifier_dal" it
returns the normalized unit key "via_verifier_dal" (i.e., detect the pattern in
via_crate_root and return "via_verifier_dal" instead of "verifier_dal"); apply
the same normalization logic to the analogous checks later in the file where
inner crate names under top-level workspaces are mapped (the code around the
other workspace-handling branches).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d009ee6-b126-4d03-aba5-a25b32f88775
⛔ Files ignored due to path filters (1)
etc/refork/inventory.csvis excluded by!**/*.csv
📒 Files selected for processing (7)
docs/refork/README.mddocs/refork/port-inventory.mddocs/refork/seams/02-settlement-btc-sender.mddocs/refork/seams/03-config.mddocs/refork/seams/06-dal-migrations.mddocs/refork/smithers-workflow.mdetc/refork/build_inventory.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
docs/**
⚙️ CodeRabbit configuration file
docs/**: Focus on technical accuracy, stale commands, unsafe operator guidance, and source-vs-live or deployment-boundary claims.
Avoid prose churn.
Files:
docs/refork/seams/02-settlement-btc-sender.mddocs/refork/seams/06-dal-migrations.mddocs/refork/smithers-workflow.mddocs/refork/seams/03-config.mddocs/refork/README.mddocs/refork/port-inventory.md
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdScope
Applies to this repository and all descendants unless a nested
AGENTS.mdadds more specific instructions.Purpose
This repository owns Via source and runtime behavior. It does not own live or desired-state deployment.
Read first
Before changing non-trivial runtime behavior, read the relevant Via guide and the nearest crate README or config.
Important paths
- Runtime:
core/- Verifier:
via_verifier/- Prover / Indexer:
prover/,via_indexer/- Bitcoin:
core/lib/via_btc_client/,core/node/via_btc_*, and verifier BTC paths- DA / Reorg:
core/lib/via_da_clients/,core/node/via_main_node_reorg_detector/, andvia_verifier/node/via_reorg_detector/. (When touching one, check the other — see Reuse and duplication discipline.)Source-of-truth rules
- Source code describes behavior; it does not prove deployment.
- Prefer Via-specific
via_modules when extending fork behavior.- Follow the call graph into upstream (non-
via_*) code when necessary, and explain why a Via extension was insufficient.- For Bitcoin work, remember that txids are byte-reversed.
Safety rules
- Never commit secrets or live credentials.
- Do not run migrations or deploy from this repo without explicit approval.
- Keep local agent scratch directories (
.gitnexus/,.agents/, etc.) out of commits.Reuse and duplication discipline
Apply this section before writing implementation code. The PR template’s Reuse & Duplication and Performance, Complexity, and Resource Impact sections cannot be filled in honestly after the fact — they exist to surface the reasoning required here.
Production code is permanent audit cost. Every new function, detector, client, poller, watcher, or worker adds ongoing review and maintenance burden. Production LOC is measured as net delta (lines added minus lines removed, excluding comments, documentation, tests, and generated files). Before writing new code, identify the existing function...
Files:
docs/refork/seams/02-settlement-btc-sender.mddocs/refork/seams/06-dal-migrations.mddocs/refork/smithers-workflow.mddocs/refork/seams/03-config.mddocs/refork/README.mddocs/refork/port-inventory.mdetc/refork/build_inventory.py
🪛 Ruff (0.15.15)
etc/refork/build_inventory.py
[warning] 122-122: Missing return type annotation for private function resolve_renamed
(ANN202)
[warning] 201-203: Unnecessary dict() call (rewrite as a literal)
Rewrite as a literal
(C408)
🔇 Additional comments (2)
docs/refork/smithers-workflow.md (2)
56-56: ⚡ Quick winClarify the regenesis migration cutoff date.
Line 56 specifies
20260602000000(June 2, 2026) as the cutoff after which migrations should be renumbered. This date is 10 days in the past as of the review date (June 12, 2026). Clarify whether this is:
- A historical marker (migrations were already renumbered in a prior regenesis phase), and if so, explicitly state that in the document.
- A rule that should be updated to a future-safe value before the workflow runs.
The ambiguity creates risk that operators might misunderstand whether this cutoff is current or stale when the workflow is instantiated.
Source: Coding guidelines
39-40: ⚡ Quick winDefine the iteration limit
Nexplicitly.Line 39–40 references a backpressure loop with "max N iters," but the value of
Nis not specified. For operator clarity and to prevent indefinite spinning, define the concrete iteration limit (e.g., "max 3 iterations", "max 5 iterations") and document the escalation criteria (when a unit should be parked for interactive work).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8ee0fadc9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fieldnames = ["path", "old_path", "status", "category", "subclass", | ||
| "v29_fate", "loc_added", "loc_deleted", "port_unit"] | ||
| with open(OUT_CSV, "w", newline="") as fh: | ||
| w = csv.DictWriter(fh, fieldnames=fieldnames) |
There was a problem hiding this comment.
The repo's required validation includes git diff --check, but this csv.DictWriter uses the default Excel dialect, which writes \r\n line endings when combined with newline="". The checked-in etc/refork/inventory.csv therefore reports trailing whitespace on every row under git diff --check, so this commit cannot pass the documented validation until the writer uses lineterminator="\n" and the CSV is regenerated.
Useful? React with 👍 / 👎.
| if status == "A" and path.endswith(".sql") and "/migrations/" in path: | ||
| # via's history cherry-picked upstream migrations; only via-owned | ||
| # ones are port work, backports already sit in the v29 schema. | ||
| if path in v29_tree or head_blobs.get(path) in v29_by_blob: |
There was a problem hiding this comment.
For added migrations whose down script is just the stock placeholder, head_blobs.get(path) in v29_by_blob treats any blob match as an upstream backport even when the path is not in v29. The current CSV demonstrates this with via_verifier/lib/verifier_dal/migrations/20250920102508_witdrawals.down.sql: it is marked backported-migration because its one-line placeholder matches an unrelated core/lib/dal down migration, while the paired .up.sql is a Via withdrawal migration. That drops part of a Via-owned schema change from the migration port queue, so blob matches should be constrained to the same migration path or not used for generic down scripts.
Useful? React with 👍 / 👎.
| """Return the via crate directory a path belongs to, or None.""" | ||
| parts = path.split("/") | ||
| for i, seg in enumerate(parts[:-1]): | ||
| if seg.startswith(("via_", "via-")) and seg not in ("via_guides",): |
There was a problem hiding this comment.
When a non-crate path has any segment starting with via_/via-, this returns that segment as a crate root before port_unit() can classify CI, Docker, infrastructure, or playground files. The generated CSV already marks .github/lint/via-structural/..., docker/via-server/Dockerfile, and via-playground/... as A/via-crate, so the headline “39 standalone via crates” bucket and port ownership queue include tooling/config files that are not Rust crates.
Useful? React with 👍 / 👎.
| From `Cargo.toml` graphs of the 39 via crates (via→via edges only; all also depend on | ||
| `zksync_*` crates, which the refork provides): | ||
|
|
||
| 1. **Wave 1**: `via_btc_client`, `via_da_clients`, `via_da_client`, `via_mempool`, `via_reorg`, `via_consensus`, `via_block_reverter`, `via_da_dispatcher_lib`, `via_indexer_dal` |
There was a problem hiding this comment.
Seam 03 says the config unit must land in Wave 1 because every Via crate reads its config struct, and the CSV has port_unit=config rows for all core/lib/config/src/configs/via_*.rs; however this Wave 1 queue starts with via_btc_client / DA / other crates and never lists config. A Smithers run following this queue will start porting crates before the smart-config structs and secrets are registered, causing avoidable build blockers and skipping the required early config unit.
Useful? React with 👍 / 👎.
| path = old_path or new_path | ||
| if path in v29_tree: |
There was a problem hiding this comment.
For renamed rows this sets path to the old path, so a file that now exists in v29 at the renamed destination is not classified as present; the current CSV shows 96 self-referential moved?:<same new path> rows such as core/lib/types/src/pubdata_da.rs -> core/lib/basic_types/src/pubdata_da.rs. Those are not weak move hints — they are files already present at the destination in the pin — and the wrong fate sends port work through the ambiguous-move workflow instead of re-deriving the delta at the actual v29 path.
Useful? React with 👍 / 👎.
|
Review findings addressed in b8ee0fa (all verified empirically before fixing — counts below are from the regenerated Rename LOC parsing (@gemini-code-assist high-priority, also Copilot/CodeRabbit/Codex) — confirmed: Migration backports misclassified (Codex) — confirmed, the largest content error: of 71
Self-inclusion of the Phase 0 artifacts (Codex) — fixed via Nits (Copilot) — unused Docs citing the affected numbers ( |
… migration backports, LF csv)
|
Second review round addressed in 46e7ea6 — all eight findings verified against the data first, and all eight were valid:
Migration backport false positive (Codex) — confirmed: via-crate over-matching (Codex) — confirmed: 74 tooling rows (via-playground,
CRLF in the checked-in CSV (Codex) — confirmed (all 3,458 lines); writer now uses
Docs citing affected counts ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46e7ea6dd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| hits = v29_by_blob.get(base_blobs.get(path) or head_blobs.get(new_path)) | ||
| if hits: | ||
| return f"moved:{min(hits)}" |
There was a problem hiding this comment.
For files whose blob is empty or a stock placeholder, this records an arbitrary v29 path as a confirmed moved: fate even though there is no lineage. The checked-in CSV demonstrates this with core/tests/via_loadnext/src/sdk/abi/IL1Bridge.json (empty) being mapped to CHANGELOG.md, and with the verifier withdrawal down migration mapped to an unrelated DAL down script. Since moved is documented as a confirmed reorganization that should be re-derived at the target path, these rows send the refork work queue to edit unrelated files; common/empty blobs need to be ignored or constrained by path similarity.
Useful? React with 👍 / 👎.
| 3. **Wave 3**: `via_state_keeper`, `via_btc_watch`, `via_btc_sender`†, `via_external_node`, `via_verifier_dal`, `via_withdrawal_client`, `via_indexer` | ||
| 4. **Wave 4**: `via_musig2`, `via_verifier_btc_sender`, `via_verifier_btc_watch`, `via_verifier_reorg_detector`, `via_verifier_state`, `via_verifier_storage_init` | ||
| 5. **Wave 5**: `via_verifier_coordinator`, `via_zk_verifier` |
There was a problem hiding this comment.
The workflow groups work by the CSV port_unit, but this wave list omits several package-name units that the generated inventory contains, including via_indexer_bin, via_indexer_storage_init, via_block_reverter_cli, via_verifier, and via_verifier_block_reverter. A Smithers run following this ordering has no wave assignment for those rows, so whole binaries/storage-init/reverter units can be skipped or parked indefinitely; either list them explicitly or make the documented grouping match the CSV units.
Useful? React with 👍 / 👎.
| if p[0] == "core" and len(p) > 2 and p[1] in ("lib", "node", "bin", "tests"): | ||
| return p[2] |
There was a problem hiding this comment.
For upstream crates this stores the directory segment as port_unit (for example config, state_keeper, or basic_types) rather than the actual Cargo package names (zksync_config, zksync_state_keeper, zksync_basic_types). The Smithers workflow consumes this CSV as the ownership map and runs cargo build -p <crate>, so those units fail validation or require an undocumented manual mapping before any port work can be tested; the inventory needs a build package column or package-name units for Cargo crates.
Useful? React with 👍 / 👎.
| `etc/refork/inventory.csv`, grouped by `port_unit`, ordered by the dependency waves in | ||
| `port-inventory.md`. A unit's work item bundles: | ||
|
|
||
| - its Category A rows (crate files / embedded additions), |
There was a problem hiding this comment.
This queues every Category A row for a unit, but Category A also includes the 262 generated .sqlx rows and the 31 backported-migration rows that the inventory and seam 06 explicitly say are regenerated or already present in v29. A Smithers run following this work-queue spec can waste time copying stale SQLx metadata or reapplying upstream migrations that should be zero port work; the queue should exclude generated and backported-migration subclasses (and any A rows marked identical-to-v29) instead of taking all Category A rows.
Useful? React with 👍 / 👎.
What ❔
Phase 0 deliverables for the refork of zksync-era at
core-v29.20.0:etc/refork/inventory.csv+build_inventory.py— all 3,457 diverged paths vs forkpoint
f37b84ac75, categorized (via crates / modified upstream / deleted /migrations) and joined against the pin for their v29 fate, including byte-identical
backport detection.
docs/refork/port-inventory.md— headline findings and the dependency-wave portingorder for the 39 via crates.
docs/refork/seams/01–07— per-seam mapping of what via patched in v24 to theverified v29 extension point.
docs/refork/smithers-workflow.md+differential-evals.md— orchestration andparity-eval specs for the port execution phase.
Why ❔
via-core is ~21 months behind upstream; merging is no longer feasible. These artifacts
are the inventory, architecture mapping, and orchestration specs the refork executes
against. Key findings: the refork is additive (via never deleted the ETH stack), 591
diverged paths are pure backports of upstream work, and the forked via crates carry
small deltas that should be re-applied onto v29 crates rather than carried as forks.
Checklist
Reproduce the inventory:
https://claude.ai/code/session_01XoczhkbwY36DmhAwnkN5i4