feat(cli): add consistent JSON output to project mutations - #2218
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice, well-scoped refactor. The new ProjectMutationResult contract via src/handlers/project/output.ts is consistent across create, add *, and remove *, and the addProjectResource helper in src/handlers/project/add/shared.ts cleanly collapses the duplicated for await loops. A few small observations, none blocking:
parentForinadd/shared.tsand the ternary chain inremove/index.ts(lines 135–142) encode the same parent-relationship knowledge in two places. If a third resource ever gains a parent, both must be updated. Consider extracting a singleparentFor(resourceType, flags)helper shared by both sides — optional cleanup, not required now.addProjectResourcehard-codesinteractive: falseforrunWithProgress. That preserves byte-for-byte behavior of the previousfor awaitloop, which is what the comment says is intended, so this is fine — just worth noting if you later wantaddcommands to get the same live TaskList UI ascreate/deploy.removealways emitsremovedEnvironmentKeys(including[]when nothing was removed), whileadd/createnever emit it. The contract is unambiguous, but you may want to document that shape somewhere consumer-facing (schema/docs) so tools can rely on it.
The tests use real temp-dir projects and go through run(...) end-to-end — no excessive mocking. The router showGlobalOptions: true change is small, covered by a new test, and orthogonal but reasonable to bundle here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor #2218 +/- ##
============================================
+ Coverage 97.02% 97.03% +0.01%
============================================
Files 564 566 +2
Lines 39233 39397 +164
============================================
+ Hits 38064 38228 +164
Misses 1169 1169 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude Security Review: no high-confidence findings. (run) |
775a74a to
61734e4
Compare
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Clean, well-scoped refactor. A few observations, all verified against the code and none blocking:
- The new
ProjectMutationResultcontract insrc/handlers/project/output.tsis applied uniformly acrosscreate, alladd *handlers, andremove(includingremove all). The parent-relationship logic lives in a singleparentForhelper, whichremoveuses viaprojectMutationResource(resource, name, input)— no duplicated ternary chain remains inremove/index.ts. add/shared.ts'saddProjectResourcecorrectly threads the returnedProjectback out ofrunWithProgress(matchingaddResource'sAsyncGenerator<ProjectEvent, Project>signature) and the hardcodedinteractive: falsepreserves the historical plain progress-line behavior ofaddcommands byte-for-byte; the inline comment calls this out.- The
gateway-connectorcase correctly re-types the resource to"gateway-connector"viaoptions.resourceTypewhile still deriving theparent: { type: "gateway", ... }from the underlyingresourceType: "gateway-target"input. New test covers this. - The
gateway-test-support.tschange fromjoin(directory, name)toprocess.cwd()normalizes symlinks (e.g., macOS/varvs/private/var), which is necessary for the newpathfield in the JSON output to match. Sensible fix. - Post-mutation stderr side-messages (
.env.localguidance for credentials, "attached to N gateway(s)" for policy-engine, auto-payment warning for payment-manager) intentionally remain on stderr and are not part of the machine-readable JSON; that's the right tradeoff and preserves prior behavior. - Secrets aren't leaked in the new
--jsoncredential result — verified by the new test assertingsk-secret-valuedoesn't appear in stdout.
Tests use real temp-dir projects and go through run(...) end-to-end; no excessive mocking. The unrelated showGlobalOptions: true router change is small, covered by a new test, and reasonable to include.
Prior review by the bot already noted the small readability suggestion about extracting parent-relationship knowledge into a shared helper — that's already effectively done via parentFor in output.ts. No further issues from me.
61734e4 to
67f5981
Compare
|
Claude Security Review: no high-confidence findings. (run) |
tejaskash
left a comment
There was a problem hiding this comment.
Verified locally on 67f5981: tsc, oxlint, and 668 tests under src/handlers/project and src/router pass. No behavior-breaking bugs. Inline comments below.
One note not tied to a line: the PR body says human-readable output is unchanged, but remove ... --json previously emitted { message: "removed ..." } and a stderr success line. It now emits the new envelope only and the stderr line is suppressed. That is a fine outcome, but the description should state it since this was the one pre-existing project mutation JSON contract.
67f5981 to
75b90a3
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Uses the existing global
JsonKey/JsonRendererKeypath with a shared, type-checked project mutation envelope for create, add, and remove. All project add leaves now use onerunWithProgresswrapper, nested help shows inherited global options, and command guidance is returned in structurednotesunder--json.Human-readable success and guidance output is preserved when
--jsonis not used.