Conversation
…t/namespace validation 1. toSnakeCase: handle consecutive uppercase (CPULoad → cpu_load) 2. mapParamsToFieldIndices: error on unknown params instead of silent skip 3. resolveTargetIndex: error on non-numeric targets instead of defaulting to 0 4. resolveNamespaceIndex: exact match only, remove loose bidirectional prefix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Addresses PR #56 review feedback by tightening and correcting FaultSpec (friendly YAML) → chaos.Node conversion behavior, especially around parameter name normalization, input validation, and namespace/target resolution.
Changes:
- Fix
toSnakeCaseto correctly handle consecutive uppercase runs (e.g.,CPULoad→cpu_load) and update related tests. - Make conversion fail fast on invalid inputs: unknown params now error, non-numeric targets now error, and namespace matching is now exact-only.
- Update
aegisctl injecthelp text to reflect numerictargetindices and add/adjust unit tests for new error cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/service/producer/spec_convert.go |
Fixes toSnakeCase, enforces stricter namespace/target rules, and errors on unknown params during reflection-based mapping. |
src/service/producer/spec_convert_test.go |
Updates/extends tests to cover consecutive-uppercase snake_case conversion and the new validation/error behaviors. |
src/cmd/aegisctl/cmd/inject.go |
Updates CLI help note to instruct users to use numeric container indices for target. |
| NOTE: --project is required for submit, list, and search commands. | ||
| It accepts project names (resolved to IDs automatically). | ||
| The 'target' field accepts container names (resolved to indices automatically). | ||
| The 'target' field accepts numeric container indices (use 'aegisctl inject metadata' to look up indices). | ||
| Duration accepts Go time strings: "60s", "5m", "1h", etc.`, |
There was a problem hiding this comment.
The CLI help example under SPEC FILE FORMAT still shows target: frontend (a name), but this PR makes the backend reject non-numeric targets and the NOTE now says target must be a numeric container index. To avoid users copy/pasting a now-invalid example, update the sample YAML to use a numeric target (e.g., target: "0") or explicitly explain when names are allowed (if any).
| seen := make(map[int]bool) | ||
| for name, fieldIdx := range nameToIdx { | ||
| if !seen[fieldIdx] { | ||
| seen[fieldIdx] = true | ||
| available = append(available, name) |
There was a problem hiding this comment.
The unknown-param error builds the available list by iterating over nameToIdx (a map containing multiple aliases per field: exact, lowercase, snake_case). Because Go map iteration order is randomized, this error message will be nondeterministic and may show an arbitrary alias for each field (e.g., cpuload instead of cpu_load), which is confusing for users and makes assertions/golden outputs flaky. Consider building the available list deterministically from the struct fields (e.g., loop fields 3+, append a canonical name like toSnakeCase(field.Name) or field.Name), and sort.Strings before formatting the error.
…istic error messages
1. Update CLI help example to use numeric target ("0") instead of "frontend"
since non-numeric targets are now rejected by the backend.
2. Build unknown-param available fields list deterministically from struct
field order using canonical snake_case names, sorted alphabetically.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Addresses the 4 REQUEST_CHANGES items from PR #56 review:
CPULoad→cpu_load(wasc_p_u_load)mapParamsToFieldIndicesnow returns an error with available field names instead of silently skipping unrecognized paramsresolveTargetIndexrejects non-numeric targets with a clear message pointing toaegisctl inject metadataHasPrefix; only exact match is accepted nowTest plan
go build -tags duckdb_arrowpassesgo test ./service/producer/... -v— 27/27 pass (3 new tests added)go test ./dto/... -v— 10/10 passTestFriendlySpecToNode_UnknownParamError,TestFriendlySpecToNode_NonNumericTargetError,TestFriendlySpecToNode_LooseNamespaceNoMatchTestToSnakeCase+TestToSnakeCase_ConsecutiveUppercaseassert correct snake_case for CPULoad/HTTPDelay/etcTestFriendlySpecToNode_SnakeCaseParamsnow confirmscpu_loadmatches (was a known failure)Closes #43
🤖 Generated with Claude Code