Skip to content

swagger: agent, correctly capitalize eventBasedHold param - #4898

Merged
Michal-Leszczynski merged 1 commit into
masterfrom
ml/fix-agent-hold-swagger
Aug 6, 2026
Merged

swagger: agent, correctly capitalize eventBasedHold param#4898
Michal-Leszczynski merged 1 commit into
masterfrom
ml/fix-agent-hold-swagger

Conversation

@Michal-Leszczynski

Copy link
Copy Markdown
Collaborator

All SM agent handlers expect eventBasedHold, not EventBasedHold. This was overlooked because our hold test interceptor works on swagger models, so it was consistent in the client and test interceptor interaction. Since our gcs mock does not support holds, we didn't spot it until we executed tests against the real gcs.

All SM agent handlers expect eventBasedHold, not EventBasedHold.
This was overlooked because our hold test interceptor works on
swagger models, so it was consistent in the client and test
interceptor interaction. Since our gcs mock does not support holds,
we didn't spot it until we executed tests against the real gcs.
Copilot AI balanced review requested due to automatic review settings August 5, 2026 08:05
@Michal-Leszczynski
Michal-Leszczynski marked this pull request as ready for review August 5, 2026 08:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Ready to approve

The schema and generated models now consistently match the agent handler contract.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Corrects the agent Swagger contract so event-based hold fields match the lower-camel-case parameter expected by agent handlers.

Changes:

  • Renames both affected JSON properties to eventBasedHold.
  • Regenerates corresponding agent model tags.
File summaries
File Description
v3/swagger/agent.json Corrects both schema property names.
v3/swagger/gen/agent/models/check_permissions_options.go Updates the generated request JSON tag.
v3/swagger/gen/agent/models/list_item.go Updates the generated response JSON tag.
Review details

Files not reviewed (2)

  • v3/swagger/gen/agent/models/check_permissions_options.go: Generated file
  • v3/swagger/gen/agent/models/list_item.go: Generated file
  • Files reviewed: 1/3 changed files
  • Comments generated: 0
  • Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@VAveryanov8 VAveryanov8 left a comment

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.

Do you think it's possible (worth) to adjust our integration tests somehow to catch similar issue in the future?

@Michal-Leszczynski

Copy link
Copy Markdown
Collaborator Author

Do you think it's possible (worth) to adjust our integration tests somehow to catch similar issue in the future?

So in general such a thing would be caught in most other cases, but here we lack gcs mock support for event based holds and both SM client and SM test interceptor operate on the same swagger file, so this failure wasn't caught there.

I can extend the interceptor to have some simple validation on the json fields returned from SM agent in the PR which will bump swagger dep in the main module.

@Michal-Leszczynski
Michal-Leszczynski merged commit 9acf01e into master Aug 6, 2026
82 checks passed
@Michal-Leszczynski
Michal-Leszczynski deleted the ml/fix-agent-hold-swagger branch August 6, 2026 14:38
Michal-Leszczynski added a commit that referenced this pull request Aug 13, 2026
This bump includes the changes from:
- #4898
- #4893
Michal-Leszczynski added a commit that referenced this pull request Aug 13, 2026
This bump includes the changes from:
- #4898
- #4893
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.

4 participants