feat(safety-profile): lock flag values with locked-flags - #976
feat(safety-profile): lock flag values with locked-flags#976ronny-rentner wants to merge 10 commits into
Conversation
…ofile can fix output settings the command line cannot change
… the locked variants to the read path
… which flags not to lock
…erator output; rebuild locked names per parse
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
|
Codex review: needs changes before merge. Reviewed August 10, 2026, 3:02 PM ET / 19:02 UTC. ClawSweeper reviewWhat this changesThe PR adds a Merge readinessKeep open: the prior blockers are addressed, but false-valued Priority: P2 Review scores
Verification
How this fits togetherSafety profiles compile command restrictions into gog binaries before commands execute. This change injects profile-controlled flag values after parsing and before output-mode resolution and command execution. flowchart LR
A[Safety profile YAML] --> B[Profile generator]
B --> C[Compiled gog binary]
D[CLI arguments and environment] --> E[Command parser]
C --> F[Locked-flag enforcement]
E --> F
F --> G[Output-mode resolution]
G --> H[Command execution]
Decision needed
Why: The mechanism is a new policy surface, and the PR explicitly leaves whether to ship the two new presets as a maintainer choice. Before merge
Findings
Agent review detailsSecurityNeeds attention: False-valued output locks are enforced as enabled modes, so this security-policy feature needs a narrow correction before merge. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Resolve output-mode conflicts from the enforced boolean values so false locks prevent only their own flag and leave the opposite mode available. Do we have a high-confidence way to reproduce the issue? Yes, from source: a profile with Is this the best way to solve the issue? No; output precedence must distinguish a true lock from a false lock while retaining explicit-argument rejection for genuinely enabled locked modes. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 71c6c1e63787. LabelsLabel justifications:
EvidenceSecurity concerns:
Acceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (2 earlier review cycles)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fee967bd12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if flagProvided(kctx, flag.Name) { | ||
| return usagef("flag --%s is locked by baked safety profile %q", flag.Name, bakedSafetyProfileName()) | ||
| } | ||
| if err := flag.Value.Parse(kong.ScanFromTokens(kong.Token{Type: kong.FlagValueToken, Value: value}), flag.Value.Target); err != nil { |
There was a problem hiding this comment.
Parsing the locked value directly into the target does not add the flag to kctx.Path, so flagProvided() remains false. Commands whose behavior depends on presence therefore silently ignore the lock: for example, locking contacts-only: true for gmail vacation update sets the struct field but internal/cmd/gmail_vacation.go never adds restrict_to_contacts to the update, and contacts/calendar/task update handlers have the same pattern. Record injected locks as provided or make the presence checks aware of them.
Useful? React with 👍 / 👎.
| for name, raw := range entries { | ||
| flag := strings.TrimSpace(strings.ToLower(name)) | ||
| if flag == "" { | ||
| return fmt.Errorf("empty flag name") | ||
| } | ||
| switch typed := raw.(type) { |
There was a problem hiding this comment.
Any non-empty name is accepted and baked without checking it against the CLI model, while runtime enforcement only queries locks for flags in kctx.Flags(). A typo such as readonlly: true therefore builds successfully and is ignored on every invocation, potentially leaving mutations enabled even though the profile appears to enforce read-only operation. Validate that each locked name matches at least one canonical CLI flag while still allowing per-command flags to be absent from unrelated commands.
Useful? React with 👍 / 👎.
| if err = enforceLockedFlags(kctx); err != nil { | ||
| return reportEarlyError(runtimeIO.Err, err) |
There was a problem hiding this comment.
When a profile locks home and the caller omits --home, this hook updates cli.Home only after preScanHomeArg() and bindRuntimeLayoutResolver() have already selected the default config/data roots. Commands consequently keep reading the user's normal configuration and credentials rather than the locked directory, despite the flag target showing the locked value. The locked home must participate in layout binding before the resolver is constructed, or this flag should be rejected as unsupported.
Useful? React with 👍 / 👎.
| if err = enforceLockedFlags(kctx); err != nil { | ||
| return reportEarlyError(runtimeIO.Err, err) |
There was a problem hiding this comment.
Locked values are injected only after parser.Parse(args) has completed, so a profile cannot supply a flag tagged required:"": Kong rejects the command as missing that flag before this code runs, while explicitly supplying it is also rejected as an override. Thus locks such as title, parent, or state-file can never satisfy commands that require them, contrary to the stated behavior that callers need not pass locked values. Inject locks before parse validation or reject required flags during profile generation.
Useful? React with 👍 / 👎.
…locked json or plain wins over the competing mode
… a locked one, override only environment defaults
clawsweeper
Bot
added
merge-risk: 🚨 security-boundary
|
Fixed in Precedence now runs after One behaviour decision worth surfacing — the competing flag is treated by origin:
Two explicit modes remain a conflict error, as before. Proof, with a throwaway profile locking only name: json-locked
locked-flags:
json: true
version: trueBoth previously failed with Four tests cover the CLI and environment combinations; I confirmed they're not vacuous by restoring the old ordering and watching them fail. Docs updated to state the environment-versus-explicit split. Not guarded, deliberately: a profile locking both The two P1s are unchanged and on your side; the PR description lists the preset options. |
…s that match nothing, and refuse locks on --home or required flags
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
All four addressed in Presence — Unknown names — the generator emits a count alongside the hashed lookup, and a pre-enforcement pass walks the CLI model counting matches. A name matching nothing refuses to run: The count rather than a name list is deliberate: emitting the names would have put them in the binary as patchable text and undone the hashing the rule matchers rely on. One difference from the finding as written: this refuses at first invocation rather than during profile validation, so a typo still builds. Validating at bake time would mean the generator constructing the Kong model to know which flags exist — happy to do it if you want the check there.
Six tests added, docs updated with the behaviour and the three refusals. |
|
Maintainer verdict: LAND, using the hardened maintainer stack at The core idea is sound, but a generic bool/int/string lock surface was too broad for a security boundary. The landing stack narrows The stack additionally fixes false-valued output locks, makes Proof run on the final stack: Final structured review found no accepted/actionable P0/P1 findings (confidence 0.94). The original PR CI failure was the reported staticcheck selector issue; the landing stack fixes it and the local lint gate is clean. |
2 participants
Safety profiles can allow or deny commands, but nothing in them can fix a flag value. Sanitized output happens only if the caller passes
--sanitize-content, and a flag that does take a value from the environment is still overridden by the command line. When that command line is written by a model rather than a person, neither is a setting an operator can rely on.This adds a
locked-flagsmapping to the profile format. A locked flag is applied before the command runs, and setting it on the command line is an error rather than an override.How it works.
parseRawreadslocked-flagsalongsideallow/denyand keeps it out of command flattening; values may be bool, int or string. The generator emitsbakedSafetyLockedFlagas a hashed switch with the same collision check as the rule matchers, so a locked flag name is no more patchable than a command rule.enforceLockedFlagsruns besideenforceBakedSafetyProfile, applies each value through the flag's own parser, and rejects a command line that sets one. Locks apply per selected command, so a locked flag a command doesn't declare is silently inert.Because a locked value can make a command reject a combination the caller never asked for, usage errors carry a note naming the locked flags and the profile. That's attached once in the error path, so no command carries profile knowledge.
Not a behaviour change. Profiles without
locked-flagsare unaffected, stock builds compile a stub returning "not locked", andreadonly.yaml/agent-safe.yamlare untouched.locked-flagscould not previously appear in a profile — an unknown top-level key was a parse error — so no existing profile changes meaning.On the two new profiles.
readonly-locked.yamlandagent-safe-locked.yamlare copies of their originals plus the locks above (andreadonly: trueon the read side, which also rejects a mutating request inside an allowed command). I added them as copies because I didn't know whether you want preset locked profiles shipped at all — they're easy to drop if the mechanism alone is what you want, or they could replace the existing presets instead of sitting beside them, if you'd rather the shipped agent profile be locked by default. That last option is a behaviour change for anyone buildingagent-safe, which is why I didn't do it. Note that lockingsanitize-contentmakesgmail get --format rawunavailable, since that combination is rejected; both descriptions say so.Proof — no mailbox or credentials involved. The lock check runs before account resolution, and the last command needs only an account string, failing at flag validation before any network call.
m1is a placeholder that is never used.The last one is the lock taking effect: nothing on that command line mentions
--sanitize-content, and the refusal only fires when it is true.Tests. Sixteen.
internal/safetyprofilecovers value parsing, non-scalar rejection, and locked flags not leaking into command rules.cmd/bake-safety-profilecovers the emitted hashed lookup, that the flag name never appears verbatim, and that profiles without locks still compile.internal/cmdcovers every spelling of setting a locked flag (including setting it to the value it already has), inertness on commands without the flag, stock builds ignoring locks, unparsable values failing loudly, an int lock reaching the command, and the note text.go test ./...passes exceptTestDocsWriteUpdate_FileInputErrors, which fails in my sandbox because/nonexistentreturnspermission deniedinstead ofno such file; unrelated, reproducible withcat.Docs.
docs/safety-profiles.mdgains a "Locked Flags" section covering syntax, semantics, and which flags not to lock, with--reply-allas the worked counter-example.