Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
openclaw-barnacle
Bot
added
cli
92ea93d to
43ea027
Compare
openclaw-barnacle
Bot
removed
the
triage: needs-pr-context
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: found issues before merge. Reviewed August 25, 2026, 5:52 PM ET / 21:52 UTC. ClawSweeper reviewWhat this changesThe PR rejects inherited Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readinessKeep open: the guard repairs the silent no-op and has credible CLI proof, but it turns an inherited documented option into an immediate error without a maintainer compatibility decision; the remaining documentation wording for Priority: P2 Review scores
Verification
Live VerificationCommand: Result: FAIL (failed) — step 1 Assertions:
How this fits togetherThe models CLI accepts a parent agent selector and routes subcommands either to an agent-specific operation or to global model defaults. Aliases and scan lack an agent-specific downstream path, so the new guard stops a misleading inherited option before command execution. flowchart LR
A[Operator command] --> B[Models CLI parser]
B --> C{Subcommand scope}
C -->|Agent-aware| D[Resolve configured agent]
C -->|Global-only| E[Reject inherited agent option]
D --> F[Agent model operation]
E --> G[Formatted operator error]
C --> H[Global defaults or catalog scan]
Decision needed
Why: The option was accepted by the parent parser and existing scripts can carry it, while the current reference already says these commands use global behavior; repository policy treats documented CLI flags and upgrade behavior as compatibility-sensitive. Before merge
Findings
Agent review detailsSecurityNone. PR surfaceSource +14, Tests +82, Docs +6. Total +102 across 5 files. View PR surface stats
Review metrics
Stored data modelPersistent data-model change detected: Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Describe scan as having no agent-scoped mode and only global configuration effects, then apply the maintainer-selected compatibility policy before landing the rejection. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: current main registers aliases and scan without consuming the parent option, while the branch’s real CLI transcript shows the proposed after-fix error path. Is this the best way to solve the issue? Unclear: rejecting the inert option is the right owner-boundary repair, but the immediate compatibility break versus a warning-release transition requires maintainer direction. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e1e2818a6ab0. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (9 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
proof: sufficient
d7fd7c7 to
c97c6f6
Compare
clawsweeper
Bot
added
rating: 🦐 gold shrimp
c97c6f6 to
5904b84
Compare
|
The P2 finding is correct, and it was worse than a latent risk — the assertion was already vacuous, not just fragile. I checked before changing anything.
Green, with the leaf provably never invoked. The earlier row's call satisfied it. Fixed by giving Same mutation after the fix: One failure, on the row whose leaf I blocked. Guard-removal mutation still behaves — deleting all four Clean run on the pushed head: No production code changed in this push — the diff is On @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
removed
the
rating: 🦐 gold shrimp
clawsweeper
Bot
added
rating: 🐚 platinum hermit
5904b84 to
16d7f5c
Compare
…ng it
`openclaw models aliases {list,add,remove}` and `openclaw models scan` accepted
`--agent <id>` from the parent `models` command and did nothing with it — no
scoping, and no validation, so a nonexistent id passed silently:
openclaw models --agent totally-bogus-agent-id status
# Unknown agent id "totally-bogus-agent-id".
openclaw models --agent totally-bogus-agent-id aliases list
# prints the normal alias list, no error
Every other `--agent`-aware `models` subcommand validates the id, so the flag
looks like it scoped the command when it did nothing at all (openclaw#126597).
Rejecting is the right shape here rather than wiring `--agent` through, because
there is no agent-scoped path to wire it to: `modelsAliasesListCommand` and its
siblings take `(opts, runtime)` and read `cfg.agents?.defaults?.models` with no
agent parameter anywhere in the chain. `models set` / `set-image` already guard
the same way for the same reason.
Two things had to change with the guard rather than around it:
- It is no longer write-only. `aliases list` and `scan --no-probe` are reads, so
`rejectAgentScopedModelWrite` is renamed `rejectAgentScopedModelCommand`, and
its message says the command "only affects the global model defaults under
agents.defaults" instead of "only updates" them.
- `commandName` is now a `GlobalOnlyModelsCommand` union, so a new global-only
subcommand has to name itself rather than pass a free-form string.
Tests extend the existing parent-`--agent` rejection table with the four
subcommands, assert the message names the one that was rejected, and pin that
all four still run normally without `--agent` — so the guard cannot quietly grow
into a blanket refusal.
Verified on Windows, Node 24:
- src/cli/models-cli.test.ts + models-cli.lazy.test.ts 48 passed (was 38)
- test/scripts/write-cli-startup-metadata.test.ts 16 passed, 5 skipped
- tsc --noEmit 0 errors
- oxfmt --check clean
Mutation-checked: removing the four guard calls fails exactly the six new
rejection cases, and leaves the "still runs without --agent" cases green.
Closes openclaw#126597
…cument it ClawSweeper review on openclaw#126761. Two findings, both mine to fix. The guards threw before `runModelsCommand`, because I copied the shape `models set` uses. The prior alias and scan actions ran *inside* `withModelsRuntime`, so that move made the error path worse for exactly the commands this PR touches: `runCommandWithRuntime` catches, renders through `formatCliOperatorError` and exits 1, and a throw outside it reaches the CLI parser's unexpected-error path instead. An ordinary unsupported-option mistake was being presented as a runtime failure. The guards are back inside `withModelsRuntime`, which also restores the original action shape, so the diff is now smaller than it was. The tests had to follow, and that is the evidence the finding was right: the `rejects.toThrow` assertions went red once the error stopped propagating. They now assert the formatted path instead — the message reaching `console.error`, naming the subcommand it rejected, and `exit(1)`. Two details cost a round each and are worth recording: `defaultRuntime.error` routes through `console.error` rather than a direct `process.stderr` write, and `defaultRuntime.exit` throws an "unreachable" sentinel once `process.exit` is mocked (`src/runtime.ts:121`). `docs/cli/models.md` still said `scan` and `aliases` "always use the configured default agent". They are listed with `set`/`set-image` as rejecting `--agent` now, and an upgrade note states the compatibility change: scripts passing `--agent` to these subcommands will fail, and the behaviour they were getting was always the global default, so dropping the flag does not change what the command does. Verified on Windows, Node 24: - src/cli/models-cli.test.ts + models-cli.lazy.test.ts 46 passed (38 before) - tsc --noEmit 0 errors - oxfmt --check clean - real CLI after `pnpm build`: $ node openclaw.mjs models --agent totally-bogus-agent-id aliases list openclaw models aliases list does not support --agent; it only affects the global model defaults under agents.defaults. ... exit=1 $ node openclaw.mjs models aliases list Aliases (0): - none exit=0 Mutation-checked: removing the four guard calls fails exactly the four new operator-error cases.
`pnpm format:check` covers docs as well as source, and I had only run oxfmt on the TypeScript files. `check-docs` failed on `docs/cli/models.md`; the reflow is whitespace only. `check-lint` in the same run was cancelled rather than failed once check-docs went red.
The four "still runs without --agent" rows all asserted the same shared `noopAsync` spy, and nothing cleared it between rows. Once the first row called it the assertion was satisfied for every later row, so a regression that blocked a later leaf still passed. Verified: blocking `models aliases remove` from ever reaching its leaf command left all 44 tests green. Each subcommand now owns a spy, cleared in `beforeEach`, and each row asserts its own leaf ran exactly once -- the same mutation now fails, and only on that row. Both tables are driven from one list, so a rejected case and an unscoped case can no longer describe different commands.
16d7f5c to
ef85a4e
Compare
AGENTS.md requires American spelling, and docs/ already uses `behavior` in 331 files against `behaviour` in 2.
|
P3 fixed in CI is green on that head: 142 passed, 0 failed. One thing worth flagging, since it cost me some time and may be useful to you. Before this push,
It is a flake, and the third run settles it: the only change between the red head and the green one is the single word above, in a Markdown file, which cannot affect that test. Nothing in this PR touches The shape of the failure suggests an ordering-dependent leak: something in that shard replaces Still open from your side is the compatibility question in the review — immediate rejection as it stands, or a deprecation warning for a release first. I have no preference and can push either. |
clawsweeper
Bot
added
clawsweeper-recovery-stuck
rating: 🦐 gold shrimp
|
Thanks @ntdatt812 for the reproduction, implementation, and careful test isolation work. Closing this as superseded by #126864, merged as 93573e8f. Its merge is an ancestor of the main checkout verified here, bcb5403d. The linked issue #126597 is already closed. I independently checked the complete patch, current command registration, shared option resolver, aliases/scan owners, error boundary, Commander 15 source, and neighboring command semantics. Current main rejects inherited Fresh proof on the built main CLI, Node 26.7.0, with isolated HOME/state/config:
There is a presentation difference: this PR catches the guard inside the local command wrapper, whereas main's top-level CLI boundary prints the reason with generic startup/help guidance. The real CLI still exits 1 and performs no mutation; there is no unhandled rejection or remaining silent-ignore defect here. We should retain main's canonical guard instead of applying the older duplicate implementation. This is proof of fixed behavior on main, not a claim that a stable release contains it. Per-agent fallbacks/image-fallbacks are a distinct surface tracked by #106346 / #106414 and are not being closed by this decision. |
Closes #126597.
What Problem This Solves
openclaw models aliases {list,add,remove}andopenclaw models scanaccept--agent <id>from the parentmodelscommand and do nothing with it — no scoping, and no validation, so a nonexistent id passes silently:Every other
--agent-awaremodelssubcommand validates the id, so the flag looks like it scoped the command when it did nothing at all. An operator scoping a command to one agent in a multi-agent fleet gets no feedback that the flag was inert, and a typo'd agent id is never caught.Why reject rather than wire it through
The issue offered both options, so I checked the downstream before choosing. There is no agent-scoped path to wire
--agentto —modelsAliasesListCommandand its siblings take(opts, runtime)and readcfg.agents?.defaults?.models, with no agent parameter anywhere in the chain:models set/set-imagealready guard the same way, for the same reason.Changes
rejectAgentScopedModelCommandinsidewithModelsRuntime, so the rejection renders throughformatCliOperatorErrorand exits 1 like any other operator error. (My first revision threw before the wrapper, copying the shapemodels setuses; ClawSweeper caught that it made the error path worse for exactly these commands, and it was right.)rejectAgentScopedModelWrite→rejectAgentScopedModelCommand: it now covers reads (aliases list,scan --no-probe), so the write-shaped name and its "only updates global model defaults" wording were both inaccurate. The message says "only affects the global model defaults underagents.defaults".commandNameis aGlobalOnlyModelsCommandunion, so a future global-only subcommand names itself instead of passing a free-form string.docs/cli/models.mdlistedscanandaliasesas commands that "always use the configured default agent". They are now listed withset/set-imageas rejecting--agent, plus an upgrade note for the compatibility change.Evidence
Real CLI, after
pnpm build, on this branch (Windows, Node 24):Focused tests and static checks:
Mutation-checked. Removing the four guard calls, keeping the tests:
Exactly the four new cases, and the "still runs without
--agent" cases stay green.Whole
src/clisuite, measured on both sides. It has a pre-existing failure surface on Windows, so I ran it against the unpatched tree (git checkout HEAD~1 -- src/cli/) rather than assume. The failing files are identical with and without the patch — 14 files includingprofile(9),update-cli(40),plugins-cli.install(4) andgateway-backed-exit.process(8).models-clihas zero failures in both runs, and none of those files is touched here.One note on the build used for the CLI proof:
pnpm buildcompleted the compile but its last step,write-cli-startup-metadata, failed rendering help for the browser plugin (dist/extensions/browser/index.js: Cannot read properties of undefined (reading 'join')). That is unrelated to this change and I did not verify whether it also fails on a clean tree;dist/entry.jswas produced and the commands above ran against it.Tests added
The four subcommands assert the formatted operator-error path: the message reaching
console.error, naming the subcommand it rejected (so a copy-paste that guardsscanwhile reportingsetfails), andexit(1). A second block pins that all four still run normally without--agent, so the guard cannot quietly grow into a blanket refusal.Two harness details cost a round each and are worth stating for the next person:
defaultRuntime.errorroutes throughconsole.errorrather than a directprocess.stderrwrite, anddefaultRuntime.exitthrows an"unreachable"sentinel onceprocess.exitis mocked (src/runtime.ts:121).@clawsweeper re-review