Skip to content

Fix SecOps rule detection arguments - #283

Merged
dandye merged 1 commit into
google:mainfrom
abdeltaehass:agent/fix-rule-detections-arguments
Aug 28, 2026
Merged

Fix SecOps rule detection arguments#283
dandye merged 1 commit into
google:mainfrom
abdeltaehass:agent/fix-rule-detections-arguments

Conversation

@abdeltaehass

Copy link
Copy Markdown
Contributor

Summary

  • pass ChronicleClient.list_detections filters and pagination arguments by keyword
  • expose optional start_time, end_time, and list_basis parameters through get_rule_detections
  • add focused regression coverage for alert-state, pagination, and time-range forwarding

Root cause

get_rule_detections passed alert_state, page_size, and page_token positionally. The Chronicle SDK places start_time, end_time, and list_basis before those parameters, so values such as "ALERTING" were bound to start_time and failed when the SDK called strftime().

The wrapper now uses explicit keyword arguments and converts optional ISO-8601 time strings to datetime objects before calling the SDK.

Fixes #263

Validation

  • focused get_rule_detections tests: 2 passed
  • SecOps unit tests: 18 passed
  • git diff --check: passed

@google-cla

google-cla Bot commented Aug 15, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@abdeltaehass
abdeltaehass marked this pull request as ready for review August 15, 2026 02:10
@abdeltaehass
abdeltaehass requested a review from a team August 15, 2026 02:10
@abdeltaehass

Copy link
Copy Markdown
Contributor Author

Friendly ping on this one. CI is green and it still merges cleanly.

Happy to rebase, split it up, or add more test coverage if any of that would help. Is there anything else I can do to move it along?

@dandye

dandye commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Thank you for your contribution, @abdeltaehass! I'll try to review ASAP.

Comment on lines +320 to +329
start_dt = (
datetime.fromisoformat(start_time.replace("Z", "+00:00"))
if start_time
else None
)
end_dt = (
datetime.fromisoformat(end_time.replace("Z", "+00:00"))
if end_time
else None
)

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.

I know you are following precedent in this project, so there is no blame, but this has some issues, which I've described in #291. I think I will fast follow with a fix of all occurrences of this rather than block your PR.

@dandye
dandye merged commit 9ce114e into google:main Aug 28, 2026
6 checks passed
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.

secops_mcp_get_rule_detections_bug

2 participants