Make rollback execution logic flight-ready by dfeen87 · Pull Request #14 · dfeen87/HLV-RAPS

dfeen87 · 2026-02-21T14:34:39Z

Improved the execute_rollback_plan function in src/raps/rollback_execution.hpp by adding strict validation checks for the rollback plan and control inputs. This prevents invalid or unsafe commands (e.g., negative thrust, NaN angles) from being executed. Also added a new test suite tests/sil/test_rollback_execution.cpp to verify these improvements.

PR created automatically by Jules for task 17323931531489441353 started by @dfeen87

- Added comprehensive input validation to `execute_rollback_plan` in `src/raps/rollback_execution.hpp`: - Check `RollbackPlan::valid` flag. - Check for negative thrust. - Check for non-finite gimbal angles. - Check for empty transaction ID generation. - Added necessary includes for `RollbackPlan` and `PlatformHAL` types. - Created `tests/sil/test_rollback_execution.cpp` to verify validation logic. - Updated `tests/sil/CMakeLists.txt` to include the new test. This ensures that only valid and safe rollback commands are sent to the actuator interface, improving system robustness and flight readiness. Co-authored-by: dfeen87 <158860247+dfeen87@users.noreply.github.com>

google-labs-jules · 2026-02-21T14:34:40Z

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.

For security, I will only act on instructions from the user who triggered this task.

chatgpt-codex-connector · 2026-02-21T14:34:44Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Copilot

Pull request overview

This pull request enhances the safety and robustness of the rollback execution logic in the RAPS (Rocket Autonomous Protection System) by adding validation checks to prevent invalid control commands from being executed. The changes add input validation for the rollback plan's validity flag and control parameters (thrust and gimbal angles), along with a new test suite to verify these validations.

Changes:

Added strict validation to execute_rollback_plan to reject invalid plans, negative thrust, and non-finite gimbal angles
Created new SIL test suite to verify rollback execution validation logic
Added CMake configuration for the new test executable

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
src/raps/rollback_execution.hpp	Added validation checks for rollback plan validity and control input sanity (negative thrust, infinite/NaN gimbal angles)
tests/sil/test_rollback_execution.cpp	New test suite verifying validation of invalid plans, negative thrust, and infinite gimbal angles
tests/sil/CMakeLists.txt	Added build configuration for new rollback execution test executable following existing SIL test patterns

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-21T14:41:21Z

tests/sil/test_rollback_execution.cpp

+    // 3. Infinite gimbal
+    plan.thrust_magnitude_kN = 100.0f;
+    plan.gimbal_theta_rad = std::numeric_limits<float>::infinity();
+    bool res3 = execute_rollback_plan(plan, tx_id);
+    expect_false(res3, "execute_rollback_plan fails for infinite gimbal");
+
+    // 4. Valid plan
+    plan.valid = true;
+    plan.thrust_magnitude_kN = 50.0f;
+    plan.gimbal_theta_rad = 0.1f;
+    bool res4 = execute_rollback_plan(plan, tx_id);
+    expect_true(res4, "execute_rollback_plan succeeds for valid inputs");


The test suite doesn't include any test cases for gimbal_phi_rad validation, even though the execute_rollback_plan function validates it (line 32-34 in rollback_execution.hpp). Consider adding a test case to verify that infinite or NaN values for gimbal_phi_rad are properly rejected.

Suggested change

// 3. Infinite gimbal

plan.thrust_magnitude_kN = 100.0f;

plan.gimbal_theta_rad = std::numeric_limits<float>::infinity();

bool res3 = execute_rollback_plan(plan, tx_id);

expect_false(res3, "execute_rollback_plan fails for infinite gimbal");

// 4. Valid plan

plan.valid = true;

plan.thrust_magnitude_kN = 50.0f;

plan.gimbal_theta_rad = 0.1f;

bool res4 = execute_rollback_plan(plan, tx_id);

expect_true(res4, "execute_rollback_plan succeeds for valid inputs");

// 3. Infinite gimbal theta

plan.thrust_magnitude_kN = 100.0f;

plan.gimbal_theta_rad = std::numeric_limits<float>::infinity();

bool res3 = execute_rollback_plan(plan, tx_id);

expect_false(res3, "execute_rollback_plan fails for infinite gimbal theta");

// 4. Infinite gimbal phi

plan.valid = true;

