Skip to content

feat(safety-profile): lock boolean policy flags - #979

Merged
steipete merged 13 commits into
mainfrom
triage/t8-pr976
Aug 11, 2026
Merged

feat(safety-profile): lock boolean policy flags#979
steipete merged 13 commits into
mainfrom
triage/t8-pr976

Conversation

@steipete

Copy link
Copy Markdown
Collaborator

Lands #976 with maintainer hardening on top of @ronny-rentner's work. His
commits are preserved as authored; the additional commits narrow the design
after adversarial review found real bypasses in the generic form:

  • Scope narrowed to boolean policy flags. The original generic
    bool/int/string locking was unsafe to guarantee: non-boolean flags have too
    many injection shapes to enforce a lock honestly.
  • Two genuine bypasses fixed: a locked-to-false flag could be flipped, and
    GOG_AUTO_JSON=1 overrode a locked --json. Environment variables now only
    supply defaults; they never override a lock.
  • Pre-parse, unknown, and nothing-matching flag names are refused, as are
    locks on --home and required flags. Output-mode precedence resolves after
    locks so a locked json/plain beats a competing mode.
  • Regression coverage for the bypass shapes, lock inertness, and generator
    output; docs and changelog updated.

Enforcement proof from the built binary:

$ ./gog-lock-json-true --machine=false version
flag --json is locked by baked safety profile "live-json-true"
exit 2

$ GOG_AUTO_JSON=1 ./gog-lock-json-false version
v0.35.0-16-… (runs; env var no longer bypasses the lock)

make test, make lint (0 issues), and make docs-check (709 command pages)
all pass.

Closes #976. Thanks @ronny-rentner — the feature concept and the core
implementation are yours; the hardening keeps its promise enforceable.

ronny-rentner and others added 13 commits August 10, 2026 15:00
…ofile can fix output settings the command line cannot change
…erator output; rebuild locked names per parse
…locked json or plain wins over the competing mode
… a locked one, override only environment defaults
…s that match nothing, and refuse locks on --home or required flags
Co-authored-by: Ronny Rentner <github@ronny-rentner.de>
Co-authored-by: Ronny Rentner <github@ronny-rentner.de>
Co-authored-by: Ronny Rentner <github@ronny-rentner.de>
@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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Aug 11, 2026
@clawsweeper

clawsweeper Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed August 10, 2026, 10:52 PM ET / August 11, 2026, 02:52 UTC.

ClawSweeper review

What this changes

Adds an opt-in locked-flags safety-profile mapping that bakes boolean CLI values into a binary and rejects command-line, environment, and configuration overrides.

Merge readiness

⚠️ Ready for maintainer review - 4 items remain

Keep open: current main has no locked-flag profile capability, and this collaborator-authored PR adds a new safety-policy contract that needs explicit product sponsorship before merge. The patch is otherwise technically sound on review.

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

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused implementation, extensive regression coverage, and relevant built-binary proof support a good merge-ready patch once the product contract is sponsored.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): The PR body includes after-fix terminal output from built binaries showing both rejected command-line override and rejected environment-default bypass behavior.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The PR body includes after-fix terminal output from built binaries showing both rejected command-line override and rejected environment-default bypass behavior.
Evidence reviewed 5 items Current-main gap: Current main's safety-profile model contains only command allow/deny rules; it has no locked-flag representation or parser path.
Policy enforcement: The PR verifies locked names against the parser model, rejects unsupported flags, applies the value through the flag parser, and rejects explicit overrides.
Regression coverage: Coverage includes aliases, false-valued locks, output-mode precedence, ambient JSON defaults, unknown names, unsupported flags, and lock inertness.
Findings None None.
Security None None.

How this fits together

gog parses CLI arguments and ambient defaults into command settings before enforcing its baked safety profile. This change adds a policy step that injects locked boolean values and rejects overrides before the selected Google Workspace command runs.

flowchart LR
  A[CLI arguments] --> C[Argument parser]
  B[Environment and config defaults] --> C
  C --> D[Baked safety profile]
  D --> E[Locked flag enforcement]
  E --> F[Output-mode resolution]
  F --> G[Command execution]
