Resolve env-dependent storybook.registry via allow-listed interpolation#3486
Conversation
clintandrewhall
temporarily deployed
to
integration-tests
GitHub Actions
Inactive
|
Warning Review limit reached
Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR adds allow-listed shell-style interpolation for Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/Elastic.Documentation.Configuration.Tests/ConfigurationFileStorybookRegistryTests.cs`:
- Around line 43-50: The test DisallowedVariable_IsLeftLiteral_AndWarns
currently asserts only that config.StorybookRegistry remains the literal and
does not contain the secret; update the test to also assert that the
DiagnosticsCollector (collector) recorded a warning. After creating config via
CreateConfiguration(...) and before completing the test, add an assertion that
collector contains at least one warning entry (or a specific warning message) to
verify the warning path is exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 433b691f-3bf0-4666-a3da-e958b6a42ba5
📒 Files selected for processing (15)
docs/syntax/storybook.mdsrc/Elastic.Documentation.Configuration/BuildContext.cssrc/Elastic.Documentation.Configuration/Builder/ConfigurationFile.cssrc/Elastic.Documentation.Configuration/EnvironmentInterpolation.cssrc/Elastic.Documentation/IDocumentationContext.cssrc/Elastic.Markdown/Myst/Directives/Storybook/StorybookBlock.cstests/Elastic.Documentation.Configuration.Tests/ApiConfigurationTests.cstests/Elastic.Documentation.Configuration.Tests/ConfigurationFileExcludeTests.cstests/Elastic.Documentation.Configuration.Tests/ConfigurationFileStorybookRegistryTests.cstests/Elastic.Documentation.Configuration.Tests/CrossLinkRegistryTests.cstests/Elastic.Documentation.Configuration.Tests/EnvironmentInterpolationTests.cstests/Elastic.Markdown.Tests/Directives/DirectiveBaseTests.cstests/Elastic.Markdown.Tests/Directives/StorybookTests.cstests/Navigation.Tests/TestDocumentationSetContext.cstests/authoring/Framework/CrossLinkResolverAssertions.fs
clintandrewhall
temporarily deployed
to
integration-tests
GitHub Actions
Inactive
clintandrewhall
force-pushed
the
storybook-registry-env-interpolation
branch
from
08611cd to
3018204
Compare
clintandrewhall
temporarily deployed
to
integration-tests
GitHub Actions
Inactive
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Elastic.Documentation.Configuration.Tests/ConfigurationFileReleaseNotesTests.cs (1)
151-167: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
MockDocumentationSetContext.Environmentwires up the real system environment, not a deterministic stub.Line 166 returns
SystemEnvironmentVariables.Instance, the live process-environment implementation, rather than a deterministic/mockIEnvironmentVariables. This file's test data never setsstorybook.registry, so it's harmless today, but it contradicts the contract this layer introduces —IEnvironmentVariablesexists precisely so config-resolution tests are deterministic and don't leak host/CI environment state. The hook inDirectiveBaseTestsis "Override to inject a deterministic environment for env-dependent config (e.g.storybook.registry)." Any future addition ofstorybook.registry(or similar env-dependent fields) to tests in this file would silently pick up real CI/host env vars.♻️ Suggested fix
- public IEnvironmentVariables Environment => SystemEnvironmentVariables.Instance; + public IEnvironmentVariables Environment { get; init; } = NullEnvironmentVariables.Instance;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Elastic.Documentation.Configuration.Tests/ConfigurationFileReleaseNotesTests.cs` around lines 151 - 167, MockDocumentationSetContext.Environment is using the live SystemEnvironmentVariables.Instance instead of a deterministic test stub. Update the MockDocumentationSetContext in ConfigurationFileReleaseNotesTests to inject a controlled IEnvironmentVariables implementation (as intended by DirectiveBaseTests) so env-dependent config like storybook.registry cannot leak host/CI state into these tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@tests/Elastic.Documentation.Configuration.Tests/ConfigurationFileReleaseNotesTests.cs`:
- Around line 151-167: MockDocumentationSetContext.Environment is using the live
SystemEnvironmentVariables.Instance instead of a deterministic test stub. Update
the MockDocumentationSetContext in ConfigurationFileReleaseNotesTests to inject
a controlled IEnvironmentVariables implementation (as intended by
DirectiveBaseTests) so env-dependent config like storybook.registry cannot leak
host/CI state into these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6352e1a3-ac55-4028-8b4c-7fe494db31db
📒 Files selected for processing (16)
docs/syntax/storybook.mdsrc/Elastic.Documentation.Configuration/BuildContext.cssrc/Elastic.Documentation.Configuration/Builder/ConfigurationFile.cssrc/Elastic.Documentation.Configuration/EnvironmentInterpolation.cssrc/Elastic.Documentation/IDocumentationContext.cssrc/Elastic.Markdown/Myst/Directives/Storybook/StorybookBlock.cstests/Elastic.Documentation.Configuration.Tests/ApiConfigurationTests.cstests/Elastic.Documentation.Configuration.Tests/ConfigurationFileExcludeTests.cstests/Elastic.Documentation.Configuration.Tests/ConfigurationFileReleaseNotesTests.cstests/Elastic.Documentation.Configuration.Tests/ConfigurationFileStorybookRegistryTests.cstests/Elastic.Documentation.Configuration.Tests/CrossLinkRegistryTests.cstests/Elastic.Documentation.Configuration.Tests/EnvironmentInterpolationTests.cstests/Elastic.Markdown.Tests/Directives/DirectiveBaseTests.cstests/Elastic.Markdown.Tests/Directives/StorybookTests.cstests/Navigation.Tests/TestDocumentationSetContext.cstests/authoring/Framework/CrossLinkResolverAssertions.fs
💤 Files with no reviewable changes (2)
- tests/authoring/Framework/CrossLinkResolverAssertions.fs
- tests/Navigation.Tests/TestDocumentationSetContext.cs
✅ Files skipped from review due to trivial changes (1)
- tests/Elastic.Documentation.Configuration.Tests/ConfigurationFileExcludeTests.cs
🚧 Files skipped from review as they are similar to previous changes (12)
- src/Elastic.Documentation/IDocumentationContext.cs
- tests/Elastic.Documentation.Configuration.Tests/CrossLinkRegistryTests.cs
- tests/Elastic.Documentation.Configuration.Tests/ApiConfigurationTests.cs
- tests/Elastic.Markdown.Tests/Directives/DirectiveBaseTests.cs
- src/Elastic.Documentation.Configuration/Builder/ConfigurationFile.cs
- tests/Elastic.Documentation.Configuration.Tests/EnvironmentInterpolationTests.cs
- docs/syntax/storybook.md
- src/Elastic.Documentation.Configuration/BuildContext.cs
- tests/Elastic.Documentation.Configuration.Tests/ConfigurationFileStorybookRegistryTests.cs
- tests/Elastic.Markdown.Tests/Directives/StorybookTests.cs
- src/Elastic.Markdown/Myst/Directives/Storybook/StorybookBlock.cs
- src/Elastic.Documentation.Configuration/EnvironmentInterpolation.cs
docset.yml is committed and static, but storybook.registry is environment-dependent (local, per-PR preview, main). Resolving it from an allow-listed environment variable lets one committed value serve every environment without per-env edits. The allow-list is required because docs-builder renders untrusted PR branches, so unrestricted env access would be a secret-exfiltration vector. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The DisallowedVariable_IsLeftLiteral_AndWarns test named the warning path but never verified it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
clintandrewhall
force-pushed
the
storybook-registry-env-interpolation
branch
from
3018204 to
06afc80
Compare
clintandrewhall
temporarily deployed
to
integration-tests
GitHub Actions
Inactive
2 participants
Closes #3485
Why
docset.ymlis a committed, static artifact, butstorybook.registryis inherently environment-dependent: local serve, per-PR preview, andmain/production each need a different URL that varies only in one ref segment. Today contributors hand-editdocset.ymlper environment and revert before merge, which is error-prone. The actor that knows the ref at render time is the build/preview layer, not the author at commit time.What
storybook.registrynow supports shell-style environment interpolation with a committed default, so one committed value works everywhere:${VAR}/${VAR:-default}resolve with familiar shell semantics. With no env var set (e.g. amainbuild) the committed default is used byte-for-byte.KIBANA_STORYBOOK_REGISTRY). Any other name is never read from the process environment — it is left literal and a warning is emitted — so a maliciousdocset.ymlcannot exfiltrate CI secrets such as${AWS_SECRET_ACCESS_KEY}.The substitution primitive lives in
Elastic.Documentation.Configurationand is repo-agnostic: docs-builder does not know how to construct a Kibanapr-<N>URL; the consumer supplies the variable.IEnvironmentVariablesis threaded throughIDocumentationSetContextso configuration resolution is injectable and the set/unset/fallback cases are deterministic in tests.