Reviewing files that changed from the base of the PR and between d4b8484 and d648232.

• internal/cmd/agent_executor.go (1 hunks)
• internal/cmd/agent_executor_test.go (1 hunks)
• internal/cmd/context.go (8 hunks)
• internal/cmd/coordinator.go (1 hunks)
• internal/cmd/dry.go (1 hunks)
• internal/cmd/exec.go (1 hunks)
• internal/cmd/flags.go (1 hunks)
• internal/cmd/restart.go (1 hunks)
• internal/cmd/retry.go (1 hunks)
• internal/cmd/scheduler.go (1 hunks)
• internal/cmd/start.go (1 hunks)
• internal/common/collections/syncmap.go (1 hunks)
• internal/common/config/config.go (2 hunks)
• internal/common/config/context.go (1 hunks)
• internal/common/config/context_test.go (1 hunks)
• internal/common/config/env.go (1 hunks)
• internal/common/config/env_test.go (1 hunks)
• internal/common/config/loader.go (15 hunks)
• internal/common/config/loader_test.go (12 hunks)
• internal/common/config/timezone_test.go (4 hunks)
• internal/common/config/timzone.go (1 hunks)
• internal/common/masking/masker.go (1 hunks)
• internal/common/stringutil/kv.go (2 hunks)
• internal/core/dag.go (1 hunks)
• internal/core/execution/context.go (1 hunks)
• internal/core/spec/builder_test.go (2 hunks)
• internal/core/spec/variables.go (1 hunks)
• internal/integration/base_dag_test.go (2 hunks)
• internal/integration/parallel_test.go (1 hunks)
• internal/runtime/agent/dbclient.go (1 hunks)
• internal/runtime/env.go (4 hunks)
• internal/runtime/env_test.go (2 hunks)
• internal/runtime/runner.go (2 hunks)
• internal/runtime/subcmd.go (1 hunks)
• internal/runtime/subcmd_test.go (1 hunks)
• internal/service/frontend/server.go (2 hunks)
• internal/service/scheduler/scheduler.go (1 hunks)
• internal/service/worker/handler_test.go (1 hunks)
• internal/test/helper.go (4 hunks)

• GitHub Check: Test on ubuntu-latest

internal/common/stringutil/kv.go (2)

15-18: LGTM! Clean refactor to strings.Cut.

The use of strings.Cut simplifies the logic and correctly handles cases where no "=" delimiter exists by returning the entire string as the key.

---

20-26: LGTM! Correct handling of delimiter presence.

The conditional return based on found ensures that entries without "=" correctly return an empty value.

internal/common/collections/syncmap.go (1)

29-32: LGTM: Improved key-value parsing with strings.Cut.

The refactor from strings.SplitN to strings.Cut is cleaner and more efficient. The found flag properly guards against malformed entries (strings without "="), ensuring only valid key-value pairs are inserted.

internal/common/masking/masker.go (1)

76-80: LGTM: Consistent use of strings.Cut for env parsing.

The migration to strings.Cut is consistent with the broader refactor and improves clarity. Returning empty strings when "=" is not found is more explicit than checking array bounds.

internal/runtime/agent/dbclient.go (1)

44-47: LGTM: Output variable parsing aligned with refactor.

The use of strings.Cut with the found guard is consistent with the repository-wide refactor and correctly handles output variable parsing.

internal/runtime/env_test.go (2)

371-372: LGTM: Test correctly uses strings.Cut for env parsing.

The test properly mirrors the production code refactor, extracting key-value pairs and handling evaluated values.

Also applies to: 377-377

---

399-400: LGTM: Consistent env parsing in test setup.

The test setup correctly uses strings.Cut to parse and evaluate step environment variables, consistent with the refactor pattern.

Also applies to: 406-407

internal/integration/base_dag_test.go (1)

72-72: LGTM: Config namespace migration to Core.

The change from Global.Peer to Core.Peer is consistent with the PR's config refactor objectives.

Also applies to: 186-186

internal/common/config/context.go (1)

36-36: LGTM: BaseEnv access migrated to Core namespace.

The change from Global.BaseEnv to Core.BaseEnv aligns with the config refactor and is straightforward.

internal/cmd/start.go (1)

491-491: LGTM: Peer config migrated to Core namespace.

The change from Global.Peer to Core.Peer is consistent with the config refactor across the codebase.

internal/cmd/dry.go (1)

102-102: LGTM: Peer config migrated to Core namespace.

