feat: dependency #376
feat: dependency #376
Conversation
📝 WalkthroughWalkthroughAdds a task-dependency subsystem: protobuf types and error codes, new CLI flags in calloc/cbatch/crun, parsing/validation utilities to populate per-task Dependencies, integration to apply dependencies before submission, and dependency-status formatting in ccontrol output. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as CLI user
participant Cmd as Command (calloc/cbatch/crun)
participant Parser as util.SetTaskDependencies
participant CTLD as ctld (submission)
participant Output as ccontrol/output
Note right of Cmd: parse flags / script args
User->>Cmd: invoke with --dependency / script deps
Cmd->>Parser: SetTaskDependencies(task, depStr)
alt parse success
Parser-->>Cmd: task.Dependencies populated
Cmd->>CTLD: submit TaskToCtld (includes Dependencies)
CTLD-->>Cmd: returns TaskInfo (may include DependencyStatus)
Cmd->>Output: printJobDetails(TaskInfo)
Output-->>Cmd: formatted dependency status (if present)
Cmd-->>User: displays job details
else parse error
Parser-->>Cmd: returns error
Cmd-->>User: exit with error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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: 9
🧹 Nitpick comments (4)
internal/util/string.go (2)
1313-1389: Consider validating job IDs are non-zero.Line 1366 parses the job ID but doesn't validate that it's non-zero. Zero is typically an invalid job ID in schedulers. Consider adding validation.
Apply this diff:
for _, jobMatch := range jobMatches { jobId, _ := strconv.ParseUint(jobMatch[1], 10, 32) + if jobId == 0 { + return fmt.Errorf("invalid job ID '0' in dependency: %s", subDepStr) + } var delaySeconds uint64 = 0
1370-1373: Document the auto-conversion behavior for numeric delays.The code automatically treats numeric-only delay values as minutes by appending 'm'. This is a reasonable default, but it might be unexpected for users. Consider documenting this behavior in a comment or ensuring it's mentioned in user-facing documentation.
protos/PublicDefs.proto (2)
135-162: Dependency schema additions look coherent; consider adding brief docs for API usersThe new
DependencyType,DependencyCondition,Dependencies, andDependencyStatusdefinitions are structurally consistent and leave room for future extension (e.g., additional dependency types). To make this public surface easier to consume, consider adding short comments describing:
- The intended string/CLI representation of each
DependencyType(and how it maps to whatinternal/ccontrol/output.goprints).- The precise meaning of
is_orand howpendingplusready_time/infinite_future/infinite_pastshould be interpreted by clients.This will help keep server behavior, CLI parsing, and display formatting aligned over time.
728-729: Ensure new dependency-related ErrCode values are wired through all error-handling pathsAdding
ERR_MISSING_DEPENDENCYandERR_DB_INSERT_FAILEDextends the public error enum cleanly, but callers that switch onErrCode(CLI, frontend, logs, translations) may need updates to:
- Provide user-friendly messages for these two new codes.
- Avoid falling back to overly generic handling paths.
Please verify that error-to-string mappings and any user-visible messaging have been updated accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/calloc/calloc.go(1 hunks)internal/calloc/cmd.go(2 hunks)internal/cbatch/cbatch.go(2 hunks)internal/cbatch/cmd.go(2 hunks)internal/ccontrol/output.go(2 hunks)internal/crun/cmd.go(2 hunks)internal/crun/crun.go(1 hunks)internal/util/err.go(2 hunks)internal/util/string.go(2 hunks)protos/PublicDefs.proto(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
internal/calloc/cmd.go (2)
internal/cbatch/cmd.go (1)
FlagDependency(64-64)internal/crun/cmd.go (1)
FlagDependency(43-43)
internal/crun/crun.go (4)
internal/calloc/cmd.go (1)
FlagDependency(49-49)internal/cbatch/cmd.go (1)
FlagDependency(64-64)internal/crun/cmd.go (1)
FlagDependency(43-43)internal/util/string.go (1)
SetTaskDependencies(1313-1389)
internal/cbatch/cbatch.go (2)
internal/util/string.go (1)
SetTaskDependencies(1313-1389)internal/cbatch/cmd.go (1)
FlagDependency(64-64)
internal/calloc/calloc.go (4)
internal/calloc/cmd.go (1)
FlagDependency(49-49)internal/cbatch/cmd.go (1)
FlagDependency(64-64)internal/crun/cmd.go (1)
FlagDependency(43-43)internal/util/string.go (1)
SetTaskDependencies(1313-1389)
internal/crun/cmd.go (2)
internal/calloc/cmd.go (1)
FlagDependency(49-49)internal/cbatch/cmd.go (1)
FlagDependency(64-64)
internal/ccontrol/output.go (1)
internal/util/string.go (1)
SecondTimeFormat(438-452)
🪛 GitHub Actions: Go Code Quality Check
internal/util/err.go
[error] 1-1: gofmt formatting issue detected. Run 'gofmt -w .' to fix formatting.
internal/cbatch/cmd.go
[error] 1-1: gofmt formatting issue detected. Run 'gofmt -w .' to fix formatting.
internal/calloc/cmd.go
[error] 1-1: gofmt formatting issue detected. Run 'gofmt -w .' to fix formatting.
internal/cbatch/cbatch.go
[error] 1-1: gofmt formatting issue detected. Run 'gofmt -w .' to fix formatting.
internal/ccontrol/output.go
[error] 1-1: gofmt formatting issue detected. Run 'gofmt -w .' to fix formatting.
🔇 Additional comments (8)
internal/calloc/cmd.go (1)
107-107: LGTM!The flag registration follows the established pattern and the short flag
-ddoesn't conflict with other flags in this command.internal/util/err.go (1)
176-176: LGTM!The updated error message is more specific and clear compared to the generic "System error occurred" message.
internal/crun/cmd.go (2)
43-43: LGTM!The variable declaration follows the established pattern in this file.
121-121: LGTM!The flag registration is correct and the short flag
-ddoesn't conflict with existing flags.internal/cbatch/cmd.go (1)
159-159: LGTM!The flag registration follows the established pattern and is consistent with other commands.
internal/cbatch/cbatch.go (1)
341-346: LGTM!The error handling is consistent with the rest of the function, properly wrapping and returning errors instead of using
log.Fatal.internal/util/string.go (1)
605-613: LGTM!The duplicate task ID check is correctly implemented and provides a clear error message.
internal/ccontrol/output.go (1)
565-571: Dependency line integration inprintJobDetailslooks goodThe new block cleanly gates dependency output on a non-nil
DependencyStatusand non-empty formatted string, so it won’t spam emptyDependency=lines for jobs without dependencies. This is a straightforward, safe addition.
| if FlagDependency != "" { | ||
| err := util.SetTaskDependencies(task, FlagDependency) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Use consistent error handling pattern.
This code uses log.Fatal(err) which exits immediately. Other validation errors in this function return wrapped errors (e.g., lines 405, 412, 476). Use the same pattern for consistency.
Apply this diff:
if FlagDependency != "" {
err := util.SetTaskDependencies(task, FlagDependency)
if err != nil {
- log.Fatal(err)
+ return util.WrapCraneErr(util.ErrorCmdArg, "Invalid argument: %s", err)
}
}📝 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.
| if FlagDependency != "" { | |
| err := util.SetTaskDependencies(task, FlagDependency) | |
| if err != nil { | |
| log.Fatal(err) | |
| } | |
| } | |
| if FlagDependency != "" { | |
| err := util.SetTaskDependencies(task, FlagDependency) | |
| if err != nil { | |
| return util.WrapCraneErr(util.ErrorCmdArg, "Invalid argument: %s", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/calloc/calloc.go around lines 466 to 471, the code currently calls
log.Fatal(err) when util.SetTaskDependencies fails; change this to return a
wrapped error consistent with surrounding validation (e.g., return
fmt.Errorf("set task dependencies: %w", err)). Also add the fmt import if
missing and ensure the function signature returns an error so this return
compiles and matches the existing error-handling pattern used elsewhere in the
function.
internal/cbatch/cmd.go
Outdated
|
|
||
| FlagConfigFilePath string | ||
| FlagJson bool | ||
| FlagDependency string |
There was a problem hiding this comment.
Address the gofmt formatting issue.
The pipeline detected a formatting issue in this file. Run gofmt -w internal/cbatch/cmd.go to fix it.
🤖 Prompt for AI Agents
In internal/cbatch/cmd.go around line 64, the file has a gofmt formatting issue
(improper spacing/formatting around the FlagDependency declaration); run the
formatter to fix it by executing `gofmt -w internal/cbatch/cmd.go` (or `gofmt -w
.` in repo root) and commit the updated file so the code matches gofmt
formatting rules.
| func formatDependencyType(depType protos.DependencyType) string { | ||
| switch depType { | ||
| case protos.DependencyType_AFTER: | ||
| return "AFTER" | ||
| case protos.DependencyType_AFTER_ANY: | ||
| return "AFTERANY" | ||
| case protos.DependencyType_AFTER_OK: | ||
| return "AFTEROK" | ||
| case protos.DependencyType_AFTER_NOT_OK: | ||
| return "AFTERNOTOK" | ||
| default: | ||
| return "UNKNOWN" | ||
| } | ||
| } | ||
|
|
||
| func formatDependencyCondition(cond *protos.DependencyCondition) string { | ||
| if cond == nil { | ||
| return "" | ||
| } | ||
| depTypeStr := formatDependencyType(cond.Type) | ||
| if cond.DelaySeconds > 0 { | ||
| delayStr := util.SecondTimeFormat(int64(cond.DelaySeconds)) | ||
| return fmt.Sprintf("%s:%d+%s", depTypeStr, cond.JobId, delayStr) | ||
| } | ||
| return fmt.Sprintf("%s:%d", depTypeStr, cond.JobId) | ||
| } | ||
|
|
||
| func formatDependencyStatus(depStatus *protos.DependencyStatus) string { | ||
| var buf strings.Builder | ||
|
|
||
| sep := "," | ||
| if depStatus.IsOr { | ||
| sep = "?" | ||
| } | ||
|
|
||
| if len(depStatus.Pending) > 0 { | ||
| var pendingStrs []string | ||
| for _, cond := range depStatus.Pending { | ||
| pendingStrs = append(pendingStrs, formatDependencyCondition(cond)) | ||
| } | ||
| buf.WriteString(fmt.Sprintf("PendingDependencies=%s ", strings.Join(pendingStrs, sep))) | ||
| } | ||
|
|
||
| switch depStatus.GetReadyTimeType().(type) { | ||
| case *protos.DependencyStatus_ReadyTime: | ||
| readyTime := depStatus.GetReadyTime().AsTime() | ||
| if(depStatus.IsOr || len(depStatus.Pending) == 0) { | ||
| buf.WriteString(fmt.Sprintf("Status=ReadyAfter %s", readyTime.In(time.Local).Format("2006-01-02 15:04:05"))) | ||
| } else { | ||
| buf.WriteString("Status=WaitForAll") | ||
| } | ||
| case *protos.DependencyStatus_InfiniteFuture: | ||
| if depStatus.GetInfiniteFuture() { | ||
| if depStatus.IsOr { | ||
| if len(depStatus.Pending) == 0 { | ||
| buf.WriteString("Status=AllFailed") | ||
| } else { | ||
| buf.WriteString("Status=WaitForAny") | ||
| } | ||
| } else { | ||
| buf.WriteString("Status=SomeFailed") | ||
| } | ||
| } | ||
| case *protos.DependencyStatus_InfinitePast: | ||
| // must be AND | ||
| if depStatus.GetInfinitePast() { | ||
| buf.WriteString("Status=WaitForAll") | ||
| } | ||
| } | ||
| return buf.String() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "output.go" -path "*/ccontrol/*" | head -5Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 99
🏁 Script executed:
wc -l internal/ccontrol/output.goRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 101
🏁 Script executed:
sed -n '689,759p' internal/ccontrol/output.goRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 2042
🏁 Script executed:
fd -e proto | xargs rg "message DependencyCondition" -A 10 2>/dev/nullRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 527
🏁 Script executed:
fd -type f -name "*.go" | xargs rg "func SecondTimeFormat" -B 2 -A 5 2>/dev/null | head -30Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 599
🏁 Script executed:
gofmt -d internal/ccontrol/output.go | head -100Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 113
🏁 Script executed:
fd -e go -path "*/ccontrol/*" -type f | xargs rg "formatDependencyStatus|TestFormatDependency" 2>/dev/nullRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 448
🏁 Script executed:
sed -n '715,759p' internal/ccontrol/output.goRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 1322
🏁 Script executed:
find . -name "*.md" -o -name "*.txt" | xargs rg "DependencyStatus|dependency.*status" -i 2>/dev/null | head -20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 747
🏁 Script executed:
find . -name "*.go" | xargs rg "DependencyStatus" -B 2 -A 2 2>/dev/null | grep -i "test\|spec\|comment" | head -30Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 139
🏁 Script executed:
rg "message DependencyStatus" protos/ -A 30Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 1479
🏁 Script executed:
python3 << 'EOF'
import import math
# math.MaxInt64 = 2^63 - 1 = 9223372036854775807
max_int64 = 2**63 - 1
max_uint64 = 2**64 - 1
print(f"math.MaxInt64: {max_int64}")
print(f"math.MaxUint64: {max_uint64}")
print(f"Overflow risk: uint64 values > {max_int64} will overflow")
EOFRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 156
🏁 Script executed:
rg "delay_seconds|DelaySeconds" . --type go -B 3 -A 3 2>/dev/null | head -50Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 962
🏁 Script executed:
sed -n '/^func SecondTimeFormat/,/^func [A-Z]/p' internal/util/string.go | head -40Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 571
🏁 Script executed:
rg "if\s*\(" internal/ccontrol/output.go | head -10Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 123
🏁 Script executed:
find . -name "*.yml" -o -name "*.yaml" | xargs cat 2>/dev/null | grep -i "gofmt\|format" | head -20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 355
Fix type conversion overflow and gofmt formatting issue
Two issues require attention:
- Type conversion overflow in
formatDependencyCondition:cond.DelaySecondsisuint64whileutil.SecondTimeFormatexpectsint64. Any delay value exceedingmath.MaxInt64(9,223,372,036,854,775,807 seconds) will overflow to a negative value and produce malformed output (e.g., negative days). Add bounds checking before conversion:if cond.DelaySeconds > math.MaxInt64 { /* handle large value */ }or use a fallback representation. - gofmt style violation: The condition
if(depStatus.IsOr || len(depStatus.Pending) == 0)must have a space afterif. The CI pipeline enforces gofmt compliance, so rungofmt -w internal/ccontrol/output.gobefore merging.
The status mapping logic in formatDependencyStatus is complex and would benefit from validation against the ctld-side specification, though the code logic appears internally consistent.
🤖 Prompt for AI Agents
internal/ccontrol/output.go lines 689-759: fix two issues — first, prevent
uint64->int64 overflow in formatDependencyCondition by checking if
cond.DelaySeconds > math.MaxInt64 before converting; if it is, use a safe
fallback representation (e.g., cap to math.MaxInt64, or return a special string
like "huge" or format seconds directly) and add the required import for math;
second, fix gofmt spacing by adding a space after the if in
formatDependencyStatus (change "if(depStatus.IsOr ..." to "if (depStatus.IsOr
..." or better "if depStatus.IsOr || len(depStatus.Pending) == 0") and run gofmt
-w internal/ccontrol/output.go before committing.
| if FlagDependency != "" { | ||
| err := util.SetTaskDependencies(task, FlagDependency) | ||
| if err != nil { | ||
| log.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Use consistent error handling pattern.
This code uses log.Fatal(err) which exits immediately without proper error handling. Other validation errors in this function return wrapped errors (e.g., util.NewCraneErr or util.WrapCraneErr). Use the same pattern for consistency.
Apply this diff:
if FlagDependency != "" {
err := util.SetTaskDependencies(task, FlagDependency)
if err != nil {
- log.Fatal(err)
+ return util.WrapCraneErr(util.ErrorCmdArg, "Invalid argument: %s", err)
}
}📝 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.
| if FlagDependency != "" { | |
| err := util.SetTaskDependencies(task, FlagDependency) | |
| if err != nil { | |
| log.Fatal(err) | |
| } | |
| } | |
| if FlagDependency != "" { | |
| err := util.SetTaskDependencies(task, FlagDependency) | |
| if err != nil { | |
| return util.WrapCraneErr(util.ErrorCmdArg, "Invalid argument: %s", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/crun/crun.go around lines 1108-1113, replace the direct
log.Fatal(err) call with the repository's consistent error-wrapping/return
pattern: capture the error from util.SetTaskDependencies and return a wrapped
Crane error (e.g., return util.WrapCraneErr(err, "setting task dependencies") or
similar existing wrapper used elsewhere) so the function returns a propagated,
wrapped error instead of exiting the process.
|
|
||
| uint32 gid = 14; // egid | ||
|
|
||
| Dependencies dependencies = 15; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the current state of TaskToCtld in PublicDefs.proto
rg -A 30 "message TaskToCtld" protos/PublicDefs.proto | head -60Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 776
🏁 Script executed:
# Check git history to see what was previously at field tag 15
cd protos && git log --oneline -p -- PublicDefs.proto | grep -A 5 -B 5 "= 15;" | head -100Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 2796
🏁 Script executed:
# Also check if there are any recent commits affecting field 15 in TaskToCtld
git log --all --pretty=format:"%h %s" -- protos/PublicDefs.proto | head -20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 953
🏁 Script executed:
# Check the state of PublicDefs.proto before the dependency commit
git show e4695a0^:protos/PublicDefs.proto | grep -A 40 "message TaskToCtld"Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 108
🏁 Script executed:
# Also verify if there are any other commits that might have used tag 15 in TaskToCtld
git log --all -p -- protos/PublicDefs.proto | grep -B 10 "TaskToCtld" | grep "= 15;" | head -20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 52
🏁 Script executed:
# Check if there's a CHANGELOG or release notes that might indicate field history
find . -type f \( -name "CHANGELOG*" -o -name "RELEASE*" -o -name "*.md" \) | grep -iE "change|release|history" | head -10Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 52
🏁 Script executed:
# Check for any documentation about proto versioning or compatibility
rg -l "tag.*15|field.*15|compatibility|breaking" --type md | head -10Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 218
🏁 Script executed:
# Check if there are git tags that might represent releases
git tag | head -20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 52
🏁 Script executed:
# Check if there's any released version information in git
git branch -a | head -20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 190
Verify tag 15 in TaskToCtld was not used in previous releases before assigning dependencies
Adding Dependencies dependencies = 15; is wire-compatible only if field number 15 was never assigned to a different field in any released version. The current codebase shows tag 15 is newly assigned to dependencies, but confirm against your project's release history and language-specific proto bindings that tag 15 was not previously used in TaskToCtld. Reusing a tag that held a different field in any shipped release would be a breaking change.
🤖 Prompt for AI Agents
In protos/PublicDefs.proto around line 186, the new field "Dependencies
dependencies = 15;" may reuse tag 15 from prior releases; verify that tag 15 was
never previously assigned in any shipped version of TaskToCtld by searching git
history (git log/blame), checking released tags/branches, and inspecting
language-specific generated protobuf bindings and published artifacts
(packages/jars/wheels) for any prior use of field number 15; if tag 15 was ever
used, pick an unused field number instead (or restore the old semantics), and if
you must permanently avoid reuse add a reserved 15 line to the proto for past
uses and choose a new number, then regenerate language bindings and update
changelog/release notes accordingly.
protos/PublicDefs.proto
Outdated
| /* 24-28 Reserved */ | ||
|
|
||
| // Dynamic task information | ||
| optional DependencyStatus dependency_status = 29; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd protos && cat PublicDefs.proto | grep -n "message TaskInfo" -A 100 | head -150Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 3333
🏁 Script executed:
cd protos && git log --oneline -20 -- PublicDefs.protoRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 94
🏁 Script executed:
cd protos && git show e4695a0 -- PublicDefs.proto | head -100Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 2749
🏁 Script executed:
cd protos && git log --all --oneline | tail -20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 791
🏁 Script executed:
cd protos && git log --all --oneline -- PublicDefs.protoRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 3183
🏁 Script executed:
cd protos && git show e4695a0^:protos/PublicDefs.proto | grep -n "message TaskInfo" -A 60 | head -80Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 108
🏁 Script executed:
cd protos && git show e4695a0 --statRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 7514
🏁 Script executed:
cd protos && git show 5788342:protos/PublicDefs.proto | grep -n "message TaskInfo" -A 60 | head -80Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 1999
🏁 Script executed:
cd protos && git log --all -p -- protos/PublicDefs.proto | grep -B 5 -A 5 "Reserved" | head -60Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 52
🏁 Script executed:
cd protos && git blame -L 449,449 5788342:protos/PublicDefs.protoRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 140
🏁 Script executed:
cd protos && for commit in 04ee4e9 bd29cfb 3fd94f1 692d3d9 8e7e6a9; do
echo "=== Commit $commit ==="
git show $commit:protos/PublicDefs.proto 2>/dev/null | grep -A 30 "message TaskInfo" | grep -E "(Reserved|optional|bool held|TaskStatus status)" | head -5
doneRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 313
🏁 Script executed:
cd protos && git diff 5788342 e4695a0 -- protos/PublicDefs.proto | grep -A 20 -B 5 "Reserved\|dependency_status" | head -60Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 52
🏁 Script executed:
cd protos && git show e4695a0:protos/PublicDefs.proto | grep -B 3 -A 3 "24-28 Reserved"Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 290
Verify tag 29 was never used in production before reassigning it
TaskInfo now changes the reserved range from 26-29 (in commit 5788342) to 24-28 and assigns tag 29 to optional DependencyStatus dependency_status. While tag 29 was previously marked as reserved, this change should be validated:
- Tags 24-25 are being removed from reserved (previously part of 26-29 range)
- Tag 29 is transitioning from reserved to active use
- Reserved tags are currently declared only as a comment (
/* 24-28 Reserved */) rather than using proper proto3 syntax (reserved 24, 25, 26, 27, 28;)
Recommend either: (1) confirming tag 29 was never actually used in any released version, or (2) using formal reserved declarations in the proto file to prevent accidental reuse in the future.
🤖 Prompt for AI Agents
In protos/PublicDefs.proto around lines 480 to 483, TaskInfo's reserved range
was changed from 26-29 to 24-28 and tag 29 was assigned to optional
DependencyStatus; verify that tag 29 was never used in any released/production
descriptor sets or wire data before assigning it (check historical commits,
released .proto artifacts, compiled descriptors, and any running services), and
either (A) confirm no production usage and update the comment to a proper proto
reserved declaration for any tags that should remain protected, or (B) if tag 29
was used, revert the assignment and instead add a formal reserved 24,25,26,27,28
(and 29 if needed) declaration to prevent accidental reuse.
NamelessOIer
force-pushed
the
dev/dependency_re
branch
from
e4695a0 to
47afab6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
internal/util/err.go (1)
203-204: Fix the gofmt formatting issue.The indentation on line 203 uses tabs instead of spaces, which is inconsistent with other entries and causes the gofmt pipeline failure. Run
gofmt -w internal/util/err.goto fix.internal/calloc/cmd.go (1)
49-49: Fix the gofmt formatting issue.The declaration has inconsistent spacing. Run
gofmt -w internal/calloc/cmd.goto fix.internal/cbatch/cmd.go (1)
64-64: Fix the gofmt formatting issue.Run
gofmt -w internal/cbatch/cmd.goto fix the formatting.internal/cbatch/cbatch.go (1)
196-200: Fix the indentation issue.The code inside the case block is not properly indented, causing the gofmt pipeline failure.
Apply this diff:
case "--dependency", "-d": - err := util.SetTaskDependencies(task, arg.val) - if err != nil { + err := util.SetTaskDependencies(task, arg.val) + if err != nil { return nil, fmt.Errorf("invalid argument: failed to set dependencies: %w", err) - } + }protos/PublicDefs.proto (2)
186-186: Verify field number 15 was never used in prior releases.Adding
Dependencies dependencies = 15;is only safe if field number 15 was never assigned to a different field in any released version ofTaskToCtld. Reusing a field number that held a different field in any shipped release would break wire compatibility with older clients/servers. Verify against your project's release history that tag 15 was not previously used.Run the following script to check the git history for prior usage of field number 15 in TaskToCtld:
#!/bin/bash # Description: Search git history for any prior assignment of field number 15 in TaskToCtld # Search through all commits for changes to TaskToCtld around field 15 git log --all -p -- protos/PublicDefs.proto | grep -B 10 -A 2 "= 15;" | grep -B 10 -A 2 "TaskToCtld" | head -50 # Also check if there are any git tags (releases) we should verify against echo "=== Git tags/releases ===" git tag -l | head -20
479-482: Verify tag 29 was never used before reassigning from reserved range.The reserved range was changed from
26-29to25-28, and tag 29 is now assigned tooptional DependencyStatus dependency_status. This is only safe if:
- Tag 29 was never used in any released version before being reserved
- No production systems have descriptors or wire data using tag 29
Additionally, consider using proper proto3
reservedsyntax instead of comments for future protection:reserved 25, 26, 27, 28;Run the following script to check tag 29 usage history:
#!/bin/bash # Description: Verify tag 29 was never used in TaskInfo before being reserved # Check git history for any prior usage of tag 29 in TaskInfo git log --all -p -- protos/PublicDefs.proto | grep -B 15 -A 2 "= 29;" | grep -B 15 -A 2 "TaskInfo" | head -100 # Check when the reserved range was introduced git log --all -p -- protos/PublicDefs.proto | grep -B 5 -A 5 "Reserved" | grep -B 5 -A 5 "26-29\|25-28" | head -50internal/ccontrol/output.go (2)
721-731: Fix type conversion overflow.Line 727 converts
cond.DelaySeconds(uint64) to int64 forutil.SecondTimeFormat, which will overflow for values exceedingmath.MaxInt64(9,223,372,036,854,775,807 seconds, approximately 292 billion years). Add bounds checking before conversion.Apply this diff:
func formatDependencyCondition(cond *protos.DependencyCondition) string { if cond == nil { return "" } depTypeStr := formatDependencyType(cond.Type) if cond.DelaySeconds > 0 { - delayStr := util.SecondTimeFormat(int64(cond.DelaySeconds)) + var delayStr string + if cond.DelaySeconds > math.MaxInt64 { + delayStr = fmt.Sprintf("%ds", cond.DelaySeconds) + } else { + delayStr = util.SecondTimeFormat(int64(cond.DelaySeconds)) + } return fmt.Sprintf("%s:%d+%s", depTypeStr, cond.JobId, delayStr) } return fmt.Sprintf("%s:%d", depTypeStr, cond.JobId) }
733-776: Fix gofmt formatting issue.Line 752 has a gofmt violation:
if(depStatus.IsOr || len(depStatus.Pending) == 0)must have a space afterif. The CI pipeline is failing due to this formatting issue. Rungofmt -w internal/ccontrol/output.goto fix.Apply this diff:
- if(depStatus.IsOr || len(depStatus.Pending) == 0) { + if depStatus.IsOr || len(depStatus.Pending) == 0 { buf.WriteString(fmt.Sprintf("Status=ReadyAfter %s", readyTime.In(time.Local).Format("2006-01-02 15:04:05"))) } else { buf.WriteString("Status=WaitForAll") }
🧹 Nitpick comments (1)
internal/util/string.go (1)
1476-1480: Consider tightening the job list validation regex.The regex
^[\d+:a-zA-Z]+$is quite permissive and would match strings without any digits (e.g., "abc"). While the subsequentjobPatternregex would fail to extract valid job IDs, the error message at line 1496 ("no valid job IDs found") is less informative than the one at line 1479 about format. Consider validating that at least one digit exists, or rely solely on thejobPatternmatching.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/calloc/calloc.go(1 hunks)internal/calloc/cmd.go(2 hunks)internal/cbatch/cbatch.go(2 hunks)internal/cbatch/cmd.go(2 hunks)internal/ccontrol/output.go(2 hunks)internal/crun/cmd.go(2 hunks)internal/crun/crun.go(1 hunks)internal/util/err.go(2 hunks)internal/util/string.go(2 hunks)protos/PublicDefs.proto(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/calloc/calloc.go
- internal/crun/crun.go
- internal/crun/cmd.go
🧰 Additional context used
🧬 Code graph analysis (4)
internal/cbatch/cmd.go (1)
internal/crun/cmd.go (1)
FlagDependency(46-46)
internal/cbatch/cbatch.go (4)
internal/util/string.go (1)
SetTaskDependencies(1447-1523)internal/calloc/cmd.go (1)
FlagDependency(49-49)internal/cbatch/cmd.go (1)
FlagDependency(64-64)internal/crun/cmd.go (1)
FlagDependency(46-46)
internal/calloc/cmd.go (2)
internal/cbatch/cmd.go (1)
FlagDependency(64-64)internal/crun/cmd.go (1)
FlagDependency(46-46)
internal/ccontrol/output.go (1)
internal/util/string.go (1)
SecondTimeFormat(438-452)
🪛 GitHub Actions: Go Code Quality Check
internal/cbatch/cmd.go
[error] 1-1: gofmt formatting check failed. The file is not properly formatted according to gofmt.
internal/cbatch/cbatch.go
[error] 1-1: gofmt formatting check failed. The file is not properly formatted according to gofmt.
internal/calloc/cmd.go
[error] 1-1: gofmt formatting check failed. The file is not properly formatted according to gofmt.
internal/util/err.go
[error] 1-1: gofmt formatting check failed. The file is not properly formatted according to gofmt.
internal/ccontrol/output.go
[error] 1-1: gofmt formatting check failed. The file is not properly formatted according to gofmt.
🔇 Additional comments (9)
internal/util/err.go (1)
176-176: LGTM on the simplified error message.The updated message for
ERR_BEYOND_TASK_IDis more concise and accurately describes the error condition.internal/calloc/cmd.go (1)
107-107: LGTM on the new dependency flag.The flag is correctly wired with
-dshorthand and appropriate description, consistent with other modules.internal/cbatch/cmd.go (1)
159-159: LGTM on the dependency flag registration.Consistent with the pattern used in calloc and crun modules.
internal/cbatch/cbatch.go (1)
341-346: LGTM on CLI dependency handling.The CLI flag handling correctly follows the established pattern where command-line arguments take precedence over script values.
internal/util/string.go (1)
637-645: LGTM on duplicate dependency check.The validation correctly prevents duplicate task IDs in the dependency list.
internal/ccontrol/output.go (2)
582-588: LGTM! Clean integration of dependency status output.The dependency status output is appropriately gated by nil-check and empty-string check, ensuring it only prints when meaningful information is available. This follows the pattern used for other optional fields in the job details output.
706-719: LGTM! Straightforward enum-to-string mapping.The implementation correctly maps all defined DependencyType enum values to their string representations and provides a safe fallback for unexpected values.
protos/PublicDefs.proto (2)
135-162: LGTM! Well-designed dependency type definitions.The new protobuf messages provide a comprehensive dependency system:
DependencyTypeenum covers standard scheduling dependency typesDependencyConditionincludes job ID, type, and optional delayDependenciessupports both OR and AND semantics viais_orflagDependencyStatusoneof design elegantly represents three states: ready at a specific time, infinite future (blocked), or infinite past (already satisfied)
726-727: LGTM! New error codes for dependency feature.The two new error codes are appropriately added:
ERR_MISSING_DEPENDENCY = 85: For handling missing/invalid dependency referencesERR_DB_INSERT_FAILED = 86: For database insertion failuresThe sequential numbering after the existing
ERR_STEP_RES_BEYOND = 84maintains enum consistency. Adding new error codes at the end of an enum is a safe, backward-compatible change.
| func SetTaskDependencies(task *protos.TaskToCtld, depStr string) error { | ||
| depStr = strings.TrimSpace(depStr) | ||
|
|
||
| hasComma := strings.Contains(depStr, ",") | ||
| hasQuestion := strings.Contains(depStr, "?") | ||
| if hasComma && hasQuestion { | ||
| return fmt.Errorf("cannot use both ',' and '?' in the dependency string") | ||
| } | ||
|
|
||
| sep := "," | ||
| if hasQuestion { | ||
| sep = "?" | ||
| } | ||
|
|
||
| task.Dependencies = &protos.Dependencies{ | ||
| IsOr: hasQuestion, | ||
| } | ||
|
|
||
| // Regex: (type):(job_id[+delay][:job_id[+delay]]...) | ||
| depPattern := regexp.MustCompile(`^(after|afterok|afternotok|afterany):(.+)$`) | ||
| jobPattern := regexp.MustCompile(`(\d+)(?:\+([0-9a-zA-Z]+))?`) | ||
|
|
||
| depStrList := strings.Split(depStr, sep) | ||
| for _, subDepStr := range depStrList { | ||
| matches := depPattern.FindStringSubmatch(subDepStr) | ||
| if matches == nil { | ||
| return fmt.Errorf("unrecognized dependency string: %s", subDepStr) | ||
| } | ||
|
|
||
| depTypeStr, jobsStr := matches[1], matches[2] | ||
|
|
||
| if !regexp.MustCompile(`^[\d+:a-zA-Z]+$`).MatchString(jobsStr) { | ||
| return fmt.Errorf("invalid job list format in '%s': should only contain job_id[+delay] separated by ':'", jobsStr) | ||
| } | ||
|
|
||
| var depType protos.DependencyType | ||
| switch depTypeStr { | ||
| case "after": | ||
| depType = protos.DependencyType_AFTER | ||
| case "afterok": | ||
| depType = protos.DependencyType_AFTER_OK | ||
| case "afternotok": | ||
| depType = protos.DependencyType_AFTER_NOT_OK | ||
| case "afterany": | ||
| depType = protos.DependencyType_AFTER_ANY | ||
| } | ||
|
|
||
| jobMatches := jobPattern.FindAllStringSubmatch(jobsStr, -1) | ||
| if jobMatches == nil { | ||
| return fmt.Errorf("no valid job IDs found in: %s", subDepStr) | ||
| } | ||
|
|
||
| for _, jobMatch := range jobMatches { | ||
| jobId, _ := strconv.ParseUint(jobMatch[1], 10, 32) | ||
| var delaySeconds uint64 = 0 | ||
| if jobMatch[2] != "" { | ||
| delayStr := jobMatch[2] | ||
| // If delayStr contains only digits, treat it as minutes by default | ||
| if regexp.MustCompile(`^\d+$`).MatchString(delayStr) { | ||
| delayStr = delayStr + "m" | ||
| } | ||
| seconds, err := ParseRelativeTime(delayStr) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid delay format '%s' in dependency: %v", jobMatch[2], err) | ||
| } | ||
| delaySeconds = uint64(seconds) | ||
| } | ||
|
|
||
| task.Dependencies.Deps = append(task.Dependencies.Deps, &protos.DependencyCondition{ | ||
| JobId: uint32(jobId), | ||
| Type: depType, | ||
| DelaySeconds: delaySeconds, | ||
| }) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for SetTaskDependencies references and tests
rg -n "SetTaskDependencies" --type go -g '!vendor/*'Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Search for test files related to SetTaskDependencies or string.go tests
find . -name "*test*.go" -type f | grep -E "(string|util)" | head -20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Also search for any test references to dependency parsing
rg -n "SetTaskDependencies|dependency.*test|test.*dependency" --type go -i | head -30Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Check if there's a string_test.go file specifically
find . -name "string_test.go" -type fRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Let's look at the complete function and verify the parsing logic
sed -n '1447,1523p' internal/util/string.goRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 2393
🏁 Script executed:
#!/bin/bash
# Search for any documentation about dependency format in the codebase
rg -n "dependency.*format|delay.*format|job.*id.*format" --type go -i | head -20Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 785
🏁 Script executed:
#!/bin/bash
# Check if there are any README or docs files that describe the dependency format
find . -type f \( -name "README*" -o -name "*.md" -o -name "DOCS*" \) | xargs grep -l "dependency\|delay" 2>/dev/null | head -10Repository: PKUHPC/CraneSched-FrontEnd
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Check for any documentation or comments above the SetTaskDependencies function
sed -n '1420,1450p' internal/util/string.goRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 975
🏁 Script executed:
#!/bin/bash
# Check the calling sites for usage patterns and documentation
sed -n '1300,1310p' internal/crun/crun.goRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 228
🏁 Script executed:
#!/bin/bash
# Check another calling site for context
sed -n '190,210p' internal/cbatch/cbatch.goRepository: PKUHPC/CraneSched-FrontEnd
Length of output: 776
Add tests and documentation for dependency parsing behavior.
The function lacks both test coverage and documentation. With no godoc comment or format specification, the expected formats are undocumented despite the function being used across multiple commands (crun, cbatch, calloc). The implementation details—numeric-only delays treated as minutes, comma for AND semantics, question mark for OR semantics—must be tested and documented to prevent regressions and ensure users understand the expected format.
🤖 Prompt for AI Agents
internal/util/string.go lines 1447-1523: the SetTaskDependencies function is
undocumented and untested; add a godoc comment immediately above the function
that clearly specifies the accepted dependency string grammar, examples (e.g.
"after:123", "afterok:123+5m:124", numeric-only delay means minutes, ',' = AND,
'?' = OR) and the semantics of values and errors, then create unit tests under
internal/util with table-driven cases that cover: valid single and multiple
dependencies, both separators (',' and '?'), numeric-only delays treated as
minutes, explicit time units, invalid formats (bad characters, unknown
dependency types), missing job IDs, parse errors from ParseRelativeTime, and
mixed separators error; ensure tests assert the produced protos.Dependencies
fields (IsOr, Deps with JobId, Type, DelaySeconds) and run go test to confirm
behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/ccontrol/output.go (1)
721-731: Fix uint64 to int64 overflow risk.Line 727 casts
cond.DelaySeconds(uint64) directly to int64 forutil.SecondTimeFormat. Any delay exceedingmath.MaxInt64(≈292 billion years, or 9,223,372,036,854,775,807 seconds) will overflow to a negative value and produce malformed output with negative days.🔎 Apply this diff to add bounds checking:
func formatDependencyCondition(cond *protos.DependencyCondition) string { if cond == nil { return "" } depTypeStr := formatDependencyType(cond.Type) if cond.DelaySeconds > 0 { - delayStr := util.SecondTimeFormat(int64(cond.DelaySeconds)) + var delayStr string + if cond.DelaySeconds > math.MaxInt64 { + delayStr = fmt.Sprintf("%d seconds", cond.DelaySeconds) + } else { + delayStr = util.SecondTimeFormat(int64(cond.DelaySeconds)) + } return fmt.Sprintf("%s:%d+%s", depTypeStr, cond.JobId, delayStr) } return fmt.Sprintf("%s:%d", depTypeStr, cond.JobId) }
🧹 Nitpick comments (1)
internal/calloc/cmd.go (1)
107-107: LGTM! Flag registration is correct and consistent.The flag registration follows the established pattern and the short flag
-ddoes not conflict with existing flags.Regarding the past review comment about gofmt formatting: the current code appears properly formatted. Please verify by running
gofmt -d internal/calloc/cmd.goto confirm the formatting issue has been resolved.💡 Optional enhancement
Consider making the description more informative by adding an example:
- RootCmd.Flags().StringVarP(&FlagDependency, "dependency", "d", "", "Conditions for job to execute") + RootCmd.Flags().StringVarP(&FlagDependency, "dependency", "d", "", "Conditions for job to execute (e.g., afterok:123,afterany:456)")This helps users understand the expected format without consulting documentation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/calloc/cmd.go(2 hunks)internal/cbatch/cbatch.go(2 hunks)internal/cbatch/cmd.go(2 hunks)internal/ccontrol/output.go(2 hunks)internal/util/err.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cbatch/cmd.go
- internal/cbatch/cbatch.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/calloc/cmd.go (1)
internal/crun/cmd.go (1)
FlagDependency(46-46)
internal/ccontrol/output.go (1)
internal/util/string.go (1)
SecondTimeFormat(438-452)
🔇 Additional comments (6)
internal/calloc/cmd.go (1)
45-49: LGTM! Variable declarations follow established patterns.The variable declarations are properly formatted and consistent with the rest of the file.
internal/util/err.go (2)
176-176: LGTM!The error message clarification removes ambiguity and provides a more precise description of the condition.
203-204: LGTM!The new error codes are well-defined with clear, user-friendly messages that align with the dependency subsystem requirements.
internal/ccontrol/output.go (3)
582-588: LGTM!The dependency output integration is well-structured with appropriate nil checks and only displays when dependency status is present and non-empty.
706-719: LGTM!The dependency type formatting is straightforward and handles all documented dependency types with a sensible fallback for unknown values.
733-776: Code is properly formatted and logically correct.The dependency status formatting function uses consistent tab indentation, proper operator spacing, and follows Go conventions. The logic correctly handles the three ReadyTimeType cases (ReadyTime, InfiniteFuture, InfinitePast) with appropriate status strings based on the IsOr flag and pending dependency count. No formatting or logical issues found.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/util/string.go (2)
1454-1533: Add comprehensive godoc comment to document the dependency string format.The function is exported and called from multiple commands (calloc, cbatch, crun) but lacks documentation. Users and maintainers need to understand the expected format and semantics without reading the implementation.
Based on learnings, this was flagged in previous reviews. The dependency string grammar should document:
- Accepted dependency types (after, afterok, afternotok, afterany)
- Job ID and delay format (jobid[+delay])
- Separator semantics (',' for AND, '?' for OR)
- Delay units (numeric-only defaults to minutes, or explicit units like m/h/d/w)
- Examples (e.g., "afterok:123+5m:124", "after:100,afterok:101")
Example godoc template
+// SetTaskDependencies parses a dependency specification string and populates task.Dependencies. +// +// Format: type:jobid[+delay][:jobid[+delay]]... +// - type: after, afterok, afternotok, afterany +// - jobid: numeric job ID (required) +// - delay: optional delay; numeric values default to minutes, or specify units (e.g., 5m, 1h, 2d) +// - Multiple dependencies: separated by ',' (AND) or '?' (OR), but not both +// +// Examples: +// - "afterok:123" - wait for job 123 to complete successfully +// - "afterok:123+5m:124" - wait for jobs 123 and 124, with 5-minute delay after 123 +// - "after:100,afterok:101" - AND: wait for job 100 (any state) AND job 101 (success) +// - "afterok:200?afterok:201" - OR: wait for job 200 OR job 201 to succeed +// +// Returns an error if the format is invalid or if both ',' and '?' are used. func SetTaskDependencies(task *protos.TaskToCtld, depStr string) error {
1454-1533: Tests for SetTaskDependencies are still missing.The function lacks unit test coverage for various input formats, edge cases, and error conditions.
Based on learnings, this was flagged in previous reviews. Consider table-driven tests covering:
- Valid single and multiple dependencies
- Both separators (',' and '?')
- Numeric-only delays (should default to minutes)
- Explicit time units (m, h, d, w)
- Invalid formats (bad characters, unknown types)
- Missing job IDs
- Parse errors from ParseRelativeTime
- Mixed separator error
- Zero job IDs (after adding validation)
Verify that the produced
protos.Dependenciesstruct has correctIsOrflag andDepsentries with expectedJobId,Type, andDelaySeconds.
🧹 Nitpick comments (1)
internal/util/string.go (1)
1507-1510: Good: Error handling for job ID parsing is now present, but add zero validation.The error from
ParseUintis now properly captured and returned, addressing the previous review comment. However, job ID 0 may be invalid in this context and should be explicitly rejected.Suggested zero validation
for _, jobMatch := range jobMatches { jobId, err := strconv.ParseUint(jobMatch[1], 10, 32) if err != nil { return fmt.Errorf("invalid job ID '%s' in dependency: %v", jobMatch[1], err) } + if jobId == 0 { + return fmt.Errorf("invalid job ID: 0 is not allowed in dependencies") + } var delaySeconds uint64 = 0
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/util/string.go
🔇 Additional comments (2)
internal/util/string.go (2)
176-204: LGTM: Compound duration parsing is well-implemented.The enhancement correctly handles multi-unit durations like "1h30m" by detecting the compound format, extracting value-unit pairs, and accumulating the total seconds. The case-insensitive unit matching and clear error messages for invalid units are good practices.
644-652: LGTM: Duplicate task ID validation is correct.The validation correctly uses a map to detect duplicate job IDs in dependencies with O(1) lookup time. The error message clearly identifies which task ID is duplicated.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.