validate: separate spec defects from values a run supplies - #2450
Merged
Conversation
`dagu validate` reported every preserved value reference at the same level,
so `${context.paths.artifacts_dir}` and `${env.API_TOKEN}` looked exactly
like `${steps.build.outputs.undeclared}`. The first two resolve on every
run; only the third can never resolve. Readers acted on the noise: a
correct workflow was deleted because validation appeared to condemn it.
Notices now carry a class. A reason that names a missing step, output,
context field, or const is a defect and is reported by default, now at
warning level. A reason that only says the evaluating scope held no value
is runtime-only and stays out of the default output; --show-unresolved
prints it. The classification needed no change to how references resolve,
because `namespace_unavailable` already meant "well formed, absent here"
while an undefined name already produced `unknown_context_field`.
Two reasons were unclassifiable because they carried no code at all.
`env` and `consts` lookups returned bare errors, so their notices arrived
with an empty reason. They now report `unknown_env_binding` and
`unknown_const_name`. The split between them is load-bearing: a const is
declared in the spec, so an unknown one is a defect, whereas a param or an
environment variable can be supplied at start time and is not.
Also fixes an unrelated false positive found while classifying: a step's
`with:` was evaluated against the DAG env scope, so a reference to the
step's own `env:` was reported as unresolved. The step scope was already
built and then discarded; it is now threaded through the field walk.
📝 WalkthroughWalkthroughUnresolved value-reference notices now carry defect or runtime-only classification. Validation hides runtime-only notices unless ChangesValue reference notice classification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ValidateCommand
participant ReportValueReferenceNotices
participant NoticeLogger
ValidateCommand->>ReportValueReferenceNotices: collect classified notices
ValidateCommand->>NoticeLogger: pass show-unresolved
NoticeLogger->>ValidateCommand: emit defects and selected runtime-only notices
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🤖 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 `@specs/003-value-resolution.md`:
- Around line 364-371: Update the validation notice rules in
specs/003-value-resolution.md to resolve the contradiction: clarify that default
validation suppresses only runtime-only notices, while unresolved-reference
defects are reported by default, including their warning-level classification.
Preserve --show-unresolved reporting both notice types and the requirement that
rendered notices distinguish them.
In
`@ui/src/features/dags/components/value-reference-notices/ValueReferenceNoticesButton.tsx`:
- Around line 18-34: Update REASON_LABELS to include a readable label for
unknown_const_name, and add unknown_const_name to DEFECT_REASONS so fallback
classification matches the backend’s defect class. Keep the existing reasonLabel
and NoticeCard behavior unchanged.
🪄 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 Plus
Run ID: e5b4795b-18e4-44c8-a971-faf890f7abac
📒 Files selected for processing (18)
api/v1/api.gen.goapi/v1/api.yamlconformance/spec007_value_resolution_steps/value_resolution_steps_test.goconformance/spec017_built_in_run_context/built_in_run_context_test.gointernal/cmd/flags.gointernal/cmd/validate.gointernal/cmd/validate_test.gointernal/cmn/value/notices.gointernal/cmn/value/notices_test.gointernal/cmn/value/template.gointernal/core/value_fields.gointernal/core/value_notices.gointernal/service/frontend/api/v1/dags.gospecs/003-value-resolution.mdspecs/006-value-resolution-env.mdspecs/017-built-in-run-context.mdui/src/api/v1/schema.tsui/src/features/dags/components/value-reference-notices/ValueReferenceNoticesButton.tsx
Comment on lines
+364
to
+371
| Each notice must carry a class. | ||
| A notice is a defect when the reference names a step, output, context field, or | ||
| const the spec does not define, because no run can resolve it. | ||
| A notice is runtime-only when the reference is well formed and the inspecting | ||
| scope simply holds no value for it. | ||
| `dagu validate` must report defects by default and must keep runtime-only | ||
| notices out of its default output; `--show-unresolved` reports both. | ||
| Inspection surfaces that render notices must let a reader tell the two apart. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve the default-warning contradiction.
The preceding rule says passive notices must not be shown as normal validation warnings, while this rule requires defects in default validation output and internal/cmd/validate.go logs them at warning level. Clarify that only runtime-only notices are hidden by default.
🧰 Tools
🪛 LanguageTool
[grammar] ~367-~367: Use a hyphen to join words.
Context: ... runtime-only when the reference is well formed and the inspecting scope simply h...
(QB_NEW_EN_HYPHEN)
[grammar] ~371-~371: Ensure spelling is correct
Context: ...ow-unresolved` reports both. Inspection surfaces that render notices must let a reader t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@specs/003-value-resolution.md` around lines 364 - 371, Update the validation
notice rules in specs/003-value-resolution.md to resolve the contradiction:
clarify that default validation suppresses only runtime-only notices, while
unresolved-reference defects are reported by default, including their
warning-level classification. Preserve --show-unresolved reporting both notice
types and the requirement that rendered notices distinguish them.
Comment on lines
+18
to
+34
| const REASON_LABELS: Record<string, string> = { | ||
| unknown_step_id: 'Step id does not exist', | ||
| unknown_output_name: 'Output name is not declared', | ||
| missing_dependency: 'Producing step is not a dependency', | ||
| self_reference: 'Step references its own output', | ||
| unknown_context_field: 'Context field is not defined', | ||
| namespace_unavailable: 'Value is supplied by a run', | ||
| unknown_env_binding: 'Environment variable is supplied by a run', | ||
| }; | ||
|
|
||
| const DEFECT_REASONS = new Set([ | ||
| 'unknown_step_id', | ||
| 'unknown_output_name', | ||
| 'missing_dependency', | ||
| 'self_reference', | ||
| 'unknown_context_field', | ||
| ]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Missing unknown_const_name entries in REASON_LABELS and DEFECT_REASONS.
This PR introduces the unknown_const_name reason (classified as defect by the backend's Class()), but neither REASON_LABELS nor the fallback DEFECT_REASONS set was updated for it. NoticeCard calls reasonLabel(notice.reason) unconditionally, so any unknown_const_name notice will display the raw enum string instead of a readable label, and the older-server fallback classification (isDefect) would also misclassify it as runtime-only if notice.class is ever absent.
🐛 Proposed fix
const REASON_LABELS: Record<string, string> = {
unknown_step_id: 'Step id does not exist',
unknown_output_name: 'Output name is not declared',
missing_dependency: 'Producing step is not a dependency',
self_reference: 'Step references its own output',
unknown_context_field: 'Context field is not defined',
+ unknown_const_name: 'Const name is not declared',
namespace_unavailable: 'Value is supplied by a run',
unknown_env_binding: 'Environment variable is supplied by a run',
};
const DEFECT_REASONS = new Set([
'unknown_step_id',
'unknown_output_name',
'missing_dependency',
'self_reference',
'unknown_context_field',
+ 'unknown_const_name',
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const REASON_LABELS: Record<string, string> = { | |
| unknown_step_id: 'Step id does not exist', | |
| unknown_output_name: 'Output name is not declared', | |
| missing_dependency: 'Producing step is not a dependency', | |
| self_reference: 'Step references its own output', | |
| unknown_context_field: 'Context field is not defined', | |
| namespace_unavailable: 'Value is supplied by a run', | |
| unknown_env_binding: 'Environment variable is supplied by a run', | |
| }; | |
| const DEFECT_REASONS = new Set([ | |
| 'unknown_step_id', | |
| 'unknown_output_name', | |
| 'missing_dependency', | |
| 'self_reference', | |
| 'unknown_context_field', | |
| ]); | |
| const REASON_LABELS: Record<string, string> = { | |
| unknown_step_id: 'Step id does not exist', | |
| unknown_output_name: 'Output name is not declared', | |
| missing_dependency: 'Producing step is not a dependency', | |
| self_reference: 'Step references its own output', | |
| unknown_context_field: 'Context field is not defined', | |
| unknown_const_name: 'Const name is not declared', | |
| namespace_unavailable: 'Value is supplied by a run', | |
| unknown_env_binding: 'Environment variable is supplied by a run', | |
| }; | |
| const DEFECT_REASONS = new Set([ | |
| 'unknown_step_id', | |
| 'unknown_output_name', | |
| 'missing_dependency', | |
| 'self_reference', | |
| 'unknown_context_field', | |
| 'unknown_const_name', | |
| ]); |
🤖 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
`@ui/src/features/dags/components/value-reference-notices/ValueReferenceNoticesButton.tsx`
around lines 18 - 34, Update REASON_LABELS to include a readable label for
unknown_const_name, and add unknown_const_name to DEFECT_REASONS so fallback
classification matches the backend’s defect class. Keep the existing reasonLabel
and NoticeCard behavior unchanged.
The notice loops in the consts, params, env, and step-reference specs assert that validation prints references the run supplies: params a caller passes at start time, environment variables the operator sets, and step outputs read from a handler. Those are runtime-only, so they now need --show-unresolved. The defect assertions in the same packages were left alone and still pass against the default output, which is the split this change is for.
The UI kept two tables mirroring the server's reason set and neither gained unknown_const_name, so such a notice rendered its raw enum string and, on a response predating the class field, fell back to runtime-only. Both tables now carry it, and a test walks the generated reason enum so the next reason added to the API fails here instead of degrading quietly. The spec said a notice must not be shown as a normal validation warning, then the new text required defects in the default output, which validate logs at warning level. The rule now applies to runtime-only notices, which is what it was protecting, and states that neither class changes the exit code.
Treat step-output references in fields without lookup scope as defects while keeping runtime context misses informational. Propagate step env scopes through foreach bodies and preserve the computed notice class in API responses.
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
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.
Problem
dagu validatereports every preserved value reference at the same level, so these three are indistinguishable in its output:${steps.build.outputs.undeclared}${context.paths.artifacts_dir}${env.API_TOKEN}Only the first is a defect. The other two are reported because validation evaluates a spec outside a run, where a synthetic scope holds almost nothing:
noticeBuiltinContextmodels 3 of the 22 supportedcontext.*bindings, and the env scope is deliberately built withincludeOS=falseso validation does not depend on the shell it runs in.The noise is not harmless. A correct workflow was deleted on the strength of a
reason=namespace_unavailableline that was a false alarm. Dagu's own bundled examples trip it too, sincedagu example 12uses${context.paths.artifacts_dir}.The true positives are worth keeping. The undeclared-output check is what caught a wrong cursor example in the docs (dagucloud/docs#24).
Approach
Classify instead of suppress.
Resolution semantics are unchanged. They already carried the needed distinction:
namespace_unavailablemeans "well formed, absent from this scope", while a name the spec does not define already producedunknown_context_field. So no resolver behaviour moves, and the tests that pin it (builtin_context_test.go,value_notices_test.go) needed no edits.unknown_step_id,unknown_output_name,missing_dependency,self_reference,unknown_context_field,unknown_const_name. Reported by default, now at warning level.namespace_unavailable,unknown_env_binding. Kept out of the default output;--show-unresolvedprints them.noticeBuiltinContextis deliberately not extended to cover all 22 keys. That would silence the symptom while duplicating the runtime list ininternal/runtime/eval.go, so every new context key would reintroduce the bug.Two reasons that had no code
bindingEnvValueand theconstsbranch ofbindingMapValuereturned barefmt.Errorf, so their notices arrived with an empty reason and could not be classified. They now reportunknown_env_bindingandunknown_const_name.The split matters: a const is declared in the spec, so an unknown one can never resolve and is a defect. A param is not, and neither is an environment variable. Verified rather than assumed:
so
paramsstays runtime-only.Unrelated false positive fixed alongside
A step's
with:was resolved against the DAG-level env scope, so referencing the step's ownenv:was reported as unresolved.reportSingleStepEnvValueReferenceNoticesalready built the step scope and threw it away; it is now returned and threaded into the field walk via a newReferenceField.OwnerStepPath.Verification
Green:
make lint(0 issues, both GOOS), andinternal/cmn/value,internal/core,internal/core/spec,internal/cmd,internal/service/frontend/api/v1, plus thespec007andspec017conformance packages.Note the defect subtests in
spec007passed untouched; only the twonamespace_unavailablecases needed--show-unresolved, which is a good sign the split lands where intended.Notes for review
api/v1/api.gen.gois hand-edited (13 lines) rather than regenerated.go.modpins oapi-codegen v2.7.1 but the checked-in file was produced by v2.5.1, so a real regeneration produces a 12,298-line diff unrelated to this change. That drift is pre-existing and worth a separate PR.make apicannot run on main at all:api-validatefails withschema "DAG": extra sibling fields: [description]. Confirmed present before this change.classis an optional API field and the UI falls back to deriving it fromreason, so an older server keeps working.Summary by cubic
Make
dagu validateseparate real spec defects from values that only exist during a run. Adds context-aware classification, warns on defects by default, and hides runtime-only references unless you pass--show-unresolved.New Features
defectvsruntime_only, with context-aware handling (step-output refs in fields without lookup scope are defects; runtime context misses stay runtime-only).--show-unresolvedto print runtime-only notices; defects log at warn, runtime-only at info.ValueReferenceNotice.classand reasonsunknown_env_bindingandunknown_const_name; responses include the computed class. UI groups notices into “Needs a fix” vs “Resolved during a run,” adds a label forunknown_const_name, and classifies old responses correctly (treats${steps.*}namespace_unavailableas defects), with tests.Bug Fixes
env:(includingforeachbodies and handlers), removing false positives.--show-unresolvedoutput.Written for commit a983067. Summary will update on new commits.
Summary by CodeRabbit
--show-unresolvedto optionally display references resolved only during workflow execution.