feat(payload): support persisted disable toggles#2741
feat(payload): support persisted disable toggles#2741xwil1 wants to merge 3 commits intorouter-for-me:devfrom
Conversation
|
This pull request targeted The base branch has been automatically changed to |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to disable specific payload rewrite rules and individual parameters within those rules via configuration. It adds Disabled and DisabledParams fields to the PayloadRule and PayloadFilterRule structs, updates the execution logic to respect these flags, and includes comprehensive unit tests. Feedback focuses on optimizing performance by moving string trimming and map allocations from the request path to the configuration loading phase, as well as ensuring the new fields are properly sanitized during initialization.
| func payloadParamDisabled(disabled map[string]struct{}, path string) bool { | ||
| if len(disabled) == 0 { | ||
| return false | ||
| } | ||
| _, ok := disabled[strings.TrimSpace(path)] | ||
| return ok | ||
| } |
There was a problem hiding this comment.
The payloadParamDisabled function performs a strings.TrimSpace on every call. Since this is executed within a loop for every parameter of every matching rule on every request, it introduces significant redundant work. It is better to ensure that Params keys and DisabledParams entries are trimmed once during configuration loading in internal/config/config.go. This would allow this check to be a simple map lookup or a linear search through a slice without repeated string manipulations.
| func payloadDisabledParamSet(paths []string) map[string]struct{} { | ||
| if len(paths) == 0 { | ||
| return nil | ||
| } | ||
| out := make(map[string]struct{}, len(paths)) | ||
| for _, path := range paths { | ||
| trimmed := strings.TrimSpace(path) | ||
| if trimmed == "" { | ||
| continue | ||
| } | ||
| out[trimmed] = struct{}{} | ||
| } | ||
| if len(out) == 0 { | ||
| return nil | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
The payloadDisabledParamSet function creates a new map for every matching rule that has DisabledParams. For a proxy handling many requests, these frequent allocations can impact performance. Consider pre-calculating these sets during configuration initialization or using a simple slice search if the number of disabled parameters is small (which is the typical case). If the list is small, a linear search avoids the overhead of map allocation and hashing entirely.
| // Params lists JSON paths (gjson/sjson syntax) to remove from the payload. | ||
| Params []string `yaml:"params" json:"params"` | ||
| // DisabledParams lists JSON paths that should remain visible in config but not be applied. | ||
| DisabledParams []string `yaml:"disabled-params,omitempty" json:"disabled-params,omitempty"` |
There was a problem hiding this comment.
The DisabledParams field (and the keys in Params) should be sanitized during configuration loading. The current SanitizePayloadRules implementation in this file (around line 713) appears to skip these new fields and the standard Default/Override rules. Trimming these strings at load time would eliminate the need for expensive strings.TrimSpace calls and map allocations during request processing in the executor.
There was a problem hiding this comment.
Thanks. I addressed the correctness issue in d9d5c4e by teaching SanitizePayloadRules to skip validation for disabled raw params. I am intentionally deferring the broader load-time normalization and precomputation optimization for this PR to keep the fix scoped and avoid introducing unrelated behavior changes.
![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: 1f16781b10
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // DisabledParams lists JSON paths that should remain visible in config but not be applied. | ||
| DisabledParams []string `yaml:"disabled-params,omitempty" json:"disabled-params,omitempty"` |
There was a problem hiding this comment.
Adding disabled-params to PayloadRule here makes it possible to keep raw payload entries in config while toggling them off, but sanitizePayloadRawRules still validates every raw param and drops the whole rule on any invalid JSON (internal/config/config.go:721-747). In practice, if a disabled raw param contains an in-progress/invalid JSON fragment, config load removes the entire rule, so even enabled params in that rule stop applying; this breaks the persisted-toggle workflow for default-raw and override-raw.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d9d5c4e. sanitizePayloadRawRules now skips JSON validation for paths listed in disabled-params, so an in-progress invalid raw fragment no longer drops the whole rule. I also added regression coverage for both cases: disabled invalid raw params keep the rule intact, while enabled invalid raw params still cause the rule to be dropped.
|
This pull request targeted The base branch has been automatically changed to |
Add disabled and disabled-params support for payload rewrite rules so conflicting or uncertain model parameters can be toggled during debugging instead of being repeatedly deleted and re-added.
xwil1
force-pushed
the
feat/payload-disable-native
branch
from
1f16781 to
a5d991a
Compare
Keep raw rules intact when disabled params contain in-progress JSON so enabled params still apply.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9d5c4eeb7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| disabledParams := payloadDisabledParamSet(rule.DisabledParams) | ||
| invalid := false | ||
| for path, value := range rule.Params { | ||
| if payloadParamDisabled(disabledParams, path) { |
There was a problem hiding this comment.
sanitizePayloadRawRules now ignores disabled-params, but it still validates every raw rule even when rule.Disabled is true, so a disabled default-raw/override-raw rule with an in-progress invalid JSON fragment is dropped during config load. That breaks the persisted-toggle workflow introduced in this commit because disabling a rule is no longer enough to keep it around for later re-enable/edit, and the rule can be lost from runtime state on reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a44447a. sanitizePayloadRawRules() now preserves fully disabled raw rules before validating params, so in-progress invalid JSON can stay in YAML without the rule being dropped during config sanitization. I also added focused regression coverage in internal/config/payload_sanitize_test.go for both cases: disabled raw rules are preserved, while enabled raw rules with invalid JSON are still dropped. Re-verified with go test ./internal/config -run TestSanitizePayloadRules_ and go test ./internal/runtime/executor -run TestApplyPayloadConfigWithRoot_.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Thanks for the feature here. I found one blocking issue in the raw-rule sanitization path.
Blocking:
sanitizePayloadRawRules()still validates raw params for rules markeddisabled: true. As a result, a disableddefault-raworoverride-rawrule that contains in-progress or invalid JSON is dropped during config sanitization before runtime ever gets a chance to skip it.- That breaks the main workflow this PR is trying to enable: keeping conflicting or unfinished rules in YAML and temporarily disabling them instead of deleting and recreating them.
Repro:
- Add a
default-rawrule withdisabled: trueand an invalid raw JSON fragment such asmetadata: '{"enabled":'. - Load the config and call
SanitizePayloadRules(). - The rule is removed entirely, even though it is disabled.
I verified this with a temporary focused test against internal/config, and it fails on the current PR.
Please make sanitization ignore fully disabled raw rules as well, and add a regression test for that case.
Preserve disabled raw rules during config sanitization so in-progress JSON can stay in YAML without dropping the rule.
|
Addressed the blocking raw-rule sanitization case in a44447a.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a44447a528
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if len(disabled) == 0 { | ||
| return false | ||
| } | ||
| _, ok := disabled[strings.TrimSpace(path)] |
There was a problem hiding this comment.
ApplyPayloadConfigWithRoot normalizes rule paths via buildPayloadPath (it strips a leading . when a root is set), but payloadParamDisabled only trims whitespace. That means a rule param like .metadata.trace_id and disabled-params: ["metadata.trace_id"] are treated as different strings, so the supposedly disabled param is still applied. This breaks the new toggle behavior for rooted payload flows (for example Gemini CLI/Antigravity requests) whenever users mix equivalent dotted path forms.
Useful? React with 👍 / 👎.
3 participants
Summary
disabledanddisabled-paramssupport for payload rewrite and filter rules so test rules can be toggled without deleting YAML entriesconfig.example.yamlWhy
When testing payload rewrites for different models, many parameters cannot be confirmed in one pass and conflicting rules are common. The old workflow required repeatedly deleting and re-adding YAML rules while iterating, which made debugging painful and error-prone. This PR adds a persisted disable state so rules can stay in place while being temporarily ignored by the backend.
The management UI has also been updated locally to expose the same disable controls and was validated together with this backend behavior.
Test plan
go test ./internal/runtime/executor -run ApplyPayloadConfigWithRoot -v