Introduce layer_path class - #640
Conversation
Led to a fair number of components using layer_path, but drivers still use strings to avoid changing API.
✅ 7 CodeQL alerts resolved compared to main
Review the full CodeQL report for details. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3077e35b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #640 +/- ##
==========================================
+ Coverage 83.27% 84.05% +0.78%
==========================================
Files 162 166 +4
Lines 5912 6128 +216
Branches 670 681 +11
==========================================
+ Hits 4923 5151 +228
+ Misses 796 785 -11
+ Partials 193 192 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ea55cdadf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7231923365
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds phlex::experimental::layer_path and phlex::experimental::bulleted_list, migrates model storage/APIs and core routing/counting to use layer_path (including hashing and suffix/prefix checks), and replaces manual multi-item runtime-error rendering with bulleted_list; plugins and tests are updated accordingly. ChangesLayer path type migration with bulleted list utilities
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 562791c8be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@phlex/model/data_cell_index.cpp`:
- Around line 67-75: Add a direct include for the C assert header in
phlex/model/data_cell_index.cpp so the new use of assert(ptr) does not rely on
transitive includes; insert `#include` <cassert> at the top of the translation
unit (near other includes) to ensure assert is defined for the code that
iterates layers using ptr, layer_name(), and parent_.get().
In `@phlex/model/data_layer_hierarchy.cpp`:
- Around line 4-5: The TU uses std::views::transform in data_layer_hierarchy.cpp
(around the code that constructs transformed views) but relies on an indirect
include; make the translation unit self-contained by adding a direct `#include`
<ranges> at the top of phlex/model/data_layer_hierarchy.cpp so the usage of
std::views::transform and other std::views utilities resolve without depending
on bulleted_list.hpp; update the include section near the existing `#include`
"phlex/utilities/bulleted_list.hpp".
In `@phlex/model/layer_path.cpp`:
- Around line 15-27: The constructor layer_path::layer_path builds layer_path_
from path then calls is_complete() which assumes layer_path_ has at least one
element, causing UB for inputs like "/" where layer_path_ is empty; fix by
guarding that call — either check layer_path_.empty() before calling
is_complete() or explicitly validate/throw for an empty layer_path_ prior to
is_complete(); update the constructor to use if (path.starts_with("/") &&
!layer_path_.empty() && not is_complete()) or add an early empty check that
throws, referencing layer_path::layer_path, layer_path_, is_complete(), and
validate() so the empty-path case is rejected safely.
- Around line 86-97: layer_path::hashes() fails to include the implicit "/job"
parent hash for incomplete paths causing ancestor mismatch with
layer_path::hash(); update hashes() to always insert the root job hash when the
path is not complete so that both complete and incomplete inputs produce the
same ancestor set: detect when is_complete() is false and ensure you insert
"job"_idq.hash (or the equivalent experimental::hash("job"_idq.hash,
layer_path_[0].hash()) as appropriate) as the first ancestor before iterating
the rest of layer_path_ (use the same cumulative_hash logic as in hash() to
maintain consistency with experimental::hash and cumulative_hash updates).
In `@phlex/model/layer_path.hpp`:
- Around line 6-10: This header is not self-contained: add the missing standard
headers so the code compiles without relying on transitive includes—include
<utility> for std::move, include <concepts> for std::constructible_from and
std::same_as used in the template constraints, and include <compare> for
three-way comparison operators; update the top-of-file includes accordingly so
symbols referenced in layer_path.hpp (std::move, std::constructible_from,
std::same_as, and any operator<=>/comparison usage) are provided directly.
In `@phlex/utilities/bulleted_list.hpp`:
- Around line 21-42: The bulleted_list templates unconditionally prepend a
prefix causing bulleted_list(rng) to return " - " for empty ranges; update both
overloads (template <detail::range_of_formattable R> std::string
bulleted_list(...) and the template <detail::range_of_to_stringable R>
requires(...) overload) to early-return an empty string when the input range is
empty (use std::ranges::empty(rng) or check begin==end) before constructing
prefix/prefix_with_newline and joining entries so empty ranges produce ""
instead of a dangling bullet.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: acce8139-cd0d-436b-9f27-59d3c0e7d815
📒 Files selected for processing (22)
phlex/core/framework_graph.cppphlex/core/index_router.cppphlex/core/index_router.hppphlex/core/input_arguments.hppphlex/core/producer_catalog.cppphlex/model/CMakeLists.txtphlex/model/data_cell_index.cppphlex/model/data_cell_index.hppphlex/model/data_layer_hierarchy.cppphlex/model/data_layer_hierarchy.hppphlex/model/fixed_hierarchy.cppphlex/model/fixed_hierarchy.hppphlex/model/handle.hppphlex/model/identifier.hppphlex/model/layer_path.cppphlex/model/layer_path.hppphlex/utilities/CMakeLists.txtphlex/utilities/bulleted_list.hppplugins/layer_generator.cpptest/CMakeLists.txttest/core_misc_test.cpptest/layer_path.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: build (gcc, none)
- GitHub Check: clang-tidy-check
- GitHub Check: coverage
🧰 Additional context used
🪛 Cppcheck (2.20.0)
phlex/model/layer_path.cpp
[style] 44-44: The function 'is_strict_prefix_of' is never used.
(unusedFunction)
[style] 56-56: The function 'ends_with' is never used.
(unusedFunction)
[style] 86-86: The function 'hashes' is never used.
(unusedFunction)
[style] 49-49: The function 'format_as' is never used.
(unusedFunction)
[style] 50-50: The function 'hash_value' is never used.
(unusedFunction)
🔇 Additional comments (13)
test/CMakeLists.txt (1)
32-32: LGTM!test/core_misc_test.cpp (1)
6-6: LGTM!Also applies to: 49-54
test/layer_path.cpp (1)
1-36: LGTM!phlex/core/index_router.hpp (1)
59-61: LGTM!Also applies to: 94-95
phlex/core/index_router.cpp (1)
4-5: LGTM!Also applies to: 33-34, 69-72, 102-121, 226-253
phlex/core/framework_graph.cpp (1)
73-74: LGTM!Also applies to: 139-140
phlex/core/producer_catalog.cpp (1)
79-82: LGTM!phlex/utilities/CMakeLists.txt (1)
15-27: LGTM!phlex/model/data_layer_hierarchy.hpp (1)
8-9: LGTM!Also applies to: 29-29, 43-50
phlex/model/fixed_hierarchy.hpp (1)
7-7: LGTM!Also applies to: 29-29, 91-91
phlex/model/fixed_hierarchy.cpp (1)
30-52: LGTM!Also applies to: 75-75, 98-100
plugins/layer_generator.cpp (1)
136-143: LGTM!phlex/model/handle.hpp (1)
98-98: LGTM!
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phlex/model/layer_path.cpp (1)
40-42: 🧹 Nitpick | 🔵 TrivialNo
_id/_idqlogic inconsistency—hashes line up for"job"
- In
phlex/model/identifier.cpp,"job"_idqbuilds anidentifier_queryfromidentifier::hash_string("job")(hash-only), while"job"_idconstructs anidentifierwith the samehash_stringinhash_.- Equality is compatible in both directions (
identifier == identifier_querycompares hash;identifier == identifiercompares hash then content), sovalidate()’s"job"_idcontains check andis_complete()’s"job"_idqcompletion check won’t diverge.- Optional:
validate()could use"job"_idqto avoid constructing a literalidentifier, but it’s not required for correctness.🤖 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 `@phlex/model/layer_path.cpp` around lines 40 - 42, The check in validate() uses a literal identifier `"job"_id` while elsewhere completion checks use `"job"_idq`; to keep construction semantics consistent (and avoid constructing a full identifier), change the contains() call in phlex::model::LayerPath::validate (referencing layer_path_) to use the identifier_query form `"job"_idq` instead of `"job"_id`; ensure the std::ranges::contains call compares against std::span{layer_path_}.subspan(1) with `"job"_idq` so equality uses the hash-only path and behavior remains identical.
♻️ Duplicate comments (1)
phlex/model/layer_path.cpp (1)
89-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
hashes()still omits the/jobroot ancestor for incomplete paths.The previous review correctly identified this: for an incomplete path like
"subrun/event"(which implies/job/subrun/event),hashes()returns{hash("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/job/subrun"), hash("/job/subrun/event")}but misseshash("/job"). Meanwhilehash()correctly treats the same path as rooted at/job.This will break downstream
fixed_hierarchylookups that expect all ancestor hashes to be present (see context:index_router.cppuseslayer_path::hashes()for membership tests).The fix from the previous review still applies:
Proposed fix
std::set<std::size_t> hashes; - // Add the appropriate first hash - std::size_t cumulative_hash = - is_complete() ? "job"_idq.hash : experimental::hash("job"_idq.hash, layer_path_[0].hash()); + std::size_t cumulative_hash = "job"_idq.hash; hashes.insert(cumulative_hash); - for (auto const& name : layer_path_ | std::views::drop(1)) { + for (auto const& name : layer_path_ | std::views::drop(is_complete() ? 1 : 0)) { cumulative_hash = experimental::hash(cumulative_hash, name.hash()); hashes.insert(cumulative_hash); }🤖 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 `@phlex/model/layer_path.cpp` around lines 89 - 100, layer_path::hashes() currently fails to include the "/job" ancestor for incomplete paths; update it so when is_complete() is false you explicitly insert the root hash ("job"_idq.hash) before computing cumulative hashes for the rest of layer_path_: compute and insert the root hash, then initialize cumulative_hash by hashing the root with the first element (using experimental::hash("job"_idq.hash, layer_path_[0].hash()) or equivalent), and proceed with the loop as before so all ancestor hashes including "/job" are present for downstream fixed_hierarchy lookups.
🤖 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.
Outside diff comments:
In `@phlex/model/layer_path.cpp`:
- Around line 40-42: The check in validate() uses a literal identifier
`"job"_id` while elsewhere completion checks use `"job"_idq`; to keep
construction semantics consistent (and avoid constructing a full identifier),
change the contains() call in phlex::model::LayerPath::validate (referencing
layer_path_) to use the identifier_query form `"job"_idq` instead of `"job"_id`;
ensure the std::ranges::contains call compares against
std::span{layer_path_}.subspan(1) with `"job"_idq` so equality uses the
hash-only path and behavior remains identical.
---
Duplicate comments:
In `@phlex/model/layer_path.cpp`:
- Around line 89-100: layer_path::hashes() currently fails to include the "/job"
ancestor for incomplete paths; update it so when is_complete() is false you
explicitly insert the root hash ("job"_idq.hash) before computing cumulative
hashes for the rest of layer_path_: compute and insert the root hash, then
initialize cumulative_hash by hashing the root with the first element (using
experimental::hash("job"_idq.hash, layer_path_[0].hash()) or equivalent), and
proceed with the loop as before so all ancestor hashes including "/job" are
present for downstream fixed_hierarchy lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e1ba76a2-c4c1-4f3a-bbdf-6df43af41fdf
📒 Files selected for processing (1)
phlex/model/layer_path.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: build (gcc, none)
- GitHub Check: coverage
- GitHub Check: clang-tidy-check
🔇 Additional comments (5)
phlex/model/layer_path.cpp (5)
22-24: LGTM!
45-45: LGTM!
47-57: LGTM!
59-72: LGTM!
74-87: LGTM!
- _idq is now consteval - _idq has a function call operator allowing it to be used as a callable in ranges::any_of
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
phlex/model/fixed_hierarchy.cpp (1)
43-53: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReject empty layer-path entries before building hashes.
convert_vector_vector_string()happily turns{}into an emptyexperimental::layer_path, andbuild_hashes()immediately callspath.hashes()on it. Inphlex/model/layer_path.cpp:80-102,layer_path::hashes()indexeslayer_path_[0]for incomplete paths, sofixed_hierarchy{{}}becomes an out-of-bounds read instead of a clean validation error.Possible fix
std::vector<phlex::experimental::layer_path> convert_vector_vector_string( std::vector<std::vector<std::string>>&& layer_paths) { using namespace phlex::experimental; return std::move(layer_paths) | std::views::transform([](std::vector<std::string>& lp) { + if (lp.empty()) { + throw std::invalid_argument("Fixed hierarchy layer paths must be non-empty."); + } auto lp_as_ids = lp | std::views::transform([](auto& str) { return identifier(std::move(str)); }) | std::ranges::to<std::vector>(); return layer_path(std::move(lp_as_ids)); }) | std::ranges::to<std::vector<layer_path>>(); }🤖 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 `@phlex/model/fixed_hierarchy.cpp` around lines 43 - 53, convert_vector_vector_string currently converts empty inner vectors into empty experimental::layer_path objects which later cause out-of-bounds access in layer_path::hashes; modify convert_vector_vector_string to reject or filter out empty layer-path entries before constructing layer_path: when mapping over layer_paths in convert_vector_vector_string, skip any inner std::vector<std::string> that is empty (or explicitly throw a validation error) rather than creating a layer_path from it, and keep using identifier(...) and layer_path(...) for non-empty entries so downstream calls to layer_path::hashes() never see an empty layer_path.
🤖 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 `@phlex/model/fixed_hierarchy.cpp`:
- Around line 35-39: build_hashes() currently always seeds the set with
"job"_idq.hash, so fixed_hierarchy constructed with an empty layer_paths yields
a non-empty layer_hashes_; change build_hashes() (and the other similar block
around where hashes is initialized/merged) to return an empty
std::set<std::size_t> when layer_paths.empty(), or alternatively defer inserting
"job"_idq.hash until after you iterate and see at least one configured path;
ensure layer_hashes_ remains empty for fixed_hierarchy({}) by only merging
path.hashes() into hashes and then adding "job"_idq.hash conditionally when
there was at least one path.
---
Outside diff comments:
In `@phlex/model/fixed_hierarchy.cpp`:
- Around line 43-53: convert_vector_vector_string currently converts empty inner
vectors into empty experimental::layer_path objects which later cause
out-of-bounds access in layer_path::hashes; modify convert_vector_vector_string
to reject or filter out empty layer-path entries before constructing layer_path:
when mapping over layer_paths in convert_vector_vector_string, skip any inner
std::vector<std::string> that is empty (or explicitly throw a validation error)
rather than creating a layer_path from it, and keep using identifier(...) and
layer_path(...) for non-empty entries so downstream calls to
layer_path::hashes() never see an empty layer_path.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6ff6f648-ce41-4820-8cc0-bd3139ed20a2
📒 Files selected for processing (1)
phlex/model/fixed_hierarchy.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (gcc, none)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: clang-tidy-check
- GitHub Check: coverage
The |
Clang-Tidy Check ResultsFound 5939 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
|
Should be good now, I think |
This reverts commit 732b4bd.
This reverts commit 74dc348.
3 participants
The new
layer_pathclass is used inindex_routerand the related infrastructure.I've kept strings and
vector<string>s for now in drivers. That might be the next step.CI / CodeQL
Code (Core Framework)
Code (Model / Data Structures)
Utilities
Build System
Tests
Plugins
Public API / Signature Changes (notable)
Notes / Review Impact