Skip to content

[codex] Preserve the first action callback error#85

Open
pawelchcki wants to merge 4 commits into
mainfrom
codex/action-error-preservation
Open

[codex] Preserve the first action callback error#85
pawelchcki wants to merge 4 commits into
mainfrom
codex/action-error-preservation

Conversation

@pawelchcki

Copy link
Copy Markdown
Contributor

What

Keeps the first failing action callback result while continuing to invoke later actions. Adds a self-contained C regression with a failing action followed by a successful action.

Why

perform_actions overwrote its accumulated result after every callback, so a later success could mask an earlier enforcement failure.

Impact

Policy evaluation now reports the original action failure instead of incorrectly returning success.

Validation

  • Focused policy_actions.later_success_does_not_erase_failure CTest
  • Full C suite: 56/56 passing
  • Clang formatting and diff checks

Part of #82

@datadog-prod-us1-4

datadog-prod-us1-4 Bot commented Jul 14, 2026

Copy link
Copy Markdown

Tests

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 100.00%
Overall Coverage: 23.13%

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: d91dbce | Docs | Datadog PR Page | Give us feedback!

@pawelchcki

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 228cc12065

ℹ️ 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".

Comment on lines +38 to +39
dd_wls_Action_create(&builder, dd_wls_ActionId_INJECT_DENY, first_description, values),
dd_wls_Action_create(&builder, dd_wls_ActionId_INJECT_ALLOW, second_description, values),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixture constructs a policy containing both INJECT_DENY and INJECT_ALLOW, which appears contradictory and makes the scenario difficult to understand.

Comment thread c/src/evaluator.c

static inline plcs_errors perform_actions(plcs_evaluation_result eval_res, dd_ns(Action_vec_t) actions_vec) {
plcs_errors res = PLCS_ESUCCESS;
plcs_errors first_error = PLCS_ESUCCESS;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we switch from retaining the last callback result to retaining only the first error. maybe it's best if switch to some errno like behavior where we register all the failures and consumers can use an get_actions_errno() to consume an array or a linked list of 'action, result' key value pairs?

  typedef struct {
      plcs_actions action;
      plcs_errors result;
      size_t action_index;
  } plcs_action_result;

  size_t plcs_get_action_results(
      plcs_action_result *results,
      size_t capacity
  );

because anyway i think these are being ignored now.

I talked to @annacai21 about doing something similar for the evaluation...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants