Skip to content

Conversation

@jbleon95
Copy link
Contributor

@jbleon95 jbleon95 commented Jan 28, 2026

Overview

Closes AUTH-2553.

This PR adds a new commandAnnotation field to the BaseCommandCreate which is inherited by all PE command create objects. This field is a list that will contain the annotation IDs of any active command annotation when that command is created/queued, that will then be assigned to the command. This is fed in by the SyncClient which a future PAPI PR will hook up to most PE calls within the engine cores. For now, all commands generated will have this field be an empty list.

This change is accompanied by version 17 of the command schema which now contains this field in the schema.

One additional change was made in this PR, changing the BaseCommand commandAnnotations field type from Optional[List[Str]] with a default of None to List[Str] with a default factory to create an empty list.

Test Plan and Hands on Testing

No behavioral changes yet but tested running this code on a Flex and ensuring that nothing crashed when running protocols between recent alphas and this branch.

Changelog

  • Adds commandAnnotation field to BaseCommandCreate
  • Adds command_annotation field to SyncClient execute calls
  • Changed BaseCommand commandAnnotations field type from Optional[List[Str]] to List[Str]

Review requests

I made the change for the BaseCommand field type since it seemed to me unnecessary to have it be None when an empty list communicated the same thing, but if someone feels differently (e.g. it might be useful for older protocols, legacy JSON protocols, etc), I can change that.

Risk assessment

Medium-high. This is a non-trivial change to the command objects, and does affect a lot of unit tests and will be a big analyses snapshot change, but there's no actual behavioral changes here, the risk is all backwards-compatibility.

@codecov
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.53%. Comparing base (4c2d5cb) to head (965350c).
⚠️ Report is 6 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #20726      +/-   ##
==========================================
- Coverage   56.54%   56.53%   -0.01%     
==========================================
  Files        3916     3916              
  Lines      323257   323275      +18     
  Branches    45767    45778      +11     
==========================================
- Hits       182774   182772       -2     
- Misses     140267   140287      +20     
  Partials      216      216              
Flag Coverage Δ
app 45.95% <ø> (+<0.01%) ⬆️
protocol-designer 19.67% <ø> (-0.01%) ⬇️
step-generation 5.73% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jbleon95 jbleon95 marked this pull request as ready for review January 29, 2026 18:23
@jbleon95 jbleon95 requested review from a team as code owners January 29, 2026 18:23
@jbleon95 jbleon95 requested review from jerader, mjhuff, sanni-t and sfoster1 and removed request for a team January 29, 2026 18:23
Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

3 participants