Repository navigation
fix: warn that --format does not change files written by agents-as-code commands - #176
danieldutu wants to merge 2 commits into
Conversation
PaulAsjes
left a comment
PaulAsjes
left a comment
There was a problem hiding this comment.
Thanks, but I think we can make a few optimizations here.
| pub fn warn_format_ignored(matches: &clap::ArgMatches, command: &str) { | ||
| if format_flag(matches).is_some() { | ||
| eprintln!( | ||
| "--format controls how API responses are printed and has no effect on {command}; agent configs are always written as JSON." |
There was a problem hiding this comment.
This text will be wrong for anything that isn't an agent config (e.g. tools, tests, push command). The caller should pass in the noun and the verb.
|
|
||
| fn handle_push(matches: &clap::ArgMatches, ctx: &AppContext) -> Result<(), CliError> { | ||
| let _scope = api::command_scope("agents.push"); | ||
| warn_format_ignored(matches, "agents push"); |
There was a problem hiding this comment.
Instead of fixing this at the handler level (and having to remember to add this to any future handlers) we should fix it at the source in CliCommand that makes CliApp dispatch the warning.
| } | ||
|
|
||
| #[test] | ||
| fn format_flag_reads_the_flag_before_the_subcommand() { |
There was a problem hiding this comment.
The tests don't exercise the real path. They use a made-up clap tree and never call warn_format_ignored or the real CliApp command tree. A change to how globals are attached in src/app.rs:1225-1260 would break the warning while both tests still pass.
…ispatch
The first version called warn_format_ignored inside six push/pull handlers.
Every future handler had to remember the call. The message also said "agent
configs" for tools and tests. Its two unit tests ran on a hand-built clap tree,
so they never touched the real global flag.
CliApp::ignores_format(path, noun, verb) now records the declaration on the
registered CliCommand. Dispatch prints the warning once on stderr before it
calls the handler, and only when --format was given. The agents, tools and
tests push/pull commands declare it where they register. Each declaration
gives its own noun ("agent configs", "tool configs", "test configs") and verb
("read" for push, "written" for pull). I removed format_flag,
warn_format_ignored and their tests from util.rs.
tests/format_flag_test.rs runs the built binary on five argv cases. Removing
.global(true) from the --format arg in src/app.rs makes 4 of the 5 fail.
|
Thanks for the review. I pushed a new commit on top of the first one.
Not covered: |
elevenlabs agents pull --format yamlexited 0 and wrote JSON files. Nothing said that--formathad no effect. The same is true foragents push,tools push/pullandtests push/pull.--formatis declared withglobal(true)on the root command insrc/app.rs, so clap accepts it on every subcommand. These six commands read or write local JSON configs and never pass a response to the formatter.Changes:
CliApp::ignores_format(path, noun, verb)(src/app.rs) records on the registeredCliCommandthat the command ignores--format.CliAppprints one line on stderr before it calls the handler, and only when--formatwas given:--format controls how API responses are printed and has no effect on <command>; <noun> are always <verb> as JSON.tools pushwarns that "tool configs are always read as JSON", andagents pullwarns that "agent configs are always written as JSON". A future command gets the warning from one builder call.--formathelp text and the README now say that the flag styles printed API responses only.tests/format_flag_test.rsis listed in.fernignorewith the other hand-written tests, so a regeneration keeps it.Evidence:
tests/format_flag_test.rsruns the built binary withELEVENLABS_BASE_URLpointed at a closed localhost port, so no request leaves the machine. It coversagents pull --format yaml,--format yaml agents pull,tools push --format json,tests pull --format table, andagents pullwithout the flag. It asserts the exact warning text in the first four cases and no warning in the fifth..global(true)from the--formatarg as a check. 4 of the 5 tests then failed.cargo test,cargo test --manifest-path elevenlabs-types/Cargo.tomlandcargo test --manifest-path elevenlabs-sdk/Cargo.tomlpass with 0 failures.Boundaries: the change does not alter what
--formatdoes, and it does not reject the flag. A hard error would break scripts that already pass--formatto every command.agents init/add/statusare typed commands. They can use the same declaration, but I did not add it. I tested on macOS only.Fixes #171.