The change from Global.Peer to Core.Peer aligns with the PR's config namespace refactor.

internal/service/frontend/server.go (2)

75-87: Frontend funcsConfig: TZ/TzOffsetInSec now sourced from Core – consistent with new config model

Switching TZ and TzOffsetInSec to cfg.Core.* aligns this server with the new Core configuration struct; types and semantics remain unchanged.

---

149-157: HTTP request logger now reads log format from Core.LogFormat – LGTM

Using srv.config.Core.LogFormat == "json" to control structured logging matches the Core-based config refactor and keeps behavior equivalent to the previous Global-based check.

internal/cmd/scheduler.go (1)

42-45: Scheduler logging: log-format now read from Core.LogFormat – OK

The info log during scheduler initialization correctly pulls ctx.Config.Core.LogFormat, consistent with the new Core config struct.

internal/cmd/retry.go (1)

166-183: Retry agent: peer config switched to Core.Peer – consistent with Core migration

Passing ctx.Config.Core.Peer into agent.New maintains the expected peer TLS configuration and aligns this call site with the new Core-based config API.

internal/cmd/agent_executor_test.go (1)

104-115: Tests updated to use Config.Core – matches new public config struct

Initializing Config.Core{Debug, LogFormat} mirrors the new config.Config shape and keeps configureLoggerForProgress tests exercising the right fields.

internal/common/config/env.go (1)

40-52: Env filtering: strings.Cut-based parsing is a safe, clearer improvement

Using strings.Cut to derive key and skipping entries without "=" simplifies the code and avoids relying on malformed environment entries; allowed-key and prefix checks remain unchanged.

internal/common/config/config.go (1)

10-12: Config API: introduction of Core field/type looks coherent with repo-wide usage

Defining an exported Core struct and hanging it off Config.Core cleanly encapsulates global settings (debug, log format, TZ/location, peer, base env). The field and type are well-documented and line up with all observed call sites in commands and services.

Also applies to: 52-81

internal/service/scheduler/scheduler.go (1)

75-78: Scheduler time location: now taken from Core.Location – behavior preserved

Using cfg.Core.Location (falling back to time.Local when nil) is consistent with the new Core config and maintains the same defaulting semantics for the scheduler.

internal/cmd/restart.go (1)

150-162: Restart agent: peer config migrated to Core.Peer – consistent and correct

Routing ctx.Config.Core.Peer into agent.New brings the restart flow in line with the Core-based config model without changing agent behavior.

internal/cmd/exec.go (1)

165-169: LGTM! Clean refactor to strings.Cut.

The migration from strings.SplitN to strings.Cut is more idiomatic and improves readability. The validation logic correctly checks found, key, and value to ensure proper key=value format.

internal/runtime/subcmd.go (1)

29-29: LGTM! Configuration namespace migration.

The change from cfg.Global.BaseEnv to cfg.Core.BaseEnv aligns with the broader refactor migrating configuration to the Core namespace.

internal/cmd/coordinator.go (1)

146-151: LGTM! TLS configuration migrated to Core namespace.

The TLS configuration now correctly reads from cfg.Core.Peer.* fields, aligning with the project-wide configuration refactor.

internal/common/config/timezone_test.go (1)

14-64: LGTM! Test updated for Core-based configuration.

All test cases correctly use &Core{} instead of &Global{}, aligning with the updated setTimezone function signature.

internal/core/dag.go (1)

352-355: LGTM! Cleaner parsing with strings.Cut.

The refactor from strings.SplitN to strings.Cut improves readability and is more idiomatic. The behavior remains the same: only parameters containing "=" are included in the map.

internal/common/config/timzone.go (1)

9-9: LGTM! Function signature updated for Core configuration.

The parameter type change from *Global to *Core aligns with the configuration refactor. The function body remains unchanged as the required fields exist in the Core struct.

internal/integration/parallel_test.go (1)

970-974: LGTM! Test helper refactored to use strings.Cut.

The parsing logic is cleaner with strings.Cut. The found flag provides explicit validation, and the code is more readable than array indexing.

internal/cmd/agent_executor.go (1)

72-77: LGTM! Logger configuration updated to Core namespace.

The logger configuration now correctly reads from ctx.Config.Core.Debug and ctx.Config.Core.LogFormat, consistent with the configuration refactor.

internal/runtime/subcmd_test.go (1)

24-26: LGTM! Clean migration to Core-based configuration.

