feat: implement value resolution env spec - #2294
Merged
Merged
Conversation
📝 WalkthroughWalkthroughImplements spec 006 (Value Resolution Env) end-to-end. Adds ChangesSpec 006: Env Value Resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
conformance/spec006_env/value_resolution_env_test.go (1)
12-233: Usestretchr/testifyassertions or document an approved exception for conformance harness pattern.This test suite uses a custom harness pattern (
harness.NewRunner,result.ExpectExitCode(), etc.) instead ofstretchr/testifyassertions. While this pattern is systematic across the conformance test suite (spec002, spec004, spec005, spec006), it violates the project guideline:**/*_test.go: Use stretchr/testify assertions for Go tests. Most other test files in the project (internal/launcher, internal/upgrade, internal/workspace, etc.) follow the testify standard. Either migrate the assertions to testify (e.g.,require.Equal(),assert.Contains()) or clarify whether the conformance harness is an approved exception to be documented in the guidelines.🤖 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 `@conformance/spec006_env/value_resolution_env_test.go` around lines 12 - 233, The TestValidate and TestRuntime test functions use a custom harness pattern (harness.NewRunner, result.ExpectExitCode, result.ExpectStdout, result.ExpectStderr, result.ExpectStderrContains, dagu.ExpectNoFile, dagu.ExpectFileContent) instead of stretchr/testify assertions as required by project guidelines. Either migrate all these custom assertion calls to use testify's require or assert functions (e.g., require.Equal, assert.Contains) throughout both test functions, or update the project guidelines documentation to explicitly approve the conformance harness pattern as an exception to the testify requirement.Source: Coding guidelines
internal/cmn/value/pipeline_test.go (1)
595-599: ⚡ Quick winRename the test case to reflect preserved behavior.
POSIXDefaultExpandednow asserts the opposite behavior ("${UNDEFINED:-default}"is preserved). Renaming (e.g.,POSIXDefaultPreserved) will prevent confusion.🤖 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 `@internal/cmn/value/pipeline_test.go` around lines 595 - 599, The test case name `POSIXDefaultExpanded` is misleading because it asserts that the POSIX default expansion syntax is preserved (not expanded) when the variable is undefined. Rename this test case to `POSIXDefaultPreserved` or similar to accurately reflect the preserved behavior being tested, where the input `"${UNDEFINED:-default}"` remains unchanged in the output.
🤖 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 `@internal/cmn/value/expand.go`:
- Around line 146-149: The documentation comment for the expandWithShellContext
function is outdated and does not reflect the current behavior. The current code
in lines 146-149 preserves undefined variables regardless of the ExpandOS flag,
but the function's doc block still describes ExpandOS=true as applying POSIX
undefined handling. Update the doc comment block for expandWithShellContext to
accurately describe that undefined variables are now preserved for their owning
runtime regardless of the ExpandOS setting value.
---
Nitpick comments:
In `@conformance/spec006_env/value_resolution_env_test.go`:
- Around line 12-233: The TestValidate and TestRuntime test functions use a
custom harness pattern (harness.NewRunner, result.ExpectExitCode,
result.ExpectStdout, result.ExpectStderr, result.ExpectStderrContains,
dagu.ExpectNoFile, dagu.ExpectFileContent) instead of stretchr/testify
assertions as required by project guidelines. Either migrate all these custom
assertion calls to use testify's require or assert functions (e.g.,
require.Equal, assert.Contains) throughout both test functions, or update the
project guidelines documentation to explicitly approve the conformance harness
pattern as an exception to the testify requirement.
In `@internal/cmn/value/pipeline_test.go`:
- Around line 595-599: The test case name `POSIXDefaultExpanded` is misleading
because it asserts that the POSIX default expansion syntax is preserved (not
expanded) when the variable is undefined. Rename this test case to
`POSIXDefaultPreserved` or similar to accurately reflect the preserved behavior
being tested, where the input `"${UNDEFINED:-default}"` remains unchanged in the
output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e029e5d-8850-4509-be5c-765db946a0b1
📒 Files selected for processing (36)
conformance/spec006_env/testdata/braced_non_env_text.yamlconformance/spec006_env/testdata/container_env_ordering.yamlconformance/spec006_env/testdata/direct_execution_env_expansion.yamlconformance/spec006_env/testdata/env_forms_order.yamlconformance/spec006_env/testdata/env_top_to_bottom.yamlconformance/spec006_env/testdata/invalid_env_declaration_name.yamlconformance/spec006_env/testdata/invalid_env_declaration_shape.yamlconformance/spec006_env/testdata/invalid_env_list_entry.yamlconformance/spec006_env/testdata/missing_env_references.yamlconformance/spec006_env/testdata/root_env_sources.yamlconformance/spec006_env/testdata/shell_run_boundary.yamlconformance/spec006_env/testdata/shell_style_env_expressions.yamlconformance/spec006_env/testdata/single_quoted_env_references.yamlconformance/spec006_env/testdata/step_env_sources.yamlconformance/spec006_env/testdata/validate_missing_runtime_sources.yamlconformance/spec006_env/value_resolution_env_test.gointernal/cmn/schema/dag.schema.jsoninternal/cmn/value/envscope.gointernal/cmn/value/expand.gointernal/cmn/value/expand_test.gointernal/cmn/value/names.gointernal/cmn/value/notices.gointernal/cmn/value/pipeline_test.gointernal/cmn/value/template.gointernal/core/spec/dag.gointernal/core/spec/loader.gointernal/core/spec/manifest_decoder.gointernal/core/spec/step.gointernal/core/spec/types/env.gointernal/core/spec/types/env_test.gointernal/core/spec/value_reference_notices_test.gointernal/core/spec/variables.gointernal/core/value_notices.gointernal/runtime/builtin/docker/eval_test.gospecs/006-value-resolution-env.mdspecs/README.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/cmn/value/template.go">
<violation number="1" location="internal/cmn/value/template.go:130">
P2: Braced numeric vars can be skipped incorrectly when followed by identifier chars. `${1}a` should expand `${1}` then append `a`, but current boundary check preserves `${1}` unchanged.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Summary by cubic
Implements Spec 006 env value resolution with ordered, top-to-bottom evaluation, strict env name validation, and shell-style expansion that preserves undefined references. Also fixes Windows env precedence by treating env names case-insensitively during merges, and stabilizes tests by improving
store.MarkDispatchIndexReconcileDueForTestbehavior.New Features
enventries in YAML source order for DAG, steps, and containers; preserve map-form order viagithub.com/goccy/go-yaml/parser.${env.NAME},${NAME},$NAME, braced numeric${1}, and POSIX${VAR:-...}; resolve when defined and keep undefined values literal; single-quoted and unsupported text like$1astay literal.envcan read consts, runtime params, root env, and predecessor outputs; emit notices with exact field paths (including simple$VARinrunandcontainer.env).Migration
^[A-Za-z_][A-Za-z0-9_]*$; invalid names and malformedKEY=valueentries now fail.envevaluation should be reviewed for the new top-to-bottom behavior.make conformance CONFORMANCE_TEST_TARGET=./conformance/spec006_env.Written for commit e1e3caf. Summary will update on new commits.
Summary by CodeRabbit
New Features
${env.NAME},$NAME,${NAME}, and shell-style operatorsTests
Documentation