feat: surface passive value reference notices - #2290
Merged
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (74)
💤 Files with no reviewable changes (9)
📝 WalkthroughWalkthroughReplaces value-resolution validation warnings with a structured passive-notice mechanism ( ChangesPassive Notices for Value Resolution
Sequence Diagram(s)sequenceDiagram
participant Client
participant GetDAGSpec
participant LoadYAMLWithResult
participant ValueReferenceNoticeCollector
participant ReportValueReferenceNotices
Client->>GetDAGSpec: GET /dags/{fileName}/spec
GetDAGSpec->>LoadYAMLWithResult: data, opts (WithoutEval, workspace)
LoadYAMLWithResult->>ValueReferenceNoticeCollector: create collector
LoadYAMLWithResult->>LoadYAMLWithResult: attach collector to BuildContext
LoadYAMLWithResult->>ReportValueReferenceNotices: resolve ReferenceFields, Report unresolved
ReportValueReferenceNotices->>ValueReferenceNoticeCollector: Report(ValueReferenceNotice)
LoadYAMLWithResult-->>GetDAGSpec: LoadResult{DAG, ValueReferenceNotices}
GetDAGSpec-->>Client: 200 {spec, errors, valueReferenceNotices:[{message, fieldPath, token}]}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1/api.yaml (1)
12963-12992:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
diagnosticsto theDAGRunDetailsrequired list.The property is declared but omitted from
required, so generated clients will treat run diagnostics as optional while the API is meant to return a stable array contract. Represent unavailable diagnostics as[]and require the field here. Based on PR context, DAG-run details expose transient diagnostics through the API response.🔧 Proposed OpenAPI contract fix
required: - rootDAGRunName - rootDAGRunId - log - nodes + - diagnostics🤖 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 `@api/v1/api.yaml` around lines 12963 - 12992, The diagnostics property is declared in the DAGRunDetails schema but is missing from the required fields list. Add diagnostics to the required array in the DAGRunDetails schema alongside the existing required properties rootDAGRunName, rootDAGRunId, log, and nodes to ensure the API contract reflects that diagnostics will always be returned as a stable array (even if empty when unavailable).
🧹 Nitpick comments (2)
internal/diagnostic/stream.go (1)
52-61: 💤 Low valueWrite errors are silently ignored (fire-and-forget diagnostic semantics).
The implementation ignores both JSON marshaling errors (line 55) and write errors (line 60). For the Diagnostic struct with primitive fields, marshaling should never fail. Write failures will silently drop diagnostics, which is acceptable for best-effort diagnostic reporting but means diagnostics can be lost if the writer encounters I/O errors.
This is a reasonable design choice for non-critical diagnostic output.
🤖 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/diagnostic/stream.go` around lines 52 - 61, The Report method in the streamSink struct intentionally ignores both JSON marshaling errors and write errors to implement fire-and-forget diagnostic semantics. Add clear comments above the error handling sections (where err is checked after json.Marshal and where fmt.Fprintf is called) to explicitly document that this silent dropping of errors is intentional and acceptable for best-effort diagnostic reporting. Document that marshaling errors should be rare with the Diagnostic struct's primitive fields, and that write failures are acceptable trade-offs for non-critical diagnostic output.ui/src/features/dags/components/dag-details/DAGDetailsContent.tsx (1)
152-154: ⚡ Quick winNote the significant breakpoint change from
lgto2xl.Changing the responsive breakpoint from
lg(1024px) to2xl(1536px) means the desktop tab layout will only appear on larger screens. Users on tablets and smaller laptops (1024px–1535px) will now see the compact icon-only tab layout instead of the full labeled tabs. Ensure this aligns with the intended design and has been tested across target screen sizes.Also applies to: 296-296
🤖 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/dag-details/DAGDetailsContent.tsx` around lines 152 - 154, Verify the responsive breakpoint change in the DAGDetailsContent component where the desktop tab layout was changed from `lg` (1024px) to `2xl` (1536px). If this change was unintentional, revert the `2xl:block` and `2xl:flex-row` classes back to `lg:block` and `lg:flex-row` respectively to restore the original responsive behavior for tablets and smaller laptops. If the change was intentional, ensure it aligns with the design specification and test the layout across all target screen sizes including 1024px–1535px range. Note that this change also applies to the related classes at line 296, so update both locations consistently.
🤖 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.
Outside diff comments:
In `@api/v1/api.yaml`:
- Around line 12963-12992: The diagnostics property is declared in the
DAGRunDetails schema but is missing from the required fields list. Add
diagnostics to the required array in the DAGRunDetails schema alongside the
existing required properties rootDAGRunName, rootDAGRunId, log, and nodes to
ensure the API contract reflects that diagnostics will always be returned as a
stable array (even if empty when unavailable).
---
Nitpick comments:
In `@internal/diagnostic/stream.go`:
- Around line 52-61: The Report method in the streamSink struct intentionally
ignores both JSON marshaling errors and write errors to implement
fire-and-forget diagnostic semantics. Add clear comments above the error
handling sections (where err is checked after json.Marshal and where fmt.Fprintf
is called) to explicitly document that this silent dropping of errors is
intentional and acceptable for best-effort diagnostic reporting. Document that
marshaling errors should be rare with the Diagnostic struct's primitive fields,
and that write failures are acceptable trade-offs for non-critical diagnostic
output.
In `@ui/src/features/dags/components/dag-details/DAGDetailsContent.tsx`:
- Around line 152-154: Verify the responsive breakpoint change in the
DAGDetailsContent component where the desktop tab layout was changed from `lg`
(1024px) to `2xl` (1536px). If this change was unintentional, revert the
`2xl:block` and `2xl:flex-row` classes back to `lg:block` and `lg:flex-row`
respectively to restore the original responsive behavior for tablets and smaller
laptops. If the change was intentional, ensure it aligns with the design
specification and test the layout across all target screen sizes including
1024px–1535px range. Note that this change also applies to the related classes
at line 296, so update both locations consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa072b75-af6f-40aa-a080-175daed24be3
📒 Files selected for processing (90)
Makefileapi/v1/api.gen.goapi/v1/api.yamlconformance/harness/runner.goconformance/spec004_consts/value_resolution_consts_test.goconformance/spec005_params/testdata/env_list_forms.yamlconformance/spec005_params/testdata/handler_run_array.yamlconformance/spec005_params/testdata/handler_stderr_artifact.yamlconformance/spec005_params/testdata/handler_stdout_artifact.yamlconformance/spec005_params/testdata/parallel_items_params.yamlconformance/spec005_params/testdata/parallel_items_value.yamlconformance/spec005_params/testdata/parallel_variable.yamlconformance/spec005_params/testdata/params_default_literal.yamlconformance/spec005_params/testdata/params_reference_in_consts_runtime.yamlconformance/spec005_params/testdata/root_shell_string.yamlconformance/spec005_params/testdata/root_working_dir.yamlconformance/spec005_params/testdata/step_output_nested.yamlconformance/spec005_params/testdata/step_repeat_policy_condition.yamlconformance/spec005_params/testdata/step_run_array.yamlconformance/spec005_params/testdata/step_stderr_artifact.yamlconformance/spec005_params/testdata/step_stdout_artifact.yamlconformance/spec005_params/testdata/step_stdout_outputs.yamlconformance/spec005_params/testdata/step_with_nested.yamlconformance/spec005_params/value_resolution_params_test.gointernal/agent/dag_manage_test.gointernal/cmd/context.gointernal/cmd/dry.gointernal/cmd/migrator_test.gointernal/cmd/restart.gointernal/cmd/retry.gointernal/cmd/start.gointernal/cmn/telemetry/collector_test.gointernal/cmn/value/diagnostics.gointernal/cmn/value/references.gointernal/cmn/value/resolver.gointernal/cmn/value/resolver_test.gointernal/cmn/value/template.gointernal/core/dag.gointernal/core/exec/dag.gointernal/core/output_reference_validation.gointernal/core/output_reference_validation_test.gointernal/core/spec/builder.gointernal/core/spec/consts.gointernal/core/spec/consts_test.gointernal/core/spec/diagnostics_test.gointernal/core/spec/loader.gointernal/core/spec/output_reference_validation_test.gointernal/core/spec/value_reference_fields_test.gointernal/core/spec/variables.gointernal/core/validator.gointernal/core/value_reference_validation_test.gointernal/diagnostic/collector.gointernal/diagnostic/collector_test.gointernal/diagnostic/context.gointernal/diagnostic/context_test.gointernal/diagnostic/diagnostic.gointernal/diagnostic/run_store.gointernal/diagnostic/stream.gointernal/launcher/diagnostic_stream.gointernal/launcher/launcher.gointernal/launcher/launcher_test.gointernal/persis/file/dag/store.gointernal/runtime/agent/agent.gointernal/runtime/agent/agent_test.gointernal/runtime/agent/dbclient_test.gointernal/runtime/env.gointernal/runtime/node.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/dagruns.gointernal/service/frontend/api/v1/dagruns_edit_retry.gointernal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/dags_internal_test.gointernal/service/frontend/api/v1/diagnostics.gointernal/service/notification/service_test.gospecs/003-value-resolution.mdspecs/004-value-resolution-consts.mdspecs/005-value-resolution-params.mdspecs/006-value-resolution-env.mdspecs/007-value-resolution-steps.mdspecs/009-step-reference.mdspecs/010-field-evaluation.mdspecs/011-dynamic-evaluation.mdspecs/013-step-run.mdspecs/README.mdui/src/api/v1/schema.tsui/src/features/dags/components/DAGStatus.tsxui/src/features/dags/components/dag-details/DAGDetailsContent.tsxui/src/features/dags/components/dag-editor/DAGSpec.tsxui/src/features/dags/components/diagnostics/DiagnosticsButton.tsxui/src/features/dags/components/diagnostics/index.ts
💤 Files with no reviewable changes (6)
- internal/core/dag.go
- internal/core/output_reference_validation.go
- internal/core/validator.go
- internal/core/spec/output_reference_validation_test.go
- internal/core/output_reference_validation_test.go
- internal/cmn/value/references.go
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
yohamta0
changed the title
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/cmn/value/template.go (1)
34-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve the original unresolved token text instead of rebuilding it from the path.
Line 37 rebuilds the token as
"${" + path + "}", which can normalize whitespace and change the original literal text. Use the matched token fromwalkBindingsso unresolved references are truly left unchanged.Proposed fix
-resolved, err := walkBindings(input, func(_ string, path string) (string, error) { +resolved, err := walkBindings(input, func(token string, path string) (string, error) { value, err := bindingValue(ctx, path, scope, true) if err != nil { - token := "${" + path + "}" addUnresolvedReferenceNotice(notices, field, token, err) placeholder := uniqueToken(seed, "__DAGU_UNRESOLVED_REF__") seed += placeholder protected[placeholder] = token return placeholder, nil }🤖 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/template.go` around lines 34 - 42, The code rebuilds the unresolved token as "${" + path + "}" on line 37, which can normalize whitespace and alter the original literal text. Modify the walkBindings callback to receive the original matched token text (not just the path) as a parameter, then use that original token value instead of reconstructing it when calling addUnresolvedReferenceNotice and when storing in the protected map. This ensures unresolved references preserve their exact original formatting without any normalization.ui/src/features/dags/components/dag-editor/DAGSpec.tsx (1)
153-161:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep existing notices in cache during SSE sync instead of clearing them.
Line 159 hard-resets
valueReferenceNoticesto[], which makes the notices UI disappear until the follow-up revalidation completes.💡 Proposed fix
useSSECacheSync(dagSSE, mutateSpec, (next) => next.spec === undefined ? undefined : { dag: next.dag, errors: next.errors ?? [], - valueReferenceNotices: [], + valueReferenceNotices: data?.valueReferenceNotices ?? [], spec: next.spec, } );As per coding guidelines, “NEVER use full-page loading overlays or LoadingIndicator components that hide content; instead show stale data while fetching updates.”
🤖 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/dag-editor/DAGSpec.tsx` around lines 153 - 161, The valueReferenceNotices property in the useSSECacheSync callback is being hard-reset to an empty array, which clears the UI notices prematurely during SSE updates. Instead of setting valueReferenceNotices to an empty array in the cache update object, preserve the existing notices from the previous cache state so users continue to see stale data while the SSE update completes. You can access the current cache value to retrieve the previous valueReferenceNotices and maintain them during the sync operation.Source: Coding guidelines
🧹 Nitpick comments (2)
internal/service/frontend/api/v1/dags_internal_test.go (1)
831-837: ⚡ Quick winPrefer asserting stable notice contract fields over human-readable message text.
require.Contains(notice.Message, "was left unchanged")is fragile to wording changes. AssertKind/Code(and keep FieldPath/Token) so this test tracks API contract, not copy text.Suggested test hardening
notice := specResp.ValueReferenceNotices[0] + require.NotNil(t, notice.Kind) + require.Equal(t, "value_resolution", *notice.Kind) + require.NotNil(t, notice.Code) + require.Equal(t, "value_reference_unresolved", *notice.Code) require.NotNil(t, notice.FieldPath) require.Equal(t, "consts.image", *notice.FieldPath) require.NotNil(t, notice.Token) require.Equal(t, "${consts.missing}", *notice.Token) - require.Contains(t, notice.Message, "was left unchanged") + require.NotEmpty(t, notice.Message)🤖 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/service/frontend/api/v1/dags_internal_test.go` around lines 831 - 837, The test at the end of the test block is asserting on the human-readable message text of the notice object, which is fragile to wording changes. Replace the require.Contains assertion on notice.Message with assertions on the stable API contract fields like notice.Kind and notice.Code. Keep the existing assertions on notice.FieldPath and notice.Token, and verify that the Kind and Code fields match the expected contract for this type of notice rather than checking for specific message text.internal/core/spec/value_reference_notices_test.go (1)
31-34: ⚡ Quick winAssert
kindandcodeexplicitly in notice tests.Current checks rely on message text, which is more brittle than the structured contract fields this feature introduces.
Suggested assertion additions
got := result.ValueReferenceNotices[0] + assert.Equal(t, "value_resolution", got.Kind) + assert.Equal(t, "value_reference_unresolved", got.Code) assert.Equal(t, "consts.image", got.FieldPath) assert.Equal(t, "${consts.missing}", got.Token) assert.Contains(t, got.Message, "was left unchanged")Also applies to: 53-55
🤖 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/core/spec/value_reference_notices_test.go` around lines 31 - 34, The test assertions are relying on the Message field to verify behavior, which is brittle. Add explicit assertions for the kind and code fields of the got notice object alongside the existing FieldPath, Token, and Message checks. This applies to both test cases mentioned in the review (around lines 31-34 and 53-55). Assert the appropriate kind and code values that correspond to the expected notice type, making the test more robust by checking the structured contract fields instead of just the message text.
🤖 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/core/spec/variables.go`:
- Around line 117-123: The field path passed to DAGEnvField in the resolver
initialization is using "env." + p.key format, which differs from the env[i]
format used during post-load reference inspection, causing duplicate diagnostics
for unresolved tokens. Normalize the field path passed to DAGEnvField to use a
consistent format that matches what post-load reference inspection will use,
likely changing from "env." + p.key to a normalized path such as just "env" or
an array-index format to ensure both evaluation contexts report using the same
FieldPath for proper deduplication.
In `@internal/service/chatbridge/monitor_external_test.go`:
- Around line 154-156: The test is missing the assertion to verify that no reads
occurred, which is part of the test's contract as indicated by the test name.
After the require.Eventually block that waits for store.lastHead() to advance to
at least 1, add an assertion to check that readCalls equals 0 to ensure the
no-destination fast-path behavior is properly guarded and the test validates
both cursor advancement and the absence of read operations.
In `@specs/003-value-resolution.md`:
- Around line 134-136: Update the statement about inspection surfaces on line
134 to include the REST API inspection surface alongside the existing mentions
of dagu validate and the Web UI spec editor. Add explicit documentation that
notices also surface via the spec inspection API response so that all three
interfaces (CLI, Web UI, and REST API) are properly documented in the
specification.
In `@ui/src/features/dags/components/DAGStatus.tsx`:
- Around line 447-507: Add aria-label attributes to each Tab component to
provide accessible names for screen readers, since the text labels are hidden on
small screens with the hidden sm:inline class. For each Tab with activeTab
values of 'status', 'approval', 'timeline', 'outputs', 'artifacts', 'chat', and
'spec', add an aria-label prop with the corresponding label text (e.g.,
aria-label="Status", aria-label="Approval", etc.) to ensure the tabs remain
accessible to assistive technologies even when the visual labels are not
displayed.
In
`@ui/src/features/dags/components/value-reference-notices/ValueReferenceNoticesButton.tsx`:
- Line 101: The `<p>` element rendering `notice.message` lacks text wrapping
utilities, causing long strings to overflow and break the dialog layout in
narrow widths. Add the Tailwind CSS classes `whitespace-normal break-words` to
the className attribute of the `<p>` element that displays notice.message to
ensure proper text wrapping and prevent layout overflow.
---
Outside diff comments:
In `@internal/cmn/value/template.go`:
- Around line 34-42: The code rebuilds the unresolved token as "${" + path + "}"
on line 37, which can normalize whitespace and alter the original literal text.
Modify the walkBindings callback to receive the original matched token text (not
just the path) as a parameter, then use that original token value instead of
reconstructing it when calling addUnresolvedReferenceNotice and when storing in
the protected map. This ensures unresolved references preserve their exact
original formatting without any normalization.
In `@ui/src/features/dags/components/dag-editor/DAGSpec.tsx`:
- Around line 153-161: The valueReferenceNotices property in the useSSECacheSync
callback is being hard-reset to an empty array, which clears the UI notices
prematurely during SSE updates. Instead of setting valueReferenceNotices to an
empty array in the cache update object, preserve the existing notices from the
previous cache state so users continue to see stale data while the SSE update
completes. You can access the current cache value to retrieve the previous
valueReferenceNotices and maintain them during the sync operation.
---
Nitpick comments:
In `@internal/core/spec/value_reference_notices_test.go`:
- Around line 31-34: The test assertions are relying on the Message field to
verify behavior, which is brittle. Add explicit assertions for the kind and code
fields of the got notice object alongside the existing FieldPath, Token, and
Message checks. This applies to both test cases mentioned in the review (around
lines 31-34 and 53-55). Assert the appropriate kind and code values that
correspond to the expected notice type, making the test more robust by checking
the structured contract fields instead of just the message text.
In `@internal/service/frontend/api/v1/dags_internal_test.go`:
- Around line 831-837: The test at the end of the test block is asserting on the
human-readable message text of the notice object, which is fragile to wording
changes. Replace the require.Contains assertion on notice.Message with
assertions on the stable API contract fields like notice.Kind and notice.Code.
Keep the existing assertions on notice.FieldPath and notice.Token, and verify
that the Kind and Code fields match the expected contract for this type of
notice rather than checking for specific message text.
🪄 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: da5f2db3-d456-4e77-a6a3-95ab11aefebd
📒 Files selected for processing (74)
Makefileapi/v1/api.gen.goapi/v1/api.yamlconformance/harness/runner.goconformance/spec004_consts/value_resolution_consts_test.goconformance/spec005_params/testdata/env_list_forms.yamlconformance/spec005_params/testdata/handler_run_array.yamlconformance/spec005_params/testdata/handler_stderr_artifact.yamlconformance/spec005_params/testdata/handler_stdout_artifact.yamlconformance/spec005_params/testdata/parallel_items_params.yamlconformance/spec005_params/testdata/parallel_items_value.yamlconformance/spec005_params/testdata/parallel_variable.yamlconformance/spec005_params/testdata/params_default_literal.yamlconformance/spec005_params/testdata/params_reference_in_consts_runtime.yamlconformance/spec005_params/testdata/root_shell_string.yamlconformance/spec005_params/testdata/root_working_dir.yamlconformance/spec005_params/testdata/step_output_nested.yamlconformance/spec005_params/testdata/step_repeat_policy_condition.yamlconformance/spec005_params/testdata/step_run_array.yamlconformance/spec005_params/testdata/step_stderr_artifact.yamlconformance/spec005_params/testdata/step_stdout_artifact.yamlconformance/spec005_params/testdata/step_stdout_outputs.yamlconformance/spec005_params/testdata/step_with_nested.yamlconformance/spec005_params/value_resolution_params_test.gointernal/cmd/context.gointernal/cmd/validate.gointernal/cmd/validate_test.gointernal/cmn/value/notices.gointernal/cmn/value/references.gointernal/cmn/value/references_test.gointernal/cmn/value/resolver.gointernal/cmn/value/resolver_test.gointernal/cmn/value/template.gointernal/cmn/value/template_test.gointernal/core/dag.gointernal/core/output_reference_validation.gointernal/core/output_reference_validation_test.gointernal/core/spec/builder.gointernal/core/spec/consts.gointernal/core/spec/consts_test.gointernal/core/spec/dparams.gointernal/core/spec/dparams_inline.gointernal/core/spec/loader.gointernal/core/spec/output_reference_validation_test.gointernal/core/spec/value_reference_fields_test.gointernal/core/spec/value_reference_notices_test.gointernal/core/spec/variables.gointernal/core/validator.gointernal/core/value_fields.gointernal/core/value_notices.gointernal/core/value_reference_validation_test.gointernal/launcher/launcher.gointernal/persis/file/proc/store_test.gointernal/runtime/env.gointernal/runtime/node.gointernal/service/chatbridge/monitor_external_test.gointernal/service/frontend/api/v1/dags.gointernal/service/frontend/api/v1/dags_internal_test.gospecs/003-value-resolution.mdspecs/004-value-resolution-consts.mdspecs/005-value-resolution-params.mdspecs/006-value-resolution-env.mdspecs/007-value-resolution-steps.mdspecs/009-step-reference.mdspecs/010-field-evaluation.mdspecs/011-dynamic-evaluation.mdspecs/013-step-run.mdspecs/README.mdui/src/api/v1/schema.tsui/src/features/dags/components/DAGStatus.tsxui/src/features/dags/components/dag-details/DAGDetailsContent.tsxui/src/features/dags/components/dag-editor/DAGSpec.tsxui/src/features/dags/components/value-reference-notices/ValueReferenceNoticesButton.tsxui/src/features/dags/components/value-reference-notices/index.ts
💤 Files with no reviewable changes (9)
- internal/core/output_reference_validation.go
- internal/core/spec/output_reference_validation_test.go
- internal/core/dag.go
- internal/core/output_reference_validation_test.go
- internal/cmd/context.go
- internal/cmn/value/references.go
- internal/core/validator.go
- internal/core/spec/dparams_inline.go
- internal/cmn/value/references_test.go
Comment on lines
154
to
156
| require.Eventually(t, func() bool { | ||
| _, readCalls := store.stats() | ||
| return store.lastHead() >= 1 && readCalls == 0 | ||
| return store.lastHead() >= 1 | ||
| }, time.Second, 10*time.Millisecond) |
There was a problem hiding this comment.
Restore the “no reads” assertion to match the test’s contract.
At Line 155, the test now only checks cursor advancement, but the test name says without reading events. Please also assert readCalls == 0 (after waiting for head advancement) so this test still guards the no-destination fast-path behavior.
Suggested patch
require.Eventually(t, func() bool {
return store.lastHead() >= 1
}, time.Second, 10*time.Millisecond)
+
+_, readCalls := store.stats()
+require.Equal(t, 0, readCalls)📝 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
| require.Eventually(t, func() bool { | |
| _, readCalls := store.stats() | |
| return store.lastHead() >= 1 && readCalls == 0 | |
| return store.lastHead() >= 1 | |
| }, time.Second, 10*time.Millisecond) | |
| require.Eventually(t, func() bool { | |
| return store.lastHead() >= 1 | |
| }, time.Second, 10*time.Millisecond) | |
| _, readCalls := store.stats() | |
| require.Equal(t, 0, readCalls) |
🤖 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/service/chatbridge/monitor_external_test.go` around lines 154 - 156,
The test is missing the assertion to verify that no reads occurred, which is
part of the test's contract as indicated by the test name. After the
require.Eventually block that waits for store.lastHead() to advance to at least
1, add an assertion to check that readCalls equals 0 to ensure the
no-destination fast-path behavior is properly guarded and the test validates
both cursor advancement and the absence of read operations.
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
This was referenced Jun 18, 2026
This was referenced Jul 6, 2026
This was referenced Jul 24, 2026
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 CodeRabbit
GET /dags/{fileName}/specto includevalueReferenceNotices.ValueReferenceNoticesButtonwhen notices are present.