fix(updex): refuse legacy sysext runners instead of falling back to SysextDir - #405
Draft
bketelsen wants to merge 1 commit into
Draft
fix(updex): refuse legacy sysext runners instead of falling back to SysextDir#405bketelsen wants to merge 1 commit into
bketelsen wants to merge 1 commit into
Conversation
…ysextDir An injected SysextRunner that predates PathSysextRunner could not be told which directory to link into, so the SDK linked through its pathless LinkToSysext and read the mutable package-global sysext.SysextDir at call time. That silently ignored RuntimePaths.SysextLinkDir and broke the ADR-0011 capture-at-construction invariant the SDK spec claims. The client now links only through sysext.PathSysextRunner with the directory NewClient captured, and every non-dry-run operation that may link refuses a legacy runner with the new updex.ErrLegacySysextRunner before it mutates anything: CatalogAdd before writing a definition, EnableFeature --now before writing the drop-in, installTransfer before removing a legacy symlink or downloading. The catalog rollback snapshot uses the captured directory too. The SysextRunner interface is unchanged, so existing implementations still compile. sysext.MockRunner gains LinkToSysextAt (recording the directory in LinkToSysextAtDir; LinkToSysextCalled is still set by either entry point) and a compile-time PathSysextRunner assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UigLsmd17TVDuaaSN4yfdU
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ClientConfig.SysextRunnerwas the one hole in the ADR-0011capture-at-construction invariant. A runner implementing only the original
four-method
sysext.SysextRunnerwas linked through its pathlessLinkToSysext, andClient.sysextLinkDirForRunner()returned the mutablepackage-global
sysext.SysextDir— read at call time, long afterNewClient.A client configured with
RuntimePaths.SysextLinkDirtherefore linked(and snapshotted its catalog rollback state) somewhere else entirely, while
docs/specs/sdk-api.mdasserted that "mutating ...sysext.SysextDircannotredirect the client".
This change removes the fallback rather than papering over it:
Client.linkToSysextlinks only throughsysext.PathSysextRunner, withc.paths.sysextLinkDir.sysextLinkDirForRunneris gone; the catalogrollback snapshot (
updex/catalog.go) uses the captured directory directly.updex.ErrLegacySysextRunner(testable witherrors.Is). Every non-dry-run operation that may end in a link checksrequireLinkableRunner()before it mutates anything:CatalogAddbefore writing a definition,
EnableFeaturewithNowbefore writing thedrop-in,
installTransferbefore removing a legacyCurrentSymlinkordownloading. A legacy runner therefore fails cleanly instead of part-way
through an install into the wrong directory.
SysextRunnerinterface is untouched: existing implementations stillcompile, and adding
LinkToSysextAtis what makes one usable for a realinstall.
sysext.MockRunnerimplementsPathSysextRunner(compile-time assertionincluded) and records the directory it was handed in
LinkToSysextAtDir;LinkToSysextCalledis still set by either entry point, so existingassertions are unaffected.
docs/specs/sdk-api.mdanddocs/design/overview.mdnow state the legacybehavior precisely instead of overstating client isolation.
Resolves the Snowcat architecture-gap item
28c975eb-f265-4597-aefb-d4c66080df4a.The only remaining
sysext.SysextDirread in non-testupdexcode isresolveRuntimePaths(updex/updex.go:142) — the construction-time captureitself.
Checks
make fmt— code is formatted (gofmt -lclean;make ci's gofmtstage passes)
make ci— tidy, vet, gofmt, lint (.golangci.yml), unit tests, the80.0% coverage floor (
make test-coverage-checkthenmake coverage-check), race tests, linux amd64/arm64 buildsmake ci's./tests/e2e/...stage (green);no CLI surface changed by this PR
New tests in
updex/install_link_test.go, all against a client constructedwith an instance
SysextLinkDirwhose package global is redirected to adifferent temp dir after construction:
TestUpdateFeatures_LegacyRunnerRefusedBeforeAnyMutation— the legacyrunner's
LinkToSysextis never called, the redirected global dir and thecaptured link dir are both empty, and the target dir still holds only the
staged image.
TestEnableFeature_Now_LegacyRunnerRefusedBeforeDropIn—errors.Is(err, ErrLegacySysextRunner)and no drop-in was written.TestEnableFeature_DryRunNowStillWorksWithLegacyRunner— the dry-run pathis unaffected.
TestUpdateFeatures_PathRunnerReceivesCapturedLinkDir— aPathSysextRunneris handed the captured dir, never the moved global.Falsification: with the guards removed and the old fallback restored, the two
refusal tests fail (
expected UpdateFeatures to refuse a legacy sysext runner,EnableFeature error = <nil>, want ErrLegacySysextRunner); with thefix in place all seven link tests pass. The pre-existing
DefaultRunner-backed link tests (RestoresSysextLinkForCurrentImage,LeavesCorrectSysextLinkAlone,EnableFeature_Now_RestoresSysextLinkForCurrentImage)are unchanged and still green.
Risk classification
Rationale:
sysext/**, areview-requiredprotected boundary inpolicies/agent-governance.json(installation-and-update) with aminimum_risk_tierof high, and it changes a privileged filesystem-writepath: which directory a client is allowed to create sysext symlinks in.
It is also a behavior break for any out-of-tree
SysextRunnerthat lacksLinkToSysextAt— such a runner now returnsErrLegacySysextRunnerinstead of silently linking into
/var/lib/extensions. That refusal is thepoint (the old behavior wrote outside the client's declared directory), but
it is a compatibility change and takes the highest applicable tier.
Docs housekeeping
README.md,docs/design/overview.md,docs/specs/*updated forbehavior changes;
AGENTS.mdfor convention/workflow changes —docs/specs/sdk-api.md(theNewClientisolation paragraph and thesysextpackage reference) anddocs/design/overview.md(testingpatterns) updated; no
README.mdorAGENTS.mdchange is implied.TEMPLATE.mdand indexed indocs/README.md— no new docs; existing docs edited in place.applicable: this enforces the existing ADR-0011 invariant rather than
deciding anything new.
instead.
Verification
node scripts/check-docs.mjsgreenmake test-docs-checkalso green (test-check-docs: all assertions passed).PR review rubric