The test correctly updates from Global to Core configuration namespace, aligning with the new public API structure.

internal/core/spec/variables.go (1)

38-42: LGTM! More idiomatic string parsing.

The migration from strings.SplitN to strings.Cut is cleaner and more readable for extracting key-value pairs. The found flag makes the intent explicit.

internal/core/execution/context.go (1)

37-40: LGTM! Consistent key-value parsing.

The strings.Cut approach correctly handles environment variable parsing, skipping entries without "=" delimiters. This aligns with the broader migration to Cut-based parsing across the codebase.

internal/common/config/env_test.go (1)

50-53: LGTM! Test helper aligns with production code.

The test correctly adopts strings.Cut for environment variable parsing, matching the updated production code patterns.

internal/core/spec/builder_test.go (1)

3657-3660: LGTM! Consistent test updates.

Both test segments correctly adopt strings.Cut for environment variable parsing, maintaining consistency with the broader codebase refactor.

Also applies to: 3685-3688

internal/common/config/context_test.go (1)

72-72: LGTM! Correct configuration namespace update.

The test properly updates from Global to Core configuration, aligning with the new API structure.

internal/runtime/runner.go (1)

473-490: LGTM! Improved environment variable evaluation logic.

The refactor correctly:

• Uses strings.Cut for cleaner key-value extraction
• Evaluates only the value portion (not the entire "KEY=value" string), which is the correct behavior
• Maintains proper storage format as KEY=evaluatedValue
• Adds appropriate error logging for malformed entries

This is a logical improvement over the previous approach.

internal/service/worker/handler_test.go (1)

203-205: LGTM! Test configuration updated correctly.

The test properly migrates from Global to Core configuration namespace, consistent with the broader refactor.

internal/runtime/env.go (3)

77-82: LGTM! Clean refactor to strings.Cut.

The use of strings.Cut is more idiomatic than strings.SplitN and the found check properly handles entries without an = separator.

---

117-121: LGTM! Consistent use of strings.Cut for param parsing.

The pattern correctly stores the full param string (in key=value format) when a separator is found, using the extracted key as the map key.

---

390-394: LGTM! Consistent refactor to strings.Cut.

The change aligns with the rest of the codebase and properly handles the found flag to skip malformed entries.

internal/cmd/flags.go (1)

294-304: LGTM! Explicit Viper instance injection.

Passing an explicit *viper.Viper instance enables per-command configuration isolation and aligns with the new ConfigLoader pattern. This improves testability and avoids global state pollution.

internal/cmd/context.go (6)

60-68: LGTM! Migration to Core.* configuration paths.

The logger options now correctly reference Core.Debug and Core.LogFormat instead of the removed Global.* paths.

---

80-81: LGTM! Isolated Viper instance for command context.

Creating a fresh viper.New() instance and passing it to bindFlags prevents configuration leakage between commands and improves testability.

---

108-112: LGTM! Service-scoped configuration loading.

The serviceForCommand helper enables loading only necessary configuration sections per command, which improves startup performance and reduces configuration coupling.

---

231-235: LGTM! Migration to Core.Peer for coordinator client.

The peer configuration is now correctly sourced from Core.Peer instead of Global.Peer.

---

291-291: LGTM! Migration to Core.SkipExamples.

The DAG store option now correctly references Core.SkipExamples.

---

181-199: Verify command name coverage for service mapping

Coverage is comprehensive and correct. All CLI commands are properly accounted for: infrastructure commands (server, scheduler, worker, coordinator) have explicit mappings; agent commands (start, restart, retry, dry, exec) map to ServiceAgent; queue operations (enqueue, dequeue) and utility commands (version, validate, status, stop, start-all, migrate) appropriately use the ServiceNone default, which loads all config sections. This is a safe and efficient approach.

internal/test/helper.go (4)

130-137: LGTM! Test helper migrated to new loader pattern.

The test helper now creates an isolated Viper instance and uses the new ConfigLoader pattern. The Core.* field assignments for timezone settings are correct.

---

232-238: LGTM! Config file writer uses Core.* paths.

The helper config file generation correctly sources values from Core.Debug, Core.LogFormat, Core.DefaultShell, and Core.TZ.

---

505-507: LGTM! Consistent strings.Cut usage in test assertions.

The parsing logic correctly uses strings.Cut and validates that the output is in key=value format before checking the value.

---

567-568: LGTM! Agent construction uses Core.Peer.

