Skip to content

fix(aws): honor explicit --output json/yaml losslessly - #2140

Closed
maxmilian wants to merge 2 commits into
rtk-ai:developfrom
maxmilian:fix/aws/honor-explicit-output-json
Closed

maxmilian wants to merge 2 commits into
rtk-ai:developfrom
maxmilian:fix/aws/honor-explicit-output-json

Conversation

@maxmilian

Copy link
Copy Markdown
Contributor

Summary

  • Fixes aws lambda commands: rtk corrupts --output json (returns non-JSON schema skeleton, drops field values) #2139: rtk aws ... --output json returned a lossy schema skeleton (string, int, string[64]) or a one-line table summary instead of valid JSON, dropping field values (ImageUri, CodeSha256, env vars) and breaking downstream jq / json.load.
  • Root cause: rtk applied its summarizers (filter_lambda_get) or generic schema compression (filter_json_string) regardless of the user-requested output format.
  • Fix: when the user explicitly requests a structured, parseable format (--output json / --output=json / yaml / yaml-stream), run() short-circuits to a new byte-faithful run_passthrough. Lossy/human formats (table, text) and the no-flag default keep the existing compression behavior.

This covers all aws subcommands, not just lambda.

Test plan

  • cargo fmt --all clean
  • cargo clippy --all-targets — zero warnings
  • cargo test --all — 1947 passed
  • New unit test test_explicit_lossless_format covers json/yaml/=json positives and table/text/absent negatives

🤖 Generated with Claude Code

@maxmilian

Copy link
Copy Markdown
Contributor Author

Automated triage on #2139 (by wshm) independently confirms this: categorized as bug, priority high, 93% confidence, with the recommended fix being to "bypass summarization when an explicit output format flag is present" — which is exactly what this PR does.

This PR narrows that slightly: it only bypasses for parseable formats (json / yaml / yaml-stream) where losslessness is a contract, while keeping the existing compression for the human-readable table / text formats and the no-flag default — so token savings are preserved everywhere they don't break parsing.

@maxmilian

Copy link
Copy Markdown
Contributor Author

Note for the PR Testing Checklist: the "token savings ≥60%" item is N/A here. This isn't a new/compressing filter — it's a correctness fix that bypasses compression when the user explicitly requests --output json/yaml. It directly applies design principle Correctness vs Token Savings: a flag that explicitly asks for parseable, lossless output must not be reformatted into a lossy summary/skeleton.

The remaining checklist items are covered:

  • Unit tests added (test_explicit_lossless_format, incl. repeated/dangling --output)
  • cargo fmt --all --check && cargo clippy --all-targets && cargo test — all green (1947 passed)
  • Manual rtk aws ... --output json verified to pass through byte-for-byte

On security: the new run_passthrough forwards the user's args to the aws binary via Command::arg (no shell string interpolation), so it doesn't widen any injection surface versus the existing runners.

@dessite

dessite commented May 28, 2026

Copy link
Copy Markdown
Contributor

I believe I tackled this problem a bit differently a few hours earlier #2135 ;) rtk adds --output json on it's own if no --output defined afair

@maxmilian

Copy link
Copy Markdown
Contributor Author

Thanks @dessite — I looked at #2135 and I don't think they collide; they fix different layers and are actually complementary.

#2135 swaps filter_json_string → filter_json_compact in run_generic, so the compressed output keeps real values instead of string/int placeholders. That's a real improvement for the default token-saving path. But by design the output is still rtk's compact form — unquoted keys, depth cap, array summarization (your own "After" shows GlobalSettings: with no quotes) — so it's still not parseable JSON; jq / json.load would still choke on it.

#2140 (this PR) targets the --output json|yaml contract (#2139): when the user explicitly asks for a structured, parseable format, it short-circuits to a byte-faithful passthrough — the real aws output, untouched — so downstream parsing works. The default/no-flag path and table/text keep the existing compression (i.e. they keep your #2135 improvement).

Two notes specific to #2139:

On "rtk adds --output json on its own if none is defined" — agreed, and that auto-added path stays on run_generic (your territory). explicit_lossless_format only inspects the args the user actually passed, so rtk's auto-injected json isn't treated as explicit and the two don't overlap.

So I think both can land: #2135 makes the default compressed output value-preserving, #2140 honors an explicit format request losslessly. Happy to defer to maintainers on sequencing though.

maxmilian and others added 2 commits June 8, 2026 12:21
rtk forced its summarizers/schema compression on aws commands even when
the user explicitly requested --output json (or yaml), dropping field
values and emitting non-parseable output. Route explicitly-requested
lossless formats through an unmodified passthrough so jq/json.load keep
working (issue rtk-ai#2139).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- explicit_lossless_format: last --output wins (aws CLI semantics), and a
  dangling --output (no value) no longer falls through to lossy filtering
- run_generic: detect the --output=<fmt> form so it doesn't inject a
  second --output json on top of an explicit human format
- run_passthrough: document the intentional tee skip
- tests: cover repeated and dangling --output

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@maxmilian
maxmilian force-pushed the fix/aws/honor-explicit-output-json branch from e79670e to 47dad55 Compare June 8, 2026 04:23
@maxmilian

Copy link
Copy Markdown
Contributor Author

Rebased onto latest develop and resolved the conflict with #2135 (now merged as ad2bfd3).

The two changes sit at different layers and compose cleanly:

Conflict was a one-line doc comment on run_generic; kept #2135's wording ("compact (values preserved)") and layered the explicit-lossless functions ahead of it.

Validation on the rebased branch: cargo fmt --check clean, cargo clippy --all-targets clean, cargo test 2073 passed / 0 failed (incl. #2135's test_aws_unsupported_subcommand_json_preserves_values and this PR's test_explicit_lossless_format).

@maxmilian

Copy link
Copy Markdown
Contributor Author

Still needed on rtk 0.42.4 — the --output json contract is only half-fixed.

I retested on rtk 0.42.4 (which ships the #2376 fix) using a stub aws that returns a known get-function-configuration document:

rtk aws lambda get-function-configuration --function-name watermark-processor --output json

Output:

{
  Code:
  {
    ImageUri: "...dkr.ecr.../wm:v7"
    ResolvedImageUri: "...@sha256:deadbeef"
  }
  CodeSha256: "abc123DEF456...VWX="
  CodeSize: 20480123
  Environment:
  {
    Variables:
    {
      LOG_LEVEL: "INFO"
      S3_BUCKET_NAME: "commeet-watermark-prod"
    }
  }
  FunctionName: "watermark-processor"
  ...
}

Fixed by #2376: scalar values are now preserved — no more string / int / string[64] / ? type placeholders. ✅

Still broken on 0.42.4: the output is not valid JSON — keys are unquoted and there are no commas/separators — so --output json still breaks downstream jq / json.load, which was the other half of #2139.

This PR's approach (short-circuit to a byte-faithful passthrough when the user explicitly asks for --output json / yaml) fixes exactly that remaining half. So #2140 is not superseded by the 0.42.4 changes — it's still required. Could a maintainer take a look? Happy to rebase if mergeStateStatus: BLOCKED is just a stale base.

@maxmilian

Copy link
Copy Markdown
Contributor Author

Friendly ping — #2139 is still open. To be clear this doesn't overlap with the merged #2135: that one keeps output compact (but not valid JSON), whereas this honors an explicit --output json/yaml with byte-faithful passthrough. I re-verified on 0.42.4 that the explicit-format path is still lossy without this. Mergeable — happy to get a review whenever.

@maxmilian

Copy link
Copy Markdown
Contributor Author

Re-verified this against develop @ f9d8c77. The bug is unchanged and #2139 is still open, so posting the current evidence rather than just bumping.

This does not overlap with the merged #2135. That's the most likely reason to skip this PR, so concretely:

  1. lambda get-function never reaches the code fix(aws): preserve values in JSON output for unsupported subcommands #2135 changed. Dispatch at src/cmds/cloud/aws_cmd.rs:121 matches on the subcommand alone and routes it to run_aws_filtered. fix(aws): preserve values in JSON output for unsupported subcommands #2135 (ad2bfd3) changed one line inside run_generic — the path for subcommands not on the specialized list. Its own PR body scopes it that way ("unsupported subcommands / NOT on the hardcoded specialized list").
  2. rtk discards the user's stated intent before anything else runs. run_aws_json() at src/cmds/cloud/aws_cmd.rs:293-309 strips --output and its value, and --output=…, then appends its own --output json. Nothing downstream can tell that the user asked for a format explicitly.
  3. The result isn't JSON at all. filter_lambda_get (src/cmds/cloud/aws_cmd.rs:857-899) formats the response into a single space-separated line. jq and json.load fail outright — worse than the "lossy schema skeleton" aws lambda commands: rtk corrupts --output json (returns non-JSON schema skeleton, drops field values) #2139 describes.

The never_worse guard from #2551 doesn't rescue this either: it compares token counts only, and the one-line summary always wins against a full Lambda response, so the truncated form is what the user gets every time.

On the security note — I want to raise this rather than wait for it. filter_lambda_get:871 carries // SECURITY: Environment and Code.Location intentionally NOT read, and that's clearly deliberate. This PR doesn't overturn it: default behavior is unchanged, and the full response is returned only when the user explicitly passes --output json, i.e. when they've asked for data they already have IAM permission to read. If you'd rather keep env vars redacted even then, I'm happy to add a redaction pass — just say which fields.

This isn't a new pattern, it's an existing one aws is missing. k8s_get_requests_raw_output at src/cmds/cloud/container.rs:800-807 already does exactly this for kubectl: an explicit -o/--output sends the command down raw passthrough. This PR brings aws in line with that.

Happy to rebase if it's drifted.

@maxmilian

Copy link
Copy Markdown
Contributor Author

Closing this to reduce my own open-PR footprint here, not because the change is wrong.

I opened seven PRs across 26–28 May, which was too many at once from someone with no merge history in this repo — and this one has had no review activity in the two months since. I'd rather leave a short, meaningful queue than a wall of stale PRs on your review list, so I'm keeping only #2105 (approved, single file) and #2142 (backed by the user report in #1830).

The branch stays on my fork and this is trivially reopenable if it's ever wanted — no work is lost.

@maxmilian maxmilian closed this Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

aws lambda commands: rtk corrupts --output json (returns non-JSON schema skeleton, drops field values)

2 participants