fix(safety-profile): accept the value a locked flag already sets and note it only where it is mentioned

docs/safety-profiles.md

@@ -184,31 +184,19 @@ tokens, or other arbitrary configuration into a binary.
 A locked flag behaves as follows:
 
 - the value is applied before the command runs, so the caller need not pass it.
-- setting that flag on the command line is an error, not an override.
-- aliases are covered, since the check is on the canonical flag name.
-- a locked flag that the selected command does not define is ignored rather than
-  an error, so per-command flags can be locked without breaking other commands.
+- setting that flag to a different value is an error, not an override.
+- a lock matches the canonical flag name, so aliases are covered and it takes effect
+  wherever the selected command has a flag of that name.
 - a locked output mode wins over the competing one. With `json` locked, `GOG_PLAIN`
   gives way silently, since an environment default is a preference rather than a
   request, while an explicit `--plain` is refused for asking output the profile
   forbids.
-- commands that build partial requests from which flags were given treat a locked
-  flag as given, so the locked value reaches the request.
-- override errors name the flag and profile without printing the locked value.
 
-Non-boolean flags are refused. `--help` and `--version`, which Kong executes before
-normal parsing, are refused too, as are `--home`, required flags, and names matching
-no flag at all. In each case the binary declines to run instead of reporting a
-guarantee it does not have.
+A lock the binary cannot enforce is refused at build time rather than silently
+ignored.
 
-Setting a locked flag fails before the command handler runs:
-
-```text
-flag --sanitize-content is locked by baked safety profile "agent-safe-locked"
-```
-
-Because a locked value can make a command reject a combination the caller never
-asked for, usage errors name the locked flags:
+Because a locked value reaches the command without appearing on the command line, a
+usage error that names a locked flag carries a note saying where the value came from:
 
 ```text
 --sanitize-content cannot be used with --format raw

internal/cmd/root.go

@@ -456,10 +456,8 @@ func reportEarlyError(w io.Writer, err error) error {
 	return err
 }
 
-// errorMessage renders a command's error for display. Usage errors carry the
-// locked-flag note, because a baked profile can supply a value the caller never
-// passed and the rejection then names a flag absent from their command line. The
-// pre-run enforcement errors skip it: those name the locked flag themselves.
+// errorMessage formats err for display and appends a special locked-flag note to
+// errors that mention another locked flag, as when those flags are mutually exclusive.
 func errorMessage(err error) string {
 	msg := strings.TrimSpace(errfmt.Format(err))
 	if msg == "" {
@@ -468,7 +466,11 @@ func errorMessage(err error) string {
 	if ExitCode(err) != 2 {
 		return msg
 	}
-	note := lockedFlagsNote()
+	var refusal lockedFlagRefusal
+	if errors.As(err, &refusal) {
+		return msg
+	}
+	note := lockedFlagsNote(msg)
 	if note == "" {
 		return msg
 	}

internal/cmd/safety_profile.go

@@ -46,24 +46,26 @@ func enforceBakedSafetyProfile(kctx *kong.Context) error {
 	return nil
 }
 
-// lockedFlagNames records the flags enforceLockedFlags set, so a command rejecting a
-// value it never received on the command line can say where that value came from.
+// lockedFlagNames records the locked flags in force for this parse, so a command
+// rejecting a value it never received on the command line can say where it came from.
 var lockedFlagNames = map[string]bool{}
 
 func resetLockedFlagState() {
 	lockedFlagNames = map[string]bool{}
 }
 
-// lockedFlagsNote names the locked flags for display beneath a usage error. A command
-// can reject a combination involving a value the caller never passed, so the note is
-// what explains where that value came from. Empty when nothing is locked.
-func lockedFlagsNote() string {
-	if len(lockedFlagNames) == 0 {
-		return ""
-	}
+// lockedFlagsNote names the locked flags for display beneath a usage error that
+// mentions one of them: a command can reject a combination involving a value that
+// never appeared on the command line, and the note is what explains where it came from.
+func lockedFlagsNote(msg string) string {
 	names := make([]string, 0, len(lockedFlagNames))
+	mentioned := false
 	for name := range lockedFlagNames {
 		names = append(names, "--"+name)
+		mentioned = mentioned || strings.Contains(msg, "--"+name)
+	}
+	if !mentioned {
+		return ""
 	}
 	sort.Strings(names)
 	return fmt.Sprintf("note: %s locked by baked safety profile %q", strings.Join(names, ", "), bakedSafetyProfileName())
@@ -110,10 +112,6 @@ func verifyLockedFlagsExist(root *kong.Node) error {
 // profile that reports a guarantee the run never had.
 func lockUnsupported(flag *kong.Flag) error {
 	switch {
-	case flag.Name == "home":
-		// The layout resolver reads --home straight from argv before Kong parses, so
-		// config and credential roots are already chosen by the time locks run.
-		return usagef("baked safety profile %q locks --home, which is read before flags are parsed and cannot be locked", bakedSafetyProfileName())
 	case flag.Name == "help" || flag.Name == "version":
 		// Kong executes these flags from a BeforeReset hook and exits before normal
 		// defaults, validation, and locked-flag enforcement run.
@@ -128,9 +126,16 @@ func lockUnsupported(flag *kong.Flag) error {
 	return nil
 }
 
+// lockedFlagRefusal marks a rejection that already names the locked flag, so the
+// display layer knows not to add the note explaining where a locked value came from.
+type lockedFlagRefusal struct{ error }
+
+func (e lockedFlagRefusal) Unwrap() error { return e.error }
+
 // enforceLockedFlags applies the profile's locked flag values and refuses a command
-// line that sets one of them. The value is locked rather than merely defaulted so it
-// holds without help from the environment, which the caller may not control.
+// line that sets one of them to a different value. The value is locked rather than
+// merely defaulted so it holds without help from the environment, which the caller
+// may not control.
 func enforceLockedFlags(kctx *kong.Context) error {
 	// Rebuilt per parse: carrying names over would let one run's note describe a
 	// profile that is not in force.
@@ -144,17 +149,21 @@ func enforceLockedFlags(kctx *kong.Context) error {
 		if !locked {
 			continue
 		}
-		if flagOnCommandLine(kctx, flag.Name) {
-			return usagef("flag --%s is locked by baked safety profile %q", flag.Name, bakedSafetyProfileName())
-		}
+		// Recorded before anything here can fail, so the note names the profile's locks
+		// instead of however many the loop applied before it stopped.
+		lockedFlagNames[flag.Name] = true
 		// Decode into a zero target so the locked boolean replaces any environment or
 		// default value already resolved by Kong.
 		lockedTarget := reflect.New(flag.Target.Type()).Elem()
 		if err := flag.Parse(kong.ScanFromTokens(kong.Token{Type: kong.FlagValueToken, Value: value}), lockedTarget); err != nil {
 			return usagef("locked boolean flag --%s could not be applied", flag.Name)
 		}
+		// Requesting the value the profile already locks is not an override: refusing it
+		// would break every caller that asks for the safe setting while protecting nothing.
+		if flagOnCommandLine(kctx, flag.Name) && !reflect.DeepEqual(flag.Target.Interface(), lockedTarget.Interface()) {
+			return lockedFlagRefusal{usagef("flag --%s is locked to %s by baked safety profile %q", flag.Name, value, bakedSafetyProfileName())}
+		}
 		flag.Apply(lockedTarget)
-		lockedFlagNames[flag.Name] = true
 		lockedPaths = append(lockedPaths, &kong.Path{Flag: flag})
 	}
 	if len(lockedPaths) == 0 {

internal/cmd/safety_profile_locked_flags_test.go

@@ -14,16 +14,15 @@ gmail:
   search: true
 `
 