The peer configuration is correctly sourced from Core.Peer for agent construction in tests.

internal/common/config/loader_test.go (6)

15-29: LGTM! Well-designed test helpers.

The testLoad and testLoadWithError helpers properly use t.Helper() for better stack traces and create isolated Viper instances per test. This aligns with the new ConfigLoader pattern and prevents test pollution.

Based on learnings, using stretchr/testify/require is the preferred approach.

---

142-161: LGTM! Test expectations updated for Core namespace.

The expected config structure correctly uses Core instead of Global, with all relevant fields (Debug, LogFormat, TZ, Location, DefaultShell, SkipExamples, Peer, BaseEnv) properly referenced.

---

248-256: LGTM! Path test uses Core.BaseEnv.

The assertion correctly accesses cfg.Core.BaseEnv.AsSlice() instead of the old Global path.

---

355-370: LGTM! YAML test expectations updated.

The expected Core struct is correctly populated with all fields including the Peer configuration.

---

540-591: LGTM! Error case tests use new helper.

All error-expecting tests now use testLoadWithError, which provides cleaner test code and consistent error handling.

---

789-800: LGTM! TestBindEnv_AsPath uses explicit loader.

The test now creates a ConfigLoader instance and calls bindEnvironmentVariables directly, aligning with the new explicit Viper instance pattern.

internal/common/config/loader.go (5)

960-967: Good use of strings.Cut for parsing.

The refactor from manual string splitting to strings.Cut is cleaner and more robust. The handling of empty values and whitespace trimming is appropriate.

---

188-242: Well-structured service-aware configuration loading.

The buildConfig method cleanly separates core/paths loading (always needed) from service-specific sections, with proper validation gating. The modular approach improves maintainability.

---

112-119: Correct bit-flag checking with sensible fallback.

The requires method properly uses bitwise AND for section checking and safely falls back to SectionAll for unknown services.

---

131-186: Clean configuration loading flow with isolated viper instance.

The Load method properly uses the isolated viper instance (l.v) throughout, handles missing config files gracefully, and maintains backward compatibility with admin.yaml merging.

---

49-57: ConfigLoader struct with isolated viper instance is well-designed.

The introduction of an isolated viper instance (v *viper.Viper) in the ConfigLoader struct enables better testability and avoids global state issues. The service field enables selective configuration loading.

Reviewing files that changed from the base of the PR and between d648232 and e0cceed.

• internal/common/config/loader.go (15 hunks)
• internal/runtime/env.go (4 hunks)

• internal/runtime/env.go

• GitHub Check: Test on ubuntu-latest

internal/common/config/loader.go (4)

131-186: Config loading pipeline is cohesive and service-aware

The Load → buildConfig flow is well-structured: viper setup, config file/admin merge, unmarshal into Definition, then per-section loaders gated via requires(), followed by legacy handling, path finalization, and targeted validation. Always loading core+paths while gating the rest aligns with the Service/ConfigSection model, and timezone initialization errors are surfaced clearly.

No functional issues stand out in this part.

Also applies to: 188-242, 244-275, 277-293

---

295-401: Server and auth configuration handling is thorough

The server loader and validator cover the main edge cases (permissions overrides, optional TLS, legacy vs. new auth settings, TTL parsing with warnings, OIDC auto-detection) and normalize BasePath via cleanServerBasePath in a predictable way. The validation checks are appropriate and avoid over-constraining the config.

No changes needed here from a correctness perspective.

Also applies to: 403-415, 990-1008

---

416-454: Section-specific loaders and defaults are generally well-behaved

UI, queues, coordinator, worker, monitoring, and path finalization logic all look consistent and robust. In particular, seeding cfg.UI from viper defaults before applying def.UI fixes the prior default/validation mismatch, and parseWorkerLabels plus the map-based cases give good flexibility for worker labels while ignoring malformed entries safely.

Also applies to: 456-472, 474-514, 516-559, 561-587, 589-612

---

964-987: Worker label parsing is robust

parseWorkerLabels’ use of trimming, skipping empty segments, and strings.Cut to only split on the first = yields predictable label maps and tolerates messy input without panicking or mis-parsing values containing =.

Looks good as-is.

Reviewing files that changed from the base of the PR and between e0cceed and b9fa844.

• internal/common/config/loader.go (15 hunks)
• internal/runtime/env.go (4 hunks)

• GitHub Check: Test on ubuntu-latest

