fix(template): drop device-labels resolver + add DispatchAction template gate (#196)#236
Merged
PaulDotterer merged 1 commit intoMay 11, 2026
Conversation
…ate gate (#196) Two scope corrections to the group-variables feature shipped in #196. ## Drop device-labels rung from the resolver Variables are EXCLUSIVELY a group concept (defined on device-groups + user-groups). The original implementation added a third resolver layer that read device.Labels and surfaced them as STRING-typed variables with highest precedence — that was never in scope. Device labels are a separate concept (group-membership filter / dynamic-group eval), not a variable source. - internal/api/template/resolver.go: drop collectDeviceLabels + the call site; update doc comments to two-layer (device-group > user-group) precedence. - internal/api/template/render.go: update Resolver doc. - internal/api/template/resolver_test.go: drop TestStoreResolver_DeviceLabels_Surface and TestStoreResolver_PrecedenceLabelOverridesDeviceGroup (they exercised the now-removed code path). ## Add templated-params gate to DispatchAction Ad-hoc DispatchAction has no group context to resolve {{ var.NAME }} from \xe2\x80\x94 the renderer only runs on the SyncActions path where the agent's device \xe2\x86\x92 group memberships supply the values. Without this gate, an operator who tries to run-now an action whose params contain {{ var.X }} would have those literal markers enqueued to the device verbatim. The agent would then consume them as opaque strings. - internal/api/template/render.go: new exported HasReference(s) helper colocated with the renderer's regex (single source of truth for the {{ var.NAME }} grammar). - internal/api/action_dispatch.go: marshal inputs.params once, scan for templates via template.HasReference, return CodeFailedPrecondition with a clear operator message that explains variables are group-only and the action should be assigned to a group instead. Marshal failure is fail-closed (CodeInternal) \xe2\x80\x94 unscannable params can't be passed. - internal/api/dispatch_validation_test.go: new TestDispatchAction_TemplatedParamsRefused. DispatchToGroup, DispatchActionSet, DispatchDefinition, and DispatchToMultiple all funnel into DispatchAction \xe2\x80\x94 the single gate covers every ad-hoc dispatch path. DispatchInstantAction is parameterless (REBOOT/SYNC) and unaffected. Refs #196 (scope correction).
📝 WalkthroughWalkthroughThe PR removes device labels from variable resolution precedence and adds a pre-dispatch validation gate that rejects templated parameters in ad-hoc device dispatches. Variable resolution now uses only device-group and user-group variables (group-only). A new ChangesTemplate Parameters Gate for Ad-Hoc Dispatches
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/api/dispatch_validation_test.go (1)
181-216: ⚡ Quick winAdd the same assertion for the
ActionIdpath.This only pins the inline branch, but the new gate has separate normalization logic for stored actions (
json.Unmarshal/ string fallback). A templated stored action can regress independently and still leave this suite green. OneActionIdcase here would cover the whole feature much better.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/dispatch_validation_test.go` around lines 181 - 216, The test TestDispatchAction_TemplatedParamsRefused only exercises the InlineAction branch; add a second assertion that calls h.DispatchAction with an ActionSource using ActionId (i.e. populate pm.DispatchActionRequest.ActionSource as &pm.DispatchActionRequest_ActionId{ActionId: <pm.ActionId>}) so the stored-action normalization path (json.Unmarshal / string fallback) is exercised; expect a non-nil error, connect.CodeFailedPrecondition from connect.CodeOf(err), and that err.Error() contains "templated parameters" just like the inline case to prevent regressions in the stored-action path.internal/api/template/resolver_test.go (1)
61-65: ⚡ Quick winReplace the removal note with a negative regression test.
Deleting the old label-based tests drops the only coverage around label interaction. Since the new contract is “labels are ignored,” it’s worth asserting that explicitly so a future resolver change can’t accidentally reintroduce label-sourced variables or label-over-group shadowing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/action_dispatch.go`:
- Around line 150-177: The current gate marshals inputs.params to JSON and runs
template.HasReference over the whole blob (paramsJSONForScan), which rejects
var-like literals even in non-templateable fields (e.g.,
ShellParams.environment); instead, walk only the templateable fields the
renderer uses and run template.HasReference on those string values. Update
action_dispatch.go to replace the json.Marshal(inputs.params) +
template.HasReference(string(...)) check with a targeted walk (or call the
renderer's template-field walker if available, e.g.,
renderer.WalkTemplateableFields or a new walkTemplateableParams(inputs.params))
that visits the same annotated/templateable fields and calls
template.HasReference on each field value, keeping the existing fail-closed
behavior for any marshal/iteration errors.
---
Nitpick comments:
In `@internal/api/dispatch_validation_test.go`:
- Around line 181-216: The test TestDispatchAction_TemplatedParamsRefused only
exercises the InlineAction branch; add a second assertion that calls
h.DispatchAction with an ActionSource using ActionId (i.e. populate
pm.DispatchActionRequest.ActionSource as
&pm.DispatchActionRequest_ActionId{ActionId: <pm.ActionId>}) so the
stored-action normalization path (json.Unmarshal / string fallback) is
exercised; expect a non-nil error, connect.CodeFailedPrecondition from
connect.CodeOf(err), and that err.Error() contains "templated parameters" just
like the inline case to prevent regressions in the stored-action path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03a9b98b-f66e-4cab-adc7-e72b7134e578
📒 Files selected for processing (5)
internal/api/action_dispatch.gointernal/api/dispatch_validation_test.gointernal/api/template/render.gointernal/api/template/resolver.gointernal/api/template/resolver_test.go
Comment on lines
+150
to
+177
| // Templated-params gate: ad-hoc DispatchAction has no group context | ||
| // to resolve `{{ var.NAME }}` from (variables are exclusively a | ||
| // group concept; the renderer only runs on the SyncActions path | ||
| // where the agent's device → group memberships supply the values). | ||
| // Refuse here rather than enqueueing literal `{{ ... }}` markers | ||
| // the agent would consume verbatim. | ||
| // | ||
| // Marshals to JSON once for the scan, regardless of params shape | ||
| // (map[string]any from the stored branch, []byte from the inline | ||
| // branch, or the rare string fallback). The marshal happens again | ||
| // downstream for signing — a one-extra-allocation cost in exchange | ||
| // for one shared scan path. Marshal failure here MUST be fail- | ||
| // closed: a params shape we can't serialise can't be scanned for | ||
| // templates either, and silently bypassing the gate would let | ||
| // templated content through that we couldn't see. The downstream | ||
| // signing step would also fail on the same params, but failing | ||
| // here is more honest about which gate is rejecting the call. | ||
| paramsJSONForScan, err := json.Marshal(inputs.params) | ||
| if err != nil { | ||
| return nil, apiErrorCtx(ctx, ErrInternal, connect.CodeInternal, | ||
| fmt.Sprintf("failed to marshal action params for template scan: %v", err)) | ||
| } | ||
| if template.HasReference(string(paramsJSONForScan)) { | ||
| return nil, apiErrorCtx(ctx, ErrValidationFailed, connect.CodeFailedPrecondition, | ||
| "this action contains templated parameters ({{ var.NAME }}) and cannot be dispatched ad-hoc to a single device. "+ | ||
| "Variables are resolved from device-group / user-group memberships at agent sync time. "+ | ||
| "Assign the action to a device-group or user-group instead of running it directly.") | ||
| } |
There was a problem hiding this comment.
Keep the gate scoped to templateable fields only.
This JSON-wide scan will also reject {{ var.NAME }} inside params the renderer intentionally never substitutes, such as ShellParams.environment or any future non-templateable string field. That changes the contract from “only annotated fields are templates” to “any var-like literal is forbidden” for ad-hoc dispatches. Please gate against the same templateable-field walk the renderer uses instead of scanning the entire marshaled params blob.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/action_dispatch.go` around lines 150 - 177, The current gate
marshals inputs.params to JSON and runs template.HasReference over the whole
blob (paramsJSONForScan), which rejects var-like literals even in
non-templateable fields (e.g., ShellParams.environment); instead, walk only the
templateable fields the renderer uses and run template.HasReference on those
string values. Update action_dispatch.go to replace the
json.Marshal(inputs.params) + template.HasReference(string(...)) check with a
targeted walk (or call the renderer's template-field walker if available, e.g.,
renderer.WalkTemplateableFields or a new walkTemplateableParams(inputs.params))
that visits the same annotated/templateable fields and calls
template.HasReference on each field value, keeping the existing fail-closed
behavior for any marshal/iteration errors.
5 tasks
4 tasks
5 tasks
PaulDotterer
added a commit
that referenced
this pull request
May 12, 2026
Reverts the group-based variables feature shipped via 5 server PRs: - #197 (f0934bf): server storage + handlers + RBAC + events + migration 043_group_variables.sql + sqlc queries + group-variable validator + audit-redactor schema entries for the new event types - #198 (6dafcef): server-side {{ var.NAME }} template renderer + internal_handler.go SyncActions hook + main.go renderer wiring - #236 (3bb247a): drop device-labels resolver + add DispatchAction template gate - #237 (ee455b1): rewire ListAvailableVariables to group-scoped + SDK pin bump - #238 (05c7bc1): distinct error code for templated-dispatch refusal Squashed into one cohesive rollback commit alongside the SDK pin bump to power-manage-sdk@dda53a0 (which is the post-rollback SDK HEAD that no longer carries the group-vars proto). Conflicts resolved during revert: - cmd/control/main.go: dropped both the 'template' import (we are removing the package) AND the 'asynqutil' import the revert wanted to put back (it lives in valkey.go now post-#157 slice 4 extraction; not main.go's responsibility anymore). - go.mod: SDK pin set to dda53a0 (post-revert SDK HEAD) rather than the pre-#197 SHA the revert hunk targeted, so the server compiles against an SDK that also doesn't have the variables proto. - go.sum: regenerated via 'go mod tidy' against the new pin. Why: the design has fundamental scaling problems that were not caught at design time. Group-bound variables become monsters when many actions are assigned to a group; assignment-bound variables suffer the inverse N\xc3\x97M repetition problem; bundle-based reuse needs more design work before re-implementation. Cleaner to revert wholesale than to ship a half-shape that locks operators into the wrong API. Companion reverts: - SDK #64 (merged): the proto + TS-client wrappers - Web 09a8c75 (shipped): the variables UI + hint banner + error mapping Refs #196.
3 tasks
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
1 participant
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
Two scope corrections to the group-variables feature shipped in #196.
Drop device-labels rung from the resolver
Variables are EXCLUSIVELY a group concept (defined on device-groups + user-groups). The original implementation added a third resolver layer that read device.Labels and surfaced them as STRING-typed variables with highest precedence — that was never in scope. Device labels are a separate concept (group-membership filter / dynamic-group eval), not a variable source.
Add templated-params gate to DispatchAction
Ad-hoc `DispatchAction` has no group context to resolve `{{ var.NAME }}` from — the renderer only runs on the SyncActions path where the agent's device → group memberships supply the values. Without this gate, an operator who tries to run-now an action whose params contain `{{ var.X }}` would have those literal markers enqueued to the device verbatim. The agent would then consume them as opaque strings.
`DispatchToGroup`, `DispatchActionSet`, `DispatchDefinition`, and `DispatchToMultiple` all funnel into `DispatchAction` — the single gate covers every ad-hoc dispatch path. `DispatchInstantAction` is parameterless (REBOOT/SYNC) and unaffected.
Test plan
Follow-ups
This is PR 1 of a 4-PR scope correction:
Refs #196 (scope correction).
Summary by CodeRabbit
New Features
Bug Fixes
Tests