-// A lock must hold whatever spelling reaches it, including the value it already
-// has: accepting a matching value would make the lock depend on what the caller
-// asked for.
-func TestLockedFlag_RejectsEveryFormOfSettingIt(t *testing.T) {
+// A lock must hold whatever spelling asks for a different value, aliases included.
+func TestLockedFlag_RejectsEveryFormOfOverridingIt(t *testing.T) {
 	for _, arg := range []string{
 		"--sanitize-content=false",
-		"--sanitize-content=true",
-		"--sanitize-content",
+		"--sanitize-content=0",
+		"--sanitize-content=FALSE",
+		"--sanitize-content=no",
 		"--sanitize=false",
-		"--safe",
+		"--safe=false",
 	} {
 		t.Run(arg, func(t *testing.T) {
 			withBakedSafetyProfile(t, lockedFlagsProfile)
@@ -41,6 +40,55 @@ func TestLockedFlag_RejectsEveryFormOfSettingIt(t *testing.T) {
 	}
 }
 
+// Every spelling of the locked value counts as matching, because the comparison is
+// between parsed booleans and not between the strings the caller and profile wrote.
+func TestLockedFlag_AcceptsTheLockedValue(t *testing.T) {
+	for _, arg := range []string{
+		"--sanitize-content",
+		"--sanitize-content=true",
+		"--sanitize-content=1",
+		"--sanitize-content=TRUE",
+		"--sanitize-content=yes",
+		"--safe",
+	} {
+		t.Run(arg, func(t *testing.T) {
+			withBakedSafetyProfile(t, lockedFlagsProfile)
+			result := executeWithTestRuntime(t, []string{
+				"--json", "--account", "a@b.com",
+				"gmail", "get", "m1", arg,
+			}, nil)
+			if result.err != nil && strings.Contains(result.err.Error(), "is locked") {
+				t.Fatalf("%s asks for the locked value and must not be refused: %v", arg, result.err)
+			}
+		})
+	}
+}
+
+func TestLockedFlag_OverrideRefusalOmitsTheNote(t *testing.T) {
+	withBakedSafetyProfile(t, lockedFlagsProfile)
+	result := executeWithTestRuntime(t, []string{
+		"--json", "--account", "a@b.com",
+		"gmail", "get", "m1", "--sanitize-content=false",
+	}, nil)
+	if result.err == nil {
+		t.Fatalf("override must fail, got stdout=%q", result.stdout)
+	}
+	if strings.Contains(result.stderr, "note:") {
+		t.Fatalf("refusal must not append the locked-flag note: %q", result.stderr)
+	}
+}
+
+func TestLockedFlag_UnrelatedErrorCarriesNoNote(t *testing.T) {
+	withBakedSafetyProfile(t, lockedFlagsProfile)
+	result := executeWithTestRuntime(t, []string{"--json", "gmail", "get", "m1"}, nil)
+	if result.err == nil {
+		t.Fatalf("missing account must fail, got stdout=%q", result.stdout)
+	}
+	if strings.Contains(result.stderr, "note:") {
+		t.Fatalf("unrelated error must not carry the locked-flag note: %q", result.stderr)
+	}
+}
+
 // Locking a flag one command declares must not disturb commands that do not, or
 // locking a per-command flag would break the rest of the CLI.
 func TestLockedFlag_InertOnCommandsWithoutTheFlag(t *testing.T) {
@@ -299,7 +347,7 @@ version: true
 	if result.err == nil {
 		t.Fatalf("locking --home must fail, got stdout=%q", result.stdout)
 	}
-	if !strings.Contains(result.err.Error(), "locks --home") {
+	if !strings.Contains(result.err.Error(), "only boolean flags can be locked") {
 		t.Fatalf("err = %v", result.err)
 	}
 }