internal/runtime/env.go (4)

77-80: LGTM: Clean migration to strings.Cut.

The use of strings.Cut is more idiomatic than strings.SplitN for parsing "key=value" strings. It clearly signals intent with the found boolean and handles edge cases correctly (empty values, values containing "=", missing separators).

---

94-101: LGTM: Correct use of strings.Cut with precedence check.

The migration to strings.Cut is correct, and the exists check (line 98) properly implements the lower precedence for Step.Env as clarified in the comments above.

---

120-123: LGTM: Consistent strings.Cut usage for DAG params.

The migration is correct. Storing the entire param string (line 122) is consistent with how Variables is used throughout the codebase—values are expected in "key=value" format (see line 63 and line 77).

---

393-396: LGTM: Clean strings.Cut migration.

The migration is correct and follows the same safe pattern as the other changes in this file.

internal/common/config/loader.go (5)

18-119: Excellent service-aware loading architecture!

The introduction of Service and ConfigSection types with the requirements mapping creates a clean, efficient system for loading only necessary configuration. The bit flag approach for ConfigSection is idiomatic and performant, and the requires() method provides a safe API with sensible fallback to SectionAll for unknown services.

The architecture correctly handles Core and Paths as always-loaded sections (confirmed in buildConfig lines 193-197), while other sections load conditionally based on service requirements.

---

188-242: Well-structured configuration pipeline!

The refactored buildConfig method implements a clear, maintainable pipeline:

1. Always loads Core and Paths (required by all services)
2. Conditionally loads service-specific sections using the requires() API
3. Applies legacy field and environment overrides
4. Finalizes derived paths
5. Validates only the loaded sections

This modular approach improves both performance and code clarity.

---

416-454: UI config loading correctly fixed!

The refactored loadUIConfig now properly handles the case where the UI section is absent from the config file:

1. Lines 418-423 explicitly retrieve viper defaults BEFORE checking def.UI
2. Lines 427-443 only override defaults when explicit values are provided
3. Validation at line 450 now passes even when the UI section is absent, as MaxDashboardPageLimit correctly defaults to 100 (set via viper on line 795)

This resolves the previous validation issue mentioned in past reviews.

---

933-951: Note: Path normalization mutates environment variables.

Lines 942-946 normalize relative paths in environment variables to absolute paths and update the environment variables using os.Setenv:

if b.isPath {
    if val := os.Getenv(fullEnv); val != "" {
        if abs, err := filepath.Abs(val); err == nil && abs != val {
            _ = os.Setenv(fullEnv, abs)
        }
    }
}

This ensures consistency but has the side effect that any code reading these environment variables after configuration loading will see the normalized absolute paths rather than the original values. This behavior is acceptable for internal configuration loading but is worth noting for maintainability.

---

967-991: Excellent use of strings.Cut for label parsing!

The refactored parseWorkerLabels uses strings.Cut instead of strings.SplitN, which is cleaner and more idiomatic:

key, value, found := strings.Cut(pair, "=")
if found {
    key = strings.TrimSpace(key)
    value = strings.TrimSpace(value)
    if key != "" {
        labels[key] = value
    }
}

This correctly handles all edge cases:

• "key=value" → labels["key"] = "value"
• "key=" → labels["key"] = "" (allows empty values)
• "key" (no =) → skipped
• "=value" → skipped (empty key rejected)
• Whitespace trimmed appropriately

The implementation is robust and aligns with the PR objectives of standardizing to strings.Cut.

Reviewing files that changed from the base of the PR and between b9fa844 and 38551ca.

• internal/cmd/agent_executor.go (1 hunks)
• internal/cmd/context.go (8 hunks)
• internal/cmd/coordinator.go (2 hunks)
• internal/cmd/dry.go (2 hunks)
• internal/cmd/exec.go (2 hunks)
• internal/cmd/flags.go (2 hunks)
• internal/cmd/restart.go (2 hunks)
• internal/cmd/retry.go (2 hunks)
• internal/cmd/scheduler.go (1 hunks)
• internal/cmd/start.go (2 hunks)
• internal/common/config/context.go (1 hunks)
• internal/common/config/env.go (1 hunks)
• internal/common/config/loader.go (15 hunks)
• internal/common/config/timzone.go (1 hunks)
• internal/common/masking/masker.go (1 hunks)
• internal/common/stringutil/kv.go (2 hunks)
• internal/core/spec/variables.go (2 hunks)
• internal/runtime/env.go (4 hunks)
• internal/runtime/subcmd.go (1 hunks)
• internal/service/frontend/server.go (3 hunks)
• internal/service/scheduler/scheduler.go (2 hunks)
• internal/test/helper.go (5 hunks)

• internal/service/scheduler/scheduler.go
• internal/common/config/timzone.go
• internal/runtime/subcmd.go
• internal/service/frontend/server.go
• internal/common/config/context.go
• internal/cmd/dry.go
• internal/common/config/env.go
• internal/cmd/scheduler.go
• internal/runtime/env.go
• internal/cmd/restart.go
• internal/cmd/coordinator.go

• GitHub Check: Test on ubuntu-latest

internal/core/spec/variables.go (2)

16-26: LGTM! Comprehensive docstring.

The updated docstring clearly documents the function's parameters, behavior (including command substitution and variable expansion), and error conditions.

---

48-52: LGTM! Clean migration to strings.Cut.

The change from strings.SplitN to strings.Cut is correct and more idiomatic. The found boolean properly indicates whether the separator was present.

internal/common/masking/masker.go (1)

74-82: LGTM! Clean migration to strings.Cut.

The refactored splitEnv function correctly uses strings.Cut to split on the first '=' and safely handles entries without '=' by returning two empty strings. The caller at line 27 properly handles this by skipping empty values at line 30.

internal/common/stringutil/kv.go (2)

17-20: LGTM! Correct use of strings.Cut.

The Key() method correctly uses strings.Cut to extract the key portion before the first '='.

---

22-28: LGTM! Proper handling of entries without '='.

The Value() method correctly uses strings.Cut with the found flag to return an empty string when '=' is not present.

internal/cmd/agent_executor.go (1)

72-78: LGTM!

The migration from ctx.Config.Global.Debug and ctx.Config.Global.LogFormat to ctx.Config.Core.Debug and ctx.Config.Core.LogFormat is consistent with the Core struct definition. The logic remains unchanged.

internal/cmd/start.go (2)

460-469: Well-documented function.

The new docstring for executeDAGRun is comprehensive—it clearly describes the initialization flow, log file handling, DAG store setup, agent construction, and error scenarios.

---

500-500: LGTM!

The change from ctx.Config.Global.Peer to ctx.Config.Core.Peer is consistent with the Core struct definition and the overall config migration in this PR.

internal/cmd/exec.go (2)

65-72: Well-documented function.

The new docstring for runExec comprehensively describes the function's responsibilities including flag parsing, validation, DAG building, and execution/enqueueing logic.

---

173-178: LGTM! Improved parsing with strings.Cut.

Using strings.Cut is more idiomatic for Go 1.18+ and correctly handles edge cases:

• Missing = separator
• Empty key (e.g., =value)
• Empty value (e.g., key=)

internal/cmd/retry.go (2)

140-143: Well-documented function.

The docstring for executeRetry clearly describes the setup steps (log file, environment, DAG store, agent configuration) and the delegation to the shared executor.

---

180-180: LGTM!

The change from ctx.Config.Global.Peer to ctx.Config.Core.Peer is consistent with the Core struct definition and aligns with the same change in start.go and other files.

internal/cmd/flags.go (2)

279-295: LGTM!

The initFlags function is well-structured with clear separation of concerns:

• Always includes base flags (config, dagu-home, quiet, cpu-profile)
• Properly handles boolean vs string flag registration
• Marks required flags appropriately

---

297-311: LGTM! Thread-safe explicit Viper binding.

The updated bindFlags function:

• Takes an explicit *viper.Viper instance for better testability and per-service config isolation
• Uses config.WithViperLock for thread-safe registration
• Converts kebab-case flag names to camelCase keys via stringutil.KebabToCamel
• Only binds flags with bindViper: true, which is the correct subset

internal/cmd/context.go (4)

82-83: Good use of isolated viper instance per context.

Creating a dedicated viper instance and binding flags explicitly avoids global state pollution across commands. This aligns well with the refactor goals.

---

60-68: Core field migration looks correct.

References to c.Config.Core.Debug and c.Config.Core.LogFormat align with the new Core-based configuration structure.

---

236-240: Coordinator client config correctly uses Core.Peer.

The migration from Global.Peer to Core.Peer for TLS/connection settings is consistent with the broader refactor.

---

188-204: Mapping is intentionally partial with a safe fallback.

The switch statement explicitly maps performance-critical services: server, scheduler, worker, coordinator, and agent commands (start, restart, retry, dry, exec). All other commands—including stop, status, start-all, migrate, enqueue, dequeue, and version—fall back to ServiceNone, which loads all config sections. This is functionally correct and safe; commands get everything they might need rather than failing on missing configuration.

If optimization is desired later, commands like status, stop, and version could benefit from lighter service profiles (e.g., ServiceMinimal skipping Queues). However, this is not necessary; the current approach is straightforward and maintainable.

internal/test/helper.go (5)

132-134: Correct use of isolated viper instance in tests.

Creating a fresh viper instance per test avoids cross-test contamination, mirroring the production pattern in NewContext.

---

137-139: Core field assignments are correct.

Setting cfg.Core.TZ, cfg.Core.Location, and cfg.Core.TzOffsetInSec aligns with the Core-based config structure.

---

237-243: Core-based field references in config file writer are correct.

The fields cfg.Core.Debug, cfg.Core.LogFormat, cfg.Core.DefaultShell, and cfg.Core.TZ are correctly sourced from the Core struct.

---

510-512: Good use of strings.Cut for key=value parsing.

Using strings.Cut with a presence check (found) is cleaner and more idiomatic than strings.SplitN with length checks.

---

572-572: Agent construction correctly uses Core.Peer.

Passing d.Config.Core.Peer aligns with the Core-based configuration migration.

internal/common/config/loader.go (8)

18-47: Service type and constants are well-documented.

The Service enum with clear docstrings for each service type improves code clarity. The comments now correctly reference "Core" instead of "Global".

---

89-101: Bit flag comments are now correct.

The inline comments (// 2, // 4, etc.) now match the actual values after iota increment from SectionNone = 0. Previous review feedback has been addressed.

---

417-455: UI config default loading issue addressed.

The loadUIConfig now retrieves defaults via l.v.GetInt("ui.maxDashboardPageLimit") and similar calls before applying def.UI overrides. This addresses the previous review concern about validation failing when the UI section is absent.

---

557-562: ZombieDetectionInterval silent-disable behavior remains.

Per the past review comment: if a user provides an invalid duration string (e.g., "invalid"), parsing fails, a warning is logged, but IsSet() returns true, so the default (45s) is not applied. This silently disables zombie detection rather than failing validation.

The current behavior is documented in comments (lines 557-559), but consider whether this is the intended UX—invalid config silently disabling a feature could be surprising. The alternative is to return a validation error for unparseable durations.

Is the current "warn but disable" behavior intentional for zombieDetectionInterval? If so, document this in user-facing config documentation. If not, consider returning an error on parse failure to prevent misconfiguration.

---

819-824: envBinding struct is clear and well-documented.

The declarative binding approach with isPath flag for path normalization is clean and maintainable.

---

934-952: Environment variable binding with path normalization is correct.

The loop correctly applies path normalization before binding and uses the loader's isolated viper instance.

---

968-994: parseWorkerLabels uses strings.Cut correctly.

The function properly handles the key=value format, trims whitespace, and skips empty keys. The use of strings.Cut is more idiomatic than SplitN.

---

860-863: Duplicate environment bindings for legacy UI keys.

Lines 860-863 duplicate bindings for ui.maxDashboardPageLimit, ui.logEncodingCharset, ui.navbarColor, and ui.navbarTitle with legacy env var names. This is intentional for backward compatibility, but note that viper will use the last bound env var if multiple are set simultaneously. Document this precedence if it matters.

Reviewing files that changed from the base of the PR and between 38551ca and 3b1d20e.

• internal/cmd/agent_executor.go (1 hunks)
• internal/common/config/loader.go (15 hunks)
• internal/common/stringutil/kv.go (2 hunks)
• internal/common/stringutil/kv_test.go (7 hunks)

• GitHub Check: Test on ubuntu-latest

internal/common/stringutil/kv_test.go (3)

11-11: LGTM! Parallelization enhances test performance.

The addition of t.Parallel() to the test functions and subtests follows Go testing best practices and will improve test execution speed.

As per coding guidelines, table-driven tests with parallel execution are encouraged.

Also applies to: 14-14, 22-22, 33-33, 43-43, 129-129, 147-147, 183-183, 209-209, 219-219, 303-303

---

21-30: LGTM! Test correctly validates multi-equals handling in values.

This test case properly verifies that when a value contains '=' characters, Key() and Value() correctly split only on the first '=' separator.

---

42-53: LGTM! Round-trip test validates correctness.

This test confirms that a key-value pair constructed with NewKeyValue can be correctly retrieved via Key() and Value() methods, ensuring round-trip integrity.

internal/common/stringutil/kv.go (3)

18-21: LGTM! Cut-based parsing simplifies Key extraction.

The refactoring from strings.SplitN to strings.Cut correctly extracts the key (substring before the first '='). For entries without '=', it returns the entire string, which matches the documented behavior.

---

23-29: LGTM! Cut-based parsing handles multi-equals correctly.

The strings.Cut approach properly extracts everything after the first '=' as the value, correctly handling cases where the value itself contains '=' characters (e.g., "foo=bar=baz" returns "bar=baz").

---

56-69: LGTM! Clear documentation and correct Cut-based implementation.

The updated documentation (lines 56-59) accurately describes the behavior:

• Splits at first '='
• Skips entries without '='
• Allows empty values and empty keys

The implementation correctly uses strings.Cut and the found return value to skip invalid entries.

internal/cmd/agent_executor.go (2)

69-70: Comment properly updated.

The function comment now correctly describes the purpose and behavior of configureLoggerForProgress. Good improvement from the previous truncated version.

---

73-73: LGTM: Refactored to use Core configuration.

The changes correctly update the configuration access from Global to Core, aligning with the PR's refactoring objectives.

Also applies to: 77-78

internal/common/config/loader.go (13)

18-47: LGTM: Well-documented service-aware configuration design.

The service type enumeration and associated documentation clearly explain which configuration sections each service requires. This selective loading approach should improve performance and reduce unnecessary processing.

---

86-101: LGTM: Bit flag values correctly documented.

The inline comments now accurately reflect the computed bit flag values. The use of bit flags for configuration sections is appropriate for efficient OR-based membership checks.

---

103-120: LGTM: Service requirements correctly implemented.

The serviceRequirements map and requires() method correctly implement the service-to-section membership logic using bit flags. The fallback to SectionAll for unknown services (line 117) is a safe default.

---

189-243: LGTM: Well-structured configuration building workflow.

The buildConfig method correctly orchestrates configuration loading with appropriate service-aware gating. The sequence—core/paths → service-specific sections → legacy overrides → finalization → validation—is logical and ensures only necessary sections are processed.

---

245-276: LGTM: Core configuration loaded correctly.

The function properly initializes the Core configuration, including base environment and peer settings. The timezone initialization with error handling is appropriate.

---

417-455: LGTM: UI configuration loading properly defaults and validates.

The addition of default value loading from viper (lines 419-424) before checking the definition resolves the previous concern about validation failures when the UI section is absent from the config file. The viper defaults (set on line 795 to 100) ensure MaxDashboardPageLimit is never 0, allowing validation to pass.

---

617-678: LGTM: Service-aware legacy field handling.

The refactored LoadLegacyFields correctly applies service-aware gating, ensuring legacy fields are only processed for configuration sections that are being loaded. Path-related legacy fields are correctly applied unconditionally since paths are always loaded.

---

680-744: LGTM: Service-aware legacy environment variable handling.

The loadLegacyEnv function correctly implements service-aware gating for legacy environment variables. The use of SectionNone for universally applicable settings (like path fields) is a clear pattern.

---

967-993: LGTM: Improved label parsing with strings.Cut.

The refactor to use strings.Cut (line 982) instead of strings.SplitN is a good modernization. The function correctly handles malformed entries by skipping pairs without an = delimiter, and properly trims whitespace around keys and values.

---

592-615: LGTM: Path finalization correctly derives subdirectories.

The finalizePaths function properly sets derived paths relative to DataDir and handles the executable path with appropriate error handling.

---

765-816: LGTM: Comprehensive default values configured.

The setViperDefaultValues function properly initializes all configuration defaults using the loader's isolated viper instance. The default values are sensible and align with validation requirements.

---

933-951: LGTM: Environment variable binding with path normalization.

The bindEnvironmentVariables function correctly binds configuration keys to environment variables using the loader's viper instance. The automatic path normalization for path-type bindings (lines 941-946) is a useful feature.

---

746-765: LGTM: Viper setup correctly uses isolated instance.

Both setupViper and configureViper properly use the loader's isolated viper instance (l.v), ensuring configuration isolation per loader. The setup sequence is appropriate.

Also applies to: 953-965