ci(config): enforce lowerCamelCase and max depth in reference.conf#30
Open
bladehan1 wants to merge 2 commits into
Open
ci(config): enforce lowerCamelCase and max depth in reference.conf#30bladehan1 wants to merge 2 commits into
bladehan1 wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Python CLI ChangesReference Configuration Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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 @.github/scripts/check_reference_conf.py:
- Around line 108-117: The parser only pushes the key prefix into frames when
the delimiter is a literal '{', so cases like "foo = {", "foo: {", or "foo = [ {
... } ]" lose the "foo" prefix; update the branch in the block around ident
handling (the code that checks p2 < n and src[p2] in '=:{') to treat a following
'{' or '[' as a nested-value indicator regardless of whether the delimiter was
'=' or ':' and push the full_parts prefix onto frames; if src[p2] == '{' push
{"kind":"obj","prefix":full_parts}, if src[p2] == '[' push
{"kind":"arr","prefix":full_parts}, and set pos = p2 + 1 in both cases so nested
object/array keys retain the parent key prefix.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac43e6ed-37ff-4ccc-b11a-352a56d2c404
📒 Files selected for processing (2)
.github/scripts/check_reference_conf.py.github/workflows/pr-check.yml
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/pr-check.yml:
- Around line 106-113: Pin the GitHub action and the pyhocon install to
immutable versions: replace uses: actions/setup-python@v5 with the action
referenced by its full commit SHA (e.g., actions/setup-python@<commit-sha>) and
change the pip install step to install a specific pyhocon release (e.g., pip
install pyhocon==<version>), updating the workflow steps that currently
reference actions/setup-python@v5 and the pip install command so the action and
dependency are immutable and reproducible.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 423dc7fa-48b7-40eb-80e8-3482cc0ee539
📒 Files selected for processing (2)
.github/scripts/check_reference_conf.py.github/workflows/pr-check.yml
bladehan1
force-pushed
the
ci/reference-conf-gate
branch
from
e7d540a to
52d2e77
Compare
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 @.github/scripts/check_reference_conf.py:
- Line 54: The constant MAX_DEPTH is set one too high; change MAX_DEPTH from 6
to 5 to enforce the PR contract of <= 5 depth (update the variable MAX_DEPTH in
this script and any tests or references that assume the limit); ensure any
related comparisons or error messages referencing MAX_DEPTH still reflect the
new value.
- Around line 84-95: The walk function currently only increments and emits the
synthetic array step (array_path/array_depth) when list elements are ConfigTree,
so pure scalar lists never report the array step; modify the list branch in walk
(the variables node, array_path, array_depth, seen) to always emit the
array_path once for scalar elements: detect if the list contains any
non-ConfigTree scalars (or no ConfigTree yielded subpaths) and yield array_path
with array_depth and sub_leaf=True (or appropriate leaf flag) exactly once
before/if you skip into per-element traversal, and still keep the existing logic
for ConfigTree elements while using seen to avoid duplicate sub_path emissions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b6d3ef0-c4ee-4a5f-b0ed-98edebc650b2
📒 Files selected for processing (2)
.github/scripts/check_reference_conf.py.github/workflows/pr-check.yml
bladehan1
force-pushed
the
ci/reference-conf-gate
branch
from
52d2e77 to
ff0474c
Compare
bladehan1
commented
May 20, 2026
bladehan1
force-pushed
the
ci/reference-conf-gate
branch
2 times, most recently
from
ef37233 to
cd96b77
Compare
Add a CI gate that scans common/src/main/resources/reference.conf and fails the build when any key violates lowerCamelCase (^[a-z][a-zA-Z0-9]*$ per dot-separated segment) or exceeds the maximum hierarchy depth (5). Array element keys are validated the same way; each array step counts as one depth level — e.g. an inner field at `rate.limiter.rpc[].component` is depth 5. Parsing is delegated to pyhocon, the reference Python HOCON implementation. It returns a fully-merged ConfigTree where dotted-form keys expand into nested objects — the same canonical key set Typesafe Config and ConfigBeanFactory see at runtime — and handles triple-strings, substitutions, includes, +=, and block comments without us re-implementing the grammar. Four legacy PBFT* keys are grandfathered via an in-script allowlist so the gate fails only on new violations. A consolidated GHA error annotation lists every offending key, and sys.exit(1) drives step failure. The script also accepts `--debug` to print every parsed key with its depth (trailing `/` marks namespace intermediates) for manual verification against the source file. Runs as a new step in the existing checkstyle job of pr-check.yml (setup-python + `pip install pyhocon`), so no extra runner spin-up.
bladehan1
force-pushed
the
ci/reference-conf-gate
branch
from
cd96b77 to
2b2fa79
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/scripts/check_reference_conf.py (1)
55-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the agreed depth gate (
MAX_DEPTH = 6).This change tightens the validator from the agreed
<= 6contract to<= 5, which can fail CI for a single additional nesting level that was intentionally buffered.Suggested fix
-# Set at the current max depth of reference.conf (5). No buffer: a mature -# project should not allow silent drift, so any new key going deeper must -# bump MAX_DEPTH via an explicit, reviewed change (deeper trees hurt -# readability and complicate ConfigBeanFactory mapping). -MAX_DEPTH = 5 +# Current observed max depth in reference.conf is 5; keep +1 buffer to avoid +# CI flakiness on a single incremental nesting change. Any increase beyond 6 +# must be an explicit, reviewed decision. +MAX_DEPTH = 6Based on learnings: In
.github/scripts/check_reference_conf.py, treatMAX_DEPTH = 6as an intentional safety buffer (current deepest path is 5), and avoid changing that contract silently.🤖 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 @.github/scripts/check_reference_conf.py around lines 55 - 60, Restore the agreed depth gate by changing the constant MAX_DEPTH from 5 back to 6 in the module (the line defining MAX_DEPTH = 5). Update the adjacent comment if needed to reflect that MAX_DEPTH is intentionally 6 as a safety buffer; leave KEY_REGEX unchanged. Ensure any unit tests or usages that depend on MAX_DEPTH reference this constant name so the validator allows up to 6 levels.
🤖 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.
Duplicate comments:
In @.github/scripts/check_reference_conf.py:
- Around line 55-60: Restore the agreed depth gate by changing the constant
MAX_DEPTH from 5 back to 6 in the module (the line defining MAX_DEPTH = 5).
Update the adjacent comment if needed to reflect that MAX_DEPTH is intentionally
6 as a safety buffer; leave KEY_REGEX unchanged. Ensure any unit tests or usages
that depend on MAX_DEPTH reference this constant name so the validator allows up
to 6 levels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78e58979-a8fb-439c-8c76-25ef78ff386f
📒 Files selected for processing (2)
.github/scripts/check_reference_conf.py.github/workflows/pr-check.yml
Pin pyhocon via .github/scripts/requirements.txt so the gate is not exposed to silent behavior changes in upstream releases, and enable actions/setup-python's pip cache (keyed off the requirements file) so subsequent CI runs skip the PyPI round-trip.
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
1 participant
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.
What does this PR do?
Adds a CI gate that scans
common/src/main/resources/reference.confand fails the build when any key violates either of:^[a-z][a-zA-Z0-9]*$(lowerCamelCase — required byConfigBeanFactoryauto-binding).<= 6. Each list/array step counts as one additional level — e.g.rate.limiter.rpc[].componentis depth 5 (rate=1, limiter=2, rpc=3, []=4, component=5).Implementation:
.github/scripts/check_reference_conf.py— parses via pyhocon, the reference Python HOCON implementation. Returns a fully-mergedConfigTreewhere dotted-form keys expand into nested objects — the same canonical key set Typesafe Config /ConfigBeanFactorysee at runtime. Handles triple-strings, substitutions, includes,+=, block comments, etc. without us re-implementing the grammar.rate.limiter.rpc = [{ component=..., ... }]) are validated the same way. Element keys are deduplicated across list entries because well-formed arrays use homogeneous object shapes.Validate reference.conf key names and depthstep in the existingcheckstylejob of.github/workflows/pr-check.yml— saves runner-minutes vs a standalone workflow.::error file=...) with all entries packed via%0Aso they show in the Checks panel;sys.exit(1)is the sole failure driver (noexit 1in the workflow shell wrapper).--debugflag prints every parsed key with its depth in walk order; trailing/marks namespace intermediates. Useful for manual verification against the source file.Why are these changes required?
Without an automated gate, key names in
reference.confhave drifted: 4 legacy keys (node.http.PBFTEnable,node.http.PBFTPort,node.rpc.PBFTEnable,node.rpc.PBFTPort) start with an uppercase acronym and require manual normalization inArgs.java. Each additional drift costs reviewer attention and risks silentConfigBeanFactorybinding failures.The 4 existing violators are grandfathered via an explicit
ALLOWLISTso the gate fails only on new violations.This PR has been tested by:
common/src/main/resources/reference.confreturnsOK: ... 294 keys, all lowerCamelCase, depth <= 6(exit 0). The 294 includes 13 array-element fields undergenesis.block.assets[],genesis.block.witnesses[], andevent.subscribe.topics[].::errorannotation; reverting passes again.--debugoutput manually eyeballed against the source file; depth column shows the[]array steps explicitly so MAX_DEPTH math is verifiable by inspection.Follow up
Args.javasince userconfig.conffiles use the legacy names).XxxConfig.javafield) — deliberately not coupled with this PR.Extra details
MAX_DEPTH = 6(current max is 5, one level of headroom).KEY_REGEX = ^[a-z][a-zA-Z0-9]*$; allowlist contains exactly the 4 PBFT keys.pBFTExpireNum,allowPBFT,isOpenFullTcpDisconnectstart with lowercase and pass the regex unchanged.reference.conf(e.g.node.shutdown.BlockTime) are not seen by pyhocon, so they don't need to be allowlisted.Summary by cubic
Adds a CI gate to enforce lowerCamelCase and a max depth of 5 for
reference.confkeys, including keys inside arrays. Usespyhocon, emits one consolidated::erroron violations, allowlists four legacyPBFT*keys, and supports--debug..github/scripts/check_reference_conf.pyin thecheckstylejob.pyhoconfrom.github/scripts/requirements.txtin.github/workflows/pr-check.yml.Written for commit 33928a0. Summary will update on new commits. Review in cubic
Summary by CodeRabbit