-
Notifications
You must be signed in to change notification settings - Fork 795
Fix #1029: correct left-associativity for arithmetic operators in scr… #1067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…ipt parser The scripting engine evaluated "A - B + C" as "A - (B + C)" instead of "(A - B) + C". The root cause was that string_concat's operand chain referenced math_sum, causing it to appear twice in lexy's operation list with conflicting binding power levels. Changed string_concat::operand from math_sum to math_prefix to eliminate the duplicate entry. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…lid scripts The bug (fixed in cb6c751) was that ValidateScript used a fixed char[2048] buffer for error messages, causing out-of-bounds reads when error output exceeded the buffer. The current code uses std::string with std::back_inserter. This adds a regression test using the original reproducer to ensure the fix holds. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Comparing against negative literals (e.g., "A!=-1") used to throw "operator cannot be mixed with previous operators" because the unary minus prefix was in a conflicting operator group. This was resolved by the #1029 fix (string_concat operand change). This commit adds a regression test covering both direct script evaluation and XML precondition usage. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdjusted scripting operator precedence for string concatenation; added and duplicated parser/unit tests; upgraded untyped blackboard string entries to typed values during XML parsing; extended LoopNode to accept/convert std::vector and string queues and expose dequeued output; added JSON vector parsing; multiple new port/node tests; added FIXME issue tracker. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
…g remapping When a SubTree passes a constant string literal (e.g., queue="1;2;3") to a child tree's port, the subtree remapping stores it as a plain std::string in the child blackboard without type information. Later, when a strongly-typed node like LoopDouble reads the entry, Any::cast<SharedQueue<double>>() fails because there's no conversion path from string to the expected type. Fix: in createNodeFromXML, when initializing port entries, detect when an existing blackboard entry is weakly typed but the port manifest declares a strong type. In this case, convert the string value using the port's parseString() method and upgrade the entry's type info in place. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ions
When a port of type std::vector<T> has a default empty value {} and no
input is specified in XML, toStr() converts the empty vector to "json:[]".
The explicit convertFromString specializations for vector types did not
handle the "json:" prefix (unlike the generic template), so this string
was split by ';' producing a vector with one literal "json:[]" element.
Fix: add "json:" prefix detection at the top of all four vector
convertFromString specializations (int, double, bool, string). When
detected, parse directly with nlohmann::json instead of splitting by ';'.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
LoopNode's queue port now uses an untyped BidirectionalPort to avoid port type mismatch when upstream nodes produce std::vector<T>. At runtime, tick() tries SharedQueue<T> first, then falls back to std::vector<T> (converted to deque on first read), then string (via convertFromString). This allows upstream nodes that output vector<T> to feed directly into LoopNode without manual conversion. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/behaviortree_cpp/decorators/loop_node.h (1)
59-109:⚠️ Potential issue | 🟠 MajorReset dynamic queues on IDLE to avoid stale vector/string inputs.
For vector/string sources,
current_queue_stays non-null after the first run, so the!current_queue_guard prevents re-reading updated port values on subsequent executions. Reset the queue when the node re-entersIDLE(and the port isn’t static) so each run can refresh from the port.🐛 Suggested fix
if(status() == NodeStatus::IDLE) { child_running_ = false; // special case: the port contains a string that was converted to SharedQueue<T> if(static_queue_) { current_queue_ = std::make_shared<std::deque<T>>(); *current_queue_ = *static_queue_; } + else + { + current_queue_.reset(); + } }
🤖 Fix all issues with AI agents
In `@FIXME.md`:
- Around line 12-23: Wrap all bare URLs in FIXME.md for the listed entries
(`#1029` and `#923`) to satisfy markdownlint MD034 by enclosing them in angle
brackets or using markdown link syntax; update both occurrences shown (the URLs
for the GitHub issues) so lines like - **URL:** https://github.com/... become -
**URL:** <https://github.com/...> (or [text](https://...)) and apply the same
wrapping consistently for any other bare URLs in the file.
- Around line 244-258: The markdown table separator row lacks spaces around the
pipe characters; update the separator row beneath the header "| Category | Count
| Issue Numbers |" so each pipe has a space on both sides (e.g., change
"|----------|-------|---------------|" to a separator with spaces around the
pipes and hyphens for each column) to satisfy MD060 table style.
| ### [x] #1029 - Mathematical order of operations in script is wrong | ||
| - **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029 | ||
| - **Summary:** `A - B + C` is evaluated as `A - (B + C)` instead of left-to-right. The expression parser applies incorrect associativity for addition and subtraction at the same precedence level. | ||
| - **Component:** `src/scripting/` | ||
| - **Test file:** `tests/gtest_scripting.cpp` (or new) | ||
|
|
||
| ### [x] #923 - Out-of-bounds read in script validation | ||
| - **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/923 | ||
| - **Summary:** `ValidateScript` can create a `std::string` from a non-NULL-terminated fixed-size stack buffer when parsing invalid scripts that generate large error messages, causing `strlen` to read out of bounds. | ||
| - **Component:** `src/scripting/` | ||
| - **Test file:** `tests/gtest_scripting.cpp` (or new) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap bare URLs to satisfy markdownlint (MD034).
markdownlint flags bare URLs throughout the file; please wrap them in <...> or use link syntax (apply consistently to all entries).
🛠️ Example fix
- **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029
+ **URL:** <https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### [x] #1029 - Mathematical order of operations in script is wrong | |
| - **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029 | |
| - **Summary:** `A - B + C` is evaluated as `A - (B + C)` instead of left-to-right. The expression parser applies incorrect associativity for addition and subtraction at the same precedence level. | |
| - **Component:** `src/scripting/` | |
| - **Test file:** `tests/gtest_scripting.cpp` (or new) | |
| ### [x] #923 - Out-of-bounds read in script validation | |
| - **URL:** https://github.com/BehaviorTree/BehaviorTree.CPP/issues/923 | |
| - **Summary:** `ValidateScript` can create a `std::string` from a non-NULL-terminated fixed-size stack buffer when parsing invalid scripts that generate large error messages, causing `strlen` to read out of bounds. | |
| - **Component:** `src/scripting/` | |
| - **Test file:** `tests/gtest_scripting.cpp` (or new) | |
| ### [x] `#1029` - Mathematical order of operations in script is wrong | |
| - **URL:** <https://github.com/BehaviorTree/BehaviorTree.CPP/issues/1029> | |
| - **Summary:** `A - B + C` is evaluated as `A - (B + C)` instead of left-to-right. The expression parser applies incorrect associativity for addition and subtraction at the same precedence level. | |
| - **Component:** `src/scripting/` | |
| - **Test file:** `tests/gtest_scripting.cpp` (or new) | |
| ### [x] `#923` - Out-of-bounds read in script validation | |
| - **URL:** <https://github.com/BehaviorTree/BehaviorTree.CPP/issues/923> | |
| - **Summary:** `ValidateScript` can create a `std::string` from a non-NULL-terminated fixed-size stack buffer when parsing invalid scripts that generate large error messages, causing `strlen` to read out of bounds. | |
| - **Component:** `src/scripting/` | |
| - **Test file:** `tests/gtest_scripting.cpp` (or new) |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 13-13: Bare URL used
(MD034, no-bare-urls)
[warning] 19-19: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In `@FIXME.md` around lines 12 - 23, Wrap all bare URLs in FIXME.md for the listed
entries (`#1029` and `#923`) to satisfy markdownlint MD034 by enclosing them in
angle brackets or using markdown link syntax; update both occurrences shown (the
URLs for the GitHub issues) so lines like - **URL:** https://github.com/...
become - **URL:** <https://github.com/...> (or [text](https://...)) and apply
the same wrapping consistently for any other bare URLs in the file.
| | Category | Count | Issue Numbers | | ||
| |----------|-------|---------------| | ||
| | Scripting / Script Parser | 3 | #1029, #923, #832 | | ||
| | Ports / Type Conversion | 5 | #1065, #982, #969, #948, #858 | | ||
| | Blackboard | 3 | #974, #942, #408 | | ||
| | Control Nodes (Parallel) | 3 | #1045, #1041, #966 | | ||
| | Control Nodes (Reactive) | 2 | #1031, #917 | | ||
| | XML Parsing | 4 | #979, #883, #880, #672 | | ||
| | BehaviorTreeFactory | 4 | #1046, #937, #934, #930 | | ||
| | Loggers | 1 | #1057 | | ||
| | Tree Execution | 1 | #686 | | ||
| | TreeNode | 1 | #861 | | ||
| | JSON Exporter | 1 | #989 | | ||
| | Build / Dependencies | 1 | #959 | | ||
| | Platform-Specific | 1 | #869 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix summary table pipe spacing (MD060).
The separator row should include spaces around pipes to satisfy the configured table style.
🛠️ Example fix
-|----------|-------|---------------|
+| -------- | ----- | ------------- |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Category | Count | Issue Numbers | | |
| |----------|-------|---------------| | |
| | Scripting / Script Parser | 3 | #1029, #923, #832 | | |
| | Ports / Type Conversion | 5 | #1065, #982, #969, #948, #858 | | |
| | Blackboard | 3 | #974, #942, #408 | | |
| | Control Nodes (Parallel) | 3 | #1045, #1041, #966 | | |
| | Control Nodes (Reactive) | 2 | #1031, #917 | | |
| | XML Parsing | 4 | #979, #883, #880, #672 | | |
| | BehaviorTreeFactory | 4 | #1046, #937, #934, #930 | | |
| | Loggers | 1 | #1057 | | |
| | Tree Execution | 1 | #686 | | |
| | TreeNode | 1 | #861 | | |
| | JSON Exporter | 1 | #989 | | |
| | Build / Dependencies | 1 | #959 | | |
| | Platform-Specific | 1 | #869 | | |
| | Category | Count | Issue Numbers | | |
| | -------- | ----- | ------------- | | |
| | Scripting / Script Parser | 3 | `#1029`, `#923`, `#832` | | |
| | Ports / Type Conversion | 5 | `#1065`, `#982`, `#969`, `#948`, `#858` | | |
| | Blackboard | 3 | `#974`, `#942`, `#408` | | |
| | Control Nodes (Parallel) | 3 | `#1045`, `#1041`, `#966` | | |
| | Control Nodes (Reactive) | 2 | `#1031`, `#917` | | |
| | XML Parsing | 4 | `#979`, `#883`, `#880`, `#672` | | |
| | BehaviorTreeFactory | 4 | `#1046`, `#937`, `#934`, `#930` | | |
| | Loggers | 1 | `#1057` | | |
| | Tree Execution | 1 | `#686` | | |
| | TreeNode | 1 | `#861` | | |
| | JSON Exporter | 1 | `#989` | | |
| | Build / Dependencies | 1 | `#959` | | |
| | Platform-Specific | 1 | `#869` | |
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 245-245: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 245-245: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
[warning] 245-245: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 245-245: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
[warning] 245-245: Table column style
Table pipe is missing space to the right for style "compact"
(MD060, table-column-style)
[warning] 245-245: Table column style
Table pipe is missing space to the left for style "compact"
(MD060, table-column-style)
🤖 Prompt for AI Agents
In `@FIXME.md` around lines 244 - 258, The markdown table separator row lacks
spaces around the pipe characters; update the separator row beneath the header
"| Category | Count | Issue Numbers |" so each pipe has a space on both sides
(e.g., change "|----------|-------|---------------|" to a separator with spaces
around the pipes and hyphens for each column) to satisfy MD060 table style.
…ations parseString<T>() had two bugs for enum types: 1. Null pointer dereference on config().enums when getInput() was called before the factory assigned the scripting enum registry. 2. Enums with a convertFromString<T> specialization (e.g. NodeType, PortDirection) were never dispatched to it — the fallback tried convertFromString<int> which fails for named strings like "Control". The fix adds a null check for config().enums, tries numeric parsing via std::from_chars for integer enum values, then falls back to convertFromString<T>() for types with user-provided specializations. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The bug (getInput throwing std::out_of_range when a port has a default value but is not specified in XML) was already fixed in the current code. getInputStamped now properly falls back to the manifest's default value using find() instead of at(). Adding a regression test to prevent future regressions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…scripts When blackboard values are stored as strings (e.g. via blackboard->set(key, value.dump())), scripting comparisons between two such entries used lexicographic ordering instead of numeric. This caused "9" < "10" to evaluate as false. The fix attempts numeric parsing via strtod when both operands in a comparison are strings. If both parse as valid numbers, numeric comparison is used; otherwise falls back to lexicographic comparison. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
When a port declares a default blackboard remapping (e.g., "{=}") but
the XML doesn't explicitly specify the port, getLockedPortContent
returned an empty AnyPtrLocked because no blackboard entry existed.
The fix lazily creates the blackboard entry using the port's type info
from the manifest when the entry is missing but the remapped key is
valid. This makes getLockedPortContent consistent with the expectation
that default-remapped ports produce usable locked references.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
debugMessage() only printed the remapping direction for subtree entries (e.g., "[local] remapped to port of parent tree [parent]") without showing the type of the parent blackboard entry. Now it resolves the parent entry and appends the type name, making the debug output useful for diagnosing subtree port remapping issues. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@include/behaviortree_cpp/tree_node.h`:
- Around line 436-454: The numeric parsing uses a fixed int (tmp) which can
truncate/wrap enums with different underlying types; change the temporary to the
enum's underlying type (use std::underlying_type_t<T>, possibly wrapped with
std::make_unsigned_t if you want unsigned parsing) and use that type with
std::from_chars, then static_cast<T>(tmp) on success; update the parsing block
that currently declares "int tmp" (and the std::from_chars call) to use the
underlying-type temporary so config().enums handling and the fallback to
convertFromString<T>(str) remain unchanged.
In `@src/tree_node.cpp`:
- Around line 482-496: The condition uses implicit pointer-to-bool conversion
for _p->config.manifest; change the check to an explicit null comparison so
clang-tidy passes. Update the if in the getAnyLocked/createEntry flow (the block
that checks result and manifest) to use (_p->config.manifest != nullptr) instead
of just _p->config.manifest; keep the rest of the logic around getAnyLocked,
manifest->ports.find(key), createEntry(bb_key, port_it->second) and the
subsequent getAnyLocked unchanged.
| // Check the ScriptingEnumsRegistry first, if available. | ||
| if(config().enums) | ||
| { | ||
| return static_cast<T>(it->second); | ||
| auto it = config().enums->find(str); | ||
| if(it != config().enums->end()) | ||
| { | ||
| return static_cast<T>(it->second); | ||
| } | ||
| } | ||
| else | ||
| // Try numeric conversion (e.g. "2" for an enum value). | ||
| int tmp = 0; | ||
| auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), tmp); | ||
| if(ec == std::errc() && ptr == str.data() + str.size()) | ||
| { | ||
| // hopefully str contains a number that can be parsed. May throw | ||
| return static_cast<T>(convertFromString<int>(str)); | ||
| return static_cast<T>(tmp); | ||
| } | ||
| // Fall back to convertFromString<T>, which uses a user-provided | ||
| // specialization if one exists. Issue #948. | ||
| return convertFromString<T>(str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, find and verify the file exists
git ls-files include/behaviortree_cpp/tree_node.hRepository: BehaviorTree/BehaviorTree.CPP
Length of output: 109
🏁 Script executed:
# Check the exact code at lines 436-454
sed -n '430,460p' include/behaviortree_cpp/tree_node.hRepository: BehaviorTree/BehaviorTree.CPP
Length of output: 1020
🏁 Script executed:
# Get more context around this function to understand the template signature
sed -n '420,460p' include/behaviortree_cpp/tree_node.hRepository: BehaviorTree/BehaviorTree.CPP
Length of output: 1339
🏁 Script executed:
# Check C++ standard used in the project
grep -r "CMAKE_CXX_STANDARD\|std_version\|std11\|std14\|std17\|std20" CMakeLists.txt | head -20Repository: BehaviorTree/BehaviorTree.CPP
Length of output: 99
🏁 Script executed:
# Verify std::underlying_type_t is available (C++11+)
rg "std::underlying_type" include/behaviortree_cpp/ --type cpp --type hRepository: BehaviorTree/BehaviorTree.CPP
Length of output: 1859
Use the enum's underlying type for numeric parsing, not int.
Line 446 parses all enum values into int, which silently truncates or wraps values from enums with wider or unsigned underlying types.
Suggested fix
- int tmp = 0;
- auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), tmp);
+ using Underlying = std::underlying_type_t<T>;
+ Underlying tmp{};
+ auto [ptr, ec] = std::from_chars(str.data(), str.data() + str.size(), tmp);🤖 Prompt for AI Agents
In `@include/behaviortree_cpp/tree_node.h` around lines 436 - 454, The numeric
parsing uses a fixed int (tmp) which can truncate/wrap enums with different
underlying types; change the temporary to the enum's underlying type (use
std::underlying_type_t<T>, possibly wrapped with std::make_unsigned_t if you
want unsigned parsing) and use that type with std::from_chars, then
static_cast<T>(tmp) on success; update the parsing block that currently declares
"int tmp" (and the std::from_chars call) to use the underlying-type temporary so
config().enums handling and the fallback to convertFromString<T>(str) remain
unchanged.
| const auto bb_key = std::string(*remapped_key); | ||
| auto result = _p->config.blackboard->getAnyLocked(bb_key); | ||
| if(!result && _p->config.manifest) | ||
| { | ||
| // Entry doesn't exist yet. Create it using the port's type info | ||
| // from the manifest so that getLockedPortContent works even when | ||
| // the port is not explicitly declared in XML. Issue #942. | ||
| auto port_it = _p->config.manifest->ports.find(key); | ||
| if(port_it != _p->config.manifest->ports.end()) | ||
| { | ||
| _p->config.blackboard->createEntry(bb_key, port_it->second); | ||
| result = _p->config.blackboard->getAnyLocked(bb_key); | ||
| } | ||
| } | ||
| return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cat -n src/tree_node.cpp | sed -n '482,496p'Repository: BehaviorTree/BehaviorTree.CPP
Length of output: 830
🌐 Web query:
clang-tidy readability-implicit-bool-conversion rule documentation
💡 Result:
readability-implicit-bool-conversion is a clang-tidy readability check that flags implicit conversions between built-in types and bool (e.g., int→bool, pointer→bool, bool→int/float) because they can hide bugs and/or reduce readability; it also provides fix-it hints (e.g., if (i) → if (i != 0), if (p) → if (p != nullptr), while (1) → while (true)). [1]
Options (common)
AllowIntegerConditions(default0): allow integer-to-bool in conditions. [1]AllowPointerConditions(default0): allow pointer-to-bool in conditions. [1]
Name note
This check was renamed from readability-implicit-bool-cast to readability-implicit-bool-conversion. [2]
How to set options
Via .clang-tidy or -config with CheckOptions, e.g. enabling pointer conditions: [3]
clang-tidy -config="{Checks: '-*,readability-implicit-bool-conversion',
CheckOptions: [{key: readability-implicit-bool-conversion.AllowPointerConditions, value: 1}]}" \
file.cpp -- -std=c++20Sources: LLVM/Clang-Tidy docs for the check and its options [1], rename notice [2], and a working -config example [3].
Make the pointer check explicit to satisfy clang-tidy.
Line 484 uses implicit pointer-to-bool conversion on _p->config.manifest, triggering readability-implicit-bool-conversion.
Suggested fix
- if(!result && _p->config.manifest)
+ if(!result && _p->config.manifest != nullptr)🧰 Tools
🪛 GitHub Actions: pre-commit
[warning] 484-484: clang-tidy: readability-implicit-bool-conversion: implicit conversion from 'const TreeNodeManifest *' to 'bool' detected.
🤖 Prompt for AI Agents
In `@src/tree_node.cpp` around lines 482 - 496, The condition uses implicit
pointer-to-bool conversion for _p->config.manifest; change the check to an
explicit null comparison so clang-tidy passes. Update the if in the
getAnyLocked/createEntry flow (the block that checks result and manifest) to use
(_p->config.manifest != nullptr) instead of just _p->config.manifest; keep the
rest of the logic around getAnyLocked, manifest->ports.find(key),
createEntry(bb_key, port_it->second) and the subsequent getAnyLocked unchanged.
…ipt parser
The scripting engine evaluated "A - B + C" as "A - (B + C)" instead of "(A - B) + C". The root cause was that string_concat's operand chain referenced math_sum, causing it to appear twice in lexy's operation list with conflicting binding power levels. Changed string_concat::operand from math_sum to math_prefix to eliminate the duplicate entry.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.