Repository navigation
feat: note on stderr before writes to resources commonly managed as code - #147
Conversation
Dashboards, alert policies and conditions, and synthetic monitors are often managed as code, so a direct change can be overwritten by the next deploy. dashboards and synthetics create/update/delete, and nerdgraph query mutations that write those resources, now print a one-time note to stderr before the write. Stdout and exit codes are unchanged. NEWRELIC_CLI_NO_AS_CODE_NOTICE=1 hides it.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 9fa858c225c6
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| documentation:docs | 0 |
| policies:conventions | 0 |
| harness-engineering:repo-health | 1 |
| structure:repo-health | 0 |
| architecture:solid-reviewer-agnostic | 2 |
| security:code-auditor | 0 |
go:implementation-tests (1 finding)
Minor - cmd/nrq/ascode_notice_test.go:217
The command-level tests leave several wired call sites uncovered:
dashboards update,synthetics updateandsynthetics delete. The only delete test (TestDashboardsDelete_NoteOnce) passes--force. The PR states that for deletes the note prints before the confirmation prompt, and no test checks that ordering. Movingascode.Notifybelow theif !opts.forceblock, or dropping it from one command, would still pass every test. Add a table-driven case per command: update and synthetics delete with the note asserted on stderr, plus a dashboards or synthetics delete without--forceand withopts.Stdinansweringn. In that case assert that stderr starts withascode.Message(...)before the prompt text and that no mutation request was sent.
harness-engineering:repo-health (1 finding)
Nits - internal/ascode/graphql.go:1
The detected NerdGraph mutation fields are a hand-maintained allowlist (dashboardMutations map, an alertsNrqlCondition prefix, a synthetics verb-prefix loop). New Relic adding a write field, for example a new synthetics monitor type, would silently produce no note. docs/development.md says to extend classifyMutationField for a new Kind but gives no pointer to where the field lists come from or how to refresh them. Add a one-line comment or doc note naming the NerdGraph schema as the source and the review step for new write fields, so the list does not drift unnoticed.
architecture:solid-reviewer-agnostic (2 findings)
Minor - internal/ascode/ascode.go:72
U-O1: the canonical Kind order is written out twice, here in Message and again in graphql.go:40 in MutationKinds, alongside the plural() switch. Adding the next managed-as-code kind (for example workflows or notification destinations) means editing three places, and a missed one silently drops the kind from either the note or the detection result. Suggested fix: declare one package-level
var allKinds = []Kind{Dashboard, AlertPolicy, AlertCondition, SyntheticMonitor}(or derive the order by iterating from Dashboard to a sentinelkindCount) and use it in both loops, so a new kind costs one constant plus its plural.
Nits - internal/ascode/ascode.go:51
U-D1: Notify reaches for os.Getenv through Disabled() rather than taking the setting as an input. This matches the repo's existing pattern (internal/config/config.go reads NEWRELIC_ACCOUNT_ID and NEWRELIC_REGION the same way, and the tests use t.Setenv), so I am not blocking on it. If more notices land, consider resolving the flag once into root.Options (next to Stderr) and passing it to Notify, which keeps the package free of process-environment reads.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (broad); scope: 8 files; examples:cmd/nrq/ascode_notice_test.go,internal/ascode/ascode.go,internal/ascode/ascode_test.go,internal/ascode/graphql.go,internal/cmd/dashboards/dashboards.go; 3 omitted; body-inspected: 8 files; examples:cmd/nrq/ascode_notice_test.go,internal/ascode/ascode.go,internal/ascode/ascode_test.go,internal/ascode/graphql.go,internal/cmd/dashboards/dashboards.go; 3 omitted; skipped: none; constraints: Reviewed ascode_test.go only through its first 80 lines. Tests were read but not executed.documentation:docs— complete (broad); scope: 3 files; examples:CHANGELOG.md,README.md,docs/development.md; body-inspected: 3 files; examples:CHANGELOG.md,README.md,docs/development.md; skipped: none; context (not coverage): 2 files; examples:internal/ascode/ascode.go,internal/ascode/graphql.go; constraints: Docs claims were checked against internal/ascode and the dashboards, synthetics and nerdgraph command code; tests were not run.policies:conventions— complete (broad); scope: 7 files; examples:CHANGELOG.md,README.md,docs/development.md,internal/cmd/dashboards/dashboards.go,internal/cmd/nerdgraph/nerdgraph.go; 2 omitted; body-inspected: 7 files; examples:CHANGELOG.md,README.md,docs/development.md,internal/cmd/dashboards/dashboards.go,internal/cmd/nerdgraph/nerdgraph.go; 2 omitted; skipped: none; context (not coverage): 1 file; examples:CLAUDE.md; constraints: Shared cli-common docs were not available locally; review relied on repo-local docs/development.md and CLAUDE.md conventions. internal/ascode and test files are outside the assigned scope and were not reviewed.harness-engineering:repo-health— complete (constrained); scope: 16 files; examples:cmd/nrq/ascode_notice_test.go,cmd/nrq/testdata/ascode/dashboard_created.json,cmd/nrq/testdata/ascode/dashboard_get.json,cmd/nrq/testdata/ascode/dashboard_input.json,cmd/nrq/testdata/ascode/nerdgraph_current_user.json; 11 omitted; body-inspected: 16 files; examples:cmd/nrq/ascode_notice_test.go,cmd/nrq/testdata/ascode/dashboard_created.json,cmd/nrq/testdata/ascode/dashboard_get.json,cmd/nrq/testdata/ascode/dashboard_input.json,cmd/nrq/testdata/ascode/nerdgraph_current_user.json; 11 omitted; skipped: none; context (not coverage): 1 file; examples:docs/development.md; constraints: Fixture JSON files and CHANGELOG/README hunks were not read line by line; the review covered the Go sources, docs/development.md and command wiring. Fixtures were read statically; the tests that consume them were not run. Read-only inspection of the checkout; tests were not run.structure:repo-health— complete (broad); scope: 7 files; examples:docs/development.md,internal/ascode/ascode.go,internal/ascode/graphql.go,internal/cmd/dashboards/dashboards.go,internal/cmd/nerdgraph/nerdgraph.go; 2 omitted; body-inspected: 7 files; examples:docs/development.md,internal/ascode/ascode.go,internal/ascode/graphql.go,internal/cmd/dashboards/dashboards.go,internal/cmd/nerdgraph/nerdgraph.go; 2 omitted; skipped: none; constraints: Did not read the full testdata fixtures or ascode_test.go bodies beyond test names. Static review only; tests were not run.architecture:solid-reviewer-agnostic— complete (broad); scope: 5 files; examples:internal/ascode/ascode.go,internal/ascode/graphql.go,internal/cmd/dashboards/dashboards.go,internal/cmd/nerdgraph/nerdgraph.go,internal/cmd/synthetics/synthetics.go; body-inspected: 5 files; examples:internal/ascode/ascode.go,internal/ascode/graphql.go,internal/cmd/dashboards/dashboards.go,internal/cmd/nerdgraph/nerdgraph.go,internal/cmd/synthetics/synthetics.go; skipped: none; context (not coverage): 4 files; examples:cmd/nrq/ascode_notice_test.go,internal/ascode/ascode_test.go,internal/cmd/root/root.go,internal/config/config.go; constraints: Ran go test for internal/ascode and cmd/nrq at head: both pass. Did not run the full make lint target.security:code-auditor— complete (broad); scope: 3 files; examples:internal/ascode/ascode.go,internal/ascode/graphql.go,internal/cmd/nerdgraph/nerdgraph.go; body-inspected: 3 files; examples:internal/ascode/ascode.go,internal/ascode/graphql.go,internal/cmd/nerdgraph/nerdgraph.go; skipped: none; constraints: Static review only; tests were not run and no fuzzing was done. The GraphQL scanner only drives an advisory stderr note and is not an authorization control, so a detection bypass has no security impact.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 6m 57s | ~$3.92 (est.) | claude-sonnet-5-5, claude-opus-5-5 | cr 0.10.342
| Field | Value |
|---|---|
| Model | claude-sonnet-5-5, claude-opus-5-5 |
| Reviewers | go:implementation-tests, documentation:docs, policies:conventions, harness-engineering:repo-health, structure:repo-health, architecture:solid-reviewer-agnostic, security:code-auditor |
| Engine | claude_cli · claude-sonnet-5-5, claude-opus-5-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 6m 57s wall · 18m 52s compute |
| Cost | ~$3.92 (est.) |
| Pricing basis | baseline-2026-10-02/pricing |
| Tokens | 98 in / 20.6k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5-5- In: 6
- Out: 2.5k
- Cache read: 60.5k
- Cache create: 53.8k
- Cost: ~$0.25 (est.)
- Duration: 52s
go:implementation-tests— claude-sonnet-5-5- In: 18
- Out: 3.6k
- Cache read: 463.1k
- Cache create: 165.6k
- Cost: ~$0.79 (est.)
- Duration: 3m 35s
documentation:docs— claude-sonnet-5-5- In: 12
- Out: 1.7k
- Cache read: 221.5k
- Cache create: 58.8k
- Cost: ~$0.30 (est.)
- Duration: 1m 36s
policies:conventions— claude-sonnet-5-5- In: 10
- Out: 1.4k
- Cache read: 170.7k
- Cache create: 59.6k
- Cost: ~$0.29 (est.)
- Duration: 1m 35s
harness-engineering:repo-health— claude-sonnet-5-5- In: 18
- Out: 3.8k
- Cache read: 389.2k
- Cache create: 152.0k
- Cost: ~$0.72 (est.)
- Duration: 3m 53s
structure:repo-health— claude-sonnet-5-5- In: 10
- Out: 1.9k
- Cache read: 184.5k
- Cache create: 67.3k
- Cost: ~$0.33 (est.)
- Duration: 2m 00s
architecture:solid-reviewer-agnostic— claude-opus-5-5- In: 10
- Out: 2.8k
- Cache read: 185.6k
- Cache create: 65.3k
- Cost: ~$0.62 (est.)
- Duration: 2m 41s
security:code-auditor— claude-sonnet-5-5- In: 8
- Out: 1.6k
- Cache read: 116.0k
- Cache create: 57.4k
- Cost: ~$0.27 (est.)
- Duration: 1m 42s
orchestrator-rollup— claude-sonnet-5-5- In: 6
- Out: 1.3k
- Cache read: 141.0k
- Cache create: 80.1k
- Cost: ~$0.36 (est.)
- Duration: 54s
Adds command cases for update and synthetics delete, a declined delete that shows the note before the prompt and sends no write, one canonical kind list, and a pointer to the NerdGraph schema as the source of the detected mutation fields.
Summary
Dashboards, alert policies and conditions, and synthetic monitors are often managed as code (Terraform, a repository or similar), so a direct change can be overwritten by the next deploy from that source. nrq now prints a one-time note to stderr just before it writes one of them, so a person or an agent driving the CLI checks for a source of truth first:
dashboards create|update|deleteandsynthetics create|update|delete(with their aliases), andnerdgraph query(ng,graphql) when the document is a mutation whose top-level fields write those resources. For deletes the note prints before the confirmation prompt.internal/ascode, a small built-in GraphQL lexer, no new dependency): top-level fields of mutation operations, through inline fragments and fragment spreads, with aliases resolved; comments, strings and nested fields are ignored. Fields:dashboardCreate,dashboardUpdate,dashboardUpdatePage,dashboardUpdateWidgetsInPage,dashboardAddWidgetsToPage,dashboardDelete,dashboardUndelete,alertsPolicyCreate|Update|Delete,alertsNrqlCondition*,alertsConditionDelete,synthetics{Create,Update,Delete}*Monitor,syntheticsUpdateScript. Reads and other mutations (snapshot URLs, tagging, muting rules, secure credentials) print nothing.NEWRELIC_CLI_NO_AS_CODE_NOTICE=1(ortrue), following the existingNEWRELIC_CLI_*names.Type of Change
Checklist
make test)make lint)Model: Claude Opus 5.5 (context unknown, high effort) in Claude Code. Subagents: Claude Opus 5.5 for the implementation and tests.