Repository navigation
Conversation
d2e7caf to
62e7c26
Compare
|
Rebased onto the latest No review comments on this PR. CI will verify the build and test gate. |
62e7c26 to
3d2a850
Compare
🔐 Codex Security Review
|
|
Rebased onto the latest Still-needed assessment: No upstream commits in the 62-commit window address the same issue. This PR remains needed. Review requests: |
3d2a850 to
c790e6f
Compare
|
Rebased onto current Still needed: Checked all 38 new upstream commits for overlap with this PR's topic. None of the upstream changes address the issue this PR fixes. The PR remains relevant. Verification:
|
c790e6f to
bda6f80
Compare
Rebase and review — 2026-09-03Base commit: Accuracy review: Comma-containing arguments still break the legacy |
bda6f80 to
dce466f
Compare
Rebase and review update — Sep 3, 2026Rebased onto latest main ( Accuracy review: Main does not have Mergeable status: Confirmed MERGEABLE on GitHub after force-push. |
dce466f to
1aff9eb
Compare
Rebase pass — Sep 5, 2025Rebased onto current upstream main ( Rebase resultRebased cleanly onto current main. No conflicts. Still-needed assessmentChecked all 62 new main commits for overlap with this PR's topic. None obsoleted:
Verification
|
1aff9eb to
c7af019
Compare
Rebase pass — Sep 9, 2026Rebased onto current upstream main ( Rebase resultRebased cleanly onto current main. No conflicts. Still-needed assessmentChecked all 8 new main commits for overlap with this PR's topic. None obsoleted. #6189 (mesh-llm upgrade) changes Cargo.lock and mesh files — does not affect structured argument transport. Verification
|
c7af019 to
c67c750
Compare
Rebase pass — Sep 16, 2026Rebased onto current upstream main ( Rebase resultRebased cleanly onto current main. No conflicts. Still-needed assessmentNo main commit adds BUZZ_ACP_AGENT_ARGS_JSON; #7586 (point agents at buzz --help) and #7584 (command tree in --help) change help text, not argument transport. Verification
What was not doneNo local test suites were run. The rebase only repositions commits; conflict resolutions carry the same logical changes forward (CI will verify builds). |
BradGroux
left a comment
There was a problem hiding this comment.
Review refresh — 2026-10-02
Reviewed against current main 5a4b2d3d.
Verdict: Lossless JSON argument transport is still needed, but this branch requires correction.
Blocking finding: set_agent_args_env() does not remove an ambient BUZZ_ACP_AGENT_ARGS_JSON when the selected path uses legacy args. An inherited value can therefore override or conflict with newly selected legacy arguments. Explicitly clear the JSON variable on the legacy path and cover inherited-env precedence.
Rebase check: The branch conflicts in Cargo.lock and current Desktop env-var tests, with substantial ACP/runtime overlap. Current rustfmt also fails. Resolve, format, and run focused ACP/Desktop/Kubernetes env tests before publication.
No branch update was pushed in this pass because updating the PR head would trigger the repository's pull_request GitHub Actions workflows, which this workstream is not authorized to trigger.
BUZZ_ACP_AGENT_ARGS uses clap's value_delimiter = ',' so a value like "-m my-model --reasoning low" arrives as a single comma-delimited entry containing spaces. The old normalize_agent_args passed it through as one argv element, which the agent binary could not parse — every agent with multi-flag args timed out at 60s with a misleading error. Add a minimal shell_split() that handles single/double quotes, backslash escapes, and whitespace delimiting. When a comma-delimited entry contains a space, it is shell-split into separate argv elements. Entries without spaces pass through unchanged, preserving the existing comma-delimited behavior. No new dependency added (shlex is only a transitive build-dep via cc/cmake/aws-lc-sys). AGENTS.md says do not add production dependencies without approval, so the splitter is implemented inline. 7 new tests: shell_split unit tests (space-separated, single-arg, double-quoted, single-quoted, backslash escape, empty) plus normalize_agent_args integration tests. Closes block#6017 Co-authored-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: dm-builder <f01486f036641ccb52c11bb1e0ff2346ea89a8b4e3b49cd772249948f6fcbae6@digitalmeld.communities.buzz.xyz> Signed-off-by: Brad Groux <bradgroux@hotmail.com> Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com>
The hand-written shell_split had three issues identified in review: - Malformed shell syntax (unmatched quote, trailing backslash) was silently reinterpreted, removing the quote/backslash and launching with different argv than the operator supplied. - Only literal space was treated as a delimiter; tabs and other whitespace were not split. - The README still said arguments were comma-only. Replace shell_split with shlex::split (already a transitive build-dependency via cc). Malformed entries are now preserved as-is with a warning instead of being mutated. All whitespace is detected. README documents both comma- and space-separated forms and the quoting contract. Addresses themiguelamador's review feedback. Co-authored-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <bradgroux@hotmail.com> Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com>
…sport ravarora2's review identified that shell-splitting every BUZZ_ACP_AGENT_ARGS entry containing whitespace breaks backward compatibility with structured argument producers. Desktop and the Kubernetes controller already hold agent arguments as Vec<String>/lists, and an element containing whitespace may intentionally be one argv element. Once the list crosses the comma-delimited environment boundary, Buzz cannot reliably infer whether whitespace means one argument containing spaces or several shell words. The shlex approach also corrupted Windows paths (removing backslashes) and silently dropped text after # comment syntax. Replaced the heuristic with an explicit, lossless design: - BUZZ_ACP_AGENT_ARGS_JSON: new canonical JSON array of strings, parsed strictly with serde_json. No shell splitting, no character reinterpretation. Preserves spaces, backslashes, quotes, and empty arguments exactly. - BUZZ_ACP_AGENT_ARGS: legacy comma-delimited behavior, unchanged. Entries are preserved as-is without shell splitting. - Selection: neither set → default; JSON only → strict parse; legacy only → unchanged; both set → startup error; invalid JSON → startup error. - BUZZ_ACP_AGENT_ARGS_JSON added to Desktop reserved env keys (code-execution surface, same category as BUZZ_ACP_AGENT_ARGS). - shlex dependency removed from buzz-acp. Co-authored-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <bradgroux@hotmail.com> Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com>
ravarora2's review identified that the PR only added the JSON env var parsing in buzz-acp but did not update the actual producer paths — Desktop spawn, model probe, auth, and the Kubernetes backend still emit only the legacy comma-delimited BUZZ_ACP_AGENT_ARGS. This means normal Desktop/K8s configuration is not fixed; only manual env var users benefit. Wire all four producer paths to use set_agent_args_env, which selects the transport based on whether any argument contains a comma: - No commas: legacy comma-delimited BUZZ_ACP_AGENT_ARGS (backward compatible) - Any comma: JSON-serialized BUZZ_ACP_AGENT_ARGS_JSON + legacy set to default "acp" (which the harness treats as "not configured") Added agent_args_env() as a testable decision function returning (legacy_value, Option<json_value>), with 4 unit tests covering comma-safe, empty, single-comma, and multi-comma cases. Added a K8s test for the comma-in-arg JSON transport path and updated the existing args test to verify JSON is absent for comma-safe args. Updated buzz-acp README to document producer behavior. Co-authored-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <brad@digitalmeld.com> Co-authored-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <brad@digitalmeld.com> Signed-off-by: Brad Groux <bradgroux@hotmail.com> Signed-off-by: Brad Groux <3053586+BradGroux@users.noreply.github.com>
c67c750 to
52d0dd4
Compare
|
Still needed on current Verification: 1,070 ACP tests passed (three ignored), 158 Kubernetes backend tests, 51 native environment tests, strict root/native all-target Clippy, formatting, file-size, and diff checks passed. Native tests ran without system keyring integration; packaged GUI, Windows launch, and live Kubernetes deployment remain unverified. Head: |
Introduce
BUZZ_ACP_AGENT_ARGS_JSONas a structured argument transport while preserving the legacy comma-separated behavior. Reject conflicting non-default legacy and JSON input and validate JSON without heuristically shell-splitting existing structured arguments.Desktop and Kubernetes use JSON whenever commas, empty arguments, or edge whitespace would be lost by legacy normalization. Desktop explicitly removes inherited JSON input when selecting the legacy path, preventing stale configuration from overriding the intended arguments. Existing reserved environment controls remain intact; no production dependency is added.
Closes #6017.
Verification
Rebased onto
mainat2d289f720de0. All 1,070 ACP tests (three ignored), 158 Kubernetes backend tests, and 51 native environment tests pass. Strict root/native all-target Clippy, formatting, file-size, and diff checks pass. Native tests ran without system keyring integration. Packaged GUI, Windows process launch, and live Kubernetes deployment were not exercised.