fix(gateway): scan csv variables element by element in guardrails - #60
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
e2b689e to
fa8663b
Compare
The length heuristic flags any PUBLIC value over 64 characters that has no spaces and no `http` prefix (gateway/internal/guardrails/guardrails.go:105 before this change). A `csv` value of short kebab-case feature flags fits that description once the list passes 64 characters. Under `strict_guardrails: true` (`--strict`) the gateway then refuses to start (gateway/internal/server/server.go:84-89), so adding one flag to a values file became a failed rollout. Reproduced with a 67-character comma list on v0.1.7. The same list with ", " separators started, because it then contained spaces. `Scan` scanned `v.Value` whole and had no way to learn the declared type. It now takes the loaded manifest (nil when there is none). A variable declared `type: csv` is split on commas, each element is trimmed, and every heuristic (known format, entropy, length) runs on each element. The check is not weakened: an 80-character opaque token inside a list is still flagged, and the warning and log name its position (`csv element 2: ...`, `csv_element=2`). A known-format token past the first element is now caught; before, only a prefix of the joined string was checked. Every other type, and any variable with no manifest entry, is scanned whole as before. Spec §3.3 and the guardrails section of the docs describe the per-element rule.
The repo used `FEATURE_FLAGS` as an example throughout but had no page
on using it. The new `guides/feature-flags` page, under Guides next to
the manifest guide, covers:
- one `FEATURE_FLAGS: {tier: public, type: csv, default: ""}` variable
rather than one variable per flag, with an optional pattern;
- reading it with the SDK, where a flag is off unless named;
- that flags are public and are not access control;
- the per-element guardrail, and the dev-plugin caveat;
- flipping a flag by changing the environment and restarting, or with
hot reload when the value comes from `--env-file`;
- what it isn't: no per-user targeting, rollouts or experiments.
The agents playbook and the README's SDK example link to it.
- gateway README "Upgrading / To 0.1.8": csv values are scanned element by element, warnings name the element (`csv_element`), and a known key format is now caught anywhere in the list. - Variable classification: the `csv_element` log attribute, and that the dev plugins scan whole values. - Vite and Next.js plugin READMEs and reference pages, and the local development guide: the plugins don't read the manifest, so with `strict` a long flag list can throw in development even though the gateway accepts it. Documented, not fixed: a fix needs a new plugin option or a YAML parser in two packages. - REP-RFC-0001 revision history: the §3.3 rule joins the 0.2.0 entry.
Simplify follow-up: - The caveat that the dev plugins scan csv values whole was copied in eight places, and the copies had started to disagree. It now lives once, under "Dev plugins" in the feature-flags guide. The other pages and the two plugin READMEs keep a single clause that links there. - The feature-flags guide drops sentences that repeated themselves. Instead of restating them, it links to Defaults, the guardrails section, Hot Reload, the Kubernetes recipe and the existing Next.js worked example.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| vars.ApplyDefaults(s.cfg.Manifest) |
There was a problem hiding this comment.
If a benign environment override masks a secret-shaped PUBLIC manifest default at startup, removing the override makes reload apply that default without scanning it or enforcing --strict. The gateway then publishes the value in HTML and an SSE update instead of rejecting it.
How this was verified: A strict gateway published a synthetic secret after reload, while startup with that default refused to start.
Artifacts
Go source for the focused reload and HTTP probe
- The authored test exercises startup, Reload, injected HTML, and SSE using a synthetic token; it is the executed source.
Go test overlay for the untracked probe
- The overlay supplies the authored test to the server package without editing tracked source files.
HTTP responses before removing the benign override
- The captured test run shows `200 OK` HTML and SSE responses while the override remains and the synthetic token is not published.
HTTP responses after removing the override and reloading
- The captured test run shows `200 OK` HTML and SSE responses containing the synthetic token after Reload, while a fresh strict startup rejects it.
| const enabled = new Set( | ||
| rep.get('FEATURE_FLAGS', '').split(',').map((f) => f.trim()).filter(Boolean), | ||
| ); | ||
|
|
||
| export const isOn = (flag: string) => enabled.has(flag); |
There was a problem hiding this comment.
The guide constructs enabled only once. After a hot-reload update, the SDK has the new flags, but the example’s isOn still checks the original Set. Readers following the example can make stale flag decisions until the page reloads.
Artifacts
- The authored script renders the verbatim guide snippet with the real SDK and sends an SSE flag update, establishing the executed source.
- The command output records its working directory, exit code, SDK values, and flag results before and after the SSE update, confirming the stale result.
▶ Guide example before hot reload
- Chromium renders the guide snippet with only `landing-page` enabled, showing `new-dashboard` off before the update.
Guide example before hot reload poster
- The captured browser frame shows the initial SDK value and the guide's matching off result.
▶ Guide example after SDK hot reload
- Chromium renders the same snippet after an SSE update, showing that the SDK includes `new-dashboard` while `isOn` still reports off.
Guide example after hot reload poster
- The captured browser frame shows the updated SDK value beside the stale guide result.
fa8663b to
e455a77
Compare
| You flip a flag by changing the environment, not the image: | ||
|
|
||
| - **Restart:** set `REP_PUBLIC_FEATURE_FLAGS` and restart the container or roll the deployment. | ||
| - **Hot reload:** with `--hot-reload` and `--env-file`, the gateway picks up edits to that file without a restart; it does not see changes to its own process environment. Pages using `rep.onChange('FEATURE_FLAGS', …)` or a framework adapter update in place. See [Hot Reload](/concepts/hot-reload/), the [Kubernetes recipe](/deployment/kubernetes/), and the end-to-end [Next.js example](/examples/nextjs-csr-embedded/). |
There was a problem hiding this comment.
The guide says --hot-reload with --env-file picks up file edits, but the default signal mode does not watch the file. Readers following these instructions will keep serving stale flags until they send SIGHUP. Explain that trigger or show a file-detection mode with its required options.
Artifacts
Authored live gateway reproduction script
- The script builds and launches the gateway, edits the env file, requests the injected payload, and sends a control SIGHUP; it contains the exact test commands.
Live gateway run and observed output
- Running the authored script exited successfully and recorded the launch arguments, unchanged value after the edit, changed value after SIGHUP, and gateway logs; file editing alone did not reload the flag.
Gateway response before the env-file edit
- An HTTP request to the running gateway returned the initial injected flag value `landing-page`; this establishes the comparison baseline.
Gateway response after the env-file edit without SIGHUP
- An HTTP request three seconds after editing the file still returned `landing-page`; the documented invocation did not pick up the edit.
Gateway response after sending SIGHUP
- An HTTP request after SIGHUP returned `landing-page,dark-mode`; the edited file was readable when reload was triggered.
| ```yaml | ||
| # .rep.yaml | ||
| variables: |
There was a problem hiding this comment.
The YAML labeled .rep.yaml omits the required top-level version. Copying it as a manifest makes rep validate fail before readers can use the feature flags. Add a version or clearly label the YAML as a fragment of an existing manifest.
Artifacts
Feature-flags manifest validation script
- The authored script extracts the guide’s YAML and invokes the CLI before and after adding only the required version, so both runs can be reproduced.
Copied manifest fails validation
- The captured CLI run exited 1 and reported the missing required `version` property, confirming the copied example fails.
Manifest with version passes validation
- The captured CLI run exited 0 after adding only `version: "0.1.0"`, isolating the omission.
1 participant
What
For a variable the manifest declares
type: csv, the guardrails now check each element (split on commas and trimmed) instead of the joined string. The three checks (known format, entropy, length) all still run on every element. Every other type is scanned whole, as before.This PR also adds a feature-flags guide and documents the new behaviour on every user-facing surface.
Why
Root cause (lines on
main)gateway/internal/guardrails/guardrails.go:105: any PUBLIC value over 64 characters, with no spaces and nohttpprefix, getslength_anomaly.guardrails.go:64-67:Scanonly ever seesv.Valueand cannot know its type.server.go:84-89: under--strict(orstrict_guardrails: true), any warning stops startup.A list of short kebab-case flags matches the length rule once it passes 64 characters, so adding one flag became a failed rollout. Reproduced on v0.1.7: a 67-character list was refused, and the same list with
", "separators started.How
guardrails.Scan(vars, m, logger)receives the loaded manifest (nilif there isn't one).scanValueruns once per trimmed element.csv element 2: …, with the log attributecsv_element=2.RFC
§3.3 states the per-element rule as a MUST. Its line sits under the same 0.2.0 entry in Appendix C that #59 creates.
Feature-flags docs
There was no guide before.
FEATURE_FLAGSappeared only as example values, in one worked Kubernetes example (examples/nextjs-csr-embedded), and in one-line snippets.The new
docs/src/content/docs/guides/feature-flags.mdxsits in the sidebar under Guides, next to Manifest File. It covers:FEATURE_FLAGS: {tier: public, type: csv, default: ""}variable, with an optional kebab-casepattern(tested against the realVarDecl.Check);--env-file, and never by rebuilding;The example is a generic version of StageFlow's
landing-pageflag, with no hostnames.User-facing docs
gateway/README.md: a csv bullet under Upgrading → To 0.1.8concepts/variable-classification.mdx:csv element N/csv_elementguides/manifest.mdx: thecsvrow links to the guideguides/feature-flags.mdx, added tosidebar.mjs(so it also appears inllms.txtand gets a.mdmirror)agents.mdx: source list links to the guideREADME.md: the SDK example links to the guideplugins/*/src/guardrails.ts:84), andstrictthrows (vite/src/index.ts:45-46,next/src/index.ts:77-78).guides/feature-flags.mdx#dev-plugins. One-clause links to it from:reference/plugins/vite.mdxreference/plugins/next.mdxguides/development.mdxplugins/vite/README.mdplugins/next/README.md--help:--strict("Exit on guardrail warnings") is still accurate, so no change.npm side effect. This PR touches
plugins/*/README.md, so release-please will include the npm packages in a docs-only patch release, as with #59'scli/changes.Tests
guardrails.TestScan_CSVJudgedByElement(9 cases):stringis flaggedserver.TestNew_StrictGuardrailsJudgeCSVByElement, underStrict: true:stringis refusedBefore:
After:
go vet, race tests,golangci-lint(0 issues) andgofmtare clean.pnpm -r buildandpnpm -r testpass.pnpm docs:buildsucceeds: 48 pages, and the#dev-pluginsanchor resolves from all four linking pages.Real binary under strict:
FEATURE_FLAGSvaluecsv_element=2Type
fix(patch)Scope
gatewayspecdocsChecklist