Preserve zero-valued input defaults - #58291
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the implementation label and has <=100 new lines of code in business logic directories.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Completed PR #58291 review with no actionable blocking comments.
|
|
Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix correctly preserves zero defaults and is covered by an end-to-end normalization test.
Pull request overview
Preserves explicit zero defaults during custom safe-output normalization and adds regression coverage.
Changes:
- Replaces truthy fallback with nullish fallback.
- Tests numeric
0defaults and adds smoke workflow inputs. - Regenerates the compiled workflow and dependency pins.
File summaries
| File | Description |
|---|---|
actions/setup/js/collect_ndjson_output.cjs |
Preserves falsy defaults during normalization. |
actions/setup/js/collect_ndjson_output.test.cjs |
Adds zero-default regression coverage. |
.github/workflows/smoke-workflow-call-with-inputs.md |
Adds numeric zero inputs to both triggers. |
.github/workflows/smoke-workflow-call-with-inputs.lock.yml |
Regenerates the compiled workflow. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — no blocking issues found; approving.
📋 Details
The root cause (truthiness check inputSchema.default || undefined treating falsy defaults like 0/false/'' as missing) is correctly fixed with a nullish-coalescing (??) fallback. A regression test (should preserve zero-valued defaults in custom safe-job inputs) exercises the exact failure mode. The added task-count (type: number, default: 0) example in smoke-workflow-call-with-inputs.md/.lock.yml gives good end-to-end coverage for both workflow_call and workflow_dispatch triggers. Checked the rest of collect_ndjson_output.cjs for similar truthiness bugs on inputSchema.* — none found, so the fix is complete for this file.
Positive highlights:
- ✅ Root cause fixed (not just symptom) with the correct operator
- ✅ Regression test added matching the described bug
- ✅ Real workflow example added to prevent silent regressions in compiled output
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 19.7 AIC · ⌖ 14.5 AIC · ⊞ 10.3K
Comment /matt to run again
There was a problem hiding this comment.
Reviewed with harden/audit lenses (bug_fix). The || → ?? fix correctly preserves falsy defaults (0, false, "") without introducing new edge-state issues; regression test and workflow example are appropriate. No blocking or actionable issues found; lock.yml is auto-generated. Approving.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 15.5 AIC · ⌖ 14.1 AIC · ⊞ 8.3K
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
|
🎉 This pull request is included in a new release. Release: |
Numeric input defaults of
0were treated as missing because runtime normalization used truthiness. This affected custom safe-output inputs despite workflows compiling the default correctly.Changes
0defaults.workflow_callandworkflow_dispatchexamples withtype: numberanddefault: 0.