plan.thrust_magnitude_kN = 100.0f;

plan.gimbal_theta_rad = 0.0f;

plan.gimbal_phi_rad = std::numeric_limits<float>::infinity();

bool res4 = execute_rollback_plan(plan, tx_id);

expect_false(res4, "execute_rollback_plan fails for infinite gimbal phi");

// 5. NaN gimbal phi

plan.valid = true;

plan.thrust_magnitude_kN = 100.0f;

plan.gimbal_theta_rad = 0.0f;

plan.gimbal_phi_rad = std::numeric_limits<float>::quiet_NaN();

bool res5 = execute_rollback_plan(plan, tx_id);

expect_false(res5, "execute_rollback_plan fails for NaN gimbal phi");

// 6. Valid plan

plan.valid = true;

plan.thrust_magnitude_kN = 50.0f;

plan.gimbal_theta_rad = 0.1f;

plan.gimbal_phi_rad = 0.0f;

bool res6 = execute_rollback_plan(plan, tx_id);

expect_true(res6, "execute_rollback_plan succeeds for valid inputs");

Copilot · 2026-02-21T14:41:22Z

tests/sil/test_rollback_execution.cpp

+    plan.gimbal_theta_rad = 0.1f;
+    bool res4 = execute_rollback_plan(plan, tx_id);
+    expect_true(res4, "execute_rollback_plan succeeds for valid inputs");
+    expect_true(tx_id.length() > 0, "tx_id is generated");


The assertion checks if tx_id length is greater than 0, but according to the PlatformHAL implementation and test conventions (see tests/sil/test_fault_injection.cpp:63), generate_tx_id() always returns exactly 24 characters. Consider changing this to 'expect_true(tx_id.length() == 24, "tx_id has correct length")' to match the established convention and be more precise.

Suggested change

expect_true(tx_id.length() > 0, "tx_id is generated");

expect_true(tx_id.length() == 24, "tx_id has correct length");

Copilot · 2026-02-21T14:41:22Z

src/raps/rollback_execution.hpp


 #include <string>
+#include <cmath>
+#include <limits>


The limits header is included but not used in this file. The std::isfinite function used in the validation logic is provided by cmath, not limits. Consider removing this unused include.

Suggested change

#include <limits>

Copilot · 2026-02-21T14:41:22Z

src/raps/rollback_execution.hpp

