Skip to content

Follow-ups from Spec 108-a profile policy model review (#1380) #1393

Description

@Dumbris

Follow-up findings from the zcode (GLM-5.3) review of #1380 that were below the merge bar (no critical/high). Each was verified against the code; fix in small PRs.

  • low — internal/profile/glob.go:24: Glob compilation omits the (?s) regexp flag, so '*' (compiled to '.') never matches an embedded newline in a tool identity, a narrow fail-open for a deny rule.
  • low — internal/config/profiles_v3_test.go:16-26: Legacy-profile round-trip test and its doc comment claim 'byte-identical' but the assertion uses require.JSONEq (semantic equality), not require.Equal.
  • low — internal/profile/glob_test.go:25: Glob test case labeled 'tool name containing a slash' uses a fixture with no slash character; the real slash-containing coverage is the adjacent case.
  • low — internal/config/rollout_gate_ast_test.go:130-134,143 (policyEnforcementTestOverride declared at internal/config/profiles.go:143): The new AST test (which fixes ledger F1's missing artifact) allowlists any '.Load' selector on policyEnforcementTestOverride but never pins the variable's declared type to atomic.Bool. Confirmed by reading the test: it only asserts policyEnforcementReadyBase is const, not the override var's type.
  • low — internal/config/rollout_gate_ast_test.go:54-57,169-205: The 'override unreachable from production code' walk exempts the entire file internal/config/profiles.go, not just EnablePolicyForTest's declaration, so any new exported function added anywhere in profiles.go that calls policyEnforcementTestOverride.Store() directly would be invisible to the walk, and the sibling AST test only inspects PolicyEnforcementReady's body, not other functions.
  • low — internal/httpapi/config_profile_v3_rollout_gate_test.go:47-56 vs internal/runtime/runtime.go:1665-1693: profileGateController.ApplyConfig (the REST-door test's fake) re-implements a validate-then-apply contract by calling cfg.ValidateDetailed() itself, rather than driving the real internal/runtime.Runtime.applyConfigLocked, which is where production validation actually happens on these two REST doors. Verified applyConfigLocked (runtime.go:1683) does call ValidateDetailed() unconditionally today with no field-specific bypass, so this is a coverage gap, not a live defect.
  • low — internal/server/profiles_v3_node_fixture_test.go:53-56: Comment claims the JSON node fixtures and the in-process Go fixture (profiles_v3_fixture_test.go / scope_fixture_test.go) 'never drift apart on content' because tool order matches; in fact their per-tool descriptions and write-tool annotation shape already differ (JSON: description==tool name, single readOnlyHint:false; in-process: descriptive text, both readOnlyHint:false and destructiveHint:false set).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    kind/bugSomething isn't workingpriority/lowNice to have; address when bandwidth allowstriage/acceptedTriaged and accepted for the backlog

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions