Skip to content

docs, .claude: add hardfork rollout review guidance#2229

Merged
pratikspatil024 merged 4 commits into
developfrom
psp-pos-3598
May 19, 2026
Merged

docs, .claude: add hardfork rollout review guidance#2229
pratikspatil024 merged 4 commits into
developfrom
psp-pos-3598

Conversation

@pratikspatil024
Copy link
Copy Markdown
Member

@pratikspatil024 pratikspatil024 commented May 18, 2026

Summary

Adds standalone hardfork rollout review guidance for Bor.

Hardforks in Bor require coordinated wiring across runtime chain presets, packaged genesis files, fork ID calculation, config compatibility, startup visibility, EVM/precompile behavior, consensus logic, and miner behavior. This PR adds a
path-scoped Claude rule plus repo-level AGENTS guidance so AI-assisted reviews can catch incomplete hardfork rollouts even when run directly inside this repository.

The new guidance specifically calls out the drift class where a fork block is present in packaged genesis JSON but missing from the normal runtime Go chain presets. In that case a node started through the normal bor server --config ...
path can load Bor.<Fork>Block == nil, so fork checks such as Bor.Is<Fork>(num) never activate on the standard startup path.

What changed

  • Added .claude/rules/hardfork-rollout.md with a path-scoped frontmatter that auto-loads on edits to params/, internal/cli/server/chains/, builder/files/genesis-*.json, core/forkid/forkid.go, fork-gated files under core/vm/,
    consensus/bor/bor.go, and miner/worker.go.
  • Added the new rule to the AGENTS.md security rules table.
  • Added a Standalone Hardfork Rollout Review subsection to AGENTS.md so the guidance applies even when an AI agent is launched directly inside this repository, outside the PoS team workspace.
  • Documented required cross-checks across:
    • params.BorMainnetChainConfig
    • params.AmoyChainConfig
    • internal/cli/server/chains/mainnet.go
    • internal/cli/server/chains/amoy.go
    • packaged genesis JSON files
    • fork ID inputs
    • startup banner / config output
    • EVM gas / opcode / precompile gates
    • consensus and miner fork-gated behavior
    • N-1 / N / N+1 activation-boundary test coverage
    • cross-client (Erigon) parity tracking

Executed tests

Docs / rules-only change; no runtime code touched.

git diff --check

Also checked the updated markdown files for trailing whitespace.

The rule body was empirically validated by running fresh-context AI reviews against three historical hardfork-shaped PRs across bor and heimdall-v2. Each case was flagged at CRITICAL severity with concrete file:line evidence; per-case
verdicts and full findings live in the corresponding workspace PR.

Rollout notes

No runtime behavior changes. This only affects repo-local AI guidance and review checklists.

Future hardfork PRs should use this rule to verify activation blocks, runtime config parity, packaged genesis parity, fork ID behavior, startup visibility, N-1/N/N+1 boundary tests, and cross-client parity where consensus output changes.

Related PRs

  - add hardfork rollout rule for fork config, genesis, fork ID, VM, consensus, and miner paths
  - document runtime config vs packaged genesis parity checks
  - call out the packaged-genesis-only activation failure class
  - add standalone AGENTS guidance for direct repo sessions
Copilot AI review requested due to automatic review settings May 18, 2026 12:12
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds repo-local AI review guidance for Bor hardfork rollout checks, focusing on runtime/genesis parity, fork activation wiring, fork ID behavior, and boundary test expectations.

Changes:

  • Adds .claude/rules/hardfork-rollout.md with path-scoped hardfork review guidance.
  • Updates AGENTS.md to list the new rule and add standalone hardfork rollout review instructions.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
AGENTS.md Adds the hardfork rollout rule to the security rules table and documents standalone review checks.
.claude/rules/hardfork-rollout.md Introduces the hardfork rollout review checklist and path-scoped trigger configuration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .claude/rules/hardfork-rollout.md Outdated
@claude
Copy link
Copy Markdown

claude Bot commented May 18, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@pratikspatil024 pratikspatil024 requested a review from a team May 18, 2026 12:28
@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.36%. Comparing base (23b80b7) to head (649976a).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2229      +/-   ##
===========================================
- Coverage    52.38%   52.36%   -0.03%     
===========================================
  Files          885      885              
  Lines       155855   155904      +49     
===========================================
- Hits         81644    81638       -6     
- Misses       68979    69025      +46     
- Partials      5232     5241       +9     

see 20 files with indirect coverage changes
see 20 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