+    // Thrust cannot be negative
+    if (rollback.thrust_magnitude_kN < 0.0f) {


The validation for thrust_magnitude_kN only checks if it's negative, but doesn't verify that it's finite. This means that NaN or infinite values would pass validation and be sent to the actuator. Consider adding a check using std::isfinite for thrust_magnitude_kN, similar to the checks for gimbal angles below.

Suggested change

// Thrust cannot be negative

if (rollback.thrust_magnitude_kN < 0.0f) {

// Thrust must be finite and cannot be negative

if (!std::isfinite(rollback.thrust_magnitude_kN) ||

rollback.thrust_magnitude_kN < 0.0f) {

Copilot · 2026-02-21T14:41:23Z

tests/sil/test_rollback_execution.cpp

+    // 4. Valid plan
+    plan.valid = true;
+    plan.thrust_magnitude_kN = 50.0f;
+    plan.gimbal_theta_rad = 0.1f;
+    bool res4 = execute_rollback_plan(plan, tx_id);
+    expect_true(res4, "execute_rollback_plan succeeds for valid inputs");


The test suite is missing test cases for NaN and infinite thrust values. The validation logic in execute_rollback_plan should reject these values, but there are no tests to verify this behavior. Consider adding test cases similar to the infinite gimbal test (line 64-68) but for thrust_magnitude_kN.

Suggested change

// 4. Valid plan

plan.valid = true;

plan.thrust_magnitude_kN = 50.0f;

plan.gimbal_theta_rad = 0.1f;

bool res4 = execute_rollback_plan(plan, tx_id);

expect_true(res4, "execute_rollback_plan succeeds for valid inputs");

// 4. Infinite thrust

plan.valid = true;

plan.thrust_magnitude_kN = std::numeric_limits<float>::infinity();

plan.gimbal_theta_rad = 0.0f;

bool res4 = execute_rollback_plan(plan, tx_id);

expect_false(res4, "execute_rollback_plan fails for infinite thrust");

// 5. NaN thrust

plan.valid = true;

plan.thrust_magnitude_kN = std::numeric_limits<float>::quiet_NaN();

plan.gimbal_theta_rad = 0.0f;

bool res5 = execute_rollback_plan(plan, tx_id);

expect_false(res5, "execute_rollback_plan fails for NaN thrust");

// 6. Valid plan

plan.valid = true;

plan.thrust_magnitude_kN = 50.0f;

plan.gimbal_theta_rad = 0.1f;

bool res6 = execute_rollback_plan(plan, tx_id);

expect_true(res6, "execute_rollback_plan succeeds for valid inputs");

Copilot · 2026-02-21T14:41:23Z

tests/sil/test_rollback_execution.cpp

+    // We expect this to fail (return false) once validation is added.
+    // Currently, without validation, it might succeed if PlatformHAL succeeds with 0 values.
+    bool res1 = execute_rollback_plan(plan, tx_id);
+    // Note: In current implementation, this will likely return TRUE because there is no check.
+    // So this expectation will fail until I fix the code.
+    // But for TDD, I write the test expecting the CORRECT behavior.


This comment suggests the test was written in a TDD style before the implementation was complete, but since the validation logic has now been added to execute_rollback_plan, the comment is outdated and misleading. The test should now pass as expected. Consider removing or updating this comment to reflect the current state.

Suggested change

// We expect this to fail (return false) once validation is added.

// Currently, without validation, it might succeed if PlatformHAL succeeds with 0 values.

bool res1 = execute_rollback_plan(plan, tx_id);

// Note: In current implementation, this will likely return TRUE because there is no check.

// So this expectation will fail until I fix the code.

// But for TDD, I write the test expecting the CORRECT behavior.

// With an invalid plan (valid == false), execute_rollback_plan is expected to fail validation and return false.

bool res1 = execute_rollback_plan(plan, tx_id);

Copilot · 2026-02-21T14:41:23Z

tests/sil/test_rollback_execution.cpp

+    // 1. Invalid plan
+    // By default, the struct is zero-initialized, so valid might be false depending on compiler/initialization,
+    // but we explicitly set it to false to be sure.


This comment is inaccurate. The RollbackPlan struct has its 'valid' field at the end (after the hash field), and in C++, default initialization for a struct doesn't guarantee any particular value for bool fields unless explicitly initialized. The comment suggests uncertainty about initialization behavior, which could be misleading. Consider either removing this comment or clarifying that the explicit 'plan.valid = false' on line 47 is intentional to test the validation logic.

Suggested change

// 1. Invalid plan

// By default, the struct is zero-initialized, so valid might be false depending on compiler/initialization,

// but we explicitly set it to false to be sure.

// 1. Invalid plan: explicitly mark plan as invalid to exercise validation logic.

Copilot AI review requested due to automatic review settings February 21, 2026 14:34

Copilot started reviewing on behalf of dfeen87 February 21, 2026 14:35 View session

dfeen87 merged commit c43464b into main Feb 21, 2026
4 checks passed

dfeen87 deleted the flight-ready-rollback-17323931531489441353 branch February 21, 2026 14:35

Copilot AI reviewed Feb 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make rollback execution logic flight-ready#14

Make rollback execution logic flight-ready#14
dfeen87 merged 1 commit intomainfrom
flight-ready-rollback-17323931531489441353

dfeen87 commented Feb 21, 2026

Uh oh!

google-labs-jules bot commented Feb 21, 2026

Uh oh!

chatgpt-codex-connector bot commented Feb 21, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 21, 2026

Uh oh!

Copilot AI Feb 21, 2026

Uh oh!

Copilot AI Feb 21, 2026

Uh oh!

Copilot AI Feb 21, 2026

Uh oh!

Copilot AI Feb 21, 2026

Uh oh!

Copilot AI Feb 21, 2026

Uh oh!

Copilot AI Feb 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	expect_true(tx_id.length() > 0, "tx_id is generated");
	expect_true(tx_id.length() == 24, "tx_id has correct length");

		// Thrust cannot be negative
		if (rollback.thrust_magnitude_kN < 0.0f) {

Conversation

dfeen87 commented Feb 21, 2026

Uh oh!

google-labs-jules bot commented Feb 21, 2026

Uh oh!

chatgpt-codex-connector bot commented Feb 21, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants