Repository navigation
Break the last source dependency cycle, and forbid new ones - #8476
Merged
Amaury Chamayou (achamayou) merged 1 commit intoSep 30, 2026
Conversation
Remove the eight js -> node includes, so the source component graph is acyclic: - Move the governance-only JS extensions (ccf.network, ccf.node and the GovEffectsExtension implementation) to node/gov/extensions. - Move gov_logging.h from node/rpc to ds. - Move UVM endorsement verification from node to pal, and the generic COSE/CWT helpers it uses from node/cose_common.h to crypto/cose_utils.h. - Drop a stale jwt_management.h include from converters.cpp. Add dependency policies for js and node, and make check-source-dependencies.py require a policy for every source component and reject policies that allow a cycle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
September 30, 2026 16:47
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It moves governance and attestation code across component boundaries, while the full governance test remains unconfirmed.
Review effort: Balanced
Findings: None
What changed in this PR
Removes the final js-to-node source dependency and enforces an acyclic dependency policy across all source components.
Changes:
- Relocates governance extensions, logging helpers, and UVM verification to appropriate lower-level components.
- Moves shared COSE/CWT utilities from
nodeintocrypto. - Adds complete dependency policies and rejects missing or cyclic policies.
Custom instructions used
.github/copilot-instructions.md.github/instructions/reviewing.instructions.md.github/skills/testing/SKILL.md.github/skills/formatting-and-linting/SKILL.md
| File | Description |
|---|---|
CMakeLists.txt |
Updates relocated source paths in libraries and tests. |
scripts/check-source-dependencies.py |
Rejects missing and cyclic dependency policies. |
scripts/source-dependencies.json |
Adds policies for js and node. |
src/crypto/cose_utils.h |
Hosts shared COSE errors and CWT utilities. |
src/ds/gov_logging.h |
Relocates governance logging macros. |
src/js/extensions/ccf/converters.cpp |
Replaces a node include with direct crypto includes. |
src/js/extensions/console.cpp |
Uses the relocated governance logger. |
src/js/extensions/snp_attestation.cpp |
Uses PAL-owned UVM verification. |
src/kv/README.md |
Points dependency documentation to enforced policy. |
src/node/cose_common.h |
Removes utilities moved into crypto. |
src/node/gov/extensions/gov_effects.cpp |
Relocates governance-effects implementation. |
src/node/gov/extensions/network.cpp |
Relocates network extension implementation. |
src/node/gov/extensions/network.h |
Relocates network extension declaration. |
src/node/gov/extensions/node.cpp |
Relocates node extension implementation. |
src/node/gov/extensions/node.h |
Relocates node extension declaration. |
src/node/gov/handlers/helpers.h |
Uses the relocated governance logger. |
src/node/gov/handlers/proposals.h |
Includes governance extensions from node. |
src/node/internal_tables_access.h |
Uses PAL-owned UVM verification. |
src/node/node_state.h |
Uses PAL-owned UVM verification. |
src/node/quote.cpp |
Uses PAL-owned UVM verification. |
src/node/rpc/member_frontend.h |
Removes unused JS extension dependencies. |
src/node/rpc/node_call_types.h |
Uses PAL-owned UVM verification. |
src/node/test/endorsements.cpp |
Updates the relocated UVM include. |
src/pal/test/verify_uvm_attestation_and_endorsements.cpp |
Updates the relocated UVM include. |
src/pal/test/verify_uvm_attestation_and_endorsements.h |
Updates the relocated UVM include. |
src/pal/uvm_endorsements.cpp |
Relocates UVM verification implementation. |
src/pal/uvm_endorsements.h |
Uses shared crypto COSE utilities. |
💡 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
Sep 30, 2026
Amaury Chamayou (achamayou)
deleted the
achamayou-issue-3517-clarify-dependencies-between-framework-s-350c39
branch
September 30, 2026 19:45
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
Closes #3517. This is the last step of the plan in the issue: remove
js -> node.Since #8475, the only cycle left in the source dependency graph is
js <-> node.jsis in it only because eight of its#includedirectives namenodeheaders. None of them is the JS runtime depending on the node. They are governance-only extensions that happen to live insrc/js/, two node headers that belong lower down, and one stale include. This PR moves them. The graph has no cycles, every component has a dependency policy, and the checker now rejects any change that would add a cycle back.js,nodeFollowing includes transitively,
src/js/now reaches 145 files outside itself instead of 200, and none innode(previously 9).The resulting layering (transitive reduction of the 81 edges)
An arrow is omitted when a longer path implies it.
msgpackandthreadinghave no internal dependencies in either direction.Implementation summary
Code moves only, one per removed include:
js/extensions/ccf/tonode/gov/extensions/(five includes). These areNetworkExtension(network.h/.cpp),NodeExtension(node.h/.cpp) and the implementation ofGovEffectsExtension(gov_effects.cpp). They installccf.network.*,ccf.node.*,ccf.setJwtPublicSigningKeys()and so on for the constitution'sapply(), and their only installer isnode/gov/handlers/proposals.h.GovEffectsExtensionkeeps its public declaration inccf/js/extensions/ccf/gov_effects.h, asbuild_receipt_for_committed_tx()did in Take endpoints out of the source dependency cycle #8475. All three stay in theccf_jslibrary, so linking does not change.member_frontend.hincludednetwork.handnode.hbut used neither. It now includesnode/network_state.h, which is what it needed from them.gov_logging.hmoves fromnode/rpc/tods/. It only defines theGOV_*_FMTmacros overds/internal_logger.h, the governance counterpart ofCCF_APP_*.console.cppuses them forconsole.log()in governance contexts.uvm_endorsements.h/.cpp) moves fromnode/topal/. The publicccf/pal/uvm_endorsements.halready declaredccf::pal::verify_uvm_endorsements_descriptor(), butnodeimplemented it.snp_attestation.cppneeds it forccf.snp_attestation.verifySnpAttestation(). The helpers it uses fromnode/cose_common.hare generic COSE/CWT parsing:COSEDecodeError,COSESignatureValidationError,CwtClaims,decode_cwt_claims()andvalidate_cwt_iat_against_x5chain(). They move tocrypto/cose_utils.h, next toparse_x5chain(), sopalstill only depends oncryptoandds.cose_common.hincludescose_utils.h, so its users see the same names, and the CCF receipt decoding stays innode. The moved header drops its unused direct include ofccf/service/tables/uvm_endorsements.h.ccf/pal/uvm_endorsements.hstill includes it for theDIDandFeedaliases.uvm_endorsements.cppis compiled into the same targets as before.converters.cppdropsnode/rpc/jwt_management.h. It used nothing JWT-specific, only thePem, verifier andSha256Hashdeclarations forccf.pemToId(). It now includes those directly.source-dependencies.jsongains policies forjsandnode, which are exactly the measured sets.check-source-dependencies.pynow also fails if:src/component has no policy, orgraphlib, and the error prints the cycle.The existing check matches every include against its component's policy. With both new checks, the include graph is a subgraph of an acyclic policy graph, so no cycle can form without a policy change that the checker rejects. For example, adding
nodetojs's policy fails withDependency policy allows a cycle: js -> node -> js. A new component such assrc/tracingfails until it gets a policy. This PR's policy applied tomain's sources reports all eightjs -> nodeincludes, andmain's policy applied to this checker reportsMissing dependency policy for: js, node.Finally,
src/kv/README.mdclaimed thatkvonly depends onds. It was one of the rotted dependency READMEs that opened this issue, and it now points to the enforced policy.Validated locally with a clang 21 Debug build:
js_test,js_policy_test,endorsements_test,cose_test,frontend_test,node_frontend_test,internal_tables_access_testandhistorical_queries_test.programmability_and_jwtpasses. It coversccf.snp_attestation.verifySnpAttestation(), with UVM endorsements verified insrc/pal/uvm_endorsements.cpp, and JWT signing keys set and removed through governance.includes-checks.sh(including the dependency checker), clang-format 18, black, prettier, gersemi, and the ASCII, copyright and TODO checks pass.governance_testdid not pass in a single local run, so CI needs to confirm it. Each of its nine sub-tests passed in at least one of four full runs. Every failure was a spurious election or a dropped connection (SessionConsistencyLost,PrimaryNotFound,RPC could not be forwarded to unknown primary,TransactionReplicationFailed,CCFConnectionException), in a different sub-test each time. This machine is slow for the test, with fsync stalls of up to 12s. With sub-tests run one at a time, onlymember_clientfailed, with a ballot rejected with 503 after an election. With the workspace on tmpfs,member_clientpassed and onlysession_coseauthlost its connection. None of the failures involves anapply()error, and the logs show the constitution calling the movedccf.node.*andconsolecode.Safety and compatibility
Production behaviour does not change. Every function and type keeps its name, namespace and body. Only the file that defines it changes.
ccf.network.*,ccf.node.*and JWT signing key functions,console.*logging, UVM endorsement verification for node joins andccf.snp_attestation.verifySnpAttestation(), and COSE receipt decoding run the same code as before..cppstays in the library or test targets it was built into, so applications compile and link as before.Nothing under
include/changes. There are no wire, ledger or KV format changes, and no effect on mixed-version operation or recovery. Nothing is user-facing, so there is noCHANGELOG.mdentry.