marcello33
marcello33 previously approved these changes May 18, 2026
lucca30
lucca30 previously approved these changes May 18, 2026
  The previous frontmatter listed 5 specific files (contracts, eips,
  gas_table, jump_table, operations_acl) which was tighter than the
  consensus-critical EVM surface warrants. Hardfork-gated behavior can
  also land in evm.go, instructions.go, interpreter.go, gas.go, opcodes.go,
  etc. — those changes would not have triggered the rule.

  Broadens to core/vm/*.go so the rule auto-loads on any top-level VM file.
  The rule's Trigger Conditions let the agent quickly conclude "not
  hardfork-shaped" when the diff doesn't touch fork-relevant code, so the
  extra noise on unrelated VM edits is acceptable for the coverage win.

  Addresses Copilot inline review comment on PR #2229.
@pratikspatil024 pratikspatil024 dismissed stale reviews from lucca30 and marcello33 via 6b3ace2 May 19, 2026 04:35
@pratikspatil024
Copy link
Copy Markdown
Member Author

@claude review once

@claude
Copy link
Copy Markdown

claude Bot commented May 19, 2026

Claude finished @pratikspatil024's task in 1m 54s —— View job


Code Review

  • Gather context and understand the PR
  • Review .claude/rules/hardfork-rollout.md
  • Review AGENTS.md changes
  • Cross-check consistency between the two files
  • Post review findings

Summary

This is a well-structured docs/rules-only PR that adds hardfork rollout review guidance. The content addresses a real and dangerous failure class — fork blocks present in genesis JSON but missing from Go runtime presets — and the guidance is actionable. No issues found that would block merge.

Findings

No bugs or security issues — this is purely documentation. Two minor observations below.

1. Scope column in AGENTS.md table omits params/protocol_params.go (Nit)

The frontmatter in hardfork-rollout.md includes params/protocol_params.go as a trigger path, but the table row in AGENTS.md:146 only lists params/config.go. Since the table is a summary this is fine, but it could cause confusion if someone uses the table as the authoritative path list. Consider adding it or noting "selected params/ files" in the scope column.

2. Intentional duplication between rule file and AGENTS.md standalone section

The "Standalone Hardfork Rollout Review" section in AGENTS.md:150-178 largely repeats the content from hardfork-rollout.md. This is acknowledged in the PR description as intentional (for agents running outside the PoS team workspace where path-scoped rules don't auto-load). Just flagging that both files will need coordinated updates if the guidance evolves — e.g., if new config surfaces or networks are added.

What looks good

  • The specific "nil fork block" failure class description is clear and directly actionable — this is the kind of precise guidance that prevents real incidents.
  • Path scope broadened to core/vm/*.go (commit 6b3ace2) correctly addresses the Copilot feedback. The slight noise on unrelated VM edits is the right tradeoff.
  • The N-1/N/N+1 boundary testing requirement and Erigon parity tracking are valuable additions to the review checklist.
  • Trigger conditions section lets the agent quickly determine "not hardfork-shaped" and bail out, avoiding false positives on unrelated edits to scoped files.
  • All referenced file paths verified to exist in the repository.

Verdict: Looks good to merge. The two nits above are non-blocking.


  - include params/protocol_params.go in the AGENTS rule scope
  - note that AGENTS hardfork guidance mirrors the path-scoped rule
Copilot AI review requested due to automatic review settings May 19, 2026 08:11
@pratikspatil024
Copy link
Copy Markdown
Member Author

@claude addressed both nits in the follow-up commit:

  • Updated the AGENTS.md scope row to include params/protocol_params.go via params/{config,protocol_params}.go.
  • Added an explicit note that the standalone AGENTS hardfork section intentionally summarizes .claude/rules/hardfork-rollout.md and should be kept in sync with it.

Thanks for the review.

@claude
Copy link
Copy Markdown

claude Bot commented May 19, 2026

Claude finished @pratikspatil024's task in 29s —— View job


Verifying follow-up commit

  • Verify nit 1: scope row updated to include params/protocol_params.go
  • Verify nit 2: sync note added to standalone AGENTS section
  • Post confirmation

Confirmed both nits addressed in db97666:

  1. Scope row — updated from params/config.go to params/{config,protocol_params}.go in the table.
  2. Sync note — added the "intentionally summarizes / keep both surfaces in sync" preamble to the standalone section.

LGTM — nothing else from my side. Thanks for the quick follow-up.


View job

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread AGENTS.md Outdated
Comment thread .claude/rules/hardfork-rollout.md
  - include recursive core/vm paths in the hardfork rollout rule
  - make the AGENTS rule scope explicit and consistent
@sonarqubecloud
Copy link
Copy Markdown

@pratikspatil024 pratikspatil024 merged commit 3350958 into develop May 19, 2026
26 of 28 checks passed
@pratikspatil024 pratikspatil024 deleted the psp-pos-3598 branch May 19, 2026 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants