fix(#1660): add validation for start command parameters

[yottahmd]

fixes #1660

Summary by CodeRabbit

Release Notes

• New Features

  • Added support for JSON-formatted parameters when executing DAG commands

• Bug Fixes

  • Enhanced parameter validation to verify correct positional parameter counts during DAG execution
  • Improved error reporting for invalid parameter configurations

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR introduces parameter validation for the dagu start command to enforce the -- separator between the DAG path and positional parameters. It adds validation logic to check for proper argument separation and validate that the count of provided positional parameters matches the declared parameters in the DAG.

Changes

Cohort / File(s)	Summary
Parameter Validation Module internal/cmd/params_validation.go	Introduces parameter parsing and validation utilities including argument separator validation, positional parameter count validation, token parsing with support for quoted values, JSON input detection, and helper functions to count declared versus provided positional parameters.
Start Command Integration internal/cmd/start.go	Integrates pre-load validation checks to validate the argument separator and positional parameter count before loading and executing the DAG.
Test Coverage internal/cmd/start_test.go	Adds comprehensive test cases for positional parameter validation (too few/many params), named parameter handling with --params flag, backward compatibility, and imports testify/assert for assertions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(#1660): add validation for start command parameters' clearly summarizes the main change: adding parameter validation to the dagu start command to address issue #1660.
Linked Issues check	✅ Passed	The code changes implement validation for the start command parameters as required by issue #1660, including separator validation and positional parameter count checking.
Out of Scope Changes check	✅ Passed	All changes are directly related to the linked issue #1660: parameter validation logic, error handling, and test coverage for the dagu start command.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch 1660-fix

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@internal/cmd/params_validation.go`:
- Around line 82-92: The negated compound condition in isJSONParams (currently
written as if !((strings.HasPrefix(input, "{") && strings.HasSuffix(input, "}"))
|| (strings.HasPrefix(input, "[") && strings.HasSuffix(input, "]")) ) { return
false }) should be rewritten to satisfy the linter by removing the outer
negation using De Morgan's law: replace the negated OR with the equivalent AND
of negations (i.e., !(A&&B) && !(C&&D)) or, more readably, invert the branch to
check the positive case ((A&&B) || (C&&D)) and return false in the else branch;
update the condition in isJSONParams accordingly referencing the same
HasPrefix/HasSuffix checks.
- Around line 20-22: paramTokenRegex uses an incorrect subpattern for
backtick-quoted values (`(?:\\"|[^"]*)`) that matches non-quote characters
instead of non-backtick characters; update the regex used by paramTokenRegex to
use a backtick-safe pattern (e.g., `[^`]*` for the backtick-quoted branch) so
backtick-quoted tokens like `echo "message"` are tokenized correctly, and make
the identical fix to the same pattern occurrence in the params validation logic
in the spec package (note: leave the separate backtickRegex used for evaluation
unchanged).
- Around line 37-55: In validateStartPositionalParamCount, avoid failing when
the user supplied only named params: after extracting provided tokens (from
extractProvidedParamTokens) and before comparing got :=
countPositionalParams(provided) to expected :=
countDeclaredPositionalParams(...), detect that there are zero positional tokens
(got == 0) or that all provided tokens are named and in that case return nil
(i.e., skip the positional-count check); update the check around
countPositionalParams/provide handling so only cases with actual positional
tokens trigger the got != expected error.

🧹 Nitpick comments (4)

internal/cmd/params_validation.go (2)

102-118: Consider using stringutil.RemoveQuotes for consistency with the rest of the codebase.

The manual quote-stripping logic (strings.Trim + strings.ReplaceAll) diverges from the existing stringutil.RemoveQuotes utility (which uses strconv.Unquote and handles more edge cases). Since this is only used for counting tokens the impact is low, but aligning with the shared utility reduces maintenance surface.

---

130-142: countDeclaredPositionalParams never returns an error but has error in its signature.

The function always returns nil for the error. If no error path is anticipated, simplify the return type to just int. This avoids unnecessary error-checking boilerplate at the call site (line 47-49).

internal/cmd/start_test.go (2)

153-157: Use require.NoError instead of assert.NoError for consistency.

The coding guidelines and retrieved learnings specify using stretchr/testify/require for assertions. While assert vs require doesn't matter functionally here (last statement), mixing both packages in the same file creates inconsistency.

Suggested fix

-	assert.NoError(t, err)
+	require.NoError(t, err)

And drop the assert import on line 12 if no other usages remain.

Based on learnings: "Use stretchr/testify/require for assertions and shared fixtures from internal/test instead of duplicating mocks."

---

108-141: Consider adding a test case for JSON params bypassing positional count validation.

The code in extractProvidedParamTokens explicitly skips validation for JSON input (isJSONParams → skipValidation=true), but no test exercises this path. A subtest like "AcceptsJSONParamsWithoutPositionalCount" passing --params '{"key":"value"}' to a DAG with positional defaults would close the gap.

Based on learnings: "Co-locate Go tests as *_test.go files; favour table-driven cases and cover failure paths."

[codecov]

Codecov Report

❌ Patch coverage is 84.94624% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.70%. Comparing base (d496dde) to head (dcba7f7).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/cmd/params_validation.go	43.75%	3 Missing and 6 partials ⚠️
internal/core/startparams.go	94.52%	2 Missing and 2 partials ⚠️
internal/cmd/start.go	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1663      +/-   ##
==========================================
- Coverage   70.18%   69.70%   -0.48%     
==========================================
  Files         345      347       +2     
  Lines       38664    38957     +293     
==========================================
+ Hits        27135    27154      +19     
- Misses       9361     9479     +118     
- Partials     2168     2324     +156     

Files with missing lines	Coverage Δ
internal/core/spec/params.go	89.60% <ø> (+0.48%)	⬆️
internal/cmd/start.go	29.33% <75.00%> (-7.85%)	⬇️
internal/core/startparams.go	94.52% <94.52%> (ø)
internal/cmd/params_validation.go	6.73% <43.75%> (ø)

... and 19 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d496dde...dcba7f7. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.