Loading

Decision needed

Question Recommendation
Should gog support opt-in boolean flag locks as part of the baked safety-profile contract? Sponsor the narrow boolean-only contract: Merge the opt-in locking feature with its current no-default-profile-change scope and regression coverage.

Why: This adds a durable configuration and behavior surface rather than repairing an established contract; VISION.md asks for discussion before behavior changes that could affect scripts.

Before merge

  • Resolve merge risk (P1) - Custom baked profiles can intentionally change output and validation behavior for every invocation; a context-sensitive but valid boolean lock may make an otherwise allowed command fail validation.
  • Resolve merge risk (P1) - This feature extends the immutable safety-policy boundary, so its boolean-only scope and override precedence are compatibility- and security-sensitive contract decisions.
  • Complete next step (P2) - A collaborator-authored new configuration and safety-policy contract needs product approval; there is no narrow automated repair to perform.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Code and test growth production +294/-21; tests +490; docs/changelog +89/-2 The policy implementation is accompanied by substantial targeted regression coverage and user documentation.
Patch surface 14 files affected The change crosses profile parsing, code generation, CLI enforcement, tests, and documentation.

Merge-risk options

Maintainer options:

  1. Approve the opt-in policy contract (recommended)
    Accept the compatibility risk for custom baked profiles while retaining the boolean-only scope and existing-profile behavior.
  2. Pause the feature surface
    Defer the new locked-flags configuration contract if maintainers do not want profile settings to govern CLI value precedence.

Technical review

Best possible solution:

Adopt an opt-in, boolean-only locked-flags contract for custom baked profiles while leaving existing profiles unchanged and retaining parser validation plus built-binary regression coverage.

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

Not applicable: this PR introduces a new baked-profile capability rather than reporting a broken existing behavior. The PR body supplies built-binary after-fix terminal evidence for its intended policy behavior.

Is this the best way to solve the issue?

Yes, conditional on product sponsorship: boolean-only locks provide a narrow, maintainable implementation without changing existing stock or custom profiles that omit the new mapping.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: This is a bounded new safety-profile capability with normal-priority product and compatibility impact.
  • add merge-risk: 🚨 compatibility: Custom baked profiles gain new precedence rules that can alter CLI output and validation behavior.
  • add merge-risk: 🚨 security-boundary: The new mechanism is intended to make baked safety policy immutable against caller-controlled inputs.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from built binaries showing both rejected command-line override and rejected environment-default bypass behavior.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal output from built binaries showing both rejected command-line override and rejected environment-default bypass behavior.

Label justifications:

  • P2: This is a bounded new safety-profile capability with normal-priority product and compatibility impact.
  • merge-risk: 🚨 compatibility: Custom baked profiles gain new precedence rules that can alter CLI output and validation behavior.
  • merge-risk: 🚨 security-boundary: The new mechanism is intended to make baked safety policy immutable against caller-controlled inputs.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-fix terminal output from built binaries showing both rejected command-line override and rejected environment-default bypass behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix terminal output from built binaries showing both rejected command-line override and rejected environment-default bypass behavior.

Evidence

What I checked:

Likely related people:

  • steipete: Peter Steinberger introduced baked safety profiles and authored the final boolean-only hardening commit in this branch. (role: feature owner and recent area contributor; confidence: high; commits: f26af3adba6d, 6d30196ff912; files: internal/cmd/safety_profile.go, internal/safetyprofile/profile.go)
  • Drew Burchfield: Drew Burchfield added the existing generated-code hardening that this PR extends for locked flag lookups. (role: security-hardening contributor; confidence: high; commits: 46900109e029; files: internal/cmd/safety_profile.go, cmd/bake-safety-profile/main.go)

Rank-up moves

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

  • Obtain explicit maintainer approval for the new baked-profile configuration contract.

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 6af89c8 into main Aug 11, 2026
15 checks passed
@steipete
steipete deleted the triage/t8-pr976 branch August 11, 2026 02:56
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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants