docs/api: document fix-encryption-support by raimundo-henriques · Pull Request #17702 · canonical/snapd

raimundo-henriques · 2026-09-23T13:27:07Z

Problem

Although action fix-encryption-support was already documented, it lacked some details and had some errors:

nowhere in the action description was it stated that a GET request to the same systems label was needed before executing it;
nowhere was it mention that the values of fix-action are defined by the secboot library and are returned by availability-check-errors.actions;
args was wrongly defined as an array of strings;
systems-label did not document the 500 response (which action fix-encryption-support might return, for instance when called without a prior GET request.

Solution

The necessary context was added where needed.
The definition of args was fixed: it is now described as a non-empty object.
Response 500 was added to systems-label.

Open questions

The description of fix-actions currently treats the actions returned in availability-check-errors.actions as an example of the operations that can be made with this action.
If I understand correctly, as long as the action exists in secboot, then it can be used here, even if it is not explicitly mention in availability-check-errors.actions.
Is this the case? If so, is the current formulation sufficiently clear?

No mention of the ordering of the actions is present. If I understand correctly, this is a wider problem, outside of the spec's scope. I propose addressing it in the surrounding documentation (for now).

Drive-by changes

Fixed the use of back-ticks. A more consistent review of the specs in this regard is, however, in order.

Context

This PR presupposes #17696 and is dependent on its approval.

SNAPDENG-36197

Clarify and make small fixes to the request body schema of action fix-encryption-support: - document that the action requires a prior GET request to create a non-stateful pre-install check context; - document that the fix-action values are defined by secboot, indicating that they are returned in availability-check-errors.actions and that an empty string maps to ActionNone; - fix the type of args from array of strings to non-empty object; - document the 500 response on systems-label. As a drive-by change it also fixes the use of backticks.

Copilot

Copilot review overview

🟡 Changes recommended

The new action reference contradicts the documented SystemDetails schema.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Fix availability-check-errors schema to expose error actions · New

What changed in this PR

Improves OpenAPI documentation for the storage-encryption repair workflow.

Changes:

Documents prerequisite context, valid fix actions, and argument structure.
Adds the possible HTTP 500 response.
Standardizes inline code formatting.

File	Description
`docs/api/v2/paths/systems-label.yaml`	Documents responses and result variants.
`docs/api/v2/components/schemas/SystemActionInstallGenerateRecoveryKeyResponse.yaml`	Corrects inline code formatting.
`docs/api/v2/components/schemas/SystemActionFixEncryptionSupport.yaml`	Expands the fix-action contract.
`docs/api/v2/components/schemas/SystemActionCheckAuthQualityResponse.yaml`	Corrects inline code formatting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-09-23T13:50:20Z

Thu Sep 24 17:08:03 UTC 2026

Spread tests skipped

ernestl · 2026-09-23T23:56:28Z

Open questions

The description of fix-actions currently treats the actions returned in availability-check-errors.actions as an example of the operations that can be made with this action.
If I understand correctly, as long as the action exists in secboot, then it can be used here, even if it is not explicitly mention in availability-check-errors.actions.
Is this the case? If so, is the current formulation sufficiently clear?

https://github.com/canonical/secboot/blob/master/efi/preinstall/checks_context.go#L639

Only action "None" is always allowed

The "context" that is carried between iterations during error fixing contains the allowed actions based on the errors previously encountered.

@raimundo-henriques

ernestl · 2026-09-24T00:02:36Z

Open questions

No mention of the ordering of the actions is present. If I understand correctly, this is a wider problem, outside of the spec's scope. I propose addressing it in the surrounding documentation (for now).

Excellent question!

Look at this: canonical/secboot#454 (comment) and also canonical/secboot#470

So in conclusion, order is critical. This is what you want: https://github.com/canonical/secboot/pull/470/changes#diff-69eda06609ee6462011fbd75e585d6aacd71906b9126db7bfec20b435d87f453R38-R57

@raimundo-henriques

ernestl

Good additions. Answered open questions. Some comments for your consideration.

ernestl · 2026-09-24T00:06:38Z

-description: Payload to apply a corrective action to fix storage encryption support.
+description: >-
+  Payload to apply a corrective action to fix storage encryption support.
+  This action can only be performed after a pre-install check context is created


Perhaps "after an initial" -> we need to be clear that single GET is required to allow as many iterations of fixes as is required until success/failure.

Perhaps "Actions" instead of "This action" helps clarify multiple iterations?

Just suggestions to help demonstrate my point, if you agree you are welcome to choose better words.

Good point! Thank you! 'Actions' might be ambiguous, though, as it might be taken to refer to any action, whereas this only applies to iterations fix-encryption-support. But your point still stands, of course. I'll come up with a proposal.

ernestl · 2026-09-24T00:07:38Z

+  Payload to apply a corrective action to fix storage encryption support.
+  This action can only be performed after a pre-install check context is created
+  through a GET request to the same system label. That context is kept in memory
+  and is lost after a subsequent GET request, when snapd restarts, or when the


Is this specific enough? Its not any GET request, its a very specific request that resets context.

Also a very fair point. The idea was:

'GET request to the same system label' -> GET /v2/systems/{label}

'subsequent GET request' -> subsequent GET request to the same system label
But as I type I see that this is far from transparent.
Thanks!

ernestl · 2026-09-24T00:14:36Z

-    description: The specific fix to apply.
+    description: >-
+      The specific fix to apply. Valid values are the action names defined in the secboot
+      library, e.g., those in the `storage-encryption.availability-check-errors.actions`


There are two points to make here:

The exhaustive list is defined in secboot here: https://github.com/canonical/secboot/blob/master/efi/preinstall/actions.go

Which of these are allowed to be used to attempt to fix currently reported errors are determined by secboot and communicated via storage-encryption.availability-check-errors.actions

Consider separating the points more clearly.

This overlaps with the question you've also answered separately. You're absolutely right. Thanks for the comment and the context!

ZeyadYasser

Looks really good, Thanks! left tiny nitpicks comments

ZeyadYasser · 2026-09-24T07:38:48Z

-    description: The specific fix to apply.
+    description: >-
+      The specific fix to apply. Valid values are the action names defined in the secboot
+      library, e.g., those in the `storage-encryption.availability-check-errors.actions`


Only the actions from storage-encryption.availability-check-errors.actions are allowed as Ernest highlighted.

Perfect! I'll update this soon, thanks!

ZeyadYasser · 2026-09-24T07:40:21Z

    type: string
-    description: The specific fix to apply.
+    description: >-
+      The specific fix to apply. Valid values are the action names defined in the secboot


I wonder if we should mention secboot vs keeping the spec self-contained to avoid people depending on internals of secboot?

This is a very good question. The problem for me is that we are dependent on secboot. So, if we don't refer to it, then we need to provide more details as to which action names are accepted (for instance). But then we need to maintain it, which sounds like a bad idea... The same applies to errors, for instance. So, while in principle I would defend the self-contained option, in this case I think it's not really maintainable. What do you think?

I think you make a good point here, if we mention the content of secboot stuff here then we need to ensure it is maintained and up-to-date. Is there any public documentation available for secboot? If so, IMO we should provide a link to the relevant area of documentation, but if not, we may need to provide some of the information here. @ZeyadYasser what do you think?

Just to clarify, I don't think linking to the secboot source code would be a good solution for this. If we are going to link to it, it should be formatted documentation

Thanks @Rnfudge02 ! As far as I can tell, there is no secboot documentation. Which left exactly with the dilemma of either not linking at all or linking to the source code. I opted for the former.
The issue I see with providing more secboot-related context is that it will be needed in multiple places. The changes I'm proposing here, for instance, would be a place where we would have to add a lot more context. I see three options:
(A) not mention secboot at all;
(B) mention secboot and let the user do their research if they need more details;
(C) adding more details about secboot (errors, actions, etc.).
I think (A) is unhelpful and (C) unmaintainable, as we would have to constantly keep updating the spec whenever something changes on secboot. So I've landed on (B) although it feels suboptimal.
I guess the best option would be for secboot to have documentation we could link to. Happy to discuss that, but it's definitely outside this PR's scope...

Rnfudge02

A little comment about secboot actions and errors, other than that, LGTM! Thanks!

Enrich the descriptions of fix-encryption-support to: - make it clearer that an initial GET request is required and that multiple iterations of the fix-encryption-support action can be performed; and - indicate that the actions should be performed in the order by which they are returned in the SystemDetails.

raimundo-henriques · 2026-09-24T16:58:39Z

@ernestl , @ZeyadYasser and @Rnfudge02 thanks for your reviews! I've made some updates to the fix-encryption-support payload description. I am afraid this level of detail might already be a bit much, but I think it's justified in this context. I would, however, try not to add more to it.
I'll update PR #17696 to mention the action order in SystemDetails as well.
We still need to figure out the references to secboot, but my position is to keep it as is for now.
(Incidentally, I realized that we're using the term 'action' for two very different things.)

Copilot

Copilot review overview

🟡 Changes recommended

The context lifecycle documentation inaccurately equates replacement by a subsequent GET with context loss.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

Clarify context replacement versus loss after a second GET · New

Resolved since last review (1)

Fix availability-check-errors schema to expose error actions

raimundo-henriques · 2026-09-24T17:26:17Z

+  check context is lost if another GET request to the same system is made
+  (`GET /v2/systems/{label}`), if snapd restarts, or if the system reboots.


I'm inclined to accept this formulation.

codecov · 2026-09-24T17:08:50Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.39%. Comparing base (dc1e09e) to head (c290882).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #17702      +/-   ##
==========================================
- Coverage   78.45%   78.39%   -0.06%     
==========================================
  Files        1404     1415      +11     
  Lines      200464   200582     +118     
  Branches     2503     2503              
==========================================
- Hits       157269   157248      -21     
- Misses      33773    33917     +144     
+ Partials     9422     9417       -5

Flag	Coverage Δ
unittests	`78.39% <ø> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

raimundo-henriques requested review from Rnfudge02, ZeyadYasser, ernestl and valentindavid and a balanced review from Copilot September 23, 2026 13:27

raimundo-henriques added Simple 😃 A small PR which can be reviewed quickly Documentation Skip spread Indicate that spread job should not run labels Sep 23, 2026

Copilot started reviewing on behalf of raimundo-henriques September 23, 2026 13:27 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Comment thread docs/api/v2/components/schemas/SystemActionFixEncryptionSupport.yaml Outdated

raimundo-henriques changed the title ~~thdocs/api: document fix-encryption-support~~ docs/api: document fix-encryption-support Sep 23, 2026

ernestl reviewed Sep 24, 2026

View reviewed changes

ZeyadYasser reviewed Sep 24, 2026

View reviewed changes

Rnfudge02 approved these changes Sep 24, 2026

View reviewed changes

raimundo-henriques requested review from ZeyadYasser and ernestl and a balanced review from Copilot September 24, 2026 16:59

Copilot started reviewing on behalf of raimundo-henriques September 24, 2026 16:59 View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

ernestl approved these changes Sep 24, 2026

View reviewed changes

		check context is lost if another GET request to the same system is made
		(`GET /v2/systems/{label}`), if snapd restarts, or if the system reboots.

Conversation

raimundo-henriques commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Open questions

Drive-by changes

Context

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

github-actions Bot commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Spread tests skipped

Uh oh!

ernestl commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ernestl commented Sep 24, 2026

Uh oh!

ernestl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeyadYasser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rnfudge02 left a comment

Choose a reason for hiding this comment

Uh oh!

raimundo-henriques commented Sep 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 24, 2026

Codecov Report

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

raimundo-henriques commented Sep 23, 2026 •

edited

Loading

github-actions Bot commented Sep 23, 2026 •

edited

Loading

ernestl commented Sep 23, 2026 •

edited

Loading