Repository navigation
Extract NodeState basic hook callbacks - #8509
Merged
Merged
Conversation
Move the six setup_basic_hooks callbacks into typed member helpers and retain small binding lambdas without changing hook semantics. Remove only the registration function's cognitive-complexity suppression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
October 5, 2026 20:16
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The mechanical extraction preserves callback types, order, captures, return behavior, and existing validation coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Extracts six NodeState hook callbacks into private helpers to reduce cognitive complexity without changing behavior.
Changes:
- Adds concretely typed callback helpers.
- Retains hook order and forwarding semantics while removing the suppression.
Custom instructions used: .github/copilot-instructions.md, .github/instructions/reviewing.instructions.md, .github/skills/testing/SKILL.md
| File | Description |
|---|---|
src/node/node_state.h |
Extracts hook implementations and simplifies registration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Max (maxtropets)
approved these changes
Oct 6, 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
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.
Motivation
The inline callbacks in
NodeState::setup_basic_hooksmake the registration function exceed the cognitive-complexity threshold. Extracting them removes this function's baseline suppression without changing hook behavior.Related to #7358. This is an independent change based on main at
a6fe2f2fc, not stacked on #8504, and does not resolve the remaining backlog.Implementation summary
Move all six callback bodies into concretely typed member helpers and retain small
[this]forwarding lambdas. Remove only thesetup_basic_hookscomplexity suppression; no other suppressions, check configuration, logging sample code, or join/recovery control flow are changed.Actual clang-tidy 18.1.8 measurements with
IgnoreMacros=true:setup_basic_hooksfalls from 66 to 0, and each of its six forwarding lambdas scores 0. A threshold-zero diagnostic probe measured the positive scores below and confirmed no score above zero for the registration function or adapters; the repository threshold remains 50.on_secrets_local_commiton_secrets_global_commiton_nodes_global_commiton_node_endorsed_certificates_local_commiton_node_endorsed_certificates_global_commiton_service_global_commitEvery helper and retained lambda independently remains below the threshold.
Safety and compatibility
This is a mechanical internal refactor with no API, ledger format, mixed-version, or intended behavior changes. Callback bodies are token-identical apart from whitespace, and hook registration order, map names, wrapper types, captures, and all code outside the extracted region are unchanged. Typed write sets remain synchronously borrowed, and map hooks return the same null consensus hooks. Ledger secret filtering/decryption/version handling, recovery restrictions and transitions, commit/rollback timing, certificate locking/renewal, deferred frontend opening, retirement notification, and exception/logging behavior are preserved.
Validation:
CLANG_TIDY=ONand clang-tidy 18.1.8. The affectedCMakeFiles/ccf.dir/src/enclave/main.cpp.opassed the full configured checks.kv_test,ledger_test,ledger_secrets_test,encryptor_test,logging,logging_cose_only,logging_cose_only_allow_join_dual, andjs_genericwith the full configured tidy checks.build-tidy/tests.sh:kv_test,ledger_test,ledger_secrets_test,encryptor_test,recovery_test,recovery_test_suite(rekey/recovery and membership/recovery interleavings),governance_test(including node/service certificate renewal), andnodes_test.scripts/ci-checks.shpassed without auto-fix, including C++ formatting, ASCII, and all other local repository checks. Token/registration comparisons andgit diff --checkalso passed.Environment note: the initial broader build with the AUTO-selected
std::stacktracebackend on this Clang 21 host failed on the pre-existingsrc/tasks/worker.cpp:83bugprone-empty-catchdiagnostic. Reconfiguring the same build intentionally with the supported backend passed configuration, the full selected build, and all selected tests without modifying baseline code or disabling checks:cmake -S . -B build-tidy -GNinja -DCMAKE_BUILD_TYPE=Debug -DCLANG_TIDY=ON -DCCF_STACKTRACE_BACKEND=LIBBACKTRACEThere are no remaining local validation blockers; required CI still needs to pass before merge.