fix: keep system messages at the start of chat history#6552
fix: keep system messages at the start of chat history#6552drbparadise wants to merge 3 commits into
Conversation
relda88
left a comment
There was a problem hiding this comment.
Good improvement overall. One edge case: empty string vs null aren't treated the same downstream — might need a normalization step.
|
Follow-up applied: empty system content is now treated as absent during normalization, so we no longer emit placeholder system messages or lose the fallback prompt on session load. Verified with: cargo test -p zeroclaw-providers flatten_system_messages -- --nocapture
cargo test -p zeroclaw-runtime load_interactive_session -- --nocapture
cargo test -p zeroclaw-runtime tool_loop_normalizes_non_leading_system_messages_before_provider_request -- --nocapture
cargo fmt --all -- --check
cargo clippy -p zeroclaw-providers -p zeroclaw-runtime --all-targets -- -D warnings |
Audacity88
left a comment
There was a problem hiding this comment.
Thanks for taking on #6551. I checked the current head 8f63ec6 against the linked issue, the prior review comment, the current diff in crates/zeroclaw-runtime/src/agent/history.rs, crates/zeroclaw-runtime/src/agent/loop_.rs, and crates/zeroclaw-providers/src/compatible.rs, plus the green CI state. I do not see anything blocking this.
✅ Resolved — Empty system content is treated as absent
The follow-up for the empty-string edge case addresses the earlier review concern. normalize_system_messages() now drops empty system fragments instead of manufacturing an empty leading system message, and load_interactive_session_history() restores the fallback prompt when normalization would otherwise leave the session without a leading system message. The new runtime/provider tests cover both the persisted-session fallback case and compatible-provider flattening.
🟢 What looks good — The provider-facing invariant is enforced at the right boundaries
The runtime now normalizes history immediately before provider dispatch, self-heals persisted interactive sessions on load, and merges loop-detection feedback into the leading system message instead of appending a later one. The OpenAI-compatible provider normalization now collapses non-leading system messages in flatten_system_messages(), and the added provider tests cover the strict-endpoint shape where merge_system_into_user = false, so strict endpoints should see at most one leading system role even when older history or runtime paths produced the rejected system,user,assistant,system,user shape.
The regression tests are pointed at the right failure mode. I also noted the local doctest failure described in the PR body; with GitHub's Test job green and the failure attributed to duplicate local rustdoc --default-theme=ayu flags, I do not see it as PR-blocking. The compatibility, security/privacy, and rollback notes match the behavior I see in the diff. Approving.
Summary
mastersystemmessages are merged into the first message before provider dispatch.systemmessage instead of appending a non-leadingsystemmessage.systemmessages on load.merge_system_into_user = falsestill sends at most one leadingsystemmessage.Validation Evidence (required)
Local validation is the signal CI cannot replace. Run the full battery and paste literal output (tails, failures, warnings — not "all passed").
cargo fmt --all -- --check cargo clippy --all-targets -- -D warnings cargo testDocs-only changes: replace with markdown lint + link-integrity (
scripts/ci/docs_quality_gate.sh). Bootstrap scripts: addbash -n install.sh.CI Required Gate,Test,Lint,Security,Check (all features),Check (no default features),Check (32-bit), Linux/macOS/Windows builds, andBenchmarks Compile.system,user,assistant,system,user. After the fix, the runtime provider request and session load paths contain only one leadingsystemmessage and preserve both system contents. A self-review of commitff7da168found no blocking issues; focused runtime/provider regression tests andgit diff --checkwere rerun after review.cargo testwas run, but the local doctest phase failed because rustdoc received duplicate--default-theme=ayuflags from the current local configuration.Security & Privacy Impact (required)
Yes/No for each. Answer any
Yeswith a 1–2 sentence explanation.No)No)No)No)Yes, describe the risk and mitigation:N/ACompatibility (required)
Yes)No)NoorYesto either: exact upgrade steps for existing users:N/ARollback (required for
risk: mediumandrisk: high)Low-risk PRs:
git revert <sha>is the plan unless otherwise noted.Medium/high-risk PRs must fill:
git revert ff7da168NoneSystem message must be at the beginning.if non-leading system messages reappear.Supersede Attribution (required only when
Supersedes #is used)#<pr> by @<author>, one per line):N/AN/ACo-authored-bytrailers added in commit messages for incorporated contributors? (No)No, why (inspiration-only, no direct code/design carry-over):No superseded PR or external contributor code was incorporated.Labels live in the GitHub label UI, not in the body. Set
risk:*,size:*, and scope labels via the sidebar. Auto-label corrections: addrisk: manualand the intended label.Commit trailers capture AI-assisted collaboration (
Co-Authored-By: Claude ...) — no separate section needed.Privacy contract (
docs/book/src/contributing/privacy.md) is a merge gate. Never commit real identities, secrets, personal emails, or PII in diff, tests, fixtures, or docs.