Repository navigation
Make public header components acyclic, and forbid new cycles - #8478
Draft
Amaury Chamayou (achamayou) wants to merge 3 commits into
Draft
Amaury Chamayou (achamayou) wants to merge 3 commits into
Amaury Chamayou (achamayou) wants to merge 3 commits into
Conversation
The source dependency checker only scanned src/, so public headers under include/ccf could still include each other in cycles between components, and could include private headers with angle brackets. Break the three cycles by moving declarations down, without renaming anything: the base64 declarations to ccf/ds/base64.h, the BlitSerialiser<Sha256Hash> specialisation next to its primary template, and the DID and Feed aliases to ccf/pal/uvm_endorsements.h. Assign endpoint.h, endpoint_context.h and common_auth_policies.h to endpoints, tx.h to kv, and ccf/indexing/strategies to its own component. Keep ccf/tx.h providing the typed KV maps, and stop ccf/research/create_tx_claims_digest.h from including the private kv/kv_types.h. The checker now treats public headers as their own layer: they must only include public headers, and their components must not form a cycle. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
September 30, 2026 21:43
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Four new changelog entries lack the required #8478 pull-request reference.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Extends dependency enforcement to public headers and removes existing component cycles without changing runtime behavior.
Changes:
- Adds public-header dependency and private-include validation.
- Relocates declarations and adds compatibility includes.
- Updates release notes and checker documentation.
Custom instructions used: .github/copilot-instructions.md, .github/instructions/reviewing.instructions.md, .github/instructions/changelog.instructions.md, and the formatting/testing skills.
| File | Description |
|---|---|
.github/skills/formatting-and-linting/SKILL.md |
Updates include-check coverage. |
CHANGELOG.md |
Documents header and dependency changes. |
include/ccf/crypto/base64.h |
Forwards to the new DS declaration header. |
include/ccf/crypto/sha256_hash.h |
Removes the KV dependency. |
include/ccf/ds/base64.h |
Adds relocated base64 declarations. |
include/ccf/ds/json.h |
Uses the lower-layer base64 header. |
include/ccf/kv/serialisers/blit_serialiser.h |
Relocates the SHA-256 specialization. |
include/ccf/pal/uvm_endorsements.h |
Owns DID and Feed aliases. |
include/ccf/research/create_tx_claims_digest.h |
Replaces a private include. |
include/ccf/service/tables/uvm_endorsements.h |
Imports aliases from PAL. |
include/ccf/tx.h |
Makes required and compatibility includes explicit. |
scripts/check-source-dependencies.py |
Checks public-header layering and cycles. |
scripts/includes-checks.sh |
Describes expanded dependency checks. |
scripts/source-dependencies.json |
Adds public component ownership overrides. |
src/node/internal_tables_access.h |
Adds its direct table dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ccf/tx.h used to provide ccf/kv/unit_value.h transitively, through ccf/service/map.h. Include it with the other typed KV containers, so that ccf/tx.h and its includers only lose the ccf::Service* aliases, and list ccf::ServiceUnit among them in the changelog. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This branch has not been deployed
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
Follow-up to #8476, for #3517.
#8476 made the
src/component graph acyclic, butcheck-source-dependencies.pyonly scanssrc/. Underinclude/ccf/, three groups of public header components still include each other in cycles. For example,ccf/ds/json.hincludesccf/crypto/base64.h, andccf/crypto/sha256_hash.hincludesccf/service/map.hfor a single serialiser, so the lowest-level headers pull in the whole typed KV API. Nothing stops new cycles either.ccf/research/create_tx_claims_digest.halso included the private<kv/kv_types.h>, which is not installed, so applications could not use it.includes-checks.shonly checks quoted includes, so it missed this.The resulting public header layering, and the includes this PR removes
Each node is a public header component, a directory under
include/ccf/.+lists the top-level headers that an override assigns to a component. Other top-level headers are left out: an arrow means that a component's headers include another's, directly or through top-level headers. As in #8476, an arrow is omitted when a longer path implies it.The dashed red arrows are the includes this PR removes. Three pointed up the layering and closed the cycles between
crypto,ds,kv,palandservice, and one reached into a private header. Their replacements follow existing arrows:crypto -> dsfor base64,kv -> cryptofor the serialiser, andservice -> palforDIDandFeed. The cycles throughendpoint.handnode_context.hare resolved by the overrides instead.Implementation summary
Public headers are checked as their own layer, not as part of the
src/component of the same name. A component's interface can legitimately be used by components that its implementation depends on:ccf/node_context.hexposes the indexing interface, whilesrc/indexingdepends onnode. The checker now also requires that:include/ccf/is a component and each top-level header is its own, unlesspublic_component_overridesinsource-dependencies.jsonsays otherwise.Since public headers cannot include private ones and the
src/policy is acyclic, no cycle can span the two layers, so the whole include graph between components is acyclic.Three declarations move down a layer to break the cycles. No names, namespaces or signatures change:
ds -> crypto): theccf::cryptobase64 declarations move to a newccf/ds/base64.h, whichccf/crypto/base64.hincludes.json.hmust keep encodingstd::vector<uint8_t>as base64, so this dependency can only move, not disappear.BlitSerialiser<Sha256Hash>(crypto -> service): the specialisation moves next to its primary template inccf/kv/serialisers/blit_serialiser.h, soccf/crypto/sha256_hash.hno longer includesccf/service/map.h. It is now visible whereverBlitSerialiseris.ccf::DIDandccf::Feed(pal -> service): thesestd::stringaliases were all thatccf/pal/uvm_endorsements.hused fromccf/service/tables/uvm_endorsements.h. They move to thepalheader, which the table header now includes.Five ownership overrides settle the remaining cycles without code changes.
endpoint.h,endpoint_context.handcommon_auth_policies.hbelong toendpoints, asccf/endpoints/authentication/js.hbuilds on them.tx.hbelongs tokv.ccf/indexing/strategies/is its own component, as strategies take anAbstractNodeContext.To limit source breaks:
ccf/tx.hincludesccf/kv/map.h,set.h,unit_value.handvalue.h, which it used to provide transitively. It also includesccf/kv/abstract_handle.h, which it always needed: it destroys astd::unique_ptr<AbstractHandle>of a forward-declared type, and only compiled because the full type arrived transitively.ccf/research/create_tx_claims_digest.hincludesccf/claims_digest.handccf/tx.hinstead of the private header.src/node/internal_tables_access.hincludes the UVM endorsements table header it uses.The two scans share their include parsing, and report source and public violations together, with one include per edge of a cycle. On
main, the checker reports:Validated locally:
-Werror(clang 21, syntax only), as onmain. Only declarations moved, so nothing was linked or run; CI covers the tests.main(for example,ccf/rest_verb.husesfmtwithout including it).ccf/pal/uvm_endorsements.handccf/research/create_tx_claims_digest.hfailed onmainand now compile, as does the newccf/ds/base64.h.pal -> serviceinclude, an angled private include, an unresolvedccf/include, a macro include, unknown overrides, and dropping thetx.hoverride.includes-checks.sh, clang-format 18.1.8, black, prettier, and the release notes, ASCII, copyright and TODO checks pass.Safety and compatibility
Runtime behaviour does not change. Every moved declaration and definition is unchanged, so there is no ABI, wire, ledger, KV or JSON format change, and no effect on mixed-version operation or recovery.
Sha256Hashkeys are still serialised as hex, and byte vectors as base64 in JSON.This is a source-only change, for code that relied on transitive includes.
CHANGELOG.mdlists it:ccf/tx.h, and headers that include it such asccf/rpc_context.handccf/node_context.h, no longer provideccf::ServiceMap,ccf::ServiceValue,ccf::ServiceSetorccf::ServiceUnit.ccf/crypto/sha256_hash.h,ccf/crypto/hash_provider.h,ccf/claims_digest.h,ccf/receipt.h,ccf/service/local_sealing.hand severalccf/pal/headers no longer provide the typed KV maps.ccf/app_interface.h,ccf/endpoint_registry.h,ccf/common_endpoint_registry.handccf/json_handler.hstill provide everything they did, and the samples compile unchanged.Losing transitive includes cannot silently change behaviour here. The headers that some includers lose contain only two specialisations,
fmt::formatter<ccf::ByteVector>andBlitSerialiser<Sha256Hash>, and each is in the header that declares its type or its primary template. Code that can name the type still sees the specialisation; anything else fails to compile.This lands in a patch release: 7.0.18 already contains a documented source break (#8309).