Pass addon arguments through verbatim, as documented

[kfkonrad]

Fixes #2696: pass addon arguments through verbatim, as documented.

As we discussed in chat and in #2696 I split the changes into four commits in the following order:

1. The required-value flag pre-check ran against the union of every subcommand's flags and scanned past --, so hledger foo --sort and hledger help --sort failed with "--sort needs a value". It is now given only the flags relevant to the identified command, and stops at the first --.
2. filter (/="--") removed every -- from the addon's arguments. Only the escaping one is consumed now; later ones are passed on.
3. -n/--no-conf/--conf VAL were stripped from the whole addon argument list. They are now stripped only before the first --, so hledger foo -- -n delivers -n. Without a -- they keep their current meaning as hledger's own options; the manual says so now.
4. Addons are run with rawSystem rather than a shell command line, so empty and shell-significant arguments survive.

Tests: A total of 12 new cases in hledger/test/cli/addons.test and cli.test, using a new hledger-argvdump dummy addon (as demoed in #2696). All tests passed on my machine for each commit.

Note that commit 4 only fixes the issue on unix-like systems. For Windows we still shell out to cmd, which presumably swallows empty arguments (""). I can try to verify this on my Windows machine soon. I'm mostly using a Mac or Linux these days. So no regression on Windows but also presumably no fix. If I can verify that behavior I'll work around that too in a 5th commit.

I also noticed that hledger run may be subject to similar bugs regarding empty or shell-significant arguments (like "'"). I may file a separate issue if I can confirm that so that heldger and hledger run behave the same when using addons. As I don't use hledger run myself I'm not sure if changing its argument parsing or dispatch might brake existing use cases.

AI usage: written with some help from Claude Opus, from a plan created with Claude Fable. Some finer adjustments in the plan were my doing and I made sure to test and review each commit thoroughly. Usage in output token is denoted on each commit. btw: I chose to keep with the existing pattern of only documenting use of one model per commit, which is why the plan's AI usage is included in the first commit and the implementation is divvied up among the rest.

[simonmichael]

Thanks @kfkonrad.

This seems not to satisfy the first-time contributor rule in the https://hledger.org/PULLREQUESTS.html I mentioned. That rule was added to screen out drive-by and policy-ignoring contributors. You're clearly not one of those (right ? :) so I might change this rule.

On the other hand, complicating the rule and requiring maintainer judgement every time defeats the purpose a little bit. Is there any other small PR you'd like to submit manually to become a non-first-time contributor ?

[simonmichael]

Also, the rule's wording has some ambiguity.

First-time hledger contributors must not use AI to generate PRs. New-contributor PRs that seem AI-generated will be closed.

[simonmichael]

The changes look reasonable to my eye.. though when I've done similar fixups like adding a -- it usually breaks something that I find eventually, maybe years later.

It would be good to test this also in repl and run, at least manually.

[kfkonrad]

I'm not a first time contributor, I hand-wrote this PR (and noted how I used AI in it, which was for a local review only): #2679

I can see if I can find another small PR to file manually if you'd like to see another one before getting partially AI-generated PRs from me. Ultimately the judgement is yours, regardless of what exactly the policy states

[kfkonrad]

I'll do some additional manual testing either way. If I find any interesting test cases I'll add them to the test cases in code too

[simonmichael]

Ah, sorry! I wonder why github is saying so. never mind then

[simonmichael]

Somehow GH didn't register your #2679 and is still showing the first-time badge.. but this one will probably fix it. Please excuse my short memory :)

I'll do some here testing too. I got this earlier - you may have seen similar but I'm sharing it in case it's useful. A few of these look worth fixing up, or if you prefer I could do it. https://claude.ai/code/session_019zkbChUyusi3ndEy45t8ar

[simonmichael]

I see we were literally chatting yesterday. Now I have the nicks wired up.

These changes look good. I'll give it a little time for us to use it and possible polish.

[acinader]

I hope you two don't mind if I crash the party, but this was an interesting looking problem that gave me a good excuse to learn our ci system and the various ways to invoke hledger and pass it arguments.

Summary: I opened a pull request against this branch to fix the -- processing. I had one other finding that I think we shouldn't fix. I opened a few unrelated pr's as a result of what I observed while working through this.

Sequencing would be for @kfkonrad to merge kfkonrad#1 before merging this pr into main.

PR #2699 review — hledger addon argument passing (#2696)

Method: built upstream/main (6c718b9d8) and PR head (6bd6c66ba), compared behavior with a dummy hledger-argvdump addon.

Action items

1. A shebang-less addon script no longer runs on linux — silent failure. Reporting as a non-blocker in my opinion.
2. The -- in a config file section or a command alias — finding 1 of Simon's review session. I implemented the fix sketched there and it leaks hledger's own options to the addon; splitting per source instead does not. PR with tests ready, details below.

I reviewed Simon's claude session log. Everything else I've done her is either verification that the PR does what it says, or defects that predate it and are handled elsewhere (see below).

Reported cases

case	main	PR #2699
argvdump --sort	Error: --sort needs a value	argv: [--sort]
argvdump -- --sort	Error: --sort needs a value	argv: [--sort]
argvdump --sort ""	argv: [--sort]	argv: [--sort] []
argvdump -n	argv:	argv:
argvdump -- -n	argv:	argv: [-n]
argvdump -- foo -- bar	argv: [foo] [bar]	argv: [foo] [--] [bar]
help --sort	Error: --sort needs a value	Error: Unknown flag: --sort

repl and run (Per Simon's request)

Neither is changed by this PR — both dispatch through Run.hs, which 6bd6c66ba does not touch.

• repl: feeding zzzz a "" b, zzzz -- -n, and a shebang-less addon gives identical results on main and on this branch.
• run: shares that dispatch, so likewise unchanged. It still goes through the shell, which is why the Action 1 shebang-less regression is confined to the direct hledger ADDON path.

run and repl still lose empty arguments on every platform, where the CLI path now does not — see "Empty arguments" below and #2711.

Bonus bug fix!

hledger ui "Trader Joe's"	result
main	argument not delivered; the command fails
this PR	runs

Covered with test (in pr I opened against @kfkonrad's branch see Action 2 section below):

# ** 17. An argument containing an apostrophe is preserved.
$  PATH=$PATH:addons hledger argvdump "Sam's Diner"
argv: [Sam's Diner]

Action 1 — a shebang-less addon script no longer runs on linux

6bd6c66ba replaces system with rawSystem on every platform except Windows. A script with no #! line no longer runs.

	main	this PR
macOS	runs	runs
linux	runs	produces nothing

Confirmed on ubuntu-24.04 in a real CI run of this branch, with the addon tests enabled: cases 1-13 [OK], the shebang-less case [Failed] with empty stdout and empty stderr — https://github.com/acinader/hledger/actions/runs/33284617328

It fails silently, so the command appears to do nothing at all. The same mechanism, probed directly:

darwin        ExitSuccess
ubuntu-24.04  FAILED: posix_spawnp: invalid argument (Exec format error)

run and repl are not affected — they still dispatch through the shell, so only the direct hledger ADDON path breaks.

Shebang-less scripts are unusual but legal. Probably better to not fix, see if it causes anyone an issue, and then try to better understand why this behavior is needed.

Action 2 — the -- in a config file section or alias (session finding 1)

I tried the fix sketched in Simon's session: break the whole concatenation once, then dropCliSpecificOpts the part before the separator, which is not a complete solution. The sources are joined as conf <> alias <> cli, so a -- from a config section or alias falls before the command line args and moves them all into the "after" partition, where hledger's own options are deliberately not stripped:

[argvdump] section containing "alpha -- beta", run as: hledger --conf CONF argvdump gamma

main            argv: [alpha] [beta] [gamma]
this PR         argv: [alpha] [--] [beta] [gamma]
sketched        argv: [alpha] [beta] [--conf] [CONF] [gamma]     <- leaks --conf
per-source      argv: [alpha] [beta] [gamma]

Each source has to consume its own first --:

consumeSeparator as = let (bs, cs) = breakAtFirstSeparator as in dropCliSpecificOpts bs <> cs
addonargs = concatMap consumeSeparator
              [supportedgenargsfromconf, confcmdargs0, aliasargs, cliargswithoutcmd]

Simon observed that -- -n works only from the command line. With a per-source separator it works from a config section and an alias too.

I opened a PR against Konrad's branch with the fix and four test cases — three for the separator, one for the apostrophe above: kfkonrad#1

Bonus: unrelated bugs found along the way

The new addon tests do not run in CI. -x /addons prevents 10 of the 13 new cases from running. just functest does run them locally. The exclusion is vestigial: 1afe5fb (2017) added it when the dummy addons were gitignored and never turned back on. #2710 turns the addon tests back on.

Empty arguments are lost wherever a shell command line is built. see #2711 for details

AI usage: Claude Opus 5, ~50k output tokens