Skip to content

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

Merged
steipete merged 1 commit into
openclaw:mainfrom
ronny-rentner:fix/locked-flags
Aug 11, 2026
Merged

fix(safety-profile): accept the value a locked flag already sets and note it only where it is mentioned#982
steipete merged 1 commit into
openclaw:mainfrom
ronny-rentner:fix/locked-flags

Conversation

@ronny-rentner

Copy link
Copy Markdown
Contributor

Unfortunately, it turns out I've missed that locking a flag refuses any command line
that mentions it, including one asking for the value the profile already locks:
gog gmail get <id> --sanitize-content failed with exit 2 against a profile that
locks sanitize-content: true. Only a different value is an override now.

Two note defects showed up alongside it. The note was assembled as enforcement walked
the flags, so on a refusal it named the locks applied before the failure and omitted
the flag the error itself named; the locks are recorded up front now, and a refusal
renders without a note. It also rode along on errors that had nothing to do with
locks, a missing account among them, and now attaches only where the message mentions
a locked flag.

lockUnsupported's --home case is gone. --home is a string flag, so the boolean
rule already refuses it; the case only changed which message you got, and its premise,
that locks run too late for it, never applied.

Docs. The section described the old contract, and stated several rules more than
once:

  • "setting that flag on the command line is an error" becomes "to a different value".
  • the override-error example is dropped, since the bullet above it states the rule.
  • the alias bullet and the command-scope bullet both said matching is by canonical
    name, so they are one bullet.
  • the bullet about commands building partial requests is dropped. It described
    flagProvided's internals, which is not something a profile author decides.
  • the list of unlockable flags is replaced by the principle behind it: a lock the
    binary cannot enforce is refused at build time rather than silently ignored.
  • the note paragraph said a locked value makes a command reject a combination "the
    caller never asked for". They did ask for it, by running the baked binary; what is
    true is that it never appeared on their command line.

Proof — run against a configured account; $ID is a message id, held in a
variable rather than printed.

$ ./gog-before gmail get $ID --json --sanitize-content
flag --sanitize-content is locked by baked safety profile "demo"
note: --no-input locked by baked safety profile "demo"

$ ./gog-fixed gmail get $ID --json --sanitize-content | jq -c keys
["body","headers","message"]

$ ./gog-fixed gmail get $ID --json --sanitize-content=false
flag --sanitize-content is locked to true by baked safety profile "demo"

$ ./gog-fixed gmail get $ID --json --format raw
--sanitize-content cannot be used with --format raw
note: --no-input, --sanitize-content locked by baked safety profile "demo"

$ ./gog-fixed gmail get --json
expected "<messageId>"

The demo profile locks no-input and sanitize-content. Both binaries were built
from clean trees, gog-before from main and gog-fixed from this branch.

Tests. RejectsEveryFormOfSettingIt asserted the old contract and is now
RejectsEveryFormOfOverridingIt, covering =false, =0, =FALSE, =no,
--sanitize=false and --safe=false. Added: AcceptsTheLockedValue
(--sanitize-content, =true, =1, =TRUE, =yes, --safe),
OverrideRefusalOmitsTheNote, and UnrelatedErrorCarriesNoNote. HomeIsRefused now
asserts the boolean-rule message. Both note tests were checked against the unfixed
code to confirm they fail there.

go test ./... passes.

My check originally.

@clawsweeper

clawsweeper Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Aug 11, 2026
@clawsweeper

clawsweeper Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Codex review: found issues before merge. Reviewed August 11, 2026, 10:08 AM ET / 14:08 UTC.

ClawSweeper review

What this changes

The PR permits explicitly supplied boolean values that match baked safety-profile locks and narrows lock-origin notes to relevant usage errors.

Merge readiness

⚠️ Needs maintainer review before merge - 4 items remain

Keep open for a maintainer decision: this intentionally changes a documented safety-profile contract from refusing every explicit locked flag to allowing matching values. Likely related people: Peter Steinberger (high confidence) and Ronny Rentner (medium confidence).

Priority: P2
Reviewed head: a24845ac33e7d8d8569b88f5f8b020eb7d1cc2e7
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The patch is focused and has credible live proof, but its deliberate safety-profile compatibility change needs owner approval.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The PR includes a redacted before/after terminal transcript from a configured Gmail account showing the matching-value invocation succeeds after the patch and a differing value remains refused.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR includes a redacted before/after terminal transcript from a configured Gmail account showing the matching-value invocation succeeds after the patch and a differing value remains refused.
Evidence reviewed 6 items Current documented contract: Current main says that setting a locked flag on the command line is an error, including when the requested value matches the baked value.
Current implementation and regression coverage: Current main refuses every command-line occurrence of a locked flag before applying its value, and its test explicitly covers matching true and alias forms as rejections.
Feature provenance: The current behavior dates to the locked-flag feature commit, whose message and tests describe rejecting every explicit setting, including a matching value.
Findings 1 actionable finding [P1] Preserve the documented all-explicit-flag refusal contract
Security None None.

How this fits together

Baked safety profiles constrain which gog commands and boolean options can run before a Google Workspace command executes. The CLI parser applies locks, validates the resulting invocation, and formats any usage error for stderr.

flowchart LR
  Profile[Baked safety profile] --> Parse[CLI flag parsing]
  Args[User command arguments] --> Parse
  Parse --> Locks[Locked flag enforcement]
  Locks --> Validate[Command validation]
  Validate --> Command[Google Workspace command]
  Validate --> Errors[CLI error output]
Loading

Decision needed

Question Recommendation
Should baked safety profiles continue to reject every explicit locked flag, or should a matching boolean value be accepted? Keep refusal semantics: Restore rejection for all explicitly supplied locked flags while preserving the independent error-note fixes.

Why: Current main deliberately documents and tests refusal even for matching values; changing that observable safety-profile boundary requires owner intent rather than a reviewer inference.

Before merge

  • Preserve the documented all-explicit-flag refusal contract (P1) - Current main deliberately documents and tests rejection even for --sanitize-content=true; these lines instead permit it and let the command run. That changes a safety-profile CLI contract for existing current-main automation, so retain the refusal behavior unless the safety-profile owner explicitly approves this compatibility change.
  • Resolve merge risk (P1) - Existing current-main automation that relies on the documented exit-2 refusal for any explicit locked flag would instead execute when it supplies the matching value.
  • Complete next step (P2) - A safety-profile owner must choose the compatibility contract before the branch can be assessed as merge-ready.

Findings

  • [P1] Preserve the documented all-explicit-flag refusal contract — internal/cmd/safety_profile.go:161-165
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 4 files, +98 / -51 lines The patch spans enforcement, error rendering, tests, and the public safety-profile contract.
Production versus test delta production +35 / -24; tests +56 / -8 Most added code is focused regression coverage for the revised lock semantics.

Merge-risk options

Maintainer options:

  1. Preserve the current contract (recommended)
    Restore refusal for matching explicit values and retain only the error-note cleanup until the policy direction is separately approved.
  2. Accept the compatibility change
    Approve the value-based contract and accept that callers which formerly received exit 2 for a matching flag will now proceed.

Technical review

Best possible solution:

Keep the existing refusal contract while retaining the scoped error-note corrections, unless a safety-profile owner explicitly approves value-based lock semantics and its compatibility change.

Do we have a high-confidence way to reproduce the issue?

Yes—current main deterministically refuses any command-line occurrence of a locked flag, and the contributor supplies a before/after configured-account terminal transcript for the matching-value case.

Is this the best way to solve the issue?

Unclear—the implementation keeps the final boolean value immutable, but it intentionally replaces the documented refusal contract and needs explicit safety-profile owner approval.

Full review comments:

  • [P1] Preserve the documented all-explicit-flag refusal contract — internal/cmd/safety_profile.go:161-165
    Current main deliberately documents and tests rejection even for --sanitize-content=true; these lines instead permit it and let the command run. That changes a safety-profile CLI contract for existing current-main automation, so retain the refusal behavior unless the safety-profile owner explicitly approves this compatibility change.
    Confidence: 0.95

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against cd965f1d38bb.

Labels

Label changes:

  • add P2: This is a bounded safety-profile behavior change with compatibility implications, but it is not a demonstrated user-facing outage.
  • add merge-risk: 🚨 compatibility: The branch changes documented current-main behavior from refusing any explicit locked flag to permitting matching values.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR includes a redacted before/after terminal transcript from a configured Gmail account showing the matching-value invocation succeeds after the patch and a differing value remains refused.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR includes a redacted before/after terminal transcript from a configured Gmail account showing the matching-value invocation succeeds after the patch and a differing value remains refused.

Label justifications:

  • P2: This is a bounded safety-profile behavior change with compatibility implications, but it is not a demonstrated user-facing outage.
  • merge-risk: 🚨 compatibility: The branch changes documented current-main behavior from refusing any explicit locked flag to permitting matching values.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR includes a redacted before/after terminal transcript from a configured Gmail account showing the matching-value invocation succeeds after the patch and a differing value remains refused.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR includes a redacted before/after terminal transcript from a configured Gmail account showing the matching-value invocation succeeds after the patch and a differing value remains refused.

Evidence

What I checked:

  • Current documented contract: Current main says that setting a locked flag on the command line is an error, including when the requested value matches the baked value. (docs/safety-profiles.md:187, cd965f1d38bb)
  • Current implementation and regression coverage: Current main refuses every command-line occurrence of a locked flag before applying its value, and its test explicitly covers matching true and alias forms as rejections. (internal/cmd/safety_profile.go:147, cd965f1d38bb)
  • Feature provenance: The current behavior dates to the locked-flag feature commit, whose message and tests describe rejecting every explicit setting, including a matching value. (internal/cmd/safety_profile_locked_flags_test.go:17, 6af89c8dd7cb)
  • Proposed contract change: The branch compares the parsed caller value with the locked target and permits equality, so a formerly exit-2 invocation can reach the command handler. (internal/cmd/safety_profile.go:161, a24845ac33e7)
  • After-fix behavior proof: The PR body includes a before/after terminal transcript against a configured Gmail account: the matching flag fails on main and returns real Gmail JSON keys on the branch; it also demonstrates refusal of a differing value. (a24845ac33e7)
  • Release timing: The locked-flag feature is after the v0.35.0 release commit, so this is an unreleased current-main contract rather than a shipped-release regression. (6af89c8dd7cb)

Likely related people:

  • Peter Steinberger: Authored the current locked-flag feature and the central enforcement, tests, and documentation. (role: introduced current behavior; confidence: high; commits: 6af89c8dd7cb; files: internal/cmd/safety_profile.go, internal/cmd/safety_profile_locked_flags_test.go, docs/safety-profiles.md)
  • Ronny Rentner: The original locked-flag feature commit credits Ronny as co-author, connecting this contributor to the existing safety-profile design. (role: co-authored locked-flag feature; confidence: medium; commits: 6af89c8dd7cb; files: internal/cmd/safety_profile.go, internal/cmd/safety_profile_locked_flags_test.go)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Obtain an explicit safety-profile owner decision on whether matching explicit values should be accepted.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit 0121e78 into openclaw:main Aug 11, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants