fix(nofxi): review fixes on top of #1495 — chat trade unblock + management fast path + claw402 + UX - #1498
Open
shinchan-zhai wants to merge 24 commits into
Open
fix(nofxi): review fixes on top of #1495 — chat trade unblock + management fast path + claw402 + UX#1498shinchan-zhai wants to merge 24 commits into
shinchan-zhai wants to merge 24 commits into
Conversation
This reverts commit 8d8a0cc.
This reverts commit fe0dbce.
…es"" This reverts commit 3619f82.
This reverts commit 03a3079.
…4 endpoints, x402-aware CallWithRequest
- stripMaxTokens reapplied (regression vs dev — Kimi K2.5 / DeepSeek R1/V4
let reasoning tokens consume the entire budget, leaving delta.content empty)
- dropAssistantHistory for /v4-flash, /v4-pro, /chat/reasoner: DeepSeek
thinking models reject any assistant turn lacking reasoning_content
("The reasoning_content in the thinking mode must be passed back to the API")
- Add deepseek-v4-flash and deepseek-v4-pro endpoints (PR NoFxAiOS#1488 on dev,
missing on feat/nofxi)
- New Claw402Client.CallWithRequest override: base mcp.Client.CallWithRequest
issues a bare HTTP request and treats 402 as terminal, bypassing the x402
payment loop. The agent has 20+ CallWithRequest sites (planner, brain,
workflow, llm_skill_router, memory, etc.) — without this override every
one of them fails with "API returned error (status 402)"
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three independent gates were closing this path:
1. parseTradeCommand existed but was never called — explicit trade verbs
like "做多 BTC 0.01" went through the LLM router and got reinterpreted
as analysis or trader-creation requests. New handleTradeIntent provides
a deterministic fast path: parse → resolve trader → validate → enqueue
pending trade → return confirmation prompt. Bypasses the LLM router so
unambiguous orders cannot be re-routed to "let me first analyse the
market" flows.
2. parseTradeCommand silently dropped unparseable quantity / leverage,
producing TradeAction{Quantity: 0}. For close_long/close_short that
forwards to underlyingTrader.CloseLong(symbol, 0, ...), which most
exchanges interpret as "close entire position" — different from what
the user typed. Now reject the parse instead.
3. CanExecuteTrade was hardcoded to (user_id == "admin"). On a single-user
local deployment (CLAUDE.md) every authenticated user is the operator
of their own nofx-server, so the literal-string admin gate locked out
non-admin users from ever triggering trades via chat. Allow all
authenticated users; risk caps and the explicit confirmation prompt
still apply.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…fixes + prompt strengthening
Routing
- New management_fastpath.go: regex matches 6 verbs (create / list / delete /
stop / start / configure / switch) × 4 entities (trader / exchange / model /
strategy) → dispatches directly through driveActiveSession, bypassing the
LLM router. The LLM router consistently picks direct_answer for these
unambiguous commands ("创建一个新交易员" → "好的我来帮你..."conversational
preface, action never executes), and prompt strengthening alone did not
shift this — measured: hand-tuned rules + 14 worked few-shot examples kept
pass rate at ~21/36; deterministic fast path moved it to 36/36.
- Diagnosis cues ("怎么没下单", "连不上", "失败") plus an entity keyword route
to *_diagnosis skills.
- agent.go wires both handleTradeIntent and handleManagementIntent into
stream + non-stream entry points, in front of the LLM router.
LLM client
- Use mcp.NewAIClientByProvider so claw402 routes through Claw402Client
(with x402 EIP-712 signing + per-model endpoint mapping) instead of the
bare OpenAI-compatible client. AllowTradeExecution defaults to true on
single-user local deployments.
Streaming UX
- web.go: 120s → 180s ctx timeout (multi-step planner with v4-flash
routinely needs 100s+); on context cancellation OR empty resp, emit
event:error + event:done so the React layer no longer falls back to
the misleading "No response" string.
- planner_runtime.go: planStepTypeReason now calls emitStreamText for the
reasoning text. Without this, reason steps emit only step_complete
events — the analysis text stays in OutputSummary and observation log
but never reaches SSE deltas, so the UI shows step labels with no body.
Prompts
- llm_skill_router.go: distinguish "give me a trading decision /
recommendation" (planned_agent) from "create/configure trader entity"
(trader_management); add 14 worked few-shot examples covering all major
zh management/diagnosis intents. Largely ineffective at runtime — kept
as belt-and-suspenders alongside the deterministic fast path.
- central_brain.go: add ACTION VERBS REQUIRE NEW_TASK rule with concrete
zh examples; forbid EXPLAIN_KNOWLEDGE as a soft "好的我来帮你..." preface.
- model_provider_catalog.go: add deepseek-v4-flash / -pro / -reasoner +
kimi-k2.5 to claw402 RecommendedModelHints; remove claude-opus
(claw402 routes it to /api/v1/ai/anthropic/messages/opus which speaks
Anthropic Messages API — the planner JSON contract doesn't parse this
format, every turn fails with "invalid next step decision json").
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- main.go: stop the NOFXi agent BEFORE traderManager.StopAll. Chat-driven goroutines call into TraderManager (start/stop/open trade), so tearing the manager down first risks nil-deref or write-to-closed-channel from in-flight requests. Also remove the misleading "✅ NOFXi agent stopped" log that fired before the deferred Stop() actually ran. - api/agent_preferences.go: cap POST /api/agent/preferences body to 8 KB via http.MaxBytesReader BEFORE bind. The post-bind 500-rune check on Text was too late if the raw body was already huge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
shinchan-zhai
requested review from
SkywalkerJi,
hzb1115 and
tangmengqiu
as code owners
May 5, 2026 05:26
|
|
1 similar comment
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Stacked on top of #1495 (NOFXi agent chat workflow). The 4 commits added by this PR are the review-fix follow-ups; everything in #1495 itself is unchanged. Please merge #1495 first or merge this together with #1495.
Summary of the 4 commits
fix(claw402)— re-applystripMaxTokens(regression vs dev — Kimi K2.5 / DeepSeek R1/V4 thinking models drop content), drop assistant history on thinking endpoints (reasoning_contentprotocol), add deepseek-v4-flash/v4-pro endpoints (PR feat(payment): add new DeepSeek V4 models to claw402 endpoints and pricing #1488 missing), and overrideClaw402Client.CallWithRequestso all 20+ agent call sites get x402 payment retry instead of failing on bare 402.feat(agent): unblock chat-driven trade execution— three independent gates were closing this path:parseTradeCommandwas defined but never called (explicit "做多 BTC 0.01" went through LLM router and got reinterpreted),AllowTradeExecutiondefaulted false with no code path setting it true, andCanExecuteTradewas hardcoded to admin-only (CLAUDE.md specifies single-user local). All three fixed; risk caps and confirmation prompt still apply.feat(agent): deterministic management fast path— LLM router consistently picksdirect_answerfor management commands ("创建一个新交易员" → conversational preface, action never executes). Prompt strengthening + 14 worked few-shot examples kept pass rate at ~21/36; newagent/management_fastpath.go(regex 6 verbs × 4 entities, dispatches todriveActiveSessiondirectly) +planner_runtime.goreason stepemitStreamTextfix + SSE timeout protection moved it to 36/36 on the self-test matrix.chore(review)— shutdown order (agent before traderManager), agent_preferences body cap.Test plan
Notes for reviewer
AllowTradeExecution: false → true. CLAUDE.md says single-user local deployment; the user owns their own nofx-server. Each trade still goes through explicit confirmation + risk caps. If you'd prefer an env opt-out, happy to add.agent/management_fastpath.go248 lines new code — regex/keyword dispatcher. Intentional duplicate of skill JSONintentsuntil follow-up unifies them./tmp/nofxi_test_*locally; can be promoted toagent/agent_e2e_test.goin a follow-up PR.🤖 Generated with Claude Code