Repository navigation
feat(incidents): add set-status command with bulk dismiss support - #38
Conversation
Signed-off-by: Ben <ben@armosec.io>
Signed-off-by: Ben <ben@armosec.io>
Signed-off-by: Ben <ben@armosec.io>
Signed-off-by: Ben <ben@armosec.io>
…heet Signed-off-by: Ben <ben@armosec.io>
Signed-off-by: Ben <ben@armosec.io>
slashben
added
the
ai-assisted
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/incidents/setstatus_test.go`:
- Line 117: The json.Unmarshal call in the test is ignoring its error return
value, which can hide unmarshaling failures and lead to subsequent assertions
operating on zero values. Check the error returned by
json.Unmarshal(stdout.Bytes(), &got) and properly handle it by failing the test
if unmarshaling fails, ensuring that any JSON parsing issues are caught and
reported rather than silently masked.
- Line 91: The json.Unmarshal call in this test is ignoring the error return
value by assigning it to underscore, which can mask test failures. Capture the
error return value from json.Unmarshal when unmarshaling stdout.Bytes() into the
got variable, and add a check to fail the test immediately if an error occurs
(using t.Fatalf or t.Errorf with an appropriate error message). This ensures
that any unmarshaling failures are caught and reported rather than silently
proceeding with zero values.
- Line 150: The error returned by the Decode method of json.NewDecoder is being
ignored using the blank identifier in the line with
json.NewDecoder(r.Body).Decode(&body). Instead of ignoring this error with an
underscore, capture the error return value and check if it is not nil, then
handle it appropriately (such as failing the test or logging an error). This
ensures that JSON parsing failures are detected and don't cause misleading test
assertions operating on zero values.
In `@docs/features/incidents-set-status.md`:
- Line 24: The documentation sentence about required selection methods is
incomplete and excludes `--stdin` as a valid option for supplying GUIDs. Update
the sentence in the incidents-set-status.md file to include `--stdin` alongside
GUIDs, `--filter`, and `--search` as valid standalone methods for specifying
incidents. This reflects the actual behavior in cmd/incidents/setstatus.go where
stdin GUIDs are merged into the selection set before validation occurs.
In `@docs/superpowers/plans/2026-06-22-incidents-set-status.md`:
- Around line 680-682: Add language tags to all fenced code blocks in the
markdown file to comply with the MD040 linting rule. The code fence at lines
680-682 containing the armoctl incidents set-status command example should be
labeled with the bash tag since it represents a command-line interface command.
Apply the same fix to all other unlabeled fenced code blocks referenced in the
comment (lines 696-706 and 715-717), using appropriate language tags (bash for
CLI commands, text for plain text examples, markdown for markdown examples)
based on the content of each block.
In `@docs/superpowers/specs/2026-06-22-incidents-set-status-design.md`:
- Around line 44-47: Add language tags to all fenced code blocks in the markdown
spec to comply with markdownlint rule MD040. For the command examples in the
fenced blocks at lines 44-47, 66-72, and 104-112, add the appropriate language
identifier (bash for CLI commands, text for plain text examples, json for JSON
examples) immediately after the opening backticks of each code fence. This will
ensure the spec passes linting checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2971458-9643-4aa2-bb4b-4319e46d0bc8
📒 Files selected for processing (11)
cmd/incidents/changestatus.gocmd/incidents/changestatus_test.gocmd/incidents/incidents.gocmd/incidents/resolve.gocmd/incidents/setstatus.gocmd/incidents/setstatus_test.gocmd/incidents/skill.gocmd/incidents/types.godocs/features/incidents-set-status.mddocs/superpowers/plans/2026-06-22-incidents-set-status.mddocs/superpowers/specs/2026-06-22-incidents-set-status-design.md
| t.Fatal(err) | ||
| } | ||
| var got map[string]any | ||
| _ = json.Unmarshal(stdout.Bytes(), &got) |
There was a problem hiding this comment.
Check the error from json.Unmarshal.
Ignoring the unmarshal error could mask test failures. If unmarshaling fails, subsequent type assertions will silently use zero values, potentially causing confusing or misleading test failures.
🛡️ Proposed fix
var got map[string]any
-_ = json.Unmarshal(stdout.Bytes(), &got)
+if err := json.Unmarshal(stdout.Bytes(), &got); err != nil {
+ t.Fatalf("failed to unmarshal output: %v", err)
+}
req, _ := got["request"].(map[string]any)📝 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.
| _ = json.Unmarshal(stdout.Bytes(), &got) | |
| var got map[string]any | |
| if err := json.Unmarshal(stdout.Bytes(), &got); err != nil { | |
| t.Fatalf("failed to unmarshal output: %v", err) | |
| } | |
| req, _ := got["request"].(map[string]any) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/incidents/setstatus_test.go` at line 91, The json.Unmarshal call in this
test is ignoring the error return value by assigning it to underscore, which can
mask test failures. Capture the error return value from json.Unmarshal when
unmarshaling stdout.Bytes() into the got variable, and add a check to fail the
test immediately if an error occurs (using t.Fatalf or t.Errorf with an
appropriate error message). This ensures that any unmarshaling failures are
caught and reported rather than silently proceeding with zero values.
| t.Fatal(err) | ||
| } | ||
| var got map[string]any | ||
| _ = json.Unmarshal(stdout.Bytes(), &got) |
There was a problem hiding this comment.
Check the error from json.Unmarshal.
Ignoring the unmarshal error could mask test failures. If unmarshaling fails, the subsequent type assertions and checks will operate on zero values, potentially causing confusing test failures.
🛡️ Proposed fix
var got map[string]any
-_ = json.Unmarshal(stdout.Bytes(), &got)
+if err := json.Unmarshal(stdout.Bytes(), &got); err != nil {
+ t.Fatalf("failed to unmarshal output: %v", err)
+}
req, _ := got["request"].(map[string]any)📝 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.
| _ = json.Unmarshal(stdout.Bytes(), &got) | |
| var got map[string]any | |
| if err := json.Unmarshal(stdout.Bytes(), &got); err != nil { | |
| t.Fatalf("failed to unmarshal output: %v", err) | |
| } | |
| req, _ := got["request"].(map[string]any) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/incidents/setstatus_test.go` at line 117, The json.Unmarshal call in the
test is ignoring its error return value, which can hide unmarshaling failures
and lead to subsequent assertions operating on zero values. Check the error
returned by json.Unmarshal(stdout.Bytes(), &got) and properly handle it by
failing the test if unmarshaling fails, ensuring that any JSON parsing issues
are caught and reported rather than silently masked.
| t.Errorf("path: %s", r.URL.Path) | ||
| } | ||
| var body map[string]any | ||
| _ = json.NewDecoder(r.Body).Decode(&body) |
There was a problem hiding this comment.
Check the error from json.NewDecoder.Decode.
Ignoring the decode error could mask issues with the request body format. If the body is not valid JSON or doesn't decode properly, the subsequent assertions will operate on zero values, potentially causing misleading test results.
🛡️ Proposed fix
var body map[string]any
-_ = json.NewDecoder(r.Body).Decode(&body)
+if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
+ t.Errorf("failed to decode request body: %v", err)
+ w.WriteHeader(400)
+ return
+}
if body["status"] != "Dismissed" {📝 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.
| _ = json.NewDecoder(r.Body).Decode(&body) | |
| var body map[string]any | |
| if err := json.NewDecoder(r.Body).Decode(&body); err != nil { | |
| t.Errorf("failed to decode request body: %v", err) | |
| w.WriteHeader(400) | |
| return | |
| } | |
| if body["status"] != "Dismissed" { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/incidents/setstatus_test.go` at line 150, The error returned by the
Decode method of json.NewDecoder is being ignored using the blank identifier in
the line with json.NewDecoder(r.Body).Decode(&body). Instead of ignoring this
error with an underscore, capture the error return value and check if it is not
nil, then handle it appropriately (such as failing the test or logging an
error). This ensures that JSON parsing failures are detected and don't cause
misleading test assertions operating on zero values.
| | `--dry-run` | Preview the request without sending it. | | ||
| | `--yes` | Confirm the mutation in non-interactive mode. | | ||
|
|
||
| At least one of GUIDs, `--filter`, or `--search` is required. |
There was a problem hiding this comment.
Clarify the selection requirement.
The current sentence omits --stdin, which is also a valid standalone way to supply GUIDs. Tightening this wording will avoid telling users that piped bulk-dismiss flows are unsupported.
As per the cmd/incidents/setstatus.go:15-63 snippet, stdin GUIDs are merged into the selection set before validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/features/incidents-set-status.md` at line 24, The documentation sentence
about required selection methods is incomplete and excludes `--stdin` as a valid
option for supplying GUIDs. Update the sentence in the incidents-set-status.md
file to include `--stdin` alongside GUIDs, `--filter`, and `--search` as valid
standalone methods for specifying incidents. This reflects the actual behavior
in cmd/incidents/setstatus.go where stdin GUIDs are merged into the selection
set before validation occurs.
| ``` | ||
| armoctl incidents set-status [guid...] --status <Open|Investigating|Dismissed|Resolved> [flags] | ||
| ``` |
There was a problem hiding this comment.
Add language tags to the fenced examples.
Markdownlint already flags the unlabeled fences in this plan (MD040). Label them text/bash/markdown as appropriate so the plan stays lint-clean.
Also applies to: 696-706, 715-717
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 682-682: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-06-22-incidents-set-status.md` around lines 680 -
682, Add language tags to all fenced code blocks in the markdown file to comply
with the MD040 linting rule. The code fence at lines 680-682 containing the
armoctl incidents set-status command example should be labeled with the bash tag
since it represents a command-line interface command. Apply the same fix to all
other unlabeled fenced code blocks referenced in the comment (lines 696-706 and
715-717), using appropriate language tags (bash for CLI commands, text for plain
text examples, markdown for markdown examples) based on the content of each
block.
Source: Linters/SAST tools
| ``` | ||
| armoctl incidents set-status [guid...] --status <Open|Investigating|Dismissed|Resolved> [flags] | ||
| armoctl incidents resolve <guid> [--false-positive] # unchanged; now an alias | ||
| ``` |
There was a problem hiding this comment.
Add language tags to the fenced examples.
Markdownlint already flags the unlabeled fences in this doc (MD040). Label them text/bash/json as appropriate so the spec stays lint-clean.
Also applies to: 66-72, 104-112
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/specs/2026-06-22-incidents-set-status-design.md` around
lines 44 - 47, Add language tags to all fenced code blocks in the markdown spec
to comply with markdownlint rule MD040. For the command examples in the fenced
blocks at lines 44-47, 66-72, and 104-112, add the appropriate language
identifier (bash for CLI commands, text for plain text examples, json for JSON
examples) immediately after the opening backticks of each code fence. This will
ensure the spec passes linting checks.
Source: Linters/SAST tools
There was a problem hiding this comment.
Pull request overview
Adds a new armoctl incidents set-status CLI command to change incident status to any backend-supported state (including bulk dismiss), while refactoring the existing resolve command to reuse a shared change-status helper and preserving current resolve behavior.
Changes:
- Introduces shared
runStatusChangehelper (request building +safety.Wrapmutation contract) and refactorsresolveto call it. - Adds
set-statuscommand supporting bulk selection via positional GUIDs,--stdin,--filter, and--search(query param). - Updates incidents skill metadata, cheatsheet text, and adds feature/spec/plan documentation.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/superpowers/specs/2026-06-22-incidents-set-status-design.md | New design spec for incidents set-status behavior and architecture. |
| docs/superpowers/plans/2026-06-22-incidents-set-status.md | New implementation plan outlining tasks, code shape, and tests. |
| docs/features/incidents-set-status.md | New user-facing feature documentation for incidents set-status. |
| cmd/incidents/changestatus.go | New shared helper: status normalization + request building + mutation execution via safety.Wrap. |
| cmd/incidents/changestatus_test.go | Unit tests for status normalization behavior. |
| cmd/incidents/setstatus.go | New set-status command implementation (flags parsing, stdin GUID ingestion, filters parsing). |
| cmd/incidents/setstatus_test.go | Tests covering set-status dry-run preview, filters, search query, stdin, invalid inputs, and --yes posting. |
| cmd/incidents/resolve.go | Refactors resolve to be a thin wrapper around the shared helper. |
| cmd/incidents/incidents.go | Registers the new set-status subcommand under incidents. |
| cmd/incidents/skill.go | Updates incident status notes and adds set-status recipes. |
| cmd/incidents/types.go | Updates cheatsheet text to list all four canonical statuses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ixes - regenerate skills/armoctl-incidents/SKILL.md for the new set-status command (CI verify gate) - check json decode/unmarshal errors in set-status tests - note --stdin as a selection method in the feature doc - add language tags to fenced code blocks in spec/plan (MD040) Signed-off-by: Ben <ben@armosec.io